crypto/x509: Add a helper to get cached TBS cert or else marshal it As a convenience, to avoid making a copy when possible. Migrates X509_verify() from the previous helper, which always makes a copy, to the new helper which sometimes avoids a copy. Change-Id: I76f771e4f0a591fe615fd0162038db866a6a6964 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/100450 Commit-Queue: Lily Chen <chlily@google.com> Reviewed-by: David Benjamin <davidben@google.com> Auto-Submit: Lily Chen <chlily@google.com>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index c238921..17d6eeb 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h
@@ -189,8 +189,20 @@ ~X509Impl(); } /* X509 */; +// x509_marshal_tbs_cert sets `cbb` to the serialized TBSCertificate of `x509`. +// It either replays the saved TBSCertificate encoding from the `CRYPTO_BUFFER`, +// or marshals the TBSCertificate from fields set on `x509`. It returns one on +// success or zero on error. int x509_marshal_tbs_cert(CBB *cbb, const X509 *x509); +// x509_get_or_marshal_tbs_cert sets `out` to the serialized TBSCertificate of +// `x509`. If possible, it gets the saved TBSCertificate encoding from the +// `CRYPTO_BUFFER` of `x509`, otherwise it marshals the TBSCertificate from +// fields set on `x509` into `scratch`. It returns one on success or zero on +// error. +int x509_get_or_marshal_tbs_cert(CBS *out, Array<uint8_t> *scratch, + const X509 *x509); + // X509 is an `ASN1_ITEM` whose ASN.1 type is X.509 Certificate (RFC 5280) and C // type is `X509*`. DECLARE_ASN1_ITEM(X509)
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index f98aa26..d8b428b 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -10671,6 +10671,75 @@ {/*self_issued=*/true, /*pathlen=*/1}})); } +#if !defined(BORINGSSL_SHARED_LIBRARY) +TEST(X509Test, MarshalTBSCertCached) { + UniquePtr<X509> cert = CertFromPEM(kLeafPEM); + ASSERT_TRUE(cert); + + ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 64)); + EXPECT_TRUE(x509_marshal_tbs_cert(cbb.get(), cert.get())); + + CBS tbs; + Array<uint8_t> scratch; + EXPECT_TRUE(x509_get_or_marshal_tbs_cert(&tbs, &scratch, cert.get())); + + // The non-copying and copying helper should return identical data. + EXPECT_EQ(Bytes(CBS_data(&tbs), CBS_len(&tbs)), + Bytes(CBB_data(cbb.get()), CBB_len(cbb.get()))); + + // Check that the non-copying helper returned data within the CRYPTO_BUFFER. + auto *impl = FromOpaque(cert.get()); + ASSERT_TRUE(impl->buf); + const uint8_t *buf_data = CRYPTO_BUFFER_data(impl->buf.get()); + size_t buf_len = CRYPTO_BUFFER_len(impl->buf.get()); + EXPECT_GE(CBS_data(&tbs), buf_data); + EXPECT_LE(CBS_data(&tbs) + CBS_len(&tbs), buf_data + buf_len); +} + +TEST(X509Test, MarshalTBSCertNoCache) { + // Create a programmatically constructed certificate (no cached TBSCert). + UniquePtr<EVP_PKEY> pkey(PrivateKeyFromPEM(kRSAKey)); + ASSERT_TRUE(pkey); + + UniquePtr<X509> cert(X509_new()); + ASSERT_TRUE(cert); + + EXPECT_TRUE(X509_set_version(cert.get(), X509_VERSION_3)); + EXPECT_TRUE(ASN1_INTEGER_set_int64(X509_get_serialNumber(cert.get()), 1)); + EXPECT_TRUE(X509_gmtime_adj(X509_getm_notBefore(cert.get()), 0)); + EXPECT_TRUE(X509_gmtime_adj(X509_getm_notAfter(cert.get()), 60 * 60 * 24)); + X509_NAME *subject = X509_get_subject_name(cert.get()); + ASSERT_TRUE(X509_NAME_add_entry_by_txt( + subject, "CN", MBSTRING_ASC, reinterpret_cast<const uint8_t *>("Test"), + -1, -1, 0)); + EXPECT_TRUE(X509_set_issuer_name(cert.get(), subject)); + EXPECT_TRUE(X509_set_pubkey(cert.get(), pkey.get())); + + UniquePtr<X509_ALGOR> algor(X509_ALGOR_new()); + ASSERT_TRUE(algor); + ASSERT_TRUE(X509_ALGOR_set0(algor.get(), + OBJ_nid2obj(NID_sha256WithRSAEncryption), + V_ASN1_NULL, nullptr)); + ASSERT_TRUE(X509_set1_signature_algo(cert.get(), algor.get())); + + // There is no cached encoding to return. + ASSERT_FALSE(FromOpaque(cert.get())->buf); + + ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 64)); + EXPECT_TRUE(x509_marshal_tbs_cert(cbb.get(), cert.get())); + + CBS tbs; + Array<uint8_t> scratch; + EXPECT_TRUE(x509_get_or_marshal_tbs_cert(&tbs, &scratch, cert.get())); + + // Both helpers should return identical data. + EXPECT_EQ(Bytes(CBS_data(&tbs), CBS_len(&tbs)), + Bytes(CBB_data(cbb.get()), CBB_len(cbb.get()))); +} +#endif // !defined(BORINGSSL_SHARED_LIBRARY) + // Tests for `x509_evaluate_mtc_subtree_inclusion_proof`, which is an // internal-only function. #if !defined(BORINGSSL_SHARED_LIBRARY)
diff --git a/crypto/x509/x_all.cc b/crypto/x509/x_all.cc index 7a50617..b9990b8 100644 --- a/crypto/x509/x_all.cc +++ b/crypto/x509/x_all.cc
@@ -42,12 +42,13 @@ return 0; } // This uses the cached TBSCertificate encoding, if any. - ScopedCBB cbb; - if (!CBB_init(cbb.get(), 128) || !x509_marshal_tbs_cert(cbb.get(), x509)) { + Array<uint8_t> scratch; + CBS tbs; + if (!x509_get_or_marshal_tbs_cert(&tbs, &scratch, x509)) { return 0; } - return x509_verify_signature(impl->sig_alg.get(), impl->signature.get(), - CBBAsSpan(cbb.get()), pkey); + return x509_verify_signature(impl->sig_alg.get(), impl->signature.get(), tbs, + pkey); } int X509_REQ_verify(const X509_REQ *req, EVP_PKEY *pkey) {
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc index 21f5380..413c40f 100644 --- a/crypto/x509/x_x509.cc +++ b/crypto/x509/x_x509.cc
@@ -254,6 +254,32 @@ return CBB_flush(cbb); } +int bssl::x509_get_or_marshal_tbs_cert(CBS *out, Array<uint8_t> *scratch, + const X509 *x509) { + auto *impl = FromOpaque(x509); + if (impl->buf != nullptr) { + // If there is a cached TBSCertificate encoding, just use it and avoid + // making a copy. + CBS cbs, cert; + CRYPTO_BUFFER_init_CBS(impl->buf.get(), &cbs); + if (!CBS_get_asn1(&cbs, &cert, CBS_ASN1_SEQUENCE) || + !CBS_get_asn1_element(&cert, out, CBS_ASN1_SEQUENCE)) { + // This should be impossible. + OPENSSL_PUT_ERROR(X509, ERR_R_INTERNAL_ERROR); + return 0; + } + } else { + ScopedCBB cbb; + if (!CBB_init(cbb.get(), 128) || !x509_marshal_tbs_cert(cbb.get(), x509) || + !CBBFinishArray(cbb.get(), scratch)) { + OPENSSL_PUT_ERROR(X509, ERR_R_INTERNAL_ERROR); + return 0; + } + CBS_init(out, scratch->data(), scratch->size()); + } + return 1; +} + static int x509_marshal(CBB *cbb, const X509 *x509) { CBB cert; auto *impl = FromOpaque(x509);