From: Tom Peters (thopeter) <thopeter@cisco.com>
Date: Thu, 4 Nov 2021 16:40:25 +0000 (+0000)
Subject: Pull request #3145: http2_inspect: hardening
X-Git-Tag: 3.1.17.0~19
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ba213ffb5710ffb55b6a09927090c019f2ad686;p=thirdparty%2Fsnort3.git

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 7b0acb9f7..fee0d104f 100644
--- 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 08f6b85d1..ce291b0c6 100644
--- 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 cec10f32a..e47fa964e 100644
--- 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 8847c6798..4884f1c51 100644
--- 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;