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.
> 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:
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.
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).
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?"
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.
2 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:
> 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.
2 replies →
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).
3 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?
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.
19 replies →
Yep. See: underhanded c code contest
https://www.underhanded-c.org/
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?"