From 6fbebc5e940c2f3e34e688ea88cd6acaa5da1ba2 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Tue, 8 Nov 2022 13:25:59 +0100 Subject: [PATCH] Fixing productive connections with missing SYN still considered partial after flipping direction. 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. This fix is a bit of a hack, but the best solution I could think of without introducing larger changes. Closes #2426. --- src/analyzer/protocol/tcp/TCP_Endpoint.cc | 4 +++- .../protocol/tcp/TCPSessionAdapter.cc | 7 +++++++ .../protocol/tcp/TCPSessionAdapter.h | 2 ++ .../Baseline/core.tcp.flip-without-syn/conn.log | 11 +++++++++++ .../Traces/tcp/http-on-irc-port-missing-syn.pcap | Bin 0 -> 6576 bytes testing/btest/core/tcp/flip-without-syn.zeek | 4 ++++ 6 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/core.tcp.flip-without-syn/conn.log create mode 100644 testing/btest/Traces/tcp/http-on-irc-port-missing-syn.pcap create mode 100644 testing/btest/core/tcp/flip-without-syn.zeek 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 a85545a407..eaf64b9b8e 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 diff --git a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.h b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.h index d20b28765b..e14c3eb659 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 0000000000000000000000000000000000000000..c622ad91924b35e7bc7f65dc1327e133b6973056 GIT binary patch literal 6576 zcma)BU5p!76`tKDp)y^OLWQb6z-3!0yPF+*#_Roao3_sWC22OhT04!95b52SYx{P` zGsFC>*HItxfCNZ|@{m^IPc4N=2%&}t9zYZUYKtlesailN0)&b{MF^=kk~iX9WMgm%a{Z%UY#e=fMt}xF7=&(#J_Q zlPzRTgH5c-i0!6dd~WUVny?uU)4%J+aa(MgW^7Kvq(=71_Fd_}@4c6NK7#KK-t_v^ z?+IL9~>IJJ(K&w$xnXt%%9lEZMS_O@j0 z-ue2-m6P~P@5D>uQTZxdfA(SR;UkxCkRS4Q&zQDJe&Fw+%SYMqWt?DSZ05|YIhHveia;;bgc}vvg_?-y*8-ccdC-)una}34#^hM0 zWLd(;6H~dVvCJti#7CIRdO>Y0v*fw1up;TXMfQjgzFueu`(JTKrtZDS__i^}- zPkxhpT)*!iA3xdX_0dL)r&)fHmEsy1%d`AUv9MUo zr^Z$)yjEo6W`54p3)aHCF*Ccs=c<-5-VTnw#od|+5tvpaLKT(>QPs~1w-(iltT1oP zj%D@(RAiBC2rrIK%ucfg-#cL%#=*fvkd?Jc%g4zhu@~ihhfDWKP-OpJPv`Q|QKmo-Z(SwrCnfqrh}y-Y_y5#-`Y++>>sN#l8(Nby%U@PqSuST6Jc5 zv12n|T01PN3)T!gJZfPNf#el++v zHaXk1NyB93OmTL$SeU~mXS+5@ZmY5u2O_n_B9DnZs9IA-b>@1men|u#3nL!IA@f`& zg1`&d#2I`n%cj`D=HRW+@iq?!%*+>O4bY_h?Z=7|HSm1EgTUE<^jrT>^LnQNRj$(2iyfE!=b3m==si0O&uTs zmwcfJfDp`XTsTR9ZpBf0;D!i8h&zlfb5{xn`N3N%G&ucCmrWB`=ZofC5g)p2R+@tE zgtYGf`{qC*i5*DcJ3y8*S2TgIU{%1|jxUiR%z{yDfDv%peH0WU_$+{mi$mO!%?)3; zC(Fw$jD6n=h&25WcTT*AY@l#pTQ1O*dxU2H4ro(qXF5Aw1yk^xKrCDis2B&KY7k17 zC_^N-z+^J!NLL^a1Qs-z&qF{`54^ZmSEd-_4y6DL!DGory$xaQ-nrTNEF}SBI@=Nr zZ&y*lYY4V>?z{{etQvR?#+y5xi~wEs7teGS<7ZIB^GVPZ(j*fMI^iSINVxNrCGxw! zc5b16ZSrh#*iiU9;1SSGP3o(?%L|L0<*6jJAGC^q#%ZyTPEi-!7J&jM1ULYXputYN zz|l0S0aOOMYr&7M-Le)uuWG@)y|v&sZ?)^qPhaY-1ppYwsbebSGp&YU+zghM2l(LM532DJm3Ein}r-#Ro+3!TNwTc|aPcc+iQ* z%)-Jj^Jyj|=JU*4C{hmg$Pdh`fH!0aiV}Y!(eRbrQs8@)xcD8#5DV%9e z9}8Jb`o&TmQ;E%kTHHVtO(hNyw~?MhJW|L{#x*+4`-h4a^`S~RkILmCGnF97C}b!# zwzTn2D4goxVcTqj9;0`FcM_96vjW*9MOizfy<8PQp+$1nO3m$4**cb(}m@T6ns= zk($Y>$5c5;bZT5*W)D;w(SzfP6rnFH>Z_<|Sl9xTFN}gJZDe9n52F?lFWC@&R{{=L(-dc!yLQNGC=IwB<_?1R2TN8G7-CB08K&M)8tHKGGux^r=Mc_gG{$_`qH%G zt?g0zD&a(HFCatALQQpJAG<+zbSDQY{9T0>hPd^&v`UJL$q>ss(FRSU*@Yr)N@+O=TwLT@bybLgV1osMi# z>k=bN1kL~*;X8y?s%Z!GD3me#n+|h*d5eLp)A%Gz*Ew}a=pfK3M4o~M3A!sibV$@e zKrCbxwI&QDsG4QVX)dKX(2Hhr}(lY0EHD^rWq+k{?h9BOquJd11?7$XeAD z(@LccYI%}Omgi%l0{P>>5`$bRH>iP%sM#cUBxjufB5uS#3(IRV(v{I6u2DE>DPf5Q zJJs5--L5jn^U%BkRvBPW5a^JVYWN~RWmQe%@P76GaDnt0rqLqPHaR<%GV2&a4a9y_ z5Tz|JtyEh|R-iB>S828;%5ZF3MEsaCLCQnl+7Aq+XsTYz06LJ{vFZxjQqte=Vh2-< zaxp14J;d(88Q@ugz}(l4QiKtDn<~!1$aI?8ZQJ7L%`edOMMt1A;CQuEIHvI%xQ-U) z819`!&_Y!gn0^#G+-)_Gn!^d+Ry;K@!V0QYSofO9Kkx;Uff}wr3s%MC?xP267wEP* z<)TON%Bd~T3XwC$h+chGJumC$S>yJ)~tbSan5 zsd9|YEqE>8TMPa? zuWG^F+NB8BcXY4UzJ~%~5J8&WFkORn%{nGK+>6mrjIR?STT8gTxq5Ny^tm;(n|Rc> zxIZ}v!Re&`V(7WhbD$`ys|N67YqL~2HTe7mFWBLj5or{KHmn9h&>Wjw9MIf>O9dzo zxQ5GCG>KJ**TO+FTcR1D0DyifvHEJ!K|Q3A#Zb|1D2zGXEYL+5ri%iN_nm$pai=aJ z$q3qIyKG=rX;A?t9oFFiN^T+@ce&HT9ZRYW-E4#Jj^~0-mpF69{6Z0Pf0t+6T|o#U zI(|}6{J^8Q5a|^}LrB^bDS92EEr;-VpDo$GnlQwi~rL?@h!s@uw#ubGJ{1E#f zsj2W#MM0#aG>DhOQ+chgMH{NCr(GFI^l`GG0*V-+qs$?-l6{pQJ;#E1qks4V*NO8q z6L;TfKlJ_mJJ^By=EA>Tq5I}XzteZ${LcpEjpc5}&zEiuj@$-&Oyal{o7>?B)zu5ON7?09d=o}yXUEeugzIyG= U*YJM~TJGPSbG(Cdyp{O)ACDmPcmMzZ literal 0 HcmV?d00001 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.