-
Notifications
You must be signed in to change notification settings - Fork 256
is_valid_hash: accept two more cases #1504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
593d53e to
be40d25
Compare
|
|
||
| if (streq(hash, "*")) | ||
| // If the hash starts with '*', this is an intentional way to prevent | ||
| // a user from logging in with password. | ||
| if (strprefix(hash, "*")) | ||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to shadow(5), ! is the prefix to lock a password. Please check the manual page to make sure both match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, ! is the prefix to lock temporarily a password.
On the other hand, * is to permanently remove a password. Thus, this should not be a prefix, but the entire hash.
Am I mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'd be happy accepting * as a prefix, if that's common practice. That would make it essentially a synonym of !, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make it essentially a synonym of
!, right?
Not fully. usermod -U user removes a ! but not a *. So your remark about permanently removed password (or "locked" password) is the key difference. In essence, both mean that the hash cannot be matched with any entered password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference between *hash and * would be that * is a permanent removal of the password hash (you'll need to regenerate it), while *hash still allows manual restoration. So, *hash would be a more inconvenient synonym of !hash, while * would mean "whatever your password hash was, we don't know it anymore, and can't recover it".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'd update the comment and the man page to state the meaning of these things
We currently allow ! followed by a valid hash but not ! followed by nothing, and * followed by nothing but not * followed by something. Accept the other cases. Signed-off-by: Serge Hallyn <[email protected]> Cc: Marc 'Zugschlus' Haber <[email protected]> Cc: Alejandro Colomar <[email protected]> Cc: Chris Hofstaedtler <[email protected]>
be40d25 to
196c17c
Compare
| hash = strprefix(hash, "!") ?: hash; | ||
| // If the hash starts with '!', it means just lock the password. | ||
| if (strprefix(hash, "!")) | ||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is accepting anything after a !, such as !invalid_hash. We'd want to make sure that for a string !hash we still verify that hash is a valid hash for some algorithm.
I think what you want is to just add a check for an empty string after jumping over the !, as done in #1505.
We currently allow ! followed by a valid hash but not ! followed by nothing, and * followed by nothing but not * followed by something.
Accept the other cases.
Cc: Marc 'Zugschlus' Haber [email protected]
Cc: Alejandro Colomar [email protected]
Cc: Chris Hofstaedtler [email protected]