Simplify some logic around X509_verify_cert callbacks
If we simply set ctx->get_crl to get_crl, as the other callbacks do, we
don't need to branch at the call site. Also get_issuer no longer needs
to be a callback. We can just check if X509_STORE_CTX_set0_trusted_stack
was called.
Change-Id: I42235f141ee9f8463631f56471c12324a8755bba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64988
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index bb256e5..d6debd2 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -348,9 +348,6 @@
CRYPTO_refcount_t references;
} /* X509_STORE */;
-typedef int (*X509_STORE_CTX_get_issuer_fn)(X509 **issuer, X509_STORE_CTX *ctx,
- X509 *x);
-
// This is the functions plus an instance of the local variables.
struct x509_lookup_st {
const X509_LOOKUP_METHOD *method; // the functions
@@ -371,11 +368,13 @@
STACK_OF(X509_CRL) *crls; // set of CRLs passed in
X509_VERIFY_PARAM *param;
- void *other_ctx; // Other info for use with get_issuer()
+
+ // trusted_stack, if non-NULL, is a set of trusted certificates to consider
+ // instead of those from |X509_STORE|.
+ STACK_OF(X509) *trusted_stack;
// Callbacks for various operations
X509_STORE_CTX_verify_cb verify_cb; // error callback
- X509_STORE_CTX_get_issuer_fn get_issuer; // get issuers cert from ctx
X509_STORE_CTX_get_crl_fn get_crl; // retrieve CRL
X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 68effaf..6bf366b 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -110,6 +110,7 @@
static int check_cert(X509_STORE_CTX *ctx);
static int check_policy(X509_STORE_CTX *ctx);
+static int get_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x);
static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, X509_CRL *crl,
X509 *x);
static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x);
@@ -233,7 +234,7 @@
}
// If asked see if we can find issuer in trusted store first
if (ctx->param->flags & X509_V_FLAG_TRUSTED_FIRST) {
- ok = ctx->get_issuer(&xtmp, ctx, x);
+ ok = get_issuer(&xtmp, ctx, x);
if (ok < 0) {
ctx->error = X509_V_ERR_STORE_LOOKUP;
goto end;
@@ -290,7 +291,7 @@
// We have a single self signed certificate: see if we can
// find it in the store. We must have an exact match to avoid
// possible impersonation.
- ok = ctx->get_issuer(&xtmp, ctx, x);
+ ok = get_issuer(&xtmp, ctx, x);
if ((ok <= 0) || X509_cmp(x, xtmp)) {
ctx->error = X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT;
ctx->current_cert = x;
@@ -335,7 +336,7 @@
if (is_self_signed) {
break;
}
- ok = ctx->get_issuer(&xtmp, ctx, x);
+ ok = get_issuer(&xtmp, ctx, x);
if (ok < 0) {
ctx->error = X509_V_ERR_STORE_LOOKUP;
@@ -372,7 +373,7 @@
!(ctx->param->flags & X509_V_FLAG_NO_ALT_CHAINS)) {
while (j-- > 1) {
xtmp2 = sk_X509_value(ctx->chain, j - 1);
- ok = ctx->get_issuer(&xtmp, ctx, xtmp2);
+ ok = get_issuer(&xtmp, ctx, xtmp2);
if (ok < 0) {
goto end;
}
@@ -511,16 +512,18 @@
return ctx->verify_cb(0, ctx);
}
-// Alternative lookup method: look from a STACK stored in other_ctx
-
-static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) {
- *issuer = find_issuer(ctx, ctx->other_ctx, x);
- if (*issuer) {
- X509_up_ref(*issuer);
- return 1;
- } else {
+static int get_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) {
+ if (ctx->trusted_stack != NULL) {
+ // Ignore the store and use the configured stack instead.
+ *issuer = find_issuer(ctx, ctx->trusted_stack, x);
+ if (*issuer) {
+ X509_up_ref(*issuer);
+ return 1;
+ }
return 0;
}
+
+ return X509_STORE_CTX_get1_issuer(issuer, ctx, x);
}
// Check a certificate chains extensions for consistency with the supplied
@@ -803,11 +806,7 @@
ctx->current_crl_score = 0;
// Try to retrieve relevant CRL
- if (ctx->get_crl) {
- ok = ctx->get_crl(ctx, &crl, x);
- } else {
- ok = get_crl(ctx, &crl, x);
- }
+ ok = ctx->get_crl(ctx, &crl, x);
// If error looking up CRL, nothing we can do except notify callback
if (!ok) {
ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL;
@@ -1668,10 +1667,6 @@
goto err;
}
- // TODO(davidben): Remove this pointer. It only exists to be overwritten by
- // X509_STORE_CTX_set0_trusted_stack.
- ctx->get_issuer = X509_STORE_CTX_get1_issuer;
-
if (store->verify_cb) {
ctx->verify_cb = store->verify_cb;
} else {
@@ -1681,7 +1676,7 @@
if (store->get_crl) {
ctx->get_crl = store->get_crl;
} else {
- ctx->get_crl = NULL;
+ ctx->get_crl = get_crl;
}
if (store->check_crl) {
@@ -1707,8 +1702,7 @@
void X509_STORE_CTX_set0_trusted_stack(X509_STORE_CTX *ctx,
STACK_OF(X509) *sk) {
- ctx->other_ctx = sk;
- ctx->get_issuer = get_issuer_sk;
+ ctx->trusted_stack = sk;
}
void X509_STORE_CTX_trusted_stack(X509_STORE_CTX *ctx, STACK_OF(X509) *sk) {
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index ba4d6a2..8fe59c7 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -3789,7 +3789,9 @@
X509 *x509, STACK_OF(X509) *chain);
// X509_STORE_CTX_set0_trusted_stack configures |ctx| to trust the certificates
-// in |sk|. |sk| must remain valid for the duration of |ctx|.
+// in |sk|. |sk| must remain valid for the duration of |ctx|. Calling this
+// function causes |ctx| to ignore any certificates configured in the
+// |X509_STORE|.
//
// WARNING: This function differs from most |set0| functions in that it does not
// take ownership of its input. The caller is required to ensure the lifetimes
