From 2f5f0d84082895cb411268d523db85fc020807a6 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 6 Mar 2014 17:08:45 -0600 Subject: [PATCH 1/2] Fix bug in Connection::FlipRoles, addresses BIT-1148. It didn't swap address values right and also didn't consider that analyzers might be scheduled for the new connection tuple. Issues were reported by Kevin McMahon, thanks. --- src/Conn.cc | 7 +++++-- src/analyzer/Manager.cc | 22 ++++++++++++++++++++++ src/analyzer/Manager.h | 7 +++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/Conn.cc b/src/Conn.cc index 0098d297e0..fa89f26d35 100644 --- a/src/Conn.cc +++ b/src/Conn.cc @@ -15,6 +15,7 @@ #include "binpac.h" #include "TunnelEncapsulation.h" #include "analyzer/Analyzer.h" +#include "analyzer/Manager.h" void ConnectionTimer::Init(Connection* arg_conn, timer_func arg_timer, int arg_do_expire) @@ -722,8 +723,8 @@ TimerMgr* Connection::GetTimerMgr() const void Connection::FlipRoles() { IPAddr tmp_addr = resp_addr; - orig_addr = resp_addr; - resp_addr = tmp_addr; + resp_addr = orig_addr; + orig_addr = tmp_addr; uint32 tmp_port = resp_port; resp_port = orig_port; @@ -742,6 +743,8 @@ void Connection::FlipRoles() if ( root_analyzer ) root_analyzer->FlipRoles(); + + analyzer_mgr->ApplyScheduledAnalyzers(this); } unsigned int Connection::MemoryAllocation() const diff --git a/src/analyzer/Manager.cc b/src/analyzer/Manager.cc index 6268fde177..56b838b5b3 100644 --- a/src/analyzer/Manager.cc +++ b/src/analyzer/Manager.cc @@ -636,3 +636,25 @@ Manager::tag_set Manager::GetScheduled(const Connection* conn) // eventually. return result; } + +void Manager::ApplyScheduledAnalyzers(Connection* conn) + { + TransportLayerAnalyzer* root = conn->GetRootAnalyzer(); + + if ( ! root ) + return; + + tag_set expected = GetScheduled(conn); + + for ( tag_set::iterator it = expected.begin(); it != expected.end(); ++it ) + { + Analyzer* analyzer = analyzer_mgr->InstantiateAnalyzer(*it, conn); + + if ( ! analyzer ) + continue; + + root->AddChildAnalyzer(analyzer, true); + DBG_ANALYZER_ARGS(conn, "activated %s analyzer as scheduled", + analyzer_mgr->GetComponentName(*it)); + } + } diff --git a/src/analyzer/Manager.h b/src/analyzer/Manager.h index 77b40a1d68..8db2bf3a98 100644 --- a/src/analyzer/Manager.h +++ b/src/analyzer/Manager.h @@ -292,6 +292,13 @@ public: TransportProto proto, const char* analyzer, double timeout); + /** + * Searched for analyzers scheduled to be attached to a given connection + * and then attaches them. + * @param conn The connection to which scheduled analyzers are attached. + */ + void ApplyScheduledAnalyzers(Connection* conn); + /** * Schedules a particular analyzer for an upcoming connection. Once * the connection is seen, BuildInitAnalyzerTree() will add the From 066473b1f1bc71fc500a0a0ec447e1603732c53a Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 11 Mar 2014 16:56:17 -0500 Subject: [PATCH 2/2] Improve analysis of TCP SYN/SYN-ACK reversal situations. - Since it's just the handshake packets out of order, they're no longer treated as partial connections, which some protocol analyzers immediately refuse to look at. - The TCP_Reassembler "is_orig" state failed to change, which led to protocol analyzers sometimes using the wrong value for that. - Add a unit test which exercises the Connection::FlipRoles() code path (i.e. the SYN/SYN-ACK reversal situation). Addresses BIT-1148. --- src/analyzer/protocol/pia/PIA.cc | 4 +- src/analyzer/protocol/tcp/TCP.cc | 7 ++-- src/analyzer/protocol/tcp/TCP_Reassembler.cc | 29 +++++++------ src/analyzer/protocol/tcp/TCP_Reassembler.h | 6 +-- .../Baseline/core.connection_flip_roles/out | 4 ++ .../btest/Traces/tcp/handshake-reorder.trace | Bin 0 -> 6335 bytes testing/btest/core/connection_flip_roles.bro | 39 ++++++++++++++++++ 7 files changed, 65 insertions(+), 24 deletions(-) create mode 100644 testing/btest/Baseline/core.connection_flip_roles/out create mode 100644 testing/btest/Traces/tcp/handshake-reorder.trace create mode 100644 testing/btest/core/connection_flip_roles.bro diff --git a/src/analyzer/protocol/pia/PIA.cc b/src/analyzer/protocol/pia/PIA.cc index ab2d1833e8..388d1d501d 100644 --- a/src/analyzer/protocol/pia/PIA.cc +++ b/src/analyzer/protocol/pia/PIA.cc @@ -331,11 +331,11 @@ void PIA_TCP::ActivateAnalyzer(analyzer::Tag tag, const Rule* rule) tcp::TCP_Reassembler* reass_orig = new tcp::TCP_Reassembler(this, tcp, tcp::TCP_Reassembler::Direct, - true, tcp->Orig()); + tcp->Orig()); tcp::TCP_Reassembler* reass_resp = new tcp::TCP_Reassembler(this, tcp, tcp::TCP_Reassembler::Direct, - false, tcp->Resp()); + tcp->Resp()); int orig_seq = 0; int resp_seq = 0; diff --git a/src/analyzer/protocol/tcp/TCP.cc b/src/analyzer/protocol/tcp/TCP.cc index 57c4ebef18..c422d01f32 100644 --- a/src/analyzer/protocol/tcp/TCP.cc +++ b/src/analyzer/protocol/tcp/TCP.cc @@ -95,9 +95,9 @@ void TCP_Analyzer::Done() void TCP_Analyzer::EnableReassembly() { SetReassembler(new TCP_Reassembler(this, this, - TCP_Reassembler::Forward, true, orig), - new TCP_Reassembler(this, this, - TCP_Reassembler::Forward, false, resp)); + TCP_Reassembler::Forward, orig), + new TCP_Reassembler(this, this, + TCP_Reassembler::Forward, resp)); reassembling = 1; @@ -590,6 +590,7 @@ void TCP_Analyzer::UpdateInactiveState(double t, // per the discussion in IsReuse. // Flip the endpoints and establish // the connection. + is_partial = 0; Conn()->FlipRoles(); peer->SetState(TCP_ENDPOINT_ESTABLISHED); } diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index 49292a04a5..da8dd857fd 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -32,13 +32,12 @@ static uint64 last_gap_bytes = 0; TCP_Reassembler::TCP_Reassembler(analyzer::Analyzer* arg_dst_analyzer, TCP_Analyzer* arg_tcp_analyzer, TCP_Reassembler::Type arg_type, - bool arg_is_orig, TCP_Endpoint* arg_endp) + TCP_Endpoint* arg_endp) : Reassembler(1, REASSEM_TCP) { dst_analyzer = arg_dst_analyzer; tcp_analyzer = arg_tcp_analyzer; type = arg_type; - is_orig = arg_is_orig; endp = arg_endp; had_gap = false; record_contents_file = 0; @@ -165,10 +164,10 @@ void TCP_Reassembler::Undelivered(int up_to_seq) if ( DEBUG_tcp_contents ) { - DEBUG_MSG("%.6f Undelivered: is_orig=%d up_to_seq=%d, last_reassm=%d, " + DEBUG_MSG("%.6f Undelivered: IsOrig()=%d up_to_seq=%d, last_reassm=%d, " "endp: FIN_cnt=%d, RST_cnt=%d, " "peer: FIN_cnt=%d, RST_cnt=%d\n", - network_time, is_orig, up_to_seq, last_reassem_seq, + network_time, IsOrig(), up_to_seq, last_reassem_seq, endpoint->FIN_cnt, endpoint->RST_cnt, peer->FIN_cnt, peer->RST_cnt); } @@ -196,9 +195,9 @@ void TCP_Reassembler::Undelivered(int up_to_seq) { if ( DEBUG_tcp_contents ) { - DEBUG_MSG("%.6f Undelivered: is_orig=%d, seq=%d, len=%d, " + DEBUG_MSG("%.6f Undelivered: IsOrig()=%d, seq=%d, len=%d, " "skip_deliveries=%d\n", - network_time, is_orig, last_reassem_seq, + network_time, IsOrig(), last_reassem_seq, seq_delta(up_to_seq, last_reassem_seq), skip_deliveries); } @@ -229,7 +228,7 @@ void TCP_Reassembler::Undelivered(int up_to_seq) { val_list* vl = new val_list; vl->append(dst_analyzer->BuildConnVal()); - vl->append(new Val(is_orig, TYPE_BOOL)); + vl->append(new Val(IsOrig(), TYPE_BOOL)); vl->append(new Val(seq, TYPE_COUNT)); vl->append(new Val(len, TYPE_COUNT)); @@ -238,11 +237,11 @@ void TCP_Reassembler::Undelivered(int up_to_seq) if ( type == Direct ) dst_analyzer->NextUndelivered(last_reassem_seq, - len, is_orig); + len, IsOrig()); else { dst_analyzer->ForwardUndelivered(last_reassem_seq, - len, is_orig); + len, IsOrig()); } } @@ -289,7 +288,7 @@ void TCP_Reassembler::MatchUndelivered(int up_to_seq) for ( b = blocks; b && seq_delta(b->upper, last_reassem_seq) <= 0; b = b->next ) tcp_analyzer->Conn()->Match(Rule::PAYLOAD, b->block, b->Size(), - false, false, is_orig, false); + false, false, IsOrig(), false); ASSERT(b); } @@ -410,7 +409,7 @@ void TCP_Reassembler::BlockInserted(DataBlock* start_block) void TCP_Reassembler::Overlap(const u_char* b1, const u_char* b2, int n) { if ( DEBUG_tcp_contents ) - DEBUG_MSG("%.6f TCP contents overlap: %d is_orig=%d\n", network_time, n, is_orig); + DEBUG_MSG("%.6f TCP contents overlap: %d IsOrig()=%d\n", network_time, n, IsOrig()); if ( rexmit_inconsistency && memcmp((const void*) b1, (const void*) b2, n) && @@ -442,9 +441,9 @@ bool TCP_Reassembler::DoUnserialize(UnserialInfo* info) void TCP_Reassembler::Deliver(int seq, int len, const u_char* data) { if ( type == Direct ) - dst_analyzer->NextStream(len, data, is_orig); + dst_analyzer->NextStream(len, data, IsOrig()); else - dst_analyzer->ForwardStream(len, data, is_orig); + dst_analyzer->ForwardStream(len, data, IsOrig()); } int TCP_Reassembler::DataSent(double t, int seq, int len, @@ -455,8 +454,8 @@ int TCP_Reassembler::DataSent(double t, int seq, int len, if ( DEBUG_tcp_contents ) { - DEBUG_MSG("%.6f DataSent: is_orig=%d seq=%d upper=%d ack=%d\n", - network_time, is_orig, seq, upper_seq, ack); + DEBUG_MSG("%.6f DataSent: IsOrig()=%d seq=%d upper=%d ack=%d\n", + network_time, IsOrig(), seq, upper_seq, ack); } if ( skip_deliveries ) diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.h b/src/analyzer/protocol/tcp/TCP_Reassembler.h index 8bb80a0570..e3dcf82a1b 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.h +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.h @@ -28,9 +28,8 @@ public: Forward, // forward to destination analyzer's children }; - TCP_Reassembler(Analyzer* arg_dst_analyzer, - TCP_Analyzer* arg_tcp_analyzer, Type arg_type, - bool arg_is_orig, TCP_Endpoint* arg_endp); + TCP_Reassembler(Analyzer* arg_dst_analyzer, TCP_Analyzer* arg_tcp_analyzer, + Type arg_type, TCP_Endpoint* arg_endp); virtual ~TCP_Reassembler(); @@ -135,7 +134,6 @@ private: TCP_Analyzer* tcp_analyzer; Type type; - bool is_orig; }; } } // namespace analyzer::* diff --git a/testing/btest/Baseline/core.connection_flip_roles/out b/testing/btest/Baseline/core.connection_flip_roles/out new file mode 100644 index 0000000000..2f596620bf --- /dev/null +++ b/testing/btest/Baseline/core.connection_flip_roles/out @@ -0,0 +1,4 @@ +schedule_analyzer, current conn_id, [orig_h=192.150.187.43, orig_p=80/tcp, resp_h=141.142.228.5, resp_p=59856/tcp] +http_request, 1.1, GET, /download/CHANGES.bro-aux.txt +http_reply, 1.1, 200, OK +connection_state_remove, [orig_h=141.142.228.5, orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp] diff --git a/testing/btest/Traces/tcp/handshake-reorder.trace b/testing/btest/Traces/tcp/handshake-reorder.trace new file mode 100644 index 0000000000000000000000000000000000000000..00581422cb25bd08a4451a93fc4bc98416438e7f GIT binary patch literal 6335 zcma)AU2Ggz72dc>TNb&b2%$ceTel^S>)AiA|0YeFjU78lZO7Q_R=m($&)nU;_3X@W zXU6M|km^Sep+y2J!ApN25)Y_Bh&TFBsYR-wq<`=b0jeq?RYgLT`hdg(gzwy$S?`|% zkycsTGv7V;JKy=vx%ZE6{OaYA%+XBx@7AqM<_Lax^>g!ITdCfYxrp!ddF!>eUU>hd z%<(^e`~6Sv&Ya6+9?fLVWb$`@qxSj>zj@?)&;9LKX8n_QuU)_K*nb`8bx&pusPu_3V|p(T>;a`s2x^?RH7#V0sj~ux>OUU%Mqa$(L3g~C?Gkpfo%WuC(==(SN&`%-u z$M=u3N1{_;AV#fG+!XI!Pu`RH9l+=_fcVs<%jpjI|ENIA`3M_9Dg{I`TMc>(~o%UFIQhC z#J3+h4Dlb|?L|zPV5LHVU3y|PyU0UfvT7rmWQAF_$`$@noMNS-S)MZSo8{H&Xm(Sm zU7<`?@wwFy`BJW&n_}Zj!uAvgT`N2YjaAQ$g@ zkZ-b3HiZ|3r>7@blkc4_77B+a3JKI`)wYkFhhi_x`wo}xDP}de3PgB%tGZ;&QAV?c zTxo`VOYg!c%o(LJD^8omf>|gtqcB@2WV4J-uqC-C-8zeW8)O`KUuJ=p`^P00qAfnZJP%4XDLN_;btO4w{+mG1}!Ed{6_ zTD_E;At`k)K#W#w`(VfPoGly?3g&^zTI4uw15DKcxUIMgNd`k%nKDY9NQ(wY0xzYq z34)N!ZeBQ*w_!zLvf-u(0$!)gEOJ*02lc_*IyKn+R2OCuT$jw^jEOH@nAMixI|0S5 z5H|-Caqb`r-vP6nxuOMr1*?JHG`}Ql<8?P@7_ zO~IDewq(#`HRUxKZ|!s{0&+Qcd8+d=ehrCuKAE*CwycY1-DgQpmh)4otf)@ zH-0wWY#@BDcnEeANMot@_40h@^~4g=2Q3rSIBgctE*gT{LTPdWkOT6F8bsPf4mtn7 zZtwiBXy<>h*ZDvAIPE2$(&T}@oAwgWiQpSpID|nF^@BB&m>J?x_&appq;e$z@onkI zu-&I4$g*h6$MsM&ibZ%LE5LZlCYowjJtS;E7CtntSc@xiG@bPh`%`ms!{8I$$M7Xq zoHME7ddvp!I=xL9K#-)CP&9pQu9W6pU+##e-*xq7irt%uS1XuPRAn8vUy)(xpmNhi zL6Om0=0mI>qSKY3?O$s<Pz;?23IxuT&KGuNa#!|_(Z@Wax0d&mM&wi z>QNItGchz|+(t1DS&>qHHmcKZ{(q|Ir9M`v!qKNZ)S)ha83qiE#g;by3xrb}+-#~g zxGyFJtWMU^2YXo0qjysWqouji;%%(xqIWrtw%&()C9thLwbt!6yR5aGJaD;BX#{sk z`-Fa#+s{M+5=M3bH*NAz>nmAs*iIN)gg}?7@yJQqsezpb+W9VQUP@qc`Y~<(u`7)k zi|o-_Gkk1Jiz4uaMN<^D2n!*==YmkxD3I}!Mi911cyU1Zy-sMw359Zb2>*53ld-fA zig0%&Mh&fjU9KeG#;a#HE*8lolO5Ah_@O>^YJ zxj?aNla*5uP!~8b#@O115i|iSPl_`(#ef-&ym6ixk1@l_8w-3=vW+32%}b>7cxtzkS(qO#FiE^)+LmUrG-*ydP*aa={XWp3XK!}O?VAsK<&{uN)hrfE;ng6HS`Nw*l|K{_l^Y8zl*ZG4yCM0X8V^TDhM5qaYh~XN1hnPtm z?2!3`>f<29Am3M|I7oGdnpoEcr|AdN0j7Y^(?p;gccq8vho%EqgRG&m1c8Jwb8IoG zoTOr0@Qe4yBi$A~#7c70Fdhb)K!)OEny=ouImy+kcSSl5f>2b*I$ufplED*M6MH&` z0l7mEZ9juk9r8IOO9`eegHVf+LeuSB(R3nUUlLVdOEu(OJ@C?-+6naQap_o|kJA#Q z9w|!Nw1|70A+628yTw-`moHa;CQRa9a22;=~ z6%GLc)35djUnulZJrE9`MgNBu$c+Jw@t4Bn>_noh6AT>>@#rKeihJH#P7%I`ks}@1DlG~A<_)_EOpJfrj1f$A}n@f+ddw2$UmJ&GA zrCADbJl>}MayT=cfpy!qIOguB=wwA(Ak%ledSVlk_#L#4f#pO<8*8J73!H2;JKSwI zQJTXAp8A_U3<3qcDQI{t)F1SMsb3$QAPZK*soclJm$u^+PA}2pZ<`4gWQM~E9S;}Q zx8m-H(x6)vatQPa=@K7-wrD8It7)v{Y?I>m)+3JBidzsVu1!+HNg}r3Xwx`%vWZ;% zik{Vm9O5WbRq28qeYVzx7mwtS19vc7EL1Q^qq^wuq~)@?xuVNjho+xFZ}eP5w<$oz zdC-&`u@ff8l9_;Ri^d>QoGX#H?qflzgtz;x`w*#h9@Fh?=)vB#*B2`5+7B?3p-^}o z*AGER&Yy?#-yDfQ{kJ_MzrXr3a{hNpz0UvPtakqUGFPsR_c}l1fqc<94CiEMtku97 z4YyZxyv2{>B3F-o@f84_DR4aipMkP$K?7jT;q_n;W<#_T>>gs%Wz^Ss4x0}P zIUE?$VWBW%6w7p_g_A>>4(gp*9$BUdAl5%cnKlVTm9!9W($N?mpi(A@ahE%7+?FIx z(5)@3-C4P8&~?m=S(-C(g6~=xw@@hikhUMU4_|qd7m~Q9Xo%!G-eT4AjauH~#zcG; zrv8%JUOZxMoU1IZo?{KJ`?3fAkb4z(P^?fx`=hNiNrl4;ce!u;8tS4KTp3EtXtJpj ziX35D%%hOvxHg|$V`{e0f3PE=KMSGLB`JLdu7Jz`{4HGpKl$^%E8sV-UBCJK{_(Hu z%B!)^aY_0MI^YuGi?>Dm!rO#+`jtV%k8bQAzyCA--XP*q?shTG-6X^>{b(rSy)1M2 z^$y~zX^gL(I?~y7e|cxhSFcfdzxP&ed4F^R<^AUVy_qY|{<~A&A7kgQ(ymt?JwP1f gvGQYz@$-M`i}B5C*FSm(m-Cs+Q=J$;LyRB)7wymPdH?_b literal 0 HcmV?d00001 diff --git a/testing/btest/core/connection_flip_roles.bro b/testing/btest/core/connection_flip_roles.bro new file mode 100644 index 0000000000..e68d94c5fe --- /dev/null +++ b/testing/btest/core/connection_flip_roles.bro @@ -0,0 +1,39 @@ +# @TEST-EXEC: bro -b -r $TRACES/tcp/handshake-reorder.trace %INPUT >out +# @TEST-EXEC: btest-diff out + +# This tests the Connection::FlipRoles code path (SYN/SYN-ACK reversal). + +# The check of likely_server_ports is before Connection::FlipRoles, so +# need to make sure that isn't the mechanism used to flip src/dst stuff. +redef likely_server_ports = {}; + +global first_packet: bool = T; + +event new_packet(c: connection, p: pkt_hdr) + { + if ( ! first_packet ) + return; + + first_packet = F; + + print "schedule_analyzer, current conn_id", c$id; + # Anticipate roles getting flipped in next packet. + Analyzer::schedule_analyzer(141.142.228.5, 192.150.187.43, 80/tcp, + Analyzer::ANALYZER_HTTP, 2mins); + } + +event connection_state_remove(c: connection) + { + print "connection_state_remove", c$id; + } + +event http_request(c: connection, method: string, original_URI: string, + unescaped_URI: string, version: string) + { + print "http_request", version, method, original_URI; + } + +event http_reply(c: connection, version: string, code: count, reason: string) + { + print "http_reply", version, code, reason; + }