Merge pull request #2473 in SNORT/snort3 from ~THOPETER/snort3:h2i3 to master

Squashed commit of the following:

commit 4915334804e793384139ea575b935a12988ac21c
Author: Tom Peters <thopeter@cisco.com>
Date:   Mon Sep 14 14:20:09 2020 -0400

    http2_inspect: convert to new stream states

diff --git a/src/service_inspectors/http2_inspect/http2_data_frame.cc b/src/service_inspectors/http2_inspect/http2_data_frame.cc
index 8bee8e13a0f953224a9ee42e122bfd8742ba0efd..ba57bfab4cbe24e509af7e55ced032084285f244 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_data_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_data_frame.cc
@@ -72,11 +72,11 @@ void Http2DataFrame::update_stream_state()
 {
     switch (stream->get_state(source_id))
     {
-        case STATE_OPEN:
+        case STREAM_EXPECT_BODY:
             if (data_length > 0)
             {
                 session_data->concurrent_files += 1;
-                stream->set_state(source_id, STATE_OPEN_DATA);
+                stream->set_state(source_id, STREAM_BODY);
                 if (session_data->concurrent_files >
                     Http2Module::get_peg_counts(PEG_MAX_CONCURRENT_FILES))
                 {
@@ -84,17 +84,18 @@ void Http2DataFrame::update_stream_state()
                 }
             }
             if (stream->is_end_stream_on_data_flush(source_id))
-                stream->set_state(source_id, STATE_CLOSED);
+                stream->set_state(source_id, STREAM_COMPLETE);
             break;
-        case STATE_OPEN_DATA:
+        case STREAM_BODY:
             if (stream->is_end_stream_on_data_flush(source_id))
             {
                 assert(session_data->concurrent_files > 0);
                 session_data->concurrent_files -= 1;
-                stream->set_state(source_id, STATE_CLOSED);
+                stream->set_state(source_id, STREAM_COMPLETE);
             }
             break;
         default:
+            // FIXIT-E build this out
             // Stream state is idle or closed - this is caught in scan so should not get here
             assert(false);
     }

diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h
index 44ef7b7b4800a6c4f59c0138075682057bc8520d..286077501438a0b692c4503cddcdbcdca681d28a 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_enum.h
+++ b/src/service_inspectors/http2_inspect/http2_enum.h
@@ -36,7 +36,8 @@ enum FrameType : uint8_t { FT_DATA=0, FT_HEADERS=1, FT_PRIORITY=2, FT_RST_STREAM
     FT_PUSH_PROMISE=5, FT_PING=6, FT_GOAWAY=7, FT_WINDOW_UPDATE=8, FT_CONTINUATION=9, FT__ABORT=254,
     FT__NONE=255 };
 
-enum StreamState { STATE_IDLE, STATE_OPEN, STATE_OPEN_DATA, STATE_CLOSED };
+enum StreamState { STREAM_EXPECT_HEADERS, STREAM_EXPECT_BODY, STREAM_BODY, STREAM_IMPLIED_COMPLETE,
+    STREAM_COMPLETE, STREAM_ERROR };
 
 // Message buffers available to clients
 // This enum must remain synchronized with Http2Api::classic_buffer_names[]

diff --git a/src/service_inspectors/http2_inspect/http2_frame.cc b/src/service_inspectors/http2_inspect/http2_frame.cc
index 3520ed99d65532f99f514d4a808a3fb4082a382e..4d01db28301bed3131db3e10f2a5e6bb4fca85e9 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_frame.cc
@@ -55,9 +55,9 @@ Http2Frame* Http2Frame::new_frame(const uint8_t* header, const int32_t header_le
     switch(session_data->frame_type[source_id])
     {
         case FT_HEADERS:
-            if (stream->get_state(source_id) == STATE_IDLE)
-                return new Http2HeadersFrameHeader(header, header_len, data, data_len, session_data,
-                    source_id, stream);
+            if (stream->get_state(source_id) == STREAM_EXPECT_HEADERS)
+                return new Http2HeadersFrameHeader(header, header_len, data, data_len,
+                    session_data, source_id, stream);
             else
                 return new Http2HeadersFrameTrailer(header, header_len, data, data_len,
                     session_data, source_id, stream);
@@ -68,7 +68,8 @@ Http2Frame* Http2Frame::new_frame(const uint8_t* header, const int32_t header_le
             return new Http2DataFrame(header, header_len, data, data_len, session_data, source_id,
                 stream);
         default:
-            return new Http2Frame(header, header_len, data, data_len, session_data, source_id, stream);
+            return new Http2Frame(header, header_len, data, data_len, session_data, source_id,
+                stream);
     }
 }
 

diff --git a/src/service_inspectors/http2_inspect/http2_frame.h b/src/service_inspectors/http2_inspect/http2_frame.h
index 9fe27e7255f6d8a1b98ba29b5dd53ee04096b1da..526d2beae26965bf978399b0f115a95b9793f029 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_frame.h
@@ -23,12 +23,12 @@
 #include "service_inspectors/http_inspect/http_common.h"
 #include "service_inspectors/http_inspect/http_field.h"
 
-/* This class is called Http2Frame, but an object of this class may not represent exactly one HTTP/2
- * frame as received on the wire. For HEADERS frames, the Http2Frame object contains the initial
- * HEADERS frame plus any following CONTINUATION frames grouped together. For DATA frames, the
- * Http2Frame object represents approximately 16kb of data to be inspected. This may consist of
- * part of a larger DATA frame cut into 16kb-sized pieces, or several smaller DATA frames aggregated
- * together.
+/* This class is called Http2Frame, but an object of this class may not represent exactly one
+ * HTTP/2 frame as received on the wire. For HEADERS frames, the Http2Frame object contains the
+ * initial HEADERS frame plus any following CONTINUATION frames grouped together. For DATA frames,
+ * the Http2Frame object represents approximately 16kb of data to be inspected. This may consist
+ * of part of a larger DATA frame cut into 16kb-sized pieces, or several smaller DATA frames
+ * aggregated together.
  */
 
 class Http2FlowData;

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
index 00fbe1283e038e5d981de3ba8b415bc8fe9abac7..b7dddc5a5f488b6b7de35c620a71e8f525bf1b34 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
@@ -77,8 +77,6 @@ void Http2HeadersFrame::clear()
     session_data->hi->clear(&dummy_pkt);
 }
 
-
-
 void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow)
 {
     if (error_during_decode)
@@ -151,13 +149,14 @@ const Field& Http2HeadersFrame::get_buf(unsigned id)
 // Return: continue processing frame
 bool Http2HeadersFrame::check_frame_validity()
 {
-    if (stream->get_state(source_id) == STATE_CLOSED)
+    if (stream->get_state(source_id) == STREAM_COMPLETE)
     {
         *session_data->infractions[source_id] += INF_TRAILERS_AFTER_END_STREAM;
         session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
         return false;
     }
-    else if ((stream->get_state(source_id) != STATE_IDLE) and !(get_flags() & END_STREAM))
+    else if ((stream->get_state(source_id) != STREAM_EXPECT_HEADERS) and
+        !(get_flags() & END_STREAM))
     {
         // Trailers without END_STREAM flag set. Alert but continue to process.
         *session_data->infractions[source_id] += INF_FRAME_SEQUENCE;
@@ -170,29 +169,31 @@ void Http2HeadersFrame::update_stream_state()
 {
     switch (stream->get_state(source_id))
     {
-        case STATE_IDLE:
+        case STREAM_EXPECT_HEADERS:
             if (get_flags() & END_STREAM)
-                stream->set_state(source_id, STATE_CLOSED);
+                stream->set_state(source_id, STREAM_COMPLETE);
             else
-                stream->set_state(source_id, STATE_OPEN);
+                stream->set_state(source_id, STREAM_EXPECT_BODY);
             break;
-        case STATE_OPEN:
+        case STREAM_EXPECT_BODY:
             // fallthrough
-        case STATE_OPEN_DATA:
+        case STREAM_BODY:
             // Any trailing headers frame will be processed as trailers regardless of whether the
             // END_STREAM flag is set
-            if (stream->get_state(source_id) == STATE_OPEN_DATA)
+            if (stream->get_state(source_id) == STREAM_BODY)
                 session_data->concurrent_files -= 1;
-            stream->set_state(source_id, STATE_CLOSED);
+            stream->set_state(source_id, STREAM_COMPLETE);
             break;
-        case STATE_CLOSED:
+        case STREAM_COMPLETE:
             // FIXIT-E frame validity should be checked before creating frame so we should not get
             // here
             break;
+        default:
+            // FIXIT-E build this out
+            break;
     }
 }
 
-
 #ifdef REG_TEST
 void Http2HeadersFrame::print_frame(FILE* output)
 {

diff --git a/src/service_inspectors/http2_inspect/http2_inspect.cc b/src/service_inspectors/http2_inspect/http2_inspect.cc
index 3830f7b1d600eaaad8e2492b91235bed2f96380b..09f942f834d04b043729e3e838f4f6201fc005f6 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_inspect.cc
+++ b/src/service_inspectors/http2_inspect/http2_inspect.cc
@@ -173,6 +173,13 @@ void Http2Inspect::clear(Packet* p)
     if (session_data == nullptr)
         return;
 
+    // FIXIT-E precaution against spurious clear() calls
+    if (!session_data->frame_in_detection)
+    {
+        assert(session_data->stream_in_hi == NO_STREAM_ID);
+        return;
+    }
+
     session_data->frame_in_detection = false;
 
     const SourceId source_id = p->is_from_client() ? SRC_CLIENT : SRC_SERVER;

diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc
index e08ae1f65f12655a287c55699048019e3b162948..1cfa5610126bded917689db9c061c05506eb9a5f 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream.cc
@@ -54,7 +54,7 @@ Http2Stream::~Http2Stream()
 void Http2Stream::eval_frame(const uint8_t* header_buffer, int32_t header_len,
     const uint8_t* data_buffer, int32_t data_len, SourceId source_id)
 {
-    delete current_frame;
+    assert(current_frame == nullptr);
     current_frame = Http2Frame::new_frame(header_buffer, header_len, data_buffer,
         data_len, session_data, source_id, this);
     current_frame->update_stream_state();
@@ -62,12 +62,19 @@ void Http2Stream::eval_frame(const uint8_t* header_buffer, int32_t header_len,
 
 void Http2Stream::clear_frame()
 {
-    if (current_frame != nullptr) // FIXIT-M why is this needed?
-        current_frame->clear();
+    assert(current_frame != nullptr);
+    current_frame->clear();
     delete current_frame;
     current_frame = nullptr;
 }
 
+void Http2Stream::set_state(HttpCommon::SourceId source_id, StreamState new_state)
+{
+    assert((STREAM_EXPECT_HEADERS <= new_state) && (new_state <= STREAM_ERROR));
+    assert(state[source_id] < new_state);
+    state[source_id] = new_state;
+}
+
 void Http2Stream::set_hi_flow_data(HttpFlowData* flow_data)
 {
     assert(hi_flow_data == nullptr);
@@ -99,7 +106,7 @@ Http2DataCutter* Http2Stream::get_data_cutter(HttpCommon::SourceId source_id)
 
 bool Http2Stream::is_open(HttpCommon::SourceId source_id)
 {
-    return (state[source_id] == STATE_OPEN) || (state[source_id] == STATE_OPEN_DATA);
+    return (state[source_id] == STREAM_EXPECT_BODY) || (state[source_id] == STREAM_BODY);
 }
 
 void Http2Stream::finish_msg_body(HttpCommon::SourceId source_id, bool expect_trailers,

diff --git a/src/service_inspectors/http2_inspect/http2_stream.h b/src/service_inspectors/http2_inspect/http2_stream.h
index 2ae810eeee524075678339fb91df1a21c5afd5ca..3aec9cd6832c8756a435c559fac9b63257884e70 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.h
+++ b/src/service_inspectors/http2_inspect/http2_stream.h
@@ -60,9 +60,9 @@ public:
     bool is_partial_buf_pending(HttpCommon::SourceId source_id)
     { return partial_buf_pending[source_id]; }
 
-    void set_state(HttpCommon::SourceId source_id, Http2Enums::StreamState new_state) 
-        { state[source_id] = new_state; }
-    Http2Enums::StreamState get_state(HttpCommon::SourceId source_id) { return state[source_id]; }
+    void set_state(HttpCommon::SourceId source_id, Http2Enums::StreamState new_state);
+    Http2Enums::StreamState get_state(HttpCommon::SourceId source_id) const
+        { return state[source_id]; }
     bool is_open(HttpCommon::SourceId source_id);
     void set_end_stream_on_data_flush(HttpCommon::SourceId source_id)
         { end_stream_on_data_flush[source_id] = true; }
@@ -84,7 +84,8 @@ private:
     Http2DataCutter* data_cutter[2] = { nullptr, nullptr};
     bool partial_buf_pending[2] = { false, false }; // used to indicate a partial buffer
     bool end_stream_on_data_flush[2] = { false, false };
-    Http2Enums::StreamState state[2] = { Http2Enums::STATE_IDLE, Http2Enums::STATE_IDLE };
+    Http2Enums::StreamState state[2] =
+        { Http2Enums::STREAM_EXPECT_HEADERS, Http2Enums::STREAM_EXPECT_HEADERS };
 };
 
 #endif

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
index b567b26c36cc0d5940469951aedf68bd6b92e5ee..c92808aaaae5c6f54894c0ece7930e3732055fc9 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
@@ -214,7 +214,7 @@ bool Http2StreamSplitter::finish(Flow* flow)
     for (const Http2FlowData::StreamInfo& stream_info : session_data->streams)
     {
         if ((stream_info.id == 0)                                                 ||
-            (stream_info.stream->get_state(source_id) == STATE_CLOSED)            ||
+            (stream_info.stream->get_state(source_id) == STREAM_COMPLETE)         ||
             (stream_info.stream->get_hi_flow_data() == nullptr)                   ||
             (stream_info.stream->get_hi_flow_data()->get_type_expected(source_id)
                 != HttpEnums::SEC_BODY_H2))