Comment by mkl
2 days ago
Even using "no_password" and "error_while_hashing" would have prevented it. The problem was picking a generic, meaningless signal string.
2 days ago
Even using "no_password" and "error_while_hashing" would have prevented it. The problem was picking a generic, meaningless signal string.
That would've happened to fix it, because "error_while_hashing" and "no_password" happen to not be valid password hashes. The right way would've been to have different syntax for hashes and non-hashes; you could've made it so that a hash starts with a '#' and if it doesn't start with a '#', the rest of the string should be ignored.
Or, for backwards compatibility, start non-hash strings with a character which can't appear in a hash, such as '@'. Your examples would then become '@no_password' and '@error_while_hashing'.
There were different syntaxes for hashes and non hashes. The problem was that both a non hash "this password isn't set" and a non hash "this call to crypt produced an error" used the same non password value.
Well neither * nor ! (the two characters I've seen to indicate "no password") will appear in a valid hash. The problem was using a character that could also be returned as an error indicator, IIUC.
Yeah but it's not forwards compatible with future hashes where '*' is suddenly a legal hash, which is what happened here. You need a grammar which lets you tell the difference at the parsing stage between "this is a password hash" and "this account has no password". That's not the same as letting the value '*' flow through the system as normal and hope it doesn't happen to match the hash.
3 replies →
Technically they did use a character which can't appear in a hash, '*'.
Right but not in a "rigorous" way. They kept the format of the file the same: the string between the colon characters is a hash. They exploited the fact that an asterisk happens to never appear in a hash, and just let the system believe the asterisk was supposed to be a hash to be matched again.
In a sense, they should've changed the parsing logic to return a struct with a field 'password_hash: Option<String>', and defined the grammar of the field to be: "a hash string, or '*' for no login". The bug is, in part, caused by the haphazard way they don't change any formal grammar or contract and just relied on the fact that '' happens to* never be a valid hash.
It's the difference between "happens to work" and "defined to work by construction". The former approach breaks once a hash function is invented where '*' is a legal hash (such as this new musl crypt() hash function where all strings longer than 256 bytes hash to '*'). The latter keeps working regardless because the asterisk is treated as "no password" already at the parser stage.