From: Tom Peters (thopeter) Date: Fri, 5 Nov 2021 19:38:52 +0000 (+0000) Subject: Pull request #3138: Hpack refactor2 X-Git-Tag: 3.1.17.0~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c8db0b040a16abe3ff28f0d45a8ccb97b12cbb7e;p=thirdparty%2Fsnort3.git Pull request #3138: Hpack refactor2 Merge in SNORT/snort3 from ~KATHARVE/snort3:hpack-refactor2 to master Squashed commit of the following: commit 3649ca44dbce29d22cbd296556816658d4f00b25 Author: Katura Harvey Date: Thu Oct 28 16:59:41 2021 -0400 http2_inspect: http1_header buffer always created immediately after decode_headers --- diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc index dcfa41049..c451f7c0c 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc @@ -53,12 +53,6 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_ hpack_decoder = &session_data->hpack_decoder[source_id]; } - -Http2HeadersFrame::~Http2HeadersFrame() -{ - hpack_decoder->cleanup(); -} - void Http2HeadersFrame::clear() { if (session_data->abort_flow[source_id] || stream->get_state(source_id) == STREAM_ERROR) @@ -68,12 +62,39 @@ void Http2HeadersFrame::clear() session_data->hi->clear(&dummy_pkt); } +bool Http2HeadersFrame::decode_headers(Http2StartLine* start_line_generator, bool trailers) +{ + const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ? + data.length() - hpack_headers_offset : 0; + if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), + encoded_headers_length, start_line_generator, trailers)) + { + if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) + { + session_data->abort_flow[source_id] = true; + session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2); + http1_header.set(STAT_PROBLEMATIC); + hpack_decoder->cleanup(); + return false; + } + } + hpack_decoder->set_decoded_headers(http1_header); + return true; +} + void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow, SourceId hi_source_id) { - if (session_data->abort_flow[source_id]) + if (session_data->abort_flow[source_id] or http1_header.length() < 0) return; - hpack_decoder->set_decoded_headers(http1_header); + if (http1_header.length() <= 0 and !session_data->is_processing_partial_header()) + { + // This shouldn't happen because well-formatted empty frames have crlf written to the + // decoded headers buffer + assert(false); + return; + } + StreamBuffer stream_buf; // http_inspect scan() of headers @@ -81,7 +102,6 @@ void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow, SourceI // be empty. Don't call scan on the empty buffer because it will create a cutter and the check // for this condition in HI::finish() will fail. Truncated headers with non-empty http1_header // buffers are still sent to HI::scan(). - assert ((http1_header.length() > 0) or session_data->is_processing_partial_header()); if (http1_header.length() > 0) { uint32_t flush_offset; diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h index ce291b0c6..f71120de9 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.h +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h @@ -32,7 +32,6 @@ class HttpFlowData; class Http2HeadersFrame : public Http2Frame { public: - ~Http2HeadersFrame() override; void clear() override; const Field& get_buf(unsigned id) override; @@ -47,6 +46,7 @@ protected: Http2HeadersFrame(const uint8_t* header_buffer, const uint32_t header_len, const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data, HttpCommon::SourceId src_id, Http2Stream* stream); + bool decode_headers(Http2StartLine* start_line_generator, bool trailers); void process_decoded_headers(HttpFlowData* http_flow, HttpCommon::SourceId hi_source_id); uint8_t get_flags_mask() const override; 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 e48db69e6..e45bbdde1 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc @@ -53,25 +53,14 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, start_line_generator = new Http2StatusLine(session_data->events[source_id], session_data->infractions[source_id]); - // Decode headers - const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ? - data.length() - hpack_headers_offset : 0; - if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), - encoded_headers_length, start_line_generator, false)) + if (decode_headers(start_line_generator, false)) { - if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) + // process start line + if (!start_line_generator->generate_start_line(start_line, are_pseudo_headers_complete())) { - session_data->abort_flow[source_id] = true; - session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2); - return; + stream->set_state(source_id, STREAM_ERROR); } } - - // process start line - if (!start_line_generator->generate_start_line(start_line, are_pseudo_headers_complete())) - { - stream->set_state(source_id, STREAM_ERROR); - } } bool Http2HeadersFrameHeader::valid_sequence(Http2Enums::StreamState state) 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 ccf9d3b67..8638dad39 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc @@ -54,19 +54,7 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer, *session_data->infractions[source_id] += INF_TRAILERS_NOT_END; session_data->events[source_id]->create_event(EVENT_TRAILERS_NOT_END); } - - // Decode trailers - const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ? - data.length() - hpack_headers_offset : 0; - if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), - encoded_headers_length, nullptr, true)) - { - if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) - { - session_data->abort_flow[source_id] = true; - session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2); - } - } + decode_headers(nullptr, true); } bool Http2HeadersFrameTrailer::valid_sequence(Http2Enums::StreamState state) diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc index f9c9c2b11..5e5599a23 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack.cc +++ b/src/service_inspectors/http2_inspect/http2_hpack.cc @@ -437,12 +437,7 @@ void Http2HpackDecoder::set_decoded_headers(Field& http1_header) void Http2HpackDecoder::cleanup() { - if (decoded_headers) - { - // get_decoded_headers() was never called because we encountered some error during - // processing. We must clean up the buffer. - delete[] decoded_headers; - decoded_headers = nullptr; - decoded_headers_size = 0; - } + delete[] decoded_headers; + decoded_headers = nullptr; + decoded_headers_size = 0; } 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 89b7841e0..a47578d54 100644 --- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc @@ -65,16 +65,7 @@ Http2PushPromiseFrame::Http2PushPromiseFrame(const uint8_t* header_buffer, hpack_headers_offset += PROMISED_ID_LENGTH; - // Decode headers - if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() - - hpack_headers_offset, start_line_generator, false)) - { - if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) - { - session_data->abort_flow[source_id] = true; - session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2); - } - } + decode_headers(start_line_generator, false); } bool Http2PushPromiseFrame::valid_sequence(Http2Enums::StreamState)