From c4ec14c71dfa6e59d7d9055daaae887ce0f6b5c2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 21 Sep 2020 18:42:52 -0400 Subject: [PATCH] Switch ssl_parse_extensions to bool and Span. This function is still a bit too C-like, but this is slightly better. Change-Id: Id8931753c9b8a2445d12089af5391833a68c4901 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43004 Reviewed-by: David Benjamin Commit-Queue: David Benjamin --- ssl/handshake.cc | 36 ++++++++++++++++++------------------ ssl/handshake_client.cc | 3 +-- ssl/internal.h | 12 ++++++------ ssl/tls13_both.cc | 3 +-- ssl/tls13_client.cc | 12 ++++-------- 5 files changed, 30 insertions(+), 36 deletions(-) diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 7bceb1dcb..66d108e79 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc @@ -235,13 +235,13 @@ bool ssl_hash_message(SSL_HANDSHAKE *hs, const SSLMessage &msg) { return hs->transcript.Update(msg.raw); } -int ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, - const SSL_EXTENSION_TYPE *ext_types, - size_t num_ext_types, int ignore_unknown) { +bool ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, + Span ext_types, + bool ignore_unknown) { // Reset everything. - for (size_t i = 0; i < num_ext_types; i++) { - *ext_types[i].out_present = 0; - CBS_init(ext_types[i].out_data, NULL, 0); + for (const SSL_EXTENSION_TYPE &ext_type : ext_types) { + *ext_type.out_present = false; + CBS_init(ext_type.out_data, nullptr, 0); } CBS copy = *cbs; @@ -252,38 +252,38 @@ int ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, !CBS_get_u16_length_prefixed(©, &data)) { OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT); *out_alert = SSL_AD_DECODE_ERROR; - return 0; + return false; } - const SSL_EXTENSION_TYPE *ext_type = NULL; - for (size_t i = 0; i < num_ext_types; i++) { - if (type == ext_types[i].type) { - ext_type = &ext_types[i]; + const SSL_EXTENSION_TYPE *found = nullptr; + for (const SSL_EXTENSION_TYPE &ext_type : ext_types) { + if (type == ext_type.type) { + found = &ext_type; break; } } - if (ext_type == NULL) { + if (found == nullptr) { if (ignore_unknown) { continue; } OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION); *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; - return 0; + return false; } // Duplicate ext_types are forbidden. - if (*ext_type->out_present) { + if (*found->out_present) { OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_EXTENSION); *out_alert = SSL_AD_ILLEGAL_PARAMETER; - return 0; + return false; } - *ext_type->out_present = 1; - *ext_type->out_data = data; + *found->out_present = 1; + *found->out_data = data; } - return 1; + return true; } enum ssl_verify_result_t ssl_verify_peer_cert(SSL_HANDSHAKE *hs) { diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 1b7bb1b58..5c800fb5f 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -358,8 +358,7 @@ static bool parse_supported_versions(SSL_HANDSHAKE *hs, uint16_t *version, uint8_t alert = SSL_AD_DECODE_ERROR; if (!ssl_parse_extensions(&extensions, &alert, ext_types, - OPENSSL_ARRAY_SIZE(ext_types), - 1 /* ignore unknown */)) { + /*ignore_unknown=*/true)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return false; } diff --git a/ssl/internal.h b/ssl/internal.h index ffc18a52d..9dd206e37 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1926,12 +1926,12 @@ struct SSL_EXTENSION_TYPE { // 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 one. -// Otherwise, it sets |*out_alert| to an alert to send and returns zero. Unknown -// extensions are rejected unless |ignore_unknown| is 1. -int ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, - const SSL_EXTENSION_TYPE *ext_types, - size_t num_ext_types, int ignore_unknown); +// 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. +bool ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert, + Span ext_types, + bool ignore_unknown); // ssl_verify_peer_cert verifies the peer certificate for |hs|. enum ssl_verify_result_t ssl_verify_peer_cert(SSL_HANDSHAKE *hs); diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index 93e2f6a1e..c6bc2b19a 100644 --- a/ssl/tls13_both.cc +++ b/ssl/tls13_both.cc @@ -244,8 +244,7 @@ bool tls13_process_certificate(SSL_HANDSHAKE *hs, const SSLMessage &msg, uint8_t alert = SSL_AD_DECODE_ERROR; if (!ssl_parse_extensions(&extensions, &alert, ext_types, - OPENSSL_ARRAY_SIZE(ext_types), - 0 /* reject unknown */)) { + /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return false; } diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index cb379b0c9..8cadd734f 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -172,8 +172,7 @@ static enum ssl_hs_wait_t do_read_hello_retry_request(SSL_HANDSHAKE *hs) { uint8_t alert = SSL_AD_DECODE_ERROR; if (!ssl_parse_extensions(&extensions, &alert, ext_types, - OPENSSL_ARRAY_SIZE(ext_types), - 0 /* reject unknown */)) { + /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } @@ -338,8 +337,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { uint8_t alert = SSL_AD_DECODE_ERROR; if (!ssl_parse_extensions(&extensions, &alert, ext_types, - OPENSSL_ARRAY_SIZE(ext_types), - 0 /* reject unknown */)) { + /*ignore_unknown=*/false)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } @@ -568,8 +566,7 @@ static enum ssl_hs_wait_t do_read_certificate_request(SSL_HANDSHAKE *hs) { !CBS_get_u16_length_prefixed(&body, &extensions) || CBS_len(&body) != 0 || !ssl_parse_extensions(&extensions, &alert, ext_types, - OPENSSL_ARRAY_SIZE(ext_types), - 1 /* accept unknown */) || + /*ignore_unknown=*/true) || (have_ca && CBS_len(&ca) == 0) || !have_sigalgs || !CBS_get_u16_length_prefixed(&sigalgs, @@ -989,8 +986,7 @@ UniquePtr tls13_create_session_with_ticket(SSL *ssl, CBS *body) { uint8_t alert = SSL_AD_DECODE_ERROR; if (!ssl_parse_extensions(&extensions, &alert, ext_types, - OPENSSL_ARRAY_SIZE(ext_types), - 1 /* ignore unknown */)) { + /*ignore_unknown=*/true)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return nullptr; }