X509_cmp(): Fix comparison in case x509v3_cache_extensions() failed to due to invalid cert

This is the upstream fix for #13698 reported for v1.1.1

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/13755)

crypto/x509/v3_purp.c

@@ -387,6 +387,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.
+ * x->sha1_hash is filled in, or else EXFLAG_NO_FINGERPRINT is set in x->flags.
  * 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.
  */
@@ -411,15 +412,12 @@ int x509v3_cache_extensions(X509 *x)
         CRYPTO_THREAD_unlock(x->lock);
         return (x->ex_flags & EXFLAG_INVALID) == 0;
     }
-    ERR_set_mark();
 
     /* Cache the SHA1 digest of the cert */
     if (!X509_digest(x, EVP_sha1(), x->sha1_hash, NULL))
-        /*
-         * 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;
+        x->ex_flags |= EXFLAG_NO_FINGERPRINT;
+
+    ERR_set_mark();
 
     /* V1 should mean no extensions ... */
     if (X509_get_version(x) == 0)
@@ -625,11 +623,13 @@ int x509v3_cache_extensions(X509 *x)
      */
 #endif
     ERR_pop_to_mark();
-    if ((x->ex_flags & EXFLAG_INVALID) == 0) {
+    if ((x->ex_flags & (EXFLAG_INVALID | EXFLAG_NO_FINGERPRINT)) == 0) {
         CRYPTO_THREAD_unlock(x->lock);
         return 1;
     }
-    ERR_raise(ERR_LIB_X509, X509V3_R_INVALID_CERTIFICATE);
+    if ((x->ex_flags & EXFLAG_INVALID) != 0)
+        ERR_raise(ERR_LIB_X509, X509V3_R_INVALID_CERTIFICATE);
+    /* If computing sha1_hash failed the error queue already reflects this. */
 
  err:
     x->ex_flags |= EXFLAG_SET; /* indicate that cert has been processed */

crypto/x509/x509_cmp.c

@@ -81,7 +81,13 @@ int X509_CRL_cmp(const X509_CRL *a, const X509_CRL *b)
 
 int X509_CRL_match(const X509_CRL *a, const X509_CRL *b)
 {
-    int rv = memcmp(a->sha1_hash, b->sha1_hash, 20);
+    int rv;
+
+    if ((a->flags & EXFLAG_NO_FINGERPRINT) == 0
+            && (b->flags & EXFLAG_NO_FINGERPRINT) == 0)
+        rv = memcmp(a->sha1_hash, b->sha1_hash, SHA_DIGEST_LENGTH);
+    else
+        return -2;
 
     return rv < 0 ? -1 : rv > 0;
 }
@@ -140,19 +146,21 @@ unsigned long X509_subject_name_hash_old(X509 *x)
  */
 int X509_cmp(const X509 *a, const X509 *b)
 {
-    int rv;
+    int rv = 0;
 
     if (a == b) /* for efficiency */
         return 0;
-    /* ensure hash is valid */
-    if (X509_check_purpose((X509 *)a, -1, 0) != 1)
-        return -2;
-    if (X509_check_purpose((X509 *)b, -1, 0) != 1)
-        return -2;
 
-    rv = memcmp(a->sha1_hash, b->sha1_hash, SHA_DIGEST_LENGTH);
+    /* attempt to compute cert hash */
+    (void)X509_check_purpose((X509 *)a, -1, 0);
+    (void)X509_check_purpose((X509 *)b, -1, 0);
+
+    if ((a->ex_flags & EXFLAG_NO_FINGERPRINT) == 0
+            && (b->ex_flags & EXFLAG_NO_FINGERPRINT) == 0)
+        rv = memcmp(a->sha1_hash, b->sha1_hash, SHA_DIGEST_LENGTH);
     if (rv != 0)
         return rv < 0 ? -1 : 1;
+
     /* Check for match against stored encoding too */
     if (!a->cert_info.enc.modified && !b->cert_info.enc.modified) {
         if (a->cert_info.enc.len < b->cert_info.enc.len)

crypto/x509/x509_lu.c

@@ -702,7 +702,7 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
             if (!X509_cmp(obj->data.x509, x->data.x509))
                 return obj;
         } else if (x->type == X509_LU_CRL) {
-            if (!X509_CRL_match(obj->data.crl, x->data.crl))
+            if (X509_CRL_match(obj->data.crl, x->data.crl) == 0)
                 return obj;
         } else
             return obj;

crypto/x509/x_all.c

@@ -392,7 +392,7 @@ int X509_digest(const X509 *cert, const EVP_MD *md, unsigned char *data,
                 unsigned int *len)
 {
     if (EVP_MD_is_a(md, SN_sha1) && (cert->ex_flags & EXFLAG_SET) != 0
-            && (cert->ex_flags & EXFLAG_INVALID) == 0) {
+            && (cert->ex_flags & EXFLAG_NO_FINGERPRINT) == 0) {
         /* Asking for SHA1 and we already computed it. */
         if (len != NULL)
             *len = sizeof(cert->sha1_hash);
@@ -436,7 +436,7 @@ int X509_CRL_digest(const X509_CRL *data, const EVP_MD *type,
                     unsigned char *md, unsigned int *len)
 {
     if (type == EVP_sha1() && (data->flags & EXFLAG_SET) != 0
-            && (data->flags & EXFLAG_INVALID) == 0) {
+            && (data->flags & EXFLAG_NO_FINGERPRINT) == 0) {
         /* Asking for SHA1; always computed in CRL d2i. */
         if (len != NULL)
             *len = sizeof(data->sha1_hash);

crypto/x509/x_crl.c

@@ -147,7 +147,7 @@ static int crl_set_issuers(X509_CRL *crl)
 
 /*
  * The X509_CRL structure needs a bit of customisation. Cache some extensions
- * and hash of the whole CRL.
+ * and hash of the whole CRL or set EXFLAG_NO_FINGERPRINT if this fails.
  */
 static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
                   void *exarg)
@@ -185,7 +185,7 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
 
     case ASN1_OP_D2I_POST:
         if (!X509_CRL_digest(crl, EVP_sha1(), crl->sha1_hash, NULL))
-            crl->flags |= EXFLAG_INVALID;
+            crl->flags |= EXFLAG_NO_FINGERPRINT;
         crl->idp = X509_CRL_get_ext_d2i(crl,
                                         NID_issuing_distribution_point, &i,
                                         NULL);

doc/internal/man3/x509v3_cache_extensions.pod

@@ -17,7 +17,8 @@ This function processes any X509v3 extensions present in an X509 object I<x>
 and caches the result of that processing as well as further derived info,
 for instance whether the certificate is self-issued or has version X.509v1.
 It computes the SHA1 digest of the certificate using the default library context
-and property query string and stores the result in x->sha1_hash.
+and property query string and stores the result in x->sha1_hash,
+or on failure sets B<EXFLAG_NO_FINGERPRINT> in x->flags.
 It sets B<X509_SIG_INFO_VALID> in x->flags if x->siginf was filled successfully,
 which may not be possible if a referenced algorithm is unknown or not available.
 Many OpenSSL functions that use an X509 object call this function implicitly.

doc/man3/X509_cmp.pod

@@ -55,7 +55,8 @@ The B<X509> comparison functions return B<-1>, B<0>, or B<1> if object I<a> is
 found to be less than, to match, or be greater than object I<b>, respectively.
 
 X509_NAME_cmp(), X509_issuer_and_serial_cmp(), X509_issuer_name_cmp(),
-X509_subject_name_cmp() and X509_CRL_cmp() may return B<-2> to indicate an error.
+X509_subject_name_cmp(), X509_CRL_cmp(), and X509_CRL_match()
+may return B<-2> to indicate an error.
 
 =head1 NOTES
 

doc/man3/X509_get_extension_flags.pod

@@ -78,12 +78,17 @@ The certificate contains an unhandled critical extension.
 
 =item B<EXFLAG_INVALID>
 
-Some certificate extension values are invalid or inconsistent. The
-certificate should be rejected.
+Some certificate extension values are invalid or inconsistent.
+The certificate should be rejected.
 This bit may also be raised after an out-of-memory error while
 processing the X509 object, so it may not be related to the processed
 ASN1 object itself.
 
+=item B<EXFLAG_NO_FINGERPRINT>
+
+Failed to compute the internal SHA1 hash value of the certificate or CRL.
+This may be due to malloc failure or because no SHA1 implementation was found.
+
 =item B<EXFLAG_INVALID_POLICY>
 
 The NID_certificate_policies certificate extension is invalid or

include/openssl/x509v3.h.in

@@ -406,6 +406,7 @@ struct ISSUING_DIST_POINT_st {
 # define EXFLAG_AKID_CRITICAL    0x20000
 # define EXFLAG_SKID_CRITICAL    0x40000
 # define EXFLAG_SAN_CRITICAL     0x80000
+# define EXFLAG_NO_FINGERPRINT   0x100000
 
 # define KU_DIGITAL_SIGNATURE    0x0080
 # define KU_NON_REPUDIATION      0x0040

test/certs/invalid-cert.pem

@@ -0,0 +1,19 @@
+-----BEGIN TRUSTED CERTIFICATE-----
+MIIDJTCCAg2gAwIBAgIUEUSW5o7qpgNCWyXic9Fc9tCLS0gwDQYJKoZIhvcNAQEL
+BQAwEzERMA8GA1UEAwwIUGVyc29TaW0wHhcNMjAxMjE2MDY1NjM5WhcNMzAxMjE2
+MDY1NjM5WjATMREwDwYDVQQDDAhQZXJzb1NpbTCCASIwDQYJKoZIhvcNAQEBBQAD
+ggEPADCCAQoCggEBAMsgRKnnZbQtG9bB9Hn+CoOOsanmnRELSlGq521qi/eBgs2w
+SdHYM6rsJFwY89RvINLGeUZh/pu7c+ODtTafAWE3JkynG01d2Zrvp1V1r97+FGyD
+f+b1hAggxBy70bTRyr1gAoKQTAm74U/1lj13EpWz7zshgXJ/Pn/hUyTmpNW+fTRE
+xaifN0jkl5tZUURGA6w3+BRhVDQtt92vLihqUGaEFpL8yqqFnN44AoQ5+lgMafWi
+UyYMHcK75ZB8WWklq8zjRP3xC1h56k01rT6KJO6i+BxMcADerYsn5qTlcUiKcpRU
+b6RzLvCUwj91t1aX6npDI3BzSP+wBUUANBfuHEMCAwEAAaNxMG8wFwYDVR0OBBA8
+yBBnvz1Zt6pHm2GwBaRyMBcGA1UdIwQQPMgQZ789WbeqR5thsAWkcjAPBgNVHRMB
+Af8EBTADAQH/MAsGA1UdDwQEAwIChDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYB
+BQUHAwIwDQYJKoZIhvcNAQELBQADggEBAIEzVbttOUc7kK4aY+74TANFZK/qtBQ7
+94a/P30TGWSRUq2HnDsR8Vo4z8xm5oKeC+SIi6NGzviWYquuzpJ7idcbr0MIuSyD
++Vg6n1sG64DxWNdGO9lR5c4mWFdIajShczS2+4QIRB/lFZCf7GhPMtIcbP1o9ckY
+2vyv5ZAEU9Z5n0PY+abrKsj0XyvJwdycEsUTywa36fuv6hP3UboLtvK6naXLMrTj
+WtSA6PXjHy7h8h0NC8XLk64mc0lcRC4WM+xJ/C+NHglpmBqBxnStpnZykMZYD1Vy
+JJ1wNc+Y3e2uMBDxZviH3dIPIgqP1Vpi2TWfqr3DTBNCRf4dl/wwNU8=
+-----END TRUSTED CERTIFICATE-----

test/recipes/80-test_x509aux.t

@@ -14,14 +14,17 @@ use OpenSSL::Test::Utils;
 
 setup("test_x509aux");
 
+my @path = qw(test certs);
+
 plan skip_all => "test_dane uses ec which is not supported by this OpenSSL build"
     if disabled("ec");
 
 plan tests => 1;                # The number of tests being performed
 
 ok(run(test(["x509aux", 
-                srctop_file("test", "certs", "roots.pem"),
-                srctop_file("test", "certs", "root+anyEKU.pem"),
-                srctop_file("test", "certs", "root-anyEKU.pem"),
-                srctop_file("test", "certs", "root-cert.pem")]
-        )), "x509aux tests");
+             srctop_file(@path, "roots.pem"),
+             srctop_file(@path, "root+anyEKU.pem"),
+             srctop_file(@path, "root-anyEKU.pem"),
+             srctop_file(@path, "root-cert.pem"),
+             srctop_file(@path, "invalid-cert.pem"),
+            ])), "x509aux tests");

test/x509aux.c

@@ -30,17 +30,16 @@ static int test_certs(int num)
     typedef int (*i2d_X509_t)(const X509 *, unsigned char **);
     int err = 0;
     BIO *fp = BIO_new_file(test_get_argument(num), "r");
-    X509 *reuse = NULL;
 
     if (!TEST_ptr(fp))
         return 0;
 
     for (c = 0; !err && PEM_read_bio(fp, &name, &header, &data, &len); ++c) {
         const int trusted = (strcmp(name, PEM_STRING_X509_TRUSTED) == 0);
-
         d2i_X509_t d2i = trusted ? d2i_X509_AUX : d2i_X509;
         i2d_X509_t i2d = trusted ? i2d_X509_AUX : i2d_X509;
         X509 *cert = NULL;
+        X509 *reuse = NULL;
         const unsigned char *p = data;
         unsigned char *buf = NULL;
         unsigned char *bufp;
@@ -93,9 +92,15 @@ static int test_certs(int num)
             goto next;
         }
         p = buf;
-        reuse = d2i(&reuse, &p, enclen);
-        if (reuse == NULL || X509_cmp (reuse, cert)) {
-            TEST_error("X509_cmp does not work with %s", name);
+        reuse = d2i(NULL, &p, enclen);
+        if (reuse == NULL) {
+            TEST_error("second d2i call failed for %s", name);
+            err = 1;
+            goto next;
+        }
+        err = X509_cmp(reuse, cert);
+        if (err != 0) {
+            TEST_error("X509_cmp for %s resulted in %d", name, err);
             err = 1;
             goto next;
         }
@@ -141,13 +146,13 @@ static int test_certs(int num)
          */
     next:
         X509_free(cert);
+        X509_free(reuse);
         OPENSSL_free(buf);
         OPENSSL_free(name);
         OPENSSL_free(header);
         OPENSSL_free(data);
     }
     BIO_free(fp);
-    X509_free(reuse);
 
     if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) {
         /* Reached end of PEM file */