openssh bugs - Mar 2024 - [Bug 3675] New: CASignatureAlgorithms should be verified before verifying signatures

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

            Bug ID: 3675
           Summary: CASignatureAlgorithms should be verified before
                    verifying signatures
           Product: Portable OpenSSH
           Version: 9.7p1
          Hardware: Other
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: andres at anarazel.de

Hi,

The code injected in CVE-2024-3094 causes RSA_public_decrypt to be
redirected to a payload.  This is not reachable for normal pubkey
authentication without 
1) the key algorithm being of a permitted type
2) knowing at least the signature of a pubkey in authorized_keys etc

However, certificates are verified before such checks:
userauth_pubkey()
-> sshkey_from_blob()
-> sshkey_from_blob_internal()
-> cert_parse()
-> sshkey_verify(key->cert->signature_key)
-> ssh_rsa_verify() (or others, depending on cert type)
-> openssh_RSA_verify()
-> RSA_public_decrypt()

The signature algorithm *is* subsequently checked, but of course
RSA_public_decrypt has already been called by that point. 

Outside of CVE-2024-3094, which is not openssh's reponsibility, that is
not a correctness issue. But doing verification of signatures with
algorithms that are disabled still seems fairly suboptimal, increasing
the amount of code reachable without having any valid access.

Looks to me that an equivalent to checking in authorized_keys can't be
done before the verification, but checking CASignatureAlgorithms seems
entirely possible.

It might also be worth rejecting certificates without any validation if
the sshd is not configured to use CA based auth.


Regards,

Andres

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Sam James <sam at gentoo.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo.org

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Luke Simmons <luke5083 at live.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |luke5083 at live.com

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Bertrand Jacquin <bertrand at jacquin.bzh> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bertrand at jacquin.bzh

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Ismail Donmez <ismail at i10z.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ismail at i10z.com

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

felix at eckhofer.com <felix at eckhofer.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |felix at eckhofer.com

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Benjamin Gilbert <bgilbert at backtick.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bgilbert at backtick.net

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 3806
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3806&action=edit
check expected key type and CA algorithm earlier

On the one hand it feels a bit like trying to fight the last battle,
but on the other it is a meaningful attack surface reduction.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3806|0                           |1
        is obsolete|                            |

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Created attachment 3807
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3807&action=edit
correct diff

oops, that was an older version of the change. Use this.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3807|0                           |1
        is obsolete|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 3808
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3808&action=edit
correct diff

-- 
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=3675

github at kalvdans.no-ip.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |github at kalvdans.no-ip.org

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

--- Comment #4 from Andres Freund <andres at anarazel.de>
---
> On the one hand it feels a bit like trying to fight the last battle, but on
the other it is a meaningful attack surface reduction.
Agreed on both points.

Thanks for the quick writing of the patch!


I don't know the openssh codebase well, so my ability to provide review
is limited.

I think there might still be one path "unprotected" after this.
userauth_hostbased() uses sshkey_from_blob() and 
a) checks options.hostbased_accepted_algos afterwards
b) uses sshkey_from_blob(), not sshkey_from_blob_expect_type(), with a
subsequent check of the certificate type


Another thing I noticed is that it might end up being a bit harder to
debug some of the error paths after the change, due to going from
specific error messages to more generic error codes. OTOH, it seems
unlikely that these paths are encountered outside of attacks.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.