Deduplicate our three ServerHello parsers.
We do this enough that it's worth extracting a common parser. And this
gives a struct we can pass around. Note this moves the server extensions
block parsing out of ssl_scan_serverhello_tlsext.
I've also consolidated a few error conditions to tighten the code up a
bit: the TLS 1.2 code distinguishes unknown from unadvertised cipher,
while the TLS 1.3 code didn't. And seeing the wrong legacy version
number in TLS 1.3 is really just a syntax error since it's not the
version field anymore. (RFC8446 specifies the value.)
Change-Id: Ia2f44ff9a3899b5a594569f1b258f2b487930496
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48908
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 9af928c..6fec445 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -3716,18 +3716,10 @@
return true;
}
-static bool ssl_scan_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs,
+static bool ssl_scan_serverhello_tlsext(SSL_HANDSHAKE *hs, const CBS *cbs,
int *out_alert) {
- SSL *const ssl = hs->ssl;
- // Before TLS 1.3, ServerHello extensions blocks may be omitted if empty.
- if (CBS_len(cbs) == 0 && ssl_protocol_version(ssl) < TLS1_3_VERSION) {
- return true;
- }
-
- // Decode the extensions block and check it is valid.
- CBS extensions;
- if (!CBS_get_u16_length_prefixed(cbs, &extensions) ||
- !tls1_check_duplicate_extensions(&extensions)) {
+ CBS extensions = *cbs;
+ if (!tls1_check_duplicate_extensions(&extensions)) {
*out_alert = SSL_AD_DECODE_ERROR;
return false;
}
@@ -3849,7 +3841,7 @@
return true;
}
-bool ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs) {
+bool ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, const CBS *cbs) {
SSL *const ssl = hs->ssl;
int alert = SSL_AD_DECODE_ERROR;
if (!ssl_scan_serverhello_tlsext(hs, cbs, &alert)) {
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index ba8f4b7..b5199ba 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -354,42 +354,36 @@
return ssl->method->add_message(ssl, std::move(msg));
}
-static bool parse_supported_versions(SSL_HANDSHAKE *hs, uint16_t *version,
- const CBS *in) {
- // If the outer version is not TLS 1.2, or there is no extensions block, use
- // the outer version.
- if (*version != TLS1_2_VERSION || CBS_len(in) == 0) {
+static bool parse_server_version(const SSL_HANDSHAKE *hs, uint16_t *out_version,
+ uint8_t *out_alert,
+ const ParsedServerHello &server_hello) {
+ // If the outer version is not TLS 1.2, use it.
+ // TODO(davidben): This function doesn't quite match the RFC8446 formulation.
+ if (server_hello.legacy_version != TLS1_2_VERSION) {
+ *out_version = server_hello.legacy_version;
return true;
}
- SSL *const ssl = hs->ssl;
- CBS copy = *in, extensions;
- if (!CBS_get_u16_length_prefixed(©, &extensions) ||
- CBS_len(©) != 0) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- return false;
- }
-
bool have_supported_versions;
CBS supported_versions;
const SSL_EXTENSION_TYPE ext_types[] = {
- {TLSEXT_TYPE_supported_versions, &have_supported_versions,
- &supported_versions},
+ {TLSEXT_TYPE_supported_versions, &have_supported_versions,
+ &supported_versions},
};
-
- uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!ssl_parse_extensions(&extensions, &alert, ext_types,
+ CBS extensions = server_hello.extensions;
+ if (!ssl_parse_extensions(&extensions, out_alert, ext_types,
/*ignore_unknown=*/true)) {
- ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return false;
}
- // Override the outer version with the extension, if present.
- if (have_supported_versions &&
- (!CBS_get_u16(&supported_versions, version) ||
- CBS_len(&supported_versions) != 0)) {
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ if (!have_supported_versions) {
+ *out_version = server_hello.legacy_version;
+ return true;
+ }
+
+ if (!CBS_get_u16(&supported_versions, out_version) ||
+ CBS_len(&supported_versions) != 0) {
+ *out_alert = SSL_AD_DECODE_ERROR;
return false;
}
@@ -657,6 +651,37 @@
return ssl_hs_flush;
}
+bool ssl_parse_server_hello(ParsedServerHello *out, uint8_t *out_alert,
+ const SSLMessage &msg) {
+ if (msg.type != SSL3_MT_SERVER_HELLO) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_MESSAGE);
+ *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
+ return false;
+ }
+ CBS body = msg.body;
+ if (!CBS_get_u16(&body, &out->legacy_version) ||
+ !CBS_get_bytes(&body, &out->random, SSL3_RANDOM_SIZE) ||
+ !CBS_get_u8_length_prefixed(&body, &out->session_id) ||
+ CBS_len(&out->session_id) > SSL3_SESSION_ID_SIZE ||
+ !CBS_get_u16(&body, &out->cipher_suite) ||
+ !CBS_get_u8(&body, &out->compression_method)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ *out_alert = SSL_AD_DECODE_ERROR;
+ return false;
+ }
+ // In TLS 1.2 and below, empty extensions blocks may be omitted. In TLS 1.3,
+ // ServerHellos always have extensions, so this can be applied generically.
+ CBS_init(&out->extensions, nullptr, 0);
+ if ((CBS_len(&body) != 0 &&
+ !CBS_get_u16_length_prefixed(&body, &out->extensions)) ||
+ CBS_len(&body) != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ *out_alert = SSL_AD_DECODE_ERROR;
+ return false;
+ }
+ return true;
+}
+
static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
SSLMessage msg;
@@ -664,26 +689,12 @@
return ssl_hs_read_server_hello;
}
- if (!ssl_check_message_type(ssl, msg, SSL3_MT_SERVER_HELLO)) {
- return ssl_hs_error;
- }
-
- CBS server_hello = msg.body, server_random, session_id;
- uint16_t server_version, cipher_suite;
- uint8_t compression_method;
- if (!CBS_get_u16(&server_hello, &server_version) ||
- !CBS_get_bytes(&server_hello, &server_random, SSL3_RANDOM_SIZE) ||
- !CBS_get_u8_length_prefixed(&server_hello, &session_id) ||
- CBS_len(&session_id) > SSL3_SESSION_ID_SIZE ||
- !CBS_get_u16(&server_hello, &cipher_suite) ||
- !CBS_get_u8(&server_hello, &compression_method)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- return ssl_hs_error;
- }
-
- // Use the supported_versions extension if applicable.
- if (!parse_supported_versions(hs, &server_version, &server_hello)) {
+ ParsedServerHello server_hello;
+ uint16_t server_version;
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ if (!ssl_parse_server_hello(&server_hello, &alert, msg) ||
+ !parse_server_version(hs, &server_version, &alert, server_hello)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
@@ -737,7 +748,7 @@
}
// Copy over the server random.
- OPENSSL_memcpy(ssl->s3->server_random, CBS_data(&server_random),
+ OPENSSL_memcpy(ssl->s3->server_random, CBS_data(&server_hello.random),
SSL3_RANDOM_SIZE);
// Enforce the TLS 1.3 anti-downgrade feature.
@@ -760,28 +771,26 @@
}
}
- const SSL_CIPHER *cipher = SSL_get_cipher_by_value(cipher_suite);
- if (cipher == NULL) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_CIPHER_RETURNED);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- return ssl_hs_error;
- }
- hs->new_cipher = cipher;
-
// The cipher must be allowed in the selected version and enabled.
+ const SSL_CIPHER *cipher = SSL_get_cipher_by_value(server_hello.cipher_suite);
uint32_t mask_a, mask_k;
ssl_get_client_disabled(hs, &mask_a, &mask_k);
- if ((cipher->algorithm_mkey & mask_k) || (cipher->algorithm_auth & mask_a) ||
+ if (cipher == nullptr ||
+ (cipher->algorithm_mkey & mask_k) ||
+ (cipher->algorithm_auth & mask_a) ||
SSL_CIPHER_get_min_version(cipher) > ssl_protocol_version(ssl) ||
SSL_CIPHER_get_max_version(cipher) < ssl_protocol_version(ssl) ||
- !sk_SSL_CIPHER_find(SSL_get_ciphers(ssl), NULL, cipher)) {
+ !sk_SSL_CIPHER_find(SSL_get_ciphers(ssl), nullptr, cipher)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CIPHER_RETURNED);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
+ hs->new_cipher = cipher;
+
if (hs->session_id_len != 0 &&
- CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len)) {
+ CBS_mem_equal(&server_hello.session_id, hs->session_id,
+ hs->session_id_len)) {
// Echoing the ClientHello session ID in TLS 1.2, whether from the session
// or a synthetic one, indicates resumption. If there was no session (or if
// the session was only offered in ECH ClientHelloInner), this was the
@@ -799,7 +808,7 @@
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
- if (ssl->session->cipher != cipher) {
+ if (ssl->session->cipher != hs->new_cipher) {
OPENSSL_PUT_ERROR(SSL, SSL_R_OLD_SESSION_CIPHER_NOT_RETURNED);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
@@ -822,10 +831,11 @@
return ssl_hs_error;
}
// Note: session_id could be empty.
- hs->new_session->session_id_length = CBS_len(&session_id);
- OPENSSL_memcpy(hs->new_session->session_id, CBS_data(&session_id),
- CBS_len(&session_id));
- hs->new_session->cipher = cipher;
+ hs->new_session->session_id_length = CBS_len(&server_hello.session_id);
+ OPENSSL_memcpy(hs->new_session->session_id,
+ CBS_data(&server_hello.session_id),
+ CBS_len(&server_hello.session_id));
+ hs->new_session->cipher = hs->new_cipher;
}
// Now that the cipher is known, initialize the handshake hash and hash the
@@ -845,26 +855,17 @@
}
// Only the NULL compression algorithm is supported.
- if (compression_method != 0) {
+ if (server_hello.compression_method != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_COMPRESSION_ALGORITHM);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
- // TLS extensions
- if (!ssl_parse_serverhello_tlsext(hs, &server_hello)) {
+ if (!ssl_parse_serverhello_tlsext(hs, &server_hello.extensions)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
return ssl_hs_error;
}
- // There should be nothing left over in the record.
- if (CBS_len(&server_hello) != 0) {
- // wrong packet length
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- return ssl_hs_error;
- }
-
if (ssl->session != NULL &&
hs->extended_master_secret != ssl->session->extended_master_secret) {
if (ssl->session->extended_master_secret) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 7505e5d..0c91724 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2163,6 +2163,21 @@
// flight. It returns true on success and false on error.
bool ssl_add_client_hello(SSL_HANDSHAKE *hs);
+struct ParsedServerHello {
+ uint16_t legacy_version = 0;
+ CBS random;
+ CBS session_id;
+ uint16_t cipher_suite = 0;
+ uint8_t compression_method = 0;
+ CBS extensions;
+};
+
+// ssl_parse_server_hello parses |msg| as a ServerHello. On success, it writes
+// the result to |*out| and returns true. Otherwise, it returns false and sets
+// |*out_alert| to an alert to send to the peer.
+bool ssl_parse_server_hello(ParsedServerHello *out, uint8_t *out_alert,
+ const SSLMessage &msg);
+
enum ssl_cert_verify_context_t {
ssl_cert_verify_server,
ssl_cert_verify_client,
@@ -3304,7 +3319,7 @@
bool ssl_add_serverhello_tlsext(SSL_HANDSHAKE *hs, CBB *out);
bool ssl_parse_clienthello_tlsext(SSL_HANDSHAKE *hs,
const SSL_CLIENT_HELLO *client_hello);
-bool ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs);
+bool ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, const CBS *extensions);
#define tlsext_tick_md EVP_sha256
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 03dcbf1..1953b88 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -3833,7 +3833,7 @@
},
},
shouldFail: true,
- expectedError: ":UNKNOWN_CIPHER_RETURNED:",
+ expectedError: ":WRONG_CIPHER_RETURNED:",
})
testCases = append(testCases, testCase{
name: "ServerHelloBogusCipher-TLS13",
@@ -13884,7 +13884,7 @@
},
},
shouldFail: true,
- expectedError: ":WRONG_VERSION_NUMBER:",
+ expectedError: ":DECODE_ERROR:",
})
testCases = append(testCases, testCase{
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index bc137aa..7a19b2a 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -101,6 +101,25 @@
return true;
}
+static bool parse_server_hello_tls13(const SSL_HANDSHAKE *hs,
+ ParsedServerHello *out, uint8_t *out_alert,
+ const SSLMessage &msg) {
+ if (!ssl_parse_server_hello(out, out_alert, msg)) {
+ return false;
+ }
+ // The RFC8446 version of the structure fixes some legacy values.
+ // Additionally, the session ID must echo the original one.
+ if (out->legacy_version != TLS1_2_VERSION ||
+ out->compression_method != 0 ||
+ !CBS_mem_equal(&out->session_id, hs->session_id, hs->session_id_len) ||
+ CBS_len(&out->extensions) == 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ *out_alert = SSL_AD_DECODE_ERROR;
+ return false;
+ }
+ return true;
+}
+
static enum ssl_hs_wait_t do_read_hello_retry_request(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
assert(ssl->s3->have_version);
@@ -117,32 +136,17 @@
return ssl_hs_error;
}
- if (!ssl_check_message_type(ssl, msg, SSL3_MT_SERVER_HELLO)) {
- return ssl_hs_error;
- }
-
- CBS body = msg.body, extensions, server_random, session_id;
- uint16_t server_version, cipher_suite;
- uint8_t compression_method;
- if (!CBS_get_u16(&body, &server_version) ||
- !CBS_get_bytes(&body, &server_random, SSL3_RANDOM_SIZE) ||
- !CBS_get_u8_length_prefixed(&body, &session_id) ||
- !CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len) ||
- !CBS_get_u16(&body, &cipher_suite) ||
- !CBS_get_u8(&body, &compression_method) ||
- compression_method != 0 ||
- !CBS_get_u16_length_prefixed(&body, &extensions) ||
- CBS_len(&extensions) == 0 ||
- CBS_len(&body) != 0) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ ParsedServerHello server_hello;
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ if (!parse_server_hello_tls13(hs, &server_hello, &alert, msg)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
// The cipher suite must be one we offered. We currently offer all supported
// TLS 1.3 ciphers, so check the version.
- const SSL_CIPHER *cipher = SSL_get_cipher_by_value(cipher_suite);
- if (cipher == NULL ||
+ const SSL_CIPHER *cipher = SSL_get_cipher_by_value(server_hello.cipher_suite);
+ if (cipher == nullptr ||
SSL_CIPHER_get_min_version(cipher) > ssl_protocol_version(ssl) ||
SSL_CIPHER_get_max_version(cipher) < ssl_protocol_version(ssl)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CIPHER_RETURNED);
@@ -152,7 +156,8 @@
hs->new_cipher = cipher;
- if (!CBS_mem_equal(&server_random, kHelloRetryRequest, SSL3_RANDOM_SIZE)) {
+ if (!CBS_mem_equal(&server_hello.random, kHelloRetryRequest,
+ SSL3_RANDOM_SIZE)) {
hs->tls13_state = state_read_server_hello;
return ssl_hs_ok;
}
@@ -166,8 +171,7 @@
&supported_versions},
};
- uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!ssl_parse_extensions(&extensions, &alert, ext_types,
+ if (!ssl_parse_extensions(&server_hello.extensions, &alert, ext_types,
/*ignore_unknown=*/false)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
@@ -287,49 +291,29 @@
if (!ssl->method->get_message(ssl, &msg)) {
return ssl_hs_read_message;
}
- if (!ssl_check_message_type(ssl, msg, SSL3_MT_SERVER_HELLO)) {
- return ssl_hs_error;
- }
-
- CBS body = msg.body, server_random, session_id, extensions;
- uint16_t server_version;
- uint16_t cipher_suite;
- uint8_t compression_method;
- if (!CBS_get_u16(&body, &server_version) ||
- !CBS_get_bytes(&body, &server_random, SSL3_RANDOM_SIZE) ||
- !CBS_get_u8_length_prefixed(&body, &session_id) ||
- !CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len) ||
- !CBS_get_u16(&body, &cipher_suite) ||
- !CBS_get_u8(&body, &compression_method) ||
- compression_method != 0 ||
- !CBS_get_u16_length_prefixed(&body, &extensions) ||
- CBS_len(&body) != 0) {
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- return ssl_hs_error;
- }
-
- if (server_version != TLS1_2_VERSION) {
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_VERSION_NUMBER);
+ ParsedServerHello server_hello;
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ if (!parse_server_hello_tls13(hs, &server_hello, &alert, msg)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
// Forbid a second HelloRetryRequest.
- if (CBS_mem_equal(&server_random, kHelloRetryRequest, SSL3_RANDOM_SIZE)) {
+ if (CBS_mem_equal(&server_hello.random, kHelloRetryRequest,
+ SSL3_RANDOM_SIZE)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_MESSAGE);
return ssl_hs_error;
}
// Check the cipher suite, in case this is after HelloRetryRequest.
- if (SSL_CIPHER_get_value(hs->new_cipher) != cipher_suite) {
+ if (SSL_CIPHER_get_value(hs->new_cipher) != server_hello.cipher_suite) {
OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CIPHER_RETURNED);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
- OPENSSL_memcpy(ssl->s3->server_random, CBS_data(&server_random),
+ OPENSSL_memcpy(ssl->s3->server_random, CBS_data(&server_hello.random),
SSL3_RANDOM_SIZE);
// Parse out the extensions.
@@ -343,8 +327,7 @@
&supported_versions},
};
- uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!ssl_parse_extensions(&extensions, &alert, ext_types,
+ if (!ssl_parse_extensions(&server_hello.extensions, &alert, ext_types,
/*ignore_unknown=*/false)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
@@ -521,17 +504,19 @@
return ssl_hs_error;
}
- CBS body = msg.body;
- if (!ssl_parse_serverhello_tlsext(hs, &body)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
- return ssl_hs_error;
- }
- if (CBS_len(&body) != 0) {
+ CBS body = msg.body, extensions;
+ if (!CBS_get_u16_length_prefixed(&body, &extensions) ||
+ CBS_len(&body) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
return ssl_hs_error;
}
+ if (!ssl_parse_serverhello_tlsext(hs, &extensions)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
+ return ssl_hs_error;
+ }
+
if (ssl->s3->early_data_accepted) {
// The extension parser checks the server resumed the session.
assert(ssl->s3->session_reused);
