Comment by amluto
2 days ago
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...
(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_sha...
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...
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-Jul...
> 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.
> If you're doing a fake password hash to obscure timing, remember that it's fake and don't let the user log in!
I think this is the crux. Instead of doing the right thing, they tried to codegolf to get the desired behavior and it got real subtle, real fast.
this is a little flip: maintaining a security-critical program naturally pushes one to minimize code changes because a 30-line change is usually easier to review than a 100-line change. Except when it's not.
> you're doing a fake password hash to obscure timing, remember that it's fake and don't let the user log in
That would be a much better approach. I see your point now.
The formatting in your comment got messed btw
Thanks, fixed