From 81ae68be16c919c4a662aed5d29cdca86e401b15 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 14 Dec 2012 16:55:41 -0600 Subject: [PATCH] Fix a case where c$resp$size is misrepresented. Addresses #730. That field is based on TCP sequence numbers and on seeing a SYN followed by a failed RST injection response, the initial sequence number tracked the value in the injection (most likely zero) instead of value in subsequent SYN response. This could make c$resp$size be set to large values when it's not really. Also removed some dead code paths. --- src/TCP.cc | 40 +++++++++++------- src/TCP.h | 7 +-- .../Baseline/core.tcp.rst-after-syn/.stdout | 3 ++ testing/btest/Traces/tcp/rst-inject-rae.trace | Bin 0 -> 943 bytes testing/btest/core/tcp/rst-after-syn.bro | 12 ++++++ 5 files changed, 44 insertions(+), 18 deletions(-) create mode 100644 testing/btest/Baseline/core.tcp.rst-after-syn/.stdout create mode 100644 testing/btest/Traces/tcp/rst-inject-rae.trace create mode 100644 testing/btest/core/tcp/rst-after-syn.bro diff --git a/src/TCP.cc b/src/TCP.cc index 555adf1b57..da977d8157 100644 --- a/src/TCP.cc +++ b/src/TCP.cc @@ -382,7 +382,7 @@ void TCP_Analyzer::ProcessFIN(double t, TCP_Endpoint* endpoint, endpoint->FIN_seq = base_seq - endpoint->StartSeq() + seq_len; } -bool TCP_Analyzer::ProcessRST(double t, TCP_Endpoint* endpoint, +void TCP_Analyzer::ProcessRST(double t, TCP_Endpoint* endpoint, const IP_Hdr* ip, uint32 base_seq, int len, int& seq_len) { @@ -406,11 +406,9 @@ bool TCP_Analyzer::ProcessRST(double t, TCP_Endpoint* endpoint, } PacketWithRST(); - - return true; } -int TCP_Analyzer::ProcessFlags(double t, +void TCP_Analyzer::ProcessFlags(double t, const IP_Hdr* ip, const struct tcphdr* tp, uint32 tcp_hdr_len, int len, int& seq_len, TCP_Endpoint* endpoint, TCP_Endpoint* peer, @@ -425,14 +423,11 @@ int TCP_Analyzer::ProcessFlags(double t, if ( flags.FIN() ) ProcessFIN(t, endpoint, seq_len, base_seq); - if ( flags.RST() && - ! ProcessRST(t, endpoint, ip, base_seq, len, seq_len) ) - return 0; + if ( flags.RST() ) + ProcessRST(t, endpoint, ip, base_seq, len, seq_len); if ( flags.ACK() ) ProcessACK(endpoint, peer, ack_seq, is_orig, flags); - - return 1; } void TCP_Analyzer::TransitionFromInactive(double t, TCP_Endpoint* endpoint, @@ -825,10 +820,27 @@ void TCP_Analyzer::UpdateClosedState(double t, TCP_Endpoint* endpoint, } } -void TCP_Analyzer::UpdateResetState(int len, TCP_Flags flags) +void TCP_Analyzer::UpdateResetState(int len, TCP_Flags flags, + TCP_Endpoint* endpoint, uint32 base_seq, + uint32 last_seq) { if ( flags.SYN() ) + { Weird("SYN_after_reset"); + + if ( endpoint->prev_state == TCP_ENDPOINT_INACTIVE ) + { + // Seq. numbers were initialized by a RST packet from this endpoint, + // but now that a SYN is seen from it, that could mean the earlier + // RST was spoofed/injected, so re-initialize. This mostly just + // helps prevent misrepresentations of payload sizes that are based + // on bad initial sequence values. + endpoint->InitStartSeq(base_seq); + endpoint->InitAckSeq(base_seq); + endpoint->InitLastSeq(last_seq); + } + } + if ( flags.FIN() ) Weird("FIN_after_reset"); @@ -871,7 +883,7 @@ void TCP_Analyzer::UpdateStateMachine(double t, break; case TCP_ENDPOINT_RESET: - UpdateResetState(len, flags); + UpdateResetState(len, flags, endpoint, base_seq, last_seq); break; } } @@ -996,10 +1008,8 @@ void TCP_Analyzer::DeliverPacket(int len, const u_char* data, bool is_orig, int seq_len = len; // length in terms of sequence space - if ( ! ProcessFlags(t, ip, tp, tcp_hdr_len, len, seq_len, - endpoint, peer, base_seq, ack_seq, - orig_addr, is_orig, flags) ) - return; + ProcessFlags(t, ip, tp, tcp_hdr_len, len, seq_len, endpoint, peer, base_seq, + ack_seq, orig_addr, is_orig, flags); uint32 last_seq = base_seq + seq_len; diff --git a/src/TCP.h b/src/TCP.h index c84202fcf6..635fda7960 100644 --- a/src/TCP.h +++ b/src/TCP.h @@ -135,13 +135,13 @@ protected: void ProcessFIN(double t, TCP_Endpoint* endpoint, int& seq_len, uint32 base_seq); - bool ProcessRST(double t, TCP_Endpoint* endpoint, const IP_Hdr* ip, + void ProcessRST(double t, TCP_Endpoint* endpoint, const IP_Hdr* ip, uint32 base_seq, int len, int& seq_len); void ProcessACK(TCP_Endpoint* endpoint, TCP_Endpoint* peer, uint32 ack_seq, int is_orig, TCP_Flags flags); - int ProcessFlags(double t, const IP_Hdr* ip, const struct tcphdr* tp, + void ProcessFlags(double t, const IP_Hdr* ip, const struct tcphdr* tp, uint32 tcp_hdr_len, int len, int& seq_len, TCP_Endpoint* endpoint, TCP_Endpoint* peer, uint32 base_seq, uint32 ack_seq, @@ -186,7 +186,8 @@ protected: int delta_last, TCP_Flags flags, int& do_close); - void UpdateResetState(int len, TCP_Flags flags); + void UpdateResetState(int len, TCP_Flags flags, TCP_Endpoint* endpoint, + uint32 base_seq, uint32 last_seq); void GeneratePacketEvent(TCP_Endpoint* endpoint, TCP_Endpoint* peer, uint32 base_seq, uint32 ack_seq, diff --git a/testing/btest/Baseline/core.tcp.rst-after-syn/.stdout b/testing/btest/Baseline/core.tcp.rst-after-syn/.stdout new file mode 100644 index 0000000000..25ed566cd0 --- /dev/null +++ b/testing/btest/Baseline/core.tcp.rst-after-syn/.stdout @@ -0,0 +1,3 @@ +[orig_h=1.2.0.2, orig_p=2527/tcp, resp_h=1.2.0.3, resp_p=6649/tcp] +orig:, [size=175, state=1, num_pkts=4, num_bytes_ip=395, flow_label=0] +resp:, [size=0, state=6, num_pkts=5, num_bytes_ip=236, flow_label=0] diff --git a/testing/btest/Traces/tcp/rst-inject-rae.trace b/testing/btest/Traces/tcp/rst-inject-rae.trace new file mode 100644 index 0000000000000000000000000000000000000000..7225cc0d35fedd0a020b4143f51bbc99f33a9ae8 GIT binary patch literal 943 zcmca|c+)~A1{MYw`2U}Qff2~j&&&6;4rFEU1F}IF3|JUM#kp$|IT&0S7#u`H9T@D` zRHGT07?^;Vne)En&)6=r8j#8jOb!e-MGOo~EUYn%%*+goj2v8G6MzIGBMTGAbmvDL z3^qVM2!jE#=^7x@_1H{{K_-K2ko?JcAEXLoL%fIs!?KqUO$-bMAeSa`;&Excj{}1t z+lgdkvl1fp1c7GNZV>wa|Lq2_OBVoL%7pGxkm*y>IT@UQ)`2h>AiLB=YAz@o6y!m! z1la%$hgzTwLJb1W0n65b{0#Izr%Mt@H^@Y$Y)*z~ARC0i0NF%esd-=%S+SZZ;T$mS zA*zYj-vP}@`0S~&?6Jqe^Z&hO`g&aIdtAzDWdyY91RpaV$2SOrg68-Nw4f0J2Fa(mzpbjDkgYgtn|!MiC>RRH;5Q6-(GiF|DCIe_}4RD`@D=U-+pDY?UkVSqx|x@ zQU+4Dri!m#&QbXMn8+8|?=fCa7=G&f>iF6t#o0T(=bpQZaObUcp*1|;Iy808b%<4L z{dlF}sPftW(R*KgeHU#cESi1#=GXmVB0L_(K!09}V#gO|4;X-9#`Xd|%s~DO0OrkH JO=##a008us*i--j literal 0 HcmV?d00001 diff --git a/testing/btest/core/tcp/rst-after-syn.bro b/testing/btest/core/tcp/rst-after-syn.bro new file mode 100644 index 0000000000..38976909d7 --- /dev/null +++ b/testing/btest/core/tcp/rst-after-syn.bro @@ -0,0 +1,12 @@ +# @TEST-EXEC: bro -b -r $TRACES/tcp/rst-inject-rae.trace %INPUT +# @TEST-EXEC: btest-diff .stdout + +# Mostly just checking that c$resp$size isn't huge due to the injected +# RST packet being used to initialize sequence number in TCP analyzer. + +event connection_state_remove(c: connection) + { + print c$id; + print "orig:", c$orig; + print "resp:", c$resp; + }