From: Mike Stepanek (mstepane) Date: Fri, 18 Sep 2020 18:47:34 +0000 (+0000) Subject: Merge pull request #2478 in SNORT/snort3 from ~THOPETER/snort3:h2i4 to master X-Git-Tag: 3.0.3-1~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=df91b194f0a83723654472043e9662de4b47b14d;p=thirdparty%2Fsnort3.git Merge pull request #2478 in SNORT/snort3 from ~THOPETER/snort3:h2i4 to master Squashed commit of the following: commit 5fb3446f7c55d1061ccda7b7566a437a08d702b7 Author: Tom Peters Date: Tue Sep 15 19:49:55 2020 -0400 http2_inspect: refactor HI interactions out of frame constructors --- diff --git a/src/pub_sub/test/pub_sub_http_event_test.cc b/src/pub_sub/test/pub_sub_http_event_test.cc index fac9e4b37..84ce96afa 100644 --- a/src/pub_sub/test/pub_sub_http_event_test.cc +++ b/src/pub_sub/test/pub_sub_http_event_test.cc @@ -37,6 +37,10 @@ using namespace snort; using namespace HttpCommon; // Stubs to make the code link +Field::Field(int32_t length, const uint8_t* start, bool own_the_buffer_) : + strt(start), len(length), own_the_buffer(own_the_buffer_) +{} + void Field::set(const Field& input) { strt = input.strt; diff --git a/src/service_inspectors/http2_inspect/http2_frame.h b/src/service_inspectors/http2_inspect/http2_frame.h index 526d2beae..73380eeb8 100644 --- a/src/service_inspectors/http2_inspect/http2_frame.h +++ b/src/service_inspectors/http2_inspect/http2_frame.h @@ -23,6 +23,8 @@ #include "service_inspectors/http_inspect/http_common.h" #include "service_inspectors/http_inspect/http_field.h" +#include "http2_enum.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, @@ -41,6 +43,9 @@ public: static Http2Frame* new_frame(const uint8_t* header_buffer, const int32_t header_len, const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data, HttpCommon::SourceId source_id, Http2Stream* stream); + virtual bool valid_sequence(Http2Enums::StreamState state) + { return state != Http2Enums::STREAM_ERROR; } + virtual void analyze_http1() { } virtual void clear() { } virtual const Field& get_buf(unsigned id); virtual uint32_t get_xtradata_mask() { return 0; } diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc index 6036abc4a..573466a53 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc @@ -66,9 +66,18 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, c // FIXIT-E set stream state to error return; } - - StreamBuffer stream_buf; - HttpFlowData* http_flow; +} + +Http2HeadersFrameHeader::~Http2HeadersFrameHeader() +{ + delete start_line; + delete start_line_generator; +} + +void Http2HeadersFrameHeader::analyze_http1() +{ + if (!process_frame || start_line == nullptr) + return; // http_inspect scan() of start line { @@ -84,6 +93,8 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, c assert((int64_t)flush_offset == start_line->length()); } + StreamBuffer stream_buf; + // http_inspect reassemble() of start line { unsigned copied; @@ -94,7 +105,8 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, c assert(copied == (unsigned)start_line->length()); } - http_flow = session_data->get_current_stream(source_id)->get_hi_flow_data(); + HttpFlowData* const http_flow = + session_data->get_current_stream(source_id)->get_hi_flow_data(); // http_inspect eval() and clear() of start line { Http2DummyPacket dummy_pkt; @@ -116,12 +128,6 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, c process_decoded_headers(http_flow); } -Http2HeadersFrameHeader::~Http2HeadersFrameHeader() -{ - delete start_line; - delete start_line_generator; -} - #ifdef REG_TEST void Http2HeadersFrameHeader::print_frame(FILE* output) { diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_header.h b/src/service_inspectors/http2_inspect/http2_headers_frame_header.h index 1bb0b3d4e..28d4778f0 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.h +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.h @@ -28,11 +28,13 @@ class Http2StartLine; class Http2HeadersFrameHeader : public Http2HeadersFrame { public: - ~Http2HeadersFrameHeader(); + ~Http2HeadersFrameHeader() override; friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const int32_t, const uint8_t*, const int32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream); + void analyze_http1() override; + #ifdef REG_TEST void print_frame(FILE* output) override; #endif diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc index 2c539ab93..622af5138 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc @@ -48,11 +48,24 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer, if (!process_frame) return; - StreamBuffer stream_buf; - HttpFlowData* http_flow; + // Decode trailers + if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() - + hpack_headers_offset, decoded_headers, nullptr, true)) + { + session_data->frame_type[source_id] = FT__ABORT; + error_during_decode = true; + } +} - http_flow = session_data->get_current_stream(source_id)->get_hi_flow_data(); +void Http2HeadersFrameTrailer::analyze_http1() +{ + if (!process_frame) + return; + + HttpFlowData* const http_flow = + session_data->get_current_stream(source_id)->get_hi_flow_data(); assert(http_flow); + if (http_flow->get_type_expected(source_id) != HttpEnums::SEC_TRAILER) { // If there was no unflushed data on this stream when the trailers arrived, http_inspect @@ -62,7 +75,8 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer, stream->finish_msg_body(source_id, true, true); // calls http_inspect scan() unsigned copied; - stream_buf = session_data->hi_ss[source_id]->reassemble(session_data->flow, + const StreamBuffer stream_buf = + session_data->hi_ss[source_id]->reassemble(session_data->flow, 0, 0, nullptr, 0, PKT_PDU_TAIL, copied); assert(stream_buf.data != nullptr); assert(copied == 0); @@ -82,14 +96,6 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer, session_data->hi->clear(&dummy_pkt); } - // Decode headers - if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() - - hpack_headers_offset, decoded_headers, nullptr, true)) - { - session_data->frame_type[source_id] = FT__ABORT; - error_during_decode = true; - } - process_decoded_headers(http_flow); } diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h index d8b0aca0f..1c12ba6bb 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h @@ -29,6 +29,8 @@ public: friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const int32_t, const uint8_t*, const int32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream); + void analyze_http1() override; + #ifdef REG_TEST void print_frame(FILE* output) override; #endif diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc index 1cfa56101..44ffc05b6 100644 --- a/src/service_inspectors/http2_inspect/http2_stream.cc +++ b/src/service_inspectors/http2_inspect/http2_stream.cc @@ -57,6 +57,9 @@ void Http2Stream::eval_frame(const uint8_t* header_buffer, int32_t header_len, assert(current_frame == nullptr); current_frame = Http2Frame::new_frame(header_buffer, header_len, data_buffer, data_len, session_data, source_id, this); + valid_frame_order[source_id] = valid_frame_order[source_id] && + current_frame->valid_sequence(state[source_id]); + current_frame->analyze_http1(); current_frame->update_stream_state(); } diff --git a/src/service_inspectors/http2_inspect/http2_stream.h b/src/service_inspectors/http2_inspect/http2_stream.h index 3aec9cd68..33413687b 100644 --- a/src/service_inspectors/http2_inspect/http2_stream.h +++ b/src/service_inspectors/http2_inspect/http2_stream.h @@ -78,6 +78,7 @@ public: private: const uint32_t stream_id; Http2FlowData* const session_data; + bool valid_frame_order[2] = { true, true }; Http2Frame* current_frame = nullptr; HttpFlowData* hi_flow_data = nullptr; HttpMsgSection* hi_msg_section = nullptr; diff --git a/src/service_inspectors/http_inspect/http_field.cc b/src/service_inspectors/http_inspect/http_field.cc index a174af2d3..2fe479860 100644 --- a/src/service_inspectors/http_inspect/http_field.cc +++ b/src/service_inspectors/http_inspect/http_field.cc @@ -32,6 +32,13 @@ using namespace HttpEnums; const Field Field::FIELD_NULL { STAT_NO_SOURCE }; +Field::Field(int32_t length, const uint8_t* start, bool own_the_buffer_) : + strt(start), len(length), own_the_buffer(own_the_buffer_) +{ + assert(!((start == nullptr) && (length > 0))); + assert(!((start != nullptr) && (length < 0))); +} + void Field::set(int32_t length, const uint8_t* start, bool own_the_buffer_) { assert(len == STAT_NOT_COMPUTE); diff --git a/src/service_inspectors/http_inspect/http_field.h b/src/service_inspectors/http_inspect/http_field.h index 193ab8357..9d42f957c 100644 --- a/src/service_inspectors/http_inspect/http_field.h +++ b/src/service_inspectors/http_inspect/http_field.h @@ -35,8 +35,7 @@ class Field public: static const Field FIELD_NULL; - Field(int32_t length, const uint8_t* start, bool own_the_buffer_ = false) : strt(start), - len(length), own_the_buffer(own_the_buffer_) { } + Field(int32_t length, const uint8_t* start, bool own_the_buffer_ = false); explicit Field(int32_t length) : len(length) { assert(length<=0); } Field() = default; ~Field() { if (own_the_buffer) delete[] strt; }