openssh bugs - May 2019 - [Bug 3005] New: Use high-level EVP PKEY API instead of low-level algorithm specific calls + separate digesting in the every backend

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

            Bug ID: 3005
           Summary: Use high-level EVP PKEY API instead of low-level
                    algorithm specific calls + separate digesting in the
                    every backend
           Product: Portable OpenSSH
           Version: 8.0p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 3277
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3277&action=edit
proposed patch

The OpenSSH is using low-level OpenSSL API to access and use keys,
which was fine in the past, but it is getting more complicated as the
amount of signature algorithms is expanding.

This patch mostly simplifies RSA signatures handling by dropping the
hardcoded hash algorithms OIDs and unifies the various key types
handling be encapsulating them in common EVP_PKEY structure.

I believe this API is also available in LibreSSL so it should not have
compatibility issues, but I did not test that.

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

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
I'm not certain of the benefit of doing this, but deleting the custom
verification code removes a security mitigation that has saved us
from
>10 bugs since Markus added it.
Many (most?) versions of OpenSSL invoke a full ASN.1 parser in the RSA
signature verification path. Our implementation avoids that massive
attack surface for something much smaller and easy to audit. We won't
delete this code until after we've dropped support for the last version
of OpenSSL that does RSA signature verification with the ASN.1 parser.

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

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Moreover the diffstat:

 14 files changed, 707 insertions(+), 489 deletions(-)

Doesn't show any benefit for code removal, and the patch introduces new
dependencies on OpenSSL's ASN.1 code, e.g. the i2d_DSA_SIG() etc.

This patch is only of benefit for signature algorithms that are
supported by OpenSSL. The last two that we added (ed25519 and XMSS)
weren't, and I don't think it's likely that future signature
algorithm
additions will land in OpenSSL before they land in OpenSSH either.

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

--- Comment #3 from Jakub Jelen <jjelen at redhat.com>
---
> This patch is only of benefit for signature algorithms that are
> supported by OpenSSL. The last two that we added (ed25519 and XMSS)
> weren't, and I don't think it's likely that future signature
algorithm
> additions will land in OpenSSL before they land in OpenSSH either.
The ed25519 curves are already implemented in OpenSSL so it should be
possible to cover this also in OpenSSL, but that is different issue to
solve. Moreover the ed25519 curves do not need to digest of the data
separately, which also simplifies things. I did not have a better look
into the XMSS yet though.
> Many (most?) versions of OpenSSL invoke a full ASN.1 parser in the RSA
> signature verification path. Our implementation avoids that massive
> attack surface for something much smaller and easy to audit.
You are probably right. The DigestInfo in OpenSSL is added in
encode_pkcs1() function by calling ASN.1 encoder on openssl (digest)
and openssh (hash algorithm) provided data. But I do not see how this
is a huge attack surface since there is really no variable data
provided by user to these functions.

On the other hand, implementation in OpenSSL is also audited, which
removes the critical code duplication.

Similarly the d2i_*() functions operate on the signature data provided
by the OpenSSL code (the signatures).

Indeed the changeset can be limited to the signatures leaving the RSA,
ECDSA and DSA structures stored in the sshkey structure, which would be
significantly smaller and touching significantly less code, if that
would be more acceptable.

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

--- Comment #4 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3283
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3283&action=edit
Alternative patch modifying only the signature and verification
routines

Other possibility is to modify only the signature and verification
routines as in the attached patch, which is making the patch smaller,
but somehow misses some aspects of the proposed simplifications.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
The most recent patch still introduces OpenSSL ASN.1 parsing in the
pre-authentication signature verification path. This is a huge attack
surface that we're simply not prepared to accept. IMO the history of
vulnerabilities that we've avoided by doing so speaks for itself.

Sorry, but we won't be adopting this approach.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
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.