From b5a276884b8e945815732845540af3c8143e8457 Mon Sep 17 00:00:00 2001 From: David Woodhouse Date: Thu, 22 Aug 2019 18:09:11 +0100 Subject: [PATCH] Fix bogus check for EVP_PKEY_supports_digest_nid() in check_cert_usable() In commit 2d263a4a73 ("Honour mandatory digest on private key in has_usable_cert()" I added two checks for the capabilities of the EVP_PKEY being used. One of them was wrong, as it should only be checking the signature of the X.509 cert (by its issuer) against the sigalgs given in a TLS v1.3 signature_algorithms_cert extension. Remove it and provide the code comments which, if they'd been present in the first place, would hopefully have prevented the mistake. Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/9672) --- ssl/t1_lib.c | 54 +++++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index ab0a0e0143..2f7b570a06 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2644,46 +2644,44 @@ static int check_cert_usable(SSL *s, const SIGALG_LOOKUP *sig, X509 *x, int mdnid, pknid, supported; size_t i; + /* + * If the given EVP_PKEY cannot supporting signing with this sigalg, + * the answer is simply 'no'. + */ + ERR_set_mark(); + supported = EVP_PKEY_supports_digest_nid(pkey, sig->hash); + ERR_pop_to_mark(); + if (supported == 0) + return 0; + + /* + * The TLS 1.3 signature_algorithms_cert extension places restrictions + * on the sigalg with which the certificate was signed (by its issuer). + */ if (s->s3.tmp.peer_cert_sigalgs != NULL) { + if (!X509_get_signature_info(x, &mdnid, &pknid, NULL, NULL)) + return 0; for (i = 0; i < s->s3.tmp.peer_cert_sigalgslen; i++) { lu = tls1_lookup_sigalg(s->s3.tmp.peer_cert_sigalgs[i]); - if (lu == NULL - || !X509_get_signature_info(x, &mdnid, &pknid, NULL, NULL) - /* - * TODO this does not differentiate between the - * rsa_pss_pss_* and rsa_pss_rsae_* schemes since we do not - * have a chain here that lets us look at the key OID in the - * signing certificate. - */ - || mdnid != lu->hash - || pknid != lu->sig) + if (lu == NULL) continue; - ERR_set_mark(); - supported = EVP_PKEY_supports_digest_nid(pkey, mdnid); - ERR_pop_to_mark(); - if (supported == 0) - continue; /* - * If it didn't report a mandatory NID (supported < 0), for - * whatever reasons, we just ignore the error and allow all - * hashes to be used. + * TODO this does not differentiate between the + * rsa_pss_pss_* and rsa_pss_rsae_* schemes since we do not + * have a chain here that lets us look at the key OID in the + * signing certificate. */ - return 1; + if (mdnid == lu->hash && pknid == lu->sig) + return 1; } return 0; } - ERR_set_mark(); - supported = EVP_PKEY_supports_digest_nid(pkey, sig->hash); - ERR_pop_to_mark(); - if (supported == 0) - return 0; + /* - * If it didn't report a mandatory NID (supported < 0), for - * whatever reasons, we just ignore the error and allow all - * hashes to be used. + * Without signat_algorithms_cert, any certificate for which we have + * a viable public key is permitted. */ - return 1; } -- 2.25.1