From: Tom Peters (thopeter) Date: Thu, 15 Apr 2021 01:08:06 +0000 (+0000) Subject: Merge pull request #2833 in SNORT/snort3 from ~MDAGON/snort3:oom to master X-Git-Tag: 3.1.4.0~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d07697fe8dcbd0a1574d8f8d13129c5dc3848b73;p=thirdparty%2Fsnort3.git Merge pull request #2833 in SNORT/snort3 from ~MDAGON/snort3:oom to master Squashed commit of the following: commit ff5bc520f269912b3589fbe4adb1cab946ad9775 Author: Maya Dagon 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 37ed729b3..34f341d48 100644 --- 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 22d47e322..a12e0e917 100644 --- 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) + 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::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 bdb0d2133..6ac9d555c 100644 --- 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 streams; + std::list 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 db6f182a8..5d7449c1a 100644 --- 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 281088d11..8aae1327f 100644 --- 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 c5dec0a6a..53cc5a5b4 100644 --- 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 41796f3d9..19aaf27f1 100644 --- 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 50b8ef741..241ed5fa7 100644 --- 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 b08a8405a..e3bd13929 100644 --- 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))