From f1cef9d2a91aab6aa019f4da0375eeeac59ee8e2 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 11 Sep 2014 10:47:56 -0500 Subject: [PATCH] Fix issue w/ TCP reassembler not delivering some segments. For example, if we have a connection between TCP "A" and TCP "B" and "A" sends segments "1" and "2", but we don't see the first and then the next acknowledgement from "B" is for everything up to, and including, "2", the gap would be reported to include both segments instead of just the first and then delivering the second. Put generally: any segments that weren't yet delivered because they're waiting for an earlier gap to be filled would be dropped when an ACK comes in that includes the gap as well as those pending segments. (If a distinct ACK was seen for just the gap, that situation would have worked). Addresses BIT-1246. --- src/analyzer/protocol/tcp/TCP_Reassembler.cc | 98 +++++++++++------- src/analyzer/protocol/tcp/TCP_Reassembler.h | 1 + .../entity_data | 4 + .../extract_files.file0 | Bin 0 -> 4705 bytes .../modbus.log | 8 +- testing/btest/Traces/http/entity_gap2.trace | Bin 0 -> 4805 bytes .../base/protocols/http/entity-gap2.bro | 24 +++++ 7 files changed, 96 insertions(+), 39 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/entity_data create mode 100644 testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/extract_files.file0 create mode 100644 testing/btest/Traces/http/entity_gap2.trace create mode 100644 testing/btest/scripts/base/protocols/http/entity-gap2.bro diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index 053e8c8f60..8b4ab0a93e 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -117,6 +117,45 @@ void TCP_Reassembler::SetContentsFile(BroFile* f) record_contents_file = f; } +static inline bool established(const TCP_Endpoint* a, const TCP_Endpoint* b) + { + return a->state == TCP_ENDPOINT_ESTABLISHED && + b->state == TCP_ENDPOINT_ESTABLISHED; + } + +static inline bool report_gap(const TCP_Endpoint* a, const TCP_Endpoint* b) + { + return content_gap && + ( BifConst::report_gaps_for_partial || established(a, b) ); + } + +void TCP_Reassembler::Gap(uint64 seq, uint64 len) + { + // Only report on content gaps for connections that + // are in a cleanly established state. In other + // states, these can arise falsely due to things + // like sequence number mismatches in RSTs, or + // unseen previous packets in partial connections. + // The one opportunity we lose here is on clean FIN + // handshakes, but Oh Well. + + if ( report_gap(endp, endp->peer) ) + { + val_list* vl = new val_list; + vl->append(dst_analyzer->BuildConnVal()); + vl->append(new Val(IsOrig(), TYPE_BOOL)); + vl->append(new Val(seq, TYPE_COUNT)); + vl->append(new Val(len, TYPE_COUNT)); + dst_analyzer->ConnectionEvent(content_gap, vl); + } + + if ( type == Direct ) + dst_analyzer->NextUndelivered(seq, len, IsOrig()); + else + dst_analyzer->ForwardUndelivered(seq, len, IsOrig()); + + had_gap = true; + } void TCP_Reassembler::Undelivered(uint64 up_to_seq) { @@ -189,48 +228,33 @@ void TCP_Reassembler::Undelivered(uint64 up_to_seq) if ( ! skip_deliveries ) { - // This can happen because we're processing a trace - // that's been filtered. For example, if it's just - // SYN/FIN data, then there can be data in the FIN - // packet, but it's undelievered because it's out of - // sequence. - - uint64 seq = last_reassem_seq; - uint64 len = up_to_seq - last_reassem_seq; - - // Only report on content gaps for connections that - // are in a cleanly established state. In other - // states, these can arise falsely due to things - // like sequence number mismatches in RSTs, or - // unseen previous packets in partial connections. - // The one opportunity we lose here is on clean FIN - // handshakes, but Oh Well. - - if ( content_gap && - (BifConst::report_gaps_for_partial || - (endpoint->state == TCP_ENDPOINT_ESTABLISHED && - peer->state == TCP_ENDPOINT_ESTABLISHED ) ) ) + DataBlock* b = blocks; + // If we have blocks that begin below up_to_seq, deliver them. + while ( b ) { - val_list* vl = new val_list; - vl->append(dst_analyzer->BuildConnVal()); - vl->append(new Val(IsOrig(), TYPE_BOOL)); - vl->append(new Val(seq, TYPE_COUNT)); - vl->append(new Val(len, TYPE_COUNT)); + if ( b->seq < last_reassem_seq ) + { + // Already delivered this block. + b = b->next; + continue; + } - dst_analyzer->ConnectionEvent(content_gap, vl); + if ( b->seq >= up_to_seq ) + // Block is beyond what we need to process at this point. + break; + + uint64 gap_at_seq = last_reassem_seq; + uint64 gap_len = b->seq - last_reassem_seq; + + Gap(gap_at_seq, gap_len); + last_reassem_seq += gap_len; + BlockInserted(b); + b = b->next; } - if ( type == Direct ) - dst_analyzer->NextUndelivered(last_reassem_seq, - len, IsOrig()); - else - { - dst_analyzer->ForwardUndelivered(last_reassem_seq, - len, IsOrig()); - } + if ( up_to_seq > last_reassem_seq ) + Gap(last_reassem_seq, up_to_seq - last_reassem_seq); } - - had_gap = true; } // We should record and match undelivered even if we are skipping diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.h b/src/analyzer/protocol/tcp/TCP_Reassembler.h index 3dfe75bf10..5d8badcef1 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.h +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.h @@ -94,6 +94,7 @@ private: DECLARE_SERIAL(TCP_Reassembler); void Undelivered(uint64 up_to_seq); + void Gap(uint64 seq, uint64 len); void RecordToSeq(uint64 start_seq, uint64 stop_seq, BroFile* f); void RecordBlock(DataBlock* b, BroFile* f); diff --git a/testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/entity_data b/testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/entity_data new file mode 100644 index 0000000000..c6e5999e07 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/entity_data @@ -0,0 +1,4 @@ +^J0.26 | 2012-08-24 15:10:04 -0700^J^J * Fixing update-changes, which could pick the wrong control file. (Robin Sommer)^J^J * Fixing GPG signing script. (Robin Sommer)^J^J0.25 | 2012-08-01 13:55:46 -0500^J^J * Fix configure script to exit with non-zero status on error (Jon Siwek)^J^J0.24 | 2012-07-05 12:50:43 -0700^J^J * Raise minimum required CMake version to 2.6.3 (Jon Siwek)^J^J * Adding script to delete old fully-merged branches. (Robin Sommer)^J^J0.23-2 | 2012-01-25 13:24:01 -0800^J^J * Fix a bro-cut error message. (Daniel Thayer)^J^J0.23 | 2012-01-11 12:16:11 -0800^J^J * Tweaks to release scripts, plus a new one for signing files.^J (Robin Sommer)^J^J0.22 | 2012-01-10 16:45:19 -0800^J^J * Tweaks for OpenBSD support. (Jon Siwek)^J^J * bro-cut extensions and fixes. (Robin Sommer)^J ^J - If no field names are given on the command line, we now pass through^J all fields. Adresses #657.^J^J - Removing some GNUism from awk script. Addresses #653.^J^J - Added option for time output in UTC. Addresses #668.^J^J - Added output field separator option -F. Addresses #649.^J^J - Fixing option -c: only some header lines were passed through^J + rather than all. (Robin Sommer)^J^J * Fix parallel make portability. (Jon Siwek)^J^J0.21-9 | 2011-11-07 05:44:14 -0800^J^J * Fixing compiler warnings. Addresses #388. (Jon Siwek)^J^J0.21-2 | 2011-11-02 18:12:13 -0700^J^J * Fix for misnaming temp file in update-changes script. (Robin Sommer)^J^J0.21-1 | 2011-11-02 18:10:39 -0700^J^J * Little fix for make-release script, which could pick out the wrong^J tag. (Robin Sommer)^J^J0.21 | 2011-10-27 17:40:45 -0700^J^J * Fixing bro-cut's usage message and argument error handling. (Robin Sommer)^J^J * Bugfix in update-changes script. (Robin Sommer)^J^J * update-changes now ignores commits it did itself. (Robin Sommer)^J^J * Fix a bug in the update-changes script. (Robin Sommer)^J^J * bro-cut now always installs to $prefix/bin by `make install`. (Jon Siwek)^J^J * Options to adjust time format for bro-cut. (Robin Sommer)^J^J The default with -d is now ISO format. The new option "-D "^J specifies a custom strftime()-style format string. Alternatively,^J the environment variable BRO_CUT_TIMEFMT can set the format as^J well.^J^J * bro-cut now understands the field separator header. (Robin Sommer)^J^J * Renaming options -h/-H -> -c/-C, and doing some general cleanup.^J^J0.2 | 2011-10-25 19:53:57 -0700^J^J * Adding support for replacing version string in a setup.py. (Robin^J Sommer)^J^J * Change generated root cert DN indices format for RFC2253^J compliance. (Jon Siwek)^J^J * New tool devel-tool +<1448 byte gap> + thread library when necessary (e.g.^J PF_RING's libpcap) (Jon Siwek)^J^J * Install binaries with an RPATH (Jon Siwek)^J^J * Workaround for FreeBSD CMake port missing debug flags (Jon Siwek)^J^J * Rewrite of the update-changes script. (Robin Sommer)^J^J0.1-1 | 2011-06-14 21:12:41 -0700^J^J * Add a script for generating Mozilla's CA list for the SSL analyzer.^J (Seth Hall)^J^J0.1 | 2011-04-01 16:28:22 -0700^J^J * Converting build process to CMake. (Jon Siwek)^J^J * Removing cf/hf/ca-* from distribution. The README has a note where^J to find them now. (Robin Sommer)^J^J * General cleanup. (Robin Sommer)^J^J * Initial import of bro/aux from SVN r7088. (Jon Siwek)^J diff --git a/testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/extract_files.file0 b/testing/btest/Baseline/scripts.base.protocols.http.entity-gap2/extract_files.file0 new file mode 100644 index 0000000000000000000000000000000000000000..8eb236cb557bb6b9df67b8c014c4898684617395 GIT binary patch literal 4705 zcmeHL+iu%N5bblmVvrW7V}vqEU2GFY07s4;7qJ~kPWzaw$rZIVZ{1y5R?v^{nI$Dt zmjdluADovs&hE_YnKNf*_{#5vBl?R%KL|bl#0v)$45Pr0`~i9XvG2cnMRZ6P>PZzT z)m0*_^y0ZFQfc1OVy@yj#buo(RH^uZ>|D}9mpB@ih1F$7GnL669Zbr5RnWA|bE)5K z*u4CFNk*jw+c&XRmEAp#AcvhG{eXggG#o~Q5rQ0cf@HTdmDXCew#b$wpOmGAvU4iR z!uu|DNyZ9W8!8KuT9=v*e#MVdwU7@84&DZ9Z^v*zK^P7FXwdIMG7-v1%2lEAIwvjv ztd*7tjjzRnq(`ZZf=PG}d!t@|(*YxTmL$(F=5vY6q?J?x%2}OdOAkn;FnF&;5znRB zi%Q=M*O3MuKmsn|AOb*0W)~L$JEa%bwta9ejS(ql=3Ep?W^^|fOP3nFzTdGK0P8Rc zMiGAK*t}av@nAS^jkpDnXmSTqR2h&JRLBMJl{5pqtNbzChZb!gDxyU@ant>@eCy|Rn z04fq7RH`0j!IWW=$6zuW%Tysz2ND++R0#t}=DMuYxidv1GA;oag2%H&19e#YaWp*c zaS;%CG?97v=%`TUk}hw)D3jAnmpO^W!@43sm*>m<^=14T6e%mqWMfpUf|0Ve6*7&2 z`*JsCzt49^C);H2yr39Ex~1Zc7)8GglFibQG$jsZ9T zkD)=No8Z9J9LFr=gn`;+cq=`)t?~b0Y0kyP=@$1YQ+BzPb&x3VP8;?HUJyXR$cIb} zqJZVBTM>*nl30O}nifK{{@(l8v@su2a1wESY)Av3ammP)0e#q$m3if) zkdv~hqU#iIYovQj(0xqf&5!zA6Kie$q^!+QXDg?G$lEN+JwgqNI37E(c1*M)-Aw?; zu#Uj@!ea`KqXDFF*o~m2LR)V?8>(4bsMQ!(bV8?fE{m1?Ama&2XOBlt`TIKM*Zg~^ z_|g`tT#x7~C91|1tFngB1`?IvFC(+r?qgSMgI3lld*?#mGW$;*itn!UhFyznA(rr1 zpwncT6Uv_|E%B@)ZiM$s`sTRY9{9G!G&XnZs@h!@$se^r!nn9m<+-qqU#-J^FChrJ zL{FKCI%}JshnE|tQ&-d5)=bYGb5h-C$ZPMMewpR=-D^h*Q^{CC8(7d|gaB1CR?j$) zgEyYBOD0|u5Pt6xdX`zK3t^#_+45~eErcSAM}@(_ao|yCCGK(c{p9xR_{-hbyQ}LD z7uR8t|BNf&qZ_inYy6P44|0lhfGEU(^I-A*;G9EyYq>I=_LDM8(KhhbWVa zaTpH!&Jv4WrZ7`*^WScnH)z_n#MqWdq|f6Ye|Y)xg1`#`F9^IK@V`a?{ZC`;#?Xz8 zQ7^HZV_PXCI)UMT9>`wWY+j!)zD}-gE-|LzXcdd<&2AsKYUUQgtb$&RX5!Xobmz(E zv%8PG=YKEtgTUUy>lL<#3oRw@X3a9p^Ewt~W4Plbl838VCQ`F&W+E3_@e(}yFSD25 z!!W*T#Qw;`tRDtEl@EAjUC+sA#N1uoCk7W~+dny~>+-wGG677+XMhE>7C(b0is|$d z1_6;R@!g=^T&EJB5(C95Yx~~-mluDPJ7hazo zM0+~%dM%7+NAuZHEWE>JD@+ibR`)fpe9iba`EYiA{ekAft=%Q?L+-U~MlarDkrg~8 z=h<&HRBV2Vx!f9Uws&KgI;5*YS%vvW6`r_G8x}dF2%$cezi9!-@$Am*#g}oCX5)KmJH}q8;)Ue*%)h&XXJ>}F zc)bx){Rkoy9-tDuB&`~W2b3Vh8+nM7B1I6=8$3jSN+hJHNT^aDka&Rboqu-MdvPL& zw94*!<~!&A&Ue0ZX8&;emv0UzgG%e)qeqHz03WV@X7X!)ogPwF@Sa}Ekw1OsgHP_< zyz|BfR}U$(igNm+*H0^_pRRrQ{n!6?@YWl@KK}80%Ibe!$QpFOL|1`ixKl=|Y?zcBjS;6Q@DR64Aj#bpCo9UDp& zO;h-|Vk9#%lv?mYyaim^3o1jYY0q_q8QGp|uvdie)uLl>i=os|>IP1B@`Z#a;;OF- z`O8-lUFa|!9QPqlMwH|^f<(S ze6Jg^#RSW08awyOP-=!p!eFIpJj%3jw#)BSiU>h^BNCycJVM3PCE-@0s=@N(T45-)N4~)#yC%Fi zI$0QHHNJCF*R;nM3I&vCrG}4_M`9<+_zt(-DP~rA5Q^yJMrlr+po*rnbasq=Tb@GI zCe&=6=>%yF0ooAt4aS1;(1KW@KFGrGvjwCgm={ciN zF!Ezave1zvg)Q5aI1o)N7I{qU*b%GSQI)x#t6ma;$HIt5amYNEi6HO-HhdOu*X_F4 zLUM?fZ$}$P0H$Y+f(E&CQnJSFP_UZq+O@dG0`aZb4ur*~m-&`p&{By0q3GH47)hx! z05Mv$ti2P{b(U~MB$x*(%dz7$R4`Qm;AX&GNHW})m7JPwXR50p3A|+U1_(kiJ9**M z-l`c#%>&m&81f2LW`?`AaL^yTAxneP=Q=QTaGf>uF#~TpFiUm8w?fK0K;9foB(;Mm zdO=88J_6|4+;TlFO|glQ;>^(g{jJCCAk3^DVN;^aPI3GjgKnU5AT1Z{+B?K%Zw8c<%uHpAWhe#L3Dm;npo(z_ssa;qNirmI6G|p9$94t! zK;VWt^LYqLs(}|*suGGZ?obN=2oZ}`0M>!_;#grkO-;a<%GN~9+m=%BYJ$zLY}jFq zl>@KFczvtg5s=H?&AIl?_!<)Nd@^gvG}#1$Pxz=b8g8RBO>y_`9h>OAH@P-BY$$vl z@CfWSA=SC=+w+s{+nbiqy3jO0jniNuouVqZB?3uK2y#FkQG-l7$RX$d*OQ(9E$RF( zc02!v4_mY36WTm5cU!XrbRzjG0!J_?qQ1Xpnr4Q40{k7iuTi^_fcU2E*ioZLN06ni zPA2`JtGW(PWEzYoZ(yi)^h1*k$ij!l16Jn&Ihw3`oBiCx#D4Hi-6!x_rcW5uab0Eu zcv;?>9YT<#mPpimX|7b}ZeMPTrq^}lVf4*xKBhX`I)#vr}^KhqMLeHr4GlO^3aE}{p~1Z7%Y};;lEHg<^IFA)CTv(rhw>V z9X+s*n|X9^%fTo#cj~->h%RQA<4Eg$$qxjMm7&qP*`@xQos&KxZ{^lE z;}8WSyMUY4cqH}J47lG(7)FG^lq&PsX^vABCl96bEv}zy!ldP8()<%wI;zgF6XjZT z>ZlY&=nIp!C>jwaQh?8eQBbBthL5RX)F9y{3E_KL(4rHGz~vGA*J+F<(n2c2-L|o6 zNDXZBz~-B{^~~D23)35=3#G;7*}3HsGhqZ+cqH*nSRP7C+P0Njr zB?(0^>cI0NW{MzUGb;eGY!emh5X#!zbT(VaNhC5o$HtaI>qPGmUcnj=c~}H3f%2*J zmYhHH6-Bu}ki2?#{(;|Jew&>C-E6n>e>5(g|2gH-wc#G;$6Ue4!N9?`6g04xVp8K7 zhL#+B92V(HvURM^U07ROnTMg^Qs3nMvHpw~lMZ9(DyRy_ma8Llw6PWH?S;#K1H)C3N5~Eq0EzT^@vMQG=rw9H}`#>ZnK|36(f}_;PrCRduehiuKX~N#U z5Ktp*(X}HRood%)K~W=kMg|YxNnY#cS6DEv^^SKE`fCt6Ju1_y?}vc=KYv3%1f2g_ z&kq5&uid!+gWV%v+O{tzLdT=>&uPse#4Ark{M=naJo#2X;)8p;N1p$bzu%8Imwr-? z6ZZ-63qRf$@mZ!^ywyg$-pcXj)PeS?d)r&dU%n=PB)HSv-Vg4fz2Dh=R=M=rzuWEo r5GTJ*r(Qa-huF_!@h6nyXaCrf<2%=GJa`vB(I^*l?HoTvjt~C}xh(JO literal 0 HcmV?d00001 diff --git a/testing/btest/scripts/base/protocols/http/entity-gap2.bro b/testing/btest/scripts/base/protocols/http/entity-gap2.bro new file mode 100644 index 0000000000..c9ade93b72 --- /dev/null +++ b/testing/btest/scripts/base/protocols/http/entity-gap2.bro @@ -0,0 +1,24 @@ +# @TEST-EXEC: bro -r $TRACES/http/entity_gap2.trace %INPUT +# @TEST-EXEC: btest-diff entity_data +# @TEST-EXEC: btest-diff extract_files/file0 + +global f = open("entity_data"); +global fn = 0; + +event http_entity_data(c: connection, is_orig: bool, length: count, + data: string) + { + print f, data; + } + +event content_gap(c: connection, is_orig: bool, seq: count, length: count) + { + print f, fmt("<%d byte gap>", length); + } + +event file_new(f: fa_file) + { + Files::add_analyzer(f, Files::ANALYZER_EXTRACT, + [$extract_filename=fmt("file%d", fn)]); + ++fn; + }