search

private-octopus / picoquic

Minimal implementation of the QUIC protocol
MIT License
561 stars 165 forks source link

Valgrind reports uninialized value in EVP_DecryptFinal_ex #1417

Closed huitema closed 1 year ago

huitema commented 1 year ago

Running valgrind and using OpenSSL V3, we get reports such as:

==4410== HEAP SUMMARY:
==4410==     in use at exit: 0 bytes in 0 blocks
==4410==   total heap usage: 869,652 allocs, 869,652 frees, 55,528,993 bytes allocated
==4410== 
==4410== All heap blocks were freed -- no leaks are possible
==4410== 
==4410== ERROR SUMMARY: 50 errors from 1 contexts (suppressed: 0 from 0)
==4410== 
==4410== 50 errors in context 1 of 1:
==4410== Conditional jump or move depends on uninitialised value(s)
==4410==    at 0x4AF62A4: ??? (in /usr/lib/x86_64-linux-gnu/libcrypto.so.3)
==4410==    by 0x4AF6581: ??? (in /usr/lib/x86_64-linux-gnu/libcrypto.so.3)
==4410==    by 0x49F70F4: EVP_DecryptFinal_ex (in /usr/lib/x86_64-linux-gnu/libcrypto.so.3)
==4410==    by 0x1B53E4: aead_do_decrypt (openssl.c:962)
==4410==    by 0x1A7503: picoquic_server_encrypt_ticket_call_back (in /home/runner/work/picoquic/picoquic/build/picoquic_ct)
==4410==    by 0x1CA3E6: try_psk_handshake (picotls.c:3544)
==4410==    by 0x1CA3E6: server_handle_hello (picotls.c:3994)
==4410==    by 0x1BF6D5: handle_handshake_record.part.0 (picotls.c:4742)
==4410==    by 0x1CE0AF: handle_handshake_record (picotls.c:4717)
==4410==    by 0x1CE0AF: ptls_server_handle_message (picotls.c:5463)
==4410==    by 0x1ABFE3: picoquic_tls_stream_process (in /home/runner/work/picoquic/picoquic/build/picoquic_ct)
==4410==    by 0x187004: picoquic_incoming_client_initial (in /home/runner/work/picoquic/picoquic/build/picoquic_ct)
==4410==    by 0x1892C0: picoquic_incoming_segment (in /home/runner/work/picoquic/picoquic/build/picoquic_ct)
==4410==    by 0x1896B0: picoquic_incoming_packet_ex (in /home/runner/work/picoquic/picoquic/build/picoquic_ct)
==4410==  Uninitialised value was created by a stack allocation
==4410==    at 0x185090: picoquic_remove_header_protection (in /home/runner/work/picoquic/picoquic/build/picoquic_ct)
==4410== 
==4410== ERROR SUMMARY: 50 errors from 1 contexts (suppressed: 0 from 0)

This issue occurs when using the OpenSSL V3 implementation of AES GCM 128. The same test reported here uses the PTLS "fusion" implementation for the packet exchanges, and that one does not trigger any report. The reports here come from using the "openssl" implementation of encrypting and decrypting session tickets.

There is an OpenSSL bug for the same issue.

huitema commented 1 year ago

The fix was to:

1) Move the "valgrind" test from a "no-fusion" branch to the "ci-test" branch 2) Force the ticket encryption to use "fusion", even if that consumes more mmory than using the OpenSSL variant of AES GCM.

Consider replacing that when the OpenSSL bug is fixed.