From e14b695497f858efc3c10d532e8e166838e10c80 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 28 Oct 2021 16:55:02 +0200 Subject: [PATCH 1/7] Accept packets that use tcp segment offloading. When checksum offloading is enabled, we now forward packets that have 0 header lengths set - and assume that they have TSO enabled. If checksum offloading is not enabled, we drop the packets. Addresses GH-1829 --- src/IP.h | 6 +++++- src/packet_analysis/protocol/ip/IP.cc | 11 ++++++++--- src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc | 4 +++- src/packet_analysis/protocol/tcp/TCP.cc | 4 ++++ .../core.ip-broken-header/ip-bogus-header-weird.log | 1 - 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/IP.h b/src/IP.h index 3c2fbef813..2b670cf72e 100644 --- a/src/IP.h +++ b/src/IP.h @@ -415,7 +415,11 @@ public: uint16_t PayloadLen() const { if ( ip4 ) - return ntohs(ip4->ip_len) - ip4->ip_hl * 4; + { + // prevent overflow in case of segment offloading/zeroed header length. + auto total_len = ntohs(ip4->ip_len); + return total_len ? total_len - ip4->ip_hl * 4 : 0; + } return ntohs(ip6->ip6_plen) + 40 - ip6_hdrs->TotalLength(); } diff --git a/src/packet_analysis/protocol/ip/IP.cc b/src/packet_analysis/protocol/ip/IP.cc index e7ea5e75da..cb45f02e7e 100644 --- a/src/packet_analysis/protocol/ip/IP.cc +++ b/src/packet_analysis/protocol/ip/IP.cc @@ -81,8 +81,13 @@ bool IPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) // TCP segmentation offloading can zero out the ip_len field. Weird("ip_hdr_len_zero", packet); - // Cope with the zero'd out ip_len field by using the caplen. - total_len = packet->cap_len - hdr_size; + if ( detail::ignore_checksums ) + // Cope with the zero'd out ip_len field by using the caplen. + total_len = packet->cap_len - hdr_size; + else + // If this is caused by segmentation offloading, the checksum will + // also be incorrect. If checksum validation is enabled - jus tbail here. + return false; } if ( packet->len < total_len + hdr_size ) @@ -236,7 +241,7 @@ bool IPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) packet->proto = proto; // Double check the lengths one more time before forwarding this on. - if ( packet->ip_hdr->TotalLen() < packet->ip_hdr->HdrLen() ) + if ( total_len < packet->ip_hdr->HdrLen() ) { Weird("bogus_IP_header_lengths", packet); return false; diff --git a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc index 785d914498..e18e46b2c1 100644 --- a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc +++ b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc @@ -115,7 +115,9 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt bool IPBasedAnalyzer::CheckHeaderTrunc(size_t min_hdr_len, size_t remaining, Packet* packet) { - if ( packet->ip_hdr->PayloadLen() < min_hdr_len ) + // If segment offloading or similar is enabled, the payload len will return 0. + // Thus, let's ignore that case. + if ( packet->ip_hdr->PayloadLen() && packet->ip_hdr->PayloadLen() < min_hdr_len ) { Weird("truncated_header", packet); return false; diff --git a/src/packet_analysis/protocol/tcp/TCP.cc b/src/packet_analysis/protocol/tcp/TCP.cc index 5a45e28438..cdd8a4c80f 100644 --- a/src/packet_analysis/protocol/tcp/TCP.cc +++ b/src/packet_analysis/protocol/tcp/TCP.cc @@ -96,6 +96,10 @@ void TCPAnalyzer::DeliverPacket(Connection* c, double t, bool is_orig, int remai { const u_char* data = pkt->ip_hdr->Payload(); int len = pkt->ip_hdr->PayloadLen(); + // If the header length is zero, tcp checksum offloading is probably enabled + // In this case, let's fix up the length. + if ( pkt->ip_hdr->TotalLen() == 0 ) + len = remaining; auto* adapter = static_cast(c->GetSessionAdapter()); const struct tcphdr* tp = ExtractTCP_Header(data, len, remaining, adapter); diff --git a/testing/btest/Baseline/core.ip-broken-header/ip-bogus-header-weird.log b/testing/btest/Baseline/core.ip-broken-header/ip-bogus-header-weird.log index d5adac806f..179c4fd1a7 100644 --- a/testing/btest/Baseline/core.ip-broken-header/ip-bogus-header-weird.log +++ b/testing/btest/Baseline/core.ip-broken-header/ip-bogus-header-weird.log @@ -8,5 +8,4 @@ #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer source #types time string addr port addr port string string bool string string XXXXXXXXXX.XXXXXX - 118.181.144.194 0 136.255.115.116 0 ip_hdr_len_zero - F zeek IP -XXXXXXXXXX.XXXXXX - 118.181.144.194 0 136.255.115.116 0 bogus_IP_header_lengths - F zeek IP #close XXXX-XX-XX-XX-XX-XX From a011b4cb702b425497331aab9ad66bbbb88d8621 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 9 Nov 2021 15:11:27 +0000 Subject: [PATCH 2/7] Packets with TSO: address review feedback. This addresses review feedback of GH-1831 and additionally fixes one case in which PayloadLen was used in a way that would have given problematic results when TSO is enabled. --- src/IP.cc | 8 +++++++- src/IP.h | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/IP.cc b/src/IP.cc index 04ac1b6676..330f2b5a85 100644 --- a/src/IP.cc +++ b/src/IP.cc @@ -384,7 +384,13 @@ RecordValPtr IP_Hdr::ToPktHdrVal(RecordValPtr pkt_hdr, int sindex) const auto tcp_hdr = make_intrusive(tcp_hdr_type); int tcp_hdr_len = tp->th_off * 4; - int data_len = PayloadLen() - tcp_hdr_len; + + // account for cases in which the payload length in the TCP header is not set, + // or is set to an impossible value. In these cases, return 0. + int data_len = 0; + auto payload_len = PayloadLen(); + if ( payload_len >= tcp_hdr_len ) + data_len = payload_len - tcp_hdr_len; tcp_hdr->Assign(0, val_mgr->Port(ntohs(tp->th_sport), TRANSPORT_TCP)); tcp_hdr->Assign(1, val_mgr->Port(ntohs(tp->th_dport), TRANSPORT_TCP)); diff --git a/src/IP.h b/src/IP.h index 2b670cf72e..0488290b00 100644 --- a/src/IP.h +++ b/src/IP.h @@ -411,6 +411,9 @@ public: /** * Returns the length of the IP packet's payload (length of packet minus * header length or, for IPv6, also minus length of all extension headers). + * + * Also returns 0 if the IPv4 length field is set to zero - which is, e.g., + * the case when TCP segment offloading is enabled. */ uint16_t PayloadLen() const { From 14f919895de00ad1c5ae686f39709874f31e2ce3 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 16 Nov 2021 13:51:29 +0000 Subject: [PATCH 3/7] Add documentation for GH-1829 This adds documentation that clarifies that the `ignore_checksums` option now also allows IPv4 packets with a length of 0. --- NEWS | 7 +++++++ man/zeek.8 | 2 +- scripts/base/init-bare.zeek | 13 ++++++++++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 2ece09bee0..836a19189a 100644 --- a/NEWS +++ b/NEWS @@ -53,6 +53,13 @@ Changed Functionality - The ``SYN_packet`` record now records TCP timestamps (TSval/TSecr) when available. +- The ``ignore_checksums`` options and the ``-C`` command-line option now additionally cause + Zeek to accept IPv4 packets that provide a length of zero in the total-length IPv4 header + field. When the length is set to zero, the capture length of the packet is used instead. + This can be used to replay traces, or analyze traffic when TCP sequence offloading is enabled + on the local NIC - which typically causes the total-length of affected packets to be set to + zero. + Removed Functionality --------------------- diff --git a/man/zeek.8 b/man/zeek.8 index 1a667f5630..7b6e046ed6 100644 --- a/man/zeek.8 +++ b/man/zeek.8 @@ -66,7 +66,7 @@ print version and exit print contents of state file .TP \fB\-C\fR,\ \-\-no\-checksums -ignore checksums +When this option is set, Zeek ignores invalid packet checksums and does process the packets. Furthermore, if this option is set Zeek also processes IP packets with a zero total length field, which is typically caused by TCP (TCP Segment Offloading) on the NIC. .TP \fB\-F\fR,\ \-\-force\-dns force DNS diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek index a4d4e1ae17..3ab33fb8eb 100644 --- a/scripts/base/init-bare.zeek +++ b/scripts/base/init-bare.zeek @@ -1016,9 +1016,16 @@ const TCP_RESET = 6; ##< Endpoint has sent RST. const UDP_INACTIVE = 0; ##< Endpoint is still inactive. const UDP_ACTIVE = 1; ##< Endpoint has sent something. -## If true, don't verify checksums. Useful for running on altered trace -## files, and for saving a few cycles, but at the risk of analyzing invalid -## data. Note that the ``-C`` command-line option overrides the setting of this +## If true, don't verify checksums, and accept packets that give a length of +## zero in the IPv4 header. This is useful when running against traces of local +## traffic and the NIC checksum offloading feature is enabled. It can also +## be useful for running on altered trace files, and for saving a few cycles +## at the risk of analyzing invalid data. +## With this option, packets that have a value of zero in the total-length field +## of the IPv4 header are also accepted, and the capture-length is used instead. +## The total-length field is commonly set to zero when the NIC sequence offloading +## feature is enabled. +## Note that the ``-C`` command-line option overrides the setting of this ## variable. const ignore_checksums = F &redef; From 1dd6c1d716625ba47bbbf42a65deba364f24f9f3 Mon Sep 17 00:00:00 2001 From: Christian Kreibich Date: Tue, 16 Nov 2021 15:02:05 -0800 Subject: [PATCH 4/7] Clean up fully after successful Docker btests If we leave files sitting around, we trigger a Docker image double-build in CI, because the build runs once, gets tested, and then gets run again when we push the Docker image: the additional btest files cause Docker to detect a different source tree, causing an image layer violation. Also rename "cleanup" target to "clean", to align with rest of our tree. --- docker/btest/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docker/btest/Makefile b/docker/btest/Makefile index 65d4d65c98..30d0348a72 100644 --- a/docker/btest/Makefile +++ b/docker/btest/Makefile @@ -1,11 +1,11 @@ DIAG=diag.log BTEST=../../auxil/btest/btest -all: cleanup btest-verbose +all: btest-verbose clean # Showing all tests. btest-verbose: @$(BTEST) -d -j -f $(DIAG) -cleanup: - @rm -f $(DIAG) +clean: + @rm -rf $(DIAG) .tmp .btest.failed.dat From 67cbec91231e45b987c9a0481679d06281ea0eaf Mon Sep 17 00:00:00 2001 From: Christian Kreibich Date: Wed, 17 Nov 2021 16:42:04 -0800 Subject: [PATCH 5/7] Update cmake and aux/zeek-aux submodules [nomail] [skip ci] --- auxil/zeek-aux | 2 +- cmake | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/auxil/zeek-aux b/auxil/zeek-aux index 00ec86d2da..b47e31916b 160000 --- a/auxil/zeek-aux +++ b/auxil/zeek-aux @@ -1 +1 @@ -Subproject commit 00ec86d2dad3c2d23438431a3ccb07e11dc59543 +Subproject commit b47e31916b79f776b57a0d36da680d2d14eed597 diff --git a/cmake b/cmake index 654400c9ff..1607f0a3cb 160000 --- a/cmake +++ b/cmake @@ -1 +1 @@ -Subproject commit 654400c9ffed760140f594603737c5e8750306ff +Subproject commit 1607f0a3cbcc6b1f8fb27f1222c0cf8ee14448f1 From ee28e3e8b10d2b3bb1534646fbe1e02194fa0b1a Mon Sep 17 00:00:00 2001 From: Christian Kreibich Date: Thu, 18 Nov 2021 17:09:40 -0800 Subject: [PATCH 6/7] Update doc and auxil/zeek-aux submodules [nomail] [skip ci] --- auxil/zeek-aux | 2 +- doc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/auxil/zeek-aux b/auxil/zeek-aux index b47e31916b..9100b9d524 160000 --- a/auxil/zeek-aux +++ b/auxil/zeek-aux @@ -1 +1 @@ -Subproject commit b47e31916b79f776b57a0d36da680d2d14eed597 +Subproject commit 9100b9d524dddfade02f1b4fceb54265a113b68c diff --git a/doc b/doc index 834027ec7c..43e7a5d288 160000 --- a/doc +++ b/doc @@ -1 +1 @@ -Subproject commit 834027ec7c6463aa37c1d8233b0ef0052b2ebf0a +Subproject commit 43e7a5d288e0dd60d4113a8ef8f42ec8df4e080e From f6a9dc416e60833ce80f96051eef3112f5b0e8ff Mon Sep 17 00:00:00 2001 From: Christian Kreibich Date: Thu, 18 Nov 2021 17:25:36 -0800 Subject: [PATCH 7/7] Updates to NEWS to cover recent additions. [nomail] [skip ci] --- NEWS | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/NEWS b/NEWS index ca92bc610e..053314904a 100644 --- a/NEWS +++ b/NEWS @@ -49,6 +49,12 @@ New Functionality - A new command-line option ``-c`` or ``--capture-unprocessed`` will dump any packets not marked as being processed, similar to the new hook and event above. +- In Zeek plugins, the new cmake function ``zeek_plugin_scripts()`` should be + used alongside ``zeek_plugin_cc()`` and related functions to establish + dependency tracking between Zeek scripts shipped with the plugin and plugin + rebuilds. Previously, updates to included Zeek scripts didn't reliably + trigger a rebuild. + Changed Functionality --------------------- @@ -64,6 +70,10 @@ Changed Functionality - The ``SYN_packet`` record now records TCP timestamps (TSval/TSecr) when available. +- The ``init-plugin`` script now focuses purely on dynamic Zeek plugins. It no + longer generates Zeek packages. To instantiate new Zeek packages, use the + ``zkg create`` command instead. + Removed Functionality ---------------------