kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Have |SSL_get_verify_result| return |X509_V_OK| when no client certificate is given.

Browse Source 
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>
This commit is contained in:
Adam Langley 2016-08-01 16:16:46 -07:00 committed by CQ bot account: commit-bot@chromium.org
parent 867bcba05d
commit 37646838e9
4 changed files with 103 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
ssl/handshake_server.c
View File

@ -809,6 +809,13 @@ static int ssl3_get_client_hello(SSL *ssl) {
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 @@ static int ssl3_get_client_certificate(SSL *ssl) {
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 @@ static int ssl3_get_client_certificate(SSL *ssl) {
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) {

159
ssl/test/runner/runner.go
View File

@ -2730,90 +2730,83 @@ func addClientAuthTests() {
},
})
}
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{

5
ssl/tls13_both.c
View File

@ -177,6 +177,11 @@ int tls13_process_certificate(SSL *ssl, int allow_anonymous) {
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;

5
ssl/tls13_server.c
View File

@ -490,6 +490,11 @@ static enum ssl_hs_wait_t do_flush(SSL *ssl, SSL_HANDSHAKE *hs) {
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;