decode: cleanup packet properly on bad packets

In case of bad IPv4, TCP or UDP, the per packet ip4vars/tcpvars/udpvar
structures would not be cleaned up because the cleanup depends on the
'header' pointer being set, but the error handling would unset that.

This could mean these structures were already filled with values before
the error was detected. As packets were recycled, the next packet decoding
would use this unclean structure.

To make things worse these structures are part of unions. IPv4/IPv6 and
TCP/ICMPv4/ICMPv6 share the same memory location.

LibFuzzer+UBSAN found this both locally and in Oss-Fuzz:

decode-ipv6.c:654:9: runtime error: load of value 6, which is not a valid value for type 'bool'
    #0 0x6146f0 in DecodeIPV6 /src/suricata/src/decode-ipv6.c:654:9
    #1 0x617e96 in DecodeNull /src/suricata/src/decode-null.c:70:13
    #2 0x9dd8a4 in DecodePcapFile /src/suricata/src/source-pcap-file.c:412:9
    #3 0x4c8ed2 in LLVMFuzzerTestOneInput /src/suricata/src/tests/fuzz/fuzz_sigpcap.c:158:25
    #4 0x457e51 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:556:15
    #5 0x457575 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:470:3
    #6 0x459917 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:698:19
    #7 0x45a6a5 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:830:5
    #8 0x448728 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:824:6
    #9 0x472552 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
    #10 0x7ff0d097b82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x41bde8 in _start (/out/fuzz_sigpcap+0x41bde8)

Bug: #3496

diff --git a/src/decode-ipv4.c b/src/decode-ipv4.c
index 6c2998cc6d749a956086a67e025c3ffed54870b8..5d9d12a8b93e31a7ed0c82592a12be9975b33588 100644 (file)
--- a/src/decode-ipv4.c
+++ b/src/decode-ipv4.c
@@ -524,7 +524,7 @@ int DecodeIPV4(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
     /* do the actual decoding */
     if (unlikely(DecodeIPV4Packet (p, pkt, len) < 0)) {
         SCLogDebug("decoding IPv4 packet failed");
-        p->ip4h = NULL;
+        CLEAR_IPV4_PACKET((p));
         return TM_ECODE_FAILED;
     }
     p->proto = IPV4_GET_IPPROTO(p);

diff --git a/src/decode-ipv6.c b/src/decode-ipv6.c
index f57b1d34e13fee5817dceb1b30e6596c82dd704c..b20440145ac74ebd8985dbed15ef38850e08b189 100644 (file)
--- a/src/decode-ipv6.c
+++ b/src/decode-ipv6.c
@@ -584,7 +584,7 @@ int DecodeIPV6(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, const uint8_t *
     /* do the actual decoding */
     int ret = DecodeIPV6Packet (tv, dtv, p, pkt, len);
     if (unlikely(ret < 0)) {
-        p->ip6h = NULL;
+        CLEAR_IPV6_PACKET(p);
         return TM_ECODE_FAILED;
     }
 

diff --git a/src/decode-sctp.c b/src/decode-sctp.c
index 6de50d41b0fed48afca35ba787b2b4f0d9e21568..0346dfb3e2ed7554ea376ddcc7497b52b0ad7963 100644 (file)
--- a/src/decode-sctp.c
+++ b/src/decode-sctp.c
@@ -65,7 +65,7 @@ int DecodeSCTP(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
     StatsIncr(tv, dtv->counter_sctp);
 
     if (unlikely(DecodeSCTPPacket(tv, p,pkt,len) < 0)) {
-        p->sctph = NULL;
+        CLEAR_SCTP_PACKET(p);
         return TM_ECODE_FAILED;
     }
 

diff --git a/src/decode-tcp.c b/src/decode-tcp.c
index b4aeb0e4bf84287d77f4ffdbaf02e16b89398263..c3f0ad240aa307c0a612fee44dad80e40a11272c 100644 (file)
--- a/src/decode-tcp.c
+++ b/src/decode-tcp.c
@@ -238,7 +238,7 @@ int DecodeTCP(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
 
     if (unlikely(DecodeTCPPacket(tv, p, pkt,len) < 0)) {
         SCLogDebug("invalid TCP packet");
-        p->tcph = NULL;
+        CLEAR_TCP_PACKET(p);
         return TM_ECODE_FAILED;
     }
 

diff --git a/src/decode-udp.c b/src/decode-udp.c
index 436b10a132f506648b22173b14953f27d984c2bd..a4bb1dd5eda6b732aa19648c7f82d4503cba875e 100644 (file)
--- a/src/decode-udp.c
+++ b/src/decode-udp.c
@@ -77,7 +77,7 @@ int DecodeUDP(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p,
     StatsIncr(tv, dtv->counter_udp);
 
     if (unlikely(DecodeUDPPacket(tv, p, pkt,len) < 0)) {
-        p->udph = NULL;
+        CLEAR_UDP_PACKET(p);
         return TM_ECODE_FAILED;
     }