Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers #2092

smittals2 · 2025-01-02T23:34:41Z

Issues:

CryptoAlg-2559

Description of changes:

SSL_CTX_new now configures ctx->tls13_cipher_suites with default TLS 1.3 ciphers (did not previously). The default preference order for TLS 1.3 ciphersuites differs based on whether AES Hardware acceleration is enabled in AWS-LC, we match this behavior in SSL_CTX_new.
The SSL_CONFIG object holds it's own TLS 1.3 ciphersuites now
Server side and client side cipher selection logic is modified to account for the new field in SSL_CONFIG
Each time a cipher list is configured either on SSL_CTX or SSL_CONFIG, the main cipher_list is updated with values from its own tls13_cipher_suites via a new function update_cipher_list. This function also updates in_group_flags accordingly. This means, TLS 1.3 ciphersuites are stored seperately while the main cipher_list var holds ALL ciphersuites in both objects
Outdated comments were updated

Call-outs:

Note SSL_CTX generally has a longer lifetime than an SSL object. SSL_CONFIG objects live on SSL objects. Both SSL_CTX and SSL_CONFIG have their own cipher_list and tls13_cipher_list fields. We support SSL_[CTX]_set_... APIs to set these fields.

In general, our TLS implementations pick cipher suites against the given connection constraints/parameters. Adding TLS 1.3 ciphersuites to ctx->cipher_list or config->cipher_list does not modify this behavior or introduce new problems.

Testing:

Added new testing to validate cipher selection behavior between SSL_CTX and SSL_CONFIG objects
Added change detection test for TLS 1.3 Client Hello
Updated old tests and to-do's regarding TLS 1.3
Testing to ensure SSL_get_ciphers returns ALL ciphersuites instead of TLS 1.2 and below.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

… tls 1.3

codecov-commenter · 2025-01-06T18:18:38Z

Codecov Report

Attention: Patch coverage is 93.33333% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.98%. Comparing base (29be983) to head (2c6d018).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
ssl/ssl_cipher.cc	88.09%	5 Missing ⚠️
ssl/ssl_lib.cc	93.93%	2 Missing ⚠️
ssl/ssl_test.cc	95.91%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2092      +/-   ##
==========================================
+ Coverage   78.95%   78.98%   +0.03%     
==========================================
  Files         610      610              
  Lines      105293   105381      +88     
  Branches    14911    14937      +26     
==========================================
+ Hits        83129    83231     +102     
+ Misses      21511    21499      -12     
+ Partials      653      651       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

include/openssl/ssl.h

ssl/ssl_lib.cc

ssl/ssl_cipher.cc

ssl/handshake_client.cc

include/openssl/ssl.h

ssl/ssl_cipher.cc

andrewhop · 2025-01-24T00:08:41Z

include/openssl/ssl.h

+// Note: TLS 1.3 ciphersuites are only configurable via
+// |SSL_CTX_set_ciphersuites| or |SSL_set_ciphersuites|. Other setter functions have
+// no impact on TLS 1.3 ciphersuites.


Do we have tests that attempt to set 1.3 ciphers and ensure they are rejected?

Do you mean use the TLS 1.2 setter functions to set TLS 1.3 ciphers? If so yes you get a no cipher match error.

ssl/handshake_client.cc

andrewhop · 2025-01-24T00:18:17Z

ssl/handshake_client.cc

+
+    if (!collect_cipher_protocol_ids(ciphers, &child, mask_k,
      mask_a, hs->max_version, hs->min_version, &any_enabled)) {
      return false;
    }


Why are you filtering the TLS 1.3 ciphers again here? From line 276 we already know we want 1.3 ciphers and tls13_cipher_list only has 1.3 ciphers in it.

Actually can these two blocks be combined into a single flow:

Call SSL_get_ciphers to get all 1.2 and 1.3 ciphers

Filter that based on min/max

Write that to the CBB

Cannot combine the two blocks. Different cases for ECH (discussed offline)

I agree you can't combine the two blocks into one shared block, but I'm still curious about the filtering, can this just use ciphers from up above which only includes 1.3 ciphers?

Are you saying use the same SSL_get_ciphers call below as well?

The reason we can't do this is if you are a ECH ClientHelloInner, and you support TLS versions less than 1.3, by RFC we still only send TLS 1.3 ciphersuites. Since collect_ciphers takes min max version from the handshake, using the above ciphers var would erroneously pull in TLS 1.2 and below ciphersuites. Therefore, we specifically pass in only the tls13_ciphersuites in the else block

andrewhop · 2025-01-24T00:24:12Z

ssl/handshake_client.cc

+    if (!collect_cipher_protocol_ids(SSL_get_ciphers(ssl), &child, mask_k,
      mask_a, hs->max_version, hs->min_version, &any_enabled)) {
      return false;


Double checking my understanding: this is in an if/else block so only hs->min_version < TLS1_3_VERSION or hs->max_version >= TLS1_3_VERSION will run. If min is less than 1.3 we want to add both, and SSL_get_ciphers returns both?

Yes if min is less than 1.3, we want to add min to max (where max could also be anything). We pass this in to collect_cipher_protocol_ids.

SSL_get_ciphers will return the SSL level (if configured) and otherwise SSL_CTX main cipher list which now holds all ciphersuites.

andrewhop · 2025-01-24T00:25:01Z

include/openssl/ssl.h

@@ -1690,10 +1690,9 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out,
 // Once an equal-preference group is used, future directives must be


Please update SSL_get_ciphers documentation to note it returns all ciphers 1.2 and 1.3

The current documentation covers this case ("cipher list for |ssl|") but I'll make it more explicit. We just weren't living up to this contract initially.

ssl/ssl_cipher.cc

andrewhop · 2025-01-24T00:38:02Z

ssl/ssl_cipher.cc

+  }
+  std::fill(updated_in_group_flags.begin(), updated_in_group_flags.end(), false);
+
+  // Copy in_group_flags from |ctx->tls13_cipher_list|


What does in_group_flags do?

https://github.com/aws/aws-lc/blob/81f138a0632151e24ecbf3e0b08c37d6f82f57ed/ssl/internal.h#L648C1-L677C8

andrewhop · 2025-01-24T00:39:25Z

ssl/ssl_cipher.cc

+    // 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;
+    }


Should this fail here instead? If this happens does that mean something happened up above and the wrong number of things got added?

Can a group include a 1.3 and 1.2 cipher suite?

No, you can only group ciphersuites within the same TLS version

andrewhop · 2025-01-24T00:59:36Z

ssl/ssl_lib.cc

+  if (!ssl_create_cipher_list(&ctx->cipher_list, has_aes_hw, str,
                                false /* not strict */,
-                                false /* don't configure TLSv1.3 ciphers */);
+                                false /* don't configure TLSv1.3 ciphers */)) {
+    return 0;
+  }
+
+  return update_cipher_list(ctx->cipher_list, ctx->cipher_list, ctx->tls13_cipher_list);


Can we just pass the 1.3 ciphers to ssl_create_cipher_list and have it put them at the beginning automatically?

The same ssl_create_cipher_list function is used to create tls13_cipher_lists and the main cipher_list. Therefore, I have chosen to do update's separately to properly distinguish what is being set.

smittals2 and others added 13 commits December 26, 2024 15:10

combine the two cipher lists

bc7b951

defining default TLS 1.3 ciphersuites

e80cb6e

configure TLS 1.3 ciphersuites by default

c49f32d

remove default TLS 13 ciphersuites macro

bfb369f

fix update function for ctx->cipher_list

ef0f455

remove default tls13_cipher_list coonfiguirating in SSL_CTX

aacb978

testing for new SSL_Get_ciphers functionality

b6f297b

configure TLS 1.3 ciphersuites by default

749513a

modified existing tests to work with TLS 1.3 configured by default

1e818d5

documentation update

425e9a5

fixed logic for configuring in_group_flags

656b710

changed TLS 1.3 ciphersuite ordering

2622e1e

Merge branch 'main' into get_ciphers

fe430f4

smittals2 changed the title ~~[DRAFT] Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites~~ Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites Jan 2, 2025

smittals2 marked this pull request as ready for review January 2, 2025 23:55

smittals2 requested a review from a team as a code owner January 2, 2025 23:55

add differentiation for AES HW acceleration when ordering ciphers for…

72603cc

… tls 1.3

justsmth previously approved these changes Jan 6, 2025

View reviewed changes

use const

eb28574

smittals2 dismissed justsmth’s stale review via eb28574 January 6, 2025 17:52

remove old include

5fcdd4a

clean up comments

d4e5047

smittals2 requested a review from justsmth January 6, 2025 20:48

andrewhop reviewed Jan 7, 2025

View reviewed changes

smittals2 added 4 commits January 8, 2025 19:22

removing hardcoded tls1.3 logic pt1

6707995

moved default tls1.3 ciphersuite preferences to internal header

ff11f99

cleaning up pt2

849bf37

fixing conditionals in collecting ciphers

3f467ff

justsmth reviewed Jan 14, 2025

View reviewed changes

ssl/handshake_client.cc Outdated Show resolved Hide resolved

include/openssl/ssl.h Outdated Show resolved Hide resolved

ssl/ssl_cipher.cc Outdated Show resolved Hide resolved

justsmth requested a review from samuel40791765 January 14, 2025 14:09

smittals2 and others added 15 commits January 15, 2025 17:20

changed cipherlist condition

dbbd45c

minor pr comments

94d2189

changed tls 1.3 cipher order

7669f46

revert sorting order

fc0c20c

new test for tls 1.3

9ee29c8

modified ssl->config to hold tls 1.3 ciphersuite state

e38bfa3

revert default TLS 1.2 ciphersuites

dbd660a

change uniqptr init

272ef8f

minor edits to logic

0991f2f

fixed test

0d7b302

set Tls 1.3 ciphersuites explicitly

2c809d3

Merge branch 'main' into get_ciphers

7798ef6

changed SSLCipherPreferenceList initialization

8442723

change server side cipher selection logic

ae84e27

added config/ctx testing

0e66ad6

smittals2 changed the title ~~Fix SSL_get_ciphers behavior to return TLS 1.3 ciphersuites~~ Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers Jan 23, 2025

andrewhop mentioned this pull request Jan 24, 2025

SSL_get_ciphers() lacks TLSv1.3 ciphersuites #1638

Open

indentation and documentation

a755968

smittals2 requested review from justsmth and andrewhop January 24, 2025 01:04

andrewhop reviewed Jan 24, 2025

View reviewed changes

pr comments

2c6d018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers #2092

Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers #2092

smittals2 commented Jan 2, 2025 •

edited

Loading

codecov-commenter commented Jan 6, 2025 •

edited

Loading

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025

andrewhop Jan 24, 2025

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025 •

edited

Loading

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025 •

edited

Loading

andrewhop Jan 24, 2025

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025 •

edited

Loading

andrewhop Jan 24, 2025

smittals2 Jan 24, 2025

		@@ -1690,10 +1690,9 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out,
		// Once an equal-preference group is used, future directives must be

Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers #2092

Are you sure you want to change the base?

Refactor TLS 1.3 cipher selection and fix SSL_get_ciphers #2092

Conversation

smittals2 commented Jan 2, 2025 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Jan 6, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smittals2 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smittals2 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smittals2 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smittals2 commented Jan 2, 2025 •

edited

Loading

codecov-commenter commented Jan 6, 2025 •

edited

Loading

smittals2 Jan 24, 2025 •

edited

Loading

smittals2 Jan 24, 2025 •

edited

Loading

smittals2 Jan 24, 2025 •

edited

Loading