From b0251b12956ed8e9e41f7bf0bbb02b337e17ad52 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 21 Apr 2023 17:56:08 -0400 Subject: [PATCH 1/2] Disable TLS_RSA_WITH_3DES_EDE_CBC_SHA by default 3DES has long been obsolete. It uses a small block size, making it vulnerable to attacks at sufficiently high volumes (see https://sweet32.info/, CVE-2016-6329). On top of this, it is slow even without constant-time protections, making it a DoS risk for server operators. Since the alias "3DES" has existed in OpenSSL for a long time, keep that one working, to reduce the risk of breaking someone who specifically wanted 3DES enabled. Update-Note: This CL disables TLS_RSA_WITH_3DES_EDE_CBC_SHA by default. Specifically, it will not be included unless explicitly listed in the cipher config, as "TLS_RSA_WITH_3DES_EDE_CBC_SHA", its legacy OpenSSL name "DES-CBC3-SHA", or the alias "3DES". To restore it, add one of the above to your cipher config. Bug: 599 Change-Id: Ib94a2f149b3bfa240ef1008b9f3729a9c10368fb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59425 Auto-Submit: David Benjamin Commit-Queue: Adam Langley Reviewed-by: Adam Langley --- ssl/ssl_cipher.cc | 20 ++--- ssl/ssl_test.cc | 151 ++++++++++++++++++++++++++++---------- ssl/test/fuzzer.h | 2 +- ssl/test/runner/runner.go | 21 ++++-- 4 files changed, 140 insertions(+), 54 deletions(-) diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc index abf5e3d4a..23af47483 100644 --- a/ssl/ssl_cipher.cc +++ b/ssl/ssl_cipher.cc @@ -540,12 +540,16 @@ static const CIPHER_ALIAS kCipherAliases[] = { {"PSK", SSL_kPSK, SSL_aPSK, ~0u, ~0u, 0}, // symmetric encryption aliases - {"3DES", ~0u, ~0u, SSL_3DES, ~0u, 0}, - {"AES128", ~0u, ~0u, SSL_AES128 | SSL_AES128GCM, ~0u, 0}, - {"AES256", ~0u, ~0u, SSL_AES256 | SSL_AES256GCM, ~0u, 0}, + {"3DES", ~0u, ~0u, SSL_3DES, ~0u, 0, /*include_deprecated=*/true}, + {"AES128", ~0u, ~0u, SSL_AES128 | SSL_AES128GCM, ~0u, 0, + /*include_deprecated=*/false}, + {"AES256", ~0u, ~0u, SSL_AES256 | SSL_AES256GCM, ~0u, 0, + /*include_deprecated=*/false}, {"AES", ~0u, ~0u, SSL_AES, ~0u, 0}, - {"AESGCM", ~0u, ~0u, SSL_AES128GCM | SSL_AES256GCM, ~0u, 0}, - {"CHACHA20", ~0u, ~0u, SSL_CHACHA20POLY1305, ~0u, 0}, + {"AESGCM", ~0u, ~0u, SSL_AES128GCM | SSL_AES256GCM, ~0u, 0, + /*include_deprecated=*/false}, + {"CHACHA20", ~0u, ~0u, SSL_CHACHA20POLY1305, ~0u, 0, + /*include_deprecated=*/false}, // MAC aliases {"SHA1", ~0u, ~0u, ~0u, SSL_SHA1, 0}, @@ -769,8 +773,8 @@ void SSLCipherPreferenceList::Remove(const SSL_CIPHER *cipher) { } bool ssl_cipher_is_deprecated(const SSL_CIPHER *cipher) { - // TODO(crbug.com/boringssl/599): Deprecate 3DES. - return cipher->id == TLS1_CK_ECDHE_RSA_WITH_AES_128_CBC_SHA256; + return cipher->id == TLS1_CK_ECDHE_RSA_WITH_AES_128_CBC_SHA256 || + cipher->algorithm_enc == SSL_3DES; } // ssl_cipher_apply_rule applies the rule type |rule| to ciphers matching its @@ -1070,8 +1074,6 @@ static bool ssl_cipher_process_rulestr(const char *rule_str, // can increase the set of matched ciphers. This is so that an alias // like "RSA" will only specifiy AES-based RSA ciphers, but // "RSA+3DES" will still specify 3DES. - // - // TODO(crbug.com/boringssl/599): Deprecate 3DES. alias.include_deprecated |= kCipherAliases[j].include_deprecated; if (alias.min_version != 0 && diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index be00e7c34..4f6a0e616 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -353,6 +353,81 @@ static const CipherTest kCipherTests[] = { // …but not in strict mode. true, }, + // 3DES ciphers are disabled by default. + { + "RSA", + { + {TLS1_CK_RSA_WITH_AES_128_GCM_SHA256, 0}, + {TLS1_CK_RSA_WITH_AES_256_GCM_SHA384, 0}, + {TLS1_CK_RSA_WITH_AES_128_SHA, 0}, + {TLS1_CK_RSA_WITH_AES_256_SHA, 0}, + }, + false, + }, + // But 3DES ciphers may be specified by name. + { + "TLS_RSA_WITH_3DES_EDE_CBC_SHA", + { + {SSL3_CK_RSA_DES_192_CBC3_SHA, 0}, + }, + false, + }, + { + "DES-CBC3-SHA", + { + {SSL3_CK_RSA_DES_192_CBC3_SHA, 0}, + }, + false, + }, + // Or by a selector that specifically includes deprecated ciphers. + { + "3DES", + { + {SSL3_CK_RSA_DES_192_CBC3_SHA, 0}, + }, + false, + }, + // Such selectors may be combined with other selectors that would otherwise + // not allow deprecated ciphers. + { + "RSA+3DES", + { + {SSL3_CK_RSA_DES_192_CBC3_SHA, 0}, + }, + false, + }, + // The cipher must still match all combined selectors, however. "ECDHE+3DES" + // matches nothing because we do not implement + // TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA. (The test includes + // TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 so the final list is not empty.) + { + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256:ECDHE+3DES", + { + {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0}, + }, + false, + }, + // Although alises like "RSA" do not match 3DES when adding ciphers, they do + // match it when removing ciphers. + { + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256:RSA:RSA+3DES:!RSA", + { + {TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 0}, + }, + false, + }, + // 3DES still participates in strength sorting. + { + "RSA:3DES:@STRENGTH", + { + {TLS1_CK_RSA_WITH_AES_256_GCM_SHA384, 0}, + {TLS1_CK_RSA_WITH_AES_256_SHA, 0}, + {TLS1_CK_RSA_WITH_AES_128_GCM_SHA256, 0}, + {TLS1_CK_RSA_WITH_AES_128_SHA, 0}, + {SSL3_CK_RSA_DES_192_CBC3_SHA, 0}, + }, + false, + }, }; static const char *kBadRules[] = { @@ -3192,39 +3267,39 @@ TEST(SSLTest, ClientHello) { uint16_t max_version; std::vector expected; } kTests[] = { - {TLS1_VERSION, - {0x16, 0x03, 0x01, 0x00, 0x5a, 0x01, 0x00, 0x00, 0x56, 0x03, 0x01, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0e, 0xc0, 0x09, - 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14, 0x00, 0x2f, 0x00, 0x35, 0x00, 0x0a, - 0x01, 0x00, 0x00, 0x1f, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, 0x00, 0x01, - 0x00, 0x00, 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, - 0x18, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00}}, - {TLS1_1_VERSION, - {0x16, 0x03, 0x01, 0x00, 0x5a, 0x01, 0x00, 0x00, 0x56, 0x03, 0x02, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0e, 0xc0, 0x09, - 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14, 0x00, 0x2f, 0x00, 0x35, 0x00, 0x0a, - 0x01, 0x00, 0x00, 0x1f, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, 0x00, 0x01, - 0x00, 0x00, 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, - 0x18, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00}}, - {TLS1_2_VERSION, - {0x16, 0x03, 0x01, 0x00, 0x82, 0x01, 0x00, 0x00, 0x7e, 0x03, 0x03, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1e, 0xcc, 0xa9, - 0xcc, 0xa8, 0xc0, 0x2b, 0xc0, 0x2f, 0xc0, 0x2c, 0xc0, 0x30, 0xc0, 0x09, - 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14, 0x00, 0x9c, 0x00, 0x9d, 0x00, 0x2f, - 0x00, 0x35, 0x00, 0x0a, 0x01, 0x00, 0x00, 0x37, 0x00, 0x17, 0x00, 0x00, - 0xff, 0x01, 0x00, 0x01, 0x00, 0x00, 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, - 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, - 0x23, 0x00, 0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x04, 0x03, 0x08, - 0x04, 0x04, 0x01, 0x05, 0x03, 0x08, 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, - 0x01, 0x02, 0x01}}, - // TODO(davidben): Add a change detector for TLS 1.3 once the spec and our - // implementation has settled enough that it won't change. + {TLS1_VERSION, + {0x16, 0x03, 0x01, 0x00, 0x58, 0x01, 0x00, 0x00, 0x54, 0x03, 0x01, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0c, 0xc0, 0x09, + 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14, 0x00, 0x2f, 0x00, 0x35, 0x01, 0x00, + 0x00, 0x1f, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, 0x00, 0x01, 0x00, 0x00, + 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00, + 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00}}, + {TLS1_1_VERSION, + {0x16, 0x03, 0x01, 0x00, 0x58, 0x01, 0x00, 0x00, 0x54, 0x03, 0x02, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0c, 0xc0, 0x09, + 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14, 0x00, 0x2f, 0x00, 0x35, 0x01, 0x00, + 0x00, 0x1f, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, 0x00, 0x01, 0x00, 0x00, + 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00, + 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00}}, + {TLS1_2_VERSION, + {0x16, 0x03, 0x01, 0x00, 0x80, 0x01, 0x00, 0x00, 0x7c, 0x03, 0x03, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1c, 0xcc, 0xa9, + 0xcc, 0xa8, 0xc0, 0x2b, 0xc0, 0x2f, 0xc0, 0x2c, 0xc0, 0x30, 0xc0, 0x09, + 0xc0, 0x13, 0xc0, 0x0a, 0xc0, 0x14, 0x00, 0x9c, 0x00, 0x9d, 0x00, 0x2f, + 0x00, 0x35, 0x01, 0x00, 0x00, 0x37, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, + 0x00, 0x01, 0x00, 0x00, 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, + 0x17, 0x00, 0x18, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, + 0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x04, 0x03, 0x08, 0x04, 0x04, + 0x01, 0x05, 0x03, 0x08, 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, 0x01, 0x02, + 0x01}}, + // TODO(davidben): Add a change detector for TLS 1.3 once the spec and our + // implementation has settled enough that it won't change. }; for (const auto &t : kTests) { @@ -5463,8 +5538,8 @@ TEST(SSLTest, ApplyHandoffRemovesUnsupportedCiphers) { ASSERT_TRUE(server); // handoff is a handoff message that has been artificially modified to pretend - // that only cipher 0x0A is supported. When it is applied to |server|, all - // ciphers but that one should be removed. + // that only TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f) is supported. When + // it is applied to |server|, all ciphers but that one should be removed. // // To make a new one of these, try sticking this in the |Handoff| test above: // @@ -5485,12 +5560,12 @@ TEST(SSLTest, ApplyHandoffRemovesUnsupportedCiphers) { 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x23, 0x00, 0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x04, 0x03, 0x08, 0x04, 0x04, 0x01, 0x05, 0x03, 0x08, - 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, 0x01, 0x02, 0x01, 0x04, 0x02, 0x00, - 0x0a, 0x04, 0x0a, 0x00, 0x15, 0x00, 0x17, 0x00, 0x18, 0x00, 0x19, 0x00, + 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, 0x01, 0x02, 0x01, 0x04, 0x02, 0xc0, + 0x2f, 0x04, 0x0a, 0x00, 0x15, 0x00, 0x17, 0x00, 0x18, 0x00, 0x19, 0x00, 0x1d, }; - EXPECT_EQ(20u, sk_SSL_CIPHER_num(SSL_get_ciphers(server.get()))); + EXPECT_LT(1u, sk_SSL_CIPHER_num(SSL_get_ciphers(server.get()))); ASSERT_TRUE( SSL_apply_handoff(server.get(), {handoff, OPENSSL_ARRAY_SIZE(handoff)})); EXPECT_EQ(1u, sk_SSL_CIPHER_num(SSL_get_ciphers(server.get()))); diff --git a/ssl/test/fuzzer.h b/ssl/test/fuzzer.h index 864c643e1..4888a39f8 100644 --- a/ssl/test/fuzzer.h +++ b/ssl/test/fuzzer.h @@ -414,7 +414,7 @@ class TLSFuzzer { SSL_CTX_enable_ocsp_stapling(ctx_.get()); // Enable versions and ciphers that are off by default. - if (!SSL_CTX_set_strict_cipher_list(ctx_.get(), "ALL")) { + if (!SSL_CTX_set_strict_cipher_list(ctx_.get(), "ALL:3DES")) { return false; } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 86daca272..da0795bbb 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -3199,7 +3199,7 @@ read alert 1 0 // elliptic curves, so no extensions are // involved. MaxVersion: VersionTLS12, - CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA}, + CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA}, Bugs: ProtocolBugs{ SendV2ClientHello: true, }, @@ -3221,7 +3221,7 @@ read alert 1 0 // elliptic curves, so no extensions are // involved. MaxVersion: VersionTLS12, - CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA}, + CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA}, Bugs: ProtocolBugs{ SendV2ClientHello: true, }, @@ -3683,6 +3683,11 @@ func addTestForCipherSuite(suite testCipherSuite, ver tlsVersion, protocol proto "-psk-identity", pskIdentity) } + if hasComponent(suite.name, "3DES") { + // BoringSSL disables 3DES ciphers by default. + flags = append(flags, "-cipher", "3DES") + } + var shouldFail bool if isTLS12Only(suite.name) && ver.version < VersionTLS12 { shouldFail = true @@ -4230,6 +4235,8 @@ func addCBCSplittingTests() { "-async", "-write-different-record-sizes", "-cbc-record-splitting", + // BoringSSL disables 3DES by default. + "-cipher", "ALL:3DES", }, }) testCases = append(testCases, testCase{ @@ -4248,6 +4255,8 @@ func addCBCSplittingTests() { "-write-different-record-sizes", "-cbc-record-splitting", "-partial-write", + // BoringSSL disables 3DES by default. + "-cipher", "ALL:3DES", }, }) } @@ -5771,7 +5780,7 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) { // elliptic curves, so no extensions are // involved. MaxVersion: VersionTLS12, - CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA}, + CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA}, Bugs: ProtocolBugs{ SendV2ClientHello: true, V2ClientHelloChallengeLength: challengeLength, @@ -9321,7 +9330,7 @@ func addRenegotiationTests() { renegotiate: 1, config: Config{ MaxVersion: VersionTLS12, - CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA}, + CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA}, }, renegotiateCiphers: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, flags: []string{ @@ -9336,7 +9345,7 @@ func addRenegotiationTests() { MaxVersion: VersionTLS12, CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, }, - renegotiateCiphers: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA}, + renegotiateCiphers: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA}, flags: []string{ "-renegotiate-freely", "-expect-total-renegotiations", "1", @@ -11353,7 +11362,7 @@ func addRSAClientKeyExchangeTests() { // version are different, to detect if the // server uses the wrong one. MaxVersion: VersionTLS11, - CipherSuites: []uint16{TLS_RSA_WITH_3DES_EDE_CBC_SHA}, + CipherSuites: []uint16{TLS_RSA_WITH_AES_128_CBC_SHA}, Bugs: ProtocolBugs{ BadRSAClientKeyExchange: bad, }, From f712c86eda36a59c5939879edda811c771990241 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 22 May 2023 16:13:08 -0400 Subject: [PATCH 2/2] Avoid locks in CRYPTO_free_ex_data Every time we free a type with ex_data (RSA, EC_KEY, DSA, SSL_CTX, SSL, SSL_SESSION, X509, X509_STORE), we allocate and take a read lock. The allocation means, if we believe in malloc failures, it is possible to leak memory on malloc failure. The read lock causes an unnecessary bit of contention writing to the cache line. Instead, since we never remove ex_data entries, just thread them in a singly-linked list. This way we only need to synchronize when to stop iterating. Add a counter to synchronize that. (Or we could make each 'next' pointers atomic, but this seemed more straightforward.) (I suspect this doesn't matter much, but it was shorter and we were already allocating the funcs structures anyway.) Bug: 570 Change-Id: Ie7ba5cc44f2b71ebd79c8971e784912d53af7f5c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60025 Reviewed-by: Adam Langley Commit-Queue: Adam Langley Auto-Submit: David Benjamin --- crypto/ex_data.c | 104 +++++++++++++++------------------------------- crypto/internal.h | 13 +++--- 2 files changed, 41 insertions(+), 76 deletions(-) diff --git a/crypto/ex_data.c b/crypto/ex_data.c index 867ced3c9..d34769f95 100644 --- a/crypto/ex_data.c +++ b/crypto/ex_data.c @@ -116,7 +116,6 @@ #include #include #include -#include #include #include "internal.h" @@ -128,14 +127,14 @@ struct crypto_ex_data_func_st { long argl; // Arbitary long void *argp; // Arbitary void pointer CRYPTO_EX_free *free_func; + // next points to the next |CRYPTO_EX_DATA_FUNCS| or NULL if this is the last + // one. It may only be read if synchronized with a read from |num_funcs|. + CRYPTO_EX_DATA_FUNCS *next; }; int CRYPTO_get_ex_new_index(CRYPTO_EX_DATA_CLASS *ex_data_class, int *out_index, long argl, void *argp, CRYPTO_EX_free *free_func) { - CRYPTO_EX_DATA_FUNCS *funcs; - int ret = 0; - - funcs = OPENSSL_malloc(sizeof(CRYPTO_EX_DATA_FUNCS)); + CRYPTO_EX_DATA_FUNCS *funcs = OPENSSL_malloc(sizeof(CRYPTO_EX_DATA_FUNCS)); if (funcs == NULL) { return 0; } @@ -143,37 +142,32 @@ int CRYPTO_get_ex_new_index(CRYPTO_EX_DATA_CLASS *ex_data_class, int *out_index, funcs->argl = argl; funcs->argp = argp; funcs->free_func = free_func; + funcs->next = NULL; CRYPTO_STATIC_MUTEX_lock_write(&ex_data_class->lock); - if (ex_data_class->meth == NULL) { - ex_data_class->meth = sk_CRYPTO_EX_DATA_FUNCS_new_null(); - } - - if (ex_data_class->meth == NULL) { - goto err; - } - + uint32_t num_funcs = CRYPTO_atomic_load_u32(&ex_data_class->num_funcs); // The index must fit in |int|. - if (sk_CRYPTO_EX_DATA_FUNCS_num(ex_data_class->meth) > - (size_t)(INT_MAX - ex_data_class->num_reserved)) { + if (num_funcs > (size_t)(INT_MAX - ex_data_class->num_reserved)) { OPENSSL_PUT_ERROR(CRYPTO, ERR_R_OVERFLOW); - goto err; + CRYPTO_STATIC_MUTEX_unlock_write(&ex_data_class->lock); + return 0; } - if (!sk_CRYPTO_EX_DATA_FUNCS_push(ex_data_class->meth, funcs)) { - goto err; + // Append |funcs| to the linked list. + if (ex_data_class->last == NULL) { + assert(num_funcs == 0); + ex_data_class->funcs = funcs; + ex_data_class->last = funcs; + } else { + ex_data_class->last->next = funcs; + ex_data_class->last = funcs; } - funcs = NULL; // |sk_CRYPTO_EX_DATA_FUNCS_push| takes ownership. - *out_index = (int)sk_CRYPTO_EX_DATA_FUNCS_num(ex_data_class->meth) - 1 + - ex_data_class->num_reserved; - ret = 1; - -err: + CRYPTO_atomic_store_u32(&ex_data_class->num_funcs, num_funcs + 1); CRYPTO_STATIC_MUTEX_unlock_write(&ex_data_class->lock); - OPENSSL_free(funcs); - return ret; + *out_index = (int)num_funcs + ex_data_class->num_reserved; + return 1; } int CRYPTO_set_ex_data(CRYPTO_EX_DATA *ad, int index, void *val) { @@ -209,33 +203,6 @@ void *CRYPTO_get_ex_data(const CRYPTO_EX_DATA *ad, int idx) { return sk_void_value(ad->sk, idx); } -// get_func_pointers takes a copy of the CRYPTO_EX_DATA_FUNCS pointers, if any, -// for the given class. If there are some pointers, it sets |*out| to point to -// a fresh stack of them. Otherwise it sets |*out| to NULL. It returns one on -// success or zero on error. -static int get_func_pointers(STACK_OF(CRYPTO_EX_DATA_FUNCS) **out, - CRYPTO_EX_DATA_CLASS *ex_data_class) { - size_t n; - - *out = NULL; - - // CRYPTO_EX_DATA_FUNCS structures are static once set, so we can take a - // shallow copy of the list under lock and then use the structures without - // the lock held. - CRYPTO_STATIC_MUTEX_lock_read(&ex_data_class->lock); - n = sk_CRYPTO_EX_DATA_FUNCS_num(ex_data_class->meth); - if (n > 0) { - *out = sk_CRYPTO_EX_DATA_FUNCS_dup(ex_data_class->meth); - } - CRYPTO_STATIC_MUTEX_unlock_read(&ex_data_class->lock); - - if (n > 0 && *out == NULL) { - return 0; - } - - return 1; -} - void CRYPTO_new_ex_data(CRYPTO_EX_DATA *ad) { ad->sk = NULL; } @@ -247,26 +214,21 @@ void CRYPTO_free_ex_data(CRYPTO_EX_DATA_CLASS *ex_data_class, void *obj, return; } - STACK_OF(CRYPTO_EX_DATA_FUNCS) *func_pointers; - if (!get_func_pointers(&func_pointers, ex_data_class)) { - // TODO(davidben): This leaks memory on malloc error. - return; - } - + uint32_t num_funcs = CRYPTO_atomic_load_u32(&ex_data_class->num_funcs); // |CRYPTO_get_ex_new_index| will not allocate indices beyond |INT_MAX|. - assert(sk_CRYPTO_EX_DATA_FUNCS_num(func_pointers) <= - (size_t)(INT_MAX - ex_data_class->num_reserved)); - for (int i = 0; i < (int)sk_CRYPTO_EX_DATA_FUNCS_num(func_pointers); i++) { - CRYPTO_EX_DATA_FUNCS *func_pointer = - sk_CRYPTO_EX_DATA_FUNCS_value(func_pointers, i); - if (func_pointer->free_func) { - void *ptr = CRYPTO_get_ex_data(ad, i + ex_data_class->num_reserved); - func_pointer->free_func(obj, ptr, ad, i + ex_data_class->num_reserved, - func_pointer->argl, func_pointer->argp); - } - } + assert(num_funcs <= (size_t)(INT_MAX - ex_data_class->num_reserved)); - sk_CRYPTO_EX_DATA_FUNCS_free(func_pointers); + // Defer dereferencing |ex_data_class->funcs| and |funcs->next|. It must come + // after the |num_funcs| comparison to be correctly synchronized. + CRYPTO_EX_DATA_FUNCS *const *funcs = &ex_data_class->funcs; + for (uint32_t i = 0; i < num_funcs; i++) { + if ((*funcs)->free_func != NULL) { + int index = (int)i + ex_data_class->num_reserved; + void *ptr = CRYPTO_get_ex_data(ad, index); + (*funcs)->free_func(obj, ptr, ad, index, (*funcs)->argl, (*funcs)->argp); + } + funcs = &(*funcs)->next; + } sk_void_free(ad->sk); ad->sk = NULL; diff --git a/crypto/internal.h b/crypto/internal.h index 4b7d82c04..d15f7534b 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -850,22 +850,25 @@ OPENSSL_EXPORT int CRYPTO_set_thread_local( typedef struct crypto_ex_data_func_st CRYPTO_EX_DATA_FUNCS; -DECLARE_STACK_OF(CRYPTO_EX_DATA_FUNCS) - // CRYPTO_EX_DATA_CLASS tracks the ex_indices registered for a type which // supports ex_data. It should defined as a static global within the module // which defines that type. typedef struct { struct CRYPTO_STATIC_MUTEX lock; - STACK_OF(CRYPTO_EX_DATA_FUNCS) *meth; + // funcs is a linked list of |CRYPTO_EX_DATA_FUNCS| structures. It may be + // traversed without serialization only up to |num_funcs|. last points to the + // final entry of |funcs|, or NULL if empty. + CRYPTO_EX_DATA_FUNCS *funcs, *last; + // num_funcs is the number of entries in |funcs|. + CRYPTO_atomic_u32 num_funcs; // num_reserved is one if the ex_data index zero is reserved for legacy // |TYPE_get_app_data| functions. uint8_t num_reserved; } CRYPTO_EX_DATA_CLASS; -#define CRYPTO_EX_DATA_CLASS_INIT {CRYPTO_STATIC_MUTEX_INIT, NULL, 0} +#define CRYPTO_EX_DATA_CLASS_INIT {CRYPTO_STATIC_MUTEX_INIT, NULL, NULL, 0, 0} #define CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA \ - {CRYPTO_STATIC_MUTEX_INIT, NULL, 1} + {CRYPTO_STATIC_MUTEX_INIT, NULL, NULL, 0, 1} // CRYPTO_get_ex_new_index allocates a new index for |ex_data_class| and writes // it to |*out_index|. Each class of object should provide a wrapper function