[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20261115033046.GA8274@openwall.com>
Date: Sun, 15 Nov 2026 04:30:46 +0100
From: Solar Designer <solar@...nwall.com>
To: Rich Felker <dalias@...c.org>
Cc: Lukas Tribus <lukas@...i.eu>, musl@...ts.openwall.com
Subject: Re: crypt: unsupported hash behavior
On Sat, Nov 14, 2026 at 11:25:51AM -0500, Rich Felker wrote:
> How about:
>
> if (output[0] != _PASSWORD_EFMT1 &&
> (ascii_is_noncanonical(output[0]) ||
> ascii_is_noncanonical(output[1]))) {
Looks good to me.
> if (strcmp(output, _setting))
> return NULL;
> }
>
> The nested if vs another && clause doesn't really make sense here, but
> see below...
> On Sat, Nov 14, 2026 at 09:44:07AM +0100, Solar Designer wrote:
> > strcmp() is not timing-safe, which for descrypt could actually matter
> > because of low salt size, but perhaps a less-irrelevant concern is we
> > may reach here for an unrecognized modern hash. Maybe pre-check for
> > _setting being of exactly length 13, perhaps our strnlen() reveals less
> > via timings since it works on words rather than chars? I assume we
> > don't readily have a timing-safe string comparison function we could
> > use, and I think this special case isn't worth introducing one.
>
> Even OpenSSH doesn't make an effort to avoid strcmp timing leaks
> comparing hash result. This should not matter for any modern hash, but
> I think with enough timing resolution you could use a combination of
> brute forcing all possible DES salts together with generating
> collisions to identify the correct salt and then byte-by-byte brute
> force the hash.
>
> Here the possible timing attack will only affect noncanonical salts,
> but perhaps we should still do something to make it as close to
> constant-time as possible. I think pre-checking for length 13 is a
> good idea.
>
> In the above, the strcmp would then be replaced my more steps to check
> length and do a fixed-length 13-byte compare that compares all of the
> bytes, justifying the nested conditional.
Yes, we could do that.
> Originally I was going to skip checking the value and just accept the
> noncanonical salt if length of the input is 13. But I thought that had
> more risk of doing the wrong thing under dubious things like what
> OpenSSH is doing (hashing one user's password with another's setting)
> and actually comparing the hash seemed like the safe way to ensure
> that can't happen.
Makes sense.
What about the concern that on failure we're nevertheless leaving the
computed hash in the output buffer, which may be our own or located in
the passed struct crypt_data?
One way to deal with this could be to use the output pointer in place of
test_buf in the self-test call if retval is NULL by that point:
- p = _crypt_extended_r_uut(test_key, test_setting, test_buf);
+ p = _crypt_extended_r_uut(test_key, test_setting, retval ? test_buf : output);
but that's dangerous in a different way: the passed struct crypt_data
would then get a fixed hash into it corresponding to our known test
password. So if any caller to crypt_r() uses that, or any caller to
crypt() reuses a previous call's returned pointer (of course either
behavior would be a bug), they may be vulnerable.
In general, do we want to try and make the output buffer content somehow
defined and safe in case our crypt*() fails (and returns either a
failure token or NULL)? If so, we should write something in there even
when we see a modern salt and so don't call __crypt_des(). But what
would a safe value be? Something like "*2" wouldn't match our main
failure tokens, but if a buggy program or two first write it into a
passwd file and then check against it for authentication it'll match.
This sort of thinking quickly gets ridiculous, and our ways of dealing
with possible misuses may be ridiculous (like we've seen in the OpenSSH
debacle, where they were differently ridiculous)... but here we are.
Also regarding the output buffer, do we perhaps want to consistently
overwrite the previous call's result? The current code would usually
replace it with the newly computed hash, but not fully if the new hash
is shorter, or maybe not at all if the new call fails. libxcrypt tried
to do that, but this broke chained calls where one crypt() call's return
value was passed as setting into the next as a user's test program did.
This was then https://github.com/besser82/libxcrypt/issues/209 "crypt()
overwrites its output buffer before reading arguments" fixed via
https://github.com/besser82/libxcrypt/pull/210 "lib/crypt.c: Do not
overwrite the output buffer too early."
Such a simple API and so tricky!
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
