Merge pull request #1791 in SNORT/snort3 from ~THOPETER/snort3:http2_cleanup to master

Squashed commit of the following:

commit a36d5d0cb46a91592a7edbf061f9af4c9ee7beae
Author: Tom Peters <thopeter@cisco.com>
Date:   Wed Oct 9 16:47:52 2019 -0400

    http2_inspect: cleanup

diff --git a/src/service_inspectors/ftp_telnet/ftp_parse.cc b/src/service_inspectors/ftp_telnet/ftp_parse.cc
index 7121512c4e4de629a0cd87b5de30b279a0fee9b5..fe1f71538c8fd7dff0c9bea4f5a251c07dfb9cde 100644 (file)
--- a/src/service_inspectors/ftp_telnet/ftp_parse.cc
+++ b/src/service_inspectors/ftp_telnet/ftp_parse.cc
@@ -592,7 +592,7 @@ int ProcessFTPCmdValidity(
 
     iRet = DoNextFormat(HeadFmt, 0, ErrorString, ErrStrLen);
 
-    /* Need to check to be sure we got a complete command  */
+    /* Need to check to be sure we got a complete command */
     if (iRet)
     {
         return FTPP_FATAL_ERR;

diff --git a/src/service_inspectors/ftp_telnet/ftpp_return_codes.h b/src/service_inspectors/ftp_telnet/ftpp_return_codes.h
index 6e0460149c5fbaa553dafa84bc160f91b043f671..5db4bf6bc6feca7aa910fe2515fd818decf94143 100644 (file)
--- a/src/service_inspectors/ftp_telnet/ftpp_return_codes.h
+++ b/src/service_inspectors/ftp_telnet/ftpp_return_codes.h
@@ -35,7 +35,6 @@
 #ifndef FTPP_RETURN_CODES_H
 #define FTPP_RETURN_CODES_H
 
-#define FTPP_BOOL_FALSE 0
 #define FTPP_SUCCESS    0
 
 /*

diff --git a/src/service_inspectors/http2_inspect/dev_notes.txt b/src/service_inspectors/http2_inspect/dev_notes.txt
index c636cc013551b913cef904cd6538c4578a565526..204ab473f2e82e28b5de9575fc93bfc4c6967559 100644 (file)
--- a/src/service_inspectors/http2_inspect/dev_notes.txt
+++ b/src/service_inspectors/http2_inspect/dev_notes.txt
@@ -1,14 +1,16 @@
 The HTTP/2 inspector (H2I) will convert HTTP/2 frames into HTTP/1.1 message sections and feed them
 to the new HTTP inspector (NHI) for further processing.
 
-The current implementation is the very first step. It splits an HTTP/2 stream into frame sections and
-forwards them for inspection. It does not interface with NHI and does not address the multiplexed
-nature of HTTP/2.
+The current implementation is the very first step. It splits an HTTP/2 stream into frame sections
+and forwards them for inspection. It does not interface with NHI and does not address the
+multiplexed nature of HTTP/2.
 
 The Http2StreamSplitter strips the frame headers from the frame data and stores them in separate
-buffers. As in NHI, long data frames are split into 16kb chunks for inspection. If a header frame is
-followed by continuation frames, all the header frames are flushed together for inspection. The
+buffers. As in NHI, long data frames are split into 16kb chunks for inspection. If a header frame
+is followed by continuation frames, all the header frames are flushed together for inspection. The
 frame headers from each frame are stored contiguously in the frame_header buffer. After cutting out
 the frame headers, the frame data is stored as a single block, consisting of the HPACK encoded
 HTTP/2 headers. HPACK decoding is under ongoing development. Decoded headers will be stored in the
 http2_decoded_header buffer.
+
+H2I supports the NHI test tool. See ../http_inspect/dev_notes.txt for usage instructions.

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
index 18fcc4cee5ad95bfc109e06f5941a97365112d84..eae9fcb60cea9c935cb65a9013b98a2afb4a5762 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
@@ -273,6 +273,7 @@ const StreamBuffer implement_reassemble(Http2FlowData* session_data, unsigned to
 
     if (offset == 0)
     {
+        // This is the first reassemble() for this frame and we need to allocate some buffers
         if (!session_data->header_coming[source_id])
             session_data->frame_data[source_id] = new uint8_t[total];
         else
@@ -299,8 +300,12 @@ const StreamBuffer implement_reassemble(Http2FlowData* session_data, unsigned to
         uint32_t data_pos = 0;
         do
         {
+            // Each pass through the loop handles one frame
+            // Multiple frames occur when a Headers frame and Continuation frame(s) are flushed
+            // together
             uint32_t remaining_len = len - data_pos;
 
+            // Process the frame header
             if (session_data->header_octets_seen[source_id] < FRAME_HEADER_LENGTH)
             {
                 uint8_t remaining_header = FRAME_HEADER_LENGTH -
@@ -347,7 +352,8 @@ const StreamBuffer implement_reassemble(Http2FlowData* session_data, unsigned to
                     frame_data_offset += 5;
                 }
                 session_data->remaining_octets_to_next_header[source_id] = frame_length;
-                session_data->remaining_frame_data_octets[source_id] = frame_length - pad_len - frame_data_offset;
+                session_data->remaining_frame_data_octets[source_id] =
+                    frame_length - pad_len - frame_data_offset;
                 session_data->remaining_frame_data_offset[source_id] = frame_data_offset;
             }
 
@@ -405,26 +411,19 @@ const StreamBuffer implement_reassemble(Http2FlowData* session_data, unsigned to
     {
         if (session_data->header_coming[source_id])
         {
-            const uint8_t type = get_frame_type(session_data->frame_header[source_id]);
-
-            if (type == FT_HEADERS || type == FT_CONTINUATION)
+            if (get_frame_type(session_data->frame_header[source_id]) == FT_HEADERS)
             {
                 assert(session_data->http2_decoded_header[source_id] == nullptr);
 
-                //FIXIT-H This will eventually be the decoded header buffer. For now just copy
-                //directly
-                               if (!decode_headers(session_data, source_id, session_data->frame_data[source_id],
-                        session_data->frame_data_size[source_id]))
+                // FIXIT-H This will eventually be the decoded header buffer. Under development.
+                if (!decode_headers(session_data, source_id, session_data->frame_data[source_id],
+                    session_data->frame_data_size[source_id]))
                     return frame_buf;
             }
         }
         // Return 0-length non-null buffer to stream which signals detection required, but don't 
         // create pkt_data buffer
-        frame_buf.length = 0;
-        if (session_data->frame_data[source_id])
-            frame_buf.data = session_data->frame_data[source_id];
-        else
-            frame_buf.data = session_data->frame_header[source_id];
+        frame_buf.data = (const uint8_t*)"";
     }
     return frame_buf;
 }
@@ -434,9 +433,8 @@ ValidationResult validate_preface(const uint8_t* data, const uint32_t length,
 {
     const uint32_t preface_length = 24;
 
-    static const uint8_t connection_prefix[] = {'P', 'R', 'I', ' ', '*', ' ',
-      'H', 'T', 'T', 'P', '/', '2', '.', '0', '\r', '\n', '\r', '\n', 'S', 'M', 
-      '\r', '\n', '\r', '\n'};
+    static const uint8_t connection_prefix[] = {'P', 'R', 'I', ' ', '*', ' ', 'H', 'T', 'T', 'P',
+        '/', '2', '.', '0', '\r', '\n', '\r', '\n', 'S', 'M', '\r', '\n', '\r', '\n'};
 
     assert(octets_seen < preface_length);
 
@@ -681,8 +679,8 @@ bool decode_header_line(Http2FlowData* session_data, HttpCommon::SourceId source
             encoded_header_length, Http2StreamSplitter::decode_int5, bytes_consumed, bytes_written);
 }
 
-//FIXIT-H This will eventually be the decoded header buffer. For now only string literals are
-//decoded
+// FIXIT-H This will eventually be the decoded header buffer. For now only string literals are
+// decoded
 bool decode_headers(Http2FlowData* session_data, HttpCommon::SourceId source_id,
     const uint8_t* encoded_header_buffer, const uint32_t header_length)
 {
@@ -714,9 +712,8 @@ bool decode_headers(Http2FlowData* session_data, HttpCommon::SourceId source_id,
 #ifdef REG_TEST
         if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
         {
-            fprintf(HttpTestManager::get_output_file(),
-                    "Error decoding headers. ");
-            if(session_data->http2_decoded_header_size[source_id] > 0)
+            fprintf(HttpTestManager::get_output_file(), "Error decoding headers. ");
+            if (session_data->http2_decoded_header_size[source_id] > 0)
                 Field(session_data->http2_decoded_header_size[source_id],
                     session_data->http2_decoded_header[source_id]).print(
                     HttpTestManager::get_output_file(), "Partially Decoded Header");
@@ -725,7 +722,7 @@ bool decode_headers(Http2FlowData* session_data, HttpCommon::SourceId source_id,
     return false;
     }
 
-    // write the last crlf to end the header
+    // write the last CRLF to end the header
     if (!write_decoded_headers(session_data, source_id, (const uint8_t*)"\r\n", 2,
             session_data->http2_decoded_header[source_id] +
             session_data->http2_decoded_header_size[source_id], MAX_OCTETS -
@@ -734,12 +731,12 @@ bool decode_headers(Http2FlowData* session_data, HttpCommon::SourceId source_id,
     session_data->http2_decoded_header_size[source_id] += line_bytes_written;
 
 #ifdef REG_TEST
-       if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
-       {
-               Field(session_data->http2_decoded_header_size[source_id],
-                       session_data->http2_decoded_header[source_id]).print(HttpTestManager::get_output_file(),
-            "Decoded Header");
-       }
+    if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
+    {
+        Field(session_data->http2_decoded_header_size[source_id],
+            session_data->http2_decoded_header[source_id]).
+            print(HttpTestManager::get_output_file(), "Decoded Header");
+    }
 #endif
 
     return success;