Repository navigation
No yescrypt - #1791
No yescrypt#1791alejandro-colomar wants to merge 1 commit into
Conversation
b64b139 to
02ab3c7
Compare
| skip(); // locale not available on this system | ||
| } | ||
|
|
||
| assert_true(is_valid_hash("$y$j9T$lChB5KbMj.6n51V3DjPfn0$6WyruF4YWm9GDgKx2mik5QEHE6J4m2RVXXegiqrfjI8")); | ||
|
|
There was a problem hiding this comment.
Would you mind sending a patch using a different hashing algorithm?
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
This is not the proper way to do things, thus I'm blocking this PR
|
@thesamesam , @jubalh , @dvzrv , @zeha I and @hallyn would like to get rid of this code. This needs some coordination with distros, since they seem to be defaulting to YESCRYPT. So, how do you see changing your defaults to something else (perhaps SHA512, which I expect is okay)? The problem with YESCRYPT is fundamental: it requires code that is more prone to buffer overflows than the other algorithms (I wrote one, and we didn't realize for many months, and we almost merged it). I found it only by chance, when trying to actually understand the format of YESCRYPT, which is actually hard to understand. |
Debian enables YESCRYPT because Debian's PAM uses YESCRYPT. From my PoV, ideally shadow with PAM would delegate all password hashing and manipulation to PAM, and let PAM do whatever it thinks is right. Or, if this is already the case, then ... what does |
Is PAM's default YESCRYPT? Or is it just an existing algorithm? Cc: @thkukuk In Debian, the default seems to be YESCRYPT (at least, my passwords are using YESCRYPT, and I didn't change the defaults in my Debian system).
That is orthogonal at the moment. YESCRYPT is prone to bugs, because of its format design, and wherever it happens, that code will be prone to buffer overflows and other important problems. I wouldn't trust shadow doing that. I also wouldn't trust PAM doing that. YESCRYPT must be extinguished.
shadow defaults to SHA512. I think PAM should also default to SHA512, indeed.
No idea. |
Debian's PAM default is YESCRYPT. |
|
I saw quite some |
|
On Arch Linux we have switched from SHA512 to YESCRYPT as default in 2023: https://archlinux.org/news/changes-to-default-password-hashing-algorithm-and-umask-settings/ To understand the switching costs here: Every user's passphrase would need to be updated? 🤔 |
The default is SHA512 if no That's hardcoded, no compile time option. |
The reasons for using yescrypt seem to be its better resilience, which is nice. The problem we have with yescrypt is actually in its format for the parameters, and not the hashing algorithm itself. Is it possible to use a different format with yescrypt? The problematic format is
Yes. That will be the case eventually, anyway. |
|
Fedora uses yescrypt by default. |
|
@besser82 , can yescrypt use |
|
Could we delegate yescrypt salt generation to libxcrypt’s crypt_gensalt_rn(), as Linux-PAM already does? That would avoid maintaining the parameter encoding in shadow. This PR also makes chpasswd -e reject existing |
As far as I could find in various web searches, yescrypt supports
I do NOT trust libxcrypt doing that, because I don't trust myself doing that. The problem is not maintaining it here. I actually trust myself better than libxcrypt to do a good job of writing safe code. The problem is that I don't think it's possible to maintain The underlying problem is that whoever thought that it would be nice to compact the string so much, using such a weird translation format, didn't think in the consequences of that. Compression software isn't easy. And just for a few bytes...
Yes, that's something we can discuss later, though. |
|
Given the claims that the yescrypt support was so bad, I was expecting tons of code needed to implement it. But if I'm not missing anything this seems to get down to just the It seems the amount of pain this removal would cause is not proportional to the amount of code involved. If this is still considered a burden, I think the proposal to delegate this to libxcrypt makes sense. |
I checked libxcrypt’s implementation: |
It's a small piece of code, but it's quite unreadable, and enough to hide a buffer overflow for more than a month to my eyes.
This depends on the answer to the question about If If anyone has doubts of what I mean:
I consider this a viable (but not ideal) option, but someone please actively work in replacing that format for the future, because it's a can of worms. |
Thanks! Then, please someone involved in YESCRYPT invent a replacement format for it. |
I've been trying to write a patch doing this, but the problem is in generating what libxcrypt calls the "prefix". That is, libxcrypt's crypt_gensalt() is the consumer of this string, not the producer. Does libxcrypt have a function for generating this string?? |
Or perhaps our source code is wrong? It feels weird that we encode the cost in the string, and then call crypt_gensalt() again with the cost. Can someone who knows about libxcrypt please check? Lines 370 to 375 in 5d25282 Line 391 in 5d25282 |
|
Yes, that encoding appears redundant when using libxcrypt. The prefix can be just "$y$", with the cost passed separately:
Linux-PAM uses the same approach (https://github.com/linux-pam/linux-pam/blob/v1.7.3/modules/pam_unix/passverify.c#L464) with crypt_gensalt_rn(). So shadow can skip YESCRYPT_salt_cost_to_buf() in the USE_XCRYPT_GENSALT path. |
Thanks!! |
|
Per the other PR (#1792), we can't hide this code in libxcrypt, because musl systems (such as Alpine) use libc crypt(3), and don't provide crypt_gensalt(3). We need to provide fallback code (and BTW, the code using libc crypt(3) is quite simple except for YESCRYPT). So, YESCRYPT remains deprecated to me. If you want to continue using YESCRYPT, please invent a new format that isn't so brain-damaged. Or, use our YESCRYPT code at your own risk. If you want to keep it around for some time, we can indeed keep it around for some time, but you'll be using it at your own risk. I warn that bugs like buffer overflows are likely in code handling YESCRYPT. |
musl's crypt implementation does not support yescrypt, so we don't need to provide a fallback crypt_gensalt for it. We require libxcrypt for yescrypt already, so relying on crypt_gensalt when libxcrypt is enabled seems like a non-issue. |
It's the first time I ever write a buffer overflow in public code (IIRC). I'm sorry if I'm freaking out, but I'm good at recognizing code which is prone to these bugs (which is the reason I've never written one), and this is such code. 5 minutes before knowing there was a buffer overflow, I was already scared of the code, and I warned that I might write such a bug there eventually. 5 minutes after that warning, an LLM told me that indeed there was a buffer overflow there (and I verified it was true). So, I think I have good reasons to freak out.
I disagree. The number of lines of code is entirely orthogonal to the number of bugs. I've written many thousands of lines of code in this same project, knowing I had no chances of making such a mistake. In these 10 lines of code, I knew something would go wrong, before actually knowing it was wrong. Tests, etc., are only a mitigation. The first barrier should be not writing code that is prone to bugs in the first place.
These bugs don't happen without a reason; not to me (so far). And I'd like it to stay like that. |
|
I think you are overreacting. Have you found a bug in the libxcrypt implementation? There are several distros using this code. Ripping support out of shadow based on your uneasy feelings is not acceptable. |
|
Deprecating it without real evidence is unacceptable as well. |
Evidence of what? Of the format resulting in code that is unreasonably prone to buffer overflows? There's evidence of that: the buffer overflow added in #1750 v1, and fixed in #1750 v4. The reason why it's prone to buffer overflows is that it forces the programmer to perform manual byte operations and pointer dereferences, which are inherently prone to that. We've been getting rid of such code everywhere we could, and this is one of the few places where we couldn't get rid of it. Rust got that part of language design right. C is not a bad language, but programmers that refuse to avoid bad patterns are what give it the bad fame it has. One can write C code that is as safe as Rust, but only by avoiding patterns such as the one promoted by YESCRYPT. What more evidence do you need?
No, but I don't need to. I hadn't yet found a bug in our code, when I already knew it was bad. 5 minutes later, an LLM found a buffer overflow for me. Are we waiting until we find such a vulnerability in libxcrypt's YESCRYPT code? Or can we pretty please solve the problem before it becomes an urgency? If you wait to fix the railways until after 2 trains crash, you've got it bad. The same happens with code. Just look at the early symptoms, and you'll avoid the bad problems entirely. |
Evidence of a bug in libxcrypt's yescrypt gensalt code that has not been addressed by its maintainers. Your broad claims that the format results in code that is prone to errors are not compelling to me. |
I don't know if github stores old versions of PRs. Sorry. You should be able to reconstruct v1 from the range-diffs I provide in the first comment in that PR, but there's no direct link to it, AFAIK.
I hope we don't have evidence of that.
Our job should be preventing bugs, not reacting to them. That's how I work, at least. |
You are basically proposing that everyone should boycott the format because you can't figure out how to re-implement it in a way you feel is safe. Discuss the issue with the author of yescrypt and the maintainer of libxcrypt if you think there is a problem. |
Can you? Illuminate me, please.
I am doing that (with the author of yescrypt; I think the author of libxcrypt is irrelevant in this discussion). |
At this moment no, because I do not fully understand what you find objectionable, and I do not fully understand the format myself. I can't claim to understand every piece of crypto code that runs on my system either. But I don't advocate that nobody use code I don't comprehend. |
Yet you are trying to argue that I'm arbitrarily saying that the format is promoting unsafe code, as if I was some passing-by rando in a social network ranting about some random topic. I have learnt the format (or rather, I've tried hard to learn it), and am experienced with this code base, and can say that it's some of the worst code I've seen in this code base, and I can't improve it, because of the format. I have been writing code free of buffer overflows for many years, and know when I can't because of external conditions, and this is the case. Please try to learn about what you're talking before actually talking.
Again, you're implying that I don't comprehend that code. Please don't. I comprehend that code and that format enough to be able to know that one can't write code as simple as for the other algorithms. And I also comprehend that it is not intrinsically necessary of the algorithm; it's just a bad design decision from the author of the format.
Well, perhaps you could try to, before dismissing my claims, don't you think? |
So the LLM prevented you from publishing code with a buffer overflow. Problem solved. No trains crashed. This clearly shows that there are tools available that make writing code with libxcrypt possible :) |
I do not see you as an authority on this subject. You haven't provided a detailed explanation of the problem. You just say the format is complex and mention a couple of features of C that you don't like (byte manipulation and pointers). Assuming the yescrypt author disagrees with you and cannot alleviate your concerns, convincing libxcrypt to deprecate it would go a long way to convince other people it is unsafe. Until then, please keep the minimal amount of code that is necessary to keep using yescrypt via libxcrypt's interface. If there's a bug, it's not your bug and you can let others worry about it. |
I was lucky, indeed. We might not be so lucky the next time. That's why we should discuss improving the situation.
No; LLMs are far from being infallible at finding these issues in code. We certainly gained some time, and we should use that time wisely. We should also learn from it. Certain bugs are orders of magnitude more likely in some code, and this is one of those cases. Can we avoid the risk in some way?
We're not in a hurry, indeed. But we should not be wasting the time we've been given either. We should now discuss about improving the format so that this doesn't happen again. We don't need to drop YESCRYPT, but we should consider adding a new format for it. |
That certainly seems like a reasonable path forward. |
Well, perhaps you should. In the subject of writing safe code and finding unsafe patterns, I believe I am quite an expert. You can ask those that work with me closely.
You keep talking as if I was some weirdo who arbitrarily dislikes some features. Rust disallows dereferences in safe code for a reason. BTW, I'm member of the C Commitee, in case you doubt my understanding of C.
I will.
Of course; that's what I said. |
|
Thank you for the explanation. I will add a couple comments, though I expect you already know these details. In the yescrypt reference implementation (and libxcrypt), the parameter string (eg. The packing algorithm is quite hard for me to follow, but Claude gives a pretty nice summary. I can understand why you might not want to implement that yourself; it makes my head hurt looking at it. |
Yup.
Heh! Indeed, that is the feeling. It seems now we're on the same page. :) |
|
I'm confused by the fact that you keep blaming the complexity of the format for you having implemented a buffer overflow, ("manual byte operations and pointer dereferences", "Here, we're manually dereferencing the pointer and performing pointer arithmetic on it",) but the actual buffer overflow bug appears to be entirely unrelated to any of this. This is the part of the diff that had the bug: 0cb8001#diff-a69570be0b33e25224d86849a25125570656605fc205d787135a9b653ee1367dL266-R286 We have a comment (correctly) stating that we're building a string of 4 bytes, but we're only creating a buffer of 4 bytes, meaning that we don't have enough space for the terminating null. This then leads to the final So I'm confused as to how you could possibly blame this specific bug on the format being complicated? We could implement the exact same operation with a single static const char *salt_cost(unsigned long cost) {
static char buf[4];
char c;
if (cost < 3) {
c = 0x36 + cost;
} else if (cost < 6) {
c = 0x34 + cost;
} else {
c = 0x3b + cost;
}
sprintf(buf, "j%c%c$", c, (cost >= 3) ? 'T' : '5');
return buf;
}Additionally, while I definitely agree that the variable-length encoding is kind of complicated to understand, one kind of doesn't even have to fully understand how it works. If you reimplement the version from the reference implementation exactly (the 12 lines here) then you will end up with the correct variable-length encoded version of the parameter. This would always be safe since all it does is perform a bunch of integer arithmetic on values that are all The bit at the end of the function then Base64 encodes this encoded value. (Or are we going to claim that the entire concept of Base64 is too complicated and brain damaged as well?) |
|
[...]
It produced confusing code, which degrades human review, which causes bugs nearby.
This is an interesting implementation, indeed. I believe this might have been caught by the compiler at compile time.
Safe as in memory-safe, but far from logic-bug-safe. But yeah, better than what we had.
Base64 isn't bad per se, but using it unnecessarily is quite OTT. |
Self-correction: not safe. *dst++ = itoa64[start + (src >> bits)];Those 12 lines you highlighted are used for producing an offset which seems later used in a dereference. That's not safe, unless I'm missing something.
|
I don't buy this. While the reason behind the three different offsets depending on the cost's range is opaque (since it's treating a doubly encoded value as a magic value), we're talking about 9 lines of C code that are very easy to understand what they do. I don't at all believe that if it was instead If anything I'd say that a reviewer would on average be more likely to be confused by the repeated use of
gcc can, clang does by default, yes.
I was very explicitly talking specifically about re-using the implementation of the variable-length encoding itself, which is what you mentioned finding confusing above. I very intentionally said nothing about the Base64 encoding, which you could then implement in whichever way you find most appealing. The simplest option would of course be to just take the result *dst++ = itoa64[(start + (src >> bits)) & 0x3F]; |
|
Because I agree that the reference implementation is inscrutable, here's a reimplementation that I threw together in case anyone want to better understand how the format works: https://c.godbolt.org/z/YonW5Prj7 That godbolt link has the reference implementation, my reimplementation, and a version that executes both of them against all representable values and asserts that the output is the same. My version is definitely longer, but I'd argue significantly more readable, and amusingly clang's generated assembly is shorter for my version. |
|
yescrypt designer and upstream maintainer here. I agree the hash encoding format is more complicated than many of us (me included) would have liked, but there are technical and historical reasons for that, as well as other people's preferences. Some actually prefer compact over verbose encoding - we just won't see them complaining unless we make the encoding too verbose instead. The comparison against sha512crypt's human-friendly encoding does not fully apply. sha512crypt is merely an iterated password hash, whereas the full upstream yescrypt is iterated, cache-hard, memory-hard, and optionally ROM-port-hard, and is both a KDF and a password hash, and there are defined ways to encode usage of larger caches and wider SIMD for future scalability. This flexibility required many more parameters, most of them optional. One alternative would have been splitting this into several formats specified separately by use case, but even then it'd be multiple parameters per use case. So what we did for libxcrypt instead was define a mapping from a linear cost parameter (which used to specify the iteration count with previous generation password hashes like bcrypt and sha512crypt) to lower-level yescrypt parameter sets that are currently optimal for this use case. As it happens, we only vary the values of two lower-level parameters while keeping the rest constant or even at implied defaults. I think this mapping is actually simple enough for what it achieves. With this, we effectively already have and use a lighter version/subset of yescrypt, where much of the other-use-case and maybe-future functionality is not exposed and the complicated encoding for those many parameters is mostly an internal detail - it can be viewed as opaque mapping from 11 linear values to two-character substrings. Not that hard, really. Possible alternative formats I can think of would be either putting the linear cost value into the string directly or putting the current multiple parameters in there in human-readable form like it's commonly done for Argon2. Both of these have drawbacks. One of my design goals for the current yescrypt format was non-redundancy - there should be exactly one way to encode a combination of parameter values. This is desirable e.g. for password security audits. We have a lot of extra complexity for detecting and dealing with effectively duplicate encodings for other hash types in John the Ripper. I didn't want more of that. If we introduce any new format now, it will already be a duplicate with the current one. Further, if we make it a human-readable format, there's a further question of whether we nevertheless enforce strict field order and minimal representation (e.g., no leading 0's in integers, all parameters must always be specified explicitly or if any have defaults then those with default values must always be omitted) or not (with more duplicates then). This will be way beyond the level of complexity that scared Alejandro here, and in many projects it will be on top of also supporting the current format for years. Focusing on the shadow project's needs, I think the approach taken in #1794 to use libxcrypt's crypt_gensalt is good at least for now, because yescrypt support here is currently only needed on systems with libxcrypt. musl doesn't have crypt_gensalt, but it also doesn't have yescrypt yet. I also think Alejandro's originally desired cleaned up function isn't actually hard to write without a buffer overflow, e.g. here are a few ways to do it, along with defensive programming against wrong inputs to the function, and a test program: #include <stdio.h>
#if 1
static /*@observer@*/const char *YESCRYPT_salt_cost(unsigned long cost)
{
static char buf[5];
if (cost < 1 || cost > 11)
return NULL;
buf[0] = 'j'; // flags = YESCRYPT_DEFAULTS
buf[1] = "78789ABCDEF"[cost - 1]; // Nlog2 from 10 to 18
buf[2] = cost < 3 ? '5' : 'T'; // r = 8 (1 KiB) or r = 32 (4 KiB)
buf[3] = '$';
buf[4] = 0;
return buf;
}
#elif 1
static /*@observer@*/const char *YESCRYPT_salt_cost(unsigned long cost)
{
static char buf[5] = "j..$"; // Assumes the caller won't overwrite
if (cost < 1 || cost > 11)
return NULL;
buf[1] = "78789ABCDEF"[cost - 1]; // Nlog2 from 10 to 18
buf[2] = cost < 3 ? '5' : 'T'; // r = 8 (1 KiB) or r = 32 (4 KiB)
return buf;
}
#else
static /*@observer@*/const char *YESCRYPT_salt_cost(unsigned long cost)
{
static const char bufs[11][5] = {"j75$", "j85$", "j7T$", "j8T$", "j9T$", "jAT$", "jBT$", "jCT$", "jDT$", "jET$", "jFT$"};
if (cost < 1 || cost > 11)
return NULL;
return bufs[cost - 1];
}
#endif
int main(void)
{
unsigned long cost;
for (cost = 0; cost <= 12; cost++)
puts(YESCRYPT_salt_cost(cost) ?: "NULL");
}It is not necessary to (re-)add any code like this to shadow now, as long as the rest of the support stays and the system uses libxcrypt. Please just do not deprecate and drop the remaining support. Doing so would result in multiple distros re-adding such support via patches maintained downstream, which would waste effort and make things worse for the ecosystem (more code variations and thus potentially more bugs too). |
[...] Hi @solardiz ! Thanks a lot for the detailed comment. I like the translation using the string literal
Being a literal (thus, with compile-time-known bounds), and with a bounds check in the previous line of code, it seems safe. Combining with @kyrias 's sprintf(3) suggestion, we can get a quite simple implementation: static const char *
YESCRYPT_salt_cost(unsigned long cost)
{
static char buf[5];
if (cost < 1 || cost > 11)
return NULL;
sprintf(buf, "j%c%c$", "78789ABCDEF"[cost - 1], (cost >= 3) ? 'T' : '5'));
return buf;
}While the rationale for the specific values is not clear, we can now see all possible translations easily (and assume it's a look-up-table of magic values). And the compiler is able to check whether the bounds of the arrays are respected. sprintf(3) writes exactly 5 bytes, so buf is safe, and the string literal is accessed at somewhere between 0 and 10, which is known to be in bounds. And the previous check is obvious now: it's there for the string literal bounds. I feel now okay with this format, and have removed the deprecation notice. Thanks! I wonder something about the difference between this and the code we had: we allowed any value of cost, while this now restricts the values to 1..11. Should we have been rejecting a value of 12 before? Was it a bug? |
|
BTW, I'm pretty happy to have been using stpcpy(3) instead of writing the last byte manually with It seems this commit has already paid off: 59e5eef (2024-07-03; "contrib, lib/, src/, tests/: Use stpcpy(3) instead of its pattern"). diff$ git show 59e5eef38f899 -- lib/salt.c
commit 59e5eef38f899cf00925f0117e3173566b14a5fc
Author: Alejandro Colomar <alx@kernel.org>
Date: 2024-05-19 01:53:12 +0200
contrib, lib/, src/, tests/: Use stpcpy(3) instead of its pattern
Signed-off-by: Alejandro Colomar <alx@kernel.org>
diff --git a/lib/salt.c b/lib/salt.c
index 529d59cace4d..efef4e59ca4f 100644
--- a/lib/salt.c
+++ b/lib/salt.c
@@ -291,6 +291,7 @@ static /*@observer@*/unsigned long YESCRYPT_get_salt_cost (/*@null@*/const int *
static /*@observer@*/void YESCRYPT_salt_cost_to_buf (char *buf, unsigned long cost)
{
const size_t buf_begin = strlen (buf);
+ char *p;
/*
* Check if the result buffer is long enough.
@@ -302,17 +303,17 @@ static /*@observer@*/void YESCRYPT_salt_cost_to_buf (char *buf, unsigned long co
*/
assert (GENSALT_SETTING_SIZE > buf_begin + 4);
- buf[buf_begin + 0] = 'j';
+ p = &buf[buf_begin];
+ p = stpcpy(p, "j");
if (cost < 3) {
- buf[buf_begin + 1] = 0x36 + cost;
+ *p++ = 0x36 + cost;
} else if (cost < 6) {
- buf[buf_begin + 1] = 0x34 + cost;
+ *p++ = 0x34 + cost;
} else {
- buf[buf_begin + 1] = 0x3b + cost;
+ *p++ = 0x3b + cost;
}
- buf[buf_begin + 2] = cost >= 3 ? 'T' : '5';
- buf[buf_begin + 3] = '$';
- buf[buf_begin + 4] = '\0';
+ p = stpcpy(p, (cost >= 3) ? "T" : "5");
+ stpcpy(p, "$");
}
#endif /* USE_YESCRYPT */
@@ -330,7 +331,7 @@ static /*@observer@*/const char *gensalt (size_t salt_size)
strcat (salt, l64a (csrand ()));
} while (strlen (salt) < salt_size);
- salt[salt_size] = '\0';
+ stpcpy(&salt[salt_size], "");
return salt;
}
@@ -421,7 +422,7 @@ static /*@observer@*/const char *gensalt (size_t salt_size)
salt_len = GENSALT_SETTING_SIZE - 1;
rounds = 0;
memset(result, '.', salt_len);
- result[salt_len] = '\0';
+ stpcpy(&result[salt_len], "");
}
char *retval = crypt_gensalt (result, rounds, NULL, 0); |
|
Thank you, @alejandro-colomar!
I seem to recall this having been pre-checked in the calling code, but we may take a closer look and confirm now. libxcrypt currently defines the mapping for this range only, which corresponds to the range from 1 MiB to 1 GiB. Some sane limit is needed as this can quickly get to ridiculous sizes, and besides I think |
Hmmm, indeed, we have checks elsewhere: Line 255 in 763e75e Now they make sense. |
It's a can of worms. The format is very difficult to understand, and
the specification is unnecessarily complex, requiring difficult manual
handling of bytes, which is highly prone to dangerous bugs, such as
buffer overflows. It seems designed to be obfuscated. Let's just stay
away from it.
I expect that other implementations of yescrypt have important bugs,
because it's just insanely difficult to handle this format. If one
cares about security, one should avoid using any software that uses this
algorithm.
Cc: @ikerexxe
Cc: @hallyn
Revisions:
v2
v3