-
Notifications
You must be signed in to change notification settings - Fork 122
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
base: main
Are you sure you want to change the base?
Changes from all commits
bc7b951
e80cb6e
c49f32d
bfb369f
ef0f455
aacb978
b6f297b
749513a
1e818d5
425e9a5
656b710
2622e1e
fe430f4
72603cc
eb28574
5fcdd4a
d4e5047
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
// 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this changing our behavior? From the PR description:
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) || | ||
|
@@ -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 */); | ||
} | ||
|
@@ -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 */); | ||
} | ||
|
@@ -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 */); | ||
} | ||
|
||
|
@@ -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 */); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.