OK, I think I see what’s going on with this, in this configuration.

The goal from an attacker’s perspective is to get this to be true: `$ret = crypt($pw, $hash); $ret == $hash` when called by OpenSSH’s sshd. This is how sshd validates that the user logging in knew the account password.

The `$hash` is the “settings” to the crypt call and will normally look like “$6$--gibberish” for users with passwords set, but could look like “*” or “!” for users not intended to be able to login.

The bug is from the intersection of all of these things:

1. musl can return “” for errors, rather than leave the return value NULL. Other libc have adopted something like this too, but seem to use a different result “0”.

2. musl can also return “+” for errors, but only does so if $hash starts with “*”. This is intended to eliminate the possibility of an error causing `$ret == $hash` to be true, however it assumes that applications calling `crypt()` will set $pw and $hash to values corresponding to the `$ret == $hash` comparison to be made immediately afterwards.

3. OpenSSH can be convinced to pass the hash of a different user if the hash is “*” or “!” to avoid timing attacks that can enumerate the user list, so it will set $pw and $hash to correspond to different users. $hash will be a real “$6$--gibberish” style hash in this case, and OpenSSH expects `crypt()` to either return NULL or return a hash string. But now there’s two hashes OpenSSH uses. It does: `$ret = crypt($pw, $hash2); $ret == $hash1`;

4. This should normally not be a problem because OpenSSH will pass in valid $pw and $hash2 that should not lead to errors in hashing, so the $ret that musl generates should be a real hash that cannot match the “*” or “!” $hash1 for the login-disabled user. However as a DDoS mitigation, musl will return error if the password is too long when using SHA-2-based hashes (the default, it seems).

5. This error will result in a non-NULL result of “” or “+”. Since OpenSSH passed in a real hash ($hash2), musl will return “”, which compares equal to the actual user hash $hash1 for the disabled-login user.

The OpenSSL adjustment ensures a minimum hash length for the user before logging them in, even if crypt() succeeds somehow.

I think the conclusion that can be drawn from this is that the behavior of crypt() is howling-at-the-moon insane, and it should no longer be considered for any system built in this century, at least not without extensive wrappers that correct and normalize all these edge cases.

Conclusion is type safety is important

In this particular case, it is type safe. NULL, by the C standard does not refer to an object. It means that every pointer in C is effectively an option type, which mean that a function that returns a pointer can return NULL for "no result", which, in the case of crypt() is perfectly adequate.

It is the opposite that is unsafe, that is, when the returned value can't be NULL, but it stills returns a pointer. C++ has references in an attempt to fix this problem.

Because C tends to prioritize performance over safety, a NULL pointer dereference is considered undefined behavior and the compiler can invoke nasal demons if it makes the code go faster, but with the right options, you can have it checked and invoke what in other languages would call a panic handler, but on UNIX is the SIGSEGV signal. You can then attach a debugger or read the core file and see what went wrong.

Note: I am not talking about "char *" for strings, that one is unsafe, but NULL is not. It is just a special case of an option type, or a result type, or an error union. Modern languages just extended this idea to objects other than pointers, and with more than one type of error.

> It is just a special case of an option type, or a result type, or an error union.

These are not the same, and your next sentence suggests why. Option has but a single error type, but result types and error unions allow arbitrary types of error.

> the compiler can invoke nasal demons if it makes the code go faster

I had never heard that phrase, and was delighted at the answer when I googled it its origin, thank you:

http://www.catb.org/jargon/html/N/nasal-demons.html

> “When the compiler encounters [a given undefined construct] it is legal for it to make demons fly out of your nose” (the implication is that the compiler may choose any arbitrarily bizarre way to interpret the code without violating the ANSI C standard)

However this is limited by the compiler's actual abilities and code. The C standard doesn't forbid nasal demons, but the compiler isn't programmed to invoke nasal demons and isn't running on hardware with that capability, so it won't happen.

I've heard nasal demons invoked as a literal possibility. No they are not a literal possibility.

You might consider it type safe according to C's rules, but it is not type safe according to contemporary opinion because the success type and error type are the same.

> [...] because the success type and error type are the same.

That's a bit weird? In Rust, if you have an thing that returns Result<String, String> that's still type safe, even though success and error types are the same.

I think I understand what you want to express, maybe. You might say it's in-band vs out-of-band error signalling?

> In Rust, if you have an thing that returns Result<String, String> that's still type safe, even though success and error types are the same.

These types are not the same:

    Ok<String>
    Err<String>

Contemporary opinion among who? Words mean things, and "type safe" simply does not mean "agrees with my preferred style of error reporting"

Papers on type soundness from the last 30 years usually explore the domain further than C semantics.

Soundness means that the program behaves in accordance with what its types express. You're right that C may be unsound due to undefined behaviour, but I don't think this is about soundness. What you're saying is that papers on types show how you can use types to express more constraints.

There can be other valid perspectives and definitions of that word than yours.

I think almost all (tech) arguments stem from simple disagreements and/or different assumptions made on the definition of a (subjective) term.

You may be right in general, but soundness comes from formal logic, where they're pretty fastidious with their definitions. Soundness certainly doesn't mean the expressivity of the types (i.e. what constraints they can represent); it's about the relationship between the runtime behaviour of a program and the types in the code.

Can you link some?

That's not the same as asserting that soundness requires adherence to the current fashion, sum types over results and errors. TIMTOWTDI.

You don't have to think C is sound or good to distinguish a disfavored API style from mathematical unsoundness. There are infinitely many type safe APIs and languages that are, despite their soundness, ugly and hard to use.

Let's not project every dimension along which a language can be good or bad onto the axis of safety.

My thoughts exactly.

"Make invalid states unrepresentable" is such a useful adage when designing even basic systems, much less the gates to control your server.

My conclusion is that an old tradition of not returning NULL in crypt as a workaround for buggy code which doesn't handle errors was a bad idea.

The idea of trying to provide a workaround for buggy code calling crypt() is terrifying. For sure, trying to grease the skids for buggy code calling printf(), go for it. But it is hard to imagine cases where slightly-incorrect usage of crypt() is a good idea.

This is still a consequence of a poor type system.

I don't think so. Even in a language with a good type system like Rust, somebody could have made this API:

    /// Hash the phrase with the given setting.
    fn crypt(phrase: &[u8], setting: &[u8]) -> [u8; CRYPT_OUTPUT_SIZE];

Then, in a later version, `crypt` needs to become fallible, so they need to signal errors somehow. But because they really want to preserve backward compatibility, they can't change the output type to `Result<..., ...>`. And so they end up doing the same thing that was done in libcrypt - they output some error value and hackily make sure it's not equal to the `setting` string, under the mistaken assumption that the output of `crypt` would only ever be compared to the `setting` string. And then someone creates the exact same bug.

Maybe I'm wrong, but it looks to me like the exact same incentives and means to screw things up exist in C and in Rust for this particular bug.

IMO type safety requires both language features and a mindset where you take efforts to keep things type safe. Your example has only the former.

For example, in a Rust web app, you can choose to use String for encoded URLs, unencoded URL fragments, escaped html, unescaped HTML, usernames, etc. Or you can choose to use newtypes and carefully design all APIs to only take and return the correct newtypes.

You are looking at it from the wrong direction.

The problem is not that you can write bad stuff in rust, the problem is that this particular usage pattern can't be sanely coded in C. This is a public API that should inform users and enforce certain properties. A Results type does exactly that, you would have to go way out of your way to force invalid stuff through that type alone.

This isn't good either. Crypt is a pure computation that can't fail for arbitrary reasons. The former implementation could fail if the salt was invalid, which is something you could guarantee. Now it also fails if the password is invalid. That's new behaviour and no matter which way you wrap it up it has the potential to be incompatible. They should have added the maximum length recommendation to the man page, and let DoS attacks continue until the calling software was updated, not the crypt library.

I don't think so. In both Rust and C, the solution is the same - make the function explicitly fallible, instead of trying to preserve backward compatibility. In C, the function would probably just look like this, returning zero on success and negative on failure:

    int crypt2(const char *phrase, const char *setting, char *out);

You can call such a signature "insane" if you want, and that's fine, but it would have averted the bug that is the topic of discussion.

what you are suggesting is a form of type safety. There are still degrees of type safety even in C, and this bug resulted from deciding that backwards compatibility is more important than type safety, which I think we agree on.

And C has effectively deprecated bad APIs over the years. Just not this one.

It's a perfectly sane signature, especially coupled with an API surface that uses the return channel for errors generally.

They would have to make it crypt2.

Yes, even in Lean you can just refuse to prove any properties.

But if you have a stronger and more expressive type system, you are more inclined to use it, instead of looking for silly workaround like in C.

You wouldn't be concerned about breaking backward compatibility with rust though, would you? A recompile would be needed anyway and the change would be detected and enforced. The rationale for the bad API here (two types in one variable) was that users were ignoring NULL - that's the part which wouldn't happen in rust.

Maybe he meant binary compatibility.

Specifically, that the lack of tagged unions (or really, structured return types in general; you can pretend those are a tagged union if you believe hard enough) in C library interop conventions is a terrible foundation on which to build widely-used APIs.

And that just ends up indicting the C language itself. Can't send errors down a separate channel; folks might not inspect errno. Can't return NULL on error, folks might compare it blindly to another result. No tagged unions, so can't return one. Can't crash on error because there's no scoped error handling. Can't return a struct for checking .error/.result separately, because alignment and equality (and tearing and accidental clobbering and more) make that brittle as hell for widely-used APIs.

C is not a good language for library APIs. Like, really not. It has its uses/strengths, sure, but without even getting into the typical gripes people have about C, libraries--one of the things that it is used for the most--are one of the things for which it is the least well-suited.

"But it has a stable ABI!" So does a land mine.

you can return a struct just fine... most API's don't though.

Historically it didn't work well between different compilers. Maybe it still doesn't.

That and in-band signalling is error-prone.

Another example that C programmers love: using 0 to mean infinity. Eg when you have a timeout setting, and 10 means ten seconds, but 0 means infinity seconds instead of zero seconds.

In general it's a problem of sentinel values. Some languages let you abstract this - I think in Rust and C++ you can define an "intrusive optional" which is like optional but uses an invalid value pattern to store the not-present state. In memory it's the same as a sentinel value but the compiler forces you to unwrap it like an optional.

That’s just a very bad API. You could do the same in Rust? There are nice C APIs that would use a nice struct that describes the timeout you want… eg GCD.

Is really the solution to a bug where a function had a wrapper that tried to solve its documented issue of returning *, to make yet another wrapper to try to solve the documented issue of returning true?

But, but 'always "succeeding" but giving an unmatchable hash is reportedly a better behavior than failing on error.'!!

Caveat:

> $ret = crypt($pw, $hash); $ret == $hash

musl is actually safe against this use. OpenSSL takes a bizarre code path, detects that $hash is too short, calls crypt($pw, $other_string), forgets that it detected that the hash was too short, and checks $hash == return value.

See my other comment.

I did note that musl would normally work just fine here. Though I appreciate that your other comment goes into much better detail about what was actually going on in the code, as my analysis basically stopped once I understood the high-level flow of what was going on.

This is a vastly clearer explanation than the linked page. Thank you!

Yep, the linked page doesn't /feel/ AI written (maybe it's using a grok or muse model whose tone I don't quite recognize yet?) but it kinda just states a lot of facts that make sense on their own but don't cohere into a whole vulnerability like the comment above does.

I think it's just Claude prompted to write in a casual, conversational, no caps style.

The surface-level hints are gone, but the attempt to make every sentence pithy, the decisions on what information to present and what to assume the audience knows, and the over-emotionalization are all still there.

Of course it is. "one debian box, one alpine box" is an obvious Claudeism.

and what same human would embed two linux systems (??) in a blog post just for a demo that prints "pass"/"fail" that hardly anyone will bother to interact with?

Because Claude wrote the linked page. Claude is impressively bad at communicating these days. with gpt-5.6+ approaching Claude level like 30% of time and Gemini much clearer but also the most annoying 7 year old ever, it’s a pretty lame environment especially for people who r new to everything

I wondered about this myself as the explanations and documents I have been maintaining are becoming increasingly 'slop', but a different kind of slop.

In the past I could get an explanation, where I was able to track the line of thought. Now when I read through the explanation it is like someone has taken two or three explanations and merged them into each other, like there is a combining of iterations without clarity.

example:

The tree is blue.

The tall tree has the color of blue.

the tree, while tall, is blue.

That finally gets squished into: the blue tall tree has height and is evident of blue.

Is this double speak or injected double effort of words?

Until Opus 5.0 I'd have agreed with you, but Opus 5.5 is honestly one of the best I've ever sparred with, vastly better than the rest, including Astra imho. Especially so when prompting for clear top-down, first-principles communication.

Disagree, I review a lot of Opus 5.5 code and its comments and docs are still pretty inane and Claudish. Is it better than the 4.x and 5.0 era, sure. But it's still nouning the verbs, using colons everywhere, a-not-b, "a foos the b" etc. A lot of lingual knots that need a human hand to untie.

I was using opus5.5 for everything but since I tried google Argon4 that has been my favorite for writing. It’s clear and to the point.

Opus 5.5 is much better, but IME I always have to force it to review its writing (Opus 5.5) to get it to put the most important facts first (and not bury the lede). It loves to park the main point far away after intermediate clauses. It always writes “premise, so consequence.” It takes a prompt asking to review against rules in context for it to fix it.

The ‘concise’ output style setting is bad with Opus 5.5, as it will often spit out a cryptic (is this really English?!), allergic-to-commas concise response, which takes more time to parse.

Oh it loves to bury a lede.

    < mundane detail>
    < If you don't run this command your computer will explode>
    < exposition > 
    < mundane reminder to follow up something else >

Then 3 back and forths later, your computer explodes, and claude has the cheek to point out when questioned that it did warn you.

Some of the page is also just wrong.

Except it turns out that at least one of those things is because I didn't enable JS, and the static text is wrong!

(Turning on JS did not actually help, because then the page is filled with horrible, cheesy animations and I can't actually read any of it.)

> 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.

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_.

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.

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

That seems orchestrated.

Also, from the patch author:

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

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

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).

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.

> 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.

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

I dig it.

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?

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.

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.

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

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.

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.

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

It's just that every other string than ""* would have been a better choice, including even the empty string (which would have been the next terrible choice).

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).

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.

> 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. :(

> musl's decision to change the return value to "*" is ununderstandable and likely intentionally evil.

Dude, they documented their rationale. It was dumb and wrong but totally relatable. Irresponsible users were getting crashes, and I’m sorry to tell you that for a lot of library maintainers

> Returning NULL on error is actually an excellent choice because most programs will crash if they try to use it without checking.

…is not something they want to write in responses to bug reports, even though it is quite true. It’s human nature to want to prevent your stuff causing problems for others, even if the problems are fully their fault. Most people cannot and will not be Steve “so don’t hold it like that” Jobs, for fear that their decision will be just as short-sighted as his was.

In this case, that was obviously the dead wrong call. But I’ve been in that seat, and “closed: user error” for the 100th time feels bad to write. Add to that a touch of misplaced cleverness, and you get “hey…what if we just returned an impossible string instead of an error?”

Evil requires motive or malice, and I’m having trouble coming up with those in this case. These patches weren’t concealed or miscategorized a la xz, the behavior was discussed in the open. They were just, in hindsight, really poor decisions made for ordinary reasons. Hanlon’s Razor and all that.

That’s why I’ve been harping on C being trash for shared-code APIs in these comments: bad decisions are gonna happen, and irresponsible users are forever, so can we maybe use tools that require you to put on a seat belt (mandatory error handling) before starting the car?

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.

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

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_unix/pam_unix.c#n142

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/Attic/fcrypt.c/?rev=1.2

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 "".)

> 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/Research-V7-Snapshot-Development/usr/src/libc/gen/crypt.c#L330

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?annotate=1.109

Well, crypt_blowfish() has a number of returns with (-1). I'm not familiar with Blowfish, so it's hard for me to trace the possibility of failure here. But what if in a distant future someone adds more hashing algorithms to the list without possessing historical knowledge of this "never return not 0" ?

* https://cgit.freebsd.org/src/tree/secure/lib/libcrypt/crypt-blowfish.c

Failing to check result of crypt() in init cannot do much harm, doing so is equivalent of going system reboot (crash init :-)). Besides, whoever has access to system console can do whatever they want with the system, no password can prevent from that.

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.

No, it's absolutely not defensible.

"The OpenSSL adjustment ensures a minimum hash length for the user before logging them in, even if crypt() succeeds somehow."

OpenSSL??

Sorry, should have written OpenSSH.

Great explanation. This affirms my biases that shell scripting and everything-is-strings is a mistake. Horrific.

In-band signaling[1] strikes again. should not have been used as special value, neither for "no password" in /etc/passwd nor* "error while hashing the password" in crypt. I'm afraid we'll find many more of these abuses, specially in string manipulations. The null was ok, an exception (or equivalent) better, and Result<T> (or equivalent) best.

[1] https://en.wikipedia.org/wiki/In-band_signaling#Computer_data_serialization

Exceptions are almost never the right choice. They create extremely complicated control flow extremely quickly, and then you have to test all the different control flows. Java software is an example of how this goes sideways fast. There's no way to create a class that prevents callers from reading the object before the constructor runs (in multi-threaded software, new first hands a reference to the caller, which can put it on the heap, then it calls the constructor), and objects are default initialized, so all fields (even static ones that are assigned in the constructor) can be null.

So, any field de-reference in Java can produce a null pointer exception. Try testing all paths in something like:

try { foo.sideEffect1().sideEffect2()...sideEffectN(); } catch { bar.sideEffect1().sideEffect2()...sideEffectM(); } finally { quuz.sideEffect1().sideEffect2()...sideEffectP(); }

There are M N P paths. The only way to write testable code with exceptions is to panic on uncaught exception, and to never catch unexpected exceptions. Even then, you're relying on nothing unexpectedly throwing the exception type you caught.

I'd argue Result<T> >> NULL >> sentries >> exceptions for this reason. Sentries are terrible, but at least they're not computationally hard to use correctly.

I'd argue that's the fault of having default nullability, not the exceptions. You would have the same problem if every attribute access returned a Result<T> too.

And I'm not a fan of breaking control flow, but I still find it preferable to nulls. A null may travel far from the place where it was generated before blowing up, making troubleshooting harder, or it might come from a function you called for its side effect but forgot to check the return value. I gotta say I'm also a careful enthusiast of checked exceptions, it's great knowing which ways a function can fail.

But I think the consensus is growing that Result<T> (plus warnings for unused return values) is simply the superior choice.

C has what you are asking for, mostly:

   int foo f() __attribute__((warn_unused_result))

As long as an error condition on the return value causes a crash on use, then it's mostly the same as result. (The main remaining difference being that you do an implicit `.unwrap()` in C, and in things like rust that's an explicit code smell.)

> There's no way to create a class that prevents callers from reading the object before the constructor runs

That can only happen if you leak the reference (given we are talking about java language level semantics and not byte code), which I give you is unfortunately not that hard to do.

As for exceptions, I think they would pair beautifully with software transactional memory, which would work well with effects. In that case (and no side effects) it would not matter what code path we took, we could just go back to a known safe point.

Because ergonomy-wise I still think exceptions win (even though I do like Maybe types, and Rust's ? macro is plenty good): - they let one focus on the actual happy path code - no error is ever swallowed, unless explicitly written so (looking at you go with useless if errs that fail to properly handle the error), they are always handled somewhere - they automatically "unwrap" the Ok result, and automatically bubble up the Err side. - bubbling up is especially useful because most errors can't be handled at their source (e.g. IO error is meaningless, how you handle it depends on what you want to do in the first place, which is done several layers higher) - they automatically attach a stackframe/source, while with result types you may get a list of Ok Ok Ok Err and have no idea how that Err got there.

> There's no way to create a class that prevents callers from reading the object before the constructor runs (in multi-threaded software, new first hands a reference to the caller, which can put it on the heap, then it calls the constructor)

Which is why the pattern of double-checked locking doesn't work.

https://en.wikipedia.org/wiki/Double-checked_locking

It's wrong. The caller doesn't get the reference until the constructor runs. Although it looks like two operations in bytecode, the only thing you can legally do with an uninitialised object reference is call its constructor.

> never catch unexpected exceptions

Yes, broad exception catch clauses are simply incorrect code.

Panic is maybe an overreaction, "scratch everything and orderly abort" is sufficient.

I'm inclined to say a Panic is absolutely preferable to root auth.

Exceptions work fine and enable a clean separation of business logic and error handling. Visibility into partially-initialized objects is a language design issue independent of exceptions. In a non-exceptional language, the corresponding access to uninitialized fields would just be bugs, not errors you can see.

Nothing in your post actually supports the idea that exceptions are "computationally hard". The CFG error edges exist however the errors are spelled

Unfortunately we’re moving to llm based systems which rely entirely on in-band signaling.

PSA: you can typically declare different types of string so that your type system objects to mixing data of different types even though deep down we know they're both strings.

If you mean type aliases, that's a good idea within a single codebase, but the distinction between different types of strings doesn't survive serialization so it wouldn't have prevented this error. What they need is a way to signal errors out-of-band, like Result<T> or exceptions.

In python, aliases are separate from typing.NewType, but you're right: it's only enforced within a codebase and then only if the dev cares to run the typechecker.

So yeah, I guess it either needs to happen at runtime, or we just do signaling and user data on separate file descriptors.

This is presuming that the type system is nomitive rather than structural and there is not serialisation/deserialisation or type refinement occurring. A structural difference is much more preferable.

This is a side-effect of Unix-y "it's all plain text" design that leans extremely heavily upon in-band signalling (among other design decisions).

If /etc/shadow was a JSON (or XML, even, although that would make me feel gross), this bug would have been much easier to see, because a field that reads '"hash": ""' sticks out a lot* more than a custom microformat that you don't understand.

With data structures, there's never "plain text" - it's all structured. The question is whether you want to use existing formats and tools, or create a new one for every tool and literally make footguns like this.

A few years back I un-bashed my life as much as possible in favor of nushell (though it's not the only shell to encourage structure). I feel like it has really helped me notice a lot of this-needs-more-structure type problems outside of my shell that I otherwise wouldn't have.

The elegance of the unix-y approach is actually that it's all bits. You can just join things together and shove the bits through, that's a wild simplification compared with having vendor-specific kinds of plugs. But we could have that kind of compatibility while also benefiting from structure.

So I encourage everybody to give a shell with structured output a try, let it rewire your brain, and then ask yourself: how is the OS holding this back from being everything it could be? Because if we build that OS, it won't encourage these problems.

When I imagine it, it would probe either side of a pipe to see if they support negotiation. If not, send the bits through in the same old unixy way. If so, use the newest schema for structured data that is supported by both sides.

At Amazon we had RecordStream which implemented most of the standard Unix tools on structured streams. It was pretty easy to write filters, aggregators, and formatted output in something not much more complicated than sed/awk/cut/join/sort pipelines. There was a convenient structured log format for services that was often used as input.

https://github.com/benbernard/RecordStream

That looks really cool. I've been heading the opposite direction: posix shell for everything.

I have a bunch of management scripts, etc, that need to run on everything from a potato to a T-1000, and that's the only language that's always available. (OpenBSD sh, FreeBSD sh, busybox, bash, dash, zsh all can cope with it).

I'd love something that's at least JSON-typed (or equivalent), but that can be bootstrapped up on least-common-denominator posix.

Has anyone built something similar? (Yes, I'm being picky -- bun / ts is already a pretty low bar for cross platform support, but these scripts do stuff like install node, so I'm not totally crazy for asking.)

Python?

The issue isn't the config, but the fact that crypt() doesn't have any way to signal failure. In better designed languages you would have exceptions, result types, or something. Even in C you have "return integer indicating success or error" which would've prevented the confusion.

Now the public API is hard to change but you could still avoid the issue by returning a longer token like "==DANGEROUS_CRYPT_ERROR_length==" or "==DANGEROUS_CRYPT_ERROR_unsupported_alg==" which would never match the password field unless someone was really trying to induce the issue.

>but the fact that crypt() doesn't have any way to signal failure

https://pubs.opengroup.org/onlinepubs/009604599/functions/crypt.html

>Upon successful completion, crypt() shall return a pointer to the encoded string. The first two characters 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.

returning a sentinel value of NULL and setting errno is pretty standard C, and shouldn't cause an error.

OP's sshhole link specifically notes that it _should_ return NULL, but some MUSL devs decided they wanted it to return trash instead, and the trash they returned instead of NULL caused this.

All because some devs were too lazy to check the result and another dev decided to be clever and "fix" NULL errors from crypt by sending back 'unmatchable' trash banking on devs not checking NULL.

edit: apparently crypt implementations have been trash ignoring the NULL return for a long time. this isn't only on MUSL, but it's still because people ignored and tried to work around the existing out-of-band error mechanism.

This is true, and "trash" is putting it mildly.

But all those sentinels were added for the reason that, when your code is a public API intended to be widely used by who-knows-who, it needs to be really difficult to misuse it. NULL-plus-errno is easy to misuse. Yes, it's a dangerous mistake and your own fault if you do it. But people have been making that mistake for years and don't show any signs of stopping.

The root cause here is that C is fundamentally a rotten choice for public APIs of any kind. I griped more here: https://news.ycombinator.com/item?id=50381800

> returning a sentinel value of NULL and setting errno is pretty standard C, and shouldn't cause an error.

> OP's sshhole link specifically notes that it _should_ return NULL

I meant there's no mechanism besides returning NULL that would make it hard to ignore the issue (exceptions need to be caught, results need to be unwrapped and their failure handled, an integer return value makes it clearer that there's something there that you need to check). I also missed errno codes when glancing at the manpage earlier which should illustrate the problem.

And yes, whoever first had the brilliant idea to start returning garbage never should've done that, I'm not disputing that.

The spec implies a string of at least two characters in the success case, am I reading that right?

If this is true, then the entire thread branch here on HN is entirely wrong. What is going on??

It's subtly wrong, just like most of the rest of the commentary in this comment section. Yes, crypt is specified to return NULL on failure but if you read the rest of that page it is only permitted to fail with ENOSYS. The spec does not allow for failure for any other reason.

So if you're strictly adhering to the spec in this case then unfortunately NULL is not an acceptable way to signal a generic error condition. Hence all the seemingly bizarre special values. (In a strange turn of events all those experts doing things that looked weird turned out to know what they were doing. Go figure.)

People don't RTFM, either on HN or when implementing "standards compliant" stuff like MUSL.

No error handling scheme can fix that problem. For instance, if a Rust API says I should return a Result<String, ENOSYS>, I can always write code like `if(ENOSYS) { return Ok("*") } instead of returning Err(ENOSYS).

Then my code would be compatible with MUSL.

Sure, but if you didn't do that, and returned the tagged union (in Rust or whatever language), your users would necessarily be forced to handle it in order to process the result.

The only reason this library did the insane thing of returning "" is because it was extremely easy to cause crashes or false-positives (NULL == NULL) by returning NULL+errno. The reason that was easy is because C lacks a means of making error handling mandatory* (exceptions, tuple returns, tagged unions, whatever).

If you return a tagged union, users might mis-handle it, it's true (https://blog.cloudflare.com/18-november-2025-outage/). But they have to handle it, which is a requirement that C APIs are incapable of providing.

You have to handle NULL in C, or your program crashes (though I suppose a misguided optimization of strcmp could do "a == b || really_strcmp(a, b)", which gets us back to your bug).

Also, in things like rust you can just do result.unwrap() if you want. In general, language constructs don't really save you from bad programmers.

> a misguided optimization of strcmp could do "a == b || really_strcmp(a, b)",

In practice, that's less of a risk than:

    char* x = function_that_might_return_null_and_error();
    dangerous_function();
    if (x == NULL) {...}

Or code for which pointer identity is relevant (e.g. accessing slices/subparts of a data structure):

    char* x = returns_null_or_a_slice_pointer(data_structure);
    char* y = other_function_that_returns_null_or_a_slice_pointer(data_structure);
    if (x == y) { puts("you're fucked now!"); }

And for sure there are much better ways to write both kinds of code! But people routinely do not do that.

> I can always write code like `if(ENOSYS) { return Ok("*") } instead of returning Err(ENOSYS)

You can do that but it's going to feel bad to write that and it's going to stand out like a sore thumb to anyone who reads it. But "errno" is just ambient state that's completely invisible unless you happen to have the manpage fresh in your memory.

They had to write

   if(ENOSYS) {
     return "*";
   }

after seeing that other implementations use different sentries like "*0" and that the spec says they MUST return NULL. That must have felt bad too.

A lazy attitude like "if we just used <n> language, all our problems would be fixed".

Yup, that's the way to explain it in order to blame musl.

Here's how to explain it in order to blame openssh:

openssh processes passwords and doesn't want to reveal through timing whether a user has a valid password (or exists at all). So far so good.

What they do if they hit a user with no valid password (i.e. "") is that they look for another user with a valid password, to use the seed from. A bit weird already, but OK from a perspective of avoiding implementation specifics on seeds and hash algorithms. They know at this point that the result must be to deny access even if the hashed value matches, because that hash is for another user*.

So they put the password received over ssh into crypt() with this other user's hashed value to get a seed. And then accept it if it matches, apparently?

Homework: what happens if the password actually matches another user? Can I log in as root if openssh randomly picks my user for the fake salt, and I type my own password correctly?

Personally I think there was nothing wrong with returning NULL. "But if people don't check for it it crashes their programs!" Good! Let it crash loudly! Then they notice and fix the issue.

Also I think it could have returned ":". Given the file format of /etc/shadow and /etc/passwd I don't think it's possible to have a : as a value for anything.

> Also I think it could have returned ":". Given the file format of /etc/shadow and /etc/passwd I don't think it's possible to have a : as a value for anything.

I wonder if somebody else would have thought the exact same thing in a different (non-/etc/*) context, and then you get the same effect in a more obscure manner.

Indeed. Let's keep NULL.

> The issue isn't the config

That's a meaningless statement. There isn't a singular "the" issue like you said. Any one of several different technical decisions would have completely prevented the issue. Including, to my point, /etc/shadow being a JSON file where an account being locked is an `"active": false` field instead of an asterisk with no intrinsic meaning.

No, that's just more in-band signaling!

Some badly designed password hashing algorithms (cough bcrypt cough) have maximum password lengths, ignoring everything after X characters. So it could happen that I set my password to a 100-character string, it gets saved as "==DANGEROUS_CRYPT_ERROR_length==", and then anybody can log in to my account by entering an unrelated long string that returns the same "error".

If a function promises a string result, it should only provide a string if the function succeeded. The null was much better than magic strings.

I know, it's bad. I agree that it should've stayed NULL. But if you really really really think you have an acceptable reason to do something like this (and you probably don't), the least you can do is use a non-trivial token.

If I was really desperate and could only return a valid string, I would probably use a message and add a random component to avoid ever matching two errors. Like SQL, where null != null.

Yes, absolutely. Take for example this cURL command:

  NO_PROXY=1 curl -qs --data @./my\ message.txt -- \
  "https://user:[email protected]/user/${userid}/my%20message.txt"

Random parts of it will be interpreted by the shell, others by cURL, or the filesystem, or the remote server. They each have their own encoding rules, nested. I count ten distinct separators and nine ways of escaping/tagging. It's one command. No, just no.

We have seriously overused strings and we're now paying the price.

Also: free text is the input format of LLMs...

Returning “*” instead of NULL to signal failure is the real footgun. Don’t blame the format.

For historical reasons, we could’ve ended up JSON as “hash”: “*” just the same. The /etc/shadow format lets you skip values completely, like “::”, so your argument doesn’t hold.

Terminating an app because of a NULL dereference is the other footgun. Sadly, we still have “panic” in modern languages like Go and Rust.

"panic" is a good thing. There's always some error condition that your code cannot handle. At some point, the least bad thing to do is crash.

For instance, what if your error handling code throws?

One spectacular example of panic saving the day was the Apollo 11 1202 error. It was basically a watchdog timer that fired if the landing computer was starved of CPU time. A misconfiguration caused the first moon landing to hit CPU overload. When that happened, the computer displayed 1202 and rebooted, then ran things in priority order. Apollo 11 landed in the middle of a tight 'panic, reboot, adjust thrusters' loop. Thanks to core memory, most of the heap survived the reboot, so the reboot step was fast enough for it to mostly land without human interaction. The pilot had to make ~1 adjustment to get it to actually hit the landing site, but the panic-ridden computer handled the rest of the descent.

https://dpma.de/english/our_office/publications/ingeniouswomen/apollosfrauen/margarethamilton/index.html

sshole-type APIs < panicking APIs < required error handling.

Panic is fine, but fundamentally a whole-program crash is undesirable in a really wide range of use-cases. I don't want other SSH connections to crash (or the server to stop accepting valid logins until someone restarts sshd) if one person tries to log in with a credential that triggers a panicking bug in crypt(). Same for crashes in request handlers and so on.

The language/runtime itself doesn't (and probably cannot, at least if it's a general purpose language) distinguish between local and global error conditions, so aborts end up being either unnecessarily damaging, or languages like Go end up normalizing panic "localization"/handling in dangerous ways. Isolation using threads isn't robust enough. Isolation using processes is far too fiddly, slow, and opaque.

The better solution is required error handling. If user code must engage with the possibility of an error state in order to access a result, that's a huge step forward. They can do the error handling wrong, but if you make most people have to do it at all most of the problems will go away.

There are other good solutions too, like Erlang's process boundaries, but those tend to have drawbacks that make them inapplicable to a lot of programming areas. Tagged unions, on the other hand, are only inapplicable in C library code because of language defects and terrible community conventions around ABIs.

"I forgot to check for an error" should not be implicated in bugs in 2026; we have had a large menu of possible alternatives (checked exceptions, unchecked exceptions, tagged unions, etc.) for decades. But we're still over here writing C (and Go), languages pathologically incapable of providing this ability.

SSH specifically has separate processes per connection the last time I checked, so a panic does the right thing: The attacker's connection drops uncleanly.

This is also why you can upgrade SSH over SSH without having the connection drop mid-stream.

Again: You cannot avoid "I forgot to check for an error". What happens if you get a sigpipe on stdout when printing the "I crashed" line? If Linux overcommits memory? If you turn off overcommit, port all software to check for + cleanly handle NULL from malloc (a substantial effort!), but then get a machine check exception from a double bit ECC error during an infallible read?

Sort of. It does some of the validation/auth code in the sshd parent daemon.

It has to: if it did 100% of the post-accept(2) work in a subprocess it would be pretty easy to remotely forkbomb/PID-exhaust a system by spamming invalid logins.

> You cannot avoid "I forgot to check for an error"

Yes you can. Checked exceptions and tagged unions both force calling code to be written with error-awareness.

In SSH's case, "I forgot to check" might lead to crashes (which might knock over sshd so new connections are impossible), or it might lead to false-positives via comparing NULL == NULL.

I'd much rather that boneheaded API users have to write code like

    resultstruct x = authenticate_user(user)
    if x.haserror {
        rv = { .user = user, .result = "successfully authenticated" } 
    } else {
        rv = { .user = user, .result = x.result }
    }
    return rv

At least that way some keystrokes are required to do the stupid thing. People still will (https://www.schneier.com/blog/archives/2008/05/random_number_b.html), but the harder we can make it, the better.

At this point I’m resigning myself to think that we’ve just gotta wait for some people to die. It feels very dramatic and glib (no pun intended lol) and whatever to say, but at this point it feels like there’s a certain hostility in some parts to changing anything from the platonic ideals of however computing happened to work in 2004. “Oh we don’t need to mess with it!”

How did you arrive at that conclusion? If anything, this vulnerability happened because someone decided to change the behaviour of crypt() without fully understanding the consequences. This specific vulnerability did not exist in 2004.

I agree with him. I only managed to get asterisks enabled by default in sudo because sudo-rs isn't owned by people who have decided that the 70s was the perfect decade and nothing should ever change. And look at the insane pushback when it did!

I can't even get an obvious UX improvement through PAM because they like it broken.

https://github.com/linux-pam/linux-pam/issues/778

I think this is a bit unfair. The way I see it, a lot of people are saying that changing breaks lots of old stuff, and risks complete fragmentation. Those are real costs to change. It might still be worth it, but let's not pretend that everyone is resisting change just because it's change.

The costs to change are dropping significantly with AI.

Citation needed.

https://commandline.microsoft.com/agentic-software-engineering-large-scale-code-migration-language-modernization-spec-driven-development/

All I see is sausage makers advocating for a sausage-based diet.

> "it's all plain text" design that leans extremely heavily upon in-band signalling

Honestly I've been shaking my fist at these since 0 becomes True/Success in Bash and the opposite everywhere else

Error code 0 means success in a lot more places than just bash.

That's not specifically bash though, that's process exit codes

Yes. But it's still contradictory

The question you are asking when querying a process exit code is "did this program exit with an error?". The default assumption is "no, it completed successfully", so 0 (false) is the logical expected default return.

A function is expected to produce a result - so the question is "what was the result?". !0 (something) is the logical expected default return.

I've never found it to be contradictory.

I'd say another UNIX-y thing - "C is good enough" - is part of the problem. C makes programming painful which results in all sorts of hacks.

E.g. in Pascal you can can define a record and then defined file of record to have typed I/O. I.e. it comes with built-in serialization format supported by compiler. Not a good one - but perhaps a step in a good direction.

OTOH C people starving for serialization often believe that `struct` byte layout made by a specific version of a compiler is a serialization format - a view not endorsed by the standard.

Pascal's record typed files have the exact same problems regarding endianness and word size as directly writing structs to files in C, so I don't see how they're any better. They're also a complete deadend and unusable for pretty much any file format from the last 40 years+, since you can't even handle a header or magic bytes at the start.

It also looked to me a bit like a semipredicate problem:

https://en.wikipedia.org/wiki/Semipredicate_problem

Even using "no_password" and "error_while_hashing" would have prevented it. The problem was picking a generic, meaningless signal string.

That would've happened to fix it, because "error_while_hashing" and "no_password" happen to not be valid password hashes. The right way would've been to have different syntax for hashes and non-hashes; you could've made it so that a hash starts with a '#' and if it doesn't start with a '#', the rest of the string should be ignored.

Or, for backwards compatibility, start non-hash strings with a character which can't appear in a hash, such as '@'. Your examples would then become '@no_password' and '@error_while_hashing'.

Well neither * nor ! (the two characters I've seen to indicate "no password") will appear in a valid hash. The problem was using a character that could also be returned as an error indicator, IIUC.

Yeah but it's not forwards compatible with future hashes where '' is suddenly a legal hash, which is what happened here. You need a grammar which lets you tell the difference at the parsing stage between "this is a password hash" and "this account has no password". That's not the same as letting the value '' flow through the system as normal and hope it doesn't happen to match the hash.

True, but if they would have picked ?, or #, or any character other than * with its designated special meaning in the /etc/shadow file, then we would not be in this pickle.

hashes can't start with * and there's no reason to think they ever will

Technically they did use a character which can't appear in a hash, '*'.

Right but not in a "rigorous" way. They kept the format of the file the same: the string between the colon characters is a hash. They exploited the fact that an asterisk happens to never appear in a hash, and just let the system believe the asterisk was supposed to be a hash to be matched again.

In a sense, they should've changed the parsing logic to return a struct with a field 'password_hash: Option<String>', and defined the grammar of the field to be: "a hash string, or '' for no login". The bug is, in part, caused by the haphazard way they don't change any formal grammar or contract and just relied on the fact that '' happens to never be a valid hash.

It's the difference between "happens to work" and "defined to work by construction". The former approach breaks once a hash function is invented where '' is a legal hash (such as this new musl crypt() hash function where all strings longer than 256 bytes hash to ''). The latter keeps working regardless because the asterisk is treated as "no password" already at the parser stage.

The whole story is a bit worse than this. Musl already worked around this type of failure. See the second-to-last line here:

https://git.musl-libc.org/cgit/musl/tree/src/crypt/crypt_des.c

(hey, this workaround is wrong if the salt is "x"!)

but they forgot to work around it here:

https://git.musl-libc.org/cgit/musl/tree/src/crypt/crypt_sha512.c#n369

The vulnerable path in musl is only a little bit vulnerable, because "" is not actually a sha256 encrypted configuration and should not reach this code: musl's crypt(..., "") will not call that code! Turns out that sshd detects that the password hash is too short and does not forward the salt!

https://github.com/openssh/openssh-portable/blob/master/auth-passwd.c#L223

Look, no else statement! If that if doesn't get taken, then salt remains NULL. And the sshd code is really quite messy and doesn't keep track of the fact that this code path is inappropriate for validating an existing password hash.

A workaround in the sha256 path in musl wouldn't even have prevented the bug!

I'm going to give some credit to musl for unwisely using "*" and some credit to OpenSSH for having nonsensical control and data flow.

> (hey, this workaround is wrong if the salt is "x"!)

When the salt is "x", it'll return "*" and still not match, won't it? The code is correct as long as the second argument is the hash that's being checked. And like you say, the sha512 path isn't relevant since you only get there if the salt started with "$".

> Turns out that sshd detects that the password hash is too short and does not forward the salt!

I think you're misunderstanding the intent of that check. It was added by the 2016 change linked in the writeup[1] and causes OpenSSH's xcrypt() to select and use a salt from a different line of /etc/shadow to prevent timing attacks. It is correct for a crypt() implementation that returns NULL on error.

[1] https://lists.mindrot.org/pipermail/openssh-commits/2016-July/005428.html

> When the salt is "x", it'll return "*" and still not match, won't it? The code is correct as long as the second argument is the hash that's being checked. And like you say, the sha512 path isn't relevant since you only get there if the salt started with "$".

I think you're right. That's what I get for doing a very very quick read.

> I think you're misunderstanding the intent of that check. It was added by the 2016 change linked in the writeup[1] and causes OpenSSH's xcrypt() to select and use a salt from a different line of /etc/shadow to prevent timing attacks. It is correct for a crypt() implementation that returns NULL on error.

I don't care what the intent of the check is. If you're doing a fake password hash to obscure timing, remember that it's fake and don't let the user log in! Don't deliberately pass nonsense into the live check and rely on the nonsense check failing to prevent the fake check from accidentally working. And don't have a code path that thinks it's checking the real password but passes in the wrong second parameter to crypt().

Or, better yet, have an entirely separate function fake_check_password() that does the fake check, returns void, and cannot possibly be confused with the actual password check code.