Extend X509 cert checks and error reporting in v3_{purp,crld}.c and x509_{set,vfy}.c

add various checks for malformedness to static check_chain_extensions() in x509_vfc.c
improve error reporting of X509v3_cache_extensions() in v3_purp.c
add error reporting to x509_init_sig_info() in x509_set.c
improve static setup_dp() and related functions in v3_purp.c and v3_crld.c
add test case for non-conforming cert from https://tools.ietf.org/html/rfc8410#section-10.2

Reviewed-by: Kurt Roeckx <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #12478)

Author: DDvO

crypto/err/openssl.txt

@@ -3487,6 +3487,7 @@ X509V3_R_BN_TO_ASN1_INTEGER_ERROR:101:bn to asn1 integer error
 X509V3_R_DIRNAME_ERROR:149:dirname error
 X509V3_R_DISTPOINT_ALREADY_SET:160:distpoint already set
 X509V3_R_DUPLICATE_ZONE_ID:133:duplicate zone id
+X509V3_R_EMPTY_KEY_USAGE:169:empty key usage
 X509V3_R_ERROR_CONVERTING_ZONE:131:error converting zone
 X509V3_R_ERROR_CREATING_EXTENSION:144:error creating extension
 X509V3_R_ERROR_IN_EXTENSION:128:error in extension
@@ -3501,6 +3502,7 @@ X509V3_R_INCORRECT_POLICY_SYNTAX_TAG:152:incorrect policy syntax tag
 X509V3_R_INVALID_ASNUMBER:162:invalid asnumber
 X509V3_R_INVALID_ASRANGE:163:invalid asrange
 X509V3_R_INVALID_BOOLEAN_STRING:104:invalid boolean string
+X509V3_R_INVALID_CERTIFICATE:158:invalid certificate
 X509V3_R_INVALID_EMPTY_NAME:108:invalid empty name
 X509V3_R_INVALID_EXTENSION_STRING:105:invalid extension string
 X509V3_R_INVALID_INHERITANCE:165:invalid inheritance
@@ -3522,6 +3524,7 @@ X509V3_R_INVALID_SYNTAX:143:invalid syntax
 X509V3_R_ISSUER_DECODE_ERROR:126:issuer decode error
 X509V3_R_MISSING_VALUE:124:missing value
 X509V3_R_NEED_ORGANIZATION_AND_NUMBERS:142:need organization and numbers
+X509V3_R_NEGATIVE_PATHLEN:168:negative pathlen
 X509V3_R_NO_CONFIG_DATABASE:136:no config database
 X509V3_R_NO_ISSUER_CERTIFICATE:121:no issuer certificate
 X509V3_R_NO_ISSUER_DETAILS:127:no issuer details
@@ -3557,9 +3560,12 @@ X509_R_CERTIFICATE_VERIFICATION_FAILED:139:certificate verification failed
 X509_R_CERT_ALREADY_IN_HASH_TABLE:101:cert already in hash table
 X509_R_CRL_ALREADY_DELTA:127:crl already delta
 X509_R_CRL_VERIFY_FAILURE:131:crl verify failure
+X509_R_ERROR_GETTING_MD_BY_NID:141:error getting md by nid
+X509_R_ERROR_USING_SIGINF_SET:142:error using siginf set
 X509_R_IDP_MISMATCH:128:idp mismatch
 X509_R_INVALID_ATTRIBUTES:138:invalid attributes
 X509_R_INVALID_DIRECTORY:113:invalid directory
+X509_R_INVALID_DISTPOINT:143:invalid distpoint
 X509_R_INVALID_FIELD_NAME:119:invalid field name
 X509_R_INVALID_TRUST:123:invalid trust
 X509_R_ISSUER_MISMATCH:129:issuer mismatch
@@ -3583,6 +3589,7 @@ X509_R_UNABLE_TO_GET_CERTS_PUBLIC_KEY:108:unable to get certs public key
 X509_R_UNKNOWN_KEY_TYPE:117:unknown key type
 X509_R_UNKNOWN_NID:109:unknown nid
 X509_R_UNKNOWN_PURPOSE_ID:121:unknown purpose id
+X509_R_UNKNOWN_SIGID_ALGS:144:unknown sigid algs
 X509_R_UNKNOWN_TRUST_ID:120:unknown trust id
 X509_R_UNSUPPORTED_ALGORITHM:111:unsupported algorithm
 X509_R_WRONG_LOOKUP_TYPE:112:wrong lookup type

crypto/x509/v3_crld.c

@@ -485,30 +485,31 @@ static int i2r_crldp(const X509V3_EXT_METHOD *method, void *pcrldp, BIO *out,
     return 1;
 }
 
+/* Append any nameRelativeToCRLIssuer in dpn to iname, set in dpn->dpname */
 int DIST_POINT_set_dpname(DIST_POINT_NAME *dpn, const X509_NAME *iname)
 {
     int i;
     STACK_OF(X509_NAME_ENTRY) *frag;
     X509_NAME_ENTRY *ne;
-    if (!dpn || (dpn->type != 1))
+
+    if (dpn == NULL || dpn->type != 1)
         return 1;
     frag = dpn->name.relativename;
+    X509_NAME_free(dpn->dpname); /* just in case it was already set */
     dpn->dpname = X509_NAME_dup(iname);
-    if (!dpn->dpname)
+    if (dpn->dpname == NULL)
         return 0;
     for (i = 0; i < sk_X509_NAME_ENTRY_num(frag); i++) {
         ne = sk_X509_NAME_ENTRY_value(frag, i);
-        if (!X509_NAME_add_entry(dpn->dpname, ne, -1, i ? 0 : 1)) {
-            X509_NAME_free(dpn->dpname);
-            dpn->dpname = NULL;
-            return 0;
-        }
+        if (!X509_NAME_add_entry(dpn->dpname, ne, -1, i ? 0 : 1))
+            goto err;
     }
     /* generate cached encoding of name */
-    if (i2d_X509_NAME(dpn->dpname, NULL) < 0) {
-        X509_NAME_free(dpn->dpname);
-        dpn->dpname = NULL;
-        return 0;
-    }
-    return 1;
+    if (i2d_X509_NAME(dpn->dpname, NULL) >= 0)
+        return 1;
+
+ err:
+    X509_NAME_free(dpn->dpname);
+    dpn->dpname = NULL;
+    return 0;
 }

crypto/x509/v3_purp.c

@@ -305,44 +305,62 @@ int X509_supported_extension(X509_EXTENSION *ex)
     return 0;
 }
 
-static int setup_dp(X509 *x, DIST_POINT *dp)
+/* return 1 on success, 0 if x is invalid, -1 on (internal) error */
+static int setup_dp(const X509 *x, DIST_POINT *dp)
 {
     const X509_NAME *iname = NULL;
     int i;
 
-    if (dp->reasons) {
+    if (dp->distpoint == NULL && sk_GENERAL_NAME_num(dp->CRLissuer) <= 0) {
+        X509err(0, X509_R_INVALID_DISTPOINT);
+        return 0;
+    }
+    if (dp->reasons != NULL) {
         if (dp->reasons->length > 0)
             dp->dp_reasons = dp->reasons->data[0];
         if (dp->reasons->length > 1)
             dp->dp_reasons |= (dp->reasons->data[1] << 8);
         dp->dp_reasons &= CRLDP_ALL_REASONS;
-    } else
+    } else {
         dp->dp_reasons = CRLDP_ALL_REASONS;
-    if (!dp->distpoint || (dp->distpoint->type != 1))
+    }
+    if (dp->distpoint == NULL || dp->distpoint->type != 1)
         return 1;
+
+    /* handle name fragment given by nameRelativeToCRLIssuer */
+    /*
+     * Note that the below way of determining iname is not really compliant
+     * with https://tools.ietf.org/html/rfc5280#section-4.2.1.13
+     * According to it, sk_GENERAL_NAME_num(dp->CRLissuer) MUST be <= 1
+     * and any CRLissuer could be of type different to GEN_DIRNAME.
+     */
     for (i = 0; i < sk_GENERAL_NAME_num(dp->CRLissuer); i++) {
         GENERAL_NAME *gen = sk_GENERAL_NAME_value(dp->CRLissuer, i);
+
         if (gen->type == GEN_DIRNAME) {
             iname = gen->d.directoryName;
             break;
         }
     }
-    if (!iname)
+    if (iname == NULL)
         iname = X509_get_issuer_name(x);
-
-    return DIST_POINT_set_dpname(dp->distpoint, iname);
+    return DIST_POINT_set_dpname(dp->distpoint, iname) ? 1 : -1;
 }
 
+/* return 1 on success, 0 if x is invalid, -1 on (internal) error */
 static int setup_crldp(X509 *x)
 {
     int i;
 
     x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, &i, NULL);
     if (x->crldp == NULL && i != -1)
         return 0;
+
     for (i = 0; i < sk_DIST_POINT_num(x->crldp); i++) {
-        if (!setup_dp(x, sk_DIST_POINT_value(x->crldp, i)))
-            return 0;
+        int res = setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
+
+        if (res < 1)
+            return res;
     }
     return 1;
 }
@@ -373,6 +391,7 @@ static int check_sig_alg_match(const EVP_PKEY *pkey, const X509 *subject)
 /*
  * Cache info on various X.509v3 extensions and further derived information,
  * e.g., if cert 'x' is self-issued, in x->ex_flags and other internal fields.
+ * X509_SIG_INFO_VALID is set in x->flags if x->siginf was filled successfully.
  * Set EXFLAG_INVALID and return 0 in case the certificate is invalid.
  */
 int x509v3_cache_extensions(X509 *x)
@@ -384,6 +403,7 @@ int x509v3_cache_extensions(X509 *x)
     EXTENDED_KEY_USAGE *extusage;
     X509_EXTENSION *ex;
     int i;
+    int res;
 
 #ifdef tsan_ld_acq
     /* fast lock-free check, see end of the function for details. */
@@ -398,30 +418,34 @@ int x509v3_cache_extensions(X509 *x)
     }
     ERR_set_mark();
 
+    /* Cache the SHA1 digest of the cert */
     if (!X509_digest(x, EVP_sha1(), x->sha1_hash, NULL))
-            x->ex_flags |= EXFLAG_INVALID;
+        /*
+         * Note that the cert is marked invalid also on internal malloc failure
+         * or on failure of EVP_MD_fetch(), potentially called by X509_digest().
+         */
+        x->ex_flags |= EXFLAG_INVALID;
 
     /* V1 should mean no extensions ... */
     if (X509_get_version(x) == 0)
         x->ex_flags |= EXFLAG_V1;
 
     /* Handle basic constraints */
+    x->ex_pathlen = -1;
     if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, &i, NULL)) != NULL) {
         if (bs->ca)
             x->ex_flags |= EXFLAG_CA;
         if (bs->pathlen != NULL) {
+            /*
+             * the error case !bs->ca is checked by check_chain_extensions()
+             * in case ctx->param->flags & X509_V_FLAG_X509_STRICT
+             */
             if (bs->pathlen->type == V_ASN1_NEG_INTEGER) {
+                X509err(0, X509V3_R_NEGATIVE_PATHLEN);
                 x->ex_flags |= EXFLAG_INVALID;
-                x->ex_pathlen = 0;
             } else {
                 x->ex_pathlen = ASN1_INTEGER_get(bs->pathlen);
-                if (!bs->ca && x->ex_pathlen != 0) {
-                    x->ex_flags |= EXFLAG_INVALID;
-                    x->ex_pathlen = 0;
-                }
             }
-        } else {
-            x->ex_pathlen = -1;
         }
         BASIC_CONSTRAINTS_free(bs);
         x->ex_flags |= EXFLAG_BCONS;
@@ -436,17 +460,17 @@ int x509v3_cache_extensions(X509 *x)
             || X509_get_ext_by_NID(x, NID_issuer_alt_name, -1) >= 0) {
             x->ex_flags |= EXFLAG_INVALID;
         }
-        if (pci->pcPathLengthConstraint) {
+        if (pci->pcPathLengthConstraint != NULL)
             x->ex_pcpathlen = ASN1_INTEGER_get(pci->pcPathLengthConstraint);
-        } else
+        else
             x->ex_pcpathlen = -1;
         PROXY_CERT_INFO_EXTENSION_free(pci);
         x->ex_flags |= EXFLAG_PROXY;
     } else if (i != -1) {
         x->ex_flags |= EXFLAG_INVALID;
     }
 
-    /* Handle (basic and extended) key usage */
+    /* Handle basic key usage */
     if ((usage = X509_get_ext_d2i(x, NID_key_usage, &i, NULL)) != NULL) {
         x->ex_kusage = 0;
         if (usage->length > 0) {
@@ -456,9 +480,16 @@ int x509v3_cache_extensions(X509 *x)
         }
         x->ex_flags |= EXFLAG_KUSAGE;
         ASN1_BIT_STRING_free(usage);
+        /* Check for empty key usage according to RFC 5280 section 4.2.1.3 */
+        if (x->ex_kusage == 0) {
+            X509err(0, X509V3_R_EMPTY_KEY_USAGE);
+            x->ex_flags |= EXFLAG_INVALID;
+        }
     } else if (i != -1) {
         x->ex_flags |= EXFLAG_INVALID;
     }
+
+    /* Handle extended key usage */
     x->ex_xkusage = 0;
     if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, &i, NULL)) != NULL) {
         x->ex_flags |= EXFLAG_XKUSAGE;
@@ -493,6 +524,7 @@ int x509v3_cache_extensions(X509 *x)
                 x->ex_xkusage |= XKU_ANYEKU;
                 break;
             default:
+                /* ignore unknown extended key usage */
                 break;
             }
         }
@@ -517,6 +549,7 @@ int x509v3_cache_extensions(X509 *x)
     x->skid = X509_get_ext_d2i(x, NID_subject_key_identifier, &i, NULL);
     if (x->skid == NULL && i != -1)
         x->ex_flags |= EXFLAG_INVALID;
+
     x->akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &i, NULL);
     if (x->akid == NULL && i != -1)
         x->ex_flags |= EXFLAG_INVALID;
@@ -538,8 +571,13 @@ int x509v3_cache_extensions(X509 *x)
     x->nc = X509_get_ext_d2i(x, NID_name_constraints, &i, NULL);
     if (x->nc == NULL && i != -1)
         x->ex_flags |= EXFLAG_INVALID;
-    if (!setup_crldp(x))
+
+    /* Handle CRL distribution point entries */
+    res = setup_crldp(x);
+    if (res == 0)
         x->ex_flags |= EXFLAG_INVALID;
+    else if (res < 0)
+        goto err;
 
 #ifndef OPENSSL_NO_RFC3779
     x->rfc3779_addr = X509_get_ext_d2i(x, NID_sbgp_ipAddrBlock, &i, NULL);
@@ -551,8 +589,7 @@ int x509v3_cache_extensions(X509 *x)
 #endif
     for (i = 0; i < X509_get_ext_count(x); i++) {
         ex = X509_get_ext(x, i);
-        if (OBJ_obj2nid(X509_EXTENSION_get_object(ex))
-            == NID_freshest_crl)
+        if (OBJ_obj2nid(X509_EXTENSION_get_object(ex)) == NID_freshest_crl)
             x->ex_flags |= EXFLAG_FRESHEST;
         if (!X509_EXTENSION_get_critical(ex))
             continue;
@@ -562,7 +599,8 @@ int x509v3_cache_extensions(X509 *x)
         }
     }
 
-    x509_init_sig_info(x);
+    /* Set x->siginf, ignoring errors due to unsupported algos */
+    (void)x509_init_sig_info(x);
 
     x->ex_flags |= EXFLAG_SET; /* indicate that cert has been processed */
 #ifdef tsan_st_rel
@@ -574,9 +612,16 @@ int x509v3_cache_extensions(X509 *x)
      */
 #endif
     ERR_pop_to_mark();
-    CRYPTO_THREAD_unlock(x->lock);
+    if ((x->ex_flags & EXFLAG_INVALID) == 0) {
+        CRYPTO_THREAD_unlock(x->lock);
+        return 1;
+    }
+    X509err(0, X509V3_R_INVALID_CERTIFICATE);
 
-    return (x->ex_flags & EXFLAG_INVALID) == 0;
+ err:
+    x->ex_flags |= EXFLAG_SET; /* indicate that cert has been processed */
+    CRYPTO_THREAD_unlock(x->lock);
+    return 0;
 }
 
 /*-

crypto/x509/v3err.c

@@ -24,6 +24,7 @@ static const ERR_STRING_DATA X509V3_str_reasons[] = {
     "distpoint already set"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_DUPLICATE_ZONE_ID),
     "duplicate zone id"},
+    {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_EMPTY_KEY_USAGE), "empty key usage"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_ERROR_CONVERTING_ZONE),
     "error converting zone"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_ERROR_CREATING_EXTENSION),
@@ -51,6 +52,8 @@ static const ERR_STRING_DATA X509V3_str_reasons[] = {
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_INVALID_ASRANGE), "invalid asrange"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_INVALID_BOOLEAN_STRING),
     "invalid boolean string"},
+    {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_INVALID_CERTIFICATE),
+    "invalid certificate"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_INVALID_EXTENSION_STRING),
     "invalid extension string"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_INVALID_INHERITANCE),
@@ -84,6 +87,8 @@ static const ERR_STRING_DATA X509V3_str_reasons[] = {
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_MISSING_VALUE), "missing value"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_NEED_ORGANIZATION_AND_NUMBERS),
     "need organization and numbers"},
+    {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_NEGATIVE_PATHLEN),
+    "negative pathlen"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_NO_CONFIG_DATABASE),
     "no config database"},
     {ERR_PACK(ERR_LIB_X509V3, 0, X509V3_R_NO_ISSUER_CERTIFICATE),

crypto/x509/x509_err.c

@@ -27,10 +27,15 @@ static const ERR_STRING_DATA X509_str_reasons[] = {
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_CRL_ALREADY_DELTA), "crl already delta"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_CRL_VERIFY_FAILURE),
     "crl verify failure"},
+    {ERR_PACK(ERR_LIB_X509, 0, X509_R_ERROR_GETTING_MD_BY_NID),
+    "error getting md by nid"},
+    {ERR_PACK(ERR_LIB_X509, 0, X509_R_ERROR_USING_SIGINF_SET),
+    "error using siginf set"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_IDP_MISMATCH), "idp mismatch"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_ATTRIBUTES),
     "invalid attributes"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_DIRECTORY), "invalid directory"},
+    {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_DISTPOINT), "invalid distpoint"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_FIELD_NAME),
     "invalid field name"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_TRUST), "invalid trust"},
@@ -66,6 +71,8 @@ static const ERR_STRING_DATA X509_str_reasons[] = {
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_UNKNOWN_NID), "unknown nid"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_UNKNOWN_PURPOSE_ID),
     "unknown purpose id"},
+    {ERR_PACK(ERR_LIB_X509, 0, X509_R_UNKNOWN_SIGID_ALGS),
+    "unknown sigid algs"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_UNKNOWN_TRUST_ID), "unknown trust id"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_UNSUPPORTED_ALGORITHM),
     "unsupported algorithm"},