openssh bugs - Oct 2010 - [Bug 1829] New: auth-rsa.c: move auth_key_is_revoked() call from auth_rsa_verify_response() to auth_rsa_key_allowed()

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

           Summary: auth-rsa.c: move auth_key_is_revoked() call from
                    auth_rsa_verify_response() to auth_rsa_key_allowed()
           Product: Portable OpenSSH
           Version: 5.6p1
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: sshd
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: ldv at altlinux.org


Created attachment 1936
  --> https://bugzilla.mindrot.org/attachment.cgi?id=1936
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #1 from Damien Miller <djm at mindrot.org> 2010-11-05 11:22:28
EST ---
What is the practical intent of this change?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

--- Comment #2 from Dmitry V. Levin <ldv at altlinux.org> 2010-11-05
11:56:03 EST ---
(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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1936|application/octet-stream    |text/plain
          mime type|                            |
   Attachment #1936|0                           |1
           is patch|                            |

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1803

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #3 from Damien Miller <djm at mindrot.org> 2010-12-04 10:57:44
EST ---
Patch applied and will be released in OpenSSH-5.7 - thanks!

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #4 from Damien Miller <djm at mindrot.org> 2011-01-24 12:33:31
EST ---
Move resolved bugs to CLOSED after 5.7 release

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Dmitry V. Levin <ldv at altlinux.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |REOPENED
         Resolution|FIXED                       |

--- Comment #5 from Dmitry V. Levin <ldv at altlinux.org> 2011-09-13
08:46:42 EST ---
(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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Dmitry V. Levin <ldv at altlinux.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1803                        |1930

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

--- Comment #6 from Damien Miller <djm at mindrot.org> 2011-10-05 00:59:23
EST ---
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"

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

--- Comment #7 from Dmitry V. Levin <ldv at altlinux.org> 2011-10-05
02:58:46 EST ---
(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"?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

--- Comment #8 from Damien Miller <djm at mindrot.org> 2011-10-05 08:44:08
EST ---
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Dmitry V. Levin <ldv at altlinux.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1930                        |1803
             Status|REOPENED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #9 from Dmitry V. Levin <ldv at altlinux.org> 2011-10-05
09:12:28 EST ---
Agreed.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1829

Dmitry V. Levin <ldv at altlinux.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.