Ensure verify error is set when X509_verify_cert() fails

Set ctx->error = X509_V_ERR_OUT_OF_MEM when verificaiton cannot
continue due to malloc failure.  Also, when X509_verify_cert()
returns <= 0 make sure that the verification status does not remain
X509_V_OK, as a last resort set it it to X509_V_ERR_UNSPECIFIED,
just in case some code path returns an error without setting an
appropriate value of ctx->error.

Reviewed-by: Richard Levitte <levitte@openssl.org>

diff --git a/crypto/x509/x509_txt.c b/crypto/x509/x509_txt.c
index 293efcfb8e6733d6ab82c9b6f2db689ee3baa397..5341e796692f6ad6cee03781bcd5f6a05b035d0c 100644 (file)
--- a/crypto/x509/x509_txt.c
+++ b/crypto/x509/x509_txt.c
@@ -161,6 +161,10 @@ const char *X509_verify_cert_error_string(long n)
         return ("CA certificate key too weak");
     case X509_V_ERR_CA_MD_TOO_WEAK:
         return ("CA signature digest algorithm too weak");
+    case X509_V_ERR_INVALID_CALL:
+        return ("Invalid certificate verification context");
+    case X509_V_ERR_STORE_LOOKUP:
+        return ("Issuer certificate lookup error");
 
     default:
         /* Printing an error number into a static buffer is not thread-safe */

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 866aa3992e06128002e6acd4d8b277396e8c5be7..a5e77896f8fbebc1b09f29f63a4a3fa3f751cbd0 100644 (file)
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -251,9 +251,11 @@ static int verify_chain(X509_STORE_CTX *ctx)
 int X509_verify_cert(X509_STORE_CTX *ctx)
 {
     SSL_DANE *dane = ctx->dane;
+    int ret;
 
     if (ctx->cert == NULL) {
         X509err(X509_F_X509_VERIFY_CERT, X509_R_NO_CERT_SET_FOR_US_TO_VERIFY);
+        ctx->error = X509_V_ERR_INVALID_CALL;
         return -1;
     }
 
@@ -263,6 +265,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
          * cannot do another one.
          */
         X509err(X509_F_X509_VERIFY_CERT, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+        ctx->error = X509_V_ERR_INVALID_CALL;
         return -1;
     }
 
@@ -273,6 +276,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
     if (((ctx->chain = sk_X509_new_null()) == NULL) ||
         (!sk_X509_push(ctx->chain, ctx->cert))) {
         X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         return -1;
     }
     X509_up_ref(ctx->cert);
@@ -283,15 +287,19 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
         !verify_cb_cert(ctx, ctx->cert, 0, X509_V_ERR_EE_KEY_TOO_SMALL))
         return 0;
 
+    if (DANETLS_ENABLED(dane))
+        ret = dane_verify(ctx);
+    else
+        ret = verify_chain(ctx);
+
     /*
-     * If dane->trecs is an empty stack, we'll fail, since the user enabled
-     * DANE.  If none of the TLSA records were usable, and it makes sense to
-     * keep going with an unauthenticated handshake, they can handle that in
-     * the verify callback, or not set SSL_VERIFY_PEER.
+     * Safety-net.  If we are returning an error, we must also set ctx->error,
+     * so that the chain is not considered verified should the error be ignored
+     * (e.g. TLS with SSL_VERIFY_NONE).
      */
-    if (DANETLS_ENABLED(dane))
-        return dane_verify(ctx);
-    return verify_chain(ctx);
+    if (ret <= 0 && ctx->error == X509_V_OK)
+        ctx->error = X509_V_ERR_UNSPECIFIED;
+    return ret;
 }
 
 /*
@@ -562,8 +570,16 @@ static int check_name_constraints(X509_STORE_CTX *ctx)
             if (nc) {
                 int rv = NAME_CONSTRAINTS_check(x, nc);
 
-                if (rv != X509_V_OK && !verify_cb_cert(ctx, x, i, rv))
+                switch (rv) {
+                case X509_V_OK:
+                    break;
+                case X509_V_ERR_OUT_OF_MEM:
                     return 0;
+                default:
+                    if (!verify_cb_cert(ctx, x, i, rv))
+                        return 0;
+                    break;
+                }
             }
         }
     }
@@ -1457,6 +1473,7 @@ static int check_policy(X509_STORE_CTX *ctx)
      */
     if (ctx->bare_ta_signed && !sk_X509_push(ctx->chain, NULL)) {
         X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         return 0;
     }
     ret = X509_policy_check(&ctx->tree, &ctx->explicit_policy, ctx->chain,
@@ -1466,6 +1483,7 @@ static int check_policy(X509_STORE_CTX *ctx)
 
     if (ret == X509_PCY_TREE_INTERNAL) {
         X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         return 0;
     }
     /* Invalid or inconsistent extensions */
@@ -1496,7 +1514,12 @@ static int check_policy(X509_STORE_CTX *ctx)
 
     if (ctx->param->flags & X509_V_FLAG_NOTIFY_POLICY) {
         ctx->current_cert = NULL;
-        ctx->error = X509_V_OK;
+        /*
+         * Verification errors need to be "sticky", a callback may have allowed
+         * an SSL handshake to continue despite an error, and we must then
+         * remain in an error state.  Therefore, we MUST NOT clear earlier
+         * verification errors by setting the error to X509_V_OK.
+         */
         if (!ctx->verify_cb(2, ctx))
             return 0;
     }
@@ -2742,6 +2765,7 @@ static int build_chain(X509_STORE_CTX *ctx)
      */
     if (ctx->untrusted && (sktmp = sk_X509_dup(ctx->untrusted)) == NULL) {
         X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         return 0;
     }
 
@@ -2758,12 +2782,14 @@ static int build_chain(X509_STORE_CTX *ctx)
     if (DANETLS_ENABLED(dane) && dane->certs != NULL) {
         if (sktmp == NULL && (sktmp = sk_X509_new_null()) == NULL) {
             X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
+            ctx->error = X509_V_ERR_OUT_OF_MEM;
             return 0;
         }
         for (i = 0; i < sk_X509_num(dane->certs); ++i) {
             if (!sk_X509_push(sktmp, sk_X509_value(dane->certs, i))) {
                 sk_X509_free(sktmp);
                 X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
+                ctx->error = X509_V_ERR_OUT_OF_MEM;
                 return 0;
             }
         }
@@ -2827,6 +2853,7 @@ static int build_chain(X509_STORE_CTX *ctx)
 
             if (ok < 0) {
                 trust = X509_TRUST_REJECTED;
+                ctx->error = X509_V_ERR_STORE_LOOKUP;
                 search = 0;
                 continue;
             }
@@ -2873,6 +2900,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                         X509_free(xtmp);
                         X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
                         trust = X509_TRUST_REJECTED;
+                        ctx->error = X509_V_ERR_OUT_OF_MEM;
                         search = 0;
                         continue;
                     }
@@ -2969,6 +2997,7 @@ static int build_chain(X509_STORE_CTX *ctx)
             if (!sk_X509_push(ctx->chain, xtmp)) {
                 X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
                 trust = X509_TRUST_REJECTED;
+                ctx->error = X509_V_ERR_OUT_OF_MEM;
                 search = 0;
                 continue;
             }

diff --git a/crypto/x509v3/v3_addr.c b/crypto/x509v3/v3_addr.c
index 53583fb754fdcac50cd45bcf5db5edfdd6a4a33d..be8ca5dcbb5a6bb5a2cf2e0b786911642eced58e 100644 (file)
--- a/crypto/x509v3/v3_addr.c
+++ b/crypto/x509v3/v3_addr.c
@@ -1166,6 +1166,11 @@ int X509v3_addr_subset(IPAddrBlocks *a, IPAddrBlocks *b)
 
 /*
  * Core code for RFC 3779 2.3 path validation.
+ *
+ * Returns 1 for success, 0 on error.
+ *
+ * When returning 0, ctx->error MUST be set to an appropriate value other than
+ * X509_V_OK.
  */
 static int addr_validate_path_internal(X509_STORE_CTX *ctx,
                                        STACK_OF(X509) *chain,
@@ -1200,6 +1205,7 @@ static int addr_validate_path_internal(X509_STORE_CTX *ctx,
     if ((child = sk_IPAddressFamily_dup(ext)) == NULL) {
         X509V3err(X509V3_F_ADDR_VALIDATE_PATH_INTERNAL,
                   ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         ret = 0;
         goto done;
     }

diff --git a/doc/crypto/X509_verify_cert.pod b/doc/crypto/X509_verify_cert.pod
index 6b4ff7e74277cb480189eb832ef325a0f74c87d1..c7a7bb4b12fbf339e649b3e698dbd5e8a3a8f5c3 100644 (file)
--- a/doc/crypto/X509_verify_cert.pod
+++ b/doc/crypto/X509_verify_cert.pod
@@ -32,7 +32,7 @@ OpenSSL internally for certificate validation, in both the S/MIME and
 SSL/TLS code.
 
 A negative return value from X509_verify_cert() can occur if it is invoked
-incurrectly, such as with no certificate set in B<ctx>, or when it is called
+incorrectly, such as with no certificate set in B<ctx>, or when it is called
 twice in succession without reinitialising B<ctx> for the second call.
 A negative return value can also happen due to internal resource problems or if
 a retry operation is requested during internal lookups (which never happens

diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h
index 3826c75b1c3067789c5d03b5c55078b8cd76dc48..44f1f1699136c1b5db2f5c66a4f0287be54ceac8 100644 (file)
--- a/include/openssl/x509_vfy.h
+++ b/include/openssl/x509_vfy.h
@@ -159,6 +159,11 @@ void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth);
 # define         X509_V_ERR_CA_KEY_TOO_SMALL                     67
 # define         X509_V_ERR_CA_MD_TOO_WEAK                       68
 
+/* Caller error */
+# define         X509_V_ERR_INVALID_CALL                         69
+/* Issuer lookup error */
+# define         X509_V_ERR_STORE_LOOKUP                         70
+
 /* Certificate verify flags */
 
 # if OPENSSL_API_COMPAT < 0x10100000L

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index a9e1a2496dfc5187742175d2ef06723d6dd0e7b5..6ceb9ec39ead7278ddf2a8b0706d5977ae9bc458 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -548,6 +548,13 @@ int ssl_verify_alarm_type(long type)
     case X509_V_ERR_CRL_NOT_YET_VALID:
     case X509_V_ERR_CERT_UNTRUSTED:
     case X509_V_ERR_CERT_REJECTED:
+    case X509_V_ERR_HOSTNAME_MISMATCH:
+    case X509_V_ERR_EMAIL_MISMATCH:
+    case X509_V_ERR_IP_ADDRESS_MISMATCH:
+    case X509_V_ERR_DANE_NO_MATCH:
+    case X509_V_ERR_EE_KEY_TOO_SMALL:
+    case X509_V_ERR_CA_KEY_TOO_SMALL:
+    case X509_V_ERR_CA_MD_TOO_WEAK:
         al = SSL_AD_BAD_CERTIFICATE;
         break;
     case X509_V_ERR_CERT_SIGNATURE_FAILURE:
@@ -561,7 +568,10 @@ int ssl_verify_alarm_type(long type)
     case X509_V_ERR_CERT_REVOKED:
         al = SSL_AD_CERTIFICATE_REVOKED;
         break;
+    case X509_V_ERR_UNSPECIFIED:
     case X509_V_ERR_OUT_OF_MEM:
+    case X509_V_ERR_INVALID_CALL:
+    case X509_V_ERR_STORE_LOOKUP:
         al = SSL_AD_INTERNAL_ERROR;
         break;
     case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT: