Merge remote-tracking branch 'security/topic/awelzel/217-quic-decrypt-crash'

* security/topic/awelzel/217-quic-decrypt-crash:
  QUIC/decrypt_crypto: Actually check if decryption was successful
  QUIC/decrypt_crypto: Limit payload_length to 10k
  QUIC/decrypt_crypto: Fix decrypting into too small stack buffer

CHANGES

@@ -1,3 +1,28 @@
+7.2.0-dev.8 | 2024-12-16 10:19:18 -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.
+
+  * Fix naming of zeromq package in Coverity workflow (Tim Wojtulewicz, Corelight)
+
+  * Fix naming of cppzmq-dev package in Coverity workflow (Tim Wojtulewicz, Corelight)
+
 7.2.0-dev.2 | 2024-12-13 16:46:18 -0700
 
   * CI: Add missing packages to coverity workflow (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-7.2.0-dev.2
+7.2.0-dev.8

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<uint8_t>& 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<uint8_t>& 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<uint8_t, MAXIMUM_PACKET_LENGTH> decrypt_buffer;
+    // Allocate memory for decryption.
+    std::vector<uint8_t> decrypt_buffer(encrypted_payload_size);
 
     // Setup context
     auto* ctx = get_aes_128_gcm();
@@ -197,7 +202,8 @@ hilti::rt::Bytes decrypt(const std::vector<uint8_t>& 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);

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:<location>)

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

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:<location>)/g" | $SCRIPTS/diff-remove-abspath' btest-diff analyzer.log.cut