Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libxcrypt: Build only with strong hashes #220557

Merged
merged 4 commits into from
Mar 15, 2023
Merged

libxcrypt: Build only with strong hashes #220557

merged 4 commits into from
Mar 15, 2023

Conversation

mweinelt
Copy link
Member

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 8.has: documentation This PR adds or changes documentation labels Mar 10, 2023
@SuperSandro2000
Copy link
Member

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 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 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

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
# must match the labels of hashes enabled in libxcrypt
id = "(y|gy|7|2b|2y|2a|6)";
Copy link
Member Author

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

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

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.

@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

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@fpletz fpletz fpletz 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 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+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 11.by: package-maintainer This PR was created by the maintainer of 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