Crash in picoquic_aead_get_checksum_length due to state mismatch or unprotected

fbussery commented 11 months ago

Problem on picoquic vesion: https://github.com/private-octopus/picoquic/commit/1e2979e8db0957c8ee798940091c4d0ef13bf8af
Using it here as a server

Sometime, I have the following crash due to the call of apicoquic_protect_packet called with a null aead_context.

Below the kind of stack we have:

#4  picoquic_aead_get_checksum_length (aead_context=aead_context@entry=0x0)
    at picoquic/tls_api.c:2266
#5  0x00005603e3cfa38c in picoquic_protect_packet (cnx=cnx@entry=0x5603e4c3c800, ptype=ptype@entry=picoquic_packet_initial, 
    bytes=bytes@entry=0x5603e478fb8a "", sequence_number=124746, length=length@entry=39, header_length=30, 
    send_buffer=0x5603e4c9fe10 "...", send_buffer_max=1232, aead_context=0x0, 
    pn_enc=0x0, path_x=0x5603e4c55de0, current_time=1697029105359355)
    at picoquic/sender.c:774
#6  0x00005603e3cfb74b in picoquic_finalize_and_protect_packet (cnx=0x5603e4c3c800, packet=0x5603e478faf0, ret=<optimized out>, length=39, 
    header_length=<optimized out>, checksum_overhead=8, send_length=<optimized out>, send_buffer=<optimized out>, send_buffer_max=<optimized out>, 
    path_x=<optimized out>, current_time=<optimized out>)
    at picoquic/sender.c:1266
#7  0x00005603e3cfed41 in picoquic_prepare_packet_server_init (cnx=cnx@entry=0x5603e4c3c800, path_x=0x5603e4c55de0, packet=packet@entry=0x5603e478faf0, 
    current_time=current_time@entry=1697029105359355, send_buffer=<optimized out>, send_buffer_max=<optimized out>, send_length=<optimized out>, 
    next_wake_time=<optimized out>, is_initial_sent=<optimized out>)
    at picoquic/sender.c:3013
#8  0x00005603e3d037e8 in picoquic_prepare_segment (is_initial_sent=0x7ffc4c1943ac, next_wake_time=0x7ffc4c1943b0, send_length=0x7ffc4c1943b8, 
    send_buffer_max=<optimized out>, 
    send_buffer=0x5603e4c9fe10 "...", current_time=1697029105359355, 
    packet=0x5603e478faf0, path_x=<optimized out>, cnx=0x5603e4c3c800)
    at picoquic/sender.c:4350
#9  picoquic_prepare_packet_ex (cnx=cnx@entry=0x5603e4c3c800, current_time=<optimized out>, 
    send_buffer=send_buffer@entry=0x5603e4c9fe10 "...", 
    send_buffer_max=send_buffer_max@entry=65535, send_length=send_length@entry=0x7ffc4c1946d0, p_addr_to=p_addr_to@entry=0x7ffc4c194640, 
    p_addr_from=<optimized out>, if_index=<optimized out>, send_msg_size=<optimized out>)
    at picoquic/sender.c:4799
#10 0x00005603e3d04972 in picoquic_prepare_next_packet_ex (send_msg_size=0x0, p_last_cnx=0x0, log_cid=0x0, if_index=0x7ffc4c1946cc, 
    p_addr_from=0x7ffc4c1945c0, p_addr_to=0x7ffc4c194640, send_length=0x7ffc4c1946d0, send_buffer_max=65535, 
    send_buffer=0x5603e4c9fe10 "...", current_time=<optimized out>, 
    quic=0x5603e44e5700)
    at picoquic/sender.c:4984
#11 picoquic_prepare_next_packet (quic=0x5603e44e5700, current_time=<optimized out>, 
    send_buffer=0x5603e4c9fe10 "H...", send_buffer_max=65535, 
    send_length=0x7ffc4c1946d0, p_addr_to=0x7ffc4c194640, p_addr_from=0x7ffc4c1945c0, if_index=0x7ffc4c1946cc, log_cid=0x0, p_last_cnx=0x0)

In fact, we did noticed that

cnx_state is picoquic_state_server_handshake
packet->ptype is picoquic_packet_initial

There may be some state mismatch that drive to that?

I'm wondering why In protect picoquic_protect_packet, we use picoquic_aead_get_checksum_length instead of picoquic_get_checksum_length which protect picoquic_aead_get_checksum_length against null cnx->crypto_context[epoch].aead_encrypt.

huitema commented 11 months ago

Good catch, and thanks for forwarding the call stack. Yes, we should clean this asap.

huitema commented 11 months ago

@fbussery, please check whether PR #1563 solves the issue.

private-octopus / picoquic

Crash in picoquic_aead_get_checksum_length due to state mismatch or unprotected #1561