From 5b3573394edbcf6c8926e84c21d84c13faa412e7 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 24 Jan 2014 15:51:58 -0600 Subject: [PATCH 1/4] Improve TCP FIN retransmission handling. In the case multiple FIN packets are seen from a TCP endpoint (e.g. when one is retransmitted), only the first counted towards a byte in the sequence space. This could cause a subsequent FIN packet to induce an incorrect wrap around in the sequence numbers (e.g. the retransmitted FIN packet now is one sequence number behind the the first) and misleadingly large connection sizes. The change is to always treat a FIN packet as counting one byte in to the sequence space. --- src/analyzer/protocol/tcp/TCP.cc | 11 ++++------- .../btest/Baseline/core.tcp.fin-retransmit/out | 2 ++ testing/btest/Traces/tcp/fin_retransmission.pcap | Bin 0 -> 434 bytes testing/btest/core/tcp/fin-retransmit.bro | 8 ++++++++ 4 files changed, 14 insertions(+), 7 deletions(-) create mode 100644 testing/btest/Baseline/core.tcp.fin-retransmit/out create mode 100644 testing/btest/Traces/tcp/fin_retransmission.pcap create mode 100644 testing/btest/core/tcp/fin-retransmit.bro diff --git a/src/analyzer/protocol/tcp/TCP.cc b/src/analyzer/protocol/tcp/TCP.cc index aefc5a1808..57c4ebef18 100644 --- a/src/analyzer/protocol/tcp/TCP.cc +++ b/src/analyzer/protocol/tcp/TCP.cc @@ -373,14 +373,11 @@ void TCP_Analyzer::ProcessSYN(const IP_Hdr* ip, const struct tcphdr* tp, void TCP_Analyzer::ProcessFIN(double t, TCP_Endpoint* endpoint, int& seq_len, uint32 base_seq) { - if ( endpoint->FIN_cnt == 0 ) - { - ++seq_len; // FIN consumes a byte of sequence space - ++endpoint->FIN_cnt; // remember that we've seen a FIN - } + ++seq_len; // FIN consumes a byte of sequence space. + ++endpoint->FIN_cnt; // remember that we've seen a FIN - else if ( t < endpoint->last_time + tcp_storm_interarrival_thresh && - ++endpoint->FIN_cnt == tcp_storm_thresh ) + if ( t < endpoint->last_time + tcp_storm_interarrival_thresh && + endpoint->FIN_cnt == tcp_storm_thresh ) Weird("FIN_storm"); // Remember the relative seq in FIN_seq. diff --git a/testing/btest/Baseline/core.tcp.fin-retransmit/out b/testing/btest/Baseline/core.tcp.fin-retransmit/out new file mode 100644 index 0000000000..8afb8222c9 --- /dev/null +++ b/testing/btest/Baseline/core.tcp.fin-retransmit/out @@ -0,0 +1,2 @@ +[size=0, state=5, num_pkts=3, num_bytes_ip=156, flow_label=0] +[size=0, state=6, num_pkts=2, num_bytes_ip=92, flow_label=0] diff --git a/testing/btest/Traces/tcp/fin_retransmission.pcap b/testing/btest/Traces/tcp/fin_retransmission.pcap new file mode 100644 index 0000000000000000000000000000000000000000..1e17844af55bcf6ccb65c637d5f4f9f3f1173321 GIT binary patch literal 434 zcmca|c+)~A1{MYw`2U}Qff2|lH#!zHv5}R*56A}LBZdF0Vh=^guQHL?!@=Onz~Ery z<-p*;#w*OlC|4@D{Gim6=W*o?i&Wthj-opy7^s$ko-S+Td;crAns*h0-5Mz$i-j-G#!L7 z+^EsH59G$J+tA$zH1%eHpaX;bGLXwa0A$v$i+pgiFkGjR&jxaxMjo2$ZuY$_{20KZ Mz_~;MViE%b0C*yJumAu6 literal 0 HcmV?d00001 diff --git a/testing/btest/core/tcp/fin-retransmit.bro b/testing/btest/core/tcp/fin-retransmit.bro new file mode 100644 index 0000000000..42bf062f5a --- /dev/null +++ b/testing/btest/core/tcp/fin-retransmit.bro @@ -0,0 +1,8 @@ +# @TEST-EXEC: bro -b -r $TRACES/tcp/fin_retransmission.pcap %INPUT >out +# @TEST-EXEC: btest-diff out + +event connection_state_remove(c: connection) + { + print c$orig; + print c$resp; + } From 9b12967d40b2624cec097732fd8e0b0efaae4612 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 24 Jan 2014 16:21:02 -0600 Subject: [PATCH 2/4] Improve gap reporting in TCP connections that never see data. The previous behavior was to accomodate SYN/FIN/RST-filtered traces by not reporting missing data (via the content_gap event) for such connections. The new behavior always reports gaps for connections that are established and terminate normally, but sequence numbers indicate that all data packets of the connection were missed. The behavior can be reverted by redef'ing "detect_filtered_trace". --- scripts/base/init-bare.bro | 6 ++++++ src/analyzer/protocol/tcp/TCP_Reassembler.cc | 2 +- src/const.bif | 1 + .../Baseline/core.tcp.miss-end-data/conn.log | 10 ++++++++++ .../btest/Baseline/core.tcp.miss-end-data/out | 1 + testing/btest/Traces/tcp/miss_end_data.pcap | Bin 0 -> 1216 bytes testing/btest/core/tcp/miss-end-data.bro | 10 ++++++++++ 7 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/core.tcp.miss-end-data/conn.log create mode 100644 testing/btest/Baseline/core.tcp.miss-end-data/out create mode 100644 testing/btest/Traces/tcp/miss_end_data.pcap create mode 100644 testing/btest/core/tcp/miss-end-data.bro diff --git a/scripts/base/init-bare.bro b/scripts/base/init-bare.bro index 8d4899b785..ce8d68d289 100644 --- a/scripts/base/init-bare.bro +++ b/scripts/base/init-bare.bro @@ -2849,6 +2849,12 @@ global load_sample_freq = 20 &redef; ## .. bro:see:: gap_report const gap_report_freq = 1.0 sec &redef; +## Whether to attempt to automatically detect SYN/FIN/RST-filtered trace +## and not report missing segments for such connections. +## If this is enabled, then missing data at the end of connections may not +## be reported via :bro:see:`content_gap`. +const detect_filtered_trace = F &redef; + ## Whether we want :bro:see:`content_gap` and :bro:see:`gap_report` for partial ## connections. A connection is partial if it is missing a full handshake. Note ## that gap reports for partial connections might not be reliable. diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index a1e20dc0e6..49292a04a5 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -178,7 +178,7 @@ void TCP_Reassembler::Undelivered(int up_to_seq) // to this method and only if this condition is not true). reporter->InternalError("Calling Undelivered for data that has already been delivered (or has already been marked as undelivered"); - if ( last_reassem_seq == 1 && + if ( BifConst::detect_filtered_trace && last_reassem_seq == 1 && (endpoint->FIN_cnt > 0 || endpoint->RST_cnt > 0 || peer->FIN_cnt > 0 || peer->RST_cnt > 0) ) { diff --git a/src/const.bif b/src/const.bif index fd0419c7d9..0ba168ca85 100644 --- a/src/const.bif +++ b/src/const.bif @@ -5,6 +5,7 @@ const ignore_keep_alive_rexmit: bool; const skip_http_data: bool; const use_conn_size_analyzer: bool; +const detect_filtered_trace: bool; const report_gaps_for_partial: bool; const exit_only_after_terminate: bool; diff --git a/testing/btest/Baseline/core.tcp.miss-end-data/conn.log b/testing/btest/Baseline/core.tcp.miss-end-data/conn.log new file mode 100644 index 0000000000..723e5becc3 --- /dev/null +++ b/testing/btest/Baseline/core.tcp.miss-end-data/conn.log @@ -0,0 +1,10 @@ +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path conn +#open 2014-01-24-22-19-38 +#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 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 count string count count count count table[string] +1331764471.664131 CXWv6p3arKYeMETxOg 192.168.122.230 60648 77.238.160.184 80 tcp http 10.048360 538 2902 SF - 2902 ShADafF 5 750 4 172 (empty) +#close 2014-01-24-22-19-38 diff --git a/testing/btest/Baseline/core.tcp.miss-end-data/out b/testing/btest/Baseline/core.tcp.miss-end-data/out new file mode 100644 index 0000000000..cd5881035f --- /dev/null +++ b/testing/btest/Baseline/core.tcp.miss-end-data/out @@ -0,0 +1 @@ +content_gap, [orig_h=192.168.122.230, orig_p=60648/tcp, resp_h=77.238.160.184, resp_p=80/tcp], F, 1, 2902 diff --git a/testing/btest/Traces/tcp/miss_end_data.pcap b/testing/btest/Traces/tcp/miss_end_data.pcap new file mode 100644 index 0000000000000000000000000000000000000000..ae5aecbaac78f2ebd7243f373f1142c690fbda0e GIT binary patch literal 1216 zcmaLXOH30%7y#huf<_xl025+-gpHA=KzFxG>24pXDMemE+6HR$V4}ZyVoczQ6gf zy8{Z_!SOvTEfORMyaa$~Klk3()<*bz?F1Ho@=ejRJl7Ke$N=vA)n%+<6%=-Nrq^D? zk-ERw-Sg$n#-WHbq+Z10dfX2!@PIWmD_$$Ab{Ii z6(TD_tW>W3SGkhupPK|=Vys9q+z=aM<{e3Trii*`F&rO_#fGDaOfJIH3r2~YEEWq4 zOS1w0q;A2kf?)}I6T(oV9Lw#; zP~YuDk5`RpqAn>~DM^+ZiV;R@WmT}@@%U(2FfE9}oSMs1@pvpjv0=xSskrO#VJ`|k z;2+gB4T?DSBw2ybpafN^A!ioK(1d1^EZeq`WEiv^{3o#tUa40U6n6GO8ZHbk*zp8k z9paoya6TOkT4_;Ek+S-zmbP@KY@|q47ZKfX)0B3P+852%>*zx)9y4rd}DH3o-+9LOr`27)X5ujsB|G^ d(^G^WP33f64*w5cx14ANo#-;o%hEmoe*w*=ZlnMJ literal 0 HcmV?d00001 diff --git a/testing/btest/core/tcp/miss-end-data.bro b/testing/btest/core/tcp/miss-end-data.bro new file mode 100644 index 0000000000..6cee7577d9 --- /dev/null +++ b/testing/btest/core/tcp/miss-end-data.bro @@ -0,0 +1,10 @@ +# @TEST-EXEC: bro -r $TRACES/tcp/miss_end_data.pcap %INPUT >out +# @TEST-EXEC: btest-diff out +# @TEST-EXEC: btest-diff conn.log + +redef report_gaps_for_partial = T; + +event content_gap(c: connection, is_orig: bool, seq: count, length: count) + { + print "content_gap", c$id, is_orig, seq, length; + } From 6d46144c3b1453d429848a699767215b61302024 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 24 Jan 2014 16:32:55 -0600 Subject: [PATCH 3/4] Improve TCP connection size reporting for half-open connections. If TCP endpoint A and B are synchronized at some point, but A closes/aborts/crashes and B goes on without knowledge of it and then A tries to re-synchronize, Bro could end up seeing something like (sequence numbers made up): A: SYN 100 B: ACK 500 A: RST 500 The final sequence number of A, in this case, is not useful in the context of determining the number of data bytes sent by A, so Bro now reports that as 0 (where before it could often be misleadingly large). --- src/analyzer/protocol/tcp/TCP_Endpoint.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/analyzer/protocol/tcp/TCP_Endpoint.cc b/src/analyzer/protocol/tcp/TCP_Endpoint.cc index d596234021..ad642a46e3 100644 --- a/src/analyzer/protocol/tcp/TCP_Endpoint.cc +++ b/src/analyzer/protocol/tcp/TCP_Endpoint.cc @@ -161,6 +161,13 @@ void TCP_Endpoint::SetState(EndpointState new_state) bro_int_t TCP_Endpoint::Size() const { + if ( prev_state == TCP_ENDPOINT_SYN_SENT && state == TCP_ENDPOINT_RESET && + peer->state == TCP_ENDPOINT_INACTIVE && ! NoDataAcked() ) + // This looks like a half-open connection was discovered and aborted. + // Sequence numbers could be misleading if used in context of data size + // and there was never a chance for this endpoint to send data anyway. + return 0; + bro_int_t size; uint64 last_seq_64 = (uint64(last_seq_high) << 32) | last_seq; From e09763e0613ea6ea8134d11957e419e3061b5db0 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 24 Jan 2014 16:47:00 -0600 Subject: [PATCH 4/4] Fix file_over_new_connection event to trigger when entire file is missed. If a file is nothing but gaps (e.g. due to missing/dropped packets), Bro can sometimes detect a file is supposed to have been present and never saw any of its content, but failed to raise file_over_new_connection events for it. This was mostly apparent because the tx_hosts/rx_hosts fields in files.log would not be populated in such cases (but are now with this change). --- src/file_analysis/File.cc | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/file_analysis/File.cc b/src/file_analysis/File.cc index 55b28763c8..deda0f9e93 100644 --- a/src/file_analysis/File.cc +++ b/src/file_analysis/File.cc @@ -103,7 +103,6 @@ File::~File() DBG_LOG(DBG_FILE_ANALYSIS, "Destroying File object %s", id.c_str()); Unref(val); - // Queue may not be empty in the case where only content gaps were seen. while ( ! fonc_queue.empty() ) { delete_vals(fonc_queue.front().second); @@ -460,20 +459,27 @@ void File::FileEvent(EventHandlerPtr h) FileEvent(h, vl); } +static void flush_file_event_queue(queue >& q) + { + while ( ! q.empty() ) + { + pair p = q.front(); + mgr.QueueEvent(p.first, p.second); + q.pop(); + } + } + void File::FileEvent(EventHandlerPtr h, val_list* vl) { + if ( h == file_state_remove ) + flush_file_event_queue(fonc_queue); + mgr.QueueEvent(h, vl); if ( h == file_new ) { did_file_new_event = true; - - while ( ! fonc_queue.empty() ) - { - pair p = fonc_queue.front(); - mgr.QueueEvent(p.first, p.second); - fonc_queue.pop(); - } + flush_file_event_queue(fonc_queue); } if ( h == file_new || h == file_timeout || h == file_extraction_limit )