diff --git a/CHANGES b/CHANGES index 21fa61a67f..84a427778a 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,24 @@ +7.0.4-10 | 2024-12-16 10:21:46 -0700 + + * QUIC/decrypt_crypto: Actually check if decryption was successful (Arne Welzel, Corelight) + + ...and bail if it wasn't. + + PCAP was produced using OSS-Fuzz input from issue 383379789. + + * QUIC/decrypt_crypto: Limit payload_length to 10k (Arne Welzel, Corelight) + + Given we dynamically allocate memory for decryption, employ a limit + that is unlikely to be hit, but allows for large payloads produced + by the fuzzer or jumbo frames. + + * QUIC/decrypt_crypto: Fix decrypting into too small stack buffer (Arne Welzel, Corelight) + + A QUIC initial packet larger than 1500 bytes could lead to crashes + due to the usage of a fixed size stack buffer for decryption. + + Allocate the necessary memory dynamically on the heap instead. + 7.0.4-5 | 2024-12-13 12:25:43 -0700 * fix for memory management associated with ZAM table iteration (Vern Paxson, Corelight) diff --git a/VERSION b/VERSION index cba4cff318..0fdcca7814 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -7.0.4-5 +7.0.4-10 diff --git a/src/analyzer/protocol/quic/decrypt_crypto.cc b/src/analyzer/protocol/quic/decrypt_crypto.cc index d38bc4154f..fa496413ae 100644 --- a/src/analyzer/protocol/quic/decrypt_crypto.cc +++ b/src/analyzer/protocol/quic/decrypt_crypto.cc @@ -60,7 +60,6 @@ const size_t AEAD_IV_LEN = 12; const size_t AEAD_HP_LEN = 16; const size_t AEAD_SAMPLE_LENGTH = 16; const size_t AEAD_TAG_LENGTH = 16; -const size_t MAXIMUM_PACKET_LENGTH = 1500; const size_t MAXIMUM_PACKET_NUMBER_LENGTH = 4; EVP_CIPHER_CTX* get_aes_128_ecb() { @@ -153,12 +152,17 @@ Function that calls the AEAD decryption routine, and returns the decrypted data. */ hilti::rt::Bytes decrypt(const std::vector& client_key, const hilti::rt::Bytes& all_data, uint64_t payload_length, const DecryptionInformation& decryptInfo) { - int out, out2, res; + int out, out2; if ( payload_length < decryptInfo.packet_number_length + AEAD_TAG_LENGTH ) throw hilti::rt::RuntimeError(hilti::rt::fmt("payload too small %ld < %ld", payload_length, decryptInfo.packet_number_length + AEAD_TAG_LENGTH)); + // Bail on large payloads, somewhat arbitrarily. 10k allows for Jumbo frames + // and sometimes the fuzzer produces packets up to that size as well. + if ( payload_length > 10000 ) + throw hilti::rt::RuntimeError(hilti::rt::fmt("payload_length too large %ld", payload_length)); + const uint8_t* encrypted_payload = data_as_uint8(all_data) + decryptInfo.unprotected_header.size(); int encrypted_payload_size = payload_length - decryptInfo.packet_number_length - AEAD_TAG_LENGTH; @@ -173,7 +177,8 @@ hilti::rt::Bytes decrypt(const std::vector& client_key, const hilti::rt const void* tag_to_check = all_data.data() + decryptInfo.unprotected_header.size() + encrypted_payload_size; int tag_to_check_length = AEAD_TAG_LENGTH; - std::array decrypt_buffer; + // Allocate memory for decryption. + std::vector decrypt_buffer(encrypted_payload_size); // Setup context auto* ctx = get_aes_128_gcm(); @@ -197,7 +202,8 @@ hilti::rt::Bytes decrypt(const std::vector& client_key, const hilti::rt EVP_CipherUpdate(ctx, decrypt_buffer.data(), &out, encrypted_payload, encrypted_payload_size); // Validate whether the decryption was successful or not - EVP_CipherFinal_ex(ctx, NULL, &out2); + if ( EVP_CipherFinal_ex(ctx, NULL, &out2) == 0 ) + throw hilti::rt::RuntimeError("decryption failed"); // Copy the decrypted data from the decrypted buffer into a Bytes instance. return hilti::rt::Bytes(decrypt_buffer.data(), decrypt_buffer.data() + out); diff --git a/testing/btest/Baseline/scripts.base.protocols.quic.decrypt-crash/analyzer.log.cut b/testing/btest/Baseline/scripts.base.protocols.quic.decrypt-crash/analyzer.log.cut new file mode 100644 index 0000000000..1a3b7336c7 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.quic.decrypt-crash/analyzer.log.cut @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +ts uid cause analyzer_kind analyzer_name failure_reason +1733909795.417573 C4J4Th3PJpwUYZZ6gc violation protocol QUIC decryption failed (<...>/QUIC.spicy:) diff --git a/testing/btest/Baseline/scripts.base.protocols.quic.decrypt-crash/conn.log.cut b/testing/btest/Baseline/scripts.base.protocols.quic.decrypt-crash/conn.log.cut new file mode 100644 index 0000000000..e9151b0dee --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.quic.decrypt-crash/conn.log.cut @@ -0,0 +1,5 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +ts uid proto history service +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h udp D - +XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc udp D - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 tcp F ftp diff --git a/testing/btest/Traces/quic/383379789-decrypt-crash.pcap b/testing/btest/Traces/quic/383379789-decrypt-crash.pcap new file mode 100644 index 0000000000..aeb9808508 Binary files /dev/null and b/testing/btest/Traces/quic/383379789-decrypt-crash.pcap differ diff --git a/testing/btest/scripts/base/protocols/quic/decrypt-crash.zeek b/testing/btest/scripts/base/protocols/quic/decrypt-crash.zeek new file mode 100644 index 0000000000..4b64b04dc4 --- /dev/null +++ b/testing/btest/scripts/base/protocols/quic/decrypt-crash.zeek @@ -0,0 +1,8 @@ +# @TEST-DOC: Trace produced by OSS-Fuzz triggered a crash due to using a too small local buffer for decryption. + +# @TEST-REQUIRES: ${SCRIPTS}/have-spicy +# @TEST-EXEC: zeek -Cr $TRACES/quic/383379789-decrypt-crash.pcap base/protocols/quic %INPUT +# @TEST-EXEC: zeek-cut -m ts uid proto history service < conn.log > conn.log.cut +# @TEST-EXEC: zeek-cut -m ts uid cause analyzer_kind analyzer_name failure_reason < analyzer.log > analyzer.log.cut +# @TEST-EXEC: btest-diff conn.log.cut +# @TEST-EXEC: TEST_DIFF_CANONIFIER='sed -E "s/\((.+)\.spicy:[0-9]+:[0-9]+(-[0-9]+:[0-9]+)?\)/(\1.spicy:)/g" | $SCRIPTS/diff-remove-abspath' btest-diff analyzer.log.cut