Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow empty lists in SSL_CTX_set_ciphersuites #1511

Merged
merged 4 commits into from
Apr 10, 2024
Merged

allow empty lists in SSL_CTX_set_ciphersuites #1511

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e961c2
allow empty lists in SSL_CTX_set_ciphersuites
samuel40791765 Mar 26, 2024
a062de4
forgot about SSL_CTX_set_ciphersuites
samuel40791765 Mar 27, 2024
81089b3
Merge branch 'main' into ssl-string-empty
skmcgrail Mar 28, 2024
fae78ed
add documentation to functions
samuel40791765 Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ssl/ssl_cipher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,9 +1336,11 @@ bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,

*out_cipher_list = std::move(pref_list);

// Configuring an empty cipher list is an error but still updates the
// output.
if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers.get()) == 0) {
// Configuring an empty cipher list is an error when |strict| is true, but
// still updates the output. When otherwise, OpenSSL explicitly allows an
// empty list.
if ((strict || (*rule_str != '\0')) &&
sk_SSL_CIPHER_num((*out_cipher_list)->ciphers.get()) == 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH);
return false;
}
Expand Down
20 changes: 13 additions & 7 deletions ssl/ssl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,8 +548,6 @@ static const char *kBadRules[] = {
"[+RSA]",
// Unknown directive.
"@BOGUS",
// Empty cipher lists error at SSL_CTX_set_cipher_list.
"",
"BOGUS",
// COMPLEMENTOFDEFAULT is empty.
"COMPLEMENTOFDEFAULT",
Expand Down Expand Up @@ -5757,12 +5755,20 @@ TEST(SSLTest, EmptyCipherList) {
// Initially, the cipher list is not empty.
EXPECT_NE(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get())));

// Configuring the empty cipher list fails.
EXPECT_FALSE(SSL_CTX_set_cipher_list(ctx.get(), ""));
ERR_clear_error();
// Configuring the empty cipher list with |SSL_CTX_set_cipher_list|
// succeeds.
EXPECT_TRUE(SSL_CTX_set_cipher_list(ctx.get(), ""));
// The cipher list is updated to empty.
EXPECT_EQ(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get())));

// Configuring the empty cipher list with |SSL_CTX_set_ciphersuites|
// also succeeds.
EXPECT_TRUE(SSL_CTX_set_ciphersuites(ctx.get(), ""));
EXPECT_EQ(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get())));
Comment on lines +5764 to +5767
Copy link

@dlenski dlenski Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is good.

Although this doesn't fully verify the pattern used by MySQL+MariaDB with OpenSSL, which is as follows…

  1. Call both:
    • SSL_CTX_set_cipher_list("NON-EMPTY list for TLSv1.2")
    • and SSL_CTX_set_ciphersuites("") // empty list for TLSv1.3
  2. Expect that there will be a non-zero number of TLSv1.2 (or earlier) ciphers available for new connections, and zero TLSv1.3 ciphersuites available 😅

As described in openssl/openssl#13704 (comment), it seems like it's quite complicated to get a list of only the TLSv1.3 ciphersuites or only the TLSv1.2 ciphers ☹️

Copy link
Contributor Author

@samuel40791765 samuel40791765 Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an integration build with mysql that runs against the tests that consume this behavior. We're in the process of updating to 8.3


// Configuring the empty cipher list fails.
EXPECT_FALSE(SSL_CTX_set_ciphersuites(ctx.get(), ""));
Comment on lines -5764 to -5765
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of deleting this check for failure… should it be replaced with an explicit check for success of setting the (TLSv1.3) ciphersuite list to an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit check for success for SSL[_CTX]_set_cipher_list is on L5758-5763 in the new change :)
But I just realized that I forgot about SSL[_CTX]_set_ciphersuites...

// Configuring the empty cipher list with |SSL_CTX_set_strict_cipher_list|
// fails.
EXPECT_FALSE(SSL_CTX_set_strict_cipher_list(ctx.get(), ""));
ERR_clear_error();

// But the cipher list is still updated to empty.
Expand Down
Loading