Pull request #3203: http2_inspect: don't send data frames to the http stream splitter...

Merge in SNORT/snort3 from ~KATHARVE/snort3:h2_unexpected_data_frames to master

Squashed commit of the following:

commit ca74f8065c003468325bfd4cfab69d3bb19de67e
Author: Katura Harvey <katharve@cisco.com>
Date:   Wed Dec 1 11:41:51 2021 -0500

    http2_inspect: don't send data frames to the http stream splitter when it's not expecting them

diff --git a/doc/reference/builtin_stubs.txt b/doc/reference/builtin_stubs.txt
index a745668b969d08e779f503641104eefe8430b87b..18db0beee018443d0777a2686a87fe4ddef28a47 100644 (file)
--- a/doc/reference/builtin_stubs.txt
+++ b/doc/reference/builtin_stubs.txt
@@ -1441,6 +1441,10 @@ More than two HTTP/2 HPACK table size updates in a single header block
 
 HTTP/2 HPACK table size update exceeds max value set by decoder in SETTINGS frame
 
+121:37
+
+Nonempty HTTP/2 Data frame where a message body was not expected.
+
 122:1
 
 Basic one host to one host TCP portscan where multiple TCP ports are scanned on

diff --git a/src/service_inspectors/http2_inspect/http2_data_cutter.cc b/src/service_inspectors/http2_inspect/http2_data_cutter.cc
index 3966be60d0a9f41b757e9de7f6157f93250d08fe..800d01bce22035c69fe442eb1a4d13630282f027 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_data_cutter.cc
+++ b/src/service_inspectors/http2_inspect/http2_data_cutter.cc
@@ -37,10 +37,64 @@ Http2DataCutter::Http2DataCutter(Http2FlowData* _session_data, HttpCommon::Sourc
     session_data(_session_data), source_id(src_id)
 { }
 
+StreamSplitter::Status Http2DataCutter::skip_over_frame(Http2Stream* const stream, uint32_t length,
+    uint32_t* flush_offset, uint32_t data_offset, uint8_t frame_flags)
+{
+    uint32_t& remaining = session_data->scan_remaining_frame_octets[source_id];
+    if (length - data_offset < remaining)
+    {
+        *flush_offset = length;
+        remaining -= length - data_offset;
+    }
+    else
+    {
+        *flush_offset = data_offset + remaining;
+        remaining = 0;
+        session_data->header_octets_seen[source_id] = 0;
+        session_data->scan_state[source_id] = SCAN_FRAME_HEADER;
+        session_data->remaining_data_padding[source_id] = 0;
+
+        if (!session_data->frame_lengths[source_id].empty())
+        {
+            session_data->frame_lengths[source_id].pop();
+            assert(session_data->frame_lengths[source_id].empty());
+        }
+
+        if (stream && ((frame_flags & FLAG_END_STREAM) != 0))
+        {
+            stream->set_end_stream_on_data_flush(source_id);
+            discarded_frame_cleanup(stream);
+        }
+    }
+
+    session_data->payload_discard[source_id] = true;
+    return StreamSplitter::FLUSH;
+}
+
+bool Http2DataCutter::check_http_state(Http2Stream* const stream)
+{
+    HttpFlowData* const http_flow = stream->get_hi_flow_data();
+    if ((http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2))
+    {
+        stream->set_state(source_id, STREAM_ERROR);
+        if (data_len > 0)
+        {
+            *session_data->infractions[source_id] += INF_UNEXPECTED_DATA_FRAME;
+            session_data->events[source_id]->create_event(EVENT_UNEXPECTED_DATA_FRAME);
+        }
+        return false;
+    }
+    return true;
+}
+
 StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t length,
     uint32_t* flush_offset, uint32_t& data_offset, uint8_t frame_flags)
 {
-    const uint32_t cur_data_offset = data_offset;
+    Http2Stream* const stream = session_data->find_stream(session_data->current_stream[source_id]);
+        
+    if (!stream or !stream->is_open(source_id) or stream->is_discard_set(source_id))
+        return skip_over_frame(stream, length, flush_offset, data_offset, frame_flags);
+
     if (frame_bytes_seen == 0)
     {
         assert(session_data->frame_lengths[source_id].size() == 1);
@@ -53,8 +107,15 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt
             data_len -= 1;
             frame_bytes_seen += 1;
         }
+
+        if (!check_http_state(stream))
+        {
+            return skip_over_frame(stream, length, flush_offset, data_offset, frame_flags);
+        }
+
     }
 
+    const uint32_t cur_data_offset = data_offset;
     uint32_t cur_pos = data_offset;
     const uint32_t missing = data_len - data_bytes_read;
     const uint32_t cur_data = (missing <= (length - cur_pos)) ? missing : (length - cur_pos);
@@ -65,8 +126,6 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt
     data_offset = cur_pos;
     *flush_offset = cur_pos;
 
-    session_data->stream_in_hi = session_data->current_stream[source_id];
-
     StreamSplitter::Status scan_result = StreamSplitter::SEARCH;
 
     if (cur_data > 0)
@@ -75,32 +134,38 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt
         Http2DummyPacket dummy_pkt;
         dummy_pkt.flow = session_data->flow;
         uint32_t unused = 0;
+        session_data->stream_in_hi = session_data->current_stream[source_id];
         if ((data_bytes_read == data_len) && (frame_flags & FLAG_END_STREAM))
         {
-            Http2Stream* const stream =
-                session_data->find_stream(session_data->current_stream[source_id]);
             HttpFlowData* const hi_flow = stream->get_hi_flow_data();
             hi_flow->set_h2_body_state(source_id, HttpEnums::H2_BODY_LAST_SEG);
         }
         scan_result = session_data->hi_ss[source_id]->scan(&dummy_pkt, data + cur_data_offset,
             cur_data, unused, &http_flush_offset);
-
-        if (scan_result == StreamSplitter::FLUSH)
+        session_data->stream_in_hi = NO_STREAM_ID;
+        switch (scan_result)
         {
+        case StreamSplitter::FLUSH:
+          {
             bytes_sent_http += http_flush_offset;
             const uint32_t unused_input = cur_data - http_flush_offset;
             data_bytes_read -= unused_input;
             data_offset -= unused_input;
             *flush_offset -= unused_input;
             session_data->scan_remaining_frame_octets[source_id] -= http_flush_offset;
-        }
-        else if (scan_result == StreamSplitter::SEARCH)
-        {
+            break;
+          }
+        case StreamSplitter::SEARCH:
             bytes_sent_http += cur_data;
             session_data->scan_remaining_frame_octets[source_id] -= cur_data;
-        }
-        else
+            break;
+        default:
+            // Since we verified HttpStreamSplitter was expecting an H2 message body shouldn't get
+            // here
             assert(false);
+            stream->set_state(source_id, STREAM_ERROR);
+            return skip_over_frame(stream, length, flush_offset, data_offset, frame_flags);        
+        }
     }
 
     if (data_bytes_read == data_len)
@@ -112,22 +177,24 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt
 
         if (frame_flags & FLAG_END_STREAM)
         {
-            Http2Stream* const stream = session_data->find_stream(
-                session_data->current_stream[source_id]);
             assert(scan_result == StreamSplitter::FLUSH || data_len == 0);
+            session_data->stream_in_hi = session_data->current_stream[source_id];
             stream->finish_msg_body(source_id, false, data_len == 0);
+            session_data->stream_in_hi = NO_STREAM_ID;
 
             // FIXIT-E this flag seems to mean both END_STREAM and the end of this frame
             stream->set_end_stream_on_data_flush(source_id);
-            session_data->stream_in_hi = NO_STREAM_ID;
             return StreamSplitter::FLUSH;
         }
-        else if (scan_result != StreamSplitter::FLUSH)
+        else if (scan_result == StreamSplitter::SEARCH)
         {
-            assert(scan_result == StreamSplitter::SEARCH);
             scan_result = StreamSplitter::FLUSH;
             if (cur_data > 0)
+            {
+                session_data->stream_in_hi = session_data->current_stream[source_id];
                 session_data->hi_ss[source_id]->prep_partial_flush(session_data->flow, 0);
+                session_data->stream_in_hi = NO_STREAM_ID;
+            }
             else
             {
                 session_data->payload_discard[source_id] = true;
@@ -193,7 +260,8 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len)
                 reassemble_state = SEND_DATA;
             else
             {
-                assert(get_frame_flags(session_data->lead_frame_header[source_id]) & FLAG_END_STREAM);
+                assert(get_frame_flags(session_data->lead_frame_header[source_id]) &
+                    FLAG_END_STREAM);
                 reassemble_state = SEND_EMPTY_DATA;
             }
             break;
@@ -244,9 +312,26 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len)
     return;
 }
 
-void Http2DataCutter::discard_cleanup()
+void Http2DataCutter::discarded_frame_cleanup(Http2Stream* const stream)
 {
     frame_bytes_seen = 0;
     reassemble_data_bytes_read = 0;
     reassemble_state = GET_FRAME_HDR;
+    
+    if (!stream->is_end_stream_on_data_flush(source_id))
+        return;
+
+    if(stream->get_state(source_id) != STREAM_ERROR)
+    {
+        if (session_data->concurrent_files > 0)
+            session_data->concurrent_files -= 1;
+        stream->set_state(source_id, STREAM_COMPLETE);
+    }
+    stream->check_and_cleanup_completed();
+    if (session_data->delete_stream)
+    {
+        session_data->processing_stream_id = session_data->get_current_stream_id(source_id);
+        session_data->delete_processing_stream();
+        session_data->processing_stream_id = NO_STREAM_ID;
+    }
 }

diff --git a/src/service_inspectors/http2_inspect/http2_data_cutter.h b/src/service_inspectors/http2_inspect/http2_data_cutter.h
index 55b750238f45cc65dbef1fbdba1e3ea6e80422ed..245f9b61de49aea30f741556d25719dca9fa125a 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_data_cutter.h
+++ b/src/service_inspectors/http2_inspect/http2_data_cutter.h
@@ -26,6 +26,7 @@
 #include "http2_enum.h"
 
 class Http2FlowData;
+class Http2Stream;
 
 class Http2DataCutter
 {
@@ -34,9 +35,13 @@ public:
     snort::StreamSplitter::Status scan(const uint8_t* data, uint32_t length,
         uint32_t* flush_offset, uint32_t& data_offset, uint8_t frame_flags);
     void reassemble(const uint8_t* data, unsigned len);
-    void discard_cleanup();
+    void discarded_frame_cleanup(Http2Stream* const stream);
 
 private:
+    bool check_http_state(Http2Stream* const stream);
+    snort::StreamSplitter::Status skip_over_frame(Http2Stream* const stream, uint32_t length,
+        uint32_t* flush_offset, uint32_t data_offset, uint8_t frame_flags);
+    
     Http2FlowData* const session_data;
     const HttpCommon::SourceId source_id;
 

diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h
index 82b61a3348b880364a9f0c28c7d48694d0940cc9..2edf828407345ce0c3a0422bfdb5ba04037c4337 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_enum.h
+++ b/src/service_inspectors/http2_inspect/http2_enum.h
@@ -92,6 +92,7 @@ enum EventSid
     EVENT_TABLE_SIZE_UPDATE_NOT_AT_HEADER_START = 34,
     EVENT_MORE_THAN_2_TABLE_SIZE_UPDATES = 35,
     EVENT_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX = 36,
+    EVENT_UNEXPECTED_DATA_FRAME = 37,
     EVENT__MAX_VALUE
 };
 
@@ -147,6 +148,7 @@ enum Infraction
     INF_HEADER_UPPERCASE = 45,
     INF_INVALID_WINDOW_UPDATE_FRAME = 46,
     INF_WINDOW_UPDATE_FRAME_ZERO_INCREMENT = 47,
+    INF_UNEXPECTED_DATA_FRAME = 48,
     INF__MAX_VALUE
 };
 

diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc
index 1aeab5241832e8d844aa759e88e09d1c7a727f74..0852fd1434abe7da2b48837fa05ae16b30498000 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream.cc
@@ -127,6 +127,7 @@ bool Http2Stream::is_open(HttpCommon::SourceId source_id)
     return (state[source_id] == STREAM_EXPECT_BODY) || (state[source_id] == STREAM_BODY);
 }
 
+// Caller must set session_data->stream_in_hi before calling this
 void Http2Stream::finish_msg_body(HttpCommon::SourceId source_id, bool expect_trailers,
     bool clear_partial_buffer)
 {

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.h b/src/service_inspectors/http2_inspect/http2_stream_splitter.h
index 624aa70daad6ed8e2338a8d82b1a387861e3f20b..bdae9ebde2f29c082ab24b8a070967bb6090e2ec 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.h
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.h
@@ -47,9 +47,8 @@ public:
 
 private:
     const HttpCommon::SourceId source_id;
-    static StreamSplitter::Status data_frame_header_checks(Http2FlowData* session_data,
-        uint32_t* flush_offset, HttpCommon::SourceId source_id, uint32_t frame_length,
-        uint32_t& data_offset);
+    static void data_frame_header_checks(Http2FlowData* session_data,
+        HttpCommon::SourceId source_id);
     static StreamSplitter::Status non_data_scan(Http2FlowData* session_data,
         uint32_t length, uint32_t* flush_offset, HttpCommon::SourceId source_id, uint8_t type,
         uint8_t frame_flags, uint32_t& data_offset);
@@ -60,7 +59,6 @@ private:
         HttpCommon::SourceId source_id);
     static bool read_frame_hdr(Http2FlowData* session_data, const uint8_t* data,
         uint32_t length, HttpCommon::SourceId source_id, uint32_t& data_offset);
-    static void discarded_data_frame_cleanup(Http2FlowData* session_data, HttpCommon::SourceId source_id, Http2Stream* stream);
 };
 
 

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 35cd94581941b474a535fa7c48385e1c7a209895..5d3519f89003b871a0c46995e8bb769a7d19fd7c 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
@@ -63,9 +63,8 @@ static ValidationResult validate_preface(const uint8_t* data, const uint32_t len
     return V_GOOD;
 }
 
-StreamSplitter::Status Http2StreamSplitter::data_frame_header_checks(Http2FlowData* session_data,
-    uint32_t* flush_offset, HttpCommon::SourceId source_id, uint32_t frame_length,
-    uint32_t& data_offset)
+void Http2StreamSplitter::data_frame_header_checks(Http2FlowData* session_data,
+     HttpCommon::SourceId source_id)
 {
     Http2Stream* const stream = session_data->find_stream(session_data->current_stream[source_id]);
 
@@ -81,22 +80,7 @@ StreamSplitter::Status Http2StreamSplitter::data_frame_header_checks(Http2FlowDa
                 stream->set_state(source_id, STREAM_ERROR);
             }
         }
-        return StreamSplitter::SEARCH;
-    }
-
-    HttpFlowData* const http_flow = (HttpFlowData*)stream->get_hi_flow_data();
-    assert(http_flow != nullptr);
-
-    // If 0 length frame and http_inspect isn't expecting body, flush without involving HI
-    if ((frame_length == 0) && (http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2))
-    {
-        *flush_offset = data_offset;
-        session_data->header_octets_seen[source_id] = 0;
-        session_data->scan_state[source_id] = SCAN_FRAME_HEADER;
-        return StreamSplitter::FLUSH;
     }
-
-    return StreamSplitter::SEARCH;
 }
 
 StreamSplitter::Status Http2StreamSplitter::non_data_scan(Http2FlowData* session_data,
@@ -283,10 +267,7 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio
                 }
 
                 if (type == FT_DATA)
-                {
-                    status = data_frame_header_checks(session_data, flush_offset, source_id,
-                        frame_length, data_offset);
-                }
+                    data_frame_header_checks(session_data, source_id);
 
                 break;
             }
@@ -330,46 +311,8 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio
                 }
                 else
                 {
-                    Http2Stream* const stream =
-                        session_data->find_stream(session_data->current_stream[source_id]);
-                    if (stream && stream->is_open(source_id) && !stream->is_discard_set(source_id))
-                    {
-                        status = session_data->data_cutter[source_id].scan(
-                            data, length, flush_offset, data_offset, frame_flags);
-                    }
-                    else
-                    {
-                        // Need to skip past Data frame in a bad stream or if passed flow depth
-                        uint32_t& remaining = session_data->scan_remaining_frame_octets[source_id];
-                        if (length - data_offset < remaining)
-                        {
-                            *flush_offset = length;
-                            remaining -= length - data_offset;
-                        }
-                        else
-                        {
-                            *flush_offset = data_offset + remaining;
-                            remaining = 0;
-                            session_data->header_octets_seen[source_id] = 0;
-                            session_data->scan_state[source_id] = SCAN_FRAME_HEADER;
-                            session_data->remaining_data_padding[source_id] = 0;
-
-                            if (!session_data->frame_lengths[source_id].empty())
-                            {
-                                session_data->frame_lengths[source_id].pop();
-                                assert(session_data->frame_lengths[source_id].empty());
-                            }
-
-                            if (stream && ((frame_flags & FLAG_END_STREAM) != 0))
-                            {
-                                stream->set_end_stream_on_data_flush(source_id);
-                                discarded_data_frame_cleanup(session_data, source_id, stream);
-                            }
-                        }
-
-                        session_data->payload_discard[source_id] = true;
-                        status = StreamSplitter::FLUSH;
-                    }
+                    status = session_data->data_cutter[source_id].scan(data, length, flush_offset,
+                            data_offset, frame_flags);
                 }
                 assert(status != StreamSplitter::SEARCH or
                     session_data->scan_state[source_id] != SCAN_EMPTY_DATA);
@@ -525,7 +468,7 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess
             if (stream)
             {
                 stream->set_discard(source_id);
-                discarded_data_frame_cleanup(session_data, source_id, stream);
+                session_data->data_cutter[source_id].discarded_frame_cleanup(stream);
             }
         }
         else
@@ -539,24 +482,3 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess
 
     return frame_buf;
 }
-
-void Http2StreamSplitter::discarded_data_frame_cleanup(Http2FlowData* session_data,
-    HttpCommon::SourceId source_id, Http2Stream* stream)
-{
-    session_data->data_cutter[source_id].discard_cleanup();
-
-    if (stream->get_state(source_id) == STREAM_ERROR ||
-        !stream->is_end_stream_on_data_flush(source_id))
-        return;
-
-    if (session_data->concurrent_files > 0)
-        session_data->concurrent_files -= 1;
-    stream->set_state(source_id, STREAM_COMPLETE);
-    stream->check_and_cleanup_completed();
-    if (session_data->delete_stream)
-    {
-        session_data->processing_stream_id = session_data->get_current_stream_id(source_id);
-        session_data->delete_processing_stream();
-        session_data->processing_stream_id = NO_STREAM_ID;
-    }
-}

diff --git a/src/service_inspectors/http2_inspect/http2_tables.cc b/src/service_inspectors/http2_inspect/http2_tables.cc
index c34429e8c24943e26f0e2e3564be82416debfefa..88919dae4dacf8deb12d32d30b46ad1f4af11f70 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_tables.cc
+++ b/src/service_inspectors/http2_inspect/http2_tables.cc
@@ -70,6 +70,7 @@ const RuleMap Http2Module::http2_events[] =
         "More than two HTTP/2 HPACK table size updates in a single header block" },
     { EVENT_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX,
         "HTTP/2 HPACK table size update exceeds max value set by decoder in SETTINGS frame" },
+    { EVENT_UNEXPECTED_DATA_FRAME, "Nonempty HTTP/2 Data frame where message body not expected" },
     { 0, nullptr }
 };
 

diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
index 238db9471e62913e69041cc5609401e3a59b3494..db156ca3fb53d7b858d4d6fc73d24ba3db74f494 100644 (file)
--- a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
+++ b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
@@ -276,7 +276,7 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total,
         (session_data->section_type[source_id] == SEC__NOT_COMPUTE))
     {
         assert(session_data->type_expected[source_id] != SEC_ABORT);
-        // assert(session_data->section_type[source_id] != SEC__NOT_COMPUTE); // FIXIT-M H2I
+        assert(session_data->section_type[source_id] != SEC__NOT_COMPUTE);
         session_data->type_expected[source_id] = SEC_ABORT;
         return { nullptr, 0 };
     }