Make callback more useful, flag adjustment and naming

Author: samuel40791765

crypto/x509/internal.h

@@ -344,8 +344,8 @@ struct x509_store_ctx_st {
   X509_STORE_CTX_verify_cb verify_cb;       // error callback
   X509_STORE_CTX_get_crl_fn get_crl;        // retrieve CRL
   X509_STORE_CTX_check_crl_fn check_crl;    // Check CRL validity
-  X509_STORE_CTX_verify_crit_oids
-      verify_crit_oids;  // Check custom critical oids
+  X509_STORE_CTX_verify_crit_oids_cb
+      verify_custom_crit_oids;  // Check custom critical oids
 
   // The following is built up
 
@@ -361,7 +361,7 @@ struct x509_store_ctx_st {
 
   int current_crl_score;         // score of current CRL
 
-  // Array of allowed custom critical extension oids.
+  // Stack of allowed custom critical extension oids.
   STACK_OF(ASN1_OBJECT) *custom_crit_oids;
 
   CRYPTO_EX_DATA ex_data;

crypto/x509/x509_test.cc

@@ -8314,6 +8314,9 @@ TEST(X509Test, X509CustomExtensions) {
   bssl::UniquePtr<X509> ca(CertFromPEM(kX509CustomExtensionsCA));
   ASSERT_TRUE(ca);
 
+  // Check that the cert has been marked as |EXFLAG_CRITICAL|.
+  EXPECT_TRUE(X509_get_extension_flags(cert.get()) & EXFLAG_CRITICAL);
+
   bssl::UniquePtr<ASN1_OBJECT> custom_oid(OBJ_txt2obj("1.3.187.25204.5", 1));
   ASSERT_TRUE(custom_oid);
 
@@ -8344,14 +8347,16 @@ TEST(X509Test, X509CustomExtensions) {
             Verify(cert.get(), {ca.get()}, {}, {},
                    /*flags=*/0, set_no_custom_ext_with_callback));
 
-  // This correctly sets up |ctx| with a customcritical extension and the
+  // This correctly sets up |ctx| with a custom critical extension and the
   // |verify_crit_oids| callback.
   auto set_custom_ext_with_callback = [&](X509_STORE_CTX *ctx) {
     SetupVerificationContext(ctx, {custom_oid.get()}, true);
   };
   EXPECT_EQ(X509_V_OK,
             Verify(cert.get(), {ca.get()}, {}, {}, /*flags=*/0,
                    set_custom_ext_with_callback));
+  // Check that |EXFLAG_CRITICAL| has been removed after validation.
+  EXPECT_FALSE(X509_get_extension_flags(cert.get()) & EXFLAG_CRITICAL);
 }
 
 TEST(X509Test, X509MultipleCustomExtensions) {
@@ -8360,6 +8365,9 @@ TEST(X509Test, X509MultipleCustomExtensions) {
   bssl::UniquePtr<X509> ca(CertFromPEM(kX509MultipleCustomExtensionsCA));
   ASSERT_TRUE(ca);
 
+  // Check that the cert has been marked as |EXFLAG_CRITICAL|.
+  EXPECT_TRUE(X509_get_extension_flags(cert.get()) & EXFLAG_CRITICAL);
+
   bssl::UniquePtr<ASN1_OBJECT> custom_oid(OBJ_txt2obj("1.3.187.25204.5", 1));
   ASSERT_TRUE(custom_oid);
   bssl::UniquePtr<ASN1_OBJECT> custom_oid2(OBJ_txt2obj("1.3.187.25204.6", 1));
@@ -8397,4 +8405,6 @@ TEST(X509Test, X509MultipleCustomExtensions) {
   };
   EXPECT_EQ(X509_V_OK, Verify(cert.get(), {ca.get()}, {}, {},
                               /*flags=*/0, set_custom_exts_with_callback));
+  // Check that |EXFLAG_CRITICAL| has been removed after validation.
+  EXPECT_FALSE(X509_get_extension_flags(cert.get()) & EXFLAG_CRITICAL);
 }

crypto/x509/x509_vfy.c

@@ -122,8 +122,11 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x);
 static int internal_verify(X509_STORE_CTX *ctx);
 
 static int null_callback(int ok, X509_STORE_CTX *e) { return ok; }
-static int null_verify_crit_oids_callback(X509_STORE_CTX *ctx, X509 *x509,
-                                          STACK_OF(ASN1_OBJECT) *oids) {
+static int null_verify_custom_crit_oids_callback(X509_STORE_CTX *ctx,
+                                                 X509 *x509,
+                                                 STACK_OF(ASN1_OBJECT) *oids) {
+  // This returns 0 by default, so that the callback must be configured by the
+  // user when enabling the custom critical extensions feature.
   return 0;
 }
 
@@ -565,7 +568,7 @@ static int get_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) {
   return X509_STORE_CTX_get1_issuer(issuer, ctx, x);
 }
 
-static int check_custom_known_critical_extension(X509_STORE_CTX *ctx, X509 *x) {
+static int check_custom_critical_extensions(X509_STORE_CTX *ctx, X509 *x) {
   if (ctx->custom_crit_oids == NULL) {
     // Fail if custom critical extensions are enabled, but none were set.
     return 0;
@@ -575,6 +578,12 @@ static int check_custom_known_critical_extension(X509_STORE_CTX *ctx, X509 *x) {
     return 0;
   }
 
+  // Allocate |found_exts| to pass to the callback.
+  STACK_OF(ASN1_OBJECT) *found_exts = sk_ASN1_OBJECT_new_null();
+  if (found_exts == NULL) {
+    return 0;
+  }
+
   // Iterate through all critical extensions of |x| and validate against the
   // ones that aren't recognized by |X509_supported_extension|.
   int last_pos = X509_get_ext_by_critical(x, 1, -1);
@@ -585,10 +594,12 @@ static int check_custom_known_critical_extension(X509_STORE_CTX *ctx, X509 *x) {
 
       // Iterate through all set |custom_crit_oids|.
       for (size_t i = 0; i < known_oid_count; i++) {
-        ASN1_OBJECT *known_ext =
-            sk_ASN1_OBJECT_value(ctx->custom_crit_oids, i);
+        ASN1_OBJECT *known_ext = sk_ASN1_OBJECT_value(ctx->custom_crit_oids, i);
         if (OBJ_cmp(ext->object, known_ext) == 0) {
           found = 1;
+          if (!sk_ASN1_OBJECT_push(found_exts, known_ext)) {
+            return 0;
+          }
           break;
         }
       }
@@ -600,7 +611,17 @@ static int check_custom_known_critical_extension(X509_STORE_CTX *ctx, X509 *x) {
     }
     last_pos = X509_get_ext_by_critical(x, 1, last_pos);
   }
-  // If we get here, all unknown critical extensions were found
+
+  // If we get here, all unknown critical extensions in |x| were
+  // properly handled and we pass the ones that were found to the caller.
+  if (!ctx->verify_custom_crit_oids(ctx, x, found_exts)) {
+    return 0;
+  }
+
+  // Remove the |EXFLAG_CRITICAL| flag from |x|, now that all unknown
+  // critical extensions have been handled.
+  x->ex_flags &= ~EXFLAG_CRITICAL;
+
   return 1;
 }
 
@@ -614,11 +635,14 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) {
   // Check all untrusted certificates
   for (int i = 0; i < ctx->last_untrusted; i++) {
     X509 *x = sk_X509_value(ctx->chain, i);
-    if ((!(ctx->param->flags & X509_V_FLAG_IGNORE_CRITICAL) &&
+    if (  // OpenSSL's historic check for unknown critical extensions.
+          // |EXFLAG_CRITICAL| indicates an unsupported critical extension was
+          // found in |x| during the initial parsing of the certificate.
+        (!(ctx->param->flags & X509_V_FLAG_IGNORE_CRITICAL) &&
          (x->ex_flags & EXFLAG_CRITICAL)) &&
-        // Do check for enabling custom unknown critical extensions.
-        (!check_custom_known_critical_extension(ctx, x) ||
-         !ctx->verify_crit_oids(ctx, x, ctx->custom_crit_oids))) {
+        // AWS-LC specific logic for enabling custom unknown critical
+        // extensions.
+        !check_custom_critical_extensions(ctx, x)) {
       ctx->error = X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION;
       ctx->error_depth = i;
       ctx->current_cert = x;
@@ -1728,7 +1752,7 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
     ctx->check_crl = check_crl;
   }
 
-  ctx->verify_crit_oids = null_verify_crit_oids_callback;
+  ctx->verify_custom_crit_oids = null_verify_custom_crit_oids_callback;
 
   return 1;
 
@@ -1830,6 +1854,7 @@ int X509_STORE_CTX_add_custom_crit_oid(X509_STORE_CTX *ctx, ASN1_OBJECT *oid) {
 }
 
 void X509_STORE_CTX_set_verify_crit_oids(
-    X509_STORE_CTX *ctx, X509_STORE_CTX_verify_crit_oids verify_crit_oids) {
-  ctx->verify_crit_oids = verify_crit_oids;
+    X509_STORE_CTX *ctx,
+    X509_STORE_CTX_verify_crit_oids_cb verify_custom_crit_oids) {
+  ctx->verify_custom_crit_oids = verify_custom_crit_oids;
 }

include/openssl/x509.h

@@ -2903,12 +2903,12 @@ OPENSSL_EXPORT int X509_STORE_CTX_set_purpose(X509_STORE_CTX *ctx, int purpose);
 // difference.
 OPENSSL_EXPORT int X509_STORE_CTX_set_trust(X509_STORE_CTX *ctx, int trust);
 
-// X509_STORE_CTX_add_custom_crit_oid adds |oid| to the list of "known"
-// critical extension OIDs in |ctx|. Typical OpenSSL/AWS-LC behavior returns
-// an error if there are any unknown critical extensions present within the
-// certificates being validated. This function lets users to specify custom
-// OIDs of any critical extensions that are within the certificates being
-// validated, that they wish to allow.
+// X509_STORE_CTX_add_custom_crit_oid adds |oid| to the list of "known" critical
+// extension OIDs in |ctx|. Typical OpenSSL/AWS-LC behavior returns an error if
+// there are any unknown critical extensions present within the certificates
+// being validated. This function lets users specify custom OIDs of any critical
+// extensions that are within the certificates being validated, that they wish
+// to allow.
 //
 // To properly consume this feature, the callback mechanism with
 // |X509_STORE_CTX_set_verify_crit_oids| must be set. See its specific
@@ -2919,18 +2919,21 @@ OPENSSL_EXPORT int X509_STORE_CTX_add_custom_crit_oid(X509_STORE_CTX *ctx,
 // X509_STORE_CTX_verify_crit_oids is the callback signature for
 // |X509_STORE_CTX_set_verify_crit_oids|. |ctx| is the context being used,
 // |x509| represents the current certificate being validated, and |oids|
-// represents the stack of custom OIDs that have been set by
+// is a stack of |ASN1_OBJECT|s representing unknown critical extension
+// OIDs that were found in |x509| and match those previously registered via
 // |X509_STORE_CTX_add_custom_crit_oid|.
-typedef int (*X509_STORE_CTX_verify_crit_oids)(X509_STORE_CTX *ctx, X509 *x509,
-                                               STACK_OF(ASN1_OBJECT) *oids);
+typedef int (*X509_STORE_CTX_verify_crit_oids_cb)(X509_STORE_CTX *ctx,
+                                                  X509 *x509,
+                                                  STACK_OF(ASN1_OBJECT) *oids);
 
 // X509_STORE_CTX_set_verify_crit_oids sets the |verify_crit_oids| callback
 // function for |ctx|. Consumers should be performing additional validation
 // against the custom extension oids after or during the handshake with
 // |X509_STORE_CTX_set_verify_crit_oids|. This callback forces users to validate
 // their custom OIDs when processing unknown custom critical extensions.
 OPENSSL_EXPORT void X509_STORE_CTX_set_verify_crit_oids(
-    X509_STORE_CTX *ctx, X509_STORE_CTX_verify_crit_oids verify_crit_oids);
+    X509_STORE_CTX *ctx,
+    X509_STORE_CTX_verify_crit_oids_cb verify_custom_crit_oids);
 
 
 // Verification parameters