Merge pull request #2833 in SNORT/snort3 from ~MDAGON/snort3:oom to master

Squashed commit of the following:

commit ff5bc520f269912b3589fbe4adb1cab946ad9775
Author: Maya Dagon <mdagon@cisco.com>
Date:   Thu Apr 1 15:55:16 2021 -0400

    http2_inspect: free streams in completed/error state

diff --git a/src/payload_injector/test/payload_injector_test.cc b/src/payload_injector/test/payload_injector_test.cc
index 37ed729b380f43c3efdeb398eb545f73d862e91c..34f341d489d4ef94a7cd99d1e5922ece3c2578b3 100644 (file)
--- a/src/payload_injector/test/payload_injector_test.cc
+++ b/src/payload_injector/test/payload_injector_test.cc
@@ -147,6 +147,7 @@ Http2FlowData::Http2FlowData(snort::Flow*) :
     data_cutter {Http2DataCutter(this, SRC_CLIENT), Http2DataCutter(this, SRC_SERVER)}
 { }
 Http2FlowData::~Http2FlowData() = default;
+size_t Http2FlowData::size_of() { return 1; }
 Http2FlowData http2_flow_data(nullptr);
 void Http2FlowData::set_mid_frame(bool val) { continuation_expected[SRC_SERVER] = val; }
 bool Http2FlowData::is_mid_frame() const { return continuation_expected[SRC_SERVER]; }

diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.cc b/src/service_inspectors/http2_inspect/http2_flow_data.cc
index 22d47e322a96f32ffa56a4eff98189f2d0f08966..a12e0e917092c4adc83dee12ddafdd6dd4ff0590 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_flow_data.cc
+++ b/src/service_inspectors/http2_inspect/http2_flow_data.cc
@@ -94,6 +94,9 @@ Http2FlowData::~Http2FlowData()
         delete hi_ss[k];
         delete[] frame_data[k];
     }
+
+    for (Http2Stream* stream : streams)
+        delete stream;
 }
 
 HttpFlowData* Http2FlowData::get_hi_flow_data() const
@@ -110,41 +113,125 @@ void Http2FlowData::set_hi_flow_data(HttpFlowData* flow)
     stream->set_hi_flow_data(flow);
 }
 
-class Http2Stream* Http2FlowData::find_stream(uint32_t key) const
+size_t Http2FlowData::size_of()
+{
+    // There are MAX_CONCURRENT_STREAMS + 1 (for stream id 0).
+    // Each stream will have class Http2Stream allocated and a node in streams list
+    const size_t max_streams_size = (MAX_CONCURRENT_STREAMS + 1) *
+        (sizeof(std::_List_node<Http2Stream*>) + sizeof(class Http2Stream));
+    return sizeof(*this) + max_streams_size;
+}
+
+Http2Stream* Http2FlowData::find_stream(const uint32_t key) const
 {
-    for (const StreamInfo& stream_info : streams)
+    for (Http2Stream* stream : streams)
     {
-        if (stream_info.id == key)
-            return stream_info.stream;
+        if (stream->get_stream_id() == key)
+            return stream;
     }
 
     return nullptr;
 }
 
-class Http2Stream* Http2FlowData::get_stream(uint32_t key)
+Http2Stream* Http2FlowData::get_stream(const uint32_t key, const SourceId source_id)
 {
     class Http2Stream* stream = find_stream(key);
     if (!stream)
     {
+        if (concurrent_streams >= CONCURRENT_STREAMS_LIMIT)
+        {
+            *infractions[source_id] += INF_TOO_MANY_STREAMS;
+            events[source_id]->create_event(EVENT_TOO_MANY_STREAMS);
+            Http2Module::increment_peg_counts(PEG_FLOWS_OVER_STREAM_LIMIT);
+            abort_flow[SRC_CLIENT] = true;
+            abort_flow[SRC_SERVER] = true;
+            return nullptr;
+        }
+
+        // Verify stream id is bigger than all previous streams initiated by same side
+        if (key != 0)
+        {
+            const bool non_housekeeping_frame = frame_type[source_id] == FT_HEADERS ||
+                                                frame_type[source_id] == FT_DATA ||
+                                                frame_type[source_id] == FT_PUSH_PROMISE;
+            if (non_housekeeping_frame)
+            {
+                // If we see both sides of traffic, odd stream id should be initiated by client,
+                // even by server. If we can't see one side, can't guarantee order
+                const bool is_on_expected_side = (key % 2 != 0 && source_id == SRC_CLIENT) ||
+                                                 (key % 2 == 0 && source_id == SRC_SERVER);
+                if (is_on_expected_side)
+                {
+                    if (key <= max_stream_id[source_id])
+                    {
+                        *infractions[source_id] += INF_INVALID_STREAM_ID;
+                        events[source_id]->create_event(EVENT_INVALID_STREAM_ID);
+                        return nullptr;
+                    }
+                    else
+                        max_stream_id[source_id] = key;
+                }
+            }
+            else // housekeeping frame
+            {
+                // Delete stream after this frame is evaluated.
+                // Prevents recreating and keeping already completed streams for
+                // housekeeping frames
+                delete_stream = true;
+            }
+        }
+
+        // Allocate new stream
         stream = new Http2Stream(key, this);
-        streams.emplace_front(key, stream);
+        streams.emplace_front(stream);
+
+        // stream 0 does not count against stream limit
+        if (key > 0)
+        {
+            concurrent_streams += 1;
+            if (concurrent_streams > Http2Module::get_peg_counts(PEG_MAX_CONCURRENT_STREAMS))
+                Http2Module::increment_peg_counts(PEG_MAX_CONCURRENT_STREAMS);
+        }
     }
     return stream;
 }
 
-class Http2Stream* Http2FlowData::get_hi_stream() const
+void Http2FlowData::delete_processing_stream()
+{
+    std::list<Http2Stream*>::iterator it;
+    for (it = streams.begin(); it != streams.end(); ++it)
+    {
+        if ((*it)->get_stream_id() == processing_stream_id)
+        {
+            delete *it;
+            streams.erase(it);
+            delete_stream = false;
+            assert(concurrent_streams > 0);
+            concurrent_streams -= 1;
+            return;
+        }
+    }
+    assert(false);
+}
+
+Http2Stream* Http2FlowData::get_hi_stream() const
 {
     return find_stream(stream_in_hi);
 }
 
-class Http2Stream* Http2FlowData::get_current_stream(HttpCommon::SourceId source_id)
+Http2Stream* Http2FlowData::find_current_stream(const SourceId source_id) const
+{
+    return find_stream(current_stream[source_id]);
+}
+
+Http2Stream* Http2FlowData::find_processing_stream() const
 {
-    return get_stream(current_stream[source_id]);
+    return find_stream(get_processing_stream_id());
 }
 
-class Http2Stream* Http2FlowData::get_processing_stream()
+Http2Stream* Http2FlowData::get_processing_stream(const SourceId source_id)
 {
-    return get_stream(get_processing_stream_id());
+    return get_stream(get_processing_stream_id(), source_id);
 }
 
 uint32_t Http2FlowData::get_processing_stream_id() const
@@ -154,7 +241,7 @@ uint32_t Http2FlowData::get_processing_stream_id() const
 
 // processing stream is the current stream except for push promise frames with properly formatted
 // promised stream IDs
-void Http2FlowData::set_processing_stream_id(const HttpCommon::SourceId source_id)
+void Http2FlowData::set_processing_stream_id(const SourceId source_id)
 {
     assert(processing_stream_id == NO_STREAM_ID);
     if (frame_type[source_id] == FT_PUSH_PROMISE)
@@ -165,7 +252,7 @@ void Http2FlowData::set_processing_stream_id(const HttpCommon::SourceId source_i
         processing_stream_id = current_stream[source_id];
 }
 
-uint32_t Http2FlowData::get_current_stream_id(const HttpCommon::SourceId source_id) const
+uint32_t Http2FlowData::get_current_stream_id(const SourceId source_id) const
 {
     return current_stream[source_id];
 }

diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.h b/src/service_inspectors/http2_inspect/http2_flow_data.h
index bdb0d2133bb34c4e615dde73a185e37cc2e7f808..6ac9d555cfc853ab1b90cf80cbb54cc7bc28400a 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_flow_data.h
+++ b/src/service_inspectors/http2_inspect/http2_flow_data.h
@@ -83,22 +83,12 @@ public:
     friend class Http2StreamSplitter;
     friend void finish_msg_body(Http2FlowData* session_data, HttpCommon::SourceId source_id);
 
-    size_t size_of() override
-    { return sizeof(*this); }
-
-    // Stream access
-    class StreamInfo
-    {
-    public:
-        const uint32_t id;
-        class Http2Stream* stream;
-
-        StreamInfo(uint32_t _id, class Http2Stream* ptr) : id(_id), stream(ptr) { assert(ptr); }
-        ~StreamInfo() { delete stream; }
-    };
-    class Http2Stream* get_current_stream(const HttpCommon::SourceId source_id);
+    size_t size_of() override;
+
+    Http2Stream* find_current_stream(const HttpCommon::SourceId source_id) const;
     uint32_t get_current_stream_id(const HttpCommon::SourceId source_id) const;
-    class Http2Stream* get_processing_stream();
+    Http2Stream* get_processing_stream(const HttpCommon::SourceId source_id);
+    Http2Stream* find_processing_stream() const;
     uint32_t get_processing_stream_id() const;
     void set_processing_stream_id(const HttpCommon::SourceId source_id);
     bool is_processing_partial_header() const { return processing_partial_header; }
@@ -160,9 +150,11 @@ protected:
     bool frame_in_detection = false;
     Http2ConnectionSettings connection_settings[2];
     Http2HpackDecoder hpack_decoder[2];
-    std::list<class StreamInfo> streams;
+    std::list<Http2Stream*> streams;
     uint32_t concurrent_files = 0;
     uint32_t concurrent_streams = 0;
+    uint32_t max_stream_id[2] = {0, 0};
+    bool delete_stream = false;
 
     // Internal to scan()
     bool preface[2] = { true, false };
@@ -204,9 +196,10 @@ protected:
 #endif
 
 private:
-    class Http2Stream* get_stream(uint32_t key);
-    class Http2Stream* get_hi_stream() const;
-    class Http2Stream* find_stream(uint32_t key) const;
+    Http2Stream* get_stream(const uint32_t key, const HttpCommon::SourceId source_id);
+    Http2Stream* get_hi_stream() const;
+    Http2Stream* find_stream(const uint32_t key) const;
+    void delete_processing_stream();
 
     // When H2I allocates http_inspect flows, it bypasses the usual FlowData memory allocation
     // bookkeeping. So H2I needs to update memory allocations and deallocations itself.

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_with_startline.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_with_startline.cc
index db6f182a898c797f1549ad86f7489618c356cffd..5d7449c1a96f8c750c85b0c0615676ce57c1db56 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_with_startline.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_with_startline.cc
@@ -54,31 +54,6 @@ bool Http2HeadersFrameWithStartline::process_start_line(HttpFlowData*& http_flow
     if (session_data->abort_flow[source_id])
         return false;
 
-    if (!stream->get_hi_flow_data())
-    {
-        if (session_data->concurrent_streams < CONCURRENT_STREAMS_LIMIT)
-        {
-            session_data->concurrent_streams += 1;
-            if (session_data->concurrent_streams >
-                Http2Module::get_peg_counts(PEG_MAX_CONCURRENT_STREAMS))
-
-            {
-                Http2Module::increment_peg_counts(PEG_MAX_CONCURRENT_STREAMS);
-            }
-        }
-        else
-        {
-            *session_data->infractions[source_id] += INF_TOO_MANY_STREAMS;
-            session_data->events[source_id]->create_event(EVENT_TOO_MANY_STREAMS);
-            Http2Module::increment_peg_counts(PEG_FLOWS_OVER_STREAM_LIMIT);
-            session_data->abort_flow[SRC_CLIENT] = true;
-            session_data->abort_flow[SRC_SERVER] = true;
-            stream->set_state(SRC_CLIENT, STREAM_ERROR);
-            stream->set_state(SRC_SERVER, STREAM_ERROR);
-            return false;
-        }
-    }
-
     // http_inspect scan() of start line
     {
         uint32_t flush_offset;

diff --git a/src/service_inspectors/http2_inspect/http2_inspect.cc b/src/service_inspectors/http2_inspect/http2_inspect.cc
index 281088d11323755fe83dbe747ed4f04180b7c2d6..8aae1327f0a68ac7a59c82f0b39ef728050c9627 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_inspect.cc
+++ b/src/service_inspectors/http2_inspect/http2_inspect.cc
@@ -86,7 +86,8 @@ bool Http2Inspect::get_buf(unsigned id, Packet* p, InspectionBuffer& b)
     if (!session_data->frame_in_detection)
         return false;
 
-    Http2Stream* const stream = session_data->get_processing_stream();
+    Http2Stream* const stream = session_data->find_processing_stream();
+    assert(stream != nullptr);
     const Field& buffer = stream->get_buf(id);
     if (buffer.length() <= 0)
         return false;
@@ -128,9 +129,17 @@ void Http2Inspect::eval(Packet* p)
     }
 
     session_data->set_processing_stream_id(source_id);
-    Http2Stream* stream = session_data->get_processing_stream();
+    Http2Stream* stream = session_data->get_processing_stream(source_id);
+    if (!stream)
+    {
+        delete[] session_data->frame_data[source_id];
+        session_data->frame_data[source_id] = nullptr;
+        session_data->frame_data_size[source_id] = 0;
+        session_data->processing_stream_id = NO_STREAM_ID;
+        return;
+    }
+
     assert(session_data->processing_stream_id != NO_STREAM_ID);
-    assert(stream);
     session_data->stream_in_hi = stream->get_stream_id();
 
     Http2Module::increment_peg_counts(PEG_TOTAL_BYTES, (uint64_t)(FRAME_HEADER_LENGTH) +
@@ -183,8 +192,11 @@ void Http2Inspect::clear(Packet* p)
 
     session_data->frame_in_detection = false;
 
-    Http2Stream* stream = session_data->get_processing_stream();
+    Http2Stream* stream = session_data->find_processing_stream();
+    assert(stream != nullptr);
     stream->clear_frame();
+    if (session_data->delete_stream)
+        session_data->delete_processing_stream();
     session_data->stream_in_hi = NO_STREAM_ID;
     session_data->processing_stream_id = NO_STREAM_ID;
     session_data->processing_partial_header = false;

diff --git a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
index c5dec0a6aa4fe6359b3fdbc5014abb5109f60674..53cc5a5b4fd2d32c24a4d44531314b8abc9f6783 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
@@ -99,8 +99,9 @@ bool Http2PushPromiseFrame::valid_sequence(Http2Enums::StreamState)
     }
 
     // Alert but continue processing if invalid sequence on stream push_promise was sent on
-    Http2Stream* const stream_sent_on = session_data->get_current_stream(source_id);
-    if (stream_sent_on->get_state(SRC_CLIENT) == STREAM_EXPECT_HEADERS or
+    Http2Stream* const stream_sent_on = session_data->find_current_stream(source_id);
+    if (stream_sent_on == nullptr or
+        stream_sent_on->get_state(SRC_CLIENT) == STREAM_EXPECT_HEADERS or
         stream_sent_on->get_state(SRC_SERVER) >= STREAM_COMPLETE)
     {
         *session_data->infractions[source_id] += INF_BAD_PUSH_SEQUENCE;

diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc
index 41796f3d954b9ec5fc2f3fe7121f6d6f5962d3ff..19aaf27f190376ec1968bb9cd89f84efddf5e681 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream.cc
@@ -77,15 +77,15 @@ void Http2Stream::clear_frame()
     delete current_frame;
     current_frame = nullptr;
 
-    if ((state[SRC_CLIENT] >= STREAM_COMPLETE) && (state[SRC_SERVER] >= STREAM_COMPLETE) &&
-        (hi_flow_data != nullptr))
+    if ((state[SRC_CLIENT] >= STREAM_COMPLETE) && (state[SRC_SERVER] >= STREAM_COMPLETE))
     {
-        session_data->deallocate_hi_memory(hi_flow_data);
-        delete hi_flow_data;
-        hi_flow_data = nullptr;
-
-        assert(session_data->concurrent_streams > 0);
-        session_data->concurrent_streams -= 1;
+        if (hi_flow_data != nullptr)
+        {
+            session_data->deallocate_hi_memory(hi_flow_data);
+            delete hi_flow_data;
+            hi_flow_data = nullptr;
+        }
+        session_data->delete_stream = true;
     }
 }
 

diff --git a/src/service_inspectors/http2_inspect/http2_stream.h b/src/service_inspectors/http2_inspect/http2_stream.h
index 50b8ef741a119e16a34b8a5f7228e685de5417f7..241ed5fa7464e75f4afa2b18aeea92b95d34f429 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.h
+++ b/src/service_inspectors/http2_inspect/http2_stream.h
@@ -36,7 +36,7 @@ class Http2Stream
 public:
     Http2Stream(uint32_t stream_id, Http2FlowData* session_data_);
     ~Http2Stream();
-    uint32_t get_stream_id() { return stream_id; }
+    uint32_t get_stream_id() const { return stream_id; }
     void eval_frame(const uint8_t* header_buffer, uint32_t header_len, const uint8_t* data_buffer,
         uint32_t data_len, HttpCommon::SourceId source_id);
     void clear_frame();

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
index b08a8405ab0f782b17e08d4e45473e14ac468bce..e3bd1392983623e2e24e00e4957ae84af633048e 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
@@ -227,23 +227,23 @@ bool Http2StreamSplitter::finish(Flow* flow)
     }
 
     // Loop through all nonzero streams with open message bodies and call NHI finish()
-    for (const Http2FlowData::StreamInfo& stream_info : session_data->streams)
+    for (const Http2Stream* stream : session_data->streams)
     {
-        if ((stream_info.id == 0)                                                 ||
-            (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)                                        ||
+        if ((stream->get_stream_id() == 0)                            ||
+            (stream->get_state(source_id) >= STREAM_COMPLETE)         ||
+            (stream->get_hi_flow_data() == nullptr)                   ||
+            (stream->get_hi_flow_data()->get_type_expected(source_id)
+                != HttpEnums::SEC_BODY_H2)                            ||
             (session_data->processing_partial_header &&
-                (stream_info.id == session_data->current_stream[source_id])))
+                (stream->get_stream_id() == session_data->current_stream[source_id])))
         {
             continue;
         }
 
-        session_data->stream_in_hi = stream_info.id;
+        session_data->stream_in_hi = stream->get_stream_id();
         if (session_data->hi_ss[source_id]->finish(flow))
         {
-            assert(stream_info.id == session_data->current_stream[source_id]);
+            assert(stream->get_stream_id() == session_data->current_stream[source_id]);
             need_reassemble = true;
 #ifdef REG_TEST
             if (HttpTestManager::use_test_input(HttpTestManager::IN_HTTP2))