Pull request #3138: Hpack refactor2

Merge in SNORT/snort3 from ~KATHARVE/snort3:hpack-refactor2 to master

Squashed commit of the following:

commit 3649ca44dbce29d22cbd296556816658d4f00b25
Author: Katura Harvey <katharve@cisco.com>
Date:   Thu Oct 28 16:59:41 2021 -0400

    http2_inspect: http1_header buffer always created immediately after decode_headers

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
index dcfa41049d9119d9b2123f21504eac89f7e9e4e7..c451f7c0c9e14fdeb838be99fb363717033bdc6b 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
@@ -53,12 +53,6 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_
     hpack_decoder = &session_data->hpack_decoder[source_id];
 }
 
-
-Http2HeadersFrame::~Http2HeadersFrame()
-{
-    hpack_decoder->cleanup();
-}
-
 void Http2HeadersFrame::clear()
 {
     if (session_data->abort_flow[source_id] || stream->get_state(source_id) == STREAM_ERROR)
@@ -68,12 +62,39 @@ void Http2HeadersFrame::clear()
     session_data->hi->clear(&dummy_pkt);
 }
 
+bool Http2HeadersFrame::decode_headers(Http2StartLine* start_line_generator, bool trailers)
+{
+    const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ?
+        data.length() - hpack_headers_offset : 0;
+    if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset),
+        encoded_headers_length, start_line_generator, trailers))
+    {
+        if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
+        {
+            session_data->abort_flow[source_id] = true;
+            session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
+            http1_header.set(STAT_PROBLEMATIC);
+            hpack_decoder->cleanup();
+            return false;
+        }
+    }
+    hpack_decoder->set_decoded_headers(http1_header);
+    return true;
+}
+
 void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow, SourceId hi_source_id)
 {
-    if (session_data->abort_flow[source_id])
+    if (session_data->abort_flow[source_id] or http1_header.length() < 0)
         return;
 
-    hpack_decoder->set_decoded_headers(http1_header);
+    if (http1_header.length() <= 0 and !session_data->is_processing_partial_header())
+    {
+        // This shouldn't happen because well-formatted empty frames have crlf written to the
+        // decoded headers buffer
+        assert(false);
+        return;
+    }
+
     StreamBuffer stream_buf;
 
     // http_inspect scan() of headers
@@ -81,7 +102,6 @@ void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow, SourceI
     // be empty. Don't call scan on the empty buffer because it will create a cutter and the check
     // for this condition in HI::finish() will fail. Truncated headers with non-empty http1_header
     // buffers are still sent to HI::scan().
-    assert ((http1_header.length() > 0) or session_data->is_processing_partial_header());
     if (http1_header.length() > 0)
     {
         uint32_t flush_offset;

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h
index ce291b0c612e89b427be3a6629f07be48dd199c6..f71120de9c86559fc07225e79acd7f304edb010e 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h
@@ -32,7 +32,6 @@ class HttpFlowData;
 class Http2HeadersFrame : public Http2Frame
 {
 public:
-    ~Http2HeadersFrame() override;
     void clear() override;
 
     const Field& get_buf(unsigned id) override;
@@ -47,6 +46,7 @@ protected:
     Http2HeadersFrame(const uint8_t* header_buffer, const uint32_t header_len,
         const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data,
         HttpCommon::SourceId src_id, Http2Stream* stream);
+    bool decode_headers(Http2StartLine* start_line_generator, bool trailers);
     void process_decoded_headers(HttpFlowData* http_flow, HttpCommon::SourceId hi_source_id);
     uint8_t get_flags_mask() const override;
 

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
index e48db69e6a31ac7e39d459a9d32a1758fcde023a..e45bbdde19d59b8516a416550cda017454862149 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
@@ -53,25 +53,14 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer,
         start_line_generator = new Http2StatusLine(session_data->events[source_id],
             session_data->infractions[source_id]);
 
-    // Decode headers
-    const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ?
-        data.length() - hpack_headers_offset : 0;
-    if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset),
-        encoded_headers_length, start_line_generator, false))
+    if (decode_headers(start_line_generator, false))
     {
-        if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
+        // process start line
+        if (!start_line_generator->generate_start_line(start_line, are_pseudo_headers_complete()))
         {
-            session_data->abort_flow[source_id] = true;
-            session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
-            return;
+            stream->set_state(source_id, STREAM_ERROR);
         }
     }
-
-    // process start line
-    if (!start_line_generator->generate_start_line(start_line, are_pseudo_headers_complete()))
-    {
-        stream->set_state(source_id, STREAM_ERROR);
-    }
 }
 
 bool Http2HeadersFrameHeader::valid_sequence(Http2Enums::StreamState state)

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
index ccf9d3b67f92b241df887d53c5925cf6699c265a..8638dad3906b0b5ba4598103ce1d9c79329beee0 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
@@ -54,19 +54,7 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer,
         *session_data->infractions[source_id] += INF_TRAILERS_NOT_END;
         session_data->events[source_id]->create_event(EVENT_TRAILERS_NOT_END);
     }
-
-    // Decode trailers
-    const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ?
-        data.length() - hpack_headers_offset : 0;
-    if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset),
-        encoded_headers_length, nullptr, true))
-    {
-        if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
-        {
-            session_data->abort_flow[source_id] = true;
-            session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
-        }
-    }
+    decode_headers(nullptr, true);
 }
 
 bool Http2HeadersFrameTrailer::valid_sequence(Http2Enums::StreamState state)

diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc
index f9c9c2b11644f8728469ba076445982a3d1b88c7..5e5599a233c2e8e30bb6463779d4bb072b17074e 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_hpack.cc
+++ b/src/service_inspectors/http2_inspect/http2_hpack.cc
@@ -437,12 +437,7 @@ void Http2HpackDecoder::set_decoded_headers(Field& http1_header)
 
 void Http2HpackDecoder::cleanup()
 {
-    if (decoded_headers)
-    {
-        // get_decoded_headers() was never called because we encountered some error during
-        // processing. We must clean up the buffer.
-        delete[] decoded_headers;
-        decoded_headers = nullptr;
-        decoded_headers_size = 0;
-    }
+    delete[] decoded_headers;
+    decoded_headers = nullptr;
+    decoded_headers_size = 0;
 }

diff --git a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
index 89b7841e082c1b98147f7771d3ef57dc0ebf2a95..a47578d54ba5455595cdcdea41f3b508737faa40 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
@@ -65,16 +65,7 @@ Http2PushPromiseFrame::Http2PushPromiseFrame(const uint8_t* header_buffer,
 
     hpack_headers_offset += PROMISED_ID_LENGTH;
 
-    // Decode headers
-    if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() -
-        hpack_headers_offset, start_line_generator, false))
-    {
-        if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
-        {
-            session_data->abort_flow[source_id] = true;
-            session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
-        }
-    }
+    decode_headers(start_line_generator, false);
 }
 
 bool Http2PushPromiseFrame::valid_sequence(Http2Enums::StreamState)