From 9660525aa1b5063c668ea54487dcdad02139900e Mon Sep 17 00:00:00 2001 From: huitema Date: Wed, 4 Dec 2024 11:42:27 -0800 Subject: [PATCH 1/5] Remove unused log_pn_dec code --- picoquic/packet.c | 9 +++++---- picoquic/picoquic_internal.h | 3 ++- picoquic/sender.c | 3 ++- picoquic/tls_api.c | 2 ++ 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/picoquic/packet.c b/picoquic/packet.c index 6a4ae152a..ad0618898 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -535,7 +535,7 @@ uint64_t picoquic_get_packet_number64(uint64_t highest, uint64_t mask, uint32_t return pn64; } - +#ifdef LOG_PN_DEC /* Debug code used to test whether the PN decryption works as expected. */ @@ -567,7 +567,7 @@ void picoquic_log_pn_dec_trial(picoquic_cnx_t* cnx) demask_bytes[0], demask_bytes[1], demask_bytes[2], demask_bytes[3], demask_bytes[4]); } } - +#endif /* * Remove header protection @@ -816,12 +816,13 @@ size_t picoquic_remove_packet_protection(picoquic_cnx_t* cnx, decoded = ph->payload_length + 1; } } - +#ifdef LOG_PN_DEC /* Add here a check that the PN key is still valid. */ if (decoded > ph->payload_length) { picoquic_log_pn_dec_trial(cnx); } - +#endif + /* by conventions, values larger than input indicate error */ return decoded; } diff --git a/picoquic/picoquic_internal.h b/picoquic/picoquic_internal.h index 1202acbef..e88959e2f 100644 --- a/picoquic/picoquic_internal.h +++ b/picoquic/picoquic_internal.h @@ -1730,8 +1730,9 @@ void picoquic_protect_packet_header(uint8_t* send_buffer, size_t pn_offset, uint size_t picoquic_protect_packet(picoquic_cnx_t* cnx, picoquic_packet_type_enum ptype, uint8_t* bytes, uint64_t sequence_number, size_t length, size_t header_length, uint8_t* send_buffer, size_t send_buffer_max, void* aead_context, void* pn_enc, picoquic_path_t* path_x, uint64_t current_time); uint64_t picoquic_get_packet_number64(uint64_t highest, uint64_t mask, uint32_t pn); - +#ifdef LOG_PN_DEC void picoquic_log_pn_dec_trial(picoquic_cnx_t* cnx); /* For debugging potential PN_ENC corruption */ +#endif 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); diff --git a/picoquic/sender.c b/picoquic/sender.c index 7b4ea1422..5724a5c5a 100644 --- a/picoquic/sender.c +++ b/picoquic/sender.c @@ -2758,9 +2758,10 @@ void picoquic_ready_state_transition(picoquic_cnx_t* cnx, uint64_t current_time) cnx->min_ack_delay_remote = cnx->ack_delay_remote; } } - +#ifdef LOG_PN_DEC /* Perform a check of the PN decryption key, for sanity */ picoquic_log_pn_dec_trial(cnx); +#endif } uint8_t * picoquic_prepare_path_challenge_frames(picoquic_cnx_t* cnx, picoquic_path_t* path_x, diff --git a/picoquic/tls_api.c b/picoquic/tls_api.c index cf4146270..9ebdc509f 100644 --- a/picoquic/tls_api.c +++ b/picoquic/tls_api.c @@ -1580,7 +1580,9 @@ void picoquic_apply_rotated_keys(picoquic_cnx_t * cnx, int is_enc) cnx->crypto_context_new.aead_encrypt = NULL; cnx->key_phase_enc ^= 1; +#ifdef LOG_PN_DEC picoquic_log_pn_dec_trial(cnx); +#endif } else { if (cnx->crypto_context_old.aead_decrypt != NULL) { From 03e76eefe69c0baf9320ef707b3672f1ce56b543 Mon Sep 17 00:00:00 2001 From: huitema Date: Wed, 4 Dec 2024 11:56:40 -0800 Subject: [PATCH 2/5] Remove unused condition in screen_initial function --- picoquic/packet.c | 57 ++---------------------------------- picoquic/picoquic_internal.h | 4 --- picoquic/sender.c | 4 --- picoquic/tls_api.c | 3 -- 4 files changed, 2 insertions(+), 66 deletions(-) diff --git a/picoquic/packet.c b/picoquic/packet.c index ad0618898..7cf12ec2f 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -111,7 +111,8 @@ int picoquic_screen_initial_packet( /* Cannot have reserved bit set before negotiation completes */ ret = PICOQUIC_ERROR_PACKET_HEADER_PARSING; } - else if (*pcnx == NULL) { + else { + /* This code assumes that *pcnx is always null when screen initial is called. / /* Verify the AEAD checkum */ void* aead_ctx = NULL; void* pn_dec_ctx = NULL; @@ -196,21 +197,6 @@ int picoquic_screen_initial_packet( } } } - else { - /* Context already exists. Check that the incoming packet is consistent. */ - if ((!(*pcnx)->client_mode && picoquic_compare_connection_id(&ph->dest_cnx_id, &(*pcnx)->initial_cnxid) == 0) || - picoquic_compare_connection_id(&ph->dest_cnx_id, &(*pcnx)->path[0]->p_local_cnxid->cnx_id) == 0) { - /* Verify that the source CID matches expectation */ - if (picoquic_is_connection_id_null(&(*pcnx)->path[0]->p_remote_cnxid->cnx_id)) { - (*pcnx)->path[0]->p_remote_cnxid->cnx_id = ph->srce_cnx_id; - } - else if (picoquic_compare_connection_id(&(*pcnx)->path[0]->p_remote_cnxid->cnx_id, &ph->srce_cnx_id) != 0) { - DBG_PRINTF("Error wrong srce cnxid (%d), type: %d, epoch: %d, pc: %d, pn: %d\n", - (*pcnx)->client_mode, ph->ptype, ph->epoch, ph->pc, (int)ph->pn); - ret = PICOQUIC_ERROR_UNEXPECTED_PACKET; - } - } - } return ret; } @@ -535,39 +521,6 @@ uint64_t picoquic_get_packet_number64(uint64_t highest, uint64_t mask, uint32_t return pn64; } -#ifdef LOG_PN_DEC -/* Debug code used to test whether the PN decryption works as expected. - */ - -void picoquic_log_pn_dec_trial(picoquic_cnx_t* cnx) -{ - if (cnx->quic->log_pn_dec && (cnx->quic->F_log != NULL || cnx->f_binlog != NULL)){ - void* pn_dec = cnx->crypto_context[picoquic_epoch_1rtt].pn_dec; - void* pn_enc = cnx->crypto_context[picoquic_epoch_1rtt].pn_enc; - uint8_t test_iv[32] = { - 0, 1, 3, 4, 4, 6, 7, 8, 9, - 0, 1, 3, 4, 4, 6, 7, 8, 9, - 0, 1, 3, 4, 4, 6, 7, 8, 9, - 0, 1 }; - size_t mask_length = 5; - uint8_t mask_bytes[5] = { 0, 0, 0, 0, 0 }; - uint8_t demask_bytes[5] = { 0, 0, 0, 0, 0 }; - - if (pn_enc != NULL) { - picoquic_pn_encrypt(pn_enc, test_iv, mask_bytes, mask_bytes, mask_length); - } - - if (pn_dec != NULL) { - picoquic_pn_encrypt(pn_dec, test_iv, demask_bytes, demask_bytes, mask_length); - } - - picoquic_log_app_message(cnx, "1RTT PN ENC/DEC, Phi: %d, signature = %02x%02x%02x%02x%02x, %02x%02x%02x%02x%02x", - cnx->key_phase_enc, - mask_bytes[0], mask_bytes[1], mask_bytes[2], mask_bytes[3], mask_bytes[4], - demask_bytes[0], demask_bytes[1], demask_bytes[2], demask_bytes[3], demask_bytes[4]); - } -} -#endif /* * Remove header protection @@ -816,12 +769,6 @@ size_t picoquic_remove_packet_protection(picoquic_cnx_t* cnx, decoded = ph->payload_length + 1; } } -#ifdef LOG_PN_DEC - /* Add here a check that the PN key is still valid. */ - if (decoded > ph->payload_length) { - picoquic_log_pn_dec_trial(cnx); - } -#endif /* by conventions, values larger than input indicate error */ return decoded; diff --git a/picoquic/picoquic_internal.h b/picoquic/picoquic_internal.h index e88959e2f..fb6de916e 100644 --- a/picoquic/picoquic_internal.h +++ b/picoquic/picoquic_internal.h @@ -655,7 +655,6 @@ typedef struct st_picoquic_quic_t { unsigned int use_unique_log_names : 1; /* Add 64 bit random number to log names for uniqueness */ unsigned int dont_coalesce_init : 1; /* test option to turn of packet coalescing on server */ unsigned int one_way_grease_quic_bit : 1; /* Grease of QUIC bit, but do not announce support */ - unsigned int log_pn_dec : 1; /* Log key hashes on key changes to debug crypto */ unsigned int random_initial : 2; /* Randomize the initial PN number */ unsigned int packet_train_mode : 1; /* Tune pacing for sending packet trains */ unsigned int use_constant_challenges : 1; /* Use predictable challenges when producing constant logs. */ @@ -1730,9 +1729,6 @@ void picoquic_protect_packet_header(uint8_t* send_buffer, size_t pn_offset, uint size_t picoquic_protect_packet(picoquic_cnx_t* cnx, picoquic_packet_type_enum ptype, uint8_t* bytes, uint64_t sequence_number, size_t length, size_t header_length, uint8_t* send_buffer, size_t send_buffer_max, void* aead_context, void* pn_enc, picoquic_path_t* path_x, uint64_t current_time); uint64_t picoquic_get_packet_number64(uint64_t highest, uint64_t mask, uint32_t pn); -#ifdef LOG_PN_DEC -void picoquic_log_pn_dec_trial(picoquic_cnx_t* cnx); /* For debugging potential PN_ENC corruption */ -#endif 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); diff --git a/picoquic/sender.c b/picoquic/sender.c index 5724a5c5a..2de40eaaa 100644 --- a/picoquic/sender.c +++ b/picoquic/sender.c @@ -2758,10 +2758,6 @@ void picoquic_ready_state_transition(picoquic_cnx_t* cnx, uint64_t current_time) cnx->min_ack_delay_remote = cnx->ack_delay_remote; } } -#ifdef LOG_PN_DEC - /* Perform a check of the PN decryption key, for sanity */ - picoquic_log_pn_dec_trial(cnx); -#endif } uint8_t * picoquic_prepare_path_challenge_frames(picoquic_cnx_t* cnx, picoquic_path_t* path_x, diff --git a/picoquic/tls_api.c b/picoquic/tls_api.c index 9ebdc509f..815aa3ddc 100644 --- a/picoquic/tls_api.c +++ b/picoquic/tls_api.c @@ -1580,9 +1580,6 @@ void picoquic_apply_rotated_keys(picoquic_cnx_t * cnx, int is_enc) cnx->crypto_context_new.aead_encrypt = NULL; cnx->key_phase_enc ^= 1; -#ifdef LOG_PN_DEC - picoquic_log_pn_dec_trial(cnx); -#endif } else { if (cnx->crypto_context_old.aead_decrypt != NULL) { From 492645fed8758bc718bffa9f886046714a90be48 Mon Sep 17 00:00:00 2001 From: huitema Date: Wed, 4 Dec 2024 12:14:01 -0800 Subject: [PATCH 3/5] Remove unused epoch=1rtt branch --- picoquic/packet.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/picoquic/packet.c b/picoquic/packet.c index 7cf12ec2f..ed4cdb490 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -54,8 +54,6 @@ picoquic_packet_type_enum picoquic_parse_long_packet_type(uint8_t flags, int ver case 3: /* Retry */ pt = picoquic_packet_retry; break; - default: /* Not a valid packet type */ - break; } break; case PICOQUIC_V2_VERSION: @@ -76,8 +74,6 @@ picoquic_packet_type_enum picoquic_parse_long_packet_type(uint8_t flags, int ver case 0: /* Retry */ pt = picoquic_packet_retry; break; - default: /* Not a valid packet type */ - break; } break; default: @@ -112,7 +108,7 @@ int picoquic_screen_initial_packet( ret = PICOQUIC_ERROR_PACKET_HEADER_PARSING; } else { - /* This code assumes that *pcnx is always null when screen initial is called. / + /* This code assumes that *pcnx is always null when screen initial is called. */ /* Verify the AEAD checkum */ void* aead_ctx = NULL; void* pn_dec_ctx = NULL; @@ -315,12 +311,8 @@ int picoquic_parse_long_packet_header( ph->pc = picoquic_packet_context_initial; ph->epoch = picoquic_epoch_initial; break; - default: /* Not a valid packet type */ - DBG_PRINTF("Packet type is not recognized: v=%08x, p[0]= 0x%02x\n", ph->vn, flags); - ph->ptype = picoquic_packet_error; - ph->version_index = -1; - ph->pc = 0; - break; + /* No default branch in this statement, because there are only 4 possible types + * parsed in picoquic_parse_long_packet_type */ } if (ph->ptype == picoquic_packet_retry) { @@ -754,16 +746,8 @@ size_t picoquic_remove_packet_protection(picoquic_cnx_t* cnx, /* TODO: get rid of handshake some time after handshake complete */ /* For all the other epochs, there is a single crypto context and no key rotation */ if (cnx->crypto_context[ph->epoch].aead_decrypt != NULL) { - if (cnx->is_multipath_enabled && ph->ptype == picoquic_packet_1rtt_protected) { - decoded = picoquic_aead_decrypt_mp(decoded_bytes + ph->offset, - bytes + ph->offset, ph->payload_length, - ph->l_cid->path_id, ph->pn64, decoded_bytes, ph->offset, - cnx->crypto_context[picoquic_epoch_1rtt].aead_decrypt); - } - else { - decoded = picoquic_aead_decrypt_generic(decoded_bytes + ph->offset, - bytes + ph->offset, ph->payload_length, ph->pn64, decoded_bytes, ph->offset, cnx->crypto_context[ph->epoch].aead_decrypt); - } + decoded = picoquic_aead_decrypt_generic(decoded_bytes + ph->offset, + bytes + ph->offset, ph->payload_length, ph->pn64, decoded_bytes, ph->offset, cnx->crypto_context[ph->epoch].aead_decrypt); } else { decoded = ph->payload_length + 1; From 1d9d2467d981cb852e8c531c6d995afb7d006640 Mon Sep 17 00:00:00 2001 From: huitema Date: Wed, 4 Dec 2024 12:17:38 -0800 Subject: [PATCH 4/5] Remove redundant payload length test --- picoquic/packet.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/picoquic/packet.c b/picoquic/packet.c index ed4cdb490..a76b7a47f 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -779,15 +779,7 @@ int picoquic_parse_header_and_decrypt( *new_ctx_created = 0; if (ret == 0 ) { - if (ph->offset + ph->payload_length > PICOQUIC_MAX_PACKET_SIZE) { - ret = PICOQUIC_ERROR_PACKET_TOO_LONG; - if (*new_ctx_created) { - picoquic_delete_cnx(*pcnx); - *pcnx = NULL; - *new_ctx_created = 0; - } - } - else if (ph->ptype != picoquic_packet_version_negotiation && + if (ph->ptype != picoquic_packet_version_negotiation && ph->ptype != picoquic_packet_retry && ph->ptype != picoquic_packet_error) { /* TODO: clarify length, payload length, packet length -- special case of initial packet */ length = ph->offset + ph->payload_length; From fe7281262fc75fba7650d1d76fb6a6e57bf39f6a Mon Sep 17 00:00:00 2001 From: huitema Date: Wed, 4 Dec 2024 12:25:17 -0800 Subject: [PATCH 5/5] Remove unused code --- picoquic/packet.c | 44 -------------------------------------------- 1 file changed, 44 deletions(-) diff --git a/picoquic/packet.c b/picoquic/packet.c index a76b7a47f..74c08de8e 100644 --- a/picoquic/packet.c +++ b/picoquic/packet.c @@ -1381,50 +1381,6 @@ int picoquic_incoming_client_initial( { int ret = 0; -#if 0 - /* Logic to test the retry token. - * the "check token" value may change between the time a previous client connection - * attempt triggered a "retry" and the time that retry arrive, so it is not wrong - * to receive a retry token even if not required, as long as it decrypts correctly. - */ - if ((*pcnx)->cnx_state == picoquic_state_server_init && - !(*pcnx)->quic->server_busy) { - int is_address_blocked = !(*pcnx)->quic->is_port_blocking_disabled && picoquic_check_addr_blocked(addr_from); - int is_new_token = 0; - int is_wrong_token = 0; - if (ph->token_length > 0) { - if (picoquic_verify_retry_token((*pcnx)->quic, addr_from, current_time, - &is_new_token, &(*pcnx)->original_cnxid, &ph->dest_cnx_id, ph->pn, - ph->token_bytes, ph->token_length, new_context_created) != 0) { - is_wrong_token = 1; - } - else { - (*pcnx)->initial_validated = 1; - } - } - if (is_wrong_token && !is_new_token) { - (void)picoquic_connection_error(*pcnx, PICOQUIC_TRANSPORT_INVALID_TOKEN, 0); - ret = PICOQUIC_ERROR_INVALID_TOKEN; - } - else if (((*pcnx)->quic->check_token || is_address_blocked) && (ph->token_length == 0 || is_wrong_token)){ - uint8_t token_buffer[256]; - size_t token_size; - - if (picoquic_prepare_retry_token((*pcnx)->quic, addr_from, - current_time + PICOQUIC_TOKEN_DELAY_SHORT, &ph->dest_cnx_id, - &(*pcnx)->path[0]->p_local_cnxid->cnx_id, ph->pn, - token_buffer, sizeof(token_buffer), &token_size) != 0) { - ret = PICOQUIC_ERROR_MEMORY; - } - else { - picoquic_queue_stateless_retry(quic, ph, - addr_from, addr_to, if_index_to, token_buffer, token_size); - ret = PICOQUIC_ERROR_RETRY; - } - } - } -#endif - if (ret == 0) { if ((*pcnx)->path[0]->p_local_cnxid->cnx_id.id_len > 0 && picoquic_compare_connection_id(&ph->dest_cnx_id, &(*pcnx)->path[0]->p_local_cnxid->cnx_id) == 0) {