From abbbee10ad4739648bcbf36a5ac52f23263a67dd Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 31 Oct 2016 19:20:42 -0400 Subject: [PATCH] Detach TLS 1.3 cipher configuration from the cipher language. TLS 1.3 ciphers are now always enabled and come with a hard-coded preference order. BUG=110 Change-Id: Idd9cb0d75fb6bf2676ecdee27d88893ff974c4a3 Reviewed-on: https://boringssl-review.googlesource.com/12025 Reviewed-by: Adam Langley --- include/openssl/ssl.h | 7 +++- ssl/handshake_client.c | 55 +++++++++++++++++++---------- ssl/internal.h | 5 ++- ssl/s3_lib.c | 16 ++++++--- ssl/ssl_cipher.c | 14 +++----- ssl/ssl_test.cc | 2 -- ssl/test/bssl_shim.cc | 20 +++++++++++ ssl/test/runner/runner.go | 73 ++++++++++++++++++++++++++++----------- ssl/test/test_config.cc | 2 ++ ssl/test/test_config.h | 2 ++ ssl/tls13_client.c | 9 ++--- ssl/tls13_server.c | 46 ++++++++++++++++++++++-- 12 files changed, 187 insertions(+), 64 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 417b1944..449bd8c1 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1306,7 +1306,12 @@ OPENSSL_EXPORT int SSL_CIPHER_get_bits(const SSL_CIPHER *cipher, * [ECDHE-ECDSA-CHACHA20-POLY1305|ECDHE-ECDSA-AES128-GCM-SHA256] * * Once an equal-preference group is used, future directives must be - * opcode-less. */ + * opcode-less. + * + * TLS 1.3 ciphers do not participate in this mechanism and instead have a + * built-in preference order. Functions to set cipher lists do not affect TLS + * 1.3, and functions to query the cipher list do not include TLS 1.3 + * ciphers. */ /* SSL_DEFAULT_CIPHER_LIST is the default cipher suite configuration. It is * substituted when a cipher string starts with 'DEFAULT'. */ diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index b26b0127..67a4c091 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -152,6 +152,7 @@ #include #include +#include #include #include #include @@ -605,30 +606,48 @@ static int ssl_write_client_cipher_list(SSL *ssl, CBB *out, return 0; } - STACK_OF(SSL_CIPHER) *ciphers = SSL_get_ciphers(ssl); - - int any_enabled = 0; - for (size_t i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { - const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(ciphers, i); - /* Skip disabled ciphers */ - if ((cipher->algorithm_mkey & ssl->cert->mask_k) || - (cipher->algorithm_auth & ssl->cert->mask_a)) { - continue; + /* Add TLS 1.3 ciphers. Order ChaCha20-Poly1305 relative to AES-GCM based on + * hardware support. */ + if (max_version >= TLS1_3_VERSION) { + if (!EVP_has_aes_hardware() && + !CBB_add_u16(&child, TLS1_CK_CHACHA20_POLY1305_SHA256 & 0xffff)) { + return 0; } - if (SSL_CIPHER_get_min_version(cipher) > max_version || - SSL_CIPHER_get_max_version(cipher) < min_version) { - continue; + if (!CBB_add_u16(&child, TLS1_CK_AES_128_GCM_SHA256 & 0xffff) || + !CBB_add_u16(&child, TLS1_CK_AES_256_GCM_SHA384 & 0xffff)) { + return 0; } - any_enabled = 1; - if (!CBB_add_u16(&child, ssl_cipher_get_value(cipher))) { + if (EVP_has_aes_hardware() && + !CBB_add_u16(&child, TLS1_CK_CHACHA20_POLY1305_SHA256 & 0xffff)) { return 0; } } - /* If all ciphers were disabled, return the error to the caller. */ - if (!any_enabled) { - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHERS_AVAILABLE); - return 0; + if (min_version < TLS1_3_VERSION) { + STACK_OF(SSL_CIPHER) *ciphers = SSL_get_ciphers(ssl); + int any_enabled = 0; + for (size_t i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { + const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(ciphers, i); + /* Skip disabled ciphers */ + if ((cipher->algorithm_mkey & ssl->cert->mask_k) || + (cipher->algorithm_auth & ssl->cert->mask_a)) { + continue; + } + if (SSL_CIPHER_get_min_version(cipher) > max_version || + SSL_CIPHER_get_max_version(cipher) < min_version) { + continue; + } + any_enabled = 1; + if (!CBB_add_u16(&child, ssl_cipher_get_value(cipher))) { + return 0; + } + } + + /* If all ciphers were disabled, return the error to the caller. */ + if (!any_enabled && max_version < TLS1_3_VERSION) { + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHERS_AVAILABLE); + return 0; + } } /* For SSLv3, the SCSV is added. Otherwise the renegotiation extension is diff --git a/ssl/internal.h b/ssl/internal.h index 461de2c1..b217017a 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1658,6 +1658,9 @@ OPENSSL_EXPORT SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *session, void ssl_cipher_preference_list_free( struct ssl_cipher_preference_list_st *cipher_list); + +/* ssl_get_cipher_preferences returns the cipher preference list for TLS 1.2 and + * below. */ const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences( const SSL *ssl); @@ -1712,7 +1715,7 @@ int ssl3_output_cert_chain(SSL *ssl); /* ssl_is_valid_cipher checks that |cipher| is valid according to the current * server configuration in |ssl|. It returns 1 if valid, and 0 otherwise. */ -int ssl_is_valid_cipher(SSL *ssl, const SSL_CIPHER *cipher); +int ssl_is_valid_cipher(const SSL *ssl, const SSL_CIPHER *cipher); const SSL_CIPHER *ssl3_choose_cipher( SSL *ssl, const struct ssl_early_callback_ctx *client_hello, diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 2a7c91a9..f1195232 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -241,15 +241,21 @@ const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences( return NULL; } -int ssl_is_valid_cipher(SSL *ssl, const SSL_CIPHER *cipher) { +int ssl_is_valid_cipher(const SSL *ssl, const SSL_CIPHER *cipher) { /* Check the TLS version. */ - if (SSL_CIPHER_get_min_version(cipher) > ssl3_protocol_version(ssl) || - SSL_CIPHER_get_max_version(cipher) < ssl3_protocol_version(ssl)) { + uint16_t version = ssl3_protocol_version(ssl); + if (SSL_CIPHER_get_min_version(cipher) > version || + SSL_CIPHER_get_max_version(cipher) < version) { return 0; } - return sk_SSL_CIPHER_find(ssl_get_cipher_preferences(ssl)->ciphers, - NULL, cipher); + /* TLS 1.3 ciphers are not configurable. */ + if (version >= TLS1_3_VERSION) { + return 1; + } + + return sk_SSL_CIPHER_find(ssl_get_cipher_preferences(ssl)->ciphers, NULL, + cipher); } const SSL_CIPHER *ssl3_choose_cipher( diff --git a/ssl/ssl_cipher.c b/ssl/ssl_cipher.c index deb463ef..9ca7f24a 100644 --- a/ssl/ssl_cipher.c +++ b/ssl/ssl_cipher.c @@ -738,9 +738,6 @@ static const CIPHER_ALIAS kCipherAliases[] = { {"TLSv1", ~SSL_kCECPQ1, ~0u, ~SSL_eNULL, ~0u, SSL3_VERSION}, {"TLSv1.2", ~SSL_kCECPQ1, ~0u, ~SSL_eNULL, ~0u, TLS1_2_VERSION}, - /* AEAD-only ciphers for TLS 1.3. */ - {"GENERIC", SSL_kGENERIC, SSL_aGENERIC, ~0u, ~0u, 0}, - /* Legacy strength classes. */ {"HIGH", ~SSL_kCECPQ1, ~0u, ~SSL_eNULL, ~0u, 0}, {"FIPS", ~SSL_kCECPQ1, ~0u, ~SSL_eNULL, ~0u, 0}, @@ -943,7 +940,9 @@ static void ssl_cipher_collect_ciphers(const SSL_PROTOCOL_METHOD *ssl_method, size_t co_list_num = 0; for (size_t i = 0; i < kCiphersLen; i++) { const SSL_CIPHER *cipher = &kCiphers[i]; - if (ssl_method->supports_cipher(cipher)) { + if (ssl_method->supports_cipher(cipher) && + /* TLS 1.3 ciphers do not participate in this mechanism. */ + cipher->algorithm_mkey != SSL_kGENERIC) { co_list[co_list_num].cipher = cipher; co_list[co_list_num].next = NULL; co_list[co_list_num].prev = NULL; @@ -1386,11 +1385,8 @@ ssl_create_cipher_list(const SSL_PROTOCOL_METHOD *ssl_method, /* Now arrange all ciphers by preference: * TODO(davidben): Compute this order once and copy it. */ - /* Everything else being equal, prefer TLS 1.3 ciphers then ECDHE_ECDSA then - * ECDHE_RSA over other key exchange mechanisms */ - - ssl_cipher_apply_rule(0, SSL_kGENERIC, SSL_aGENERIC, ~0u, ~0u, 0, CIPHER_ADD, - -1, 0, &head, &tail); + /* Everything else being equal, prefer ECDHE_ECDSA and ECDHE_RSA over other + * key exchange mechanisms */ ssl_cipher_apply_rule(0, SSL_kECDHE, SSL_aECDSA, ~0u, ~0u, 0, CIPHER_ADD, -1, 0, &head, &tail); ssl_cipher_apply_rule(0, SSL_kECDHE, ~0u, ~0u, ~0u, 0, CIPHER_ADD, -1, 0, diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 4c658d3b..ed32c212 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -269,7 +269,6 @@ static const char *kMustNotIncludeNull[] = { "SSLv3", "TLSv1", "TLSv1.2", - "GENERIC", }; static const char *kMustNotIncludeCECPQ1[] = { @@ -294,7 +293,6 @@ static const char *kMustNotIncludeCECPQ1[] = { "AES256", "AESGCM", "CHACHA20", - "GENERIC", }; static const CurveTest kCurveTests[] = { diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 2e2f5802..ed17aa87 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -40,6 +40,7 @@ OPENSSL_MSVC_PRAGMA(comment(lib, "Ws2_32.lib")) #include #include +#include #include #include #include @@ -1320,6 +1321,25 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume) { } } + uint16_t cipher_id = + static_cast(SSL_CIPHER_get_id(SSL_get_current_cipher(ssl))); + if (config->expect_cipher_aes != 0 && + EVP_has_aes_hardware() && + static_cast(config->expect_cipher_aes) != cipher_id) { + fprintf(stderr, "Cipher ID was %04x, wanted %04x (has AES hardware)\n", + cipher_id, static_cast(config->expect_cipher_aes)); + return false; + } + + if (config->expect_cipher_no_aes != 0 && + !EVP_has_aes_hardware() && + static_cast(config->expect_cipher_no_aes) != cipher_id) { + fprintf(stderr, "Cipher ID was %04x, wanted %04x (no AES hardware)\n", + cipher_id, static_cast(config->expect_cipher_no_aes)); + return false; + } + + if (!config->psk.empty()) { if (SSL_get_peer_cert_chain(ssl) != nullptr) { fprintf(stderr, "Received peer certificate on a PSK cipher.\n"); diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index ae1d0b65..bc4e5701 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -5508,26 +5508,6 @@ func addResumptionVersionTests() { expectResumeRejected: true, }) - testCases = append(testCases, testCase{ - testType: serverTest, - name: "Resume-Server-DeclineBadCipher-2-TLS13", - resumeSession: true, - config: Config{ - MaxVersion: VersionTLS13, - Bugs: ProtocolBugs{ - FilterTicket: func(in []byte) ([]byte, error) { - return SetShimTicketCipherSuite(in, TLS_AES_256_GCM_SHA384) - }, - }, - }, - flags: []string{ - "-cipher", "AES128", - "-ticket-key", - base64.StdEncoding.EncodeToString(TestShimTicketKey), - }, - expectResumeRejected: true, - }) - // Sessions may not be resumed at a different cipher. testCases = append(testCases, testCase{ name: "Resume-Client-CipherMismatch", @@ -8973,6 +8953,58 @@ func addTLS13HandshakeTests() { }) } +func addTLS13CipherPreferenceTests() { + // Test that client preference is honored if the shim has AES hardware + // and ChaCha20-Poly1305 is preferred otherwise. + testCases = append(testCases, testCase{ + testType: serverTest, + name: "TLS13-CipherPreference-Server-ChaCha20-AES", + config: Config{ + MaxVersion: VersionTLS13, + CipherSuites: []uint16{ + TLS_CHACHA20_POLY1305_SHA256, + TLS_AES_128_GCM_SHA256, + }, + }, + flags: []string{ + "-expect-cipher-aes", strconv.Itoa(int(TLS_CHACHA20_POLY1305_SHA256)), + "-expect-cipher-no-aes", strconv.Itoa(int(TLS_CHACHA20_POLY1305_SHA256)), + }, + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "TLS13-CipherPreference-Server-AES-ChaCha20", + config: Config{ + MaxVersion: VersionTLS13, + CipherSuites: []uint16{ + TLS_AES_128_GCM_SHA256, + TLS_CHACHA20_POLY1305_SHA256, + }, + }, + flags: []string{ + "-expect-cipher-aes", strconv.Itoa(int(TLS_AES_128_GCM_SHA256)), + "-expect-cipher-no-aes", strconv.Itoa(int(TLS_CHACHA20_POLY1305_SHA256)), + }, + }) + + // Test that the client orders ChaCha20-Poly1305 and AES-GCM based on + // whether it has AES hardware. + testCases = append(testCases, testCase{ + name: "TLS13-CipherPreference-Client", + config: Config{ + MaxVersion: VersionTLS13, + // Use the client cipher order. (This is the default but + // is listed to be explicit.) + PreferServerCipherSuites: false, + }, + flags: []string{ + "-expect-cipher-aes", strconv.Itoa(int(TLS_AES_128_GCM_SHA256)), + "-expect-cipher-no-aes", strconv.Itoa(int(TLS_CHACHA20_POLY1305_SHA256)), + }, + }) +} + func addPeekTests() { // Test SSL_peek works, including on empty records. testCases = append(testCases, testCase{ @@ -9160,6 +9192,7 @@ func main() { addWrongMessageTypeTests() addTrailingMessageDataTests() addTLS13HandshakeTests() + addTLS13CipherPreferenceTests() addPeekTests() var wg sync.WaitGroup diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 112d642d..3d3ecd80 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -158,6 +158,8 @@ const Flag kIntFlags[] = { { "-expect-dhe-group-size", &TestConfig::expect_dhe_group_size }, { "-initial-timeout-duration-ms", &TestConfig::initial_timeout_duration_ms }, { "-max-cert-list", &TestConfig::max_cert_list }, + { "-expect-cipher-aes", &TestConfig::expect_cipher_aes }, + { "-expect-cipher-no-aes", &TestConfig::expect_cipher_no_aes }, }; const Flag> kIntVectorFlags[] = { diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index d91a74be..34c02fbc 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -117,6 +117,8 @@ struct TestConfig { int max_cert_list = 0; std::string ticket_key; bool use_exporter_between_reads = false; + int expect_cipher_aes = 0; + int expect_cipher_no_aes = 0; }; bool ParseConfig(int argc, char **argv, TestConfig *out_config); diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c index 3b854420..409dc24b 100644 --- a/ssl/tls13_client.c +++ b/ssl/tls13_client.c @@ -218,12 +218,9 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL *ssl, SSL_HANDSHAKE *hs) { return ssl_hs_error; } - /* Check if the cipher is disabled. */ - if ((cipher->algorithm_mkey & ssl->cert->mask_k) || - (cipher->algorithm_auth & ssl->cert->mask_a) || - SSL_CIPHER_get_min_version(cipher) > ssl3_protocol_version(ssl) || - SSL_CIPHER_get_max_version(cipher) < ssl3_protocol_version(ssl) || - !sk_SSL_CIPHER_find(ssl_get_ciphers_by_id(ssl), NULL, cipher)) { + /* Check if the cipher is a TLS 1.3 cipher. */ + if (SSL_CIPHER_get_min_version(cipher) > ssl3_protocol_version(ssl) || + SSL_CIPHER_get_max_version(cipher) < ssl3_protocol_version(ssl)) { OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CIPHER_RETURNED); ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); return ssl_hs_error; diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c index 574e6ddd..14ebb813 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -171,6 +172,48 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { return ssl_hs_ok; } +static const SSL_CIPHER *choose_tls13_cipher( + const SSL *ssl, const struct ssl_early_callback_ctx *client_hello) { + if (client_hello->cipher_suites_len % 2 != 0) { + return NULL; + } + + CBS cipher_suites; + CBS_init(&cipher_suites, client_hello->cipher_suites, + client_hello->cipher_suites_len); + + const int aes_is_fine = EVP_has_aes_hardware(); + + const SSL_CIPHER *best = NULL; + while (CBS_len(&cipher_suites) > 0) { + uint16_t cipher_suite; + if (!CBS_get_u16(&cipher_suites, &cipher_suite)) { + return NULL; + } + + const SSL_CIPHER *candidate = SSL_get_cipher_by_value(cipher_suite); + if (candidate == NULL || !ssl_is_valid_cipher(ssl, candidate)) { + continue; + } + + /* TLS 1.3 removes legacy ciphers, so honor the client order, but prefer + * ChaCha20 if we do not have AES hardware. */ + if (aes_is_fine) { + return candidate; + } + + if (candidate->algorithm_enc == SSL_CHACHA20POLY1305) { + return candidate; + } + + if (best == NULL) { + best = candidate; + } + } + + return best; +} + static enum ssl_hs_wait_t do_select_parameters(SSL *ssl, SSL_HANDSHAKE *hs) { if (!ssl->s3->session_reused) { /* Call |cert_cb| to update server certificates if required. */ @@ -197,8 +240,7 @@ static enum ssl_hs_wait_t do_select_parameters(SSL *ssl, SSL_HANDSHAKE *hs) { } if (!ssl->s3->session_reused) { - const SSL_CIPHER *cipher = - ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl)); + const SSL_CIPHER *cipher = choose_tls13_cipher(ssl, &client_hello); if (cipher == NULL) { OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER); ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);