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.

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

Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites #2092

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 17 additions & 5 deletions include/openssl/ssl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1690,15 +1690,26 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out,
// Once an equal-preference group is used, future directives must be
// opcode-less. Inside an equal-preference group, spaces are not allowed.
//
// 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.
// Note: TLS 1.3 ciphersuites are only configurable via
// |SSL_[CTX]_set_ciphersuites|. Other setter functions have no impact on
Copy link
Contributor

Choose a reason for hiding this comment

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

In the event we publish the docs in the future please spell out the two methods so they link correctly.

// TLS 1.3 ciphersuites.

// SSL_DEFAULT_CIPHER_LIST is the default cipher suite configuration. It is
// substituted when a cipher string starts with 'DEFAULT'.
#define SSL_DEFAULT_CIPHER_LIST "ALL"

// TLS13_DEFAULT_CIPHER_LIST_AES_HW is the default TLS 1.3 cipher suite
// configuration when AES hardware acceleration is enabled.
#define TLS13_DEFAULT_CIPHER_LIST_AES_HW "TLS_AES_128_GCM_SHA256:" \
"TLS_AES_256_GCM_SHA384:" \
"TLS_CHACHA20_POLY1305_SHA256"

// TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW is the default TLS 1.3 cipher suite
// configuration when no AES hardware acceleration is enabled.
#define TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW "TLS_CHACHA20_POLY1305_SHA256:" \
"TLS_AES_128_GCM_SHA256:" \
"TLS_AES_256_GCM_SHA384"
Comment on lines +1701 to +1711
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be in the public header? Do we expect customer to pass these back to us? Also how did you come up with these options?


// SSL_CTX_set_strict_cipher_list configures the cipher list for |ctx|,
// evaluating |str| as a cipher string and returning error if |str| contains
// anything meaningless. It returns one on success and zero on failure.
Expand All @@ -1714,7 +1725,8 @@ OPENSSL_EXPORT int SSL_CTX_set_strict_cipher_list(SSL_CTX *ctx,
// behavior is strongly advised against and only meant for OpenSSL
// compatibility.
//
// Note: this API only sets the TLSv1.2 and below ciphers.
// Note: this API only sets the TLSv1.2 and below ciphers. If unchanged,
// the default TLS 1.3 ciphersuites are pulled in as well.
// Use |SSL_CTX_set_ciphersuites| to configure TLS 1.3 specific ciphers.
OPENSSL_EXPORT int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str);

Expand Down
6 changes: 5 additions & 1 deletion ssl/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -714,10 +714,14 @@ const EVP_MD *ssl_get_handshake_digest(uint16_t version,
// rejected. If false, nonsense will be silently ignored. If |config_tls13| is
// true, only TLS 1.3 ciphers are considered in |ssl_cipher_collect_ciphers|. If
// false, TLS 1.2 and below ciphers participate in |ssl_cipher_collect_ciphers|.
// In every invocation, |ctx->cipher_list| is updated with any user-configured
// or default TLS 1.3 cipher suites in |ctx->tls13_cipher_list|.
//
// An empty result is considered an error regardless of |strict| or
// |config_tls13|. |has_aes_hw| indicates if the list should be ordered based on
// having support for AES in hardware or not.
bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
bool ssl_create_cipher_list(SSL_CTX *ctx,
UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
const bool has_aes_hw, const char *rule_str,
bool strict, bool config_tls13);

Expand Down
86 changes: 85 additions & 1 deletion ssl/ssl_cipher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1234,7 +1234,88 @@ static bool is_known_default_alias_keyword_filter_rule(const char *rule,
return false;
}

bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
// update_cipher_list updates |ctx->cipher_list| by:
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment explains what this function does, but why do we need to remove everything and then create a new list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't necessarily know whether ctx->cipher_list is getting updated or ctx->tls13_cipher_list. When ctx->tls13_cipher_list is updated, we need to remove old TLS 1.3 ciphersuites from ctx->cipher_list and add the new ones in. Note, we are only removing and adding TLS 1.3 ciphersuites here, not the entire list.

// 1. Removing any existing TLS 1.3 ciphersuites
// 2. Adding configured ciphersuites from |ctx->tls13_cipher_list|
// 3. Configuring a new |ctx->cipher_list->in_group_flags|
// This function maintains the ordering of ciphersuites and places TLS 1.3
// ciphersuites at the front of the list.
Comment on lines +1241 to +1242
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the order of TLS 1.3 and the < 1.3 ciphersuites affect what gets negotiated?

// Returns one on success and zero on failure.
static int update_cipher_list(SSL_CTX *ctx) {
bssl::UniquePtr<STACK_OF(SSL_CIPHER)> tmp_cipher_list;
int num_removed_tls13_ciphers = 0, num_added_tls13_ciphers = 0;
Array<bool> updated_in_group_flags;

if (ctx->cipher_list && ctx->cipher_list->ciphers) {
tmp_cipher_list.reset(sk_SSL_CIPHER_dup(ctx->cipher_list->ciphers.get()));
} else {
tmp_cipher_list.reset(sk_SSL_CIPHER_new_null());
}

if (tmp_cipher_list == NULL) {
return 0;
}

// Delete any existing TLSv1.3 ciphersuites. These will be first in the list
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this enforced by AWS-LC or could a customer pass in a list with the wrong order?

while (sk_SSL_CIPHER_num(tmp_cipher_list.get()) > 0 &&
SSL_CIPHER_get_min_version(sk_SSL_CIPHER_value(tmp_cipher_list.get(), 0))
== TLS1_3_VERSION) {
sk_SSL_CIPHER_delete(tmp_cipher_list.get(), 0);
num_removed_tls13_ciphers++;
}

int num_updated_tls12_ciphers = sk_SSL_CIPHER_num(tmp_cipher_list.get());

// Insert the new TLSv1.3 ciphersuites while maintaining original order
if (ctx->tls13_cipher_list != NULL && ctx->tls13_cipher_list->ciphers != NULL) {
STACK_OF(SSL_CIPHER) *tls13_cipher_stack = ctx->tls13_cipher_list->ciphers.get();
num_added_tls13_ciphers = sk_SSL_CIPHER_num(tls13_cipher_stack);
for (int i = sk_SSL_CIPHER_num(tls13_cipher_stack) - 1; i >= 0; i--) {
const SSL_CIPHER *tls13_cipher = sk_SSL_CIPHER_value(tls13_cipher_stack, i);
if (!sk_SSL_CIPHER_unshift(tmp_cipher_list.get(), tls13_cipher)) {
return 0;
}
}
}

if (!updated_in_group_flags.Init(num_added_tls13_ciphers + num_updated_tls12_ciphers)) {
return 0;
}
std::fill(updated_in_group_flags.begin(), updated_in_group_flags.end(), false);

// Copy in_group_flags from |ctx->tls13_cipher_list|
if (ctx->tls13_cipher_list && ctx->tls13_cipher_list->in_group_flags) {
const auto& tls13_flags = ctx->tls13_cipher_list->in_group_flags;
// Ensure value of last element in |in_group_flags| is 0. The last cipher
// in a list must be the end of any group in that list.
if (tls13_flags[num_added_tls13_ciphers - 1] != 0) {
tls13_flags[num_added_tls13_ciphers - 1] = false;
}
for (int i = 0; i < num_added_tls13_ciphers; i++) {
updated_in_group_flags[i] = tls13_flags[i];
}
}

// Copy remaining in_group_flags from |ctx->cipher_list|
if (ctx->cipher_list && ctx->cipher_list->in_group_flags) {
for (int i = 0; i < num_updated_tls12_ciphers; i++) {
updated_in_group_flags[i + num_added_tls13_ciphers] =
ctx->cipher_list->in_group_flags[i + num_removed_tls13_ciphers];
}
}

Span<const bool> flags_span(updated_in_group_flags.data(), updated_in_group_flags.size());
ctx->cipher_list = MakeUnique<SSLCipherPreferenceList>();
if (ctx->cipher_list == NULL) {
return 0;
}
ctx->cipher_list->Init(std::move(tmp_cipher_list), flags_span);

return 1;
}

bool ssl_create_cipher_list(SSL_CTX *ctx,
UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
const bool has_aes_hw, const char *rule_str,
bool strict, bool config_tls13) {
// Return with error if nothing to do.
Expand Down Expand Up @@ -1367,6 +1448,9 @@ bool ssl_create_cipher_list(UniquePtr<SSLCipherPreferenceList> *out_cipher_list,
return false;
}

// Update |ctx->cipher_list| with any ciphers in |ctx->tls13_cipher_list|
update_cipher_list(ctx);

return true;
}

Expand Down
28 changes: 19 additions & 9 deletions ssl/ssl_lib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,16 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) {
return nullptr;
}

if (!SSL_CTX_set_strict_cipher_list(ret.get(), SSL_DEFAULT_CIPHER_LIST) ||
const bool has_aes_hw = EVP_has_aes_hardware();
const char *cipher_rule;
if (has_aes_hw) {
cipher_rule = TLS13_DEFAULT_CIPHER_LIST_AES_HW;
} else {
cipher_rule = TLS13_DEFAULT_CIPHER_LIST_NO_AES_HW;
}

if (!SSL_CTX_set_ciphersuites(ret.get(), cipher_rule) ||
!SSL_CTX_set_strict_cipher_list(ret.get(), SSL_DEFAULT_CIPHER_LIST) ||
Comment on lines +589 to +598
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this changing our behavior? From the PR description:

SSL_CTX_new now configures ctx->tls13_cipher_suites with default TLS 1.3 ciphers (did not previously).

What did we do before if you attempted to connect to a TLS 1.3 enabled endpoint?

// Lock the SSL_CTX to the specified version, for compatibility with
// legacy uses of SSL_METHOD.
!SSL_CTX_set_max_proto_version(ret.get(), method->version) ||
Expand Down Expand Up @@ -2178,15 +2187,15 @@ const char *SSL_get_cipher_list(const SSL *ssl, int n) {
int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str) {
const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
: EVP_has_aes_hardware();
return ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
return ssl_create_cipher_list(ctx, &ctx->cipher_list, has_aes_hw, str,
false /* not strict */,
false /* don't configure TLSv1.3 ciphers */);
}

int SSL_CTX_set_strict_cipher_list(SSL_CTX *ctx, const char *str) {
const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
: EVP_has_aes_hardware();
return ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
return ssl_create_cipher_list(ctx, &ctx->cipher_list, has_aes_hw, str,
true /* strict */,
false /* don't configure TLSv1.3 ciphers */);
}
Expand All @@ -2198,15 +2207,16 @@ int SSL_set_cipher_list(SSL *ssl, const char *str) {
const bool has_aes_hw = ssl->config->aes_hw_override
? ssl->config->aes_hw_override_value
: EVP_has_aes_hardware();
return ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
return ssl_create_cipher_list(ssl->ctx.get(), &ssl->config->cipher_list, has_aes_hw, str,
false /* not strict */,
false /* don't configure TLSv1.3 ciphers */);
}

int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str) {
const bool has_aes_hw = ctx->aes_hw_override ? ctx->aes_hw_override_value
: EVP_has_aes_hardware();
return ssl_create_cipher_list(&ctx->tls13_cipher_list, has_aes_hw, str,

return ssl_create_cipher_list(ctx, &ctx->tls13_cipher_list, has_aes_hw, str,
false /* not strict */,
true /* only configure TLSv1.3 ciphers */);
}
Expand All @@ -2218,8 +2228,8 @@ int SSL_set_ciphersuites(SSL *ssl, const char *str) {
const bool has_aes_hw = ssl->config->aes_hw_override
? ssl->config->aes_hw_override_value
: EVP_has_aes_hardware();
return ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
false /* not strict */,
return ssl_create_cipher_list(ssl->ctx.get(), &ssl->config->cipher_list,
has_aes_hw, str, false /* not strict */,
true /* configure TLSv1.3 ciphers */);
}

Expand All @@ -2230,8 +2240,8 @@ int SSL_set_strict_cipher_list(SSL *ssl, const char *str) {
const bool has_aes_hw = ssl->config->aes_hw_override
? ssl->config->aes_hw_override_value
: EVP_has_aes_hardware();
return ssl_create_cipher_list(&ssl->config->cipher_list, has_aes_hw, str,
true /* strict */,
return ssl_create_cipher_list(ssl->ctx.get(), &ssl->config->cipher_list,
has_aes_hw, str, true /* strict */,
false /* don't configure TLSv1.3 ciphers */);
}

Expand Down
99 changes: 94 additions & 5 deletions ssl/ssl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1616,6 +1616,11 @@ TEST(SSLTest, CipherRules) {
bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(ctx);

// We configure default TLS 1.3 ciphersuites in |SSL_CTX| which pollute
// |ctx->cipher_list|. Set it to an empty list so we can test TLS 1.2
// configurations.
ASSERT_TRUE(SSL_CTX_set_ciphersuites(ctx.get(), ""));

// Test lax mode.
ASSERT_TRUE(SSL_CTX_set_cipher_list(ctx.get(), t.rule));
EXPECT_TRUE(
Expand Down Expand Up @@ -1654,6 +1659,11 @@ TEST(SSLTest, CipherRules) {
bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(ctx);

// We configure default TLS 1.3 ciphersuites in |SSL_CTX| which pollute
// |ctx->cipher_list|. Set it to an empty list so we can test TLS 1.2
// configurations.
ASSERT_TRUE(SSL_CTX_set_ciphersuites(ctx.get(), ""));

EXPECT_FALSE(SSL_CTX_set_cipher_list(ctx.get(), t.rule));
ASSERT_EQ(ERR_GET_REASON(ERR_get_error()), SSL_R_NO_CIPHER_MATCH);
ERR_clear_error();
Expand Down Expand Up @@ -2823,6 +2833,85 @@ TEST(SSLTest, FindingCipher) {
ASSERT_FALSE(cipher3);
}

TEST(SSLTest, SSLGetCiphersReturnsTLS13Default) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
bssl::UniquePtr<SSL_CTX> server_ctx =
CreateContextWithTestCertificate(TLS_method());
ASSERT_TRUE(client_ctx);
ASSERT_TRUE(server_ctx);
// Configure only TLS 1.3.
ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_3_VERSION));
ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_3_VERSION));
ASSERT_TRUE(SSL_CTX_set_min_proto_version(server_ctx.get(), TLS1_3_VERSION));
ASSERT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), TLS1_3_VERSION));

bssl::UniquePtr<SSL> client, server;
// Have to ensure config is not shed per current implementation of SSL_get_ciphers.
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), server_ctx.get(),
ClientConfig(), false));

// Ensure default TLS 1.3 Ciphersuites are present
const SSL_CIPHER *cipher1 = SSL_get_cipher_by_value(TLS1_3_CK_AES_128_GCM_SHA256 & 0xFFFF);
ASSERT_TRUE(cipher1);
const SSL_CIPHER *cipher2 = SSL_get_cipher_by_value(TLS1_3_CK_AES_256_GCM_SHA384 & 0xFFFF);
ASSERT_TRUE(cipher2);
const SSL_CIPHER *cipher3 = SSL_get_cipher_by_value(TLS1_3_CK_CHACHA20_POLY1305_SHA256 & 0xFFFF);
ASSERT_TRUE(cipher3);

STACK_OF(SSL_CIPHER) *client_ciphers = SSL_get_ciphers(client.get());
STACK_OF(SSL_CIPHER) *server_ciphers = SSL_get_ciphers(server.get());

ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(client_ciphers, NULL, cipher1));
ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(client_ciphers, NULL, cipher2));
ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(client_ciphers, NULL, cipher3));

ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(server_ciphers, NULL, cipher1));
ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(server_ciphers, NULL, cipher2));
ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(server_ciphers, NULL, cipher3));
}

TEST(SSLTest, SSLGetCiphersReturnsTLS13Custom) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
bssl::UniquePtr<SSL_CTX> server_ctx =
CreateContextWithTestCertificate(TLS_method());
ASSERT_TRUE(client_ctx);
ASSERT_TRUE(server_ctx);

// Configure custom TLS 1.3 Ciphersuites
SSL_CTX_set_ciphersuites(server_ctx.get(), "TLS_AES_128_GCM_SHA256");
SSL_CTX_set_ciphersuites(client_ctx.get(), "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384");

// Configure only TLS 1.3.
ASSERT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_3_VERSION));
ASSERT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_3_VERSION));
ASSERT_TRUE(SSL_CTX_set_min_proto_version(server_ctx.get(), TLS1_3_VERSION));
ASSERT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), TLS1_3_VERSION));

bssl::UniquePtr<SSL> client, server;
// Have to ensure config is not shed per current implementation of SSL_get_ciphers.
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), server_ctx.get(),
ClientConfig(), false));

// Ensure default TLS 1.3 Ciphersuites are present
const SSL_CIPHER *cipher1 = SSL_get_cipher_by_value(TLS1_3_CK_AES_128_GCM_SHA256 & 0xFFFF);
ASSERT_TRUE(cipher1);
const SSL_CIPHER *cipher2 = SSL_get_cipher_by_value(TLS1_3_CK_AES_256_GCM_SHA384 & 0xFFFF);
ASSERT_TRUE(cipher2);
const SSL_CIPHER *cipher3 = SSL_get_cipher_by_value(TLS1_3_CK_CHACHA20_POLY1305_SHA256 & 0xFFFF);
ASSERT_TRUE(cipher3);

STACK_OF(SSL_CIPHER) *client_ciphers = SSL_get_ciphers(client.get());
STACK_OF(SSL_CIPHER) *server_ciphers = SSL_get_ciphers(server.get());

ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(client_ciphers, NULL, cipher1));
ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(client_ciphers, NULL, cipher2));
ASSERT_FALSE(sk_SSL_CIPHER_find_awslc(client_ciphers, NULL, cipher3));

ASSERT_TRUE(sk_SSL_CIPHER_find_awslc(server_ciphers, NULL, cipher1));
ASSERT_FALSE(sk_SSL_CIPHER_find_awslc(server_ciphers, NULL, cipher2));
ASSERT_FALSE(sk_SSL_CIPHER_find_awslc(server_ciphers, NULL, cipher3));
}

TEST(SSLTest, GetClientCiphersAfterHandshakeFailure1_3) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
bssl::UniquePtr<SSL_CTX> server_ctx = CreateContextWithTestCertificate(TLS_method());
Expand Down Expand Up @@ -6412,17 +6501,17 @@ 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 with |SSL_CTX_set_ciphersuites| works
EXPECT_TRUE(SSL_CTX_set_ciphersuites(ctx.get(), ""));
EXPECT_EQ(0u, sk_SSL_CIPHER_num(ctx->tls13_cipher_list->ciphers.get()));
EXPECT_NE(0u, sk_SSL_CIPHER_num(SSL_CTX_get_ciphers(ctx.get())));

// 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())));

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