Have |SSL_get_verify_result| return |X509_V_OK| when no client certificate is given.
9498e74 changed the default value of verify_result to an error. This
tripped up NGINX, which depends on a bug[1] in OpenSSL. netty-tcnative
also uses this behavior, though it currently isn't tripped up by 9498e74
because it calls |SSL_set_verify_result|. However, we would like to
remove |SSL_set_verify_result| and with two data points, it seems this
is behavior we must preserve.
This change sets |verify_result| to |X509_V_OK| when a) no client
certificate is requested or b) none is given and it's optional.
[1] See BUGS in https://www.openssl.org/docs/manmaster/ssl/SSL_get_verify_result.html
Change-Id: Ibd33660ae409bfe272963a8c39b7e9aa83c3d635
Reviewed-on: https://boringssl-review.googlesource.com/9067
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/ssl/handshake_server.c b/ssl/handshake_server.c
index 4818fe5..f5c8b3c 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -809,6 +809,13 @@
if (!ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ssl->s3->tmp.cert_request = 0;
}
+
+ 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;
+ }
}
/* Now that the cipher is known, initialize the handshake hash. */
@@ -1244,13 +1251,18 @@
if (ssl->s3->tmp.message_type != SSL3_MT_CERTIFICATE) {
if (ssl->version == SSL3_VERSION &&
ssl->s3->tmp.message_type == SSL3_MT_CLIENT_KEY_EXCHANGE) {
- /* In SSL 3.0, the Certificate message is omitted to signal no certificate. */
+ /* In SSL 3.0, the Certificate message is omitted to signal no
+ * certificate. */
if (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
return -1;
}
+ /* 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;
}
@@ -1297,6 +1309,10 @@
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
goto err;
}
+
+ /* 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;
} else {
/* The hash would have been filled in. */
if (ssl->ctx->retain_only_sha256_of_client_certs) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 4c7d2e7..8221286 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2730,91 +2730,84 @@
},
})
}
+
+ testCases = append(testCases, testCase{
+ name: "NoClientCertificate-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ ClientAuth: RequireAnyClientCert,
+ },
+ shouldFail: true,
+ expectedLocalError: "client didn't provide a certificate",
+ })
+
+ testCases = append(testCases, testCase{
+ // Even if not configured to expect a certificate, OpenSSL will
+ // return X509_V_OK as the verify_result.
+ testType: serverTest,
+ name: "NoClientCertificateRequested-Server-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ },
+ flags: []string{
+ "-expect-verify-result",
+ },
+ // TODO(davidben): Switch this to true when TLS 1.3
+ // supports session resumption.
+ resumeSession: ver.version < VersionTLS13,
+ })
+
+ testCases = append(testCases, testCase{
+ // If a client certificate is not provided, OpenSSL will still
+ // return X509_V_OK as the verify_result.
+ testType: serverTest,
+ name: "NoClientCertificate-Server-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ },
+ flags: []string{
+ "-expect-verify-result",
+ "-verify-peer",
+ },
+ // TODO(davidben): Switch this to true when TLS 1.3
+ // supports session resumption.
+ resumeSession: ver.version < VersionTLS13,
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "RequireAnyClientCertificate-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ },
+ flags: []string{"-require-any-client-certificate"},
+ shouldFail: true,
+ expectedError: ":PEER_DID_NOT_RETURN_A_CERTIFICATE:",
+ })
+
+ if ver.version != VersionSSL30 {
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "SkipClientCertificate-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ Bugs: ProtocolBugs{
+ SkipClientCertificate: true,
+ },
+ },
+ // Setting SSL_VERIFY_PEER allows anonymous clients.
+ flags: []string{"-verify-peer"},
+ shouldFail: true,
+ expectedError: ":UNEXPECTED_MESSAGE:",
+ })
+ }
}
- testCases = append(testCases, testCase{
- name: "NoClientCertificate",
- config: Config{
- MaxVersion: VersionTLS12,
- ClientAuth: RequireAnyClientCert,
- },
- shouldFail: true,
- expectedLocalError: "client didn't provide a certificate",
- })
-
- testCases = append(testCases, testCase{
- name: "NoClientCertificate-TLS13",
- config: Config{
- MaxVersion: VersionTLS13,
- ClientAuth: RequireAnyClientCert,
- },
- shouldFail: true,
- expectedLocalError: "client didn't provide a certificate",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "RequireAnyClientCertificate",
- config: Config{
- MaxVersion: VersionTLS12,
- },
- flags: []string{"-require-any-client-certificate"},
- shouldFail: true,
- expectedError: ":PEER_DID_NOT_RETURN_A_CERTIFICATE:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "RequireAnyClientCertificate-TLS13",
- config: Config{
- MaxVersion: VersionTLS13,
- },
- flags: []string{"-require-any-client-certificate"},
- shouldFail: true,
- expectedError: ":PEER_DID_NOT_RETURN_A_CERTIFICATE:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "RequireAnyClientCertificate-SSL3",
- config: Config{
- MaxVersion: VersionSSL30,
- },
- flags: []string{"-require-any-client-certificate"},
- shouldFail: true,
- expectedError: ":PEER_DID_NOT_RETURN_A_CERTIFICATE:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "SkipClientCertificate",
- config: Config{
- MaxVersion: VersionTLS12,
- Bugs: ProtocolBugs{
- SkipClientCertificate: true,
- },
- },
- // Setting SSL_VERIFY_PEER allows anonymous clients.
- flags: []string{"-verify-peer"},
- shouldFail: true,
- expectedError: ":UNEXPECTED_MESSAGE:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "SkipClientCertificate-TLS13",
- config: Config{
- MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- SkipClientCertificate: true,
- },
- },
- // Setting SSL_VERIFY_PEER allows anonymous clients.
- flags: []string{"-verify-peer"},
- shouldFail: true,
- expectedError: ":UNEXPECTED_MESSAGE:",
- })
-
// Client auth is only legal in certificate-based ciphers.
testCases = append(testCases, testCase{
testType: clientTest,
diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c
index 2a2fe2f..188898c 100644
--- a/ssl/tls13_both.c
+++ b/ssl/tls13_both.c
@@ -177,6 +177,11 @@
goto err;
}
+ /* 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. */
ret = 1;
goto err;
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 1f4475e..c498407 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -490,6 +490,11 @@
static enum ssl_hs_wait_t do_process_client_certificate(SSL *ssl,
SSL_HANDSHAKE *hs) {
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. */
hs->state = state_process_client_finished;
return ssl_hs_ok;
