diff --git a/src/crypto/x509/x509_cmp.c b/src/crypto/x509/x509_cmp.c index 714212a30..9574c9b0b 100644 --- a/src/crypto/x509/x509_cmp.c +++ b/src/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/src/crypto/x509/x509_req.c b/src/crypto/x509/x509_req.c index 6b14a8c60..3c192ffbc 100644 --- a/src/crypto/x509/x509_req.c +++ b/src/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/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc index 7d46a9d6e..8cfea574c 100644 --- a/src/crypto/x509/x509_test.cc +++ b/src/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. diff --git a/src/crypto/x509/x509spki.c b/src/crypto/x509/x509spki.c index 2b9b904ee..611a05f44 100644 --- a/src/crypto/x509/x509spki.c +++ b/src/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/src/crypto/x509/x_pubkey.c b/src/crypto/x509/x_pubkey.c index 67ce46482..1428e347a 100644 --- a/src/crypto/x509/x_pubkey.c +++ b/src/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/src/include/openssl/x509.h b/src/include/openssl/x509.h index 8fe59c786..697f1938b 100644 --- a/src/include/openssl/x509.h +++ b/src/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|,