From 65fb4258114dc30c4cb9c4636b1432214cb40683 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 14 Dec 2016 18:52:21 -0500 Subject: [PATCH] Remove version-specific cipher lists. There are no longer any consumers of these APIs. These were useful back when the CBC vs. RC4 tradeoff varied by version and it was worth carefully tuning this cutoff. Nowadays RC4 is completely gone and there's no use in configuring these anymore. To avoid invalidating the existing ssl_ctx_api corpus and requiring it regenerated, I've left the entries in there. It's probably reasonable for new API fuzzers to reuse those slots. Change-Id: I02bf950e3828062341e4e45c8871a44597ae93d5 Reviewed-on: https://boringssl-review.googlesource.com/12880 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- fuzz/ssl_ctx_api.cc | 14 ++----- include/openssl/ssl.h | 31 +------------- ssl/s3_lib.c | 14 +------ ssl/ssl_lib.c | 34 --------------- ssl/test/bssl_shim.cc | 11 ----- ssl/test/runner/runner.go | 88 --------------------------------------- ssl/test/test_config.cc | 2 - ssl/test/test_config.h | 2 - 8 files changed, 6 insertions(+), 190 deletions(-) diff --git a/fuzz/ssl_ctx_api.cc b/fuzz/ssl_ctx_api.cc index a66b1a84..b721c6b2 100644 --- a/fuzz/ssl_ctx_api.cc +++ b/fuzz/ssl_ctx_api.cc @@ -347,18 +347,12 @@ static const std::function kAPIs[] = { SSL_CTX_set_cipher_list(ctx, ciphers.c_str()); }, [](SSL_CTX *ctx, CBS *cbs) { - std::string ciphers; - if (!GetString(&ciphers, cbs)) { - return; - } - SSL_CTX_set_cipher_list_tls10(ctx, ciphers.c_str()); + // This function was left blank rather than removed to avoid invalidating + // the existing corpus. New entries may reuse it. }, [](SSL_CTX *ctx, CBS *cbs) { - std::string ciphers; - if (!GetString(&ciphers, cbs)) { - return; - } - SSL_CTX_set_cipher_list_tls11(ctx, ciphers.c_str()); + // This function was left blank rather than removed to avoid invalidating + // the existing corpus. New entries may reuse it. }, [](SSL_CTX *ctx, CBS *cbs) { std::string ciphers; diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 2a76fe27..84d0e28f 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1331,25 +1331,11 @@ OPENSSL_EXPORT int SSL_CIPHER_get_bits(const SSL_CIPHER *cipher, * |str| as a cipher string. It returns one on success and zero on failure. */ OPENSSL_EXPORT int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str); -/* SSL_CTX_set_cipher_list_tls10 configures the TLS 1.0+ cipher list for |ctx|, - * evaluating |str| as a cipher string. It returns one on success and zero on - * failure. If set, servers will use this cipher suite list for TLS 1.0 or - * higher. */ -OPENSSL_EXPORT int SSL_CTX_set_cipher_list_tls10(SSL_CTX *ctx, const char *str); - -/* SSL_CTX_set_cipher_list_tls11 configures the TLS 1.1+ cipher list for |ctx|, - * evaluating |str| as a cipher string. It returns one on success and zero on - * failure. If set, servers will use this cipher suite list for TLS 1.1 or - * higher. */ -OPENSSL_EXPORT int SSL_CTX_set_cipher_list_tls11(SSL_CTX *ctx, const char *str); - /* SSL_set_cipher_list configures the cipher list for |ssl|, evaluating |str| as * a cipher string. It returns one on success and zero on failure. */ OPENSSL_EXPORT int SSL_set_cipher_list(SSL *ssl, const char *str); -/* SSL_get_ciphers returns the cipher list for |ssl|, in order of preference. If - * |SSL_CTX_set_cipher_list_tls10| or |SSL_CTX_set_cipher_list_tls11| has been - * used, the corresponding list for the current version is returned. */ +/* SSL_get_ciphers returns the cipher list for |ssl|, in order of preference. */ OPENSSL_EXPORT STACK_OF(SSL_CIPHER) *SSL_get_ciphers(const SSL *ssl); @@ -3839,21 +3825,6 @@ struct ssl_ctx_st { struct ssl_cipher_preference_list_st *cipher_list; - /* cipher_list_tls10 is the list of ciphers when TLS 1.0 or greater is in - * use. This only applies to server connections as, for clients, the version - * number is known at connect time and so the cipher list can be set then. If - * |cipher_list_tls11| is non-NULL then this applies only to TLS 1.0 - * connections. - * - * TODO(agl): this exists to assist in the death of SSLv3. It can hopefully - * be removed after that. */ - struct ssl_cipher_preference_list_st *cipher_list_tls10; - - /* cipher_list_tls11 is the list of ciphers when TLS 1.1 or greater is in - * use. This only applies to server connections as, for clients, the version - * number is known at connect time and so the cipher list can be set then. */ - struct ssl_cipher_preference_list_st *cipher_list_tls11; - X509_STORE *cert_store; LHASH_OF(SSL_SESSION) *sessions; /* Most session-ids that will be cached, default is diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 859cb9be..3b144113 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -221,19 +221,7 @@ const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences( return ssl->cipher_list; } - if (ssl->version >= TLS1_1_VERSION && ssl->ctx->cipher_list_tls11 != NULL) { - return ssl->ctx->cipher_list_tls11; - } - - if (ssl->version >= TLS1_VERSION && ssl->ctx->cipher_list_tls10 != NULL) { - return ssl->ctx->cipher_list_tls10; - } - - if (ssl->ctx->cipher_list != NULL) { - return ssl->ctx->cipher_list; - } - - return NULL; + return ssl->ctx->cipher_list; } /* If we are using default SHA1+MD5 algorithms switch to new SHA256 PRF and diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 77e86a8c..eee2fb2a 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -352,8 +352,6 @@ void SSL_CTX_free(SSL_CTX *ctx) { lh_SSL_SESSION_free(ctx->sessions); X509_STORE_free(ctx->cert_store); ssl_cipher_preference_list_free(ctx->cipher_list); - ssl_cipher_preference_list_free(ctx->cipher_list_tls10); - ssl_cipher_preference_list_free(ctx->cipher_list_tls11); ssl_cert_free(ctx->cert); sk_SSL_CUSTOM_EXTENSION_pop_free(ctx->client_custom_extensions, SSL_CUSTOM_EXTENSION_free); @@ -1680,38 +1678,6 @@ int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str) { return 1; } -int SSL_CTX_set_cipher_list_tls10(SSL_CTX *ctx, const char *str) { - STACK_OF(SSL_CIPHER) *cipher_list = - ssl_create_cipher_list(ctx->method, &ctx->cipher_list_tls10, str); - if (cipher_list == NULL) { - return 0; - } - - /* |ssl_create_cipher_list| may succeed but return an empty cipher list. */ - if (sk_SSL_CIPHER_num(cipher_list) == 0) { - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH); - return 0; - } - - return 1; -} - -int SSL_CTX_set_cipher_list_tls11(SSL_CTX *ctx, const char *str) { - STACK_OF(SSL_CIPHER) *cipher_list = - ssl_create_cipher_list(ctx->method, &ctx->cipher_list_tls11, str); - if (cipher_list == NULL) { - return 0; - } - - /* |ssl_create_cipher_list| may succeed but return an empty cipher list. */ - if (sk_SSL_CIPHER_num(cipher_list) == 0) { - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH); - return 0; - } - - return 1; -} - int SSL_set_cipher_list(SSL *ssl, const char *str) { STACK_OF(SSL_CIPHER) *cipher_list = ssl_create_cipher_list(ssl->ctx->method, &ssl->cipher_list, str); diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index a3903e2a..f2d6d9f0 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -928,17 +928,6 @@ static bssl::UniquePtr SetupCtx(const TestConfig *config) { return nullptr; } - if (!config->cipher_tls10.empty() && - !SSL_CTX_set_cipher_list_tls10(ssl_ctx.get(), - config->cipher_tls10.c_str())) { - return nullptr; - } - if (!config->cipher_tls11.empty() && - !SSL_CTX_set_cipher_list_tls11(ssl_ctx.get(), - config->cipher_tls11.c_str())) { - return nullptr; - } - bssl::UniquePtr dh(DH_get_2048_256(NULL)); if (!dh) { return nullptr; diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index fc66cf69..98226762 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -2774,94 +2774,6 @@ func addCipherSuiteTests() { }, flags: []string{"-psk", "secret"}, }) - - // versionSpecificCiphersTest specifies a test for the TLS 1.0 and TLS - // 1.1 specific cipher suite settings. A server is setup with the given - // cipher lists and then a connection is made for each member of - // expectations. The cipher suite that the server selects must match - // the specified one. - var versionSpecificCiphersTest = []struct { - ciphersDefault, ciphersTLS10, ciphersTLS11 string - // expectations is a map from TLS version to cipher suite id. - expectations map[uint16]uint16 - }{ - { - // Test that the null case (where no version-specific ciphers are set) - // works as expected. - "DES-CBC3-SHA:AES128-SHA", // default ciphers - "", // no ciphers specifically for TLS ≥ 1.0 - "", // no ciphers specifically for TLS ≥ 1.1 - map[uint16]uint16{ - VersionSSL30: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS10: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS11: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS12: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - }, - }, - { - // With ciphers_tls10 set, TLS 1.0, 1.1 and 1.2 should get a different - // cipher. - "DES-CBC3-SHA:AES128-SHA", // default - "AES128-SHA", // these ciphers for TLS ≥ 1.0 - "", // no ciphers specifically for TLS ≥ 1.1 - map[uint16]uint16{ - VersionSSL30: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS10: TLS_RSA_WITH_AES_128_CBC_SHA, - VersionTLS11: TLS_RSA_WITH_AES_128_CBC_SHA, - VersionTLS12: TLS_RSA_WITH_AES_128_CBC_SHA, - }, - }, - { - // With ciphers_tls11 set, TLS 1.1 and 1.2 should get a different - // cipher. - "DES-CBC3-SHA:AES128-SHA", // default - "", // no ciphers specifically for TLS ≥ 1.0 - "AES128-SHA", // these ciphers for TLS ≥ 1.1 - map[uint16]uint16{ - VersionSSL30: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS10: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS11: TLS_RSA_WITH_AES_128_CBC_SHA, - VersionTLS12: TLS_RSA_WITH_AES_128_CBC_SHA, - }, - }, - { - // With both ciphers_tls10 and ciphers_tls11 set, ciphers_tls11 should - // mask ciphers_tls10 for TLS 1.1 and 1.2. - "DES-CBC3-SHA:AES128-SHA", // default - "AES128-SHA", // these ciphers for TLS ≥ 1.0 - "AES256-SHA", // these ciphers for TLS ≥ 1.1 - map[uint16]uint16{ - VersionSSL30: TLS_RSA_WITH_3DES_EDE_CBC_SHA, - VersionTLS10: TLS_RSA_WITH_AES_128_CBC_SHA, - VersionTLS11: TLS_RSA_WITH_AES_256_CBC_SHA, - VersionTLS12: TLS_RSA_WITH_AES_256_CBC_SHA, - }, - }, - } - - for i, test := range versionSpecificCiphersTest { - for version, expectedCipherSuite := range test.expectations { - flags := []string{"-cipher", test.ciphersDefault} - if len(test.ciphersTLS10) > 0 { - flags = append(flags, "-cipher-tls10", test.ciphersTLS10) - } - if len(test.ciphersTLS11) > 0 { - flags = append(flags, "-cipher-tls11", test.ciphersTLS11) - } - - testCases = append(testCases, testCase{ - testType: serverTest, - name: fmt.Sprintf("VersionSpecificCiphersTest-%d-%x", i, version), - config: Config{ - MaxVersion: version, - MinVersion: version, - CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA, TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA}, - }, - flags: flags, - expectedCipher: expectedCipherSuite, - }) - } - } } func addBadECDSASignatureTests() { diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 492dd735..2772831d 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -135,8 +135,6 @@ const Flag kStringFlags[] = { { "-psk-identity", &TestConfig::psk_identity }, { "-srtp-profiles", &TestConfig::srtp_profiles }, { "-cipher", &TestConfig::cipher }, - { "-cipher-tls10", &TestConfig::cipher_tls10 }, - { "-cipher-tls11", &TestConfig::cipher_tls11 }, { "-export-label", &TestConfig::export_label }, { "-export-context", &TestConfig::export_context }, { "-expect-peer-cert-file", &TestConfig::expect_peer_cert_file }, diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 4d6a3362..29364778 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -75,8 +75,6 @@ struct TestConfig { bool fail_second_ddos_callback = false; bool fail_cert_callback = false; std::string cipher; - std::string cipher_tls10; - std::string cipher_tls11; bool handshake_never_done = false; int export_keying_material = 0; std::string export_label;