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.  Similarly for issuer lookup failures
and caller errors (bad parameters or invalid state).

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.

Add new and some missing error codes to X509 error -> SSL alert switch.

Reviewed-by: Rich Salz <rsalz@openssl.org>

5 files changed, 70 insertions, 8 deletions

diff --git a/crypto/x509/x509_txt.c b/crypto/x509/x509_txt.c
index d834180ec4..0952813bd2 100644
--- a/crypto/x509/x509_txt.c
+++ b/crypto/x509/x509_txt.c
@@ -183,6 +183,10 @@ const char *X509_verify_cert_error_string(long n)
         return ("unsupported or invalid name syntax");
     case X509_V_ERR_CRL_PATH_VALIDATION_ERROR:
         return ("CRL path validation error");
+    case X509_V_ERR_INVALID_CALL:
+        return ("Invalid certificate verification context");
+    case X509_V_ERR_STORE_LOOKUP:
+        return ("Issuer certificate lookup error");
 
     default:
         BIO_snprintf(buf, sizeof buf, "error number %ld", n);
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 3bad523f71..2e2287e6c9 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -160,6 +160,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
     STACK_OF(X509) *sktmp = NULL;
     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;
     }
     if (ctx->chain != NULL) {
@@ -168,6 +169,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;
     }
 
@@ -180,6 +182,8 @@ 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;
+        ok = -1;
         goto end;
     }
     CRYPTO_add(&ctx->cert->references, 1, CRYPTO_LOCK_X509);
@@ -189,6 +193,8 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
     if (ctx->untrusted != NULL
         && (sktmp = sk_X509_dup(ctx->untrusted)) == NULL) {
         X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
+        ok = -1;
         goto end;
     }
 
@@ -214,6 +220,8 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
             if (xtmp != NULL) {
                 if (!sk_X509_push(ctx->chain, xtmp)) {
                     X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
+                    ctx->error = X509_V_ERR_OUT_OF_MEM;
+                    ok = -1;
                     goto end;
                 }
                 CRYPTO_add(&xtmp->references, 1, CRYPTO_LOCK_X509);
@@ -293,15 +301,19 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
             if (ctx->check_issued(ctx, x, x))
                 break;
             ok = ctx->get_issuer(&xtmp, ctx, x);
-            if (ok < 0)
-                return ok;
+            if (ok < 0) {
+                ctx->error = X509_V_ERR_STORE_LOOKUP;
+                goto end;
+            }
             if (ok == 0)
                 break;
             x = xtmp;
             if (!sk_X509_push(ctx->chain, x)) {
                 X509_free(xtmp);
                 X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
-                return 0;
+                ctx->error = X509_V_ERR_OUT_OF_MEM;
+                ok = -1;
+                goto end;
             }
             num++;
         }
@@ -317,8 +329,10 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
             while (j-- > 1) {
                 xtmp2 = sk_X509_value(ctx->chain, j - 1);
                 ok = ctx->get_issuer(&xtmp, ctx, xtmp2);
-                if (ok < 0)
+                if (ok < 0) {
+                    ctx->error = X509_V_ERR_STORE_LOOKUP;
                     goto end;
+                }
                 /* Check if we found an alternate chain */
                 if (ok > 0) {
                     /*
@@ -432,6 +446,10 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
         sk_X509_free(sktmp);
     if (chain_ss != NULL)
         X509_free(chain_ss);
+
+    /* Safety net, error returns must set ctx->error */
+    if (ok <= 0 && ctx->error == X509_V_OK)
+        ctx->error = X509_V_ERR_UNSPECIFIED;
     return ok;
 }
 
@@ -654,12 +672,19 @@ static int check_name_constraints(X509_STORE_CTX *ctx)
             NAME_CONSTRAINTS *nc = sk_X509_value(ctx->chain, j)->nc;
             if (nc) {
                 rv = NAME_CONSTRAINTS_check(x, nc);
-                if (rv != X509_V_OK) {
+                switch (rv) {
+                case X509_V_OK:
+                    continue;
+                case X509_V_ERR_OUT_OF_MEM:
+                    ctx->error = rv;
+                    return 0;
+                default:
                     ctx->error = rv;
                     ctx->error_depth = i;
                     ctx->current_cert = x;
                     if (!ctx->verify_cb(0, ctx))
                         return 0;
+                    break;
                 }
             }
         }
@@ -1469,6 +1494,7 @@ static int check_policy(X509_STORE_CTX *ctx)
                             ctx->param->policies, ctx->param->flags);
     if (ret == 0) {
         X509err(X509_F_CHECK_POLICY, ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         return 0;
     }
     /* Invalid or inconsistent extensions */
@@ -1497,7 +1523,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;
     }
diff --git a/crypto/x509/x509_vfy.h b/crypto/x509/x509_vfy.h
index b7d8b2472e..eefca75183 100644
--- a/crypto/x509/x509_vfy.h
+++ b/crypto/x509/x509_vfy.h
@@ -365,13 +365,31 @@ void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth);
 # define         X509_V_ERR_PERMITTED_VIOLATION                  47
 # define         X509_V_ERR_EXCLUDED_VIOLATION                   48
 # define         X509_V_ERR_SUBTREE_MINMAX                       49
+# define         X509_V_ERR_APPLICATION_VERIFICATION             50
 # define         X509_V_ERR_UNSUPPORTED_CONSTRAINT_TYPE          51
 # define         X509_V_ERR_UNSUPPORTED_CONSTRAINT_SYNTAX        52
 # define         X509_V_ERR_UNSUPPORTED_NAME_SYNTAX              53
 # define         X509_V_ERR_CRL_PATH_VALIDATION_ERROR            54
 
-/* The application is not happy */
-# define         X509_V_ERR_APPLICATION_VERIFICATION             50
+# if 0 /* Reserved for compatibility 1.0.2 */
+/* Suite B mode algorithm violation */
+#  define         X509_V_ERR_SUITE_B_INVALID_VERSION              56
+#  define         X509_V_ERR_SUITE_B_INVALID_ALGORITHM            57
+#  define         X509_V_ERR_SUITE_B_INVALID_CURVE                58
+#  define         X509_V_ERR_SUITE_B_INVALID_SIGNATURE_ALGORITHM  59
+#  define         X509_V_ERR_SUITE_B_LOS_NOT_ALLOWED              60
+#  define         X509_V_ERR_SUITE_B_CANNOT_SIGN_P_384_WITH_P_256 61
+
+/* Host, email and IP check errors */
+#  define         X509_V_ERR_HOSTNAME_MISMATCH                    62
+#  define         X509_V_ERR_EMAIL_MISMATCH                       63
+#  define         X509_V_ERR_IP_ADDRESS_MISMATCH                  64
+# endif
+
+/* Caller error */
+# define         X509_V_ERR_INVALID_CALL                         65
+/* Issuer lookup error */
+# define         X509_V_ERR_STORE_LOOKUP                         66
 
 /* Certificate verify flags */
 
diff --git a/crypto/x509v3/v3_addr.c b/crypto/x509v3/v3_addr.c
index 94cfed0509..1290dec9bb 100644
--- a/crypto/x509v3/v3_addr.c
+++ b/crypto/x509v3/v3_addr.c
@@ -1211,6 +1211,11 @@ int v3_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 v3_addr_validate_path_internal(X509_STORE_CTX *ctx,
                                           STACK_OF(X509) *chain,
@@ -1245,6 +1250,7 @@ static int v3_addr_validate_path_internal(X509_STORE_CTX *ctx,
     if ((child = sk_IPAddressFamily_dup(ext)) == NULL) {
         X509V3err(X509V3_F_V3_ADDR_VALIDATE_PATH_INTERNAL,
                   ERR_R_MALLOC_FAILURE);
+        ctx->error = X509_V_ERR_OUT_OF_MEM;
         ret = 0;
         goto done;
     }
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 107b460f27..c51fc6ea01 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -619,7 +619,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: