Client auth is only legal in certificate-based ciphers.
OpenSSL used to only forbid it on the server in plain PSK and allow it on the
client. Enforce it properly on both sides. My read of the rule in RFC 5246 ("A
non-anonymous server can optionally request a certificate") and in RFC 4279
("The Certificate and CertificateRequest payloads are omitted from the
response.") is that client auth happens iff we're certificate-based.
The line in RFC 4279 is under the plain PSK section, but that doesn't make a
whole lot of sense and there is only one diagram. PSK already authenticates
both sides. I think the most plausible interpretation is that this is for
certificate-based ciphers.
Change-Id: If195232c83f21e011e25318178bb45186de707e6
Reviewed-on: https://boringssl-review.googlesource.com/7942
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 8a93100..3088a7e 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -236,7 +236,7 @@
case SSL3_ST_CR_CERT_A:
case SSL3_ST_CR_CERT_B:
- if (ssl_cipher_has_server_public_key(ssl->s3->tmp.new_cipher)) {
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ret = ssl3_get_server_certificate(ssl);
if (ret <= 0) {
goto end;
@@ -269,7 +269,11 @@
if (ret <= 0) {
goto end;
}
- ssl->state = SSL3_ST_CR_CERT_REQ_A;
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
+ ssl->state = SSL3_ST_CR_CERT_REQ_A;
+ } else {
+ ssl->state = SSL3_ST_CR_SRVR_DONE_A;
+ }
ssl->init_num = 0;
break;
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index 427fe23..f9268a2 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -217,7 +217,7 @@
case SSL3_ST_SW_CERT_A:
case SSL3_ST_SW_CERT_B:
- if (ssl_cipher_has_server_public_key(ssl->s3->tmp.new_cipher)) {
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
dtls1_start_timer(ssl);
ret = ssl3_send_server_certificate(ssl);
if (ret <= 0) {
diff --git a/ssl/internal.h b/ssl/internal.h
index ddd95b1..99d7f0f 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -174,6 +174,8 @@
/* SSL_aPSK is set for both PSK and ECDHE_PSK. */
#define SSL_aPSK 0x00000004L
+#define SSL_aCERT (SSL_aRSA | SSL_aECDSA)
+
/* Bits for |algorithm_enc| (symmetric encryption). */
#define SSL_3DES 0x00000001L
#define SSL_RC4 0x00000002L
@@ -239,17 +241,16 @@
* server key used in |cipher| or |EVP_PKEY_NONE| if there is none. */
int ssl_cipher_get_key_type(const SSL_CIPHER *cipher);
-/* ssl_cipher_has_server_public_key returns 1 if |cipher| involves a server
- * public key in the key exchange, sent in a server Certificate message.
- * Otherwise it returns 0. */
-int ssl_cipher_has_server_public_key(const SSL_CIPHER *cipher);
+/* ssl_cipher_uses_certificate_auth returns one if |cipher| authenticates the
+ * server and, optionally, the client with a certificate. Otherwise it returns
+ * zero. */
+int ssl_cipher_uses_certificate_auth(const SSL_CIPHER *cipher);
/* ssl_cipher_requires_server_key_exchange returns 1 if |cipher| requires a
* ServerKeyExchange message. Otherwise it returns 0.
*
- * Unlike |ssl_cipher_has_server_public_key|, this function may return zero
- * while still allowing |cipher| an optional ServerKeyExchange. This is the
- * case for plain PSK ciphers. */
+ * This function may return zero while still allowing |cipher| an optional
+ * ServerKeyExchange. This is the case for plain PSK ciphers. */
int ssl_cipher_requires_server_key_exchange(const SSL_CIPHER *cipher);
/* ssl_cipher_get_record_split_len, for TLS 1.0 CBC mode ciphers, returns the
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 3681590..8e79c81 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -256,7 +256,7 @@
case SSL3_ST_CR_CERT_A:
case SSL3_ST_CR_CERT_B:
- if (ssl_cipher_has_server_public_key(ssl->s3->tmp.new_cipher)) {
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ret = ssl3_get_server_certificate(ssl);
if (ret <= 0) {
goto end;
@@ -289,7 +289,11 @@
if (ret <= 0) {
goto end;
}
- ssl->state = SSL3_ST_CR_CERT_REQ_A;
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
+ ssl->state = SSL3_ST_CR_CERT_REQ_A;
+ } else {
+ ssl->state = SSL3_ST_CR_SRVR_DONE_A;
+ }
ssl->init_num = 0;
break;
@@ -857,7 +861,8 @@
/* If doing a full handshake with TLS 1.2, the server may request a client
* certificate which requires hashing the handshake transcript under a
* different hash. Otherwise, the handshake buffer may be released. */
- if (ssl->hit || ssl3_protocol_version(ssl) < TLS1_2_VERSION) {
+ if (ssl->hit || ssl3_protocol_version(ssl) < TLS1_2_VERSION ||
+ !ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ssl3_free_handshake_buffer(ssl);
}
@@ -1203,7 +1208,7 @@
CBS_len(&server_key_exchange_orig) - CBS_len(&server_key_exchange));
/* ServerKeyExchange should be signed by the server's public key. */
- if (ssl_cipher_has_server_public_key(ssl->s3->tmp.new_cipher)) {
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
pkey = X509_get_pubkey(ssl->session->peer);
if (pkey == NULL) {
goto err;
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 9fec49c..4b14d65 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -286,7 +286,7 @@
case SSL3_ST_SW_CERT_A:
case SSL3_ST_SW_CERT_B:
- if (ssl_cipher_has_server_public_key(ssl->s3->tmp.new_cipher)) {
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ret = ssl3_send_server_certificate(ssl);
if (ret <= 0) {
goto end;
@@ -1051,8 +1051,8 @@
ssl->s3->tlsext_channel_id_valid) {
ssl->s3->tmp.cert_request = 0;
}
- /* Plain PSK forbids Certificate and CertificateRequest. */
- if (ssl->s3->tmp.new_cipher->algorithm_mkey & SSL_kPSK) {
+ /* CertificateRequest may only be sent in certificate-based ciphers. */
+ if (!ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ssl->s3->tmp.cert_request = 0;
}
} else {
@@ -1270,7 +1270,7 @@
}
/* Add a signature. */
- if (ssl_cipher_has_server_public_key(ssl->s3->tmp.new_cipher)) {
+ if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
if (!ssl_has_private_key(ssl)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
goto err;
diff --git a/ssl/ssl_cipher.c b/ssl/ssl_cipher.c
index 58ce582..7fb8809 100644
--- a/ssl/ssl_cipher.c
+++ b/ssl/ssl_cipher.c
@@ -1957,15 +1957,8 @@
return EVP_PKEY_NONE;
}
-int ssl_cipher_has_server_public_key(const SSL_CIPHER *cipher) {
- /* PSK-authenticated ciphers do not use a certificate. (RSA_PSK is not
- * supported.) */
- if (cipher->algorithm_auth & SSL_aPSK) {
- return 0;
- }
-
- /* All other ciphers include it. */
- return 1;
+int ssl_cipher_uses_certificate_auth(const SSL_CIPHER *cipher) {
+ return (cipher->algorithm_auth & SSL_aCERT) != 0;
}
int ssl_cipher_requires_server_key_exchange(const SSL_CIPHER *cipher) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 11b75e4..f9cf3d6 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2714,6 +2714,40 @@
shouldFail: true,
expectedError: ":UNEXPECTED_MESSAGE:",
})
+
+ // Client auth is only legal in certificate-based ciphers.
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "ClientAuth-PSK",
+ config: Config{
+ CipherSuites: []uint16{TLS_PSK_WITH_AES_128_CBC_SHA},
+ PreSharedKey: []byte("secret"),
+ ClientAuth: RequireAnyClientCert,
+ },
+ flags: []string{
+ "-cert-file", path.Join(*resourceDir, rsaCertificateFile),
+ "-key-file", path.Join(*resourceDir, rsaKeyFile),
+ "-psk", "secret",
+ },
+ shouldFail: true,
+ expectedError: ":UNEXPECTED_MESSAGE:",
+ })
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "ClientAuth-ECDHE_PSK",
+ config: Config{
+ CipherSuites: []uint16{TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA},
+ PreSharedKey: []byte("secret"),
+ ClientAuth: RequireAnyClientCert,
+ },
+ flags: []string{
+ "-cert-file", path.Join(*resourceDir, rsaCertificateFile),
+ "-key-file", path.Join(*resourceDir, rsaKeyFile),
+ "-psk", "secret",
+ },
+ shouldFail: true,
+ expectedError: ":UNEXPECTED_MESSAGE:",
+ })
}
func addExtendedMasterSecretTests() {
