[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20261107135129.GE23438@brightrain.aerifal.cx>
Date: Sat, 7 Nov 2026 08:51:29 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] [RFC] Harden against misuse of crypt
On Sat, Nov 07, 2026 at 01:11:58PM +0100, Szabolcs Nagy wrote:
> * Rich Felker <dalias@...c.org> [2026-11-06 18:18:34 -0500]:
> > On Sat, Nov 07, 2026 at 12:02:59AM +0100, Szabolcs Nagy wrote:
> > > as a friend put it, if the goal is to minimize the likelihood
> > > of the failure token matching a passwd entry then it is
> > > possible to do better than "*0": e.g. 100 non-printable bytes
> > > (in case we really want to be defensive about misuses and typos
> > > in passwd files)
> >
> > Well there are multiple goals. We want a passwd utility to be able to
> > conclude that setting the password failed; I assume they do that now
> > by something like strlen(hash)<13.
> >
> > Tthe smart way would be checking whether the application of
> > crypt(pass,ยท) is idempotent - IOW, if f(h)=crypt(pass,h), does
> > f(f(h))==f(h)? This is directly testing "will I be able to get back in
> > with this password and hash?" But I doubt that's what anything does.
>
> fwiw busybox passwd accepts pretty much anything !=NULL into
> the passwd db (one more reason to use NULL)
>
> https://github.com/vda-linux/busybox_mirror/blob/master/libbb/pw_encrypt.c#L161
>
> pw_encrypt:
> encrypted = crypt(clear, salt);
> /*
> * glibc used to return "" on malformed salts (for example, ""),
> * but since 2.17 it returns NULL.
> */
> if (!encrypted || !encrypted[0])
> bb_simple_error_msg_and_die("bad salt");
>
> https://github.com/vda-linux/busybox_mirror/blob/master/loginutils/passwd.c#L88
>
> new_password:
> ret = pw_encrypt(newp, salt, 1);
> /* whee, success! */
I think we should try to get this fixed, ideally with the rigorously
correct check (idempotency). But I wouldn't be surprised if they fuss
about the +100 bytes of code size or whatever (note: it requires
making a copy of the output unless you're using crypt_r) and want to
just check strlen(encrypted)<3 or <13 instead...
> > In any case, even if the output is "100 non-printable bytes", there is
> > still the risk of a bad passwd-like utility putting it in the
> > shadow/passwd file. That's no different from "*0". The only way that
> > could be avoided is by making the output entirely nondeterministic,
> > e.g. 256 bites of getentropy output encoded base64. But I don't think
> > that's a reasonable behavior.
>
> at passwd setup something has to go wrong (salt or too
> long pass) for a crypt failure token to end up in passwd
> db, but "*0" can also end up there as a typo. arguably
> either case is unlikely enough to not be an attack target,
> but if we want to minimize the likelihood of
> strcmp(crypt(x,y),z)==0 when y!=z then i think 100*'\001'
> is better than "*0" as the latter can end up in z in more
> ways. but i won't press this if we move to NULL.
I'd really rather not be gratuitously different on this and take the
false (or deserved, if we botch it somehow) blame if there's a bad
interaction with something out there and it ends badly.
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.
