Remove ssl->verify_result.

Having two copies of this is confusing. This field is inherently tied to
the certificate chain, which lives on SSL_SESSION, so this should live
there too. This also wasn't getting reset correctly on SSL_clear, but
this is now resolved.

Change-Id: I22b1734a93320bb0bf0dc31faa74d77a8e1de906
Reviewed-on: https://boringssl-review.googlesource.com/10283
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

include/openssl/ssl.h

@@ -3669,9 +3669,9 @@ struct ssl_session_st {
    * |peer|, but when a server it does not. */
   STACK_OF(X509) *cert_chain;
 
-  /* when app_verify_callback accepts a session where the peer's certificate is
-   * not ok, we must remember the error for session reuse: */
-  long verify_result; /* only for servers */
+  /* verify_result is the result of certificate verification in the case of
+   * non-fatal certificate errors. */
+  long verify_result;
 
   long timeout;
   long time;
@@ -4131,7 +4131,6 @@ struct ssl_st {
   SSL_CTX *ctx;
 
   /* extra application data */
-  long verify_result;
   CRYPTO_EX_DATA ex_data;
 
   /* for server side, keep the list of CA_dn we can use */

ssl/handshake_client.c

@@ -1101,11 +1101,11 @@ f_err:
 }
 
 static int ssl3_verify_server_cert(SSL *ssl) {
-  if (!ssl_verify_cert_chain(ssl, ssl->s3->new_session->cert_chain)) {
+  if (!ssl_verify_cert_chain(ssl, &ssl->s3->new_session->verify_result,
+                             ssl->s3->new_session->cert_chain)) {
     return -1;
   }
 
-  ssl->s3->new_session->verify_result = ssl->verify_result;
   return 1;
 }
 

ssl/handshake_server.c

@@ -712,7 +712,6 @@ static int ssl3_get_client_hello(SSL *ssl) {
       /* Use the old session. */
       ssl->session = session;
       session = NULL;
-      ssl->verify_result = ssl->session->verify_result;
       ssl->s3->session_reused = 1;
     } else {
       SSL_set_session(ssl, NULL);
@@ -813,7 +812,6 @@ static int ssl3_get_client_hello(SSL *ssl) {
     if (!ssl->s3->tmp.cert_request) {
       /* OpenSSL returns X509_V_OK when no certificates are requested. This is
        * classed by them as a bug, but it's assumed by at least NGINX. */
-      ssl->verify_result = X509_V_OK;
       ssl->s3->new_session->verify_result = X509_V_OK;
     }
   }
@@ -1261,7 +1259,6 @@ static int ssl3_get_client_certificate(SSL *ssl) {
 
       /* OpenSSL returns X509_V_OK when no certificates are received. This is
        * classed by them as a bug, but it's assumed by at least NGINX. */
-      ssl->verify_result = X509_V_OK;
       ssl->s3->new_session->verify_result = X509_V_OK;
       ssl->s3->tmp.reuse_message = 1;
       return 1;
@@ -1312,21 +1309,21 @@ static int ssl3_get_client_certificate(SSL *ssl) {
 
     /* OpenSSL returns X509_V_OK when no certificates are received. This is
      * classed by them as a bug, but it's assumed by at least NGINX. */
-    ssl->verify_result = X509_V_OK;
+    ssl->s3->new_session->verify_result = X509_V_OK;
   } else {
     /* The hash would have been filled in. */
     if (ssl->ctx->retain_only_sha256_of_client_certs) {
       ssl->s3->new_session->peer_sha256_valid = 1;
     }
 
-    if (!ssl_verify_cert_chain(ssl, chain)) {
+    if (!ssl_verify_cert_chain(ssl, &ssl->s3->new_session->verify_result,
+                               chain)) {
       goto err;
     }
   }
 
   X509_free(ssl->s3->new_session->peer);
   ssl->s3->new_session->peer = sk_X509_shift(chain);
-  ssl->s3->new_session->verify_result = ssl->verify_result;
 
   sk_X509_pop_free(ssl->s3->new_session->cert_chain, X509_free);
   ssl->s3->new_session->cert_chain = chain;

ssl/internal.h

@@ -1301,7 +1301,8 @@ int ssl_cert_add1_chain_cert(CERT *cert, X509 *x509);
 void ssl_cert_set_cert_cb(CERT *cert,
                           int (*cb)(SSL *ssl, void *arg), void *arg);
 
-int ssl_verify_cert_chain(SSL *ssl, STACK_OF(X509) *cert_chain);
+int ssl_verify_cert_chain(SSL *ssl, long *out_verify_result,
+                          STACK_OF(X509) * cert_chain);
 void ssl_update_cache(SSL *ssl, int mode);
 
 /* ssl_get_compatible_server_ciphers determines the key exchange and

ssl/ssl_cert.c

@@ -285,7 +285,8 @@ void ssl_cert_set_cert_cb(CERT *c, int (*cb)(SSL *ssl, void *arg), void *arg) {
   c->cert_cb_arg = arg;
 }
 
-int ssl_verify_cert_chain(SSL *ssl, STACK_OF(X509) *cert_chain) {
+int ssl_verify_cert_chain(SSL *ssl, long *out_verify_result,
+                          STACK_OF(X509) *cert_chain) {
   if (cert_chain == NULL || sk_X509_num(cert_chain) == 0) {
     return 0;
   }
@@ -326,17 +327,15 @@ int ssl_verify_cert_chain(SSL *ssl, STACK_OF(X509) *cert_chain) {
     verify_ret = X509_verify_cert(&ctx);
   }
 
-  ssl->verify_result = ctx.error;
-
   /* If |SSL_VERIFY_NONE|, the error is non-fatal, but we keep the result. */
   if (verify_ret <= 0 && ssl->verify_mode != SSL_VERIFY_NONE) {
-    ssl3_send_alert(ssl, SSL3_AL_FATAL,
-                    ssl_verify_alarm_type(ssl->verify_result));
+    ssl3_send_alert(ssl, SSL3_AL_FATAL, ssl_verify_alarm_type(ctx.error));
     OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_VERIFY_FAILED);
     goto err;
   }
 
   ERR_clear_error();
+  *out_verify_result = ctx.error;
   ret = 1;
 
 err:

ssl/ssl_lib.c

@@ -441,7 +441,6 @@ SSL *SSL_new(SSL_CTX *ctx) {
     ssl->alpn_client_proto_list_len = ssl->ctx->alpn_client_proto_list_len;
   }
 
-  ssl->verify_result = X509_V_ERR_INVALID_CALL;
   ssl->method = ctx->method;
 
   if (!ssl->method->ssl_new(ssl)) {
@@ -2316,7 +2315,13 @@ void SSL_set_verify_result(SSL *ssl, long result) {
   }
 }
 
-long SSL_get_verify_result(const SSL *ssl) { return ssl->verify_result; }
+long SSL_get_verify_result(const SSL *ssl) {
+  SSL_SESSION *session = SSL_get_session(ssl);
+  if (session == NULL) {
+    return X509_V_ERR_INVALID_CALL;
+  }
+  return session->verify_result;
+}
 
 int SSL_get_ex_new_index(long argl, void *argp, CRYPTO_EX_unused *unused,
                          CRYPTO_EX_dup *dup_func, CRYPTO_EX_free *free_func) {

ssl/ssl_session.c

@@ -801,7 +801,6 @@ int SSL_set_session(SSL *ssl, SSL_SESSION *session) {
   ssl->session = session;
   if (session != NULL) {
     SSL_SESSION_up_ref(session);
-    ssl->verify_result = session->verify_result;
   }
 
   return 1;

ssl/tls13_both.c

@@ -184,7 +184,6 @@ int tls13_process_certificate(SSL *ssl, int allow_anonymous) {
 
     /* OpenSSL returns X509_V_OK when no certificates are requested. This is
      * classed by them as a bug, but it's assumed by at least NGINX. */
-    ssl->verify_result = X509_V_OK;
     ssl->s3->new_session->verify_result = X509_V_OK;
 
     /* No certificate, so nothing more to do. */
@@ -194,12 +193,11 @@ int tls13_process_certificate(SSL *ssl, int allow_anonymous) {
 
   ssl->s3->new_session->peer_sha256_valid = retain_sha256;
 
-  if (!ssl_verify_cert_chain(ssl, chain)) {
+  if (!ssl_verify_cert_chain(ssl, &ssl->s3->new_session->verify_result,
+                             chain)) {
     goto err;
   }
 
-  ssl->s3->new_session->verify_result = ssl->verify_result;
-
   X509_free(ssl->s3->new_session->peer);
   X509 *leaf = sk_X509_value(chain, 0);
   X509_up_ref(leaf);

ssl/tls13_server.c

@@ -165,7 +165,6 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) {
       return ssl_hs_error;
     }
     ssl->s3->session_reused = 1;
-    ssl->verify_result = session->verify_result;
     SSL_SESSION_free(session);
   }
 
@@ -492,7 +491,6 @@ static enum ssl_hs_wait_t do_process_client_certificate(SSL *ssl,
   if (!ssl->s3->tmp.cert_request) {
     /* OpenSSL returns X509_V_OK when no certificates are requested. This is
      * classed by them as a bug, but it's assumed by at least NGINX. */
-    ssl->verify_result = X509_V_OK;
     ssl->s3->new_session->verify_result = X509_V_OK;
 
     /* Skip this state. */