From 338d521003c66531b53dfe84755e2b3479d88d32 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Sun, 2 Mar 2014 13:52:32 -0800 Subject: [PATCH] Fixing removal of support analyzers, plus some tweaking and cleanup of CONNECT code. Removal of support analyzers was broken. The code now actually doesn't delete them immediately anymore but instead just flags them as disabled. They'll be destroyed with the parent analyzer later. Also includes a new leak tests exercising the CONNECT code. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # On branch topic/robin/http-connect # Changes to be committed: # modified: scripts/base/protocols/http/main.bro # modified: scripts/base/protocols/ssl/consts.bro # modified: src/analyzer/Analyzer.cc # modified: src/analyzer/Analyzer.h # modified: src/analyzer/protocol/http/HTTP.cc # new file: testing/btest/core/leaks/http-connect.bro # modified: testing/btest/scripts/base/protocols/http/http-connect.bro # # Untracked files: # .tags # changes.txt # conn.log # debug.log # diff # mpls-in-vlan.patch # newfile.pcap # packet_filter.log # reporter.log # src/PktSrc.cc.orig # weird.log # --- scripts/base/protocols/http/main.bro | 2 +- scripts/base/protocols/ssl/consts.bro | 1 + src/analyzer/Analyzer.cc | 114 ++++++++++-------- src/analyzer/Analyzer.h | 29 ++++- src/analyzer/protocol/http/HTTP.cc | 26 ++-- testing/btest/core/leaks/http-connect.bro | 14 +++ .../base/protocols/http/http-connect.bro | 7 +- 7 files changed, 128 insertions(+), 65 deletions(-) create mode 100644 testing/btest/core/leaks/http-connect.bro diff --git a/scripts/base/protocols/http/main.bro b/scripts/base/protocols/http/main.bro index 27257be2d6..ed20bc6586 100644 --- a/scripts/base/protocols/http/main.bro +++ b/scripts/base/protocols/http/main.bro @@ -218,7 +218,7 @@ event http_reply(c: connection, version: string, code: count, reason: string) &p c$http$info_code = code; c$http$info_msg = reason; } - + if ( c$http?$method && c$http$method == "CONNECT" && code == 200 ) { # Copy this conn_id and set the orig_p to zero because in the case of CONNECT proxies there will diff --git a/scripts/base/protocols/ssl/consts.bro b/scripts/base/protocols/ssl/consts.bro index 55289a7419..b81aebfbbb 100644 --- a/scripts/base/protocols/ssl/consts.bro +++ b/scripts/base/protocols/ssl/consts.bro @@ -86,6 +86,7 @@ export { [13172] = "next_protocol_negotiation", [13175] = "origin_bound_certificates", [13180] = "encrypted_client_certificates", + [30031] = "channel_id", [65281] = "renegotiation_info" } &default=function(i: count):string { return fmt("unknown-%d", i); }; diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc index 03734f1a22..63462c0049 100644 --- a/src/analyzer/Analyzer.cc +++ b/src/analyzer/Analyzer.cc @@ -209,11 +209,11 @@ void Analyzer::NextPacket(int len, const u_char* data, bool is_orig, int seq, if ( skip ) return; - // If we have support analyzers, we pass it to them. - if ( is_orig && orig_supporters ) - orig_supporters->NextPacket(len, data, is_orig, seq, ip, caplen); - else if ( ! is_orig && resp_supporters ) - resp_supporters->NextPacket(len, data, is_orig, seq, ip, caplen); + SupportAnalyzer* next_sibling = FirstSupportAnalyzer(is_orig); + + if ( next_sibling ) + next_sibling->NextPacket(len, data, is_orig, seq, ip, caplen); + else { try @@ -232,11 +232,11 @@ void Analyzer::NextStream(int len, const u_char* data, bool is_orig) if ( skip ) return; - // If we have support analyzers, we pass it to them. - if ( is_orig && orig_supporters ) - orig_supporters->NextStream(len, data, is_orig); - else if ( ! is_orig && resp_supporters ) - resp_supporters->NextStream(len, data, is_orig); + SupportAnalyzer* next_sibling = FirstSupportAnalyzer(is_orig); + + if ( next_sibling ) + next_sibling->NextStream(len, data, is_orig); + else { try @@ -255,11 +255,11 @@ void Analyzer::NextUndelivered(int seq, int len, bool is_orig) if ( skip ) return; - // If we have support analyzers, we pass it to them. - if ( is_orig && orig_supporters ) - orig_supporters->NextUndelivered(seq, len, is_orig); - else if ( ! is_orig && resp_supporters ) - resp_supporters->NextUndelivered(seq, len, is_orig); + SupportAnalyzer* next_sibling = FirstSupportAnalyzer(is_orig); + + if ( next_sibling ) + next_sibling->NextUndelivered(seq, len, is_orig); + else { try @@ -278,11 +278,10 @@ void Analyzer::NextEndOfData(bool is_orig) if ( skip ) return; - // If we have support analyzers, we pass it to them. - if ( is_orig && orig_supporters ) - orig_supporters->NextEndOfData(is_orig); - else if ( ! is_orig && resp_supporters ) - resp_supporters->NextEndOfData(is_orig); + SupportAnalyzer* next_sibling = FirstSupportAnalyzer(is_orig); + + if ( next_sibling ) + next_sibling->NextEndOfData(is_orig); else EndOfData(is_orig); } @@ -558,31 +557,17 @@ void Analyzer::AddSupportAnalyzer(SupportAnalyzer* analyzer) void Analyzer::RemoveSupportAnalyzer(SupportAnalyzer* analyzer) { - SupportAnalyzer** head = - analyzer->IsOrig() ? &orig_supporters : &resp_supporters; - - SupportAnalyzer* prev = 0; - SupportAnalyzer* s; - for ( s = *head; s && s != analyzer; prev = s, s = s->sibling ) - ; - - if ( ! s ) - return; - - if ( prev ) - prev->sibling = s->sibling; - else - *head = s->sibling; - - DBG_LOG(DBG_ANALYZER, "%s removed support %s", + DBG_LOG(DBG_ANALYZER, "%s disabled %s support analyzer %s", fmt_analyzer(this).c_str(), analyzer->IsOrig() ? "originator" : "responder", fmt_analyzer(analyzer).c_str()); - if ( ! analyzer->finished ) - analyzer->Done(); - - delete analyzer; + // We mark the analyzer as being removed here, which will prevent it + // from being used further. However, we don't actually delete it + // before the parent gets destroyed. While we woulc do that, it's a + // bit tricky to do at the right time and it doesn't seem worth the + // trouble. + analyzer->removing = true; return; } @@ -596,6 +581,19 @@ bool Analyzer::HasSupportAnalyzer(Tag tag, bool orig) return false; } +SupportAnalyzer* Analyzer::FirstSupportAnalyzer(bool orig) + { + SupportAnalyzer* sa = orig ? orig_supporters : resp_supporters; + + if ( ! sa ) + return 0; + + if ( ! sa->Removing() ) + return sa; + + return sa->Sibling(true); + } + void Analyzer::DeliverPacket(int len, const u_char* data, bool is_orig, int seq, const IP_Hdr* ip, int caplen) { @@ -782,16 +780,32 @@ void Analyzer::Weird(const char* name, const char* addl) conn->Weird(name, addl); } +SupportAnalyzer* SupportAnalyzer::Sibling(bool only_active) const + { + if ( ! only_active ) + return sibling; + + SupportAnalyzer* next = sibling; + while ( next && next->Removing() ) + next = next->sibling; + + return next; + } + void SupportAnalyzer::ForwardPacket(int len, const u_char* data, bool is_orig, int seq, const IP_Hdr* ip, int caplen) { // We do not call parent's method, as we're replacing the functionality. + if ( GetOutputHandler() ) GetOutputHandler()->DeliverPacket(len, data, is_orig, seq, ip, caplen); - else if ( sibling ) + + SupportAnalyzer* next_sibling = Sibling(true); + + if ( next_sibling ) // Pass to next in chain. - sibling->NextPacket(len, data, is_orig, seq, ip, caplen); + next_sibling->NextPacket(len, data, is_orig, seq, ip, caplen); else // Finished with preprocessing - now it's the parent's turn. Parent()->DeliverPacket(len, data, is_orig, seq, ip, caplen); @@ -800,12 +814,15 @@ void SupportAnalyzer::ForwardPacket(int len, const u_char* data, bool is_orig, void SupportAnalyzer::ForwardStream(int len, const u_char* data, bool is_orig) { // We do not call parent's method, as we're replacing the functionality. + if ( GetOutputHandler() ) GetOutputHandler()->DeliverStream(len, data, is_orig); - else if ( sibling ) + SupportAnalyzer* next_sibling = Sibling(true); + + if ( next_sibling ) // Pass to next in chain. - sibling->NextStream(len, data, is_orig); + next_sibling->NextStream(len, data, is_orig); else // Finished with preprocessing - now it's the parent's turn. Parent()->DeliverStream(len, data, is_orig); @@ -814,12 +831,15 @@ void SupportAnalyzer::ForwardStream(int len, const u_char* data, bool is_orig) void SupportAnalyzer::ForwardUndelivered(int seq, int len, bool is_orig) { // We do not call parent's method, as we're replacing the functionality. + if ( GetOutputHandler() ) GetOutputHandler()->Undelivered(seq, len, is_orig); - else if ( sibling ) + SupportAnalyzer* next_sibling = Sibling(true); + + if ( next_sibling ) // Pass to next in chain. - sibling->NextUndelivered(seq, len, is_orig); + next_sibling->NextUndelivered(seq, len, is_orig); else // Finished with preprocessing - now it's the parent's turn. Parent()->Undelivered(seq, len, is_orig); diff --git a/src/analyzer/Analyzer.h b/src/analyzer/Analyzer.h index f7ca07ca51..578020082b 100644 --- a/src/analyzer/Analyzer.h +++ b/src/analyzer/Analyzer.h @@ -587,7 +587,7 @@ protected: void RemoveTimer(Timer* t); /** - * Returnsn true if the analyzer has associated an SupportAnalyzer of a given type. + * Returns true if the analyzer has associated an SupportAnalyzer of a given type. * * @param tag The type to check for. * @@ -595,6 +595,14 @@ protected: */ bool HasSupportAnalyzer(Tag tag, bool orig); + /** + * Returns the first still active support analyzer for the given + * direction, or null if none. + * + * @param orig True if asking about the originator side. + */ + SupportAnalyzer* FirstSupportAnalyzer(bool orig); + /** * Adds a a new child analyzer with the option whether to intialize * it. This is an internal method. @@ -616,6 +624,12 @@ protected: */ void AppendNewChildren(); + /** + * Returns true if the analyzer has been flagged for removal and + * shouldn't be used otherwise anymore. + */ + bool Removing() const { return removing; } + private: // Internal method to eventually delete a child analyzer that's // already Done(). @@ -718,6 +732,14 @@ public: */ bool IsOrig() const { return orig; } + /** + * Returns the analyzer's next sibling, or null if none. + * + * only_active: If true, this will skip siblings that are still link + * but flagged for removal. + */ + SupportAnalyzer* Sibling(bool only_active = false) const; + /** * Passes packet input to the next sibling SupportAnalyzer if any, or * on to the associated main analyzer if none. If however there's an @@ -749,11 +771,6 @@ public: */ virtual void ForwardUndelivered(int seq, int len, bool orig); - /** - * Returns the analyzer next sibling, or null if none. - */ - SupportAnalyzer* Sibling() const { return sibling; } - protected: friend class Analyzer; diff --git a/src/analyzer/protocol/http/HTTP.cc b/src/analyzer/protocol/http/HTTP.cc index 93dbfbcb2e..0d49bb037f 100644 --- a/src/analyzer/protocol/http/HTTP.cc +++ b/src/analyzer/protocol/http/HTTP.cc @@ -950,7 +950,7 @@ void HTTP_Analyzer::DeliverStream(int len, const u_char* data, bool is_orig) if ( pia ) { - // There will be a PIA instance if this connection has been identified + // There will be a PIA instance if this connection has been identified // as a connect proxy. ForwardStream(len, data, is_orig); return; @@ -1066,14 +1066,10 @@ void HTTP_Analyzer::DeliverStream(int len, const u_char* data, bool is_orig) HTTP_Reply(); - InitHTTPMessage(content_line, - reply_message, is_orig, - ExpectReplyMessageBody(), - len); - if ( connect_request && reply_code == 200 ) { pia = new pia::PIA_TCP(Conn()); + if ( AddChildAnalyzer(pia) ) { pia->FirstPacket(true, 0); @@ -1084,13 +1080,22 @@ void HTTP_Analyzer::DeliverStream(int len, const u_char* data, bool is_orig) // need to be removed. RemoveSupportAnalyzer(content_line_orig); RemoveSupportAnalyzer(content_line_resp); + + return; } + else { + // Shouldn't really happen. + delete pia; pia = 0; } } + InitHTTPMessage(content_line, + reply_message, is_orig, + ExpectReplyMessageBody(), + len); } else { @@ -1422,6 +1427,12 @@ void HTTP_Analyzer::HTTP_Request() { ProtocolConfirmation(); + const char* method = (const char*) request_method->AsString()->Bytes(); + int method_len = request_method->AsString()->Len(); + + if ( strcasecmp_n(method_len, method, "CONNECT") == 0 ) + connect_request = true; + if ( http_request ) { val_list* vl = new val_list; @@ -1436,9 +1447,6 @@ void HTTP_Analyzer::HTTP_Request() // DEBUG_MSG("%.6f http_request\n", network_time); ConnectionEvent(http_request, vl); } - - if ( strcasecmp_n(request_method->AsString()->Len(), (const char*) (request_method->AsString()->Bytes()), "CONNECT") == 0 ) - connect_request = true; } void HTTP_Analyzer::HTTP_Reply() diff --git a/testing/btest/core/leaks/http-connect.bro b/testing/btest/core/leaks/http-connect.bro new file mode 100644 index 0000000000..e9a47d00a2 --- /dev/null +++ b/testing/btest/core/leaks/http-connect.bro @@ -0,0 +1,14 @@ +# Needs perftools support. +# +# @TEST-GROUP: leaks +# +# @TEST-REQUIRES: bro --help 2>&1 | grep -q mem-leaks +# +# @TEST-EXEC: HEAP_CHECK_DUMP_DIRECTORY=. HEAPCHECK=local btest-bg-run bro bro -b -m -r $TRACES/http/connect-with-smtp.trace %INPUT +# @TEST-EXEC: btest-bg-wait 15 + +@load base/protocols/conn +@load base/protocols/http +@load base/protocols/smtp +@load base/protocols/tunnels +@load base/frameworks/dpd diff --git a/testing/btest/scripts/base/protocols/http/http-connect.bro b/testing/btest/scripts/base/protocols/http/http-connect.bro index d8157e2c49..7073d88ac2 100644 --- a/testing/btest/scripts/base/protocols/http/http-connect.bro +++ b/testing/btest/scripts/base/protocols/http/http-connect.bro @@ -6,6 +6,9 @@ # @TEST-EXEC: btest-diff smtp.log # @TEST-EXEC: btest-diff tunnel.log -# The base analysis scripts are loaded by default. -#@load base/protocols/http +@load base/protocols/conn +@load base/protocols/http +@load base/protocols/smtp +@load base/protocols/tunnels +@load base/frameworks/dpd