From 2e0b9e030e70563afe3ecd44ea9171ea3e6ce51c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 21 Dec 2023 12:20:52 -0500 Subject: [PATCH 1/2] Test signature verification in X509_verify_cert Previously, if we just skipped signature checks, zero tests would fail. This is perhaps not ideal. Change-Id: Ife42f32d06c01b48afa9da26a8bd25814f9a909f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65049 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- crypto/x509/x509_test.cc | 168 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 167 insertions(+), 1 deletion(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 7d46a9d6e..8cfea574c 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1620,7 +1620,7 @@ static bssl::UniquePtr MakeTestCert(const char *issuer, if (!bc) { return nullptr; } - bc->ca = is_ca ? 0xff : 0x00; + bc->ca = is_ca ? ASN1_BOOLEAN_TRUE : ASN1_BOOLEAN_FALSE; if (!X509_add1_ext_i2d(cert.get(), NID_basic_constraints, bc.get(), /*crit=*/1, /*flags=*/0)) { return nullptr; @@ -4175,6 +4175,172 @@ TEST(X509Test, Expiry) { } } +TEST(X509Test, SignatureVerification) { + bssl::UniquePtr key = PrivateKeyFromPEM(kP256Key); + ASSERT_TRUE(key); + + struct Certs { + bssl::UniquePtr valid; + bssl::UniquePtr bad_key_type, bad_key; + bssl::UniquePtr bad_sig_type, bad_sig; + }; + auto make_certs = [&](const char *issuer, const char *subject, + bool is_ca) -> Certs { + Certs certs; + certs.valid = MakeTestCert(issuer, subject, key.get(), is_ca); + if (certs.valid == nullptr || + !X509_sign(certs.valid.get(), key.get(), EVP_sha256())) { + return Certs{}; + } + + static const uint8_t kInvalid[] = {'i', 'n', 'v', 'a', 'l', 'i', 'd'}; + + // Extracting the algorithm identifier from |certs.valid|'s SPKI, with + // OpenSSL's API, is very tedious. Instead, we'll just rely on knowing it is + // ecPublicKey with P-256 as parameters. + const ASN1_BIT_STRING *pubkey = X509_get0_pubkey_bitstr(certs.valid.get()); + int pubkey_len = ASN1_STRING_length(pubkey); + + // Sign a copy of the certificate where the key type is an unsupported OID. + bssl::UniquePtr pubkey_data(static_cast( + OPENSSL_memdup(ASN1_STRING_get0_data(pubkey), pubkey_len))); + certs.bad_key_type = MakeTestCert(issuer, subject, key.get(), is_ca); + if (pubkey_data == nullptr || certs.bad_key_type == nullptr || + !X509_PUBKEY_set0_param(X509_get_X509_PUBKEY(certs.bad_key_type.get()), + OBJ_nid2obj(NID_subject_alt_name), V_ASN1_UNDEF, + /*param_value=*/nullptr, pubkey_data.release(), + pubkey_len) || + !X509_sign(certs.bad_key_type.get(), key.get(), EVP_sha256())) { + return Certs{}; + } + + // Sign a copy of the certificate where the key data is unparsable. + pubkey_data.reset( + static_cast(OPENSSL_memdup(kInvalid, sizeof(kInvalid)))); + certs.bad_key = MakeTestCert(issuer, subject, key.get(), is_ca); + if (pubkey_data == nullptr || certs.bad_key == nullptr || + !X509_PUBKEY_set0_param(X509_get_X509_PUBKEY(certs.bad_key.get()), + OBJ_nid2obj(NID_X9_62_id_ecPublicKey), + V_ASN1_OBJECT, + OBJ_nid2obj(NID_X9_62_prime256v1), + pubkey_data.release(), sizeof(kInvalid)) || + !X509_sign(certs.bad_key.get(), key.get(), EVP_sha256())) { + return Certs{}; + } + + bssl::UniquePtr wrong_algo(X509_ALGOR_new()); + if (wrong_algo == nullptr || + !X509_ALGOR_set0(wrong_algo.get(), OBJ_nid2obj(NID_subject_alt_name), + V_ASN1_NULL, nullptr)) { + return Certs{}; + } + + certs.bad_sig_type.reset(X509_dup(certs.valid.get())); + if (certs.bad_sig_type == nullptr || + !X509_set1_signature_algo(certs.bad_sig_type.get(), wrong_algo.get())) { + return Certs{}; + } + + certs.bad_sig.reset(X509_dup(certs.valid.get())); + if (certs.bad_sig == nullptr || + !X509_set1_signature_value(certs.bad_sig.get(), kInvalid, + sizeof(kInvalid))) { + return Certs{}; + } + + return certs; + }; + + Certs root(make_certs("Root", "Root", /*is_ca=*/true)); + ASSERT_TRUE(root.valid); + Certs intermediate(make_certs("Root", "Intermediate", /*is_ca=*/true)); + ASSERT_TRUE(intermediate.valid); + Certs leaf(make_certs("Intermediate", "Leaf", /*is_ca=*/false)); + ASSERT_TRUE(leaf.valid); + + // Check the base chain. + EXPECT_EQ(X509_V_OK, Verify(leaf.valid.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); + + // An invalid or unsupported signature in the leaf or intermediate is noticed. + EXPECT_EQ(X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(leaf.bad_sig.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(leaf.bad_sig_type.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(leaf.valid.get(), {root.valid.get()}, + {intermediate.bad_sig.get()}, {})); + EXPECT_EQ(X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(leaf.valid.get(), {root.valid.get()}, + {intermediate.bad_sig_type.get()}, {})); + + // By default, the redundant root signature is not checked. + EXPECT_EQ(X509_V_OK, Verify(leaf.valid.get(), {root.bad_sig.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_OK, Verify(leaf.valid.get(), {root.bad_sig_type.get()}, + {intermediate.valid.get()}, {})); + + // The caller can request checking it, although it's pointless. + EXPECT_EQ( + X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(leaf.valid.get(), {root.bad_sig.get()}, {intermediate.valid.get()}, + {}, X509_V_FLAG_CHECK_SS_SIGNATURE)); + EXPECT_EQ( + X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(leaf.valid.get(), {root.bad_sig_type.get()}, + {intermediate.valid.get()}, {}, X509_V_FLAG_CHECK_SS_SIGNATURE)); + + // The above also applies when accepting a trusted, self-signed root as the + // target certificate. + EXPECT_EQ(X509_V_OK, + Verify(root.bad_sig.get(), {root.bad_sig.get()}, {}, {})); + EXPECT_EQ(X509_V_OK, + Verify(root.bad_sig_type.get(), {root.bad_sig_type.get()}, {}, {})); + EXPECT_EQ(X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(root.bad_sig.get(), {root.bad_sig.get()}, {}, {}, + X509_V_FLAG_CHECK_SS_SIGNATURE)); + EXPECT_EQ(X509_V_ERR_CERT_SIGNATURE_FAILURE, + Verify(root.bad_sig_type.get(), {root.bad_sig_type.get()}, {}, {}, + X509_V_FLAG_CHECK_SS_SIGNATURE)); + + // If an intermediate is a trust anchor, the redundant signature is always + // ignored, even with |X509_V_FLAG_CHECK_SS_SIGNATURE|. (We cannot check the + // signature without the key.) + EXPECT_EQ(X509_V_OK, + Verify(leaf.valid.get(), {intermediate.bad_sig.get()}, {}, {}, + X509_V_FLAG_CHECK_SS_SIGNATURE | X509_V_FLAG_PARTIAL_CHAIN)); + EXPECT_EQ(X509_V_OK, + Verify(leaf.valid.get(), {intermediate.bad_sig_type.get()}, {}, {}, + X509_V_FLAG_CHECK_SS_SIGNATURE | X509_V_FLAG_PARTIAL_CHAIN)); + EXPECT_EQ(X509_V_OK, Verify(leaf.valid.get(), {intermediate.bad_sig.get()}, + {}, {}, X509_V_FLAG_PARTIAL_CHAIN)); + EXPECT_EQ(X509_V_OK, + Verify(leaf.valid.get(), {intermediate.bad_sig_type.get()}, {}, {}, + X509_V_FLAG_PARTIAL_CHAIN)); + + // Bad keys in the root and intermediate are rejected. + EXPECT_EQ(X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY, + Verify(leaf.valid.get(), {root.bad_key.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY, + Verify(leaf.valid.get(), {root.bad_key_type.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY, + Verify(leaf.valid.get(), {root.valid.get()}, + {intermediate.bad_key.get()}, {})); + EXPECT_EQ(X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY, + Verify(leaf.valid.get(), {root.valid.get()}, + {intermediate.bad_key_type.get()}, {})); + + // Bad keys in the leaf are ignored. The leaf's key is used by the caller. + EXPECT_EQ(X509_V_OK, Verify(leaf.bad_key.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); + EXPECT_EQ(X509_V_OK, Verify(leaf.bad_key_type.get(), {root.valid.get()}, + {intermediate.valid.get()}, {})); +} + // kConstructedBitString is an X.509 certificate where the signature is encoded // as a BER constructed BIT STRING. Note that, while OpenSSL's parser accepts // this input, it interprets the value incorrectly. From 5b3dc49c1271554f73b976c2c625600d6bd912b0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 21 Dec 2023 19:19:16 -0500 Subject: [PATCH 2/2] Eagerly compute the cached EVP_PKEY in X509_PUBKEY Whenever the object is mutated, we can simply refresh the cached EVP_PKEY. This aligns with OpenSSL, which computes it eagerly these days. This removes the need to lock things, and also makes it easy to implement the get0 versions of the functions from OpenSSL. Change-Id: Ib17b654af694817edc43e4742d9baf9ed05c676e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65050 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- crypto/x509/x509_cmp.c | 6 +-- crypto/x509/x509_req.c | 2 +- crypto/x509/x509spki.c | 2 +- crypto/x509/x_pubkey.c | 91 ++++++++++++++++++------------------------ include/openssl/x509.h | 47 ++++++++++++++-------- 5 files changed, 74 insertions(+), 74 deletions(-) diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c index 714212a30..9574c9b0b 100644 --- a/crypto/x509/x509_cmp.c +++ b/crypto/x509/x509_cmp.c @@ -218,11 +218,11 @@ X509 *X509_find_by_subject(const STACK_OF(X509) *sk, X509_NAME *name) { return NULL; } -EVP_PKEY *X509_get_pubkey(X509 *x) { - if ((x == NULL) || (x->cert_info == NULL)) { +EVP_PKEY *X509_get_pubkey(const X509 *x) { + if (x == NULL) { return NULL; } - return (X509_PUBKEY_get(x->cert_info->key)); + return X509_PUBKEY_get(x->cert_info->key); } ASN1_BIT_STRING *X509_get0_pubkey_bitstr(const X509 *x) { diff --git a/crypto/x509/x509_req.c b/crypto/x509/x509_req.c index 6b14a8c60..3c192ffbc 100644 --- a/crypto/x509/x509_req.c +++ b/crypto/x509/x509_req.c @@ -76,7 +76,7 @@ X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req) { return req->req_info->subject; } -EVP_PKEY *X509_REQ_get_pubkey(X509_REQ *req) { +EVP_PKEY *X509_REQ_get_pubkey(const X509_REQ *req) { if ((req == NULL) || (req->req_info == NULL)) { return NULL; } diff --git a/crypto/x509/x509spki.c b/crypto/x509/x509spki.c index 2b9b904ee..611a05f44 100644 --- a/crypto/x509/x509spki.c +++ b/crypto/x509/x509spki.c @@ -68,7 +68,7 @@ int NETSCAPE_SPKI_set_pubkey(NETSCAPE_SPKI *x, EVP_PKEY *pkey) { return (X509_PUBKEY_set(&(x->spkac->pubkey), pkey)); } -EVP_PKEY *NETSCAPE_SPKI_get_pubkey(NETSCAPE_SPKI *x) { +EVP_PKEY *NETSCAPE_SPKI_get_pubkey(const NETSCAPE_SPKI *x) { if ((x == NULL) || (x->spkac == NULL)) { return NULL; } diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c index 67ce46482..1428e347a 100644 --- a/crypto/x509/x_pubkey.c +++ b/crypto/x509/x_pubkey.c @@ -65,26 +65,46 @@ #include #include #include -#include #include "../internal.h" #include "internal.h" static void x509_pubkey_changed(X509_PUBKEY *pub) { - // TODO(davidben): Instead of just dropping the key, also compute the new - // cached key. This will let us implement |X509_get0_pubkey| and remove the - // need for a mutex. EVP_PKEY_free(pub->pkey); pub->pkey = NULL; + + // Re-encode the |X509_PUBKEY| to DER and parse it with EVP's APIs. + uint8_t *spki = NULL; + int spki_len = i2d_X509_PUBKEY(pub, &spki); + if (spki_len < 0) { + goto err; + } + + CBS cbs; + CBS_init(&cbs, spki, (size_t)spki_len); + EVP_PKEY *pkey = EVP_parse_public_key(&cbs); + if (pkey == NULL || CBS_len(&cbs) != 0) { + EVP_PKEY_free(pkey); + goto err; + } + + pub->pkey = pkey; + +err: + OPENSSL_free(spki); + // If the operation failed, clear errors. An |X509_PUBKEY| whose key we cannot + // parse is still a valid SPKI. It just cannot be converted to an |EVP_PKEY|. + ERR_clear_error(); } -// Minor tweak to operation: free up EVP_PKEY static int pubkey_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, void *exarg) { + X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval; if (operation == ASN1_OP_FREE_POST) { - X509_PUBKEY *pubkey = (X509_PUBKEY *)*pval; EVP_PKEY_free(pubkey->pkey); + } else if (operation == ASN1_OP_D2I_POST) { + x509_pubkey_changed(pubkey); } return 1; } @@ -133,60 +153,25 @@ error: return 0; } -// g_pubkey_lock is used to protect the initialisation of the |pkey| member of -// |X509_PUBKEY| objects. Really |X509_PUBKEY| should have a |CRYPTO_once_t| -// inside it for this, but |CRYPTO_once_t| is private and |X509_PUBKEY| is -// not. -static CRYPTO_MUTEX g_pubkey_lock = CRYPTO_MUTEX_INIT; - -EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) { - EVP_PKEY *ret = NULL; - uint8_t *spki = NULL; - +EVP_PKEY *X509_PUBKEY_get0(const X509_PUBKEY *key) { if (key == NULL) { - goto error; + return NULL; } - CRYPTO_MUTEX_lock_read(&g_pubkey_lock); - if (key->pkey != NULL) { - CRYPTO_MUTEX_unlock_read(&g_pubkey_lock); - EVP_PKEY_up_ref(key->pkey); - return key->pkey; - } - CRYPTO_MUTEX_unlock_read(&g_pubkey_lock); - - // Re-encode the |X509_PUBKEY| to DER and parse it. - int spki_len = i2d_X509_PUBKEY(key, &spki); - if (spki_len < 0) { - goto error; - } - CBS cbs; - CBS_init(&cbs, spki, (size_t)spki_len); - ret = EVP_parse_public_key(&cbs); - if (ret == NULL || CBS_len(&cbs) != 0) { + if (key->pkey == NULL) { OPENSSL_PUT_ERROR(X509, X509_R_PUBLIC_KEY_DECODE_ERROR); - goto error; + return NULL; } - // Check to see if another thread set key->pkey first - CRYPTO_MUTEX_lock_write(&g_pubkey_lock); - if (key->pkey) { - CRYPTO_MUTEX_unlock_write(&g_pubkey_lock); - EVP_PKEY_free(ret); - ret = key->pkey; - } else { - key->pkey = ret; - CRYPTO_MUTEX_unlock_write(&g_pubkey_lock); + return key->pkey; +} + +EVP_PKEY *X509_PUBKEY_get(const X509_PUBKEY *key) { + EVP_PKEY *pkey = X509_PUBKEY_get0(key); + if (pkey != NULL) { + EVP_PKEY_up_ref(pkey); } - - OPENSSL_free(spki); - EVP_PKEY_up_ref(ret); - return ret; - -error: - OPENSSL_free(spki); - EVP_PKEY_free(ret); - return NULL; + return pkey; } int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *obj, int param_type, diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 8fe59c786..697f1938b 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -198,11 +198,16 @@ OPENSSL_EXPORT X509_NAME *X509_get_subject_name(const X509 *x509); // object. OPENSSL_EXPORT X509_PUBKEY *X509_get_X509_PUBKEY(const X509 *x509); -// X509_get_pubkey returns |x509|'s public key as an |EVP_PKEY|, or NULL if the -// public key was unsupported or could not be decoded. This function returns a -// reference to the |EVP_PKEY|. The caller must release the result with -// |EVP_PKEY_free| when done. -OPENSSL_EXPORT EVP_PKEY *X509_get_pubkey(X509 *x509); +// X509_get0_pubkey returns |x509|'s public key as an |EVP_PKEY|, or NULL if the +// public key was unsupported or could not be decoded. The |EVP_PKEY| is cached +// in |x509|, so callers must not mutate the result. +OPENSSL_EXPORT EVP_PKEY *X509_get0_pubkey(const X509 *x509); + +// X509_get_pubkey behaves like |X509_get0_pubkey| but increments the reference +// count on the |EVP_PKEY|. The caller must release the result with +// |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |x509|, so callers +// must not mutate the result. +OPENSSL_EXPORT EVP_PKEY *X509_get_pubkey(const X509 *x509); // X509_get0_pubkey_bitstr returns the BIT STRING portion of |x509|'s public // key. Note this does not contain the AlgorithmIdentifier portion. @@ -1110,11 +1115,16 @@ OPENSSL_EXPORT long X509_REQ_get_version(const X509_REQ *req); // not const-correct for legacy reasons. OPENSSL_EXPORT X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req); -// X509_REQ_get_pubkey returns |req|'s public key as an |EVP_PKEY|, or NULL if -// the public key was unsupported or could not be decoded. This function returns -// a reference to the |EVP_PKEY|. The caller must release the result with -// |EVP_PKEY_free| when done. -OPENSSL_EXPORT EVP_PKEY *X509_REQ_get_pubkey(X509_REQ *req); +// X509_REQ_get0_pubkey returns |req|'s public key as an |EVP_PKEY|, or NULL if +// the public key was unsupported or could not be decoded. The |EVP_PKEY| is +// cached in |req|, so callers must not mutate the result. +OPENSSL_EXPORT EVP_PKEY *X509_REQ_get0_pubkey(const X509_REQ *req); + +// X509_REQ_get_pubkey behaves like |X509_REQ_get0_pubkey| but increments the +// reference count on the |EVP_PKEY|. The caller must release the result with +// |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |req|, so callers must +// not mutate the result. +OPENSSL_EXPORT EVP_PKEY *X509_REQ_get_pubkey(const X509_REQ *req); // X509_REQ_check_private_key returns one if |req|'s public key matches |pkey| // and zero otherwise. @@ -1578,11 +1588,16 @@ OPENSSL_EXPORT int i2d_X509_PUBKEY(const X509_PUBKEY *key, uint8_t **outp); // object, and returns one. Otherwise, it returns zero. OPENSSL_EXPORT int X509_PUBKEY_set(X509_PUBKEY **x, EVP_PKEY *pkey); -// X509_PUBKEY_get decodes the public key in |key| and returns an |EVP_PKEY| on -// success, or NULL on error or unrecognized algorithm. The caller must release -// the result with |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |key|, -// so callers must not mutate the result. -OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key); +// X509_PUBKEY_get0 returns |key| as an |EVP_PKEY|, or NULL if |key| either +// could not be parsed or is an unrecognized algorithm. The |EVP_PKEY| is cached +// in |key|, so callers must not mutate the result. +OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get0(const X509_PUBKEY *key); + +// X509_PUBKEY_get behaves like |X509_PUBKEY_get0| but increments the reference +// count on the |EVP_PKEY|. The caller must release the result with +// |EVP_PKEY_free| when done. The |EVP_PKEY| is cached in |key|, so callers must +// not mutate the result. +OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get(const X509_PUBKEY *key); // X509_PUBKEY_set0_param sets |pub| to a key with AlgorithmIdentifier // determined by |obj|, |param_type|, and |param_value|, and an encoded @@ -2218,7 +2233,7 @@ OPENSSL_EXPORT char *NETSCAPE_SPKI_b64_encode(NETSCAPE_SPKI *spki); // NETSCAPE_SPKI_get_pubkey decodes and returns the public key in |spki| as an // |EVP_PKEY|, or NULL on error. The caller takes ownership of the resulting // pointer and must call |EVP_PKEY_free| when done. -OPENSSL_EXPORT EVP_PKEY *NETSCAPE_SPKI_get_pubkey(NETSCAPE_SPKI *spki); +OPENSSL_EXPORT EVP_PKEY *NETSCAPE_SPKI_get_pubkey(const NETSCAPE_SPKI *spki); // NETSCAPE_SPKI_set_pubkey sets |spki|'s public key to |pkey|. It returns one // on success or zero on error. This function does not take ownership of |pkey|,