[musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Runxi Yu]

Hi,

Test_User <hax@runxiyu.org> discovered that using passwd(1) from
shadow-utils on Alpine Linux 3.21 with a password longer than 256
character causes a segmentation fault.  I reported it to
<https://gitlab.alpinelinux.org/alpine/aports/-/issues/16784>, and
people in #alpine-linux helped a bit.

It turns out that musl's crypt(3) returns a "*" from a string literal,
which PAM attempts to erase with pam_overwrite_string.

(Irrelevant lines truncated)

musl/src/crypt/crypt_sha512.c
> 	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
> 		return "*";

pam/modules/pass_unix/passverify.c
> 	sp = crypt(password, salt);
> 	if (!sp || strncmp(algoid, sp, strlen(algoid)) != 0) {
> 		pam_syslog(...);
> 		if(sp) {
> 		   pam_overwrite_string(sp);  /* <-- attempt to overwrite musl's string literal */
> 		}
> 		return NULL;
> 	}

https://pubs.opengroup.org/onlinepubs/9799919799/functions/crypt.html:
> The return value of crypt() points to static data that is overwritten
> by each call.
> 
> Upon successful completion, crypt() shall return a pointer to the
> hashed password; the first two bytes of the returned value shall be
> those of the salt argument. Otherwise, it shall return a null pointer
> and set errno to indicate the error.

I think musl's behavior of returning "*" is incorrect. It's rather
reasonable for the caller to attempt to erase whatever is returned by
crypt, and AFAIK there is no specification that allows returning "*" as
a failure return value.


Note: I am not on the mailing list; please CC me in replies.

--
Best regards,

Runxi Yu (they/them)
Year 11, E House
YK Pao School SJ
https://runxiyu.org

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Markus Wichmann]

Am Sun, Dec 29, 2024 at 04:38:03PM +0800 schrieb Runxi Yu:
> musl/src/crypt/crypt_sha512.c
> > 	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
> > 		return "*";

It doesn't make sense for these lines to be the problem, because they
are only triggered if the compiler used for musl was broken and created
a version of sha512crypt that generates wrong hashes. If this path is
ever taken, then it is better for passwd to crash than to use any part
of the hash.

Ciao,
Markus

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Rich Felker]

On Mon, Dec 30, 2024 at 07:46:21AM +0100, Markus Wichmann wrote:
> Am Sun, Dec 29, 2024 at 04:38:03PM +0800 schrieb Runxi Yu:
> > musl/src/crypt/crypt_sha512.c
> > > 	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
> > > 		return "*";
> 
> It doesn't make sense for these lines to be the problem, because they
> are only triggered if the compiler used for musl was broken and created
> a version of sha512crypt that generates wrong hashes. If this path is
> ever taken, then it is better for passwd to crash than to use any part
> of the hash.

Indeed. I think there's a good chance we should revise the decision
not to return an error from the crypt interfaces (opting instead to
return unmatchable hash), but this is not the relevant point in the
code, and regardless, the code that's trying to overwrite the
unknown-size buffer returned by crypt is certainly in the wrong and in
need of fixing, independent of whatever changes we might make.

Rich

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Thorsten Glaser]

On Mon, 30 Dec 2024, Rich Felker wrote:

>Indeed. I think there's a good chance we should revise the decision
>not to return an error from the crypt interfaces (opting instead to

crypt(3) is defined to return a nōn-constant string (bad interface,
yes) in case of success ONLY and NULL in case of error, so yes, do.

>return unmatchable hash), but this is not the relevant point in the
>code, and regardless, the code that's trying to overwrite the
>unknown-size buffer returned by crypt is certainly in the wrong and in
>need of fixing, independent of whatever changes we might make.

Given it’s defined as returning a writable, NUL-terminated, string,
the code works under acceptable assumptions, and I don’t think it
should need to change. (Not my code, and I probably wouldn’t write
that, but…).

bye,
//mirabilos
-- 
(gnutls can also be used, but if you are compiling lynx for your own use,
there is no reason to consider using that package)
	-- Thomas E. Dickey on the Lynx mailing list, about OpenSSL

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Markus Wichmann]

Am Mon, Dec 30, 2024 at 07:46:21AM +0100 schrieb Markus Wichmann:
> Am Sun, Dec 29, 2024 at 04:38:03PM +0800 schrieb Runxi Yu:
> > musl/src/crypt/crypt_sha512.c
> > > 	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
> > > 		return "*";
>
> It doesn't make sense for these lines to be the problem, because they
> are only triggered if the compiler used for musl was broken and created
> a version of sha512crypt that generates wrong hashes. If this path is
> ever taken, then it is better for passwd to crash than to use any part
> of the hash.
>
> Ciao,
> Markus

I stand corrected. I neglected to look at the "!p" condition. While the
other two conditions for this return are indeed internal error checking
(and maybe crashing explicitly here would be a better solution), the
first one comes from a few conditions inside of sha512crypt, including
keys beyond 256 bytes.

There is no justification for length limits on the password, and nor
for a length limit at exactly 256 bytes. Would CPUs overheat at 257?

Ciao,
Markus

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Rich Felker]

On Tue, Dec 31, 2024 at 09:35:27AM +0100, Markus Wichmann wrote:
> Am Mon, Dec 30, 2024 at 07:46:21AM +0100 schrieb Markus Wichmann:
> > Am Sun, Dec 29, 2024 at 04:38:03PM +0800 schrieb Runxi Yu:
> > > musl/src/crypt/crypt_sha512.c
> > > > 	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
> > > > 		return "*";
> >
> > It doesn't make sense for these lines to be the problem, because they
> > are only triggered if the compiler used for musl was broken and created
> > a version of sha512crypt that generates wrong hashes. If this path is
> > ever taken, then it is better for passwd to crash than to use any part
> > of the hash.
> >
> > Ciao,
> > Markus
> 
> I stand corrected. I neglected to look at the "!p" condition. While the
> other two conditions for this return are indeed internal error checking
> (and maybe crashing explicitly here would be a better solution), the
> first one comes from a few conditions inside of sha512crypt, including
> keys beyond 256 bytes.
> 
> There is no justification for length limits on the password, and nor
> for a length limit at exactly 256 bytes. Would CPUs overheat at 257?

There absolutely is a justification for the length limit. The
laughably bad design of the sha256/512 password hash functions is
gratuitously O(n²) in password length. So some limit needs to be
imposed, and 256 was chosen as a number way larger than any reasonable
password someone could want. Sure there's no reason it couldn't have
been 257 or 300 or whatever, but either way you'd get things like this
when software has bugs in handling the condition.

Rich

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Quentin Rameau]

Hi Thorsten,

> Given it’s defined as returning a writable [...] string,

Could you explain where this is defined?
Is it derived from general rule?

Thanks!

Re: [musl] crypt(3) returns "*" from read-only region, segfaulting passwd(1) on Alpine for long passwords

[Thorsten Glaser]

Quentin Rameau dixit:

>> Given it’s defined as returning a writable [...] string,
>
>Could you explain where this is defined?

The manpages document it as returning a 'char *' and,
at the same time, do not explicitly forbid writing to
that location.

I’m certain that this is only due to the age of the API,
but it won’t do to break it now.

bye,
//mirabilos
-- 
Gast: „Ein Bier, bitte!“
Wirt: „Geht auch alkoholfrei?“
Gast: „Geht auch Spielgeld?“