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