Today's C programming exercise: what can possibly go wrong with the KEY_MAX check here (MUSL SHA512 implementation)?
/* key limit is not part of the original design, added for DoS protection. */
#define KEY_MAX 256
// ...
static char *sha512crypt(const char *key, const char *setting, char *output) {
// ...
/* reject large keys */
for (i = 0; i <= KEY_MAX && key[i]; i++);
if (i > KEY_MAX)
return 0;
len = i;
// ...
}
(Note that this was done for legit reasons, large payloads can clog sha512crypt and result in a DoS).
Hint: look what's been hiding in plain sight since the first version of MUSL:
on error "*" is returned.
So MUSL basically said "fine, crypt() returning NULL in case of error just crashes a lot of applications, so we'll always return a string and in case of error you'll get a "*"".
Again, a fair point (even though one that arguably breaks the UNIX contract of crypt()). Rather than crashing applications that always forget that crypt() can also return a NULL, just return a placeholder in case of errors that will never match upon comparison.
A couple of years later, 2016, OpenSSH gets CVE-2016-6210 - a potential user enumeration vulnerability.
The reasoning is simple. Logging in a user that doesn't exist usually results in an immediate error, while a user that exists will take a bit longer because stuff needs to be encrypted and hashes compared - and thanks to that time difference an attacker could brute force a system to figure out what users exist.
How did OpenSSH solve that problem? Well, the solution was as creative as it can get:
/*
* If we don't have a salt we are encrypting a fake password for
* for timing purposes. Pick an appropriate salt.
*/
if (salt == NULL)
salt = pick_salt();
It basically says "the user you're asking for doesn't exist, but I'll just pick up the salt of another random user and encrypt your password anyway - just for the sake of wasting time, so you can't guess by the request time if the user exists or not".
Again, it may be unorthodox, but it's a sensible solution to prevent timing-based user enumeration on the system.
But wait, what's the UNIX /etc/passwd convention for users with login disabled? That's right:
daemon:*:1:1:daemon:/bin:/bin/false
A * instead of the actual password.
And on #Alpine #Linux root has a * - it disables login, but, unlike !, it doesn't break PAM-authenticated SSH keys too.
How does #ssh check your password on login?
It looks into /etc/shadow, and if it sees a $6$ hash it assumes that it's SHA512.
But what happens when you try to log in as root? And maybe with a password longer than 256 characters?
The * in /etc/passwd prevents you from doing that. But OpenSSH won't tell you no directly. It'll try to get the salt of one of your neighbours and encrypt your password anyway.
But what if you get the salt for a SHA512 hash?
Remember that sha512crypt on MUSL was designed to specifically fail if you pass a payload longer than 256 characters.
And what does crypt() on MUSL do when the underlying crypto routine fails?
That's right - rather than NULL, it returns *.
So what does SSH do then?
It compares what crypt() returned (*) to what /etc/shadow contained as the password for that user (*).
And boom.
Note that this critical security vulnerability has been the result of unorthodox fixes for real problems that, when compounded together, led to an explosive mix.
It's fair for a password hashing function to refuse to encrypt payloads that are too long to prevent DoS. But it perhaps should have done so by raising errors that the downstream consumer could interpret.
It's fair for a library developer to assume that most of the folks will always expect crypt() to return some string, and a lot of stuff may break if there are errors. But perhaps silently returning an arbitrary string in case of errors isn't something you'd expect from a standard library.
It's fair for OpenSSH to prevent timing-based enumeration attacks by making sure that the same time is spent on each authentication request, regardless if the user exists or not. But perhaps picking up the salt of another random user to encrypt the password passed by another user is just begging for something to go wrong at some point.
This is a case where 3 unorthodox solutions to 3 real problems compounded together to create a problem that is even bigger than the problem they were trying to solve individually.
The lesson: if you write code, make sure that it always works in predictable ways. Make sure that it does what's expected for it to do, that it always returns what people expect it to return. Don't let individual problems break that contract if possible. And don't fail silently if possible.
Because when your code starts doing something that people don't expect it to do, you're just asking for a CVE to come in your ways - weeks, months or even years later.