Require non-NULL store in X509_STORE_CTX_init.
X509_STORE_CTX_init is documented upstream to allow a NULL store and has
logic to account for it. However, attempting to use such an
X509_STORE_CTX crashes in X509_verify_cert due to the
additional_untrusted logic we added.
Moreover, before that change, it still crashes because
X509_STORE_CTX_get1_issuer (the default get_issuer hook) assumes
ctx->ctx (the store) is non-null. This was also true in upstream but
later fixed in https://github.com/openssl/openssl/pull/6001. However,
without a store, there is no trust anchor, so this is not very useful.
Reject NULL stores in X509_STORE_CTX_init and remove the logic allowing
for a NULL one.
Thanks to Danny Halawi for catching this.
Update-Note: X509_STORE_CTX_init will now fail when the store is NULL,
rather than report success, only to crash later in X509_verify_cert.
Breakage should thus be limited to code which was passing in a NULL
store but never used the resulting X509_STORE_CTX.
Change-Id: I9db0289612cc245a8d62d6fa647d6b56b2daabda
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42728
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index d71dee9..426e181 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -2445,3 +2445,13 @@
EXPECT_FALSE(CertFromPEM(kV1WithIssuerUniqueIDPEM));
EXPECT_FALSE(CertFromPEM(kV1WithSubjectUniqueIDPEM));
}
+
+// Unlike upstream OpenSSL, we require a non-null store in
+// |X509_STORE_CTX_init|.
+TEST(X509Test, NullStore) {
+ bssl::UniquePtr<X509> leaf(CertFromPEM(kLeafPEM));
+ ASSERT_TRUE(leaf);
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ ASSERT_TRUE(ctx);
+ EXPECT_FALSE(X509_STORE_CTX_init(ctx.get(), nullptr, leaf.get(), nullptr));
+}
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 9839b95..a997202 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -2307,8 +2307,6 @@
int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
STACK_OF(X509) *chain)
{
- int ret = 1;
-
X509_STORE_CTX_zero(ctx);
ctx->ctx = store;
ctx->cert = x509;
@@ -2316,78 +2314,74 @@
CRYPTO_new_ex_data(&ctx->ex_data);
+ if (store == NULL) {
+ OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
+ goto err;
+ }
+
ctx->param = X509_VERIFY_PARAM_new();
if (!ctx->param)
goto err;
/*
- * Inherit callbacks and flags from X509_STORE if not set use defaults.
+ * Inherit callbacks and flags from X509_STORE.
*/
- if (store)
- ret = X509_VERIFY_PARAM_inherit(ctx->param, store->param);
- else
- ctx->param->inh_flags |= X509_VP_FLAG_DEFAULT | X509_VP_FLAG_ONCE;
+ ctx->verify_cb = store->verify_cb;
+ ctx->cleanup = store->cleanup;
- if (store) {
- ctx->verify_cb = store->verify_cb;
- ctx->cleanup = store->cleanup;
- } else
- ctx->cleanup = 0;
-
- if (ret)
- ret = X509_VERIFY_PARAM_inherit(ctx->param,
- X509_VERIFY_PARAM_lookup("default"));
-
- if (ret == 0)
+ if (!X509_VERIFY_PARAM_inherit(ctx->param, store->param) ||
+ !X509_VERIFY_PARAM_inherit(ctx->param,
+ X509_VERIFY_PARAM_lookup("default"))) {
goto err;
+ }
- if (store && store->check_issued)
+ if (store->check_issued)
ctx->check_issued = store->check_issued;
else
ctx->check_issued = check_issued;
- if (store && store->get_issuer)
+ if (store->get_issuer)
ctx->get_issuer = store->get_issuer;
else
ctx->get_issuer = X509_STORE_CTX_get1_issuer;
- if (store && store->verify_cb)
+ if (store->verify_cb)
ctx->verify_cb = store->verify_cb;
else
ctx->verify_cb = null_callback;
- if (store && store->verify)
+ if (store->verify)
ctx->verify = store->verify;
else
ctx->verify = internal_verify;
- if (store && store->check_revocation)
+ if (store->check_revocation)
ctx->check_revocation = store->check_revocation;
else
ctx->check_revocation = check_revocation;
- if (store && store->get_crl)
+ if (store->get_crl)
ctx->get_crl = store->get_crl;
else
ctx->get_crl = NULL;
- if (store && store->check_crl)
+ if (store->check_crl)
ctx->check_crl = store->check_crl;
else
ctx->check_crl = check_crl;
- if (store && store->cert_crl)
+ if (store->cert_crl)
ctx->cert_crl = store->cert_crl;
else
ctx->cert_crl = cert_crl;
- if (store && store->lookup_certs)
+ if (store->lookup_certs)
ctx->lookup_certs = store->lookup_certs;
else
ctx->lookup_certs = X509_STORE_get1_certs;
- if (store && store->lookup_crls)
+ if (store->lookup_crls)
ctx->lookup_crls = store->lookup_crls;
else
ctx->lookup_crls = X509_STORE_get1_crls;
