From 496838a41073951d1db831dac5156a0bc9f2cc88 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 25 Dec 2023 16:51:37 -0500 Subject: [PATCH 1/4] Remove X509_TRUST_OCSP_SIGN and X509_TRUST_OCSP_REQUEST These are unused and are the only options that remove the "compat" self-signed fallback. X509_TRUST_OCSP_REQUEST was intended for checking signed OCSP requests. While OpenSSL's OCSP implementation (which we've dropped) does attempt to configure it, it actually does nothing. They call X509_STORE_CTX_set_trust after X509_STORE_CTX_set_purpose, but X509_STORE_CTX_set_purpose already sets the trust parameter and X509_STORE_CTX_set_trust only acts when trust is not configured. X509_TRUST_OCSP_SIGN was briefly used in upstream's 30c278aa6bb614f4cfc5a26c7cbe66ad090f6896, by way of X509_PURPOSE_OCSP_HELPER, but then immediately undone in e9754726d236b74476cd0be5fa60acfef0c7024f. Change-Id: I6d2cf9b88a6b013e74fe95cd88f94051111086df Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65151 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 74bab4cf7b9c7c2fd9c37b3eecfa059f1ffc218a) --- crypto/x509/x509_trs.c | 12 ------------ include/openssl/x509.h | 2 -- 2 files changed, 14 deletions(-) diff --git a/crypto/x509/x509_trs.c b/crypto/x509/x509_trs.c index 680f6d651e..95daf46657 100644 --- a/crypto/x509/x509_trs.c +++ b/crypto/x509/x509_trs.c @@ -67,7 +67,6 @@ static int trust_1oidany(const X509_TRUST *trust, X509 *x, int flags); -static int trust_1oid(const X509_TRUST *trust, X509 *x, int flags); static int trust_compat(const X509_TRUST *trust, X509 *x, int flags); static int obj_trust(int id, X509 *x, int flags); @@ -82,10 +81,6 @@ static const X509_TRUST trstandard[] = { NID_email_protect, NULL}, {X509_TRUST_OBJECT_SIGN, 0, trust_1oidany, (char *)"Object Signer", NID_code_sign, NULL}, - {X509_TRUST_OCSP_SIGN, 0, trust_1oid, (char *)"OCSP responder", - NID_OCSP_sign, NULL}, - {X509_TRUST_OCSP_REQUEST, 0, trust_1oid, (char *)"OCSP request", - NID_ad_OCSP, NULL}, {X509_TRUST_TSA, 0, trust_1oidany, (char *)"TSA server", NID_time_stamp, NULL}}; @@ -155,13 +150,6 @@ static int trust_1oidany(const X509_TRUST *trust, X509 *x, int flags) { return trust_compat(trust, x, flags); } -static int trust_1oid(const X509_TRUST *trust, X509 *x, int flags) { - if (x->aux) { - return obj_trust(trust->arg1, x, flags); - } - return X509_TRUST_UNTRUSTED; -} - static int trust_compat(const X509_TRUST *trust, X509 *x, int flags) { if (!x509v3_cache_extensions(x)) { return X509_TRUST_UNTRUSTED; diff --git a/include/openssl/x509.h b/include/openssl/x509.h index e157f15c35..a0cf4ce9f2 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -3652,8 +3652,6 @@ DEFINE_STACK_OF(X509_TRUST) #define X509_TRUST_SSL_SERVER 3 #define X509_TRUST_EMAIL 4 #define X509_TRUST_OBJECT_SIGN 5 -#define X509_TRUST_OCSP_SIGN 6 -#define X509_TRUST_OCSP_REQUEST 7 #define X509_TRUST_TSA 8 // check_trust return codes From a158dcc582275b388bd406160428f517bf2ab8c8 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Mon, 8 Jan 2024 15:27:51 -0800 Subject: [PATCH 2/4] Don't define OPENSSL_LINUX for CROS_EC and CROS_ZEPHYR CrOS EC and Zephyr build "emulation" targets that run in Linux userspace. Although running on Linux, we want boringssl to run the same as if it were running on the embedded target. BUG=b/273639386 Change-Id: Id5f13391f09889e955d2a86e2c5317903b2a8bd6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65182 Reviewed-by: David Benjamin Reviewed-by: Tom Hughes Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit fcca096315377c8ed18bd8cbecf66c85bc63ad2d) --- include/openssl/target.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/openssl/target.h b/include/openssl/target.h index feb0bdfd4c..35f1add1aa 100644 --- a/include/openssl/target.h +++ b/include/openssl/target.h @@ -87,7 +87,8 @@ #endif // Trusty and Android baremetal aren't Linux but currently define __linux__. -// As a workaround, we exclude them here. We also exclude nanolibc. nanolibc +// As a workaround, we exclude them here. +// We also exclude nanolibc/CrOS EC/Zephyr. nanolibc/CrOS EC/Zephyr // sometimes build for a non-Linux target (which should not define __linux__), // but also sometimes build for Linux. Although technically running in Linux // userspace, this lacks all the libc APIs we'd normally expect on Linux, so we @@ -97,7 +98,8 @@ // TODO(b/291101350): Remove this workaround once Android baremetal no longer // defines it. #if defined(__linux__) && !defined(__TRUSTY__) && \ - !defined(ANDROID_BAREMETAL) && !defined(OPENSSL_NANOLIBC) + !defined(ANDROID_BAREMETAL) && !defined(OPENSSL_NANOLIBC) && \ + !defined(CROS_EC) && !defined(CROS_ZEPHYR) #define OPENSSL_LINUX #endif From 1b966add0ad6be6ebc174358b81ae95841b6e46c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 25 Dec 2023 17:55:24 -0500 Subject: [PATCH 3/4] Add some more TSan tests for crypto/x509 Test CRL handling, leave a TODO for bug 600, and also test that one can add to an X509_STORE while verifying, as that's meant to work. As part of this, I refreshed the test CRL so the sort wasn't degenerate. When I inject a bug in generating the sorted CRL, TSan still only flakily notices, but it does eventually notice. Bug: 600 Change-Id: I0ae92651dcac9971b034cf9f1c127e9a25332bf5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65152 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit d62bd8ebd7f0e5eab1eb03197fd042094f7d0f45) --- crypto/x509/x509_test.cc | 99 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 9 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 9cb794de16..6d78309855 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -400,16 +400,17 @@ diyu0fZ/bPAM3VAGawatf/SyWfBMyKpoPXEG39oAzmjjOj8en82psn7m474IGaho static const char kRevokedCRL[] = R"( -----BEGIN X509 CRL----- -MIIBvjCBpwIBATANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE +MIIB6DCB0QIBATANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ -Qm9yaW5nU1NMFw0xNjA5MjYxNTEyNDRaFw0xNjEwMjYxNTEyNDRaMBUwEwICEAAX -DTE2MDkyNjE1MTIyNlqgDjAMMAoGA1UdFAQDAgECMA0GCSqGSIb3DQEBCwUAA4IB -AQCUGaM4DcWzlQKrcZvI8TMeR8BpsvQeo5BoI/XZu2a8h//PyRyMwYeaOM+3zl0d -sjgCT8b3C1FPgT+P2Lkowv7rJ+FHJRNQkogr+RuqCSPTq65ha4WKlRGWkMFybzVH -NloxC+aU3lgp/NlX9yUtfqYmJek1CDrOOGPrAEAwj1l/BUeYKNGqfBWYJQtPJu+5 -OaSvIYGpETCZJscUWODmLEb/O3DM438vLvxonwGqXqS0KX37+CHpUlyhnSovxXxp -Pz4aF+L7OtczxL0GYtD2fR9B7TDMqsNmHXgQrixvvOY7MUdLGbd4RfJL3yA53hyO -xzfKY2TzxLiOmctG0hXFkH5J +Qm9yaW5nU1NMFw0xNjA5MjYxNTEyNDRaFw0xNjEwMjYxNTEyNDRaMD8wEwICEAAX +DTE2MDkyNjE1MTIyNlowEwICD/8XDTE2MDkyNjE1MTIyNlowEwICEAEXDTE2MDky +NjE1MTIyNlqgDjAMMAoGA1UdFAQDAgECMA0GCSqGSIb3DQEBCwUAA4IBAQAuepA9 +byX4PkpJcYKb+Ofn6f/mgB4Sh1BBRp0HMFm7Q3jryXB6yPZYp7UtwAufZUzbV42U +kOifOtDyQbu+fcpnpb4D9oWoFlr4DGQ+n23wujHizoTEYhhcZMj4U5yooDfmK4lI +GU6zzQZKG+1PaS6Dm4f6+kA+ekVUP+ZVjPqHP/K7Uv6akSKV7gAWs49N5QjrJKMQ +3Igrbg4Et2ipaYgThGj8t1MUZdVY4UPtQ8oltSHkFEvH4PxOW/xKpHT4QQMl/WTT +QsQqOlRJBG2ci7pu1fzOylY35YFEAApkND/MkQjQfylNNgCzDWWAPQx0pDvVKQ0v +WXdfcGJRL1D3xRXk -----END X509 CRL----- )"; @@ -1740,6 +1741,86 @@ TEST(X509Test, VerifyThreads) { thread.join(); } } + +// Using the same CRL on two threads should be safe. +TEST(X509Test, CRLThreads) { + bssl::UniquePtr root(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr leaf(CertFromPEM(kCRLTestLeaf)); + bssl::UniquePtr basic_crl(CRLFromPEM(kBasicCRL)); + bssl::UniquePtr revoked_crl(CRLFromPEM(kRevokedCRL)); + ASSERT_TRUE(root); + ASSERT_TRUE(leaf); + ASSERT_TRUE(basic_crl); + ASSERT_TRUE(revoked_crl); + + const size_t kNumThreads = 10; + std::vector threads; + for (size_t i = 0; i < kNumThreads; i++) { + threads.emplace_back([&] { + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {root.get()}, + {basic_crl.get()}, X509_V_FLAG_CRL_CHECK)); + }); + threads.emplace_back([&] { + EXPECT_EQ(X509_V_ERR_CERT_REVOKED, + Verify(leaf.get(), {root.get()}, {root.get()}, + {revoked_crl.get()}, X509_V_FLAG_CRL_CHECK)); + }); + } + + for (auto &thread : threads) { + thread.join(); + } + + // TODO(crbug.com/boringssl/600): Add a thread that iterates + // |X509_CRL_get_REVOKED| and a thread that calls |X509_CRL_print|. Those + // currently do not work correctly. +} + +TEST(X509Test, StoreThreads) { + bssl::UniquePtr root(CertFromPEM(kRootCAPEM)); + bssl::UniquePtr intermediate(CertFromPEM(kIntermediatePEM)); + bssl::UniquePtr leaf(CertFromPEM(kLeafPEM)); + ASSERT_TRUE(root); + ASSERT_TRUE(intermediate); + ASSERT_TRUE(leaf); + + bssl::UniquePtr intermediates = + CertsToStack({intermediate.get()}); + ASSERT_TRUE(intermediates); + + // Some unrelated certificates. + bssl::UniquePtr other1(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr other2(CertFromPEM(kCRLTestLeaf)); + ASSERT_TRUE(other1); + ASSERT_TRUE(other2); + + bssl::UniquePtr store(X509_STORE_new()); + ASSERT_TRUE(store); + ASSERT_TRUE(X509_STORE_add_cert(store.get(), root.get())); + + const size_t kNumThreads = 10; + std::vector threads; + for (size_t i = 0; i < kNumThreads; i++) { + threads.emplace_back([&] { + bssl::UniquePtr ctx(X509_STORE_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), leaf.get(), + intermediates.get())); + X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime); + ASSERT_TRUE(X509_verify_cert(ctx.get())); + ASSERT_EQ(X509_STORE_CTX_get_error(ctx.get()), X509_V_OK); + }); + threads.emplace_back([&] { + ASSERT_TRUE(X509_STORE_add_cert(store.get(), other1.get())); + }); + threads.emplace_back([&] { + ASSERT_TRUE(X509_STORE_add_cert(store.get(), other2.get())); + }); + } + for (auto &thread : threads) { + thread.join(); + } +} #endif // OPENSSL_THREADS static const char kHostname[] = "example.com"; From 5e1522d7a7ec9ccf4347cb4dbd0c27145e1c80a8 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 25 Dec 2023 22:52:22 -0500 Subject: [PATCH 4/4] Remove all -1 returns from X509_check_purpose Of external callers of this function, almost all are not actually doing anything with this operation and are just trying to trigger x509v3_cache_extensions. Triggering that is no longer necessarily now that the structure is opaque and accessors do it for you. There were three callers that wanted the actual operation here. One of them correctly handled the tri-state return, but did not distinguish 0 from -1. The other two did not and would misinterpret -1 as success! So this change is actually more compatible with OpenSSL callers than OpenSSL's actual behavior. Change-Id: Ifedba52dd9d4e031fc919276fd08ec22cfd33bf2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65153 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit 0c8bc4653e34892dc291b48fb38e180ce92b5921) --- crypto/x509/v3_purp.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c index a7a4e888e2..fdd920f779 100644 --- a/crypto/x509/v3_purp.c +++ b/crypto/x509/v3_purp.c @@ -118,26 +118,21 @@ static const X509_PURPOSE xstandard[] = { (char *)"timestampsign", NULL}, }; -// As much as I'd like to make X509_check_purpose use a "const" X509* I -// really can't because it does recalculate hashes and do other non-const -// things. If |id| is -1 it just calls |x509v3_cache_extensions| for its -// side-effect. -// Returns 1 on success, 0 if x does not allow purpose, -1 on (internal) error. int X509_check_purpose(X509 *x, int id, int ca) { - int idx; - const X509_PURPOSE *pt; + // This differs from OpenSSL, which uses -1 to indicate a fatal error and 0 to + // indicate an invalid certificate. BoringSSL uses 0 for both. if (!x509v3_cache_extensions(x)) { - return -1; + return 0; } if (id == -1) { return 1; } - idx = X509_PURPOSE_get_by_id(id); + int idx = X509_PURPOSE_get_by_id(id); if (idx == -1) { - return -1; + return 0; } - pt = X509_PURPOSE_get0(idx); + const X509_PURPOSE *pt = X509_PURPOSE_get0(idx); return pt->check_purpose(pt, x, ca); }