From 3a320fc6b66083a9a3b4b6dccbbc595b50f6322d Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 26 Sep 2022 17:02:59 +0200 Subject: [PATCH] packet_analysis: Do not raise analyzer_confirmation per-packet for tunnels There's a logic error in the packet analyzer's AnalyzerConfirmation() method that causes analyzer_confirmation() events to be raised for every packet rather than stopping after the first confirmation which appears to have been the intention. This affects, for example, VXLAN and Geneve tunnels. The optional arg_tag parameter was used for short-circuit'ing, but the return value of GetAnalyzerTag() used for setting the session state causing the disconnect. In scenarios where Zeek receives purely tunneled monitoring traffic, this may result in a non-negligible performance impact. Somewhat related, ensure the session state is set to violated before short-circuiting if no analyzer_violations are installed. Suggesting this as a 5.0.3 candidate. --- src/packet_analysis/Analyzer.cc | 20 +++++++++++------- .../conn.log | 12 +++++++++++ .../http.log | 11 ++++++++++ .../core.tunnels.analyzer-confirmation/out | 3 +++ .../tunnels/vxlan-encapsulated-http.pcap | Bin 0 -> 10923 bytes .../core/tunnels/analyzer-confirmation.zeek | 19 +++++++++++++++++ 6 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 testing/btest/Baseline/core.tunnels.analyzer-confirmation/conn.log create mode 100644 testing/btest/Baseline/core.tunnels.analyzer-confirmation/http.log create mode 100644 testing/btest/Baseline/core.tunnels.analyzer-confirmation/out create mode 100644 testing/btest/Traces/tunnels/vxlan-encapsulated-http.pcap create mode 100644 testing/btest/core/tunnels/analyzer-confirmation.zeek diff --git a/src/packet_analysis/Analyzer.cc b/src/packet_analysis/Analyzer.cc index ba79e13c40..877593520c 100644 --- a/src/packet_analysis/Analyzer.cc +++ b/src/packet_analysis/Analyzer.cc @@ -168,26 +168,30 @@ void Analyzer::Weird(const char* name, Packet* packet, const char* addl) const void Analyzer::AnalyzerConfirmation(session::Session* session, zeek::Tag arg_tag) { - if ( session->AnalyzerState(arg_tag) == session::AnalyzerConfirmationState::CONFIRMED ) + const auto& effective_tag = arg_tag ? arg_tag : GetAnalyzerTag(); + + if ( session->AnalyzerState(effective_tag) == session::AnalyzerConfirmationState::CONFIRMED ) return; - session->SetAnalyzerState(GetAnalyzerTag(), session::AnalyzerConfirmationState::CONFIRMED); + session->SetAnalyzerState(effective_tag, session::AnalyzerConfirmationState::CONFIRMED); if ( ! analyzer_confirmation ) return; - const auto& tval = arg_tag ? arg_tag.AsVal() : tag.AsVal(); - event_mgr.Enqueue(analyzer_confirmation, session->GetVal(), tval, val_mgr->Count(0)); + event_mgr.Enqueue(analyzer_confirmation, session->GetVal(), effective_tag.AsVal(), + val_mgr->Count(0)); } void Analyzer::AnalyzerViolation(const char* reason, session::Session* session, const char* data, int len, zeek::Tag arg_tag) { + const auto& effective_tag = arg_tag ? arg_tag : GetAnalyzerTag(); + + session->SetAnalyzerState(effective_tag, session::AnalyzerConfirmationState::VIOLATED); + if ( ! analyzer_violation ) return; - session->SetAnalyzerState(GetAnalyzerTag(), session::AnalyzerConfirmationState::VIOLATED); - StringValPtr r; if ( data && len ) @@ -200,8 +204,8 @@ void Analyzer::AnalyzerViolation(const char* reason, session::Session* session, else r = make_intrusive(reason); - const auto& tval = arg_tag ? arg_tag.AsVal() : tag.AsVal(); - event_mgr.Enqueue(analyzer_violation, session->GetVal(), tval, val_mgr->Count(0), std::move(r)); + event_mgr.Enqueue(analyzer_violation, session->GetVal(), effective_tag.AsVal(), + val_mgr->Count(0), std::move(r)); } } // namespace zeek::packet_analysis diff --git a/testing/btest/Baseline/core.tunnels.analyzer-confirmation/conn.log b/testing/btest/Baseline/core.tunnels.analyzer-confirmation/conn.log new file mode 100644 index 0000000000..875f72abca --- /dev/null +++ b/testing/btest/Baseline/core.tunnels.analyzer-confirmation/conn.log @@ -0,0 +1,12 @@ +### 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 10.1.200.131 50000 10.1.1.172 4789 udp vxlan 0.627090 10203 0 S0 - - 0 D 12 10539 0 0 - +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 172.16.11.201 40354 54.86.237.188 80 tcp http 0.627052 87 9212 SF - - 0 ShADadFf 7 459 5 9480 CHhAvVGS1DHFjwGM9 +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/core.tunnels.analyzer-confirmation/http.log b/testing/btest/Baseline/core.tunnels.analyzer-confirmation/http.log new file mode 100644 index 0000000000..3c0c803ba7 --- /dev/null +++ b/testing/btest/Baseline/core.tunnels.analyzer-confirmation/http.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 http +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p trans_depth method host uri referrer version user_agent origin request_body_len response_body_len status_code status_msg info_code info_msg tags username password proxied orig_fuids orig_filenames orig_mime_types resp_fuids resp_filenames resp_mime_types +#types time string addr port addr port count string string string string string string string count count count string count string set[enum] string string set[string] vector[string] vector[string] vector[string] vector[string] vector[string] vector[string] +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 172.16.11.201 40354 54.86.237.188 80 1 GET eu.httpbin.org /image/svg - 1.1 curl/7.76.1 - 0 8984 200 OK - - (empty) - - - - - - FTKnz016WapPYpNaxl - text/plain +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/core.tunnels.analyzer-confirmation/out b/testing/btest/Baseline/core.tunnels.analyzer-confirmation/out new file mode 100644 index 0000000000..6f82ec81b8 --- /dev/null +++ b/testing/btest/Baseline/core.tunnels.analyzer-confirmation/out @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +analyzer_confirmation, CHhAvVGS1DHFjwGM9, [orig_h=10.1.200.131, orig_p=50000/udp, resp_h=10.1.1.172, resp_p=4789/udp], 0 +analyzer_confirmation, ClEkJM2Vm5giqnMf4h, [orig_h=172.16.11.201, orig_p=40354/tcp, resp_h=54.86.237.188, resp_p=80/tcp], 6 diff --git a/testing/btest/Traces/tunnels/vxlan-encapsulated-http.pcap b/testing/btest/Traces/tunnels/vxlan-encapsulated-http.pcap new file mode 100644 index 0000000000000000000000000000000000000000..3ad59f4863d8357b65675ed3f3938ffc1b75175f GIT binary patch literal 10923 zcmcgyU5s2+9iM57Hft6EeV~})%`}DtcJAEok9AvUmv)OTP;6baF(KRS?abKe%w}e~ z+Y&!We9;ImBoq}e2*F6KArcix^a)JRYBa{k$BTg%;K4*f;6X$1_xqoFXYS0o(`_kn zn(5th?>Ya^-~Z#BxqpBD>7Q&WZ7B^uoF^4;H^Dt!Zg^Ly2vKmY#Ciu!+R zX>Y02=8BuHeE7Dh@{8Y@Dwoebf8eTLmkyu#>{dPijsDr^Zv5Lf_rLw^$A9yIteD&O zgPTe>eQa-Z_KHvZAz1p?Zy$NAbl~t)ul3+&W8FsufT{;I&{Ax1%PRa1l2O}~(`M_T%%JBVdKcWmbzfmZ|)#dlz zOh#o`*n7yRHBT(plUi@BZtOpF=s?Xj?dhrgonC*|NKTrKet-2yvu$>|_35cE_mXaP zcRgw2*~-aos}`DJ!0EeJR+3fRyuNn*^wjj!>rk!qEWiT|KAd?qS9=fcrK>-E|Cp<< zJ^a+`rF*U@HP3xbsW$z>n+r5?Cc{?{2mFqkxBP|FyxqKc%fSQlnR&-$C|~{HKFaW^ zUlhu4b*cIEI|CVRp$wTl4ac&K+rKzHb@OsRnKceB_h$?zGIpP=apcL}cCylMcG|PX@g!NTF1MO% z$@J90q`Q`MXN~&Fc5|iEZP)DBj7>{9tk($%4&3Y3Q;6pl8dp_Sy zjzX~Ja;rCM^t&g)K>7N*OY*grV!Uk+Nhs|ch4Jg9GuvO?giw0-#qAl4U%m_ez9sYZ zy*o-Je#d>i<6Fwtr+4Ljz2ob>Pfqgnj>QKT=Txeq(3Fwye)pj7HL| zH~QIVt(mOv>73eGu?)+=1b!>Krlt&IuHWppl3fRv7L3JCy)#!+V|lb}2<6#EH#t_s ze4)F%+GwuyYFJ&X82#mLJps$RR+qc*dj-_-Q6z-w&dQzcyg^oXcDl!#?Ygnh?VMaS z8l4k~v5KssM1oLVpi?aeh33(nl_Se5$LpBfKAMun{bS8mYiDK0zI|~FZ3feaW*FYf zSz~y!3wwCE;b{(QVwEBh`kmEkD>;_wl^0--?d};8N(Jz9GG+Yp_b!tCkxswgIZ;K- zHF6?Ag*;8DkRPTNx_NKp7g#7i^Wp`pi(gin-IZ3-tBg5g*T5TdYQ|s;v&q8t&|bW? zGNy>!gEn?#=3K8-o$=NP27`KUO(+)U3#yw;D#^npPCd7|`5}{E@|2zhW4ciFz|* zR72BoLc^%~X5_~+Ra@2>yrN!YV|_i>x&BN_N9J2G_!&ZHI9Buu*84)Ad~Ms3`ECq) zgE5jo9A@;3*W0o1vmHz1w>bts&x)X8svq?C9j{@X_7UBsXEbCmv;C;VKo7w>btf&DW3P=558 zB7cy|AGtzL{3-goy8K+{VZNf1>PtM)|Bzqw&+t>Up87{4@_V+zPbR-_(;wo;0{+y~ zR(PTRVSkQ^TaWfk^{dkVh4zE|!cXWceIrFL3gW?7;-ScYnf>zd6ZmPo_zM4^r{Zan z|5EvF@XO0@1*7uo_?J1T$Thx?R03By3_VTNHN*ByFVaF_(FlCgA>^1KJ*&XQS&FjF$ad(_AcjRc3XgYYllE%N|BTm!fx9fAnKojG5qg!<~L zZMq?o7q0*VashK2Bo3hmCAOJ5Wjwwbc5xK6JbgJjRup#x7_X2?`3_R1tLu!q=7vqua29i%pT>J;~-m2jVG+m&?z{BuPh`K$t0l*dx_*K z6^EJABmv2r}n4{I<<&`uhRTQe8rMU99w0QD9a>7A2rbQR&j@D zCUwWtsPC6LVQaX9#>`vPm^hOhf^>O-Sre;?kX#NSpaO{r=0d?2s|9ZBwW!D7x0$i! zIS`eu=Io+uhkTW9eweRdZ04)XK8#-2C)DiIm{<>`4nju3m9|do6AoKqDjc{Dvt$sf z2Hvt$uY`6%1mzGxJgFfO5e)N{Ai^oqnA1_?Wtpm`KCH{T7zr8&w%2(Jo@f!^i3lQa zKupSF8h>o{3M;w76So?~lds`vN%b8CG80d1$pu%Tf-CIBe1#Yg^wM77>J-FZ8Y6-$ zZYT&4i6AShhurQEFJV!x9L-mPCm0As0`@M17wuKJdfxDfJ7gBeGF?9~`~Zr0GYga3(WIEr&UP24G2Hea1S&-=%0kH)KOZJBAAUId_beC4Mg!>}N+Z*D(1k__JC;4P+mQzNC2)$4;xc;o_aqAiH^R7z&_6k2?j z`O~(TyXi1iALb%i((wq|xgH;sd9Fng zOHn9Gn3b?E16~vfj1|cu5)Gpon31dHZYFB~+SMq;)keA}@9d z0+fbSLYWE@2oyVD4krnYg=8^_)J`ltE>kaD=c%^JcUi1$oENY(!Q_q>Q&gmtwM&Z9 zL@p6a8ahkrN&$H~LFyL0L+UJXGA<&qCS2@Ero^ruwO&a8a`;8nOY5n{-5GF#h!i%q z7QuLafl5GXp2_fnsxCsO9lcAXs07F4re$?<8Fu+VmQSA(O0WYVgf>jj7^IMj3at&F z6O{v9)e=@(l0x5-2X-+DBwnD>*+4>~lBR>QY?K8;^i&>FTJlYm#9C^RW$z5gAcbn3 zl1hO+i9{M3K&1mI(8T$4C}PBy%>i)2t*c4|MR;jl2r5A~;>HjcX@c2zvE?D=vou4~ z%J@TanP>mydiD%Xq+D)&(X@!{A(9y*EKP^pCQZnPi`kqDo-%BD=n;PARy(cJ^-f#U zg7siY&(!~)da&AQw)^ly~LJj6%5;Vc(H z>INHe^xz{PVgtE6yb<77;$VG0H;@bcKR{Zn#2&_YCMkFGzTJYsthy8 zc}#)W7>h_*ZP(J=2BP6S0}Wfzz7%P~vbBBGydmHPlz=Gas({HAO)s&Ga2KoGVKLui#t^%q{z{wmF%x!%IZ-cI^}v@hU!P+?!7+6r%&z&Q>0 z&UY0Y7Izgu1draec7Fr24-r9;S!h^zpm{1XualW#HI^}VD?}*SEjN6Y4vxgz~zuC}~uYL4%_?2>oE=AXTk zcfktY4|-VM51P~84|@L^ydU)96OR6VQ1aYIEW;xr!{-ZSxU&526L=d_z3w-BN9m_8 zZTV>5iMvhSiTmE@J4)|ggLjn5_gqf4U?Y3ViWnvBT_` a9m;36zyBG&tMzy{v%@PlOuz8f<^Ka^ouN+v literal 0 HcmV?d00001 diff --git a/testing/btest/core/tunnels/analyzer-confirmation.zeek b/testing/btest/core/tunnels/analyzer-confirmation.zeek new file mode 100644 index 0000000000..22ec0d3d2c --- /dev/null +++ b/testing/btest/core/tunnels/analyzer-confirmation.zeek @@ -0,0 +1,19 @@ +# @TEST-DOC: Check how many analyzer_confirmation events a vxlan-encapsulated HTTP transaction triggers. Should be 2. +# @TEST-EXEC: zeek -b -r $TRACES/tunnels/vxlan-encapsulated-http.pcap %INPUT >out +# @TEST-EXEC: btest-diff out +# @TEST-EXEC: btest-diff conn.log +# @TEST-EXEC: btest-diff http.log + +@load base/frameworks/tunnels +@load base/protocols/conn +@load base/protocols/http + +event analyzer_confirmation(c: connection, atype: AllAnalyzers::Tag, aid: count) + { + print "analyzer_confirmation", c$uid, c$id, aid; + } + +event analyzer_violation(c: connection, atype: AllAnalyzers::Tag, aid: count, reason: string) + { + print "analyzer_violation", c$uid, c$id, aid, reason; + }