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;