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.

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

Add yescrypt support #303

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Add yescrypt support #303

merged 1 commit into from
Mar 29, 2021

Conversation

breard-r
Copy link
Contributor

This PR should fix #300

Although I have not done intensive testing, I have been able to set yescrypt-hashed passwords using chpasswd -c YESCRYPT.

Copy link

@solardiz solardiz left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this.

Not a real review, but I skimmed and made some comments. (I'm not approving nor requesting changes as I'm not a maintainer.)

As I tweeted in https://twitter.com/solardiz/status/1343593126410268673, longer-term or additionally (when recent libxcrypt is available at compile-time) we could switch shadow to our crypt_gensalt*() APIs:

I guess you're trying to mimic libxcrypt's translation of a single cost parameter to several of yescrypt's parameters. This is just right for your use case. Maybe you could have invoked libxcrypt's crypt_gensalt*() to have it do this for you (using yescrypt APIs under the hood).

Historically, we introduced those crypt_gensalt*() APIs in an Owl glibc patch in 2001, and indeed our shadow suite patches on Owl use them. ALT Linux, too, now including for yescrypt. As libxcrypt finally adopted our APIs, maybe shadow should adopt our patches to use them, too?

This removes all of the per-hash-type complexity from shadow, and eliminates the need to update it to support future hash types (adding them to libxcrypt is enough to get them supported). https://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/shadow-utils/shadow-4.0.4.1-owl-crypt_gensalt.diff
https://packages.altlinux.org/en/sisyphus/srpms/shadow http://git.altlinux.org/people/sem/packages/shadow.git

That said, perhaps I should also document the kind of translation you came up with - from libxcrypt's yescrypt cost parameter directly to prefix strings. This avoids a dependency on APIs beyond crypt(), which might be a requirement in some other projects.

libmisc/salt.c Outdated Show resolved Hide resolved
libmisc/salt.c Outdated
}
cost_prefix[2] = cost >= 3 ? 'T' : '5';
cost_prefix[3] = '$';
cost_prefix[4] = 0;

Choose a reason for hiding this comment

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

Oh, this is a rather compact way to mimic (I guess) libxcrypt's translation of a single cost parameter to yescrypt's multiple ones, which it'd then encode with yescrypt_encode_params_r(). I didn't verify it, but I assume the resulting strings were checked to match what libxcrypt produces? If so, that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. At first I tried to actually implement a proper parameter generation. Unfortunately, I found no documentation and very few comments in the code. I did not spent too much time on it, but I think it involves bit masks and non-standard base64 encoding. Instead of reading some foreign code for days and re-implementing it, I generated the 11 possible values using libxcrypt's crypt_gensalt.

Choose a reason for hiding this comment

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

I accept your criticism here: I should write a specification of the encoding, document the rationale behind the choices, and add some source code comments. The compact encoding of numeric parameters is actually specific to yescrypt, and needs comments.

salt[0] = '\0';

seedRNG ();
strcat (salt, l64a (random()));

Choose a reason for hiding this comment

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

This leading strcat() call looks unneeded as the loop contains the same (will just do one more iteration of this line is omitted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy/pasted this code from gensalt_bcrypt(), which seems to be an adaptation from gensalt(). Since I do not fully understand how this code works, I didn't touched it except for the salt's length.

Copy link

@solardiz solardiz Dec 28, 2020

Choose a reason for hiding this comment

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

Makes sense to reuse the same (non-ideal) code for this PR - in fact, this is why I didn't complain about the uses of random(). I should have realized the redundant strcat() probably came from there as well.

I wouldn't increase salt size. Sure 144 bits gets encoded more optimally, but it's excessive and besides with random() we only have at most 32 bits for real:

static void seedRNG (void)

The Owl patch I referred to above also switched to use of /dev/urandom, but of course this shouldn't be in this same PR.

BTW, it looked weird to me at first that the same code was used to generate salts for bcrypt, since bcrypt uses the crypt base64 alphabet in different order than all other crypts. This probably ended up relying on the bcrypt implementation truncating the unsupported bit values for the last character, but that's OK'ish since it's truncation from 132 to 128 no matter in which order the alphabet is.

Choose a reason for hiding this comment

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

I wouldn't increase salt size. Sure 144 bits gets encoded more optimally

Oh, maybe the reason you did that is the salt strings truncated at 22 were not being accepted, because they contained extra 4 bits in the last char, and you didn't feel like implementing encoding of exactly 128 bits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really that I don't "feel like" implementing it. My reasoning was that 144 bits does not harm in any way (libxcrypt's man 5 crypt states that the salt can be up to 512 bits) and truncating to 128 bits, while not adding value, would require me to take time study the undocumented non-standard base64 encoding with the risk of introducing bugs in my implementation. Less code, less time spent, less risks and no loss in terms of security, all the odds were in favor of a 144 bits salt.

Obviously, if you recommend to use 128 bits anyway (I'm not sure what you imply by saying it's excessive), I can do that.

Choose a reason for hiding this comment

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

the undocumented non-standard base64 encoding

For salt and hash, yescrypt uses the exact same standard crypt base64 encoding that traditional Unix crypt(3) did since the 1970s. It's also the encoding that md5crypt, sha256crypt, and sha512crypt use. (bcrypt is an outlier here.)

However, the yescrypt implementation is strict in that it only accepts salts that are exactly the same as what would be in the resulting salt+hash string. This is different from bcrypt, which silently turns the last character of salt into how it actually sees it in binary, re-encoding it in the returned salt+hash string. This is also different from md5crypt, sha256crypt, and sha512crypt, which use the salt as a mostly opaque ASCII string, preserving the full 132 bits. So it's probably because of these differences that the same code producing 132-bit salts didn't work for yescrypt here, while it did for others. Not the different base64 encoding.

Sure 144 bits does not harm, it's just different. By "excessive", I was referring to 144 vs. 128, not to 128.

libmisc/salt.c Outdated Show resolved Hide resolved
libmisc/salt.c Outdated Show resolved Hide resolved
etc/login.defs Outdated Show resolved Hide resolved
Copy link

@solardiz solardiz left a comment

Choose a reason for hiding this comment

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

There are more instances of 8 to be changed to 5 now.

etc/login.defs Outdated Show resolved Hide resolved
src/chgpasswd.c Outdated Show resolved Hide resolved
src/chpasswd.c Outdated Show resolved Hide resolved
src/newusers.c Outdated Show resolved Hide resolved
@solardiz
Copy link

@hallyn I think this is ready for your review, and should get in.

Longer-term, I'd like support for recent libxcrypt's crypt_gensalt*() to be added, which is more generic, simpler, and will use /dev/urandom for salts - but that's not a reason not to merge this PR.

@hallyn
Copy link
Member

hallyn commented Feb 1, 2021

@hallyn I think this is ready for your review, and should get in.

Longer-term, I'd like support for recent libxcrypt's crypt_gensalt*() to be added, which is more generic, simpler, and will use /dev/urandom for salts - but that's not a reason not to merge this PR.

Thanks!

@hallyn
Copy link
Member

hallyn commented Feb 1, 2021

Could you please fix the formatting in libmisc/salt.c (and a few other files - tabs instead of spaces) ? (Or if you prefer I can do it)

@hallyn
Copy link
Member

hallyn commented Feb 1, 2021

Could you please fix the formatting in libmisc/salt.c (and a few other files - tabs instead of spaces) ? (Or if you prefer I can do it)

Oh - I see, that comes in later comits.

Can you squash those? No reason to have those in history.

@breard-r
Copy link
Contributor Author

breard-r commented Feb 1, 2021

Can you squash those? No reason to have those in history.

If I am not mistaken, the correct way to do so is during the merge using the Squash and merge option. If I try to squash on my side it would not be able to push it without forcing, which may cause troubles.

Squash and merge

If not available, it can be activated: https://docs.github.com/en/enterprise/2.14/user/articles/configuring-commit-squashing-for-pull-requests

@solardiz
Copy link

solardiz commented Feb 1, 2021

@breard-r I don't know @hallyn's preference, but FWIW for projects that we manage at Openwall we currently prefer that the contributor squashes on their end and force-pushes - or avoids the multiple commits in the first place, force-pushing each change. Force-pushes work just fine, and GitHub UI allows to see diffs between them.

"Squash and merge" also works, but ends up with GitHub itself as the committer in git history (with the contributor as the author only, but not the committer), which feels unclean to me.

Edit: there's also no good alternative to force-pushes when the PR has 2+ genuinely separate commits, which should stay as such, yet the PR needs to be updated.

@breard-r
Copy link
Contributor Author

breard-r commented Feb 1, 2021

Thanx for the explanation, I understand. I'll try to squash & force-push.

@solardiz
Copy link

Any reason this isn't merged yet?

@hallyn
Copy link
Member

hallyn commented Mar 29, 2021

No - thank you.

@hallyn hallyn merged commit 697901a into shadow-maint:master Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add yescrypt as ENCRYPT_METHOD
3 participants