From: Mike Stepanek (mstepane) Date: Tue, 15 Sep 2020 13:50:37 +0000 (+0000) Subject: Merge pull request #2473 in SNORT/snort3 from ~THOPETER/snort3:h2i3 to master X-Git-Tag: 3.0.3-1~23 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=97fa912e673146e5128d4853225766af6a50f111;p=thirdparty%2Fsnort3.git Merge pull request #2473 in SNORT/snort3 from ~THOPETER/snort3:h2i3 to master Squashed commit of the following: commit 4915334804e793384139ea575b935a12988ac21c Author: Tom Peters 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 8bee8e13a..ba57bfab4 100644 --- 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 44ef7b7b4..286077501 100644 --- 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 3520ed99d..4d01db283 100644 --- 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 9fe27e725..526d2beae 100644 --- 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 00fbe1283..b7dddc5a5 100644 --- 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 3830f7b1d..09f942f83 100644 --- 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 e08ae1f65..1cfa56101 100644 --- 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 2ae810eee..3aec9cd68 100644 --- 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 b567b26c3..c92808aaa 100644 --- 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))