yescrypt backend #12
yescrypt backend #12
Conversation
besser82
added
the
enhancement
|
First of all, thank you for your contribution! @ldv-alt already announced this PR was incoming. Before I start with a detailed review, there are many build failures on TravisCI - especially with Clang - caused by Coverage is also very poor: |
Makefile.am
Outdated
| @@ -127,6 +129,7 @@ check_PROGRAMS = \ | |||
| test-crypt-pbkdf1-sha1 test-crypt-sha256 test-crypt-sha512 \ | |||
| test-crypt-sunmd5 \ | |||
| test-byteorder test-badsalt test-gensalt test-short-outbuf | |||
| test-crypt-yescrypt | |||
There was a problem hiding this comment.
You have missed a trailing blackslash \ on the previous line.
Codecov Report
@@ Coverage Diff @@
## develop #12 +/- ##
==========================================
Coverage ? 83.76%
==========================================
Files ? 28
Lines ? 3116
Branches ? 0
==========================================
Hits ? 2610
Misses ? 506
Partials ? 0
Continue to review full report at Codecov.
|
| @@ -0,0 +1,182 @@ | |||
| /*- | |||
There was a problem hiding this comment.
This file almost duplicates the contents of byteorder.h. The functions for 16 bit types could be added there and then yescrypt files can be modified to use them.
There was a problem hiding this comment.
From a quick check in coverage report, it seems the 16 bit conversions are not used anyways. You should drop this file and replace it usage with byteorder.h as it can be done in a really safe way.
There was a problem hiding this comment.
I'm OK dropping the unused functions (inherited from scrypt, which in turn shared them with code in FreeBSD - which is why). That's a simple enough change for further merges (from scrypt). I'm not OK with more invasive changes.
There was a problem hiding this comment.
Dropped the unused sysendian.h functions in my yescrypt tree.
| @@ -0,0 +1,648 @@ | |||
| /*- | |||
There was a problem hiding this comment.
This is duplicate code. The SHA256 hash algorythm is already implemented in alg-sha256.c.
There was a problem hiding this comment.
Implementing HMAC-SHA256 is pretty simple as well. See alg-hmac-sha1.c for a reference.
There was a problem hiding this comment.
How about we make this the new alg-sha256.c?
| @@ -0,0 +1,121 @@ | |||
| /*- | |||
There was a problem hiding this comment.
This is duplicate code. The SHA256 hash algorythm is already implemented in alg-sha256.h. Implementing HMAC-SHA256 is pretty simple as well. See alg-hmac-sha1.h for a reference.
There was a problem hiding this comment.
It's slightly different in API (like have additional tmp32 parameter), and implements HMAC_SHA256 on top of this, and PBKDF2_SHA256 on top of that (having calls to things like SHA256_Pad_Almost(), which alg-sha256.c does not provide).
There was a problem hiding this comment.
SHA256_Pad_Almost() isn't used in yescrypt code-base anywhere; it's a simple helper function, that is called by this specific implementation of PBKDF2_SHA256().
Implementing this functionality in several seperate reusable files (and adding the official RFC and FIPS-180 test-vectors) shouldn't be to much of a problem.
There was a problem hiding this comment.
SHA256_Pad_Almost() is indirectly used in yescrypt, and I added it specifically for its use in yescrypt. This happens via PBKDF2_SHA256() when it's called by yescrypt with c=1, as yescrypt always does (and as scrypt always did). This is optimization for this special case of c=1. While we could simplify things for libxcrypt in particular and lose maybe just ~1% of performance on this (since yescrypt spends most time in its own heavy processing and not in its initial/final PBKDF2 steps), I am uncomfortable maintaining two versions of the code and for some larger deployments a combination of micro-optimizations like this was actually needed to squeeze out the last few missing percent of performance to meet server request rate capacity requirements. An option for libxcrypt is to revert to using Colin's sha256.c from the scrypt tree as-is, though. I maintained compatibility with it.
| Sw += 64; | ||
|
|
||
| #define PWXFORM { \ | ||
| uint8_t *Sw = S2 + w + PWXFORM_WRITE_OFFSET; \ |
There was a problem hiding this comment.
This one must be changed to uint8_t alignas (salsa20_blk_t *) …. We may get serious trouble on PPC64, ARMv7, S390X and some other arches, which don't support unaligned data, otherwise.
There was a problem hiding this comment.
No. And out of those you list, I've already tested the code on PPC64 and ARM. Sw is deliberately unaligned, which is then corrected with inclusion of PWXFORM_WRITE_OFFSET also in the actual writes, keeping them aligned. We could exclude this logic on archs where it's not known to help (it is on x86[-64]), but I'd rather not complicate the code with conditional compilation unnecessarily.
There was a problem hiding this comment.
"Fixing" the PWXFORM_WRITE_OFFSET thing to mute the warning e.g. by using salsa20_blk_t * pointers results in much worse code. It's just helpful to the compiler to show that byte offsets can be maintained throughout the code instead of having to do left/right shifts by 6 or whatever. Most of the time the compiler manages to cancel out the redundant pairs of shifts at compile time, but not in all places (here, the w variable would be tough because we also apply an AND mask to it). Also, this pattern and warning is seen not only here, but also in many other places in yescrypt-opt.c - e.g. yescrypt pwxform is deliberately designed such that the S-box byte offsets rather than element indices are obtained by ANDing a value with a mask and not needing any shifts. So I think we simply need to mute the warning in the compiler. That warning and avoiding such patterns can be helpful in other and non-performance-critical code, but not here.
| } | ||
|
|
||
| if (!yescrypt_encode_params_r(¶ms, rbytes, nrbytes, output, o_size)) { | ||
| /* |
There was a problem hiding this comment.
make_failure_token() from crypt.c could be used here. It just needs some small tweaks to the general code-base.
There was a problem hiding this comment.
Another (and the better idea instead of my first sugestion) would be to allocate a temporary buffer with o_size but not more than max. needed length and strncpy it to output after success.
There was a problem hiding this comment.
Why mess with allocation and copying, if I just can write 2-3 char? My choice is better in terms of performance and simplicity of code. I also don't need token alternation functionality from make_failure_token. You also have read out of bounds in make_failure_token.
There was a problem hiding this comment.
I also considered asking for both this function and crypt_yescrypt_rn to have the yescrypt primitives write to a temporary buffer (it could be on the stack, or in the scratch space passed to crypt_yescrypt_rn; you don't need to call malloc) and then copy back on success, instead of going to the trouble of re-creating the failure token, which is somewhat subtle (see below). I don't feel that strongly about it, but the performance hit would be negligible and it would simplify the control logic in both functions.
| @@ -98,6 +98,14 @@ static const char *const bcrypt_y_expected_output[] = | |||
| "$2y$05$mAyzaIeJu41dWUkxEbn8hO" | |||
| }; | |||
| #endif | |||
| #if INCLUDE_yescrypt | |||
There was a problem hiding this comment.
What about testing gensalt for scrypt $7$ here as well?
There was a problem hiding this comment.
yescrypt code does not generate settings for (obsolete?) $7$ prefix. Only crypt could encode already existing $7$ hashes.
|
@zackw As soon as this is ready to merge: What about changing the default hashing algorythm to yescrypt? bcrypt seams to become weaker. |
| output, o_size); | ||
| if (yescrypt_free_local(&local) || | ||
| !retval) { | ||
| /* |
There was a problem hiding this comment.
Same goes here. (temporary buffer to keep error token and such)
|
I am very uncomfortable making or even approving changes to yescrypt itself for this merge to happen. I strongly prefer that we keep yescrypt itself as-is. I intend to continue to maintain it upstream, and I really don't want to create extra work and risk of bugs (now and for further merges) out of thin air. I understand that it's not great to have e.g. duplicate SHA-256 code. Maybe we can tackle this the other way around - change the rest of libxcrypt to use the SHA-256, HMAC, PBKDF2 code provided along with yescrypt? It's code I ran tests on and know it works correctly and is optimized. What's currently in libxcrypt is of unknown status from my perspective. ;-) As to making yescrypt the default: yes, we'll probably need to move away from bcrypt in the next few years because of advances in FPGA and GPU attacks. Those you linked to aren't even the best results available - we link to our better results for bcrypt on FPGA right off the yescrypt homepage. ;-) However, yescrypt with its current parameters relies on availability of SSE2 or the like for decent performance. SSE2 is implied on x86-64, but not on 32-bit x86. I already mentioned to @ldv-alt that maybe libxcrypt should include building of an SSE2 version of the library on 32-bit x86 by default, letting the dynamic linker load it where SSE2 is supported (that is, on Pentium 4 / Athlon or newer). Speaking of non-x86 archs, I also intend to add NEON/ASIMD support (again, implied on Aarch64, but optional on 32-bit ARM), but am unsure whether it's worth adding anything else. Maybe it's OK'ish that some of those will run the slower scalar code (especially the 64-bit register-rich ones like PPC64 are OK'ish running it). |
|
I forgot to say: I am OK making changes to @vt-alt's wrappers for integration of yescrypt. Those are specific to the integration into libxcrypt, so are subject to change for the merge to happen. |
|
Quickly from my phone: I endorse the idea of replacing the existing sha256
implementation with the one from yescrypt. I was thinking of suggesting
that myself, just because it would reduce the amount of LGPL code — I have
a long-term daydream of getting the BSDs to pick up this library, which
ain’t gonna happen as long as it isn’t permissively licensed. Also, the
code was originally written by Ulrich Drepper, who believed in “if it was
hard to write it should be hard to understand,” ugh.
|
|
Semi-off-topic for this issue, but I'd also like to offer my permissively licensed MD4 and MD5. IIRC, right now libxcrypt uses Ulrich's MD5 and a third-party hack of my MD5 turned into MD4 (whereas I directly have MD4 as well). I'm not convinced it's a good idea to even have MD4 in here (for NT hashes perhaps?), but if they do I think my original MD4 is better than almost the same thing via a third-party. |
|
On Tue, Jul 10, 2018 at 05:07:08AM -0700, Solar Designer wrote:
I already mentioned to @ldv-alt that maybe libxcrypt should include building of an SSE2 version of the library on 32-bit x86 by default, letting the dynamic linker load it where SSE2 is supported (that is, on Pentium 4 / Athlon or newer).
I think of adding a configure check whether __SSE2__ is defined on x86,
and installing an extra SSE2 version of the library into $(libdir)/sse2
when __SSE2__ is not defined on x86.
|
If you've got time to make a PR, I'll merge it. 😄 Yes, MD4 is present only to support NTHASH ( |
hashes.lst
Outdated
| @@ -25,6 +25,8 @@ | |||
| # DES) must be last. | |||
| # | |||
| #name gs_suffix h_prefix nrbytes flags | |||
| yescrypt : $y$ 15 STRONG | |||
| yescrypt : $7$ 15 STRONG | |||
There was a problem hiding this comment.
Why 15? Shouldn't these be 16? Somehow they're 15 for SHA-crypt as well. I guess it's to more optimally use the base-64 chars, arriving at a whole number of them. I don't really mind, but is it common and would everyone be comfortable with it? I imagine some compliance guidelines could require 128-bit or larger salts, and then it'd be a pity that a system uses 120-bit ones instead.
There was a problem hiding this comment.
As discussed, these 15's should be changed to 16's.
| @@ -0,0 +1,182 @@ | |||
| /*- | |||
There was a problem hiding this comment.
Dropped the unused sysendian.h functions in my yescrypt tree.
|
On Tue, Jul 10, 2018 at 10:11:09AM -0700, Solar Designer wrote:
#name gs_suffix h_prefix nrbytes flags
+yescrypt : $y$ 15 STRONG
+yescrypt : $7$ 15 STRONG
Why 15? Shouldn't these be 16?
I bet they should.
Somehow they're 15 for SHA-crypt as well.
If I read gensalt_sha256_rn()/gensalt_sha512_rn() and gensalt_sha_rn()
correctly, they won't read more than 15 bytes of randomness.
…--
ldv
|
|
I am confused about the state of this PR. Is it worthwhile for me to do a detailed review? If it is, I can try to find time to do that in the next few days, but it seems like enough changes have already been requested that I should wait for the next round, I am concerned that y'all seem to be arguing over cycle-count efficiency of the cryptographic primitives. I don't think it makes sense to worry about that, because the higher level is going to iterate the primitives until the overall operation is slow enough, anyway. We need to be reasonably close to optimum so that end-user choice of round count isn't wrong by orders of magnitude (so, for instance, I am not going to ask for the total removal of the assembly optimizations) but we do not need to squeeze cycles the way we might if we were writing a TLS library. I am also concerned that, in the name of efficiency, the new code is pulling type shenanigans that may cause compilers to declare its behavior undefined, and may also make it harder for humans to understand its memory access behavior. I want the code in this library to be obviously correct first, and efficient second. |
|
I expect that @vt-alt will update this PR, but not in a major way (e.g., I've since avoided a few gcc warnings beyond -Wall in my tree, which should be merged in here). I agree that for libxcrypt cleaner code would be preferable over squeezing out every last bit of performance. However, there are users of yescrypt where those last bits of performance are needed (large deployments where this translates into meeting or not the performance SLAs on multi-server yescrypt password hashing clusters - and yes, some of these already exist), and I'm not eager about maintaining very different code versions (this is a higher risk of bugs in at least one of them, and less testing for each one of them - not to mention more work). I've been very careful to avoid instances of undefined behavior, except for the widely-accepted addressing of different types via a union (if a compiler breaks this, a lot of software out there will break, so it's a de-facto standard way to deal with aliasing). All aliasing going on in yescrypt is between types that are members of a union. For easier understanding, there's Maybe we should refer to the availability of I think that at some point (hopefully soon) we'll need to introduce runtime self-test similar to what I implemented for bcrypt. This will ensure yescrypt will refuse to run if miscompiled and will also help it effectively zeroize its buffers (better than with explicit zeroization). But if we try to do that right away, we'll get scope creep and "never" merge anything. |
|
I plan to rebase again against moving head, and apply minor changes from Solar and what is discussed reasonable about my part of code. I will not change yescrypt code (except disabling warnings and making it compile on clang and removing dead code). If you want (me?) to merge alg-sha256 with yescrypt-sha256 probably that could be done after this PR? (Also I plan to make different PR on the base of this one). |
|
The alg-sha256 merge should definitely be separate from this PR (or its replacement). One issue to discuss there is the zeroization of SHA-256 context buffers, which would be too slow to do in |
|
Also, we are already using this modifications in ALT so we are quite interested in pushing it upstream. |
|
@solardiz wrote:
I definitely understand and agree with a goal of minimizing code divergence.
Good to hear.
2x is enough of a difference that I would rather keep the faster code. It is a problem if someone on lower-end hardware (underpowered Android phones are probably the most important class of "lower-end hardware" nowadays) has to use a round count that's so low that an adversary can still brute-force it.
That sounds like a good idea in general. When I said "memory access behavior", though, I meant things like "can I be sure that this function does not read or write memory it's not entitled to read or write?" and for that, a reference implementation doesn't help.
I'm planning to do that at the crypt.c level for all hash algorithms, as a replacement for the current use of swapcontext. But I can't commit to any timeframe on that, and indeed it should not block this merge. |
|
Reference implementation does partially help address the issue of out-of-bounds access: if this different and simpler code computes the same hash values, then it's more likely the optimized code doesn't do out-of-bounds accesses either. Ditto for testing on other platforms. For further assurance, albeit also only on specific tested inputs, there's ASan. Runtime self-test for all hash algorithms would be great to have, but it's also tricky to do optimally as we'd need to be requesting iteration counts below the algorithms' publicly exported minimums. For example, SHA-crypt has a minimum of 1000, but for self-test we'd invoke e.g. 7, 21, or 42 (IIRC, one of these is the lowest to explore all code paths - need to find out), so we'd need to somehow export this functionality only for the library's own self-test, but not for its users. We could also record successful self-test status in a thread-local variable and then skip self-test for similar setting strings on further invocations. |
|
@vt-alt Any news on this? |
vt-alt
force-pushed
the
yescrypt
branch
4 times, most recently
from
b1b0b2a
to
29bd4c0
Compare
|
I pushed my commits rebased over the new head, containing small fixes including Solar's. Travis seem to build it OK, even though error is reported it's the same as with two latest head commits, due to Travis' build env being broken. |
|
Please rebase again on recent develop branch. TravisCI should work then. |
|
The change of recommended password hashing scheme in crypt.5 without a corresponding change of the default prefix doesn't look consistent. I suggest moving the change of defaults and recommendations to a separate follow-up commit. |
They are imported as is, with intent to implement yescrypt-based crypt backends. It is yescrypt-1.0.3 with improvements.
There was a problem hiding this comment.
I've just suggested a few more things that can easily be dropped given that we're already dropping ROM support from here. Unfortunately, there will be some ROM support remnants left - those that would change function prototypes if dropped, which I think would be a bad idea to do. And this reminds me: rather than silently fail to support a ROM, this revision should actively fail to work (return an error) if invoked on a ROM-using setting string. Is this currently the case or not yet (needing a fix)?
crypt-port.h
Outdated
| @@ -256,4 +256,22 @@ void _xcrypt_secure_memset (void *s, size_t len) | |||
| #define gensalt_sha_rn _crypt_gensalt_sha_rn | |||
| #endif | |||
|
|
|||
| #if INCLUDE_yescrypt | |||
| #define PBKDF2_SHA256 _crypt_PBKDF2_SHA256 | |||
| #define yescrypt_digest_shared _crypt_yescrypt_digest_shared | |||
There was a problem hiding this comment.
If dropping ROM support, it's also fairly easy and non-invasive to drop yescrypt_digest_shared.
yescrypt-opt.c
Outdated
| return retval; | ||
| } | ||
|
|
||
| yescrypt_binary_t *yescrypt_digest_shared(yescrypt_shared_t *shared) |
There was a problem hiding this comment.
If dropping ROM support, it's also fairly easy and non-invasive to drop yescrypt_digest_shared.
yescrypt-opt.c
Outdated
| le64enc(digest.uc, tag[2]); | ||
| le64enc(digest.uc + 8, tag[3]); | ||
| le64enc(digest.uc + 16, tag[4]); | ||
| le64enc(digest.uc + 24, tag[5]); |
There was a problem hiding this comment.
I think these are the only uses of le64enc(), so they can go as well.
yescrypt-sysendian.h
Outdated
| #define be64enc libcperciva_be64enc | ||
| #define le32dec libcperciva_le32dec | ||
| #define le32enc libcperciva_le32enc | ||
| #define le64enc libcperciva_le64enc |
yescrypt-sysendian.h
Outdated
| p[5] = (x >> 40) & 0xff; | ||
| p[6] = (x >> 48) & 0xff; | ||
| p[7] = (x >> 56) & 0xff; | ||
| } |
| * (and followed by 32 bytes of the ROM digest). | ||
| */ | ||
| #define YESCRYPT_ROM_TAG1 0x7470797263736579 /* "yescrypt" */ | ||
| #define YESCRYPT_ROM_TAG2 0x687361684d4f522d /* "-ROMhash" */ |
There was a problem hiding this comment.
Can drop the YESCRYPT_ROM_TAG* defines. Or maybe they're still needed if we choose to keep the ROM checking code inside yescrypt_kdf_body() intact, so that it'd fail on a ROM-using hash like we want it to (because we don't provide a way to pass a ROM pointer in there).
There was a problem hiding this comment.
Can't, since they are used in yescrypt_kdf_body in conditional which is not dropped yet.
There was a problem hiding this comment.
I feared to remove these conditionals (based on coverage report) in case they are supposed to be called with some settings.
There was a problem hiding this comment.
@solardiz Maybe you could add commit later with proper removal of such code?
There was a problem hiding this comment.
OK, let's keep these for now. I agree it's less invasive that way. Thanks.
hashes.lst
Outdated
| @@ -25,6 +25,8 @@ | |||
| # DES) must be last. | |||
| # | |||
| #name gs_suffix h_prefix nrbytes flags | |||
| yescrypt : $y$ 15 STRONG | |||
| yescrypt : $7$ 15 STRONG | |||
There was a problem hiding this comment.
As discussed, these 15's should be changed to 16's.
|
@ldv OK, already reverted that in last push. |
Turn off compilation warnings. Redefine insecure_memzero to XCRYPT_SECURE_MEMSET. Remove unused code.
yescrypt is modern scalable KDF and password hashing scheme. Reference and upstream is at http://www.openwall.com/yescrypt/
|
Just applied all changes that @solardiz suggested and reminded of. |
There was a problem hiding this comment.
This review covers only the "glue" code (crypt-yescrypt.c), documentation, and tests. I will review the actual yescrypt implementation separately.
I am generally pleased with this part of the code, but I do have a bunch of minor changes to request. The largest thing I would like to ask for is, I think we should go ahead and support $7$ (scrypt) in gensalt. We do
that for all the other compatibility hashes, including the ones that really shouldn't be used anymore. scrypt is counted as STRONG in hashes.lst, if people have a concrete reason to generate new hashes using it (LDAP distribution to a system that supports scrypt but not yescrypt, for instance) we shouldn't get in their way.
| @@ -1,3 +1,6 @@ | |||
| The yescrypt code comes from yescrypt by Solar Designer <solar at | |||
| openwall.com>. It builds upon Colin Percival's scrypt. | |||
|
|
|||
There was a problem hiding this comment.
Let's provide the URL http://openwall.com/yescrypt/ here.
| @@ -23,6 +26,8 @@ under the terms of the Common Development and Distribution License, | |||
| Version 1.0 only. It has been slightly modified for portability | |||
| and use inside of libxcrypt by Björn Esser. | |||
|
|
|||
| The yescrypt's crypt and gensalt backends are by <vt at altlinux.org>. | |||
|
|
|||
There was a problem hiding this comment.
English nitpicking: "The crypt and gensalt backends for yescrypt are by <vt at altlinux.org>."
(You could also say "yescrypt's crypt and gensalt backends", with no "the", but in this context it's better to make "the crypt and gensalt backends" be the primary subject of the sentence.)
(It is improper to say "the yescrypt['s]", because "yescrypt" is a name; in English, names don't take articles. It is proper to say "the crypt and gensalt backends", because the head noun is "backends", backends are countable, and you wrote two specific ones.)
| * Copyright Solar Designer, Colin Percival; 0- and 2-clause BSD, where | ||
| appropriate: yescrypt-common.c, yescrypt.h, yescrypt-opt.c, | ||
| yescrypt-platform.c, yescrypt-sha256.c, yescrypt-sha256.h, | ||
| yescrypt-sysendian.h |
There was a problem hiding this comment.
Some redistributors (Debian, for instance) would appreciate it if we would tell them which files are 0-clause and which are 2-clause BSD.
| @@ -37,7 +38,8 @@ libcrypt_la_SOURCES = \ | |||
| crypt.c crypt-bcrypt.c crypt-des.c crypt-gensalt-static.c \ | |||
| crypt-gensalt.c crypt-md5.c crypt-nthash.c crypt-pbkdf1-sha1.c \ | |||
| crypt-sha256.c crypt-sha512.c crypt-static.c crypt-sunmd5.c \ | |||
| randombytes.c | |||
| randombytes.c yescrypt-opt.c yescrypt-sha256.c \ | |||
| yescrypt-common.c crypt-yescrypt.c | |||
There was a problem hiding this comment.
For consistency with the rest of the library, these new files should be named alg-yescrypt-opt.c, alg-yescrypt-sha256.c, etc. I know that's not how it is in upstream. It's OK to leave yescrypt.h named yescrypt.h.
| @@ -153,7 +155,8 @@ check_PROGRAMS = \ | |||
| test-crypt-sunmd5 \ | |||
| test-byteorder test-badsalt test-badsetting test-gensalt \ | |||
| test-crypt-badargs test-short-outbuf \ | |||
| test-getrandom-interface test-getrandom-fallbacks | |||
| test-getrandom-interface test-getrandom-fallbacks \ | |||
| test-crypt-yescrypt | |||
There was a problem hiding this comment.
test-crypt-yescrypt should be added immediately after test-crypt-sunmd5, not at the end. This list has three groups: first all of the test-alg-*, in alphabetical order; then all of the test-crypt-* that test hashing methods, in alphabetical order; then all the miscellaneous tests, in alphabetical order. test-crypt-badargs is a miscellaneous test; it tests bad arguments to crypt(), not a hashing method. Yes, this is confusing; it should
maybe be renamed test-badcrypt or something.
| .br | ||
| yescrypt is a scalable password hashing scheme designed by Solar | ||
| Designer, which is based on Colin Percival's scrypt. | ||
| .hash "$y$" "\e$y\e$[./A-Za-z0-9]+\e$[./A-Za-z0-9]*\e$[./A-Za-z0-9]{43}" unlimited 8 "" 256 "up to 512" "0 (default), 1 to 11 (logarithmic)" |
There was a problem hiding this comment.
Minor adjustments here:
- it should be
8 256 256instead of8 "" 256so that the formatted manpage says "Hash size 256 bits" instead of "Hash size 256 bits (effectively )". - Don't say "0 (default)". That is a convention implemented by crypt_gensalt, not a property of the hash itself.
| Designer, which is based on Colin Percival's scrypt. | ||
| .hash "$y$" "\e$y\e$[./A-Za-z0-9]+\e$[./A-Za-z0-9]*\e$[./A-Za-z0-9]{43}" unlimited 8 "" 256 "up to 512" "0 (default), 1 to 11 (logarithmic)" | ||
| .PP | ||
| scrypt hashes previously used the "$7$" prefix, yescrypt is backward-compatible with them. |
There was a problem hiding this comment.
scrypt is different enough from yescrypt that I think the
.hash macro invocations have to be all on one line, but all other lines should be hard-wrapped below 80 characters. It makes editing easier down the road if you put the newlines after clause breaks, instead of fitting as much text as possible onto each line.
| @@ -28,6 +28,8 @@ | |||
| # DES) must be last. | |||
| # | |||
| #name gs_suffix h_prefix nrbytes flags | |||
| yescrypt : $y$ 16 STRONG | |||
| yescrypt : $7$ 16 STRONG | |||
There was a problem hiding this comment.
Was $7$ ever patched into one of the OSes that we have compatibility sets for?
There was a problem hiding this comment.
No, it's not in any OS I'm aware of. However, it's in libsodium (through libsodium's use of a fork of my enhanced scrypt tree that later led to yescrypt), which is a popular library and is available via various language bindings. So such hashes may be in use, but probably not yet by an OS. Since scrypt didn't originally come with an official crypt(3)-style encoding, there are many other encodings also in use. John the Ripper -jumbo includes support for $7$, as well as some others.
$7$ was the encoding I had suggested for scrypt in 2012 or so, and it was in an informal specification document maintained by the author of the eventual scrypt RFC, but ultimately the RFC does not include any crypt(3)-style encoding.
Classic scrypt hashes may also be encoded (and with fewer characters) under the $y$ prefix (where the variable length integer encoded right after that prefix is the flavor, with single . character meaning 0, which is classic scrypt). This is the encoding that yescrypt_encode_params() will use if invoked with a params structure with flags=0, requesting classic scrypt.
So for the current yescrypt tree $7$ is a decoding-only compatibility prefix.
There was a problem hiding this comment.
Maybe we should not implement $7$ prefix at all in libcxrypt?
There was a problem hiding this comment.
Not implementing the $7$ prefix in libxcrypt at all (at least for now, until/unless we see any demand for it) is OK with me.
There was a problem hiding this comment.
Stating an argument to not add $7$ to libxcrypt: we are (in ALT) never intended to add it, it is just side effect of what yescrypt supports, to implement something that is not needed just for the sake symmetry or possibility is strange; it is purely virtual; and scrypt is already properly supported under $y$ prefix.
| @@ -294,6 +302,9 @@ static const struct testcase testcases[] = | |||
| { "$2a$", bcrypt_a_expected_output, 29, 0, 0 }, | |||
| { "$2x$", bcrypt_x_expected_output, 29, 0, 0 }, | |||
| { "$2y$", bcrypt_y_expected_output, 29, 0, 0 }, | |||
| #endif | |||
| #if INCLUDE_yescrypt | |||
| { "$y$", yescrypt_expected_output, 29, 29, 0 }, | |||
There was a problem hiding this comment.
Please add tests for cost parameters 1 and 11 as well.
| result |= test("", "$y$jD5.7$$", "$y$jD5.7$$JD8dsR.nt1ty0ltQ2HHwauaDRoOUIEaA5i.vpj2nyL."); | ||
| result |= test("test", "$y$", "*0"); | ||
| result |= test("test", "*0", "*1"); | ||
| result |= test("test", "*1", "*0"); |
There was a problem hiding this comment.
Based on the coverage report, we need a whole lot more testing than this, but I don't know how much of the uncovered code is reachable from the crypt() interface. Are there tests in the yescrypt upstream codebase that could be brought over?
There was a problem hiding this comment.
I should add that I'd be okay with merging at the current level of coverage if you are willing to put in the time to write more tests immediately afterward. What I want to avoid is us putting out a release with an embarrassingly obvious bug in it that we would have caught if we had only spent some more time writing tests.
There was a problem hiding this comment.
I think some of the uncovered code is not reachable (the remnants of ROM support beyond checking that there must be no ROM), and some is (classic scrypt). We should probably add classic scrypt test vectors using both $y$. and $7$ prefixes. Some test vectors may be taken from libsodium.
Maybe that's a reason to reconsider and stop doing it for hashes "that really shouldn't be used anymore"? The current yescrypt tree does support encoding of salts for classic scrypt hashes, but using the |
|
@zackw I'll merge this PR as I have already a local branch which addresses the changes / fixes you mentioned / requested during your review. I will push and file my branch with the fixes for review during later today. I dont't want the current PR to become a never ending story… @vt-alt Thanks for your initial work! |
This patch set implements yescrypt backend for crypt/gensalt, which is modern password hashing scheme. Original yescrypt password hashing scheme is developed by Solar Designer (@solardiz) and based on Colin Percival's work on scrypt.
yescrypt prefix is
$y$, but it also couldcrypt()scrypt's$7$hashes. Output hash size is 256-bits.countparameter translate to yescrypt's parameters specifying memory usage by formula1 << (count - 1)of MiBs, and0is recommended default.First patch just verbatim copy of yescrypt sources from the current yescrypt project cvs. Second patch rename files, cut them from dead code, and modify them to work with libxcrypt. Third patch implements crypt/gensalt backend itself.
Thanks,