Move SCSV handling out of cipher list parsing.
It's odd that a function like ssl_bytes_to_cipher_list secretly has side
effects all over the place. This removes the need for the TLS 1.3 code
to re-query the version range, and it removes the requirement that the
RI extension be first.
Change-Id: Ic9af549db3aaa8880f3c591b8a13ba9ae91d6a46
Reviewed-on: https://boringssl-review.googlesource.com/10220
Reviewed-by: Adam Langley <agl@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_client.c b/ssl/handshake_client.c
index d760d10..51d816e 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -621,8 +621,6 @@
if (!CBB_add_u16(&child, SSL3_CK_SCSV & 0xffff)) {
return 0;
}
- /* The renegotiation extension is required to be at index zero. */
- ssl->s3->tmp.extensions.sent |= (1u << 0);
}
if ((ssl->mode & SSL_MODE_SEND_FALLBACK_SCSV) ||
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index d10d7f5..43abf39 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -530,6 +530,26 @@
return ret;
}
+int ssl_client_cipher_list_contains_cipher(
+ const struct ssl_early_callback_ctx *client_hello, uint16_t id) {
+ CBS cipher_suites;
+ CBS_init(&cipher_suites, client_hello->cipher_suites,
+ client_hello->cipher_suites_len);
+
+ while (CBS_len(&cipher_suites) > 0) {
+ uint16_t got_id;
+ if (!CBS_get_u16(&cipher_suites, &got_id)) {
+ return 0;
+ }
+
+ if (got_id == id) {
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
static int ssl3_get_client_hello(SSL *ssl) {
int al = SSL_AD_INTERNAL_ERROR, ret = -1;
const SSL_CIPHER *c;
@@ -600,11 +620,22 @@
if (version > max_version) {
version = max_version;
}
+
if (version < min_version) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL);
al = SSL_AD_PROTOCOL_VERSION;
goto f_err;
}
+
+ /* Handle FALLBACK_SCSV. */
+ if (ssl_client_cipher_list_contains_cipher(
+ &client_hello, SSL3_CK_FALLBACK_SCSV & 0xffff) &&
+ version < max_version) {
+ al = SSL3_AD_INAPPROPRIATE_FALLBACK;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INAPPROPRIATE_FALLBACK);
+ goto f_err;
+ }
+
ssl->version = ssl->method->version_to_wire(version);
ssl->s3->enc_method = ssl3_get_enc_method(version);
assert(ssl->s3->enc_method != NULL);
@@ -698,7 +729,7 @@
goto f_err;
}
- ciphers = ssl_parse_client_cipher_list(ssl, &client_hello, max_version);
+ ciphers = ssl_parse_client_cipher_list(&client_hello);
if (ciphers == NULL) {
goto err;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 77759e4..827e5fd 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -967,9 +967,10 @@
CBS *out, uint16_t extension_type);
STACK_OF(SSL_CIPHER) *
- ssl_parse_client_cipher_list(SSL *ssl,
- const struct ssl_early_callback_ctx *ctx,
- uint16_t max_version);
+ ssl_parse_client_cipher_list(const struct ssl_early_callback_ctx *ctx);
+
+int ssl_client_cipher_list_contains_cipher(
+ const struct ssl_early_callback_ctx *client_hello, uint16_t id);
/* Underdocumented functions.
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 04477e5..1872f57 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1630,14 +1630,10 @@
}
STACK_OF(SSL_CIPHER) *
- ssl_parse_client_cipher_list(SSL *ssl,
- const struct ssl_early_callback_ctx *ctx,
- uint16_t max_version) {
+ ssl_parse_client_cipher_list(const struct ssl_early_callback_ctx *ctx) {
CBS cipher_suites;
CBS_init(&cipher_suites, ctx->cipher_suites, ctx->cipher_suites_len);
- ssl->s3->send_connection_binding = 0;
-
STACK_OF(SSL_CIPHER) *sk = sk_SSL_CIPHER_new_null();
if (sk == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
@@ -1652,28 +1648,6 @@
goto err;
}
- /* Check for SCSV. */
- if (ssl->s3 && cipher_suite == (SSL3_CK_SCSV & 0xffff)) {
- /* SCSV is fatal if renegotiating. */
- if (ssl->s3->initial_handshake_complete) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
- goto err;
- }
- ssl->s3->send_connection_binding = 1;
- continue;
- }
-
- /* Check for FALLBACK_SCSV. */
- if (ssl->s3 && cipher_suite == (SSL3_CK_FALLBACK_SCSV & 0xffff)) {
- if (ssl3_protocol_version(ssl) < max_version) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_INAPPROPRIATE_FALLBACK);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL3_AD_INAPPROPRIATE_FALLBACK);
- goto err;
- }
- continue;
- }
-
const SSL_CIPHER *c = SSL_get_cipher_by_value(cipher_suite);
if (c != NULL && !sk_SSL_CIPHER_push(sk, c)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 64b2dac..62f3824 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -893,25 +893,11 @@
return 1;
}
- CBS fake_contents;
- static const uint8_t kFakeExtension[] = {0};
-
if (contents == NULL) {
- if (ssl->s3->send_connection_binding) {
- /* The renegotiation SCSV was received so pretend that we received a
- * renegotiation extension. */
- CBS_init(&fake_contents, kFakeExtension, sizeof(kFakeExtension));
- contents = &fake_contents;
- /* We require that the renegotiation extension is at index zero of
- * kExtensions. */
- ssl->s3->tmp.extensions.received |= (1u << 0);
- } else {
- return 1;
- }
+ return 1;
}
CBS renegotiated_connection;
-
if (!CBS_get_u8_length_prefixed(contents, &renegotiated_connection) ||
CBS_len(contents) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_RENEGOTIATION_ENCODING_ERR);
@@ -2236,9 +2222,6 @@
/* kExtensions contains all the supported extensions. */
static const struct tls_extension kExtensions[] = {
{
- /* The renegotiation extension must always be at index zero because the
- * |received| and |sent| bitsets need to be tweaked when the "extension" is
- * sent as an SCSV. */
TLSEXT_TYPE_renegotiate,
NULL,
ext_ri_add_clienthello,
@@ -2513,8 +2496,7 @@
static int ssl_scan_clienthello_tlsext(
SSL *ssl, const struct ssl_early_callback_ctx *client_hello,
int *out_alert) {
- size_t i;
- for (i = 0; i < kNumExtensions; i++) {
+ for (size_t i = 0; i < kNumExtensions; i++) {
if (kExtensions[i].init != NULL) {
kExtensions[i].init(ssl);
}
@@ -2522,10 +2504,6 @@
ssl->s3->tmp.extensions.received = 0;
ssl->s3->tmp.custom_extensions.received = 0;
- /* The renegotiation extension must always be at index zero because the
- * |received| and |sent| bitsets need to be tweaked when the "extension" is
- * sent as an SCSV. */
- assert(kExtensions[0].value == TLSEXT_TYPE_renegotiate);
CBS extensions;
CBS_init(&extensions, client_hello->extensions, client_hello->extensions_len);
@@ -2568,17 +2546,32 @@
}
}
- for (i = 0; i < kNumExtensions; i++) {
- if (!(ssl->s3->tmp.extensions.received & (1u << i))) {
- /* Extension wasn't observed so call the callback with a NULL
- * parameter. */
- uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!kExtensions[i].parse_clienthello(ssl, &alert, NULL)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
- ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
- *out_alert = alert;
- return 0;
- }
+ for (size_t i = 0; i < kNumExtensions; i++) {
+ if (ssl->s3->tmp.extensions.received & (1u << i)) {
+ continue;
+ }
+
+ CBS *contents = NULL, fake_contents;
+ static const uint8_t kFakeRenegotiateExtension[] = {0};
+ if (kExtensions[i].value == TLSEXT_TYPE_renegotiate &&
+ ssl_client_cipher_list_contains_cipher(client_hello,
+ SSL3_CK_SCSV & 0xffff)) {
+ /* The renegotiation SCSV was received so pretend that we received a
+ * renegotiation extension. */
+ CBS_init(&fake_contents, kFakeRenegotiateExtension,
+ sizeof(kFakeRenegotiateExtension));
+ contents = &fake_contents;
+ ssl->s3->tmp.extensions.received |= (1u << i);
+ }
+
+ /* Extension wasn't observed so call the callback with a NULL
+ * parameter. */
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ if (!kExtensions[i].parse_clienthello(ssl, &alert, contents)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
+ ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
+ *out_alert = alert;
+ return 0;
}
}
@@ -2640,8 +2633,10 @@
continue;
}
- if (!(ssl->s3->tmp.extensions.sent & (1u << ext_index))) {
- /* If the extension was never sent then it is illegal. */
+ if (!(ssl->s3->tmp.extensions.sent & (1u << ext_index)) &&
+ type != TLSEXT_TYPE_renegotiate) {
+ /* If the extension was never sent then it is illegal, except for the
+ * renegotiation extension which, in SSL 3.0, is signaled via SCSV. */
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION);
ERR_add_error_dataf("extension :%u", (unsigned)type);
*out_alert = SSL_AD_UNSUPPORTED_EXTENSION;
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 96ceb79..5964233 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -166,14 +166,7 @@
}
}
- uint16_t min_version, max_version;
- if (!ssl_get_version_range(ssl, &min_version, &max_version)) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- return ssl_hs_error;
- }
-
- STACK_OF(SSL_CIPHER) *ciphers =
- ssl_parse_client_cipher_list(ssl, &client_hello, max_version);
+ STACK_OF(SSL_CIPHER) *ciphers = ssl_parse_client_cipher_list(&client_hello);
if (ciphers == NULL) {
return ssl_hs_error;
}
