From 2460fd3473023a16b2766fd1a6c4a3845e2eeb08 Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Tue, 26 Mar 2024 22:15:58 -0700 Subject: [PATCH 1/2] Verify the incoming initial packets before any action. --- CMakeLists.txt | 2 +- picoquic/packet.c | 147 ++++++++++++++++++++++------------- picoquic/picoquic.h | 2 +- picoquic/picoquic_internal.h | 2 + picoquic/tls_api.c | 2 +- 5 files changed, 97 insertions(+), 58 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 046e2a43b..8acd1a3f9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,7 +8,7 @@ else() endif() project(picoquic - VERSION 1.1.19.7 + VERSION 1.1.19.8 DESCRIPTION "picoquic library" LANGUAGES C CXX) diff --git a/picoquic/packet.c b/picoquic/packet.c index b0bfb3a8d..c8a740844 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -88,6 +88,7 @@ picoquic_packet_type_enum picoquic_parse_long_packet_type(uint8_t flags, int ver int picoquic_screen_initial_packet( picoquic_quic_t* quic, + const uint8_t* bytes, size_t packet_length, const struct sockaddr* addr_from, picoquic_packet_header* ph, @@ -111,54 +112,76 @@ int picoquic_screen_initial_packet( ret = PICOQUIC_ERROR_PACKET_HEADER_PARSING; } else if (*pcnx == NULL) { - if (quic->enforce_client_only) { - /* Cannot create a client connection if the context is client only */ - ret = PICOQUIC_ERROR_SERVER_BUSY; - } - else if (quic->server_busy) { - /* Cannot create a client connection now, send immediate close. */ - ret = PICOQUIC_ERROR_SERVER_BUSY; - } - else { - int is_address_blocked = !quic->is_port_blocking_disabled && picoquic_check_addr_blocked(addr_from); - int is_new_token = 0; - int has_good_token = 0; - int has_bad_token = 0; - picoquic_connection_id_t original_cnxid = { 0 }; - if (ph->token_length > 0) { - /* If a token is present, verify it. - * The PN verification is disabled (UINT64_MAX) because the PN - * is not decrypted yet. - */ - if (picoquic_verify_retry_token(quic, addr_from, current_time, - &is_new_token, &original_cnxid, &ph->dest_cnx_id, UINT32_MAX, - ph->token_bytes, ph->token_length, 1) == 0) { - has_good_token = 1; - } - else { - has_bad_token = 1; + /* Verify the AEAD checkum */ + void* aead_ctx = NULL; + void* pn_dec_ctx = NULL; + uint8_t decrypted_bytes[PICOQUIC_MAX_PACKET_SIZE]; + picoquic_packet_header dph = *ph; + + if (picoquic_get_initial_aead_context(quic, ph->version_index, &ph->dest_cnx_id, + 0 /* is_client=0 */, 0 /* is_enc = 0 */, &aead_ctx, &pn_dec_ctx) == 0) { + ret = picoquic_remove_header_protection_inner((uint8_t *)bytes, ph->offset + ph->payload_length, + decrypted_bytes, &dph, pn_dec_ctx, 0 /* is_loss_bit_enabled_incoming */, 0 /* sack_list_last*/); + if (ret == 0) { + size_t decrypted_length = picoquic_aead_decrypt_generic(decrypted_bytes + dph.offset, + bytes + dph.offset, dph.payload_length, dph.pn64, decrypted_bytes, dph.offset, + aead_ctx); + if (decrypted_length >= dph.payload_length) { + ret = PICOQUIC_ERROR_AEAD_CHECK; } } + } + else { + ret = PICOQUIC_ERROR_MEMORY; + } - if (has_bad_token && !is_new_token) { - /* sending a bad retry token is fatal, sending an old new token is not */ - ret = PICOQUIC_ERROR_INVALID_TOKEN; + if (ret == 0) { + if (quic->enforce_client_only) { + /* Cannot create a client connection if the context is client only */ + ret = PICOQUIC_ERROR_SERVER_BUSY; } - else if (!has_good_token && (quic->force_check_token || quic->max_half_open_before_retry <= quic->current_number_half_open || is_address_blocked)) { - /* tokens are required before accepting new connections, so ask to queue a retry packet. */ - ret = PICOQUIC_ERROR_RETRY_NEEDED; + else if (quic->server_busy) { + /* Cannot create a client connection now, send immediate close. */ + ret = PICOQUIC_ERROR_SERVER_BUSY; } else { - /* All clear */ - /* Check: what do do with odcid? */ - *pcnx = picoquic_create_cnx(quic, ph->dest_cnx_id, ph->srce_cnx_id, addr_from, current_time, ph->vn, NULL, NULL, 0); - if (*pcnx == NULL) { - /* Could not allocate the context */ - ret = PICOQUIC_ERROR_MEMORY; + int is_address_blocked = !quic->is_port_blocking_disabled && picoquic_check_addr_blocked(addr_from); + int is_new_token = 0; + int has_good_token = 0; + int has_bad_token = 0; + picoquic_connection_id_t original_cnxid = { 0 }; + if (ph->token_length > 0) { + /* If a token is present, verify it. */ + if (picoquic_verify_retry_token(quic, addr_from, current_time, + &is_new_token, &original_cnxid, &ph->dest_cnx_id, (uint32_t)dph.pn64, + ph->token_bytes, ph->token_length, 1) == 0) { + has_good_token = 1; + } + else { + has_bad_token = 1; + } } - else if (has_good_token) { - (*pcnx)->initial_validated = 1; - (void)picoquic_parse_connection_id(original_cnxid.id, original_cnxid.id_len, &(*pcnx)->original_cnxid); + + if (has_bad_token && !is_new_token) { + /* sending a bad retry token is fatal, sending an old new token is not */ + ret = PICOQUIC_ERROR_INVALID_TOKEN; + } + else if (!has_good_token && (quic->force_check_token || quic->max_half_open_before_retry <= quic->current_number_half_open || is_address_blocked)) { + /* tokens are required before accepting new connections, so ask to queue a retry packet. */ + ret = PICOQUIC_ERROR_RETRY_NEEDED; + } + else { + /* All clear */ + /* Check: what do do with odcid? */ + *pcnx = picoquic_create_cnx(quic, ph->dest_cnx_id, ph->srce_cnx_id, addr_from, current_time, ph->vn, NULL, NULL, 0); + if (*pcnx == NULL) { + /* Could not allocate the context */ + ret = PICOQUIC_ERROR_MEMORY; + } + else if (has_good_token) { + (*pcnx)->initial_validated = 1; + (void)picoquic_parse_connection_id(original_cnxid.id, original_cnxid.id_len, &(*pcnx)->original_cnxid); + } } } } @@ -539,16 +562,16 @@ void picoquic_log_pn_dec_trial(picoquic_cnx_t* cnx) /* * Remove header protection */ -int picoquic_remove_header_protection(picoquic_cnx_t* cnx, +int picoquic_remove_header_protection_inner( uint8_t* bytes, - uint8_t * decrypted_bytes, - picoquic_packet_header* ph) + size_t length, + uint8_t* decrypted_bytes, + picoquic_packet_header* ph, + void * pn_enc, + unsigned int is_loss_bit_enabled_incoming, + uint64_t sack_list_last) { int ret = 0; - size_t length = ph->offset + ph->payload_length; /* this may change after decrypting the PN */ - void * pn_enc = NULL; - - pn_enc = cnx->crypto_context[ph->epoch].pn_dec; if (pn_enc != NULL) { @@ -572,10 +595,9 @@ int picoquic_remove_header_protection(picoquic_cnx_t* cnx, else { /* Decode */ uint8_t first_byte = bytes[0]; - uint8_t first_mask = ((first_byte & 0x80) == 0x80) ? 0x0F : (cnx->is_loss_bit_enabled_incoming)?0x07:0x1F; + uint8_t first_mask = ((first_byte & 0x80) == 0x80) ? 0x0F : (is_loss_bit_enabled_incoming)?0x07:0x1F; uint8_t pn_l; uint32_t pn_val = 0; - picoquic_sack_list_t* sack_list; memcpy(decrypted_bytes, bytes, ph->pn_offset); picoquic_pn_encrypt(pn_enc, bytes + sample_offset, mask_bytes, mask_bytes, mask_length); @@ -601,12 +623,11 @@ int picoquic_remove_header_protection(picoquic_cnx_t* cnx, } /* Build a packet number to 64 bits */ - sack_list = picoquic_sack_list_from_cnx_context(cnx, ph->pc, ph->l_cid); - ph->pn64 = picoquic_get_packet_number64(picoquic_sack_list_last(sack_list), ph->pnmask, ph->pn); + ph->pn64 = picoquic_get_packet_number64(sack_list_last, ph->pnmask, ph->pn); /* Check the reserved bits */ if ((first_byte & 0x80) == 0) { - ph->has_reserved_bit_set = !cnx->is_loss_bit_enabled_incoming && (first_byte & 0x18) != 0; + ph->has_reserved_bit_set = !is_loss_bit_enabled_incoming && (first_byte & 0x18) != 0; } else{ ph->has_reserved_bit_set = (first_byte & 0x0c) != 0; @@ -629,6 +650,22 @@ int picoquic_remove_header_protection(picoquic_cnx_t* cnx, return ret; } +int picoquic_remove_header_protection(picoquic_cnx_t* cnx, + uint8_t* bytes, + uint8_t * decrypted_bytes, + picoquic_packet_header* ph) +{ + int ret = 0; + size_t length = ph->offset + ph->payload_length; /* this may change after decrypting the PN */ + void * pn_enc = cnx->crypto_context[ph->epoch].pn_dec; + + picoquic_sack_list_t* sack_list = picoquic_sack_list_from_cnx_context(cnx, ph->pc, ph->l_cid); + ret = picoquic_remove_header_protection_inner(bytes, length, decrypted_bytes, ph, + pn_enc, cnx->is_loss_bit_enabled_incoming, picoquic_sack_list_last(sack_list)); + + return ret; +} + /* * Remove packet protection */ @@ -826,7 +863,7 @@ int picoquic_parse_header_and_decrypt( if (*pcnx == NULL) { if (ph->ptype == picoquic_packet_initial) { - ret = picoquic_screen_initial_packet(quic, packet_length, addr_from, ph, current_time, pcnx, new_ctx_created); + ret = picoquic_screen_initial_packet(quic, bytes, packet_length, addr_from, ph, current_time, pcnx, new_ctx_created); } } else if (!(*pcnx)->client_mode && ph->ptype == picoquic_packet_initial && packet_length < PICOQUIC_ENFORCED_INITIAL_MTU) { @@ -851,7 +888,7 @@ int picoquic_parse_header_and_decrypt( } if (ret == 0) { - /* Remove header protection at this point -- values of bytes will change */ + /* Remove header protection at this point -- values of bytes will not change */ ret = picoquic_remove_header_protection(*pcnx, (uint8_t*)bytes, decrypted_data->data, ph); } diff --git a/picoquic/picoquic.h b/picoquic/picoquic.h index e0b3272ce..8e8df72cc 100644 --- a/picoquic/picoquic.h +++ b/picoquic/picoquic.h @@ -40,7 +40,7 @@ extern "C" { #endif -#define PICOQUIC_VERSION "1.1.19.7" +#define PICOQUIC_VERSION "1.1.19.8" #define PICOQUIC_ERROR_CLASS 0x400 #define PICOQUIC_ERROR_DUPLICATE (PICOQUIC_ERROR_CLASS + 1) #define PICOQUIC_ERROR_AEAD_CHECK (PICOQUIC_ERROR_CLASS + 3) diff --git a/picoquic/picoquic_internal.h b/picoquic/picoquic_internal.h index ea796234c..6f0b7a249 100644 --- a/picoquic/picoquic_internal.h +++ b/picoquic/picoquic_internal.h @@ -1691,6 +1691,8 @@ uint64_t picoquic_get_packet_number64(uint64_t highest, uint64_t mask, uint32_t void picoquic_log_pn_dec_trial(picoquic_cnx_t* cnx); /* For debugging potential PN_ENC corruption */ +int picoquic_remove_header_protection_inner(uint8_t* bytes, size_t length, uint8_t* decrypted_bytes, picoquic_packet_header* ph, void* pn_enc, unsigned int is_loss_bit_enabled_incoming, uint64_t sack_list_last); + size_t picoquic_pad_to_target_length(uint8_t* bytes, size_t length, size_t target); void picoquic_finalize_and_protect_packet(picoquic_cnx_t *cnx, picoquic_packet_t * packet, int ret, diff --git a/picoquic/tls_api.c b/picoquic/tls_api.c index 5d88d92cc..8b1d7396a 100644 --- a/picoquic/tls_api.c +++ b/picoquic/tls_api.c @@ -1449,7 +1449,7 @@ int picoquic_get_initial_aead_context(picoquic_quic_t * quic, int version_index, selected_secret = (is_enc) ? client_secret : server_secret; } - ret = picoquic_set_aead_from_secret(aead_ctx, cipher, 1, selected_secret, prefix_label); + ret = picoquic_set_aead_from_secret(aead_ctx, cipher, is_enc, selected_secret, prefix_label); if (ret == 0) { ret = picoquic_set_pn_enc_from_secret(pn_enc_ctx, cipher, is_enc, selected_secret, prefix_label); } From 82bbd45de5f4929938befb47c0e08bcb141acf64 Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Tue, 26 Mar 2024 22:31:47 -0700 Subject: [PATCH 2/2] Fix memory leak. --- picoquic/packet.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/picoquic/packet.c b/picoquic/packet.c index c8a740844..78b77f89a 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -135,6 +135,16 @@ int picoquic_screen_initial_packet( ret = PICOQUIC_ERROR_MEMORY; } + if (aead_ctx != NULL) { + /* Free the AEAD CTX */ + picoquic_aead_free(aead_ctx); + } + + if (pn_dec_ctx != NULL) { + /* Free the PN encryption context */ + picoquic_cipher_free(pn_dec_ctx); + } + if (ret == 0) { if (quic->enforce_client_only) { /* Cannot create a client connection if the context is client only */