Created attachment 1936 [details] proposed patch Both auth_rsa_verify_response() and auth_rsa_key_allowed() are PRIVSEP'ed, so there should be no security degradation. auth_rsa_key_allowed() is called from auth_rsa() only; auth_rsa_verify_response() is called only from auth_rsa_challenge_dialog(), which in turn is called - either from auth_rsa(), right after auth_rsa_key_allowed() call, - or from auth_rhosts_rsa(), right after auth_rhosts_rsa_key_allowed() call, which already calls auth_key_is_revoked(). As result of this change, auth_rsa_key_allowed() will be called earlier on the auth_rsa() path, before starting challenge-response, which is good, and won't be called second time on the auth_rhosts_rsa() path, which is also good.
What is the practical intent of this change?
(In reply to comment #1) > What is the practical intent of this change? The proposed change is result of code inspection. I maintain an OpenSSH key blacklisting patch (see http://www.openwall.com/lists/oss-security/2008/05/27/3 for more details) which was originally implemented for 5.0p1, before certificate authentication support (which was introduced later in 5.4p1). While merging my changes to use auth_key_is_revoked() infrastructure, I found out that one auth_key_is_revoked() call is not placed quite well: there is no use for server to start a challenge-response dialog with the key that is not allowed for authentication.
Patch applied and will be released in OpenSSH-5.7 - thanks!
Move resolved bugs to CLOSED after 5.7 release
(In reply to comment #3) > Patch applied and will be released in OpenSSH-5.7 - thanks! The patch was changed before applying, with result that the rest of the file passed to rsa_key_allowed_in_file() is going to be skipped once a revoked key is detected, while the intended behavior is to skip just those lines that define revoked keys. Please compare the original proposal https://bugzilla.mindrot.org/attachment.cgi?id=1936 with actually applied change http://hg.mindrot.org/openssh/rev/a82eca01db5b and consider applying the change in its original form.
I think the behaviour that I committed is correct: the key that is being matched has been confirmed as revoked, there is no point continuing to match and it's probably dangerous to do so - e.g. a subsequent listing of the same key will cause it to be "unrevoked"
(In reply to comment #6) > I think the behaviour that I committed is correct: the key that is > being matched has been confirmed as revoked, there is no point > continuing to match The file may still contain valid keys. Even in case of syntax error the code just skips broken lines. > and it's probably dangerous to do so - e.g. a > subsequent listing of the same key will cause it to be "unrevoked" Would it? How a key that is already revoked could be "unrevoked"?
Remember what is happening here: a key has been suggested by the client and is being compared against the lines in authorized_keys. *After* the modulus has been matched, we check whether the key is revoked. If it is revoked, then there is no point in checking further in the file to see if an non-revoked entry of the same key exists.
Agreed.