]> git.ipfire.org Git - thirdparty/snort3.git/commitdiff
tom: new_http_inspect start line parsing updates
authorRuss Combs <rucombs@cisco.com>
Tue, 19 May 2015 15:10:48 +0000 (11:10 -0400)
committerRuss Combs <rucombs@cisco.com>
Tue, 19 May 2015 15:10:48 +0000 (11:10 -0400)
12 files changed:
ChangeLog
src/service_inspectors/nhttp_inspect/nhttp_enum.h
src/service_inspectors/nhttp_inspect/nhttp_msg_request.cc
src/service_inspectors/nhttp_inspect/nhttp_msg_request.h
src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc
src/service_inspectors/nhttp_inspect/nhttp_msg_status.cc
src/service_inspectors/nhttp_inspect/nhttp_msg_status.h
src/service_inspectors/nhttp_inspect/nhttp_splitter.cc
src/service_inspectors/nhttp_inspect/nhttp_splitter.h
src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc
src/service_inspectors/nhttp_inspect/nhttp_tables.cc
src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt

index 9949276641ab1086e3b4c39090eac98d7feecbe8..738de2e3cc7a4f61ed6f84dda671d0c073d3f05d 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,7 +1,8 @@
 Pending - build 153
 
+-- new_http_inspect parsing updates
 -- use buckets for user seglist
--- add -L u2
+-- add -A u2
 -- fix u2 to output data only packets
 -- added DAQs for socket, user, and file in extras
 -- changed -K to -L (log type)
index 743ecb8efcc7935f42cb62369d3585ceee4ea508..cc4cdf80f4018dfe3e8e4e43bca62e91d88ab92e 100644 (file)
@@ -125,6 +125,12 @@ enum Infraction
     INF_ENDLESS_HEADER,
     INF_LF_WITHOUT_CR,
     INF_NOT_HTTP,
+    INF_NO_URI,
+    INF_REQUEST_WS,
+    INF_REQUEST_TAB,
+    INF_STATUS_WS,
+    INF_STATUS_TAB,
+    INF_URI_SPACE,
 };
 
 // Formats for output from a header normalization function
@@ -193,11 +199,17 @@ enum EventSid
     EVENT_LOSS_OF_SYNC,
     EVENT_NOT_HTTP,
     EVENT_WS_BETWEEN_MSGS,
+    EVENT_URI_MISSING,
+    EVENT_CTRL_IN_REASON,
+    EVENT_IMPROPER_WS,
+    EVENT_BAD_VERS,
+    EVENT_UNKNOWN_VERS,
     EVENT_MAXVALUE
 };
 
 extern const int8_t as_hex[256];
 extern const bool token_char[256];
+extern const bool is_sp_tab[256];
 } // end namespace NHttpEnums
 
 #endif
index c2948c2660eb09a0420010c2a6eede2c2ac76f20..63990ea89417535f029a8fca6f7a5b9e40b0fbb4 100644 (file)
@@ -40,51 +40,48 @@ NHttpMsgRequest::NHttpMsgRequest(const uint8_t* buffer, const uint16_t buf_size,
 
 void NHttpMsgRequest::parse_start_line()
 {
-    // FIXIT-M this needs to be redesigned to parse a truncated request line and extract the method
-    // and URI. The current implementation just gives up if the " HTTP/X.Y" isn't in its proper
-    // place at the end of the line.
-
-    // There should be exactly two spaces. One following the method and one before "HTTP/".
-    // Additional spaces located within the URI are not allowed by RFC but we will tolerate it
-    // <method><SP><URI><SP>HTTP/X.Y
-    if (start_line.start[start_line.length-9] != ' ')
+    // Check the version field
+    if ((start_line.length < 10) || !is_sp_tab[start_line.start[start_line.length-9]] ||
+         memcmp(start_line.start + start_line.length - 8, "HTTP/", 5))
     {
-        // space before "HTTP" missing or in wrong place
         infractions += INF_BAD_REQ_LINE;
+        events.create_event(EVENT_NOT_HTTP);
         return;
     }
 
-    int32_t space;
-    for (space = 0; space < start_line.length-9; space++)
-    {
-        if (start_line.start[space] == ' ')
-            break;
-    }
-    if (space >= start_line.length-9)
-    {
-        // leading space or no space
-        infractions += INF_BAD_REQ_LINE;
-        return;
-    }
+    // The splitter guarantees there will be a non-whitespace at octet 1 and a whitespace within
+    // octets 2-81. The following algorithm uses those assumptions.
+
+    int32_t first_space; // first whitespace in request line
+    for (first_space = 1; !is_sp_tab[start_line.start[first_space]]; first_space++);
+
+    int32_t first_end; // last whitespace in first clump of whitespace
+    for (first_end = first_space+1; is_sp_tab[start_line.start[first_end]]; first_end++);
+    first_end--;
+
+    int32_t last_begin; // first whitespace in clump of whitespace before version
+    for (last_begin = start_line.length - 10; is_sp_tab[start_line.start[last_begin]];
+        last_begin--);
+    last_begin++;
 
     method.start = start_line.start;
-    method.length = space;
-    derive_method_id();
-    uri = new NHttpUri(start_line.start + method.length + 1,
-        start_line.length - method.length - 10, method_id);
+    method.length = first_space;
+    method_id = (MethodId)str_to_code(method.start, method.length, method_list);
+
     version.start = start_line.start + (start_line.length - 8);
     version.length = 8;
-    assert (start_line.length == method.length + uri->get_uri().length + version.length + 2);
-}
+    derive_version_id();
 
-void NHttpMsgRequest::derive_method_id()
-{
-    if (method.length <= 0)
+    if (first_end < last_begin)
     {
-        method_id = METH__NOSOURCE;
-        return;
+        uri = new NHttpUri(start_line.start + first_end + 1, last_begin - first_end - 1,
+            method_id);
+    }
+    else
+    {
+        infractions += INF_NO_URI;
+        events.create_event(EVENT_URI_MISSING);
     }
-    method_id = (MethodId)str_to_code(method.start, method.length, method_list);
 }
 
 const Field& NHttpMsgRequest::get_uri()
@@ -107,6 +104,43 @@ const Field& NHttpMsgRequest::get_uri_norm_legacy()
 
 void NHttpMsgRequest::gen_events()
 {
+    if (infractions && INF_BAD_REQ_LINE)
+        return;
+
+    if ((start_line.start[method.length] == '\t') ||
+        (start_line.start[start_line.length - 9] == '\t'))
+    {
+        infractions += INF_REQUEST_TAB;
+        events.create_event(EVENT_APACHE_WS);
+    }
+
+    for (int k = method.length + 1; k < start_line.length - 9; k++)
+    {
+        if (is_sp_tab[start_line.start[k]])
+        {
+            if (uri && (uri->get_uri().start <= start_line.start + k) &&
+                       (start_line.start + k < uri->get_uri().start + uri->get_uri().length))
+            {
+                // inside the URI
+                if (start_line.start[k] == ' ')
+                {
+                    infractions += INF_URI_SPACE;
+                    events.create_event(EVENT_UNESCAPED_SPACE_URI);
+                }
+            }
+            else
+            {
+                infractions += INF_REQUEST_WS;
+                events.create_event(EVENT_IMPROPER_WS);
+                if (start_line.start[k] == '\t')
+                {
+                    infractions += INF_REQUEST_TAB;
+                    events.create_event(EVENT_APACHE_WS);
+                }
+            }
+        }
+    }
+
     if (method_id == METH__OTHER)
         events.create_event(EVENT_UNKNOWN_METHOD);
 
index 16e199dc6e5ef2038aedd498c7b740865a0b7e12..1c7c7c945196ce293fa0ed9522bed1cd5e4fa1f3 100644 (file)
@@ -47,7 +47,6 @@ private:
     static const StrCode method_list[];
 
     void parse_start_line() override;
-    void derive_method_id();
 
     Field method;
     NHttpUri* uri = nullptr;
index a728e384276b5417ccbd7b66d8b5aab313d74f71..7ec126618448e77a8b0ecc52a9db26f8a55059c5 100644 (file)
@@ -31,27 +31,15 @@ void NHttpMsgStart::analyze()
     start_line.start = msg_text.start;
     start_line.length = msg_text.length;
     parse_start_line();
-    derive_version_id();
 }
 
 void NHttpMsgStart::derive_version_id()
 {
-    if (version.length <= 0)
-    {
-        version_id = VERS__NOSOURCE;
-        return;
-    }
-    if (version.length != 8)
-    {
-        version_id = VERS__PROBLEMATIC;
-        infractions += INF_BAD_VERSION;
-        return;
-    }
-
-    if (memcmp(version.start, "HTTP/", 5) || (version.start[6] != '.'))
+    if (version.start[6] != '.')
     {
         version_id = VERS__PROBLEMATIC;
         infractions += INF_BAD_VERSION;
+        events.create_event(EVENT_BAD_VERS);
     }
     else if ((version.start[5] == '1') && (version.start[7] == '1'))
     {
@@ -70,11 +58,13 @@ void NHttpMsgStart::derive_version_id()
     {
         version_id = VERS__OTHER;
         infractions += INF_UNKNOWN_VERSION;
+        events.create_event(EVENT_UNKNOWN_VERS);
     }
     else
     {
         version_id = VERS__PROBLEMATIC;
         infractions += INF_BAD_VERSION;
+        events.create_event(EVENT_BAD_VERS);
     }
 }
 
index 5487ec96398769b1946c27d18a55080383e569ce..ff4ad6425edbf01a6a8b2a6b10137c357be5328f 100644 (file)
@@ -38,61 +38,59 @@ NHttpMsgStatus::NHttpMsgStatus(const uint8_t* buffer, const uint16_t buf_size,
     transaction->set_status(this);
 }
 
-// All the header processing that is done for every message (i.e. not just-in-time) is done here.
-void NHttpMsgStatus::analyze()
-{
-    NHttpMsgStart::analyze();
-    derive_status_code_num();
-}
-
 void NHttpMsgStatus::parse_start_line()
 {
-    // FIXIT-M need to be able to parse a truncated status line and extract version and status
-    // code.
+    // Splitter guarantees line begins with "HTTP/"
 
-    // Eventually we may need to cater to certain format errors, but for now exact match or treat
-    // as error. HTTP/X.Y<SP>###<SP><text>
-    if ((start_line.length < 13) || (start_line.start[8] != ' ') || (start_line.start[12] != ' '))
+    if ((start_line.length < 12) || !is_sp_tab[start_line.start[8]])
     {
         infractions += INF_BAD_STAT_LINE;
+        events.create_event(EVENT_NOT_HTTP);
         return;
     }
-    version.start = start_line.start;
-    version.length = 8;
-    status_code.start = start_line.start + 9;
-    status_code.length = 3;
-    reason_phrase.start = start_line.start + 13;
-    reason_phrase.length = start_line.length - 13;
-    for (int32_t k = 0; k < reason_phrase.length; k++)
+
+    int32_t first_end; // last whitespace in first clump of whitespace
+    for (first_end = 9; is_sp_tab[start_line.start[first_end]] && (first_end < start_line.length);
+        first_end++);
+    first_end--;
+
+    if (start_line.length < first_end + 4)
     {
-        if ((reason_phrase.start[k] <= 31) || (reason_phrase.start[k] >= 127))
-        {
-            // Illegal character in reason phrase
-            infractions += INF_BAD_PHRASE;
-            break;
-        }
+        infractions += INF_BAD_STAT_LINE;
+        events.create_event(EVENT_NOT_HTTP);
+        return;
     }
-    assert (start_line.length == version.length + status_code.length + reason_phrase.length + 2);
-}
 
-void NHttpMsgStatus::derive_status_code_num()
-{
-    if (status_code.length <= 0)
+    if ((start_line.length > first_end + 4) && !is_sp_tab[start_line.start[first_end + 4]])
     {
-        status_code_num = STAT_NOSOURCE;
+        infractions += INF_BAD_STAT_LINE;
+        events.create_event(EVENT_NOT_HTTP);
         return;
     }
-    if (status_code.length != 3)
+
+    version.start = start_line.start;
+    version.length = 8;
+    derive_version_id();
+
+    status_code.start = start_line.start + first_end + 1;
+    status_code.length = 3;
+    derive_status_code_num();
+
+    if (start_line.length > first_end + 5)
     {
-        status_code_num = STAT_PROBLEMATIC;
-        return;
+        reason_phrase.start = start_line.start + first_end + 5;
+        reason_phrase.length = start_line.length - first_end - 5;
     }
+}
 
+void NHttpMsgStatus::derive_status_code_num()
+{
     if ((status_code.start[0] < '0') || (status_code.start[0] > '9') || (status_code.start[1] <
         '0') || (status_code.start[1] > '9') ||
         (status_code.start[2] < '0') || (status_code.start[2] > '9'))
     {
         infractions += INF_BAD_STAT_CODE;
+        events.create_event(EVENT_INVALID_STATCODE);
         status_code_num = STAT_PROBLEMATIC;
         return;
     }
@@ -101,10 +99,50 @@ void NHttpMsgStatus::derive_status_code_num()
     if ((status_code_num < 100) || (status_code_num > 599))
     {
         infractions += INF_BAD_STAT_CODE;
+        events.create_event(EVENT_INVALID_STATCODE);
     }
 }
 
-void NHttpMsgStatus::gen_events() { }
+void NHttpMsgStatus::gen_events()
+{
+    if (infractions && INF_BAD_STAT_LINE)
+        return;
+
+    if (status_code.start > start_line.start + 9)
+    {
+        infractions += INF_STATUS_WS;
+        events.create_event(EVENT_IMPROPER_WS);
+    }
+
+    for (int k = 8; k < status_code.start - start_line.start; k++)
+    {
+        if (start_line.start[k] == '\t')
+        {
+            infractions += INF_STATUS_TAB;
+            events.create_event(EVENT_APACHE_WS);
+        }
+    }
+
+    if (status_code.start - start_line.start + 3 < start_line.length)
+    {
+        if (status_code.start[3] == '\t')
+        {
+            infractions += INF_STATUS_TAB;
+            events.create_event(EVENT_APACHE_WS);
+        }
+    }
+
+    for (int k=0; k < reason_phrase.length; k++)
+    {
+        if ((reason_phrase.start[k] <= 31) || (reason_phrase.start[k] >= 127))
+        {
+            // Illegal character in reason phrase
+            infractions += INF_BAD_PHRASE;
+            events.create_event(EVENT_CTRL_IN_REASON);
+            break;
+        }
+    }
+}
 
 void NHttpMsgStatus::print_section(FILE* output)
 {
index b8f836e5b61b7e24c09e44a707d5a27c9006cfb2..70dc1d252902c855efc5f15e8045c1e1d0b7313c 100644 (file)
@@ -32,7 +32,6 @@ class NHttpMsgStatus : public NHttpMsgStart
 public:
     NHttpMsgStatus(const uint8_t* buffer, const uint16_t buf_size, NHttpFlowData* session_data_,
         NHttpEnums::SourceId source_id_, bool buf_owner);
-    void analyze() override;
     void print_section(FILE* output) override;
     void gen_events() override;
     void update_flow() override;
index f4a3c3a3ac33bdf9920c601aeb234531a87d84fc..8c4defda81eaaeea08142f9b25fe7c0d2c33c2d3 100644 (file)
@@ -31,9 +31,9 @@ ScanResult NHttpStartSplitter::split(const uint8_t* buffer, uint32_t length,
         // If we have seen nothing but white space so far ...
         if (num_crlf == octets_seen + k)
         {
-            if ((buffer[k] == 32) || ((buffer[k] >= 9) && (buffer[k] <= 13)))
+            if ((buffer[k] == ' ') || ((buffer[k] >= '\t') && (buffer[k] <= '\r')))
             {
-                if ((buffer[k] != 10) && (buffer[k] != 13))
+                if ((buffer[k] != '\n') && (buffer[k] != '\r'))
                 {
                     // tab, VT, FF, or space between messages
                     infractions += INF_WS_BETWEEN_MSGS;
index 13ce2846d1d1a8e35aab9c47d09992683ea408d4..51875776b0f14ec3ed2cd7a5d92e00181ff93cea 100644 (file)
@@ -40,7 +40,6 @@ public:
     uint32_t get_octets_seen() const { return octets_seen; }
     virtual uint32_t get_num_excess() const { return 0; }
     virtual uint32_t get_num_head_lines() const { return 0; }
-    virtual bool valid() const { return true; }
 
 protected:
     // number of octets processed by previous split() calls that returned NOTFOUND
@@ -56,7 +55,6 @@ public:
     NHttpEnums::ScanResult split(const uint8_t* buffer, uint32_t length,
         NHttpInfractions& infractions, NHttpEventGen& events) override;
     uint32_t get_num_excess() const override { return (num_flush > 0) ? num_crlf : 0; }
-    bool valid() const override { return validated; }
 
 protected:
     enum ValidationResult { V_GOOD, V_BAD, V_TBD };
index 5025aedb73ce75884a266bb69102b11afaf37a88..717dcdb716d3e00b1d8dca8d7cba332b35a7d950 100644 (file)
@@ -164,7 +164,11 @@ StreamSplitter::Status NHttpStreamSplitter::scan(Flow* flow, const uint8_t* data
             return StreamSplitter::FLUSH;
         }
         data = test_data;
-        assert(session_data->type_expected[source_id] != SEC_ABORT);
+        if (session_data->type_expected[source_id] == SEC_ABORT)
+        {
+            session_data = new NHttpFlowData;
+            flow->set_application_data(session_data);
+        }
     }
     else if (NHttpTestManager::use_test_output())
     {
@@ -405,13 +409,15 @@ bool NHttpStreamSplitter::finish(Flow* flow)
     assert(session_data != nullptr);
     session_data->tcp_close[source_id] = true;
     // If there is leftover data for which we returned PAF_SEARCH and never flushed, we need to set
-    // up to process because it is about to go to reassemble().
+    // up to process because it is about to go to reassemble(). But we don't support partial start
+    // lines.
     if ((session_data->section_type[source_id] == SEC__NOTCOMPUTE) &&
         (session_data->splitter[source_id] != nullptr) &&
         (session_data->splitter[source_id]->get_octets_seen() > 0) &&
         (session_data->type_expected[source_id] != SEC_ABORT))
     {
-        if (!session_data->splitter[source_id]->valid())
+        if ((session_data->type_expected[source_id] == SEC_REQUEST) ||
+            (session_data->type_expected[source_id] == SEC_STATUS))
         {
             return false;
         }
index a308b8d736324959cb0e14fec6cf3d3a6bc6124c..e29d5121fa41a3be7463009ce7a85abaa8f01d2c 100644 (file)
@@ -303,7 +303,11 @@ const RuleMap NHttpModule::nhttp_events[] =
     { EVENT_LOSS_OF_SYNC,               "HTTP misformatted or not really HTTP" },
     { EVENT_NOT_HTTP,                   "Input apparently not HTTP" },
     { EVENT_WS_BETWEEN_MSGS,            "White space before or between messages" },
-
+    { EVENT_URI_MISSING,                "Request message without URI" },
+    { EVENT_CTRL_IN_REASON,             "Control character in reason phrase" },
+    { EVENT_IMPROPER_WS,                "Illegal extra whitespace in start line" },
+    { EVENT_BAD_VERS,                   "Corrupted HTTP version" },
+    { EVENT_UNKNOWN_VERS,               "Unknown HTTP version" },
     { 0, nullptr }
 };
 
@@ -334,7 +338,6 @@ const int8_t NHttpEnums::as_hex[256] =
     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1
 };
 
-
 const bool NHttpEnums::token_char[256] =
 {
     false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
@@ -362,3 +365,30 @@ const bool NHttpEnums::token_char[256] =
     false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false
 };
 
+const bool NHttpEnums::is_sp_tab[256] =
+{
+    false, false, false, false, false, false, false, false, false,  true, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+     true, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false,
+    false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false
+};
+
index 823ff0a637e903a32559d64587061c5b29a07cd3..e318d4fa2a4ea59c9e8077c0a38315d883e49a04 100644 (file)
@@ -280,6 +280,41 @@ HTTP/1.0 401 illegal nontext character in reason\xFFphrase delete\r\n\r\n
 @response
 HTTP/1.0 401 \x08illegal nontext character in reason phrase backspace\r\n\r\n
 
+@2029
+@break
+@response
+HTTP/1.0  310 Excessive white space\r\n\r\n
+
+@2030
+@break
+@response
+HTTP/1.0     310 Excessive white space\r\n\r\n
+
+@2031
+@break
+@response
+HTTP/1.1\t310 Tab instead of space\r\n\r\n
+
+@2032
+@break
+@response
+HTTP/1.0 310\tTab instead of space\r\n\r\n
+
+@2033
+@break
+@response
+HTTP/1.0    \t310 Excessive white space and tab\r\n\r\n
+
+@2034
+@break
+@response
+HTTP/1.0 700 \tTab is in reason phrase\r\n\r\n
+
+@2035
+@break
+@response
+HTTP/1.0\t\t\t310 Excessive white space consisting of tabs\r\n\r\n
+
 # ***********************************************************************************************
 # Valid request start lines
 @3001
@@ -392,6 +427,11 @@ GET HtTpS://1.2.3.4.5.a:6/abcdef/ghijklmnop/qrstuvwxyz/?thequery?fieldcontinues?
 @request
 \t\t\t\tBIND /1234567890?abcdef HTTP/1.1\r\n\r\n
 
+@3023
+@break
+@request
+GET12345678901234567890123456789012345678901234567890123456789012345678901234567 http://hostname.com/?# HTTP/1.1\r\n\r\n
+
 # ***********************************************************************************************
 # Invalid request start lines
 @4001
@@ -474,6 +514,36 @@ GE;T http://hostname.com/?# HTTP/1.1\r\n\r\n
 @request
 GET\xAA http://hostname.com/?# HTTP/1.1\r\n\r\n
 
+@4016
+@break
+@request
+GET   /white/space/abuse HTTP/1.1\r\n\r\n
+
+@4017
+@break
+@request
+GET /white/space/abuse  HTTP/1.1\r\n\r\n
+
+@4018
+@break
+@request
+GET\t/white/space/abuse HTTP/1.1\r\n\r\n
+
+@4019
+@break
+@request
+GET /white/space/abuse\tHTTP/1.1\r\n\r\n
+
+@4020
+@break
+@request
+GET \t /white/space/abuse HTTP/1.1\r\n\r\n
+
+@4021
+@break
+@request
+GET /white/space/abuse       \tHTTP/1.1\r\n\r\n
+
 # ***********************************************************************************************
 # Valid headers without body
 @5001