Hacker News
points by dns_snek 2 days ago | 0 comments
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.

[–] knome 2 days ago
>but the fact that crypt() doesn't have any way to signal failure

https://pubs.opengroup.org/onlinepubs/009604599/functions/cr...

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

[–] dns_snek 2 days ago
> 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.

[–] zbentley 2 days ago
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

[–] n_plus_1_acc 2 days ago
The spec implies a string of at least two characters in the success case, am I reading that right?
[–] mafuy 2 days ago
If this is true, then the entire thread branch here on HN is entirely wrong. What is going on??
[–] fc417fc802 2 days ago
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.)

[–] hedora 2 days ago
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.

[–] zbentley 2 days ago
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.

[–] hedora 2 days ago
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.

[–] jstanley 2 days ago
> You have to handle NULL in C, or your program crashes

But it crashes at runtime, so such a bug can exist for years with nobody noticing.

The existence of such bugs is what motivated the musl developers to return "*" instead of NULL.

[–] zbentley 2 days ago
> 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.
[–] hedora 1 day ago
You can always write:

   x : Result<string, err> = function...
   dangerous_function();
   switch(x) { 
     Ok() ... 
     Error() ...
   }
There's no difference between result and null other than readability (there's a mechanical transformation between the two, where you use unwrap() in rust on an unchecked null dereference, and you read errno in C instead of looking inside the Err() result variant).

To be clear, I strongly prefer Result<> because of the readability, but any dumb thing you can write with NULL + errno can be written with Result (and vice versa).

[–] dns_snek 2 days ago
> 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.

[–] hedora 2 days ago
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.
[–] dns_snek 2 days ago
Not the library authors but their users!
[–] slashdave 2 days ago
A lazy attitude like "if we just used <n> language, all our problems would be fixed".
[–] eqvinox 2 days ago
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?

[–] FLeXMurphy 2 days ago
Except it's not trash. It's a backdoor funded by our glorious leader Kim Jong Un.
[–] panzi 2 days ago
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.

[–] hnfong 2 days ago
> 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.

[–] panzi 2 days ago
Indeed. Let's keep NULL.
[–] throw10920 2 days ago
> 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.

[–] BoppreH 2 days ago
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.

[–] dns_snek 2 days ago
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.
[–] BoppreH 2 days ago
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.