From: Tom Peters (thopeter) <thopeter@cisco.com>
Date: Fri, 18 Aug 2017 19:22:27 +0000 (-0400)
Subject: Merge pull request #993 in SNORT/snort3 from nhttp85 to master
X-Git-Tag: 3.0.0-240~63
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ea54c3fc56b85b73c6349271702f0eb679bb4653;p=thirdparty%2Fsnort3.git

Merge pull request #993 in SNORT/snort3 from nhttp85 to master

Squashed commit of the following:

commit f9f1973bb5bd6e38b5b2b974bb202396f048cb6f
Author: Tom Peters <thopeter@cisco.com>
Date:   Tue Aug 1 14:11:54 2017 -0400

    http_inspect: create message sections with body data that has been dechunked and unzipped but not otherwise nortmalized.
---

diff --git a/src/detection/detection_engine.cc b/src/detection/detection_engine.cc
index a02e36f8b..ee0107cc0 100644
--- a/src/detection/detection_engine.cc
+++ b/src/detection/detection_engine.cc
@@ -52,7 +52,6 @@
 #include "regex_offload.h"
 
 static THREAD_LOCAL RegexOffload* offloader = nullptr;
-static THREAD_LOCAL DataPointer next_file_data = { nullptr, 0 };
 static THREAD_LOCAL uint64_t context_num = 0;
 
 //--------------------------------------------------------------------------
@@ -68,8 +67,7 @@ void DetectionEngine::thread_term()
 DetectionEngine::DetectionEngine()
 {
     context = Snort::get_switcher()->interrupt();
-    context->file_data = next_file_data;
-    next_file_data = { nullptr, 0 };
+    context->file_data = { nullptr, 0 };
     reset();
 }
 
@@ -81,7 +79,6 @@ DetectionEngine::~DetectionEngine()
     if ( context == sw->get_context() )
     {
         sw->complete();
-        next_file_data = { nullptr, 0 };
     }
 }
 
@@ -152,9 +149,6 @@ DataBuffer& DetectionEngine::get_alt_buffer(Packet* p)
     return p->context->alt_data;
 }
 
-void DetectionEngine::set_next_file_data(const DataPointer& dp)
-{ next_file_data = dp; }
-
 void DetectionEngine::set_file_data(const DataPointer& dp)
 { Snort::get_switcher()->get_context()->file_data = dp; }
 
diff --git a/src/detection/detection_engine.h b/src/detection/detection_engine.h
index 0531947e4..3627a139c 100644
--- a/src/detection/detection_engine.h
+++ b/src/detection/detection_engine.h
@@ -64,9 +64,6 @@ public:
     static void set_encode_packet(Packet*);
     static Packet* get_encode_packet();
 
-    static void set_next_file_data(const DataPointer&);
-    static void get_next_file_data(DataPointer&);
-
     static void set_file_data(const DataPointer&);
     static void get_file_data(DataPointer&);
 
@@ -108,12 +105,6 @@ private:
     IpsContext* context;
 };
 
-static inline void set_next_file_data(const uint8_t* p, unsigned n)
-{
-    DataPointer dp { p, n };
-    DetectionEngine::set_next_file_data(dp);
-}
-
 static inline void set_file_data(const uint8_t* p, unsigned n)
 {
     DataPointer dp { p, n };
diff --git a/src/service_inspectors/http_inspect/http_flow_data.h b/src/service_inspectors/http_inspect/http_flow_data.h
index 3c02091ca..b1ecfe9b6 100644
--- a/src/service_inspectors/http_inspect/http_flow_data.h
+++ b/src/service_inspectors/http_inspect/http_flow_data.h
@@ -95,6 +95,7 @@ private:
                                                 HttpEnums::SEC__NOT_COMPUTE };
     bool tcp_close[2] = { false, false };
     int32_t num_head_lines[2] = { HttpEnums::STAT_NOT_PRESENT, HttpEnums::STAT_NOT_PRESENT };
+    bool zero_byte_workaround[2];
 
     // Infractions and events are associated with a specific message and are stored in the
     // transaction for that message. But StreamSplitter splits the start line before there is
diff --git a/src/service_inspectors/http_inspect/http_inspect.cc b/src/service_inspectors/http_inspect/http_inspect.cc
index dbca594e3..d88175545 100644
--- a/src/service_inspectors/http_inspect/http_inspect.cc
+++ b/src/service_inspectors/http_inspect/http_inspect.cc
@@ -23,8 +23,8 @@
 
 #include "http_inspect.h"
 
-#include "protocols/packet.h"
-
+#include "detection/detection_engine.h"
+#include "http_js_norm.h"
 #include "http_msg_body.h"
 #include "http_msg_body_chunk.h"
 #include "http_msg_body_cl.h"
@@ -33,7 +33,8 @@
 #include "http_msg_request.h"
 #include "http_msg_status.h"
 #include "http_msg_trailer.h"
-#include "http_js_norm.h"
+#include "http_test_manager.h"
+#include "protocols/packet.h"
 
 using namespace HttpEnums;
 
@@ -149,7 +150,37 @@ bool HttpInspect::get_fp_buf(InspectionBuffer::Type ibt, Packet* p, InspectionBu
     return get_buf(ibt, p, b);
 }
 
-const Field& HttpInspect::process(const uint8_t* data, const uint16_t dsize, Flow* const flow,
+void HttpInspect::eval(Packet* p)
+{
+    const SourceId source_id = p->is_from_client() ? SRC_CLIENT : SRC_SERVER;
+
+    HttpFlowData* session_data =
+        (HttpFlowData*)p->flow->get_flow_data(HttpFlowData::inspector_id);
+
+    // FIXIT-H Workaround for unexpected eval() calls
+    if (session_data->section_type[source_id] == SEC__NOT_COMPUTE)
+        return;
+
+    const int remove_workaround = session_data->zero_byte_workaround[source_id] ? 1 : 0;
+    if (!process(p->data, p->dsize - remove_workaround, p->flow, source_id, true))
+    {
+        DetectionEngine::disable_content(p);
+        clear(p);
+    }
+#ifdef REG_TEST
+    else
+    {
+        if (HttpTestManager::use_test_output())
+        {
+            fprintf(HttpTestManager::get_output_file(), "Sent to detection %u octets\n\n",
+                p->dsize);
+            fflush(HttpTestManager::get_output_file());
+        }
+    }
+#endif
+}
+
+bool HttpInspect::process(const uint8_t* data, const uint16_t dsize, Flow* const flow,
     SourceId source_id, bool buf_owner) const
 {
     HttpFlowData* session_data = (HttpFlowData*)flow->get_flow_data(HttpFlowData::inspector_id);
@@ -193,7 +224,7 @@ const Field& HttpInspect::process(const uint8_t* data, const uint16_t dsize, Flo
         {
             delete[] data;
         }
-        return Field::FIELD_NULL;
+        return false;
     }
 
     session_data->latest_section->analyze();
@@ -215,32 +246,23 @@ const Field& HttpInspect::process(const uint8_t* data, const uint16_t dsize, Flo
 #endif
 
     session_data->latest_section->publish();
-    return session_data->latest_section->get_detect_buf();
+    return session_data->latest_section->detection_required();
 }
 
 void HttpInspect::clear(Packet* p)
 {
-    HttpFlowData* session_data =
+    HttpFlowData* const session_data =
         (HttpFlowData*)p->flow->get_flow_data(HttpFlowData::inspector_id);
 
     if (session_data == nullptr)
         return;
     session_data->latest_section = nullptr;
 
-    assert((p->is_from_client()) || (p->is_from_server()));
-    assert(!((p->is_from_client()) && (p->is_from_server())));
-    SourceId source_id = (p->is_from_client()) ? SRC_CLIENT : SRC_SERVER;
+    const SourceId source_id = (p->is_from_client()) ? SRC_CLIENT : SRC_SERVER;
 
     if (session_data->transaction[source_id] == nullptr)
         return;
 
-    clear(session_data, source_id);
-}
-
-void HttpInspect::clear(HttpFlowData* session_data, SourceId source_id)
-{
-    session_data->latest_section = nullptr;
-
     // If current transaction is complete then we are done with it and should reclaim the space
     if ((source_id == SRC_SERVER) && (session_data->type_expected[SRC_SERVER] == SEC_STATUS) &&
          session_data->transaction[SRC_SERVER]->final_response())
diff --git a/src/service_inspectors/http_inspect/http_inspect.h b/src/service_inspectors/http_inspect/http_inspect.h
index ed3a0b246..9484c6fbd 100644
--- a/src/service_inspectors/http_inspect/http_inspect.h
+++ b/src/service_inspectors/http_inspect/http_inspect.h
@@ -47,7 +47,7 @@ public:
     bool get_fp_buf(InspectionBuffer::Type ibt, Packet* p, InspectionBuffer& b) override;
     bool configure(SnortConfig*) override;
     void show(SnortConfig*) override { LogMessage("HttpInspect\n"); }
-    void eval(Packet*) override { }
+    void eval(Packet* p) override;
     void clear(Packet* p) override;
     void tinit() override { }
     void tterm() override { }
@@ -61,9 +61,8 @@ private:
     friend HttpApi;
     friend HttpStreamSplitter;
 
-    const Field& process(const uint8_t* data, const uint16_t dsize, Flow* const flow,
+    bool process(const uint8_t* data, const uint16_t dsize, Flow* const flow,
         HttpEnums::SourceId source_id_, bool buf_owner) const;
-    void clear(HttpFlowData* session_data, HttpEnums::SourceId source_id);
     static HttpEnums::SourceId get_latest_src(const Packet* p);
 
     const HttpParaList* const params;
diff --git a/src/service_inspectors/http_inspect/http_msg_body.cc b/src/service_inspectors/http_inspect/http_msg_body.cc
index aa16929fd..44359bfd3 100644
--- a/src/service_inspectors/http_inspect/http_msg_body.cc
+++ b/src/service_inspectors/http_inspect/http_msg_body.cc
@@ -54,7 +54,7 @@ void HttpMsgBody::analyze()
             js_norm_body.length() : session_data->detect_depth_remaining[source_id];
         detect_data.set(detect_length, js_norm_body.start());
         session_data->detect_depth_remaining[source_id] -= detect_length;
-        set_next_file_data(
+        set_file_data(
             const_cast<uint8_t*>(detect_data.start()), (unsigned)detect_data.length());
     }
 
@@ -66,6 +66,11 @@ void HttpMsgBody::analyze()
     body_octets += msg_text.length();
 }
 
+bool HttpMsgBody::detection_required() const
+{
+    return (detect_data.length() > 0) || (get_inspection_section() == IS_DETECTION);
+}
+
 void HttpMsgBody::do_utf_decoding(const Field& input, Field& output)
 {
     if ((source_id == SRC_CLIENT) || (session_data->utf_state == nullptr) || (input.length() == 0))
diff --git a/src/service_inspectors/http_inspect/http_msg_body.h b/src/service_inspectors/http_inspect/http_msg_body.h
index e37b0943e..87fbb66f0 100644
--- a/src/service_inspectors/http_inspect/http_msg_body.h
+++ b/src/service_inspectors/http_inspect/http_msg_body.h
@@ -32,9 +32,9 @@ class HttpMsgBody : public HttpMsgSection
 public:
     virtual ~HttpMsgBody() = default;
     void analyze() override;
-    const Field& get_detect_buf() const override { return detect_data; }
     HttpEnums::InspectSection get_inspection_section() const override
         { return detection_section ? HttpEnums::IS_DETECTION : HttpEnums::IS_BODY; }
+    bool detection_required() const override;
     const Field& get_classic_client_body();
     static void fd_event_callback(void* context, int event);
 
diff --git a/src/service_inspectors/http_inspect/http_msg_section.cc b/src/service_inspectors/http_inspect/http_msg_section.cc
index 31eccffd2..7a552292f 100644
--- a/src/service_inspectors/http_inspect/http_msg_section.cc
+++ b/src/service_inspectors/http_inspect/http_msg_section.cc
@@ -51,6 +51,12 @@ HttpMsgSection::HttpMsgSection(const uint8_t* buffer, const uint16_t buf_size,
     assert((source_id == SRC_CLIENT) || (source_id == SRC_SERVER));
 }
 
+bool HttpMsgSection::detection_required() const
+{
+    return ((msg_text.length() > 0) && (get_inspection_section() != IS_NONE)) ||
+           (get_inspection_section() == IS_DETECTION);
+}
+
 void HttpMsgSection::add_infraction(int infraction)
 {
     *transaction->get_infractions(source_id) += infraction;
@@ -200,12 +206,12 @@ const Field& HttpMsgSection::get_classic_buffer(unsigned id, uint64_t sub_id, ui
     case HTTP_BUFFER_RAW_REQUEST:
       {
         HttpMsgRequest* request = transaction->get_request();
-        return (request != nullptr) ? request->get_detect_buf() : Field::FIELD_NULL;
+        return (request != nullptr) ? request->msg_text : Field::FIELD_NULL;
       }
     case HTTP_BUFFER_RAW_STATUS:
       {
         HttpMsgStatus* status = transaction->get_status();
-        return (status != nullptr) ? status->get_detect_buf() : Field::FIELD_NULL;
+        return (status != nullptr) ? status->msg_text : Field::FIELD_NULL;
       }
     case HTTP_BUFFER_RAW_TRAILER:
       {
diff --git a/src/service_inspectors/http_inspect/http_msg_section.h b/src/service_inspectors/http_inspect/http_msg_section.h
index 173ebd485..f88391ab5 100644
--- a/src/service_inspectors/http_inspect/http_msg_section.h
+++ b/src/service_inspectors/http_inspect/http_msg_section.h
@@ -37,7 +37,8 @@ public:
     virtual ~HttpMsgSection() = default;
     virtual HttpEnums::InspectSection get_inspection_section() const
         { return HttpEnums::IS_NONE; }
-    HttpEnums::SourceId get_source_id() { return source_id; }
+    virtual bool detection_required() const;
+    HttpEnums::SourceId get_source_id() const { return source_id; }
 
     // Minimum necessary processing for every message
     virtual void analyze() = 0;
@@ -53,12 +54,9 @@ public:
 
     const Field& get_classic_buffer(unsigned id, uint64_t sub_id, uint64_t form);
 
-    // Provide buffer to be sent to detection
-    virtual const Field& get_detect_buf() const { return msg_text; }
-
     HttpEnums::MethodId get_method_id() const { return method_id; }
 
-    int32_t get_status_code_num() { return status_code_num; }
+    int32_t get_status_code_num() const { return status_code_num; }
 
     // Publish an inspection event for other modules to consume.
     virtual void publish() { }
diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
index 1a1297ac6..8119640bf 100644
--- a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
+++ b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
@@ -354,46 +354,24 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, un
                 (!session_data->strict_length[source_id] &&
                 (total <= session_data->octets_expected[source_id])));
         running_total = 0;
-        const Field& send_to_detection = my_inspector->process(buffer,
-            session_data->section_offset[source_id] - session_data->num_excess[source_id], flow,
-            source_id, true);
-        // delete[] not necessary because HttpMsgSection is now responsible.
-        buffer = nullptr;
-
-        session_data->section_offset[source_id] = 0;
+        const uint16_t buf_size =
+            session_data->section_offset[source_id] - session_data->num_excess[source_id];
 
-        // The detection section of a message is the first body section, unless there is no body
-        // section in which case it is the headers. The detection section is always returned to the
-        // framework and forwarded to detection even if it is empty. Other body sections and the
-        // trailer section are only forwarded if nonempty. The start line section and header
-        // sections other than the detection section are never forwarded.
-        if (((send_to_detection.length() > 0) &&
-                (session_data->latest_section->get_inspection_section() != IS_NONE)) ||
-            ((send_to_detection.length() == 0) &&
-                (session_data->latest_section->get_inspection_section() == IS_DETECTION)))
+        // FIXIT-M kludge until we work out issues with returning an empty buffer
+        http_buf.data = buffer;
+        if (buf_size > 0)
         {
-            // FIXIT-M kludge until we work out issues with returning an empty buffer
-            if (send_to_detection.length() > 0)
-            {
-                http_buf.data = send_to_detection.start();
-                http_buf.length = send_to_detection.length();
-            }
-            else
-            {
-                http_buf.data = (const uint8_t*)"";
-                http_buf.length = 1;
-            }
-#ifdef REG_TEST
-            if (HttpTestManager::use_test_output())
-            {
-                fprintf(HttpTestManager::get_output_file(), "Sent to detection %u octets\n\n",
-                    http_buf.length);
-                fflush(HttpTestManager::get_output_file());
-            }
-#endif
-            return http_buf;
+            http_buf.length = buf_size;
+            session_data->zero_byte_workaround[source_id] = false;
+        }
+        else
+        {
+            buffer[0] = '\0';
+            http_buf.length = 1;
+            session_data->zero_byte_workaround[source_id] = true;
         }
-        my_inspector->clear(session_data, source_id);
+        buffer = nullptr;
+        session_data->section_offset[source_id] = 0;
     }
     return http_buf;
 }
diff --git a/src/service_inspectors/http_inspect/http_tables.cc b/src/service_inspectors/http_inspect/http_tables.cc
index 8afed9a6f..cd3e5089e 100644
--- a/src/service_inspectors/http_inspect/http_tables.cc
+++ b/src/service_inspectors/http_inspect/http_tables.cc
@@ -283,7 +283,7 @@ const RuleMap HttpModule::http_events[] =
     { EVENT_DOUBLE_DECODE,              "double decoding attack" },
     { EVENT_U_ENCODE,                   "u encoding" },
     { EVENT_BARE_BYTE,                  "bare byte unicode encoding" },
-    { EVENT_OBSOLETE_BASE_36,           "obsolete event--should not appear" },
+    { EVENT_OBSOLETE_BASE_36,           "obsolete event--deleted" },
     { EVENT_UTF_8,                      "UTF-8 encoding" },
     { EVENT_CODE_POINT_IN_URI,          "unicode map code point encoding in URI" },
     { EVENT_MULTI_SLASH,                "multi_slash encoding" },
@@ -300,7 +300,7 @@ const RuleMap HttpModule::http_events[] =
     { EVENT_LONG_HDR,                   "long header" },
     { EVENT_MAX_HEADERS,                "max header fields" },
     { EVENT_MULTIPLE_CONTLEN,           "multiple content length" },
-    { EVENT_OBSOLETE_CHUNK_SIZE_MISMATCH, "obsolete event--should not appear" },
+    { EVENT_OBSOLETE_CHUNK_SIZE_MISMATCH, "obsolete event--deleted" },
     { EVENT_INVALID_TRUEIP,             "invalid IP in true-client-IP/XFF header" },
     { EVENT_MULTIPLE_HOST_HDRS,         "multiple host hdrs detected" },
     { EVENT_LONG_HOSTNAME,              "hostname exceeds 255 characters" },