[<prev] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20261114084407.GB4188@openwall.com>
Date: Sat, 14 Nov 2026 09:44:07 +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 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.
> 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
> 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.
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.
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.
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.
