Hacker News

points by za_creature 2 days ago | 0 comments

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.

[–] zbentley 2 days ago

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!

[–] ghostly_s 2 days ago

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.

[–] pdw 2 days ago

But it fits the common Linux implementation of the function... e.g. the Debian manpage says:

> Upon error, crypt_r, crypt_rn, and crypt_ra write an invalid hash to the _output_ field of their _data_ argument, and crypt writes an invalid hash to its static storage area. This string will be shorter than 13 characters, will begin with a ‘*’, and will not compare equal to _setting_.

[–] crunchatized 12 hours ago

That says 'begin with,' which does not remotely require it be exactly the 1-character string '*' which probably was a bit of an insane choice under GP's logic.

And that's also only in Debian 11 and later. That man page said something entirely different in Debian 10 and earlier, when they shipped glibc's libcrypt. Now they ship libxcrypt instead.

Fedora asked libxcrypt to give them back glibc's behavior, returning NULL, and they obliged by adding ./configure --disable-failure-tokens. But it's an option that Debian never enabled.

https://github.com/besser82/libxcrypt/issues/25

https://salsa.debian.org/md/libxcrypt/-/blob/master/debian/r...

[–] cloudfudge 2 days ago

It's almost sensible if you only think of one side. It's like if /etc/passwd put "NO_PASSWORD" in the field for disabled logins and someone was like, oh, I'll be clever and say crypt() returns "NO_PASSWORD" on error because that's obviously invalid and won't match. But that's exactly what *does* make it match.

[–] bryan_w 2 days ago

But this also needs the "ddos" patch that was submitted later as well.

That seems orchestrated.

[–] bryan_w 2 days ago

Also, from the patch author:

https://lists.llvm.org/pipermail/llvm-dev/2019-June/133308.h...

[–] bch 2 days ago

> 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;
  }

[–] xp84 2 days ago

> crashing should be an option

Wise words. I think early in my career I tended towards coding as defensively[1] as possible in a very specific way of "don't crash", as I definitely was not working in codebases I felt I had a full picture of. "I don't know who calls this method, and I don't want to get a call in the middle of the night that I broke prod" so I would say, return an empty string when asked to format a datetime except I'm passed a string or an int. (Yes, I was working in dynamic, untyped languages, too).

Later on, even in untyped languages, I came around more to "Throw loud and specific exceptions when asked to do things that are nonsensical" - it requires more overall cojones though because it may cause "your code" to crash. But the realization I came to was that it's better to crash ('a MaliciouslyLongPasswordError was uncaught') because "AuthManager asked PasswordService to check a password that was 2MB long" than to paper that over until something like this comes out. Or, the more common situation, you're passed garbage, you return something that won't choke the caller at the time, but what it saves to the database in that scenario ends up being corrupt data. 6 months later someone notices, and has to sort out a data mess that may or may not be recoverable.

[1] note that most "code defensively" advice is good. What I was doing could be thought of as a foolish misinterpretation of the phrase.

[–] oasisaimlessly 1 day ago

FYI, your learned approach is also known as "fail fast" methodology[1][2].

[1]: https://www.martinfowler.com/ieeeSoftware/failFast.pdf

[2]: https://en.wikipedia.org/wiki/Fail-fast_system

[–] zbentley 2 days ago

“A good offense (SIGABRT) is the best defense(ive code).”

I dig it.

[–] PokestarFan 2 days ago

I think the issue is that a lot of callers were not aware that crypt() could error at all and all the issues are downstream of that. If OpenSSH knew that crypt() could error they would have handled it (possibly by inspecting errno).

[–] creata 2 days ago

I don't think that's the problem. The man page for crypt clearly says that erroring is a possibility and that it's not standardized how it errors:

> The behavior of crypt on errors isn't well standardized. Some implementations simply can't fail (except by crashing the program), others return a null pointer or a fixed string. Most implementations don't set errno, but some do. POSIX specifies returning a null pointer and setting errno, but it defines only one possible error, ENOSYS, in the case where crypt is not supported at all.

In my opinion the problem is that the behavior of crypt is underspecified - it isn't even specified how it signals failure, which is shocking.

Another disturbing decision that crypt's signature takes in the hash its output is supposed to be compared against - if it weren't for that, this half-baked "on error the output won't compare equal to the input" measure that crypt and crypt_* use wouldn't have been possible.

You can't check errno to see if there was a failure because, as the man page says, the value of errno is unspecified when crypt succeeds.

[–] throwaway2037 2 days ago

    > You can't check errno to see if there was a failure because, as the man page says, the value of errno is unspecified when crypt succeeds.

You can set errno to zero before the call. If the value is non-zero after the call, that is a good heuristic for failure.

[–] zbentley 2 days ago

...unless crypt() is one of the many, many functions which themselves internally call functions that set errno. In which case you can't be sure.

[–] za_creature 2 days ago

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?

[–] zbentley 2 days ago

I'm agreeing with you, to be clear.

But buggy software might not crash; it might compare NULL == NULL and misbehave, instead.

So there's really no good option here, which is why C is a rancid choice of language for publicly-usable APIs. But we're stuck with it. So it goes.

[–] dboreham 2 days ago

I've been programming in C since 1978 and never until now heard of a function returning "*" on error.

[–] zbentley 2 days ago

This specific manifestation of "C is terrible for shared code APIs" is rare.

But issues caused by folks forgetting to check errno, miscomparing NULL to NULL when calling multiple APIs, dereferencing external-API-returned NULL due to missed error checking, mishandling inherited errno after successful action, or--the most common--mishandling negative integer return sentinels meant to indicate errors? Those are incredibly common.

[–] za_creature 2 days ago

This specific library call is quite poorly designed.

Standard C api calls return an integer, which is 0 on success or nonzero on error. Ignoring it is an intentional choice. `errno` is the hack that shouldn't really exist because it's trivially easy to forget to check that and also has issues with threading (patched by making it a thread local) and more recently, coroutines (which admit no general purpose patch because they're a userspace construct).

Another reason why this is terrible is that it's unclear what the returned pointer's lifetime is. Is it until the next call to crypt? If so, is it thread-local like errno? If it's passed on to the user, is it safe to call free() on it, or is there a crypt_free function that should be called instead?

But all of this, and I mean all of it pales in comparison to returning '*' on error.

[–] zbentley 2 days ago

> But all of this, and I mean all of it pales in comparison to returning '*' on error.

100% agree. This is an egregious failure. But C's error-handling story would be bad even without this, and the badness of C's error handling is what motivated the (bad) decision to return *.

It'd be great if we wrote systems software in a language that supported advanced features like...

checks notes

...returning multiple values.

Hell, even in VBA you had to at least spend the keystrokes to type "on error resume next".

[–] tredre3 2 days ago

It's not a C thing, it's a Linux thing. The asterisk is relevant to how crypt() is used for /etc/shadow.

           If the password field contains some string that is not a valid result of crypt(3), for instance ! or *, the user will not
           be able to use a unix password to log in (but the user may log in the system by other means).

[–] zbentley 2 days ago

No, it's a C thing. They previously returned NULL for invalid passwd lookups, but because there's no way in C to force API users to decide what to do if an error occurrs, that was causing program crashes due to NULL dereferences.

So they made the (very bad, but motivated by C's broken design/conventions) decision to return a magic string instead.

[–] ruslan 2 days ago

> but because there's no way in C to force API users to decide what to do if an error occurrs,

0. There are tons of libc functions that return NULL on failure, it is conventional way in C to indicate failure.

1. There's no much to decide, just fail to login if crypt() fails with NULL.

2. errno is standard way in C to check error code of a failing function, but in case of login you hardly need this, just fail!

I totally agree that fixing this issue in musl was a pretty dubious solution. Don't fix what's not broken, fix broken application instead. Publish a CVE if you cannot fix it.

I looked through FreeBSD source code for how they use crypt(), it's checked for NULL everywhere!

UPD: Except libpam. :(

[–] gpvos 2 days ago

Returning NULL on error is actually an excellent choice because most programs will crash if they try to use it without checking. A program, any program, crashing is better than this bug. Of course, modern programming languages have better constructs, but this was rare to nonexistent when C was started and if you think C is going away anytime soon you're delirious. musl's decision to change the return value to "*" is ununderstandable and likely intentionally evil.

[–] nuxi 2 days ago

Now imagine every other C API that can potentially return NULL did the same thing. Is that in any way sensible?

You can argue that C has broken design/conventions, but the musl decision was really really awkward.

[–] patmorgan23 2 days ago

This sounds like a program problem not a library problem. Why did the library try to solve it?

[–] za_creature 2 days ago

Oh, it was magic alright.

[–] cowlevel 2 days ago

C programs wouldn't accidentally compare NULL to NULL. They'd either check for null explicitly, or they'd pass the null to strcmp and crash.

[–] nextaccountic 1 day ago

Yep. See: underhanded c code contest

https://www.underhanded-c.org/

[–] dboreham 2 days ago

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?"