From 3f5dd79d8abd40193ab3ce9b3ee9a30bf77b34ba Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Tue, 22 Jul 2025 10:49:33 +0900 Subject: [PATCH 1/3] key_update: fix state transition in KTLS code path Signed-off-by: Daiki Ueno --- lib/record.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/record.c b/lib/record.c index d37f79a550..ebc75addec 100644 --- a/lib/record.c +++ b/lib/record.c @@ -2045,7 +2045,7 @@ ssize_t gnutls_record_send2(gnutls_session_t session, const void *data, FALLTHROUGH; case RECORD_SEND_KEY_UPDATE_3: if (IS_KTLS_ENABLED(session, GNUTLS_KTLS_SEND)) { - return _gnutls_ktls_send( + ret = _gnutls_ktls_send( session, session->internals.record_key_update_buffer.data, session->internals.record_key_update_buffer -- 2.50.1 From fee06c4ac19129e0f5f4b639919a4ff244bf174c Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Thu, 31 Jul 2025 15:34:48 +0900 Subject: [PATCH 2/3] constate: switch epoch lookup to linear search The previous logic of epoch lookup was utilizing the fact that epoch numbers are monotonically increasing and there are no gaps in between after garbarge collection. That is, however, no longer true when a TLS 1.3 key update is happening in only one direction. This patch switches to using linear search instead, at the cost of approx MAX_EPOCH_INDEX * 2 (= 8) comparison. Signed-off-by: Daiki Ueno --- lib/constate.c | 47 ++++++++++++++++------------------------------- lib/gnutls_int.h | 3 --- 2 files changed, 16 insertions(+), 34 deletions(-) diff --git a/lib/constate.c b/lib/constate.c index ca253a2bea..b091d891ff 100644 --- a/lib/constate.c +++ b/lib/constate.c @@ -932,17 +932,23 @@ static inline int epoch_resolve(gnutls_session_t session, static inline record_parameters_st **epoch_get_slot(gnutls_session_t session, uint16_t epoch) { - uint16_t epoch_index = epoch - session->security_parameters.epoch_min; + /* First look for a non-empty slot */ + for (size_t i = 0; i < MAX_EPOCH_INDEX; i++) { + record_parameters_st **slot = &session->record_parameters[i]; + if (*slot != NULL && (*slot)->epoch == epoch) + return slot; + } - if (epoch_index >= MAX_EPOCH_INDEX) { - _gnutls_handshake_log( - "Epoch %d out of range (idx: %d, max: %d)\n", - (int)epoch, (int)epoch_index, MAX_EPOCH_INDEX); - gnutls_assert(); - return NULL; + /* Then look for an empty slot */ + for (size_t i = 0; i < MAX_EPOCH_INDEX; i++) { + record_parameters_st **slot = &session->record_parameters[i]; + if (*slot == NULL) + return slot; } - /* The slot may still be empty (NULL) */ - return &session->record_parameters[epoch_index]; + + gnutls_assert(); + _gnutls_handshake_log("No slot available for epoch %u\n", epoch); + return NULL; } int _gnutls_epoch_get(gnutls_session_t session, unsigned int epoch_rel, @@ -1063,8 +1069,7 @@ static inline int epoch_alive(gnutls_session_t session, void _gnutls_epoch_gc(gnutls_session_t session) { - int i, j; - unsigned int min_index = 0; + int i; _gnutls_record_log("REC[%p]: Start of epoch cleanup\n", session); @@ -1091,26 +1096,6 @@ void _gnutls_epoch_gc(gnutls_session_t session) } } - /* Look for contiguous NULLs at the start of the array */ - for (i = 0; - i < MAX_EPOCH_INDEX && session->record_parameters[i] == NULL; i++) - ; - min_index = i; - - /* Pick up the slack in the epoch window. */ - if (min_index != 0) { - for (i = 0, j = min_index; j < MAX_EPOCH_INDEX; i++, j++) { - session->record_parameters[i] = - session->record_parameters[j]; - session->record_parameters[j] = NULL; - } - } - - /* Set the new epoch_min */ - if (session->record_parameters[0] != NULL) - session->security_parameters.epoch_min = - session->record_parameters[0]->epoch; - gnutls_mutex_unlock(&session->internals.epoch_lock); _gnutls_record_log("REC[%p]: End of epoch cleanup\n", session); diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h index e9ec36d585..cc5d965593 100644 --- a/lib/gnutls_int.h +++ b/lib/gnutls_int.h @@ -860,9 +860,6 @@ typedef struct { /* The epoch that the next handshake will initialize. */ uint16_t epoch_next; - /* The epoch at index 0 of record_parameters. */ - uint16_t epoch_min; - /* this is the ciphersuite we are going to use * moved here from internals in order to be restored * on resume; -- 2.50.1 From 0d25525656d3bcf2d8ca9d17d5ebe7cb738ed4c2 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Sat, 19 Jul 2025 07:08:24 +0900 Subject: [PATCH 3/3] key_update: rework the rekeying logic While RFC 8446 4.6.3 says that the sender of a KeyUpdate message should only update its sending key, the previous implementation updated both the sending and receiving keys, preventing that any application data interleaved being decrypted. This splits the key update logic into 2 phases: when sending a KeyUpdate, only update the sending key, and when receiving a KeyUpdate, only update the receiving key. In both cases, KeyUpdate messages are encrypted/decrypted with the old keys. Signed-off-by: Daiki Ueno --- lib/gnutls_int.h | 2 +- lib/tls13/key_update.c | 72 +++++++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h index cc5d965593..a7684f75c1 100644 --- a/lib/gnutls_int.h +++ b/lib/gnutls_int.h @@ -1652,7 +1652,7 @@ typedef struct { } internals_st; /* Maximum number of epochs we keep around. */ -#define MAX_EPOCH_INDEX 4 +#define MAX_EPOCH_INDEX 16 #define reset_cand_groups(session) \ session->internals.cand_ec_group = session->internals.cand_dh_group = \ diff --git a/lib/tls13/key_update.c b/lib/tls13/key_update.c index 41243651b5..beee1dc41a 100644 --- a/lib/tls13/key_update.c +++ b/lib/tls13/key_update.c @@ -52,45 +52,47 @@ static inline int set_ktls_keys(gnutls_session_t session, return 0; } -static int update_keys(gnutls_session_t session, hs_stage_t stage) +static int update_sending_key(gnutls_session_t session, hs_stage_t stage) { int ret; - ret = _tls13_update_secret(session, - session->key.proto.tls13.temp_secret, - session->key.proto.tls13.temp_secret_size); + _gnutls_epoch_bump(session); + ret = _gnutls_epoch_dup(session, EPOCH_WRITE_CURRENT); if (ret < 0) return gnutls_assert_val(ret); - _gnutls_epoch_bump(session); - ret = _gnutls_epoch_dup(session, EPOCH_READ_CURRENT); + ret = _tls13_write_connection_state_init(session, stage); if (ret < 0) return gnutls_assert_val(ret); - /* If we send a key update during early start, only update our - * write keys */ - if (session->internals.recv_state == RECV_STATE_EARLY_START) { - ret = _tls13_write_connection_state_init(session, stage); + if (IS_KTLS_ENABLED(session, GNUTLS_KTLS_SEND)) { + ret = set_ktls_keys(session, GNUTLS_KTLS_SEND); if (ret < 0) return gnutls_assert_val(ret); + } - if (IS_KTLS_ENABLED(session, GNUTLS_KTLS_SEND)) - ret = set_ktls_keys(session, GNUTLS_KTLS_SEND); - } else { - ret = _tls13_connection_state_init(session, stage); - if (ret < 0) - return gnutls_assert_val(ret); + return 0; +} - if (IS_KTLS_ENABLED(session, GNUTLS_KTLS_SEND) && - stage == STAGE_UPD_OURS) - ret = set_ktls_keys(session, GNUTLS_KTLS_SEND); - else if (IS_KTLS_ENABLED(session, GNUTLS_KTLS_RECV) && - stage == STAGE_UPD_PEERS) - ret = set_ktls_keys(session, GNUTLS_KTLS_RECV); - } +static int update_receiving_key(gnutls_session_t session, hs_stage_t stage) +{ + int ret; + + _gnutls_epoch_bump(session); + ret = _gnutls_epoch_dup(session, EPOCH_READ_CURRENT); + if (ret < 0) + return gnutls_assert_val(ret); + + ret = _tls13_read_connection_state_init(session, stage); if (ret < 0) return gnutls_assert_val(ret); + if (IS_KTLS_ENABLED(session, GNUTLS_KTLS_RECV)) { + ret = set_ktls_keys(session, GNUTLS_KTLS_RECV); + if (ret < 0) + return gnutls_assert_val(ret); + } + return 0; } @@ -128,7 +130,13 @@ int _gnutls13_recv_key_update(gnutls_session_t session, gnutls_buffer_st *buf) switch (buf->data[0]) { case 0: /* peer updated its key, not requested our key update */ - ret = update_keys(session, STAGE_UPD_PEERS); + ret = _tls13_update_secret( + session, session->key.proto.tls13.temp_secret, + session->key.proto.tls13.temp_secret_size); + if (ret < 0) + return gnutls_assert_val(ret); + + ret = update_receiving_key(session, STAGE_UPD_PEERS); if (ret < 0) return gnutls_assert_val(ret); @@ -141,7 +149,13 @@ int _gnutls13_recv_key_update(gnutls_session_t session, gnutls_buffer_st *buf) } /* peer updated its key, requested our key update */ - ret = update_keys(session, STAGE_UPD_PEERS); + ret = _tls13_update_secret( + session, session->key.proto.tls13.temp_secret, + session->key.proto.tls13.temp_secret_size); + if (ret < 0) + return gnutls_assert_val(ret); + + ret = update_receiving_key(session, STAGE_UPD_PEERS); if (ret < 0) return gnutls_assert_val(ret); @@ -248,7 +262,13 @@ int gnutls_session_key_update(gnutls_session_t session, unsigned flags) _gnutls_epoch_gc(session); /* it was completely sent, update the keys */ - ret = update_keys(session, STAGE_UPD_OURS); + ret = _tls13_update_secret(session, + session->key.proto.tls13.temp_secret, + session->key.proto.tls13.temp_secret_size); + if (ret < 0) + return gnutls_assert_val(ret); + + ret = update_sending_key(session, STAGE_UPD_OURS); if (ret < 0) return gnutls_assert_val(ret); -- 2.50.1