Hacker News Reader

Comment by tchebb | Hacker News Reader

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

View on HN

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

Reply on news.ycombinator.com