[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20261114162551.GQ5716@brightrain.aerifal.cx>
Date: Sat, 14 Nov 2026 11:25:51 -0500
From: Rich Felker <dalias@...c.org>
To: Solar Designer <solar@...nwall.com>
Cc: Lukas Tribus <lukas@...i.eu>, musl@...ts.openwall.com
Subject: Re: crypt: unsupported hash behavior
On Sat, Nov 14, 2026 at 09:44:07AM +0100, Solar Designer wrote:
> On Thu, Nov 12, 2026 at 10:53:49AM -0500, Rich Felker wrote:
> > On Thu, Nov 12, 2026 at 10:53:13AM -0500, Rich Felker wrote:
> > > On Thu, Nov 12, 2026 at 09:53:23AM -0500, Rich Felker wrote:
> > > > Yes, I've suggested we could remove the support for noncanonical DES
> > > > salts without breaking existing hashes that might exist in the wild by
> > > > doing something like, at the end of descrypt:
> > > >
> > > > if (has_bad_characters) {
> > > > if (strcmp(output,setting)) return 0;
> > > > else return output;
> > > > }
> > > >
> > > > Note that the noncanonical DES hashes are already stochastically
> > > > buggy. It's improbable but possibe for descrypt(password,"$n") to
> > > > produce a DES hash where the first few characters of the hash mimic a
> > > > valid "$n$" pattern, which when you pass the hash back to crypt, will
> > > > result in a different hash function getting used, thereby locking the
> > > > user out.
> > >
> > > Patch attached. Should we apply this now? I think it's probably a good
> > > idea. CC'ing Solar for input.
>
> Thanks. I have mixed feelings about this. On one hand, it's a cool
> hack and it supports even the ancient broken hashes with less impact on
> supporting also the modern ones than we had before this patch. On the
> other hand, it's yet another hack that would surprise people, something
> along the lines of "this invalid salt is unsupported... unless it would
> make authentication succeed?!" In a sense, this is fail-open, which is
> bad for security... but here it's deliberate if we retain any support
> for such broken hashes at all.
>
> I said "ancient" hashes, but more relevantly I guess there could be
> obscure recent uses of musl (perhaps by some custom admin interfaces for
> embedded systems) where these could have made it into passwd files just
> like in the ancient times. With this patch, we'd prevent this from
> continuing to happen while not locking anyone out... except maybe on a
> password change, if they continue to generate invalid salts and don't
> check for failure. These are obscure misuses of musl, but that's what
> we have to consider when evaluating this patch because that's part of
> what it deals with.
Yeah, that's a good point -- there might be new ones too thanks to
musl having allowed it. Not in things made by common software, but
like you said custom, most likely embedded, software setting
passwords.
> > From 3d3c479c37be19b4bc7b8b322775baf51d4ae826 Mon Sep 17 00:00:00 2001
> > From: Rich Felker <dalias@...ifal.cx>
> > Date: Thu, 12 Nov 2026 10:33:32 -0500
> > Subject: [PATCH] crypt: drop support for non-strict des salts in new hashes
> >
> > our descrypt implementation followed a historical practice of
> > interpreting salt characters outside the set [./0-9A-Za-z] as
> > alternate/noncanonical encodings. this served basically no purpose
> > except possibly supporting des hashes made on ancient systems
> > enforcing the salt character set. however, allowing this for new
> > hashes has several negative consequences, including:
> >
> > 1. unrecognized hash types encoded with "$n$" notation get interpreted
> > as very weak, fixed-value des salts.
> >
> > 2. with low but plausible probability, the result of des hashing a
> > salt consisting of a '$' followed by a digit can produce hash bits
> > that mimic the setting for a different hash type, resulting in a hash
> > that will be unmatchable later (user locked out).
> >
> > with this change, noncanonical salt characters are still interpreted
> > and result in a hash being computed, but the result is thrown away and
> > replaced with failure unless the full hash containing them was passed
> > as the setting and the password matches.
> >
> > the self-test in top-level __crypt_des is updated to canonicalize the
> > test salt; without this change, the self-test would always have
> > failed.
> > ---
> > src/crypt/crypt_des.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/crypt/crypt_des.c b/src/crypt/crypt_des.c
> > index 38e63395..ae8dd2e8 100644
> > --- a/src/crypt/crypt_des.c
> > +++ b/src/crypt/crypt_des.c
> > @@ -672,6 +672,15 @@ static inline int ascii_is_unsafe(unsigned char ch)
> > return !ch || ch == '\n' || ch == ':';
> > }
> >
> > +/*
> > + * We only allow noncanonical salt characters for checking existing
> > + * hashes, not producing new ones.
> > + */
> > +static inline int ascii_is_noncanonical(unsigned char ch)
> > +{
> > + return ch!='.' && ch!='/' && (ch|32)-'a'>=26U && ch-'0'>=10U;
> > +}
>
> I've tested that this function works as intended:
>
> #include <stdio.h>
>
> static inline int ascii_is_noncanonical(unsigned char ch)
> {
> return ch!='.' && ch!='/' && (ch|32)-'a'>=26U && ch-'0'>=10U;
> }
>
> int main(void)
> {
> unsigned char ch = 0;
> do {
> if (!ascii_is_noncanonical(ch))
> putchar(ch);
> } while (++ch);
> putchar('\n');
> return 0;
> }
>
> This prints just the expected 64 characters:
>
> ./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz
Thanks for checking!
> > static uint32_t setup_salt(uint32_t salt)
> > {
> > uint32_t obit, saltbit, saltbits;
> > @@ -978,6 +987,12 @@ static char *_crypt_extended_r_uut(const char *_key, const char *_setting, char
> > *p++ = ascii64[l & 0x3f];
> > *p = 0;
> >
> > + if (ascii_is_noncanonical(output[0])
> > + || ascii_is_noncanonical(output[1])) {
> > + if (output[0] != '_' && strcmp(output, _setting))
> > + return NULL;
> > + }
> > +
> > return output;
> > }
>
> Note that we're leaving the computed hash somewhere in memory, orphaned.
> We could want to zeroize it. Ditto on self-test failure, but that's
> unexpected in a correct build.
>
> I'd move the "output[0] != '_'" check to the outer "if" and write it
> using _PASSWORD_EFMT1 to be consistent with our other checks for this.
I put it as 2 separate ifs rather than combined in 1 to make the mixed
&& and || logic less confusing to read, and put the check for
noncanonical and then '_' first so that strcmp is never called unless
we have a noncanonical hash, so that there is no timing vector for
good salts.
How about:
if (output[0] != _PASSWORD_EFMT1 &&
(ascii_is_noncanonical(output[0]) ||
ascii_is_noncanonical(output[1]))) {
if (strcmp(output, _setting))
return NULL;
}
The nested if vs another && clause doesn't really make sense here, but
see below...
> 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.
> Our checks of the salt characters were not timing-safe in other places
> anyway, so that wouldn't be fundamentally a new problem here. But if we
> potentially proceed to check the actual hash, that's new.
>
> Having all of this said, I have no objections to a change like this.
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.
Rich
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.
