diff --git a/CHANGES b/CHANGES index e0a0808b7e..d78044b1ea 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,24 @@ +5.2.0-dev.339 | 2022-11-18 11:49:47 +0100 + + * GH-2426: Fixing productive connections with missing SYN still considered partial after flipping direction. (Robin Sommer, Corelight) + + In https://github.com/zeek/zeek/pull/2191, we added endpoint flipping + for cases where a connection starts with a SYN/ACK followed by ACK or + data. The goal was to treat the connection as productive and go ahead + and parse it. But the TCP analyzer could continue to consider it + partial after flipping, meaning that app layers would bail out. #2426 + shows such a case: HTTP gets correctly activated after flipping + through content inspection, but it won't process anything because + `IsPartial()` returns true. As the is-partial state reflects + whether we saw the first packets each in direction, this patch now + overrides that state for the originally missing SYN after flipping. + + We actually had the same problem at a couple of other locations already + as well. One of that only happened to work because of the originally + inconsistent state flipping that was fixed in the previous commit. The + corresponding unit test now broke after that change. This commit + updates that logic as well to override the state. + 5.2.0-dev.336 | 2022-11-17 16:28:40 -0800 * Publish container images to ECR in addition to docker.io. (Benjamin Bannier, Corelight) diff --git a/NEWS b/NEWS index 44d3e8bccf..705a19c1f0 100644 --- a/NEWS +++ b/NEWS @@ -138,6 +138,10 @@ Changed Functionality DPD ``max_violations`` script-level logic has a chance to run and disable the problematic analyzer. +- The TCP analyzer now continues processing payload for some + connections missing initial packets where it would previously have + stopped. This fixes a few cases where we already had the logic to + continue in place, but we still ended up considering them partial. Deprecated Functionality ------------------------ diff --git a/VERSION b/VERSION index bc0e5f6620..9ce8554221 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -5.2.0-dev.336 +5.2.0-dev.339 diff --git a/src/analyzer/Analyzer.h b/src/analyzer/Analyzer.h index 43c5552ffb..8fa18c258b 100644 --- a/src/analyzer/Analyzer.h +++ b/src/analyzer/Analyzer.h @@ -862,6 +862,12 @@ public: */ void ForwardUndelivered(uint64_t seq, int len, bool orig) override; + /** + * Signals that Zeek has flipped the direction of the connection, meaning + * that originator and responder state need to be swapped. + */ + void FlipRoles() override { orig = ! orig; } + protected: friend class Analyzer; diff --git a/src/analyzer/protocol/tcp/TCP_Endpoint.cc b/src/analyzer/protocol/tcp/TCP_Endpoint.cc index 904518138b..1389e58bed 100644 --- a/src/analyzer/protocol/tcp/TCP_Endpoint.cc +++ b/src/analyzer/protocol/tcp/TCP_Endpoint.cc @@ -275,9 +275,11 @@ bool TCP_Endpoint::CheckHistory(uint32_t mask, char code) // since if those elicit anything, it should be a RST. // // Thus, at this stage we go ahead and flip the connection. - // We then fix up the history (which will initially be "H^"). + // We then fix up the history (which will initially be "H^") and + // pretend we have actually seen that missing first packet. conn->FlipRoles(); conn->ReplaceHistory("^h"); + tcp_analyzer->SetFirstPacketSeen(true); } if ( ! IsOrig() ) diff --git a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc index dfc33a209a..3aa3438c7f 100644 --- a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc +++ b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc @@ -787,6 +787,11 @@ void TCPSessionAdapter::SetPartialStatus(analyzer::tcp::TCP_Flags flags, bool is } } +void TCPSessionAdapter::SetFirstPacketSeen(bool is_orig) + { + first_packet_seen |= (is_orig ? ORIG : RESP); + } + void TCPSessionAdapter::UpdateInactiveState(double t, analyzer::tcp::TCP_Endpoint* endpoint, analyzer::tcp::TCP_Endpoint* peer, uint32_t base_seq, uint32_t ack_seq, int len, bool is_orig, @@ -829,6 +834,7 @@ void TCPSessionAdapter::UpdateInactiveState(double t, analyzer::tcp::TCP_Endpoin is_partial = 0; Conn()->FlipRoles(); peer->SetState(analyzer::tcp::TCP_ENDPOINT_ESTABLISHED); + SetFirstPacketSeen(true); } else @@ -913,6 +919,7 @@ void TCPSessionAdapter::UpdateInactiveState(double t, analyzer::tcp::TCP_Endpoin // as partial and instead establish the connection. endpoint->SetState(analyzer::tcp::TCP_ENDPOINT_ESTABLISHED); is_partial = 0; + SetFirstPacketSeen(is_orig); } else @@ -1162,6 +1169,9 @@ void TCPSessionAdapter::FlipRoles() orig = tmp_ep; orig->is_orig = ! orig->is_orig; resp->is_orig = ! resp->is_orig; + first_packet_seen = ((first_packet_seen & ORIG) ? RESP : 0) | + ((first_packet_seen & RESP) ? ORIG : 0); + is_partial = 0; // resetting, it may be re-established later } void TCPSessionAdapter::UpdateConnVal(RecordVal* conn_val) diff --git a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.h b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.h index 96c158488d..e79242869a 100644 --- a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.h +++ b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.h @@ -80,6 +80,7 @@ public: protected: friend class analyzer::tcp::TCP_ApplicationAnalyzer; + friend class analyzer::tcp::TCP_Endpoint; friend class analyzer::tcp::TCP_Reassembler; friend class analyzer::pia::PIA_TCP; friend class packet_analysis::TCP::TCPAnalyzer; @@ -95,6 +96,7 @@ protected: bool IsReuse(double t, const u_char* pkt) override; void SetPartialStatus(analyzer::tcp::TCP_Flags flags, bool is_orig); + void SetFirstPacketSeen(bool is_orig); // Update the state machine of the TCPs based on the activity. This // includes our pseudo-states such as TCP_ENDPOINT_PARTIAL. diff --git a/testing/btest/Baseline/core.tcp.flip-without-syn/conn.log b/testing/btest/Baseline/core.tcp.flip-without-syn/conn.log new file mode 100644 index 0000000000..d058edc5de --- /dev/null +++ b/testing/btest/Baseline/core.tcp.flip-without-syn/conn.log @@ -0,0 +1,11 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path conn +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p proto service duration orig_bytes resp_bytes conn_state local_orig local_resp missed_bytes history orig_pkts orig_ip_bytes resp_pkts resp_ip_bytes tunnel_parents +#types time string addr port addr port enum string interval count count string bool bool count string count count count count set[string] +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 141.142.228.5 6669 192.150.187.43 80 tcp http 0.141744 136 5007 SF - - 0 ^hADadFf 6 456 7 5371 - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/tcp/http-on-irc-port-missing-syn.pcap b/testing/btest/Traces/tcp/http-on-irc-port-missing-syn.pcap new file mode 100644 index 0000000000..c622ad9192 Binary files /dev/null and b/testing/btest/Traces/tcp/http-on-irc-port-missing-syn.pcap differ diff --git a/testing/btest/core/tcp/flip-without-syn.zeek b/testing/btest/core/tcp/flip-without-syn.zeek new file mode 100644 index 0000000000..992fbbf4d7 --- /dev/null +++ b/testing/btest/core/tcp/flip-without-syn.zeek @@ -0,0 +1,4 @@ +# @TEST-EXEC: zeek -r $TRACES/tcp/http-on-irc-port-missing-syn.pcap %INPUT +# @TEST-EXEC: btest-diff conn.log + +# @TEST-DOC: Regression test for #2191: missing SYN and misleading well-known port. Zeek flips, and DPD still figures out the protocol ok.