Created attachment 3277 [details] 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.
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.
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.
> 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.
Created attachment 3283 [details] 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.
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.
closing resolved bugs as of 8.6p1 release