Skip to content

libxcrypt: Build only with strong hashes#220557

Merged
mweinelt merged 4 commits intoNixOS:stagingfrom
mweinelt:libxcrypt-strong
Mar 15, 2023
Merged

libxcrypt: Build only with strong hashes#220557
mweinelt merged 4 commits intoNixOS:stagingfrom
mweinelt:libxcrypt-strong

Conversation

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Mar 10, 2023

Effectively removes support for the following hashing algorithms as announced in the NixOS 22.11 release notes:

  • bcrypt_x ($2x$)
  • sha256crypt ($5$)
  • sha1crypt ($sha1$)
  • sunmd5 ($md5$)
  • md5crypt ($1$)
  • nt ($3$)
  • bdiscrypt (_)
  • bigcrypt (:)
  • descrypt (:)

Passthru tests (login, shadow) built on aarch64-linux and x86_64-linux.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin (fails early during perl build)
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@mweinelt mweinelt added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Mar 10, 2023
@mweinelt mweinelt added this to the 23.05 milestone Mar 10, 2023
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Mar 10, 2023
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 10, 2023

Do we need to change https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/config/users-groups.nix#L705-L734 ?

@mweinelt
Copy link
Member Author

mweinelt commented Mar 10, 2023 •
edited
Loading

It does not validate the used scheme value so far, and I don't have the energy to make it do that.

@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Mar 10, 2023
@github-actions github-actions bot added the 8.has: module (update) This PR changes an existing module in `nixos/` label Mar 11, 2023
@mweinelt
Copy link
Member Author

mweinelt commented Mar 11, 2023

Updated the validation for hashedPassword. Was quite a bit simpler than expected.

cc @rnhmjoj @SuperSandro2000

@mweinelt mweinelt force-pushed the libxcrypt-strong branch 3 times, most recently from 5ea8726 to dcbe1a8 Compare March 11, 2023 21:46
mweinelt
mweinelt commented Mar 11, 2023
View reviewed changes
nixos/modules/config/users-groups.nix Outdated
Comment on lines 719 to 720
Copy link
Member Author

@mweinelt mweinelt Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

#name          h_prefix  nrbytes  flags
yescrypt       $y$       16       STRONG,DEFAULT,ALT,DEBIAN,FEDORA
gost_yescrypt  $gy$      16       STRONG,ALT
scrypt         $7$       16       STRONG
bcrypt         $2b$      16       STRONG,DEFAULT,ALT,FREEBSD,NETBSD,OPENBSD,OWL,SOLARIS,SUSE
bcrypt_y       $2y$      16       STRONG,ALT,OWL,SUSE
bcrypt_a       $2a$      16       STRONG,ALT,FREEBSD,NETBSD,OPENBSD,OWL,SOLARIS,SUSE
sha512crypt    $6$       15       STRONG,DEFAULT,GLIBC,FREEBSD,SOLARIS

https://github.com/besser82/libxcrypt/blob/v4.4.33/lib/hashes.conf#L41

@mweinelt
Copy link
Member Author

mweinelt commented Mar 11, 2023

Tested with a sha256-crypt hash, and it does complain alright. Migated to yescrypt and the warning was gone.

trace: warning: The password hash of user "hexa" may be invalid. You must set a
valid hash or the user will be locked out of their account. Please
check the value of option `users.users."hexa".hashedPassword`.

@mweinelt mweinelt requested review from rnhmjoj and winterqt March 11, 2023 21:59
@mweinelt mweinelt marked this pull request as draft March 12, 2023 14:23
@mweinelt mweinelt force-pushed the libxcrypt-strong branch 3 times, most recently from 74ac1c1 to f22ccb4 Compare March 12, 2023 15:01
@mweinelt mweinelt marked this pull request as ready for review March 12, 2023 15:01
@mweinelt mweinelt requested a review from roberth as a code owner March 12, 2023 17:00
@mweinelt mweinelt force-pushed the libxcrypt-strong branch 2 times, most recently from 2b15b32 to aecd1b0 Compare March 12, 2023 17:07
@nixos-discourse
Copy link

nixos-discourse commented Mar 12, 2023

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/21

rnhmjoj
rnhmjoj reviewed Mar 12, 2023
View reviewed changes
Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good but I can't properly test this right now.
Anyway, glad to see we're finally using modern hashing schemes.

SuperSandro2000
SuperSandro2000 reviewed Mar 12, 2023
View reviewed changes
@mweinelt mweinelt force-pushed the libxcrypt-strong branch 2 times, most recently from a74ed2a to c49ed58 Compare March 12, 2023 22:52
mweinelt added 4 commits March 13, 2023 07:54
@mweinelt
libxcrypt: Build only with strong hashes
4e300e0 
Effectively removes support for the following hashing algorithms
as announced in the NixOS 22.11 release notes:

- bcrypt_x ($2x$)
- sha256crypt ($5$)
- sha1crypt ($sha1$)
- sunmd5 ($md5$)
- md5crypt ($1$)
- nt ($3$)
- bdiscrypt (_)
- bigcrypt (:)
- descrypt (:)

And exposes the crypt scheme ids for enabled algorithms, so they can be
reused for validation in the users-groups module.
@mweinelt
nixos/users-groups: Update password scheme validation
0d7cd66 
Updates the warnings message for statefully set up passwords, now that
weak algorithms have been removed from our libxcrypt package.

Additionall we now add proper validation for hashing schemes used in
`hashedPassword`.

Neither will prevent a rebuiild, but instead issue a warning, that this
requires immediate remediation, or else users will be unable to login.

Reuses the crypt scheme ids as provided by the libxcrypt package.
@mweinelt
pam: Make libxcrypt a non-optional dependency
909f394 
Our PAM configuration now defaults to yescrypt, which requires
libxcrypt.
@mweinelt
treewide: Make yescrypt the default algorithm for pam_unix.so
4472cf4 
This ensures `passwd` will default to yescrypt for newly generated
passwords.
fpletz
fpletz approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not tested yet but the diff looks fine! 👍

@mweinelt mweinelt merged commit 578fb7f into NixOS:staging Mar 15, 2023
@mweinelt mweinelt deleted the libxcrypt-strong branch March 15, 2023 16:43
@vcunat vcunat mentioned this pull request Mar 19, 2023
Merged
12 tasks
@vcunat
Copy link
Member

vcunat commented Mar 25, 2023

libxcrypt-legacy supporting all hashes:
#223034

@vcunat
Copy link
Member

vcunat commented Mar 26, 2023 •
edited
Loading

Generally I can imagine options around updating, fixing or disabling tests and maybe even using libxcrypt-legacy.

This change is part of the current staging-next, PR #221461

@orivej
Copy link
Contributor

orivej commented Mar 26, 2023

I see that mailutils uses weak auth during tests to interact with its mock server, i.e. it sends plaintext USER name, PASS guessme before sending other testing queries. Maybe we should ask its developers on the best way forward (https://savannah.gnu.org/support/?group=mailutils) and meanwhile use libxcrypt-legacy or disable tests.

This was referenced Mar 26, 2023
Merged
Merged
@stigtsp
Copy link
Member

stigtsp commented Mar 27, 2023

Nice to avoid these legacy hashes btw. 👍

@obadz
Copy link
Contributor

obadz commented Mar 30, 2023

I continue to think this should be configurable, see #208603

@orivej orivej mentioned this pull request Apr 1, 2023
Closed
This was referenced Apr 3, 2023
Merged
Merged
@aither64 aither64 mentioned this pull request May 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rnhmjoj rnhmjoj rnhmjoj left review comments

@fpletz fpletz fpletz approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@winterqt winterqt Awaiting requested review from winterqt

@roberth roberth Awaiting requested review from roberth

Assignees

No one assigned

Labels

1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

23.05

Development

Successfully merging this pull request may close these issues.

10 participants

@mweinelt @SuperSandro2000 @nixos-discourse @vcunat @orivej @stigtsp @obadz @fpletz @rnhmjoj @winterqt