Comment by annex-winged-cr
1 day ago
The implicitness is exactly the problem. The workaround that caused this vulnerability was done because people assumed that crypt() would never return null.
You don't even need a fancy result type. A simple Option<String> would've forced downstream users to consider the failure case, avoiding this situation in the first place.
The workaround was correctly designed: two error string tokens are used so that one can always be chosen that is not equal to the second argument.
Someone coded a defense against long inputs which ignored this important principle.
What turned the bug into a security disaster is that Musl stupidly chose two symbols such that one of them is exactly the same as a convention for disabled logins. So that the moment someone makes a change to the function which allows crypt("something", "*") to return "*", you have an incident.
If the same bug were made in Glibc, it would have limited impact because crypt("long .... string", "*0") returning "*0", while incorrect, wouldn't do anything in a system that doesn't use *0 in password files. *0 does not correspond to any widespread password field convention.
.unwrap()
Now what?
I think the argument would be that in C people will look at a char* and think "string" rather than "string or null". Thus consumers of the library not checking the result and the maintainer trying to be helpful by putting a "safe" value in there for the error case.
Whereas if the library returns something that is unambiguously an option type, consumers unambiguously call unwrap at their own risk.
Of course you do have a point (assuming I'm picking up what you're suggesting correctly) that all pointers in C are option types. But I think there's a cognitive ergonomics aspect to this where an option type more clearly relays the library authors' intent.
First you shouldn't have unwraps in prod code, and you can verify that in your CI/build process/whatever. Second, now you have a crash of the SSH server, which is arguably much better than some random logging in as root.
This isn't about "you", it's about whether any consumer of your code is using unwrap on the result, which currently is perfectly safe to do. The equivalent in C is returning NULL, which would also have caused a crash. They decided not crashing was preferable, which is how we ended up here.
"you shouldn't have unwraps in production code" is exactly the same form of argument as "you shouldn't be assuming crypt returns a valid non-null pointer in production code".
3 replies →