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>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 10a0423..f1c25ef 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3669,9 +3669,9 @@
* |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 @@
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 */
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index d7b1a2c..237f452 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -1101,11 +1101,11 @@
}
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;
}
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index f5bf0da..1d7c7ee 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -712,7 +712,6 @@
/* 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 @@
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 @@
/* 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 @@
/* 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;
diff --git a/ssl/internal.h b/ssl/internal.h
index 4f13459..6e3a744 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1301,7 +1301,8 @@
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
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index c35834e..3cfcd8d 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -285,7 +285,8 @@
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 @@
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:
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index da66263..9be76be 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -441,7 +441,6 @@
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 @@
}
}
-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) {
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index e26866b..a9eb5bd 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -801,7 +801,6 @@
ssl->session = session;
if (session != NULL) {
SSL_SESSION_up_ref(session);
- ssl->verify_result = session->verify_result;
}
return 1;
diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c
index 8400c60..bccc09b 100644
--- a/ssl/tls13_both.c
+++ b/ssl/tls13_both.c
@@ -184,7 +184,6 @@
/* 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 @@
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);
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index eb11738..5bb52f2 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -165,7 +165,6 @@
return ssl_hs_error;
}
ssl->s3->session_reused = 1;
- ssl->verify_result = session->verify_result;
SSL_SESSION_free(session);
}
@@ -492,7 +491,6 @@
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. */
