← Back to context

Comment by dboreham

2 days ago

> musl can return “*” for errors

wtaf ?

There is no doubt in my mind that this was intentional.

  • I agree. The “excuse” for implementing those behaviors is just providing plausible deniability. The commit that lets crypt return “*”, followed closely by a change that lets you easily trigger that behavior sounds just like how somebody would sneakily introduce a back-door into tons of linux systems while making it look as innocent as possible.

  • Well, it was intentional, but not in the way I think you mean.

    musl switched to returning `*` so folks stopped accidentally using NULL results, which was causing crashes and risked false-positives comparing NULL to itself.

    That was a misfeature and should never have been implemented. But I bet it was an easy band-aid change to make!

    • But choosing "*" as the character to return is an insane choice for anyone who knows the basics of linux sysadmin, or hell anyone who's ever used a search field in any context and thus knows it is ubiquitously used as a wildcard operator.

      4 replies →

    • > musl switched to returning `*` so folks stopped accidentally using NULL results, which was causing crashes and risked false-positives comparing NULL to itself.

      Of all the things to mitigate...

      1) crashing should be an option - it noisily indicates "there's a problem" and if it faults and dumps core, you've got a guided tour of your problem.

      2) Is testing NULL as a passable option likely? I'd guess a more likely idiom is:

        if(NULL==(str=somefunc(arg1, arg2))){
          return err;
        }

      8 replies →

    • As opposed to the false positives returned by comparing '*' to itself.

      Why * specifically? Why not some random guid generated before committing, if it's so important to not let buggy software crash?

      20 replies →

  • Of all the possible strings this seems like the absolute worst choice. If I were typing that string I'd pause to think "wait, what if the caller uses this with a regex?"

One has to remember the context here. The only error possible according to the SUS is ENOSYS, literally that crypt() is not actually implemented in the library. The traditional AT&T Unix crypt() was always implemented, and its encryption did not have failure modes, so there was never any error return to handle. This is why programs were written to never check the result of crypt(). It was only ever NULL on systems where one couldn't use the mechanism at all in the first place.

So the knock-on effect is that libraries that do have errors other than ENOSYS to accommodate, have to return magic 'impossible' values instead of setting errno and returning NULL, because of the weight of existing applications that expect nothing but the standard, traditional AT&T Unix (and indeed old BSD), behaviour.

Check the GNU libc crypt() doco. It defines a whole family of 'impossible' values that signal errors, by dint of listing a load of magic characters that should never be in the encrypted string. Including asterisk.

Also check FreeBSD's OpenPAM for a case in point. Even today, it does not check for a NULL return from crypt() and leaps straight to strcmp().

* https://cgit.freebsd.org/src/tree/lib/libpam/modules/pam_uni...

If you want to see real history here, check out the 1992 comment by Eric Andrew Young, that is still in the source for OpenBSD today (in des/des_fcrypt.c), on how SSLeay's crypt() had to be adjusted so that any single character password field, including "*", did not end up successfully matching itself.

* https://cvsweb.openbsd.org/annotate/src/lib/libcrypto/des/At...

It's worth pointing out that musl tried to do the same as SSLeay, and deliberately returns something different to the original salt so that they will not compare equal. It does not return "*" if the input is actually "*" in the first place, contrary to the headlined cartoon explanation. (The locus of the problem is the caller calling it with a salt that is not "*" and then comparing it with "*".)

  • > Check the GNU libc crypt() doco. It defines a whole family of 'impossible' values that signal errors, by dint of listing a load of magic characters that should never be in the encrypted string. Including asterisk.

    No. https://man7.org/linux/man-pages/man3/crypt.3.html

    My guess is that you are reading the libxcrypt crypt(3) manual: https://www.mankier.com/3/crypt

    That's not GNU libc, but frequently deployed on Linux systems.

  • > Also check FreeBSD's OpenPAM for a case in point. Even today, it does not check for a NULL return from crypt() and leaps straight to strcmp().

    Damn, that's a bug! Please file bug report.

    • I suggest finding a code path where FreeBSD crypt() ever returns NULL, which would mean one of its internal functions returning non-zero, before getting too excited. They all end in return (0) . (-:

      * https://cgit.freebsd.org/src/tree/lib/libcrypt

      As I said, the callers of crypt() have for a long time been using the knowledge that crypt() on their platform simply never returns NULL. Unix has never been returning NULL from crypt() as far back as 7th Edition, when it simply always returned a pointer to a single internal static buffer.

      * https://github.com/dspinellis/unix-history-repo/blob/Researc...

      The reality is that all C library people have to work with a corpus of Unix applications that do not check for crypt() returning NULL that goes back almost 50 years, and an error return mechanism that barely exists outwith the pages of the SUS.

      That includes callers in single-user mode in process 1 in some configurations.

      * https://cvsweb.netbsd.org/bsdweb.cgi/src/sbin/init/init.c?an...

      2 replies →

  • The most charitable interpretation here is that musl authors are incompetent.

    There is simply no universe in which a security-conscientious reviewer looks at the code, spots * being a valid return value, remembers that * is the default value in /etc/shadow for "no password login please" and then is all like:

    This is fine, you check for * and return + instead.

    There are more 32-long character sequences than atoms in the universe. Pick one, any of them instead.

Returning a string on error honestly is defensible (especially in a language like C where the type system doesn’t have exceptions or result types), the question is why the string was “*” instead of like “crypt ERROR: input password cannot be longer than 256 characters” lol

  • No. Musl claims (claimed?) to be strictly POSIX compliant, and POSIX requires crypt to return NULL on errors.

  • "*" is what's been historically used to represent an invalid/no value in passwd/shadow, so it makes some sense.

    Of course given the chance that one might do a direct string comparison between crypt's return and the passwd/shadow file's hash field it was still a not well thought out choice.