Check chain extensions also for trusted certificates

This includes basic constraints, key usages, issuer EKUs and auxiliary
trust OIDs (given a trust suitably related to the intended purpose).

Added tests and updated documentation.

Reviewed-by: Dr. Stephen Henson <steve@openssl.org>

3 files changed, 99 insertions, 44 deletions

diff --git a/crypto/x509/x509_trs.c b/crypto/x509/x509_trs.c
index 7392c55953..c81c725ea1 100644
--- a/crypto/x509/x509_trs.c
+++ b/crypto/x509/x509_trs.c
@@ -276,7 +276,7 @@ static int trust_1oidany(X509_TRUST *trust, X509 *x, int flags)
 
 static int trust_1oid(X509_TRUST *trust, X509 *x, int flags)
 {
-    if (x->aux)
+    if (x->aux && (x->aux->trust || x->aux->reject))
         return obj_trust(trust->arg1, x, flags);
     return X509_TRUST_UNTRUSTED;
 }
@@ -293,23 +293,26 @@ static int trust_compat(X509_TRUST *trust, X509 *x, int flags)
 
 static int obj_trust(int id, X509 *x, int flags)
 {
-    ASN1_OBJECT *obj;
+    X509_CERT_AUX *ax = x->aux;
     int i;
-    X509_CERT_AUX *ax;
-    ax = x->aux;
+
     if (!ax)
         return X509_TRUST_UNTRUSTED;
     if (ax->reject) {
         for (i = 0; i < sk_ASN1_OBJECT_num(ax->reject); i++) {
-            obj = sk_ASN1_OBJECT_value(ax->reject, i);
-            if (OBJ_obj2nid(obj) == id)
+            ASN1_OBJECT *obj = sk_ASN1_OBJECT_value(ax->reject, i);
+            int nid = OBJ_obj2nid(obj);
+
+            if (nid == id || nid == NID_anyExtendedKeyUsage)
                 return X509_TRUST_REJECTED;
         }
     }
     if (ax->trust) {
         for (i = 0; i < sk_ASN1_OBJECT_num(ax->trust); i++) {
-            obj = sk_ASN1_OBJECT_value(ax->trust, i);
-            if (OBJ_obj2nid(obj) == id)
+            ASN1_OBJECT *obj = sk_ASN1_OBJECT_value(ax->trust, i);
+            int nid = OBJ_obj2nid(obj);
+
+            if (nid == id || nid == NID_anyExtendedKeyUsage)
                 return X509_TRUST_TRUSTED;
         }
         /*
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 39d37b99a7..14d6a8d74e 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -363,17 +363,60 @@ static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx, X509_NAME *nm)
 }
 
 /*
+ * Check EE or CA certificate purpose.  For trusted certificates explicit local
+ * auxiliary trust can be used to override EKU-restrictions.
+ */
+static int check_purpose(X509_STORE_CTX *ctx, X509 *x, int purpose, int depth,
+                         int must_be_ca)
+{
+    int pu_ok = X509_check_purpose(x, purpose, must_be_ca > 0);
+    int tr_ok = X509_TRUST_UNTRUSTED;
+
+    /*
+     * For trusted certificates we want to see whether any auxiliary trust
+     * settings override the purpose constraints we failed to meet above.
+     *
+     * This is complicated by the fact that the trust ordinals in
+     * ctx->param->trust are entirely independent of the purpose ordinals in
+     * ctx->param->purpose!
+     *
+     * What connects them is their mutual initialization via calls from
+     * X509_STORE_CTX_set_default() into X509_VERIFY_PARAM_lookup() which sets
+     * related values of both param->trust and param->purpose.  It is however
+     * typically possible to infer associated trust values from a purpose value
+     * via the X509_PURPOSE API.
+     *
+     * Therefore, we can only check for trust overrides when the purpose we're
+     * checking is the same as ctx->param->purpose and ctx->param->trust is
+     * also set, or can be inferred from the purpose.
+     */
+    if (depth >= ctx->num_untrusted && purpose == ctx->param->purpose)
+        tr_ok = X509_check_trust(x, ctx->param->trust, X509_TRUST_NO_SS_COMPAT);
+
+    if (tr_ok != X509_TRUST_REJECTED &&
+        (pu_ok == 1 ||
+         (pu_ok != 0 && (ctx->param->flags & X509_V_FLAG_X509_STRICT) == 0)))
+        return 1;
+
+    ctx->error = X509_V_ERR_INVALID_PURPOSE;
+    ctx->error_depth = depth;
+    ctx->current_cert = x;
+    return ctx->verify_cb(0, ctx);
+}
+
+/*
  * Check a certificate chains extensions for consistency with the supplied
  * purpose
  */
 
 static int check_chain_extensions(X509_STORE_CTX *ctx)
 {
-    int i, ok = 0, must_be_ca, plen = 0;
+    int i, must_be_ca, plen = 0;
     X509 *x;
     int proxy_path_length = 0;
     int purpose;
     int allow_proxy_certs;
+    int num = sk_X509_num(ctx->chain);
 
     /*-
      *  must_be_ca can have 1 of 3 values:
@@ -402,8 +445,7 @@ static int check_chain_extensions(X509_STORE_CTX *ctx)
         purpose = ctx->param->purpose;
     }
 
-    /* Check all untrusted certificates */
-    for (i = 0; i == 0 || i < ctx->num_untrusted; i++) {
+    for (i = 0; i < num; i++) {
         int ret;
         x = sk_X509_value(ctx->chain, i);
         if (!(ctx->param->flags & X509_V_FLAG_IGNORE_CRITICAL)
@@ -411,17 +453,15 @@ static int check_chain_extensions(X509_STORE_CTX *ctx)
             ctx->error = X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION;
             ctx->error_depth = i;
             ctx->current_cert = x;
-            ok = ctx->verify_cb(0, ctx);
-            if (!ok)
-                goto end;
+            if (!ctx->verify_cb(0, ctx))
+                return 0;
         }
         if (!allow_proxy_certs && (x->ex_flags & EXFLAG_PROXY)) {
             ctx->error = X509_V_ERR_PROXY_CERTIFICATES_NOT_ALLOWED;
             ctx->error_depth = i;
             ctx->current_cert = x;
-            ok = ctx->verify_cb(0, ctx);
-            if (!ok)
-                goto end;
+            if (!ctx->verify_cb(0, ctx))
+                return 0;
         }
         ret = X509_check_ca(x);
         switch (must_be_ca) {
@@ -453,22 +493,12 @@ static int check_chain_extensions(X509_STORE_CTX *ctx)
         if (ret == 0) {
             ctx->error_depth = i;
             ctx->current_cert = x;
-            ok = ctx->verify_cb(0, ctx);
-            if (!ok)
-                goto end;
+            if (! ctx->verify_cb(0, ctx))
+                return 0;
         }
-        if (ctx->param->purpose > 0) {
-            ret = X509_check_purpose(x, purpose, must_be_ca > 0);
-            if ((ret == 0)
-                || ((ctx->param->flags & X509_V_FLAG_X509_STRICT)
-                    && (ret != 1))) {
-                ctx->error = X509_V_ERR_INVALID_PURPOSE;
-                ctx->error_depth = i;
-                ctx->current_cert = x;
-                ok = ctx->verify_cb(0, ctx);
-                if (!ok)
-                    goto end;
-            }
+        if (purpose > 0) {
+            if (!check_purpose(ctx, x, purpose, i, must_be_ca))
+                return 0;
         }
         /* Check pathlen if not self issued */
         if ((i > 1) && !(x->ex_flags & EXFLAG_SI)
@@ -477,9 +507,8 @@ static int check_chain_extensions(X509_STORE_CTX *ctx)
             ctx->error = X509_V_ERR_PATH_LENGTH_EXCEEDED;
             ctx->error_depth = i;
             ctx->current_cert = x;
-            ok = ctx->verify_cb(0, ctx);
-            if (!ok)
-                goto end;
+            if (!ctx->verify_cb(0, ctx))
+                return 0;
         }
         /* Increment path length if not self issued */
         if (!(x->ex_flags & EXFLAG_SI))
@@ -494,18 +523,15 @@ static int check_chain_extensions(X509_STORE_CTX *ctx)
                 ctx->error = X509_V_ERR_PROXY_PATH_LENGTH_EXCEEDED;
                 ctx->error_depth = i;
                 ctx->current_cert = x;
-                ok = ctx->verify_cb(0, ctx);
-                if (!ok)
-                    goto end;
+                if (!ctx->verify_cb(0, ctx))
+                    return 0;
             }
             proxy_path_length++;
             must_be_ca = 0;
         } else
             must_be_ca = 1;
     }
-    ok = 1;
- end:
-    return ok;
+    return 1;
 }
 
 static int check_name_constraints(X509_STORE_CTX *ctx)
@@ -2016,11 +2042,20 @@ void X509_STORE_CTX_set0_crls(X509_STORE_CTX *ctx, STACK_OF(X509_CRL) *sk)
 
 int X509_STORE_CTX_set_purpose(X509_STORE_CTX *ctx, int purpose)
 {
+    /*
+     * XXX: Why isn't this function always used to set the associated trust?
+     * Should there even be a VPM->trust field at all?  Or should the trust
+     * always be inferred from the purpose by X509_STORE_CTX_init().
+     */
     return X509_STORE_CTX_purpose_inherit(ctx, 0, purpose, 0);
 }
 
 int X509_STORE_CTX_set_trust(X509_STORE_CTX *ctx, int trust)
 {
+    /*
+     * XXX: See above, this function would only be needed when the default
+     * trust for the purpose needs an override in a corner case.
+     */
     return X509_STORE_CTX_purpose_inherit(ctx, 0, 0, trust);
 }
 
@@ -2054,6 +2089,11 @@ int X509_STORE_CTX_purpose_inherit(X509_STORE_CTX *ctx, int def_purpose,
         ptmp = X509_PURPOSE_get0(idx);
         if (ptmp->trust == X509_TRUST_DEFAULT) {
             idx = X509_PURPOSE_get_by_id(def_purpose);
+            /*
+             * XXX: In the two callers above def_purpose is always 0, which is
+             * not a known value, so idx will always be -1.  How is the
+             * X509_TRUST_DEFAULT case actually supposed to be handled?
+             */
             if (idx == -1) {
                 X509err(X509_F_X509_STORE_CTX_PURPOSE_INHERIT,
                         X509_R_UNKNOWN_PURPOSE_ID);
@@ -2211,6 +2251,18 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
         goto err;
     }
 
+    /*
+     * XXX: For now, continue to inherit trust from VPM, but infer from the
+     * purpose if this still yields the default value.
+     */
+    if (ctx->param->trust == X509_TRUST_DEFAULT) {
+        int idx = X509_PURPOSE_get_by_id(ctx->param->purpose);
+        X509_PURPOSE *xp = X509_PURPOSE_get0(idx);
+
+        if (xp != NULL)
+            ctx->param->trust = X509_PURPOSE_get_trust(xp);
+    }
+
     if (CRYPTO_new_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx,
                            &ctx->ex_data))
         return 1;
diff --git a/crypto/x509/x509_vpm.c b/crypto/x509/x509_vpm.c
index 295ce885a1..41b0fde4a5 100644
--- a/crypto/x509/x509_vpm.c
+++ b/crypto/x509/x509_vpm.c
@@ -133,7 +133,7 @@ static void x509_verify_param_zero(X509_VERIFY_PARAM *param)
         return;
     param->name = NULL;
     param->purpose = 0;
-    param->trust = 0;
+    param->trust = X509_TRUST_DEFAULT;
     /*
      * param->inh_flags = X509_VP_FLAG_DEFAULT;
      */
@@ -243,7 +243,7 @@ int X509_VERIFY_PARAM_inherit(X509_VERIFY_PARAM *dest,
         to_overwrite = 0;
 
     x509_verify_param_copy(purpose, 0);
-    x509_verify_param_copy(trust, 0);
+    x509_verify_param_copy(trust, X509_TRUST_DEFAULT);
     x509_verify_param_copy(depth, -1);
 
     /* If overwrite or check time not set, copy across */
@@ -511,7 +511,7 @@ static const X509_VERIFY_PARAM default_table[] = {
      "default",                 /* X509 default parameters */
      0,                         /* Check time */
      0,                         /* internal flags */
-     0,                         /* flags */
+     X509_V_FLAG_TRUSTED_FIRST, /* flags */
      0,                         /* purpose */
      0,                         /* trust */
      100,                       /* depth */