From a6517c5fe97b1aa1898f2233498613dd53735bd8 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Wed, 4 Oct 2023 22:40:11 -0700 Subject: [PATCH] ktls: forbid renegotiation (#4229) --- error/s2n_errno.c | 1 + error/s2n_errno.h | 1 + tests/unit/s2n_ktls_test.c | 25 +++++++++++++++++++++++++ tls/s2n_ktls.c | 16 ++++++++++++++++ tls/s2n_renegotiate.c | 2 ++ 5 files changed, 45 insertions(+) diff --git a/error/s2n_errno.c b/error/s2n_errno.c index 5778fc63ac1..fbe52416b5a 100644 --- a/error/s2n_errno.c +++ b/error/s2n_errno.c @@ -301,6 +301,7 @@ static const char *no_such_error = "Internal s2n error"; ERR_ENTRY(S2N_ERR_KTLS_BAD_CMSG, "Error handling cmsghdr.") \ ERR_ENTRY(S2N_ERR_ATOMIC, "Atomic operations in this environment would require locking") \ ERR_ENTRY(S2N_ERR_TEST_ASSERTION, "Test assertion failed") \ + ERR_ENTRY(S2N_ERR_KTLS_RENEG, "kTLS does not support secure renegotiation") \ /* clang-format on */ #define ERR_STR_CASE(ERR, str) \ diff --git a/error/s2n_errno.h b/error/s2n_errno.h index 8cac347350b..1ff5070717e 100644 --- a/error/s2n_errno.h +++ b/error/s2n_errno.h @@ -317,6 +317,7 @@ typedef enum { S2N_ERR_KTLS_UNSUPPORTED_CONN, S2N_ERR_KTLS_ENABLE, S2N_ERR_KTLS_BAD_CMSG, + S2N_ERR_KTLS_RENEG, S2N_ERR_ATOMIC, S2N_ERR_T_USAGE_END, } s2n_error; diff --git a/tests/unit/s2n_ktls_test.c b/tests/unit/s2n_ktls_test.c index 1e8a3e22370..ca9de62bb29 100644 --- a/tests/unit/s2n_ktls_test.c +++ b/tests/unit/s2n_ktls_test.c @@ -140,6 +140,12 @@ S2N_RESULT s2n_test_generate_fake_crypto_params(struct s2n_connection *conn) return S2N_RESULT_OK; } +static int s2n_test_reneg_cb(struct s2n_connection *conn, void *context, + s2n_renegotiate_response *response) +{ + return S2N_SUCCESS; +} + int main(int argc, char **argv) { BEGIN_TEST(); @@ -327,6 +333,25 @@ int main(int argc, char **argv) server_conn->managed_recv_io = true; EXPECT_SUCCESS(s2n_connection_ktls_enable_recv(server_conn)); }; + + /* Fail if renegotiation potentially supported */ + { + DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free); + + DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT), + s2n_connection_ptr_free); + EXPECT_OK(s2n_test_configure_connection_for_ktls(client)); + EXPECT_SUCCESS(s2n_connection_set_config(client, config)); + + DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER), + s2n_connection_ptr_free); + EXPECT_OK(s2n_test_configure_connection_for_ktls(server)); + EXPECT_SUCCESS(s2n_connection_set_config(server, config)); + + EXPECT_SUCCESS(s2n_config_set_renegotiate_request_cb(config, s2n_test_reneg_cb, NULL)); + EXPECT_FAILURE_WITH_ERRNO(s2n_connection_ktls_enable_recv(client), S2N_ERR_KTLS_RENEG); + EXPECT_SUCCESS(s2n_connection_ktls_enable_recv(server)); + }; }; /* Test s2n_ktls_init_aes128_gcm_crypto_info */ diff --git a/tls/s2n_ktls.c b/tls/s2n_ktls.c index 06c39eb7d53..51b1334d1ea 100644 --- a/tls/s2n_ktls.c +++ b/tls/s2n_ktls.c @@ -16,6 +16,7 @@ #include "tls/s2n_ktls.h" #include "tls/s2n_prf.h" +#include "tls/s2n_tls.h" /* Used for overriding setsockopt calls in testing */ s2n_setsockopt_fn s2n_setsockopt = setsockopt; @@ -49,6 +50,8 @@ static S2N_RESULT s2n_ktls_validate(struct s2n_connection *conn, s2n_ktls_mode k RESULT_ENSURE_REF(conn->secure->cipher_suite->record_alg); const struct s2n_cipher *cipher = conn->secure->cipher_suite->record_alg->cipher; RESULT_ENSURE_REF(cipher); + const struct s2n_config *config = conn->config; + RESULT_ENSURE_REF(config); RESULT_ENSURE(s2n_ktls_is_supported_on_platform(), S2N_ERR_KTLS_UNSUPPORTED_PLATFORM); @@ -66,6 +69,19 @@ static S2N_RESULT s2n_ktls_validate(struct s2n_connection *conn, s2n_ktls_mode k /* Check if the cipher supports kTLS */ RESULT_ENSURE(cipher->ktls_supported, S2N_ERR_KTLS_UNSUPPORTED_CONN); + /* Renegotiation requires updating the keys, which kTLS doesn't currently support. + * + * Setting the renegotiation callback doesn't guarantee that a client will + * attempt to renegotiate. The callback can also be used to send warning alerts + * signaling that renegotiation was rejected. However, we can provide applications + * with a clearer signal earlier by preventing them from enabling ktls on a + * connection that MIGHT require renegotiation. We can relax this restriction + * later if necessary. + */ + bool may_receive_hello_request = s2n_result_is_ok(s2n_client_hello_request_validate(conn)); + bool may_renegotiate = may_receive_hello_request && config->renegotiate_request_cb; + RESULT_ENSURE(!may_renegotiate, S2N_ERR_KTLS_RENEG); + /* kTLS I/O functionality is managed by s2n-tls. kTLS cannot be enabled if the * application sets custom I/O (managed_send_io == false means application has * set custom I/O). diff --git a/tls/s2n_renegotiate.c b/tls/s2n_renegotiate.c index f930390d79c..84765e44722 100644 --- a/tls/s2n_renegotiate.c +++ b/tls/s2n_renegotiate.c @@ -36,6 +36,8 @@ S2N_RESULT s2n_renegotiate_validate(struct s2n_connection *conn) RESULT_ENSURE(conn->mode == S2N_CLIENT, S2N_ERR_NO_RENEGOTIATION); RESULT_ENSURE(conn->secure_renegotiation, S2N_ERR_NO_RENEGOTIATION); RESULT_ENSURE(conn->handshake.renegotiation, S2N_ERR_INVALID_STATE); + RESULT_ENSURE(!conn->ktls_send_enabled, S2N_ERR_KTLS_RENEG); + RESULT_ENSURE(!conn->ktls_recv_enabled, S2N_ERR_KTLS_RENEG); return S2N_RESULT_OK; }