Merge pull request #2618 in SNORT/snort3 from ~THOPETER/snort3:h2i17 to master

Squashed commit of the following:

commit 58296aa1e56005645325b178504e68f3278b7f0d
Author: Tom Peters <thopeter@cisco.com>
Date:   Mon Nov 9 12:36:10 2020 -0500

    http2_inspect: improve error handling

diff --git a/src/service_inspectors/http2_inspect/http2_data_cutter.cc b/src/service_inspectors/http2_inspect/http2_data_cutter.cc
index 70cb4f7702bf27587515638e5c649a0d6c514caa..31a378159b4b015b9d9f7714ca6ceb86d7b140a2 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_data_cutter.cc
+++ b/src/service_inspectors/http2_inspect/http2_data_cutter.cc
@@ -90,15 +90,8 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt
         {
             bytes_sent_http += cur_data;
         }
-        else if (scan_result == StreamSplitter::ABORT)
-        {
-            // FIXIT-E eventually need to implement continued processing. We cannot abort just
-            // because one stream went sideways. A better approach would be to put this one stream
-            // into a pass through mode while continuing to process other streams. As long as we
-            // can parse the framing and process most streams it is reasonable to continue.
-            session_data->stream_in_hi = NO_STREAM_ID;
-            return StreamSplitter::ABORT;
-        }
+        else
+            assert(false);
     }
 
     if (data_bytes_read == data_len)
@@ -172,10 +165,11 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len)
                     reassemble_state = GET_PADDING_LEN;
                 else if (reassemble_data_len > 0)
                     reassemble_state = SEND_DATA;
-                else if (frame_flags & END_STREAM)
-                    reassemble_state = SEND_EMPTY_DATA;
                 else
-                    reassemble_state = GET_FRAME_HDR;
+                {
+                    assert(frame_flags & END_STREAM);
+                    reassemble_state = SEND_EMPTY_DATA;
+                }
             }
             break;
           }
@@ -187,10 +181,11 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len)
             cur_data_offset++;
             if (reassemble_data_len > 0)
                 reassemble_state = SEND_DATA;
-            else if (get_frame_flags(session_data->lead_frame_header[source_id]) & END_STREAM)
-                reassemble_state = SEND_EMPTY_DATA;
             else
-                reassemble_state = GET_FRAME_HDR;
+            {
+                assert(get_frame_flags(session_data->lead_frame_header[source_id]) & END_STREAM);
+                reassemble_state = SEND_EMPTY_DATA;
+            }
             break;
           }
         case SEND_EMPTY_DATA:

diff --git a/src/service_inspectors/http2_inspect/http2_frame.cc b/src/service_inspectors/http2_inspect/http2_frame.cc
index b18ad1236672fc4183d0f3b3f289d88c48607d7d..05be984ff5884f1026e5d6633c1596dec13e4e7f 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_frame.cc
@@ -46,6 +46,8 @@ Http2Frame::Http2Frame(const uint8_t* header_buffer, const uint32_t header_len,
     // FIXIT-E want to refactor so that zero-length frames are not a special case
     if (data_buffer != nullptr)
         data.set(data_len, data_buffer, true);
+    else
+        data.set(0, new uint8_t[0], true);
 }
 
 Http2Frame* Http2Frame::new_frame(const uint8_t* header, const uint32_t header_len,

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
index 70d0b1f75f18f4c30c42dad9d20cbe983285895a..c3e40bbdb03654f397f7e806a1cc9e36cb316955 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
@@ -45,13 +45,6 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_
     HttpCommon::SourceId source_id_, Http2Stream* stream_) :
     Http2Frame(header_buffer, header_len, data_buffer, data_len, session_data_, source_id_, stream_)
 {
-    // FIXIT-E zero length should not be a special case
-    if (data.length() <= 0)
-    {
-        process_frame = false;
-        return;
-    }
-
     // Remove stream dependency if present
     if (get_flags() & PRIORITY)
         hpack_headers_offset = 5;
@@ -146,7 +139,7 @@ const Field& Http2HeadersFrame::get_buf(unsigned id)
 {
     switch (id)
     {
-    // FIXIT-M need to add a buffer for the decoded start line
+    // FIXIT-E need to add a buffer for the decoded start line
     case HTTP2_BUFFER_DECODED_HEADER:
         return http1_header;
     default:

diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.cc b/src/service_inspectors/http2_inspect/http2_settings_frame.cc
index 8021fcb2234ea2f949cd94a657c26829449be619..b0144f021d5379947bbbbb5823b8cfcd07fc7a49 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_settings_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_settings_frame.cc
@@ -94,7 +94,7 @@ bool Http2SettingsFrame::sanity_check()
     // FIXIT-E this next check should possibly be moved to valid_sequence()
     if (get_stream_id() != 0)
         bad_frame = true;
-    else if (!ack and ((data.length() % 6) != 0))
+    else if (!ack and ((data.length() <= 0) or ((data.length() % 6) != 0)))
         bad_frame = true;
     else if (ack and data.length() > 0)
         bad_frame = true;

diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc
index 599b19f06c0fc76555f06c4be9d0b6f1bdb65dc6..5aed057ecc4a7c68c2d29dc7ec0972d7e958a7da 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream.cc
@@ -90,6 +90,8 @@ void Http2Stream::set_state(HttpCommon::SourceId source_id, StreamState new_stat
 {
     assert((STREAM_EXPECT_HEADERS <= new_state) && (new_state <= STREAM_ERROR));
     assert(state[source_id] < new_state);
+    assert((new_state < STREAM_EXPECT_BODY) || (new_state > STREAM_BODY) ||
+        (get_hi_flow_data() != nullptr));
     state[source_id] = new_state;
 }
 

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
index e92f352aa5ef96202fe60c25b6230e38af030d66..59dfcd1f0c5b3ebaa864861d0495aef02345ee1f 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
@@ -263,31 +263,3 @@ bool Http2StreamSplitter::finish(Flow* flow)
     return need_reassemble;
 }
 
-bool Http2StreamSplitter::init_partial_flush(Flow* flow)
-{
-    Profile profile(Http2Module::get_profile_stats());
-
-    if (source_id != SRC_SERVER)
-    {
-        assert(false);
-        return false;
-    }
-
-    Http2FlowData* session_data = (Http2FlowData*)flow->get_flow_data(Http2FlowData::inspector_id);
-    assert(session_data != nullptr);
-    if (session_data->abort_flow[source_id])
-        return false;
-
-#ifdef REG_TEST
-    if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2) &&
-        !HttpTestManager::use_test_input(HttpTestManager::IN_HTTP2))
-    {
-        printf("HTTP/2 partial flush from flow data %" PRIu64 "\n", session_data->seq_num);
-        fflush(stdout);
-    }
-#endif
-
-    // FIXIT-E not supported yet
-    return false;
-}
-

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.h b/src/service_inspectors/http2_inspect/http2_stream_splitter.h
index 3ce51ea63b6a067798fbf74d2617fe44d83fba89..de8d04c7e349bcf63c9fea9de9099481cb8f61d2 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.h
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.h
@@ -40,7 +40,6 @@ public:
     const snort::StreamBuffer reassemble(snort::Flow* flow, unsigned total, unsigned offset, const
         uint8_t* data, unsigned len, uint32_t flags, unsigned& copied) override;
     bool finish(snort::Flow* flow) override;
-    bool init_partial_flush(snort::Flow* flow) override;
     bool is_paf() override { return true; }
 
     // FIXIT-M should return actual packet buffer size
@@ -51,9 +50,6 @@ private:
     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 StreamSplitter::Status non_data_frame_header_checks(
-        Http2FlowData* session_data, HttpCommon::SourceId source_id, uint32_t frame_length,
-        uint8_t type);
     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);

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 4e0b592a8998da2be4f6c3e63234e80d7fc20d6d..018e66e79329ee8c7ee7cf6b5f6defce8389db5c 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
@@ -71,66 +71,34 @@ StreamSplitter::Status Http2StreamSplitter::data_frame_header_checks(Http2FlowDa
 
     if (!stream || !stream->is_open(source_id))
     {
-        *session_data->infractions[source_id] += INF_FRAME_SEQUENCE;
-        session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
-        // FIXIT-E We should not be aborting here
-        return StreamSplitter::ABORT;
-    }
-
-    HttpFlowData* const http_flow = (HttpFlowData*)stream->get_hi_flow_data();
-    if (http_flow == nullptr)
-    {
-        *session_data->infractions[source_id] += INF_FRAME_SEQUENCE;
-        session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
-        return StreamSplitter::ABORT;
-    }
-
-    if (http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2)
-    {
-        // If 0 length frame and http isn't expecting body, flush without involving http
-        if (frame_length == 0)
+        if (!(stream && (stream->get_state(source_id) == STREAM_ERROR)))
         {
-            *flush_offset = data_offset;
-            session_data->header_octets_seen[source_id] = 0;
-            session_data->scan_state[source_id] = SCAN_FRAME_HEADER;
-            return StreamSplitter::FLUSH;
+            *session_data->infractions[source_id] += INF_FRAME_SEQUENCE;
+            session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
+            if (stream)
+            {
+                // FIXIT-E need to do this even if the stream does not exist yet
+                stream->set_state(source_id, STREAM_ERROR);
+            }
         }
-
-        *session_data->infractions[source_id] += INF_FRAME_SEQUENCE;
-        session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
-        // FIXIT-E We should not be aborting here
-        return StreamSplitter::ABORT;
+        return StreamSplitter::SEARCH;
     }
 
-    if (frame_length > MAX_OCTETS)
-        return StreamSplitter::ABORT;
-
-    return StreamSplitter::SEARCH;
-}
-
-StreamSplitter::Status Http2StreamSplitter::non_data_frame_header_checks(
-    Http2FlowData* session_data, HttpCommon::SourceId source_id, uint32_t frame_length,
-    uint8_t type)
-{
-    // Compute frame section length once per frame
-    if (frame_length + FRAME_HEADER_LENGTH > MAX_OCTETS)
-    {
-        // FIXIT-M long non-data frame needs to be supported
-        return StreamSplitter::ABORT;
-    }
+    HttpFlowData* const http_flow = (HttpFlowData*)stream->get_hi_flow_data();
+    assert(http_flow != nullptr);
 
-    if (type == FT_CONTINUATION and !session_data->continuation_expected[source_id])
+    // 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))
     {
-        *session_data->infractions[source_id] += INF_UNEXPECTED_CONTINUATION;
-        session_data->events[source_id]->create_event(
-            EVENT_UNEXPECTED_CONTINUATION);
-        return StreamSplitter::ABORT;
+        *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,
     uint32_t length, uint32_t* flush_offset, HttpCommon::SourceId source_id, uint8_t type,
     uint8_t frame_flags, uint32_t& data_offset)
@@ -258,12 +226,26 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio
                     memcpy(session_data->lead_frame_header[source_id],
                         session_data->scan_frame_header[source_id], FRAME_HEADER_LENGTH);
                 }
+                else if (!session_data->continuation_expected[source_id])
+                {
+                    *session_data->infractions[source_id] += INF_UNEXPECTED_CONTINUATION;
+                    session_data->events[source_id]->create_event(EVENT_UNEXPECTED_CONTINUATION);
+                    return StreamSplitter::ABORT;
+                }
+
                 const uint32_t frame_length = get_frame_length(session_data->
                     scan_frame_header[source_id]);
                 session_data->frame_lengths[source_id].push(frame_length);
                 const uint8_t frame_flags = get_frame_flags(session_data->
                     scan_frame_header[source_id]);
 
+                if ((type != FT_DATA) && (frame_length + FRAME_HEADER_LENGTH > MAX_OCTETS))
+                {
+                    // FIXIT-E long non-data frames may need to be supported
+                    // FIXIT-E need an alert and infraction
+                    return StreamSplitter::ABORT;
+                }
+
                 assert(session_data->scan_remaining_frame_octets[source_id] == 0);
                 session_data->scan_remaining_frame_octets[source_id] = frame_length;
 
@@ -300,9 +282,6 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio
                     status = data_frame_header_checks(session_data, flush_offset, source_id,
                         frame_length, data_offset);
                 }
-                else
-                    status = non_data_frame_header_checks(session_data, source_id, frame_length,
-                        type);
 
                 break;
             }
@@ -340,16 +319,44 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio
                     session_data->scan_frame_header[source_id]);
                 const uint8_t frame_flags = get_frame_flags(session_data->
                     scan_frame_header[source_id]);
-                if (session_data->frame_type[source_id] == FT_DATA)
+                if (session_data->frame_type[source_id] != FT_DATA)
                 {
-                    status = session_data->data_cutter[source_id].scan(
-                        data, length, flush_offset, data_offset,
-                        frame_length - session_data->padding_length[source_id], frame_flags);
+                    status = non_data_scan(session_data, length, flush_offset, source_id, type,
+                        frame_flags, data_offset);
                 }
                 else
                 {
-                    status = non_data_scan(session_data, length, flush_offset, source_id, type,
-                        frame_flags, data_offset);
+                    Http2Stream* const stream =
+                        session_data->find_stream(session_data->current_stream[source_id]);
+                    if (stream && stream->is_open(source_id))
+                    {
+                        status = session_data->data_cutter[source_id].scan(
+                            data, length, flush_offset, data_offset,
+                            frame_length - session_data->padding_length[source_id], frame_flags);
+                    }
+                    else
+                    {
+                        // Need to skip past Data frame in a bad stream
+                        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;
+                            assert(!session_data->frame_lengths[source_id].empty());
+                            session_data->frame_lengths[source_id].pop();
+                            assert(session_data->frame_lengths[source_id].empty());
+                        }
+
+                        session_data->payload_discard[source_id] = true;
+                        status = StreamSplitter::FLUSH;
+                    }
                 }
                 assert(status != StreamSplitter::SEARCH or
                     session_data->scan_state[source_id] != SCAN_EMPTY_DATA);
@@ -381,8 +388,6 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess
     {
         if (offset == 0)
         {
-            session_data->frame_header_offset[source_id] = 0;
-
             // This is the first reassemble() for this frame - allocate data buffer
             session_data->frame_data_size[source_id] =
                 total - (session_data->frame_lengths[source_id].size() * FRAME_HEADER_LENGTH);