Merge pull request #1827 in SNORT/snort3 from ~KATHARVE/snort3:h2i_code_coverage...

Squashed commit of the following:

commit fb6ef30804b7463b132fac75af68005fe9fce16e
Author: Katura Harvey <katharve@cisco.com>
Date:   Mon Oct 28 14:53:03 2019 -0400

    http2_inspect: fix bugs in splitting long data frames and padding

diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.cc b/src/service_inspectors/http2_inspect/http2_flow_data.cc
index 438c85b3b08e14fb62e9ad16354150af230bc36b..a7a61108851bace869b42687a613b5c4518efeb7 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_flow_data.cc
+++ b/src/service_inspectors/http2_inspect/http2_flow_data.cc
@@ -82,8 +82,12 @@ Http2FlowData::~Http2FlowData()
 
 void Http2FlowData::clear_frame_data(HttpCommon::SourceId source_id)
 {
-    delete[] frame_header[source_id];
-    frame_header[source_id] = nullptr;
+    // If there is more data to be inspected in the frame, leave the frame_header
+    if (leftover_data[source_id] == 0)
+    {
+        delete[] frame_header[source_id];
+        frame_header[source_id] = nullptr;
+    }
     delete[] frame_data[source_id];
     frame_data[source_id] = nullptr;
     frame_in_detection = false;

diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc
index 25b576d985cdfe06580b31a83966e8c7162e492e..043abbae61f6f55ae2c501fea53548ad109e6791 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_hpack.cc
+++ b/src/service_inspectors/http2_inspect/http2_hpack.cc
@@ -51,7 +51,7 @@ bool Http2Hpack::write_decoded_headers(Http2FlowData* session_data, HttpCommon::
 
     if (in_length > decoded_header_length)
     {
-        length = MAX_OCTETS - session_data->raw_decoded_header_size[source_id];
+        length = decoded_header_length;
         *session_data->infractions[source_id] += INF_DECODED_HEADER_BUFF_OUT_OF_SPACE;
         session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
         ret = false;
@@ -82,9 +82,9 @@ bool Http2Hpack::decode_string_literal(Http2FlowData* session_data, HttpCommon::
     }
 
     if (!decode_string.translate(encoded_header_buffer + encoded_header_offset,
-        encoded_header_length, encoded_bytes_consumed, decoded_header_buffer,
-        decoded_header_length, decoded_bytes_written, session_data->events[source_id],
-        session_data->infractions[source_id]))
+        encoded_header_length - encoded_header_offset, encoded_bytes_consumed,
+        decoded_header_buffer, decoded_header_length, decoded_bytes_written,
+        session_data->events[source_id], session_data->infractions[source_id]))
     {
         return false;
     }
@@ -257,7 +257,10 @@ bool Http2Hpack::decode_literal_header_line(Http2FlowData* session_data,
         if (!Http2Hpack::decode_index(session_data, source_id, encoded_header_buffer,
                 encoded_header_length, decode_int, false, partial_bytes_consumed,
                 decoded_header_buffer, decoded_header_length, partial_bytes_written))
+        {
+            bytes_written += partial_bytes_written;
             return false;
+        }
     }
     // literal field name
     else
@@ -266,8 +269,10 @@ bool Http2Hpack::decode_literal_header_line(Http2FlowData* session_data,
                 encoded_header_length, true,
                 partial_bytes_consumed, decoded_header_buffer, decoded_header_length,
                 partial_bytes_written))
+        {
+            bytes_written += partial_bytes_written;
             return false;
-
+        }
         // If this was a pseudo-header value, give it to the start-line.
         if (session_data->header_start_line[source_id] and
                 session_data->header_start_line[source_id]->is_pseudo_name(
@@ -298,7 +303,10 @@ bool Http2Hpack::decode_literal_header_line(Http2FlowData* session_data,
             false, partial_bytes_consumed,
             decoded_header_buffer + bytes_written, decoded_header_length -
             bytes_written, partial_bytes_written))
+    {
+        bytes_written += partial_bytes_written;
         return false;
+    }
 
     // If this was a pseudo-header value, give it to the start-line.
     if (session_data->header_start_line[source_id] and
@@ -401,55 +409,33 @@ bool Http2Hpack::decode_headers(Http2FlowData* session_data, HttpCommon::SourceI
     if (source_id == SRC_CLIENT)
         session_data->header_start_line[source_id] = new Http2RequestLine(session_data, source_id);
 
-    while (total_bytes_consumed < header_length)
+    while (success and total_bytes_consumed < header_length)
     {
-        if (!Http2Hpack::decode_header_line(session_data, source_id,
+        success = Http2Hpack::decode_header_line(session_data, source_id,
             encoded_header_buffer + total_bytes_consumed, header_length - total_bytes_consumed,
             line_bytes_consumed, session_data->raw_decoded_header[source_id] +
             session_data->raw_decoded_header_size[source_id], MAX_OCTETS -
-            session_data->raw_decoded_header_size[source_id], line_bytes_written))
-        {
-            success = false;
-            break;
-        }
+            session_data->raw_decoded_header_size[source_id], line_bytes_written);
         total_bytes_consumed  += line_bytes_consumed;
         session_data->raw_decoded_header_size[source_id] += line_bytes_written;
     }
 
-    if (!success)
-    {
-#ifdef REG_TEST
-        if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
-        {
-            fprintf(HttpTestManager::get_output_file(), "Error decoding headers.\n");
-            if (session_data->header_start_line[source_id] and
-                session_data->header_start_line[source_id]->get_start_line().length() > 0)
-                session_data->header_start_line[source_id]->get_start_line().
-                    print(HttpTestManager::get_output_file(), "Decoded start-line");
-            if (session_data->raw_decoded_header_size[source_id] > 0)
-                Field(session_data->raw_decoded_header_size[source_id],
-                    session_data->raw_decoded_header[source_id]).print(
-                    HttpTestManager::get_output_file(), "Partially decoded raw header");
-        }
-#endif
-    return false;
-    }
-
     // If there were only pseudo-headers, finalize never got called, so create the start-line
     if (session_data->header_start_line[source_id] and
         !session_data->header_start_line[source_id]->is_finalized())
     {
-        if (!session_data->header_start_line[source_id]->finalize())
-            return false;
+        success &= session_data->header_start_line[source_id]->finalize();
     }
 
     // write the last CRLF to end the header
-    if (!Http2Hpack::write_decoded_headers(session_data, source_id, (const uint8_t*)"\r\n", 2,
-        session_data->raw_decoded_header[source_id] +
-        session_data->raw_decoded_header_size[source_id], MAX_OCTETS -
-        session_data->raw_decoded_header_size[source_id], line_bytes_written))
-        return false;
-    session_data->raw_decoded_header_size[source_id] += line_bytes_written;
+    if (success)
+    {
+        success = Http2Hpack::write_decoded_headers(session_data, source_id,
+            (const uint8_t*)"\r\n", 2, session_data->raw_decoded_header[source_id] +
+            session_data->raw_decoded_header_size[source_id], MAX_OCTETS -
+            session_data->raw_decoded_header_size[source_id], line_bytes_written);
+        session_data->raw_decoded_header_size[source_id] += line_bytes_written;
+    }
 
     // set http2_decoded_header to send to NHI
     session_data->http2_decoded_header[source_id] = new Field(
@@ -458,14 +444,30 @@ bool Http2Hpack::decode_headers(Http2FlowData* session_data, HttpCommon::SourceI
         session_data->raw_decoded_header[source_id] +
         session_data->pseudo_header_fragment_size[source_id], false);
 
+
 #ifdef REG_TEST
     if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
     {
-        if (session_data->header_start_line[source_id])
-            session_data->header_start_line[source_id]->get_start_line().
-                print(HttpTestManager::get_output_file(), "Decoded start-line");
-        session_data->http2_decoded_header[source_id]->
-            print(HttpTestManager::get_output_file(), "Decoded header");
+        if (success)
+        {
+            if (session_data->header_start_line[source_id])
+                session_data->header_start_line[source_id]->get_start_line().
+                    print(HttpTestManager::get_output_file(), "Decoded start-line");
+            session_data->http2_decoded_header[source_id]->
+                print(HttpTestManager::get_output_file(), "Decoded header");
+        }
+        else
+        {
+            fprintf(HttpTestManager::get_output_file(), "Error decoding headers.\n");
+            if (session_data->header_start_line[source_id] and
+                session_data->header_start_line[source_id]->get_start_line().length() > 0)
+                session_data->header_start_line[source_id]->get_start_line().
+                    print(HttpTestManager::get_output_file(), "Decoded start-line");
+            if (session_data->raw_decoded_header_size[source_id] > 0)
+                Field(session_data->raw_decoded_header_size[source_id],
+                    session_data->raw_decoded_header[source_id]).print(
+                    HttpTestManager::get_output_file(), "Partially decoded raw header");
+        }
     }
 #endif
 

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 cd250c3c6d0b383a5a2e3a00dc97850d83cb02a2..9120a32590c595d4f9d235a3d3eec41ae8ee749f 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
@@ -107,7 +107,8 @@ StreamSplitter::Status implement_scan(Http2FlowData* session_data, const uint8_t
         } 
 
         // Have full inspection section, flush and update leftover
-        *flush_offset = session_data->inspection_section_length[source_id];
+        *flush_offset = session_data->inspection_section_length[source_id] -
+            session_data->octets_seen[source_id];
         session_data->leftover_data[source_id] -=
             session_data->inspection_section_length[source_id];
         session_data->octets_seen[source_id] = 0;
@@ -318,13 +319,22 @@ const StreamBuffer implement_reassemble(Http2FlowData* session_data, unsigned to
 
                 frame_length = get_frame_length(session_data->frame_header[source_id] +
                     session_data->frame_header_offset[source_id]);
+
+                if (frame_length == 0)
+                    break;
+                if (remaining_len == 0)
+                    return frame_buf;
+
+                // handle split long data frames
+                if (frame_length > total)
+                    frame_length = total - data_pos;
                 frame_flags = get_frame_flags(session_data->frame_header[source_id] +
                     session_data->frame_header_offset[source_id]);
 
                 if (frame_flags & PADDED)
                 {
                     frame_data_offset += 1;
-                    pad_len = session_data->frame_data[source_id][0];
+                    pad_len = *(data + data_pos);
                 }
                 //FIXIT-M handle stream dependency and weight. For now just skip over
                 if (frame_flags & PRIORITY)