Make ssl_parse_extensions a little easier to use. std::initializer_list appears to work by instantiating a T[N] at the call site (which is what we were doing anyway), so I don't believe there is a runtime dependency. This also adds a way for individual entries to turn themselves off, which means we don't need to manually check for some unsolicited extensions. Change-Id: I40f79b6a0e9c005fc621f4a798fe201bfbf08411 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48910 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake.cc b/ssl/handshake.cc index db4ee71..3608888 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc
@@ -268,12 +268,15 @@ } bool ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, - Span<const SSL_EXTENSION_TYPE> ext_types, + std::initializer_list<SSLExtension *> extensions, bool ignore_unknown) { // Reset everything. - for (const SSL_EXTENSION_TYPE &ext_type : ext_types) { - *ext_type.out_present = false; - CBS_init(ext_type.out_data, nullptr, 0); + for (SSLExtension *ext : extensions) { + ext->present = false; + CBS_init(&ext->data, nullptr, 0); + if (!ext->allowed) { + assert(!ignore_unknown); + } } CBS copy = *cbs; @@ -287,10 +290,10 @@ return false; } - const SSL_EXTENSION_TYPE *found = nullptr; - for (const SSL_EXTENSION_TYPE &ext_type : ext_types) { - if (type == ext_type.type) { - found = &ext_type; + SSLExtension *found = nullptr; + for (SSLExtension *ext : extensions) { + if (type == ext->type && ext->allowed) { + found = ext; break; } } @@ -305,14 +308,14 @@ } // Duplicate ext_types are forbidden. - if (*found->out_present) { + if (found->present) { OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION); *out_alert = SSL_AD_ILLEGAL_PARAMETER; return false; } - *found->out_present = 1; - *found->out_data = data; + found->present = true; + found->data = data; } return true;
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index b5199ba..ee9045e 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc
@@ -364,25 +364,20 @@ return true; } - bool have_supported_versions; - CBS supported_versions; - const SSL_EXTENSION_TYPE ext_types[] = { - {TLSEXT_TYPE_supported_versions, &have_supported_versions, - &supported_versions}, - }; + SSLExtension supported_versions(TLSEXT_TYPE_supported_versions); CBS extensions = server_hello.extensions; - if (!ssl_parse_extensions(&extensions, out_alert, ext_types, + if (!ssl_parse_extensions(&extensions, out_alert, {&supported_versions}, /*ignore_unknown=*/true)) { return false; } - if (!have_supported_versions) { + if (!supported_versions.present) { *out_version = server_hello.legacy_version; return true; } - if (!CBS_get_u16(&supported_versions, out_version) || - CBS_len(&supported_versions) != 0) { + if (!CBS_get_u16(&supported_versions.data, out_version) || + CBS_len(&supported_versions.data) != 0) { *out_alert = SSL_AD_DECODE_ERROR; return false; }
diff --git a/ssl/internal.h b/ssl/internal.h index 0c91724..f9cee53 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -146,6 +146,7 @@ #include <stdlib.h> +#include <initializer_list> #include <limits> #include <new> #include <type_traits> @@ -2219,19 +2220,25 @@ bool ssl_negotiate_alps(SSL_HANDSHAKE *hs, uint8_t *out_alert, const SSL_CLIENT_HELLO *client_hello); -struct SSL_EXTENSION_TYPE { +struct SSLExtension { + SSLExtension(uint16_t type_arg, bool allowed_arg = true) + : type(type_arg), allowed(allowed_arg), present(false) { + CBS_init(&data, nullptr, 0); + } + uint16_t type; - bool *out_present; - CBS *out_data; + bool allowed; + bool present; + CBS data; }; // ssl_parse_extensions parses a TLS extensions block out of |cbs| and advances -// it. It writes the parsed extensions to pointers denoted by |ext_types|. On -// success, it fills in the |out_present| and |out_data| fields and returns -// true. Otherwise, it sets |*out_alert| to an alert to send and returns false. -// Unknown extensions are rejected unless |ignore_unknown| is true. +// it. It writes the parsed extensions to pointers in |extensions|. On success, +// it fills in the |present| and |data| fields and returns true. Otherwise, it +// sets |*out_alert| to an alert to send and returns false. Unknown extensions +// are rejected unless |ignore_unknown| is true. bool ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, - Span<const SSL_EXTENSION_TYPE> ext_types, + std::initializer_list<SSLExtension *> extensions, bool ignore_unknown); // ssl_verify_peer_cert verifies the peer certificate for |hs|.
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index 0354f39..226c67b 100644 --- a/ssl/tls13_both.cc +++ b/ssl/tls13_both.cc
@@ -235,15 +235,14 @@ } // Parse out the extensions. - bool have_status_request = false, have_sct = false; - CBS status_request, sct; - const SSL_EXTENSION_TYPE ext_types[] = { - {TLSEXT_TYPE_status_request, &have_status_request, &status_request}, - {TLSEXT_TYPE_certificate_timestamp, &have_sct, &sct}, - }; - + SSLExtension status_request( + TLSEXT_TYPE_status_request, + !ssl->server && hs->config->ocsp_stapling_enabled); + SSLExtension sct( + TLSEXT_TYPE_certificate_timestamp, + !ssl->server && hs->config->signed_cert_timestamps_enabled); uint8_t alert = SSL_AD_DECODE_ERROR; - if (!ssl_parse_extensions(&extensions, &alert, ext_types, + if (!ssl_parse_extensions(&extensions, &alert, {&status_request, &sct}, /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return false; @@ -251,20 +250,14 @@ // All Certificate extensions are parsed, but only the leaf extensions are // stored. - if (have_status_request) { - if (ssl->server || !hs->config->ocsp_stapling_enabled) { - OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION); - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION); - return false; - } - + if (status_request.present) { uint8_t status_type; CBS ocsp_response; - if (!CBS_get_u8(&status_request, &status_type) || + if (!CBS_get_u8(&status_request.data, &status_type) || status_type != TLSEXT_STATUSTYPE_ocsp || - !CBS_get_u24_length_prefixed(&status_request, &ocsp_response) || + !CBS_get_u24_length_prefixed(&status_request.data, &ocsp_response) || CBS_len(&ocsp_response) == 0 || - CBS_len(&status_request) != 0) { + CBS_len(&status_request.data) != 0) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); return false; } @@ -279,14 +272,8 @@ } } - if (have_sct) { - if (ssl->server || !hs->config->signed_cert_timestamps_enabled) { - OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION); - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION); - return false; - } - - if (!ssl_is_sct_list_valid(&sct)) { + if (sct.present) { + if (!ssl_is_sct_list_valid(&sct.data)) { OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_PARSING_EXTENSION); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); return false; @@ -294,7 +281,7 @@ if (sk_CRYPTO_BUFFER_num(certs.get()) == 1) { hs->new_session->signed_cert_timestamp_list.reset( - CRYPTO_BUFFER_new_from_CBS(&sct, ssl->ctx->pool)); + CRYPTO_BUFFER_new_from_CBS(&sct.data, ssl->ctx->pool)); if (hs->new_session->signed_cert_timestamp_list == nullptr) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); return false;
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 7a19b2a..bd7e63f 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc
@@ -162,31 +162,25 @@ return ssl_hs_ok; } - bool have_cookie, have_key_share, have_supported_versions; - CBS cookie, key_share, supported_versions; - SSL_EXTENSION_TYPE ext_types[] = { - {TLSEXT_TYPE_key_share, &have_key_share, &key_share}, - {TLSEXT_TYPE_cookie, &have_cookie, &cookie}, - {TLSEXT_TYPE_supported_versions, &have_supported_versions, - &supported_versions}, - }; - - if (!ssl_parse_extensions(&server_hello.extensions, &alert, ext_types, + SSLExtension cookie(TLSEXT_TYPE_cookie), key_share(TLSEXT_TYPE_key_share), + supported_versions(TLSEXT_TYPE_supported_versions); + if (!ssl_parse_extensions(&server_hello.extensions, &alert, + {&cookie, &key_share, &supported_versions}, /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } - if (!have_cookie && !have_key_share) { + if (!cookie.present && !key_share.present) { OPENSSL_PUT_ERROR(SSL, SSL_R_EMPTY_HELLO_RETRY_REQUEST); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); return ssl_hs_error; } - if (have_cookie) { + if (cookie.present) { CBS cookie_value; - if (!CBS_get_u16_length_prefixed(&cookie, &cookie_value) || + if (!CBS_get_u16_length_prefixed(&cookie.data, &cookie_value) || CBS_len(&cookie_value) == 0 || - CBS_len(&cookie) != 0) { + CBS_len(&cookie.data) != 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); return ssl_hs_error; @@ -197,9 +191,10 @@ } } - if (have_key_share) { + if (key_share.present) { uint16_t group_id; - if (!CBS_get_u16(&key_share, &group_id) || CBS_len(&key_share) != 0) { + if (!CBS_get_u16(&key_share.data, &group_id) || + CBS_len(&key_share.data) != 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); return ssl_hs_error; @@ -316,18 +311,11 @@ OPENSSL_memcpy(ssl->s3->server_random, CBS_data(&server_hello.random), SSL3_RANDOM_SIZE); - // Parse out the extensions. - bool have_key_share = false, have_pre_shared_key = false, - have_supported_versions = false; - CBS key_share, pre_shared_key, supported_versions; - SSL_EXTENSION_TYPE ext_types[] = { - {TLSEXT_TYPE_key_share, &have_key_share, &key_share}, - {TLSEXT_TYPE_pre_shared_key, &have_pre_shared_key, &pre_shared_key}, - {TLSEXT_TYPE_supported_versions, &have_supported_versions, - &supported_versions}, - }; - - if (!ssl_parse_extensions(&server_hello.extensions, &alert, ext_types, + SSLExtension key_share(TLSEXT_TYPE_key_share), + pre_shared_key(TLSEXT_TYPE_pre_shared_key, ssl->session != nullptr), + supported_versions(TLSEXT_TYPE_supported_versions); + if (!ssl_parse_extensions(&server_hello.extensions, &alert, + {&key_share, &pre_shared_key, &supported_versions}, /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; @@ -335,8 +323,9 @@ // Recheck supported_versions, in case this is after HelloRetryRequest. uint16_t version; - if (!have_supported_versions || - !CBS_get_u16(&supported_versions, &version) || + if (!supported_versions.present || + !CBS_get_u16(&supported_versions.data, &version) || + CBS_len(&supported_versions.data) != 0 || version != ssl->version) { OPENSSL_PUT_ERROR(SSL, SSL_R_SECOND_SERVERHELLO_VERSION_MISMATCH); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); @@ -344,15 +333,9 @@ } alert = SSL_AD_DECODE_ERROR; - if (have_pre_shared_key) { - if (ssl->session == NULL) { - OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION); - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION); - return ssl_hs_error; - } - + if (pre_shared_key.present) { if (!ssl_ext_pre_shared_key_parse_serverhello(hs, &alert, - &pre_shared_key)) { + &pre_shared_key.data)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } @@ -409,7 +392,7 @@ return ssl_hs_error; } - if (!have_key_share) { + if (!key_share.present) { // We do not support psk_ke and thus always require a key share. OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_KEY_SHARE); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_MISSING_EXTENSION); @@ -420,7 +403,7 @@ Array<uint8_t> dhe_secret; alert = SSL_AD_DECODE_ERROR; if (!ssl_ext_key_share_parse_serverhello(hs, &dhe_secret, &alert, - &key_share)) { + &key_share.data)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } @@ -456,7 +439,7 @@ SSL3_RANDOM_SIZE); } else { // Resuming against the ClientHelloOuter was an unsolicited extension. - if (have_pre_shared_key) { + if (pre_shared_key.present) { OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION); return ssl_hs_error; @@ -600,25 +583,19 @@ } - bool have_sigalgs = false, have_ca = false; - CBS sigalgs, ca; - const SSL_EXTENSION_TYPE ext_types[] = { - {TLSEXT_TYPE_signature_algorithms, &have_sigalgs, &sigalgs}, - {TLSEXT_TYPE_certificate_authorities, &have_ca, &ca}, - }; - + SSLExtension sigalgs(TLSEXT_TYPE_signature_algorithms), + ca(TLSEXT_TYPE_certificate_authorities); CBS body = msg.body, context, extensions, supported_signature_algorithms; uint8_t alert = SSL_AD_DECODE_ERROR; if (!CBS_get_u8_length_prefixed(&body, &context) || // The request context is always empty during the handshake. CBS_len(&context) != 0 || - !CBS_get_u16_length_prefixed(&body, &extensions) || + !CBS_get_u16_length_prefixed(&body, &extensions) || // CBS_len(&body) != 0 || - !ssl_parse_extensions(&extensions, &alert, ext_types, + !ssl_parse_extensions(&extensions, &alert, {&sigalgs, &ca}, /*ignore_unknown=*/true) || - (have_ca && CBS_len(&ca) == 0) || - !have_sigalgs || - !CBS_get_u16_length_prefixed(&sigalgs, + !sigalgs.present || + !CBS_get_u16_length_prefixed(&sigalgs.data, &supported_signature_algorithms) || !tls1_parse_peer_sigalgs(hs, &supported_signature_algorithms)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); @@ -626,8 +603,8 @@ return ssl_hs_error; } - if (have_ca) { - hs->ca_names = ssl_parse_client_CA_list(ssl, &alert, &ca); + if (ca.present) { + hs->ca_names = ssl_parse_client_CA_list(ssl, &alert, &ca.data); if (!hs->ca_names) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; @@ -1050,23 +1027,17 @@ return nullptr; } - // Parse out the extensions. - bool have_early_data = false; - CBS early_data; - const SSL_EXTENSION_TYPE ext_types[] = { - {TLSEXT_TYPE_early_data, &have_early_data, &early_data}, - }; - + SSLExtension early_data(TLSEXT_TYPE_early_data); uint8_t alert = SSL_AD_DECODE_ERROR; - if (!ssl_parse_extensions(&extensions, &alert, ext_types, + if (!ssl_parse_extensions(&extensions, &alert, {&early_data}, /*ignore_unknown=*/true)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return nullptr; } - if (have_early_data) { - if (!CBS_get_u32(&early_data, &session->ticket_max_early_data) || - CBS_len(&early_data) != 0) { + if (early_data.present) { + if (!CBS_get_u32(&early_data.data, &session->ticket_max_early_data) || + CBS_len(&early_data.data) != 0) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); return nullptr;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index f1a62b2..6509cc4 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -1040,20 +1040,15 @@ return ssl_hs_error; } - // Parse out the extensions. - bool have_application_settings = false; - CBS application_settings; - SSL_EXTENSION_TYPE ext_types[] = {{TLSEXT_TYPE_application_settings, - &have_application_settings, - &application_settings}}; + SSLExtension application_settings(TLSEXT_TYPE_application_settings); uint8_t alert = SSL_AD_DECODE_ERROR; - if (!ssl_parse_extensions(&extensions, &alert, ext_types, + if (!ssl_parse_extensions(&extensions, &alert, {&application_settings}, /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } - if (!have_application_settings) { + if (!application_settings.present) { OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_MISSING_EXTENSION); return ssl_hs_error; @@ -1062,7 +1057,7 @@ // Note that, if 0-RTT was accepted, these values will already have been // initialized earlier. if (!hs->new_session->peer_application_settings.CopyFrom( - application_settings) || + application_settings.data) || !ssl_hash_message(hs, msg)) { ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); return ssl_hs_error;