From 1deb41bb2de8c4c99842b2868772b1931b80d315 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 9 Aug 2016 19:36:38 -0400 Subject: [PATCH] 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 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/handshake_client.c | 2 -- ssl/handshake_server.c | 33 +++++++++++++++++++- ssl/internal.h | 7 +++-- ssl/ssl_lib.c | 28 +---------------- ssl/t1_lib.c | 69 ++++++++++++++++++++---------------------- ssl/tls13_server.c | 9 +----- 6 files changed, 70 insertions(+), 78 deletions(-) diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index d760d106..51d816ed 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -621,8 +621,6 @@ static int ssl_write_client_cipher_list(SSL *ssl, CBB *out, 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 d10d7f53..43abf39f 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c @@ -530,6 +530,26 @@ end: 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 @@ static int ssl3_get_client_hello(SSL *ssl) { 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 @@ static int ssl3_get_client_hello(SSL *ssl) { 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 77759e46..827e5fdc 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -967,9 +967,10 @@ int ssl_early_callback_get_extension(const struct ssl_early_callback_ctx *ctx, 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 04477e59..1872f57b 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -1630,14 +1630,10 @@ int SSL_set_cipher_list(SSL *ssl, const char *str) { } 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 @@ STACK_OF(SSL_CIPHER) * 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 64b2dacf..62f3824d 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -893,25 +893,11 @@ static int ext_ri_parse_clienthello(SSL *ssl, uint8_t *out_alert, 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 @@ static int ext_supported_groups_add_serverhello(SSL *ssl, CBB *out) { /* 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 @@ err: 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 @@ static int ssl_scan_clienthello_tlsext( 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 @@ static int ssl_scan_clienthello_tlsext( } } - 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 @@ static int ssl_scan_serverhello_tlsext(SSL *ssl, CBS *cbs, int *out_alert) { 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 96ceb793..5964233b 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c @@ -166,14 +166,7 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { } } - 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; }