Pull request #3145: http2_inspect: hardening

Merge in SNORT/snort3 from ~THOPETER/snort3:h2i20 to master

Squashed commit of the following:

commit a271d65b5f0146e0101b6aac999ae890dcc29235
Author: Tom Peters <thopeter@cisco.com>
Date:   Tue Oct 19 18:44:34 2021 -0400

    http2_inspect: hardening

diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.h b/src/service_inspectors/http2_inspect/http2_flow_data.h
index 7b0acb9f7668040855859fc706273359d98ef4b3..fee0d104f48ff2efb91d937f0f07e0bc988a4eef 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_flow_data.h
+++ b/src/service_inspectors/http2_inspect/http2_flow_data.h
@@ -124,8 +124,7 @@ protected:
 
     // 0 element refers to client frame, 1 element refers to server frame
 
-    // There is currently one infraction and one event object per flow per direction. This may
-    // change in the future.
+    // There is currently one infraction and one event object per flow per direction.
     Http2Infractions* const infractions[2] = { new Http2Infractions, new Http2Infractions };
     Http2EventGen* const events[2] = { new Http2EventGen, new Http2EventGen };
 
@@ -141,6 +140,7 @@ protected:
     uint32_t stream_in_hi = Http2Enums::NO_STREAM_ID;
     HttpMsgSection* hi_msg_section = nullptr;
     bool server_settings_frame_received = false;
+    bool tcp_close[2] = { false, false };
 
     // Reassemble() data to eval()
     uint8_t lead_frame_header[2][Http2Enums::FRAME_HEADER_LENGTH];

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h
index 08f6b85d1b9ea41838d0af6fd265cb9f089bf430..ce291b0c612e89b427be3a6629f07be48dd199c6 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h
@@ -50,7 +50,7 @@ protected:
     void process_decoded_headers(HttpFlowData* http_flow, HttpCommon::SourceId hi_source_id);
     uint8_t get_flags_mask() const override;
 
-    Field http1_header;                 // finalized headers to be passed to NHI
+    Field http1_header;                 // finalized headers to be passed to http_inspect
     uint32_t xtradata_mask = 0;
     bool detection_required = false;
     bool process_frame = true;

diff --git a/src/service_inspectors/http2_inspect/http2_inspect.cc b/src/service_inspectors/http2_inspect/http2_inspect.cc
index cec10f32a1db289aa6b53bf7614e8d6a254b1c1e..e47fa964eb5f6929c0bf6e8de9196cb61e8d51cd 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_inspect.cc
+++ b/src/service_inspectors/http2_inspect/http2_inspect.cc
@@ -120,9 +120,11 @@ void Http2Inspect::eval(Packet* p)
         (Http2FlowData*)p->flow->get_flow_data(Http2FlowData::inspector_id);
 
     if (!session_data)
+    {
+        assert(false);
         return;
+    }
 
-    // FIXIT-E Workaround for unexpected eval() calls
     if (session_data->abort_flow[source_id])
     {
         return;
@@ -181,9 +183,11 @@ void Http2Inspect::clear(Packet* p)
         (Http2FlowData*)p->flow->get_flow_data(Http2FlowData::inspector_id);
 
     if (session_data == nullptr)
+    {
+        assert(false);
         return;
+    }
 
-    // FIXIT-E precaution against spurious clear() calls
     if (!session_data->frame_in_detection)
     {
         assert(session_data->stream_in_hi == NO_STREAM_ID);

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
index 8847c67986c5fbc0c3a9c2353aeffb00bf65cec2..4884f1c51313e2f4f3592dc1b9933aef3194793e 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
@@ -56,7 +56,10 @@ StreamSplitter::Status Http2StreamSplitter::scan(Packet* pkt, const uint8_t* dat
         AssistantGadgetEvent event(pkt, "http");
         DataBus::publish(FLOW_ASSISTANT_GADGET_EVENT, event, pkt->flow);
         if (pkt->flow->assistant_gadget == nullptr)
+        {
+            // http_inspect is not configured
             return HttpStreamSplitter::status_value(StreamSplitter::ABORT, true);
+        }
         pkt->flow->set_flow_data(session_data = new Http2FlowData(pkt->flow));
         Http2Module::increment_peg_counts(PEG_FLOW);
     }
@@ -201,10 +204,21 @@ bool Http2StreamSplitter::finish(Flow* flow)
 
     Http2FlowData* session_data = (Http2FlowData*)flow->get_flow_data(Http2FlowData::inspector_id);
     if (!session_data)
+    {
+        // assert(false); // FIXIT-M this should not be possible but currently it may be
         return false;
+    }
     if (session_data->abort_flow[source_id])
         return false;
 
+    if (session_data->tcp_close[source_id])
+    {
+        // assert(false); // FIXIT-M this should not happen but it does
+        session_data->abort_flow[source_id] = true;
+        return false;
+    }
+    session_data->tcp_close[source_id] = true;
+
 #ifdef REG_TEST
     if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
     {
@@ -264,7 +278,6 @@ bool Http2StreamSplitter::finish(Flow* flow)
 #endif
         }
         session_data->stream_in_hi = NO_STREAM_ID;
-
     }
 
     return need_reassemble;