From 4b8ebd47eaae6f5f142253abffb5b61f195aa8ff Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 15 Nov 2023 19:02:11 -0800 Subject: [PATCH 1/3] Remove NULLs in s2n_kex --- tls/s2n_auth_selection.c | 4 ++- tls/s2n_cipher_suites.c | 6 ++-- tls/s2n_kex.c | 66 +++++++++++++++++++++++++++++++++------- tls/s2n_kex.h | 1 + 4 files changed, 62 insertions(+), 15 deletions(-) diff --git a/tls/s2n_auth_selection.c b/tls/s2n_auth_selection.c index 31ed894e1b7..564b8488027 100644 --- a/tls/s2n_auth_selection.c +++ b/tls/s2n_auth_selection.c @@ -88,7 +88,9 @@ static int s2n_is_sig_alg_valid_for_cipher_suite(s2n_signature_algorithm sig_alg * Therefore, if a cipher suite uses a non-ephemeral kex, then any signature * algorithm that requires RSA-PSS certificates is not valid. */ - if (cipher_suite->key_exchange_alg != NULL && !cipher_suite->key_exchange_alg->is_ephemeral) { + const struct s2n_kex *kex = cipher_suite->key_exchange_alg; + POSIX_ENSURE_REF(kex); + if (!kex->is_ephemeral) { POSIX_ENSURE_NE(cert_type_for_sig_alg, S2N_PKEY_TYPE_RSA_PSS); } diff --git a/tls/s2n_cipher_suites.c b/tls/s2n_cipher_suites.c index 2ea36324938..26a6f90c80b 100644 --- a/tls/s2n_cipher_suites.c +++ b/tls/s2n_cipher_suites.c @@ -698,7 +698,7 @@ struct s2n_cipher_suite s2n_tls13_aes_128_gcm_sha256 = { .available = 0, .name = "TLS_AES_128_GCM_SHA256", .iana_value = { TLS_AES_128_GCM_SHA256 }, - .key_exchange_alg = NULL, + .key_exchange_alg = &s2n_tls13_kex, .auth_method = S2N_AUTHENTICATION_METHOD_TLS13, .record_alg = NULL, .all_record_algs = { &s2n_tls13_record_alg_aes128_gcm }, @@ -712,7 +712,7 @@ struct s2n_cipher_suite s2n_tls13_aes_256_gcm_sha384 = { .available = 0, .name = "TLS_AES_256_GCM_SHA384", .iana_value = { TLS_AES_256_GCM_SHA384 }, - .key_exchange_alg = NULL, + .key_exchange_alg = &s2n_tls13_kex, .auth_method = S2N_AUTHENTICATION_METHOD_TLS13, .record_alg = NULL, .all_record_algs = { &s2n_tls13_record_alg_aes256_gcm }, @@ -726,7 +726,7 @@ struct s2n_cipher_suite s2n_tls13_chacha20_poly1305_sha256 = { .available = 0, .name = "TLS_CHACHA20_POLY1305_SHA256", .iana_value = { TLS_CHACHA20_POLY1305_SHA256 }, - .key_exchange_alg = NULL, + .key_exchange_alg = &s2n_tls13_kex, .auth_method = S2N_AUTHENTICATION_METHOD_TLS13, .record_alg = NULL, .all_record_algs = { &s2n_tls13_record_alg_chacha20_poly1305 }, diff --git a/tls/s2n_kex.c b/tls/s2n_kex.c index 0e5a80d8bf4..7f7c23dfc87 100644 --- a/tls/s2n_kex.c +++ b/tls/s2n_kex.c @@ -135,11 +135,6 @@ static S2N_RESULT s2n_configure_kem(const struct s2n_cipher_suite *cipher_suite, return S2N_RESULT_OK; } -static S2N_RESULT s2n_no_op_configure(const struct s2n_cipher_suite *cipher_suite, struct s2n_connection *conn) -{ - return S2N_RESULT_OK; -} - static S2N_RESULT s2n_check_hybrid_ecdhe_kem(const struct s2n_cipher_suite *cipher_suite, struct s2n_connection *conn, bool *is_supported) { RESULT_ENSURE_REF(cipher_suite); @@ -156,6 +151,42 @@ static S2N_RESULT s2n_check_hybrid_ecdhe_kem(const struct s2n_cipher_suite *ciph return S2N_RESULT_OK; } +static S2N_RESULT s2n_kex_configure_noop(const struct s2n_cipher_suite *cipher_suite, + struct s2n_connection *conn) +{ + return S2N_RESULT_OK; +} + +static S2N_RESULT s2n_kex_unsupported(const struct s2n_cipher_suite *cipher_suite, + struct s2n_connection *conn, bool *is_supported) +{ + RESULT_ENSURE_REF(is_supported); + *is_supported = false; + return S2N_RESULT_OK; +} + +static int s2n_kex_server_key_recv_read_data_unimplemented(struct s2n_connection *conn, + struct s2n_blob *data_to_verify, struct s2n_kex_raw_server_data *kex_data) +{ + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); +} + +static int s2n_kex_server_key_recv_parse_data_unimplemented(struct s2n_connection *conn, + struct s2n_kex_raw_server_data *kex_data) +{ + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); +} + +static int s2n_kex_io_unimplemented(struct s2n_connection *conn, struct s2n_blob *data_to_sign) +{ + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); +} + +static int s2n_kex_prf_unimplemented(struct s2n_connection *conn, struct s2n_blob *premaster_secret) +{ + POSIX_BAIL(S2N_ERR_UNIMPLEMENTED); +} + const struct s2n_kex s2n_kem = { .is_ephemeral = true, .connection_supported = &s2n_check_kem, @@ -165,15 +196,16 @@ const struct s2n_kex s2n_kem = { .server_key_send = &s2n_kem_server_key_send, .client_key_recv = &s2n_kem_client_key_recv, .client_key_send = &s2n_kem_client_key_send, + .prf = &s2n_kex_prf_unimplemented, }; const struct s2n_kex s2n_rsa = { .is_ephemeral = false, .connection_supported = &s2n_check_rsa_key, - .configure_connection = &s2n_no_op_configure, - .server_key_recv_read_data = NULL, - .server_key_recv_parse_data = NULL, - .server_key_send = NULL, + .configure_connection = &s2n_kex_configure_noop, + .server_key_recv_read_data = &s2n_kex_server_key_recv_read_data_unimplemented, + .server_key_recv_parse_data = &s2n_kex_server_key_recv_parse_data_unimplemented, + .server_key_send = &s2n_kex_io_unimplemented, .client_key_recv = &s2n_rsa_client_key_recv, .client_key_send = &s2n_rsa_client_key_send, .prf = &s2n_prf_calculate_master_secret, @@ -182,7 +214,7 @@ const struct s2n_kex s2n_rsa = { const struct s2n_kex s2n_dhe = { .is_ephemeral = true, .connection_supported = &s2n_check_dhe, - .configure_connection = &s2n_no_op_configure, + .configure_connection = &s2n_kex_configure_noop, .server_key_recv_read_data = &s2n_dhe_server_key_recv_read_data, .server_key_recv_parse_data = &s2n_dhe_server_key_recv_parse_data, .server_key_send = &s2n_dhe_server_key_send, @@ -194,7 +226,7 @@ const struct s2n_kex s2n_dhe = { const struct s2n_kex s2n_ecdhe = { .is_ephemeral = true, .connection_supported = &s2n_check_ecdhe, - .configure_connection = &s2n_no_op_configure, + .configure_connection = &s2n_kex_configure_noop, .server_key_recv_read_data = &s2n_ecdhe_server_key_recv_read_data, .server_key_recv_parse_data = &s2n_ecdhe_server_key_recv_parse_data, .server_key_send = &s2n_ecdhe_server_key_send, @@ -216,6 +248,18 @@ const struct s2n_kex s2n_hybrid_ecdhe_kem = { .prf = &s2n_hybrid_prf_master_secret, }; +const struct s2n_kex s2n_tls13_kex = { + .is_ephemeral = true, + .connection_supported = &s2n_kex_unsupported, + .configure_connection = &s2n_kex_configure_noop, + .server_key_recv_read_data = &s2n_kex_server_key_recv_read_data_unimplemented, + .server_key_recv_parse_data = &s2n_kex_server_key_recv_parse_data_unimplemented, + .server_key_send = &s2n_kex_io_unimplemented, + .client_key_recv = &s2n_kex_io_unimplemented, + .client_key_send = &s2n_kex_io_unimplemented, + .prf = &s2n_kex_prf_unimplemented, +}; + S2N_RESULT s2n_kex_supported(const struct s2n_cipher_suite *cipher_suite, struct s2n_connection *conn, bool *is_supported) { RESULT_ENSURE_REF(cipher_suite); diff --git a/tls/s2n_kex.h b/tls/s2n_kex.h index 9f31b0bd2a5..f229914e2ad 100644 --- a/tls/s2n_kex.h +++ b/tls/s2n_kex.h @@ -40,6 +40,7 @@ extern const struct s2n_kex s2n_rsa; extern const struct s2n_kex s2n_dhe; extern const struct s2n_kex s2n_ecdhe; extern const struct s2n_kex s2n_hybrid_ecdhe_kem; +extern const struct s2n_kex s2n_tls13_kex; S2N_RESULT s2n_kex_supported(const struct s2n_cipher_suite *cipher_suite, struct s2n_connection *conn, bool *is_supported); S2N_RESULT s2n_configure_kex(const struct s2n_cipher_suite *cipher_suite, struct s2n_connection *conn); From 93d091a2b79bb4b309cf64b1da1d02b0a2d01842 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Thu, 16 Nov 2023 16:00:57 -0800 Subject: [PATCH 2/3] PR feedback --- tls/s2n_cipher_suites.c | 22 +++++++++------------- tls/s2n_kex.c | 18 +++++++++--------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/tls/s2n_cipher_suites.c b/tls/s2n_cipher_suites.c index 26a6f90c80b..c00e7852217 100644 --- a/tls/s2n_cipher_suites.c +++ b/tls/s2n_cipher_suites.c @@ -1276,19 +1276,15 @@ static int s2n_set_cipher_as_server(struct s2n_connection *conn, uint8_t *wire, continue; } - /* TLS 1.3 does not include key exchange in cipher suites */ - if (match->minimum_required_tls_version < S2N_TLS13) { - /* If the kex is not supported continue to the next candidate */ - bool kex_supported = false; - POSIX_GUARD_RESULT(s2n_kex_supported(match, conn, &kex_supported)); - if (!kex_supported) { - continue; - } - - /* If the kex is not configured correctly continue to the next candidate */ - if (s2n_result_is_error(s2n_configure_kex(match, conn))) { - continue; - } + /* If the kex is not supported continue to the next candidate */ + bool kex_supported = false; + POSIX_GUARD_RESULT(s2n_kex_supported(match, conn, &kex_supported)); + if (!kex_supported) { + continue; + } + /* If the kex is not configured correctly continue to the next candidate */ + if (s2n_result_is_error(s2n_configure_kex(match, conn))) { + continue; } /** diff --git a/tls/s2n_kex.c b/tls/s2n_kex.c index 7f7c23dfc87..4583dd65df8 100644 --- a/tls/s2n_kex.c +++ b/tls/s2n_kex.c @@ -25,6 +25,14 @@ #include "tls/s2n_tls.h" #include "utils/s2n_safety.h" +static S2N_RESULT s2n_check_tls13(const struct s2n_cipher_suite *cipher_suite, + struct s2n_connection *conn, bool *is_supported) +{ + RESULT_ENSURE_REF(is_supported); + *is_supported = (s2n_connection_get_protocol_version(conn) >= S2N_TLS13); + return S2N_RESULT_OK; +} + static S2N_RESULT s2n_check_rsa_key(const struct s2n_cipher_suite *cipher_suite, struct s2n_connection *conn, bool *is_supported) { RESULT_ENSURE_REF(cipher_suite); @@ -157,14 +165,6 @@ static S2N_RESULT s2n_kex_configure_noop(const struct s2n_cipher_suite *cipher_s return S2N_RESULT_OK; } -static S2N_RESULT s2n_kex_unsupported(const struct s2n_cipher_suite *cipher_suite, - struct s2n_connection *conn, bool *is_supported) -{ - RESULT_ENSURE_REF(is_supported); - *is_supported = false; - return S2N_RESULT_OK; -} - static int s2n_kex_server_key_recv_read_data_unimplemented(struct s2n_connection *conn, struct s2n_blob *data_to_verify, struct s2n_kex_raw_server_data *kex_data) { @@ -250,7 +250,7 @@ const struct s2n_kex s2n_hybrid_ecdhe_kem = { const struct s2n_kex s2n_tls13_kex = { .is_ephemeral = true, - .connection_supported = &s2n_kex_unsupported, + .connection_supported = &s2n_check_tls13, .configure_connection = &s2n_kex_configure_noop, .server_key_recv_read_data = &s2n_kex_server_key_recv_read_data_unimplemented, .server_key_recv_parse_data = &s2n_kex_server_key_recv_parse_data_unimplemented, From 4a760deb3b10bbcbb322a36fcfae04880693e333 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Fri, 17 Nov 2023 15:03:32 -0800 Subject: [PATCH 3/3] Add comment --- tls/s2n_kex.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tls/s2n_kex.c b/tls/s2n_kex.c index 4583dd65df8..ac7529c5371 100644 --- a/tls/s2n_kex.c +++ b/tls/s2n_kex.c @@ -248,6 +248,11 @@ const struct s2n_kex s2n_hybrid_ecdhe_kem = { .prf = &s2n_hybrid_prf_master_secret, }; +/* TLS1.3 key exchange is implemented differently from previous versions and does + * not currently require most of the functionality offered by s2n_kex. + * This structure primarily acts as a placeholder, so its methods are either + * noops or unimplemented. + */ const struct s2n_kex s2n_tls13_kex = { .is_ephemeral = true, .connection_supported = &s2n_check_tls13,