From: Tom Peters (thopeter) Date: Fri, 29 Oct 2021 19:33:28 +0000 (+0000) Subject: Merge pull request #3135 in SNORT/snort3 from ~KATHARVE/snort3:hpack-refactor to... X-Git-Tag: 3.1.16.0~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8788c7c07328e168213a74ccedf403d1b3f65cb4;p=thirdparty%2Fsnort3.git Merge pull request #3135 in SNORT/snort3 from ~KATHARVE/snort3:hpack-refactor to master Squashed commit of the following: commit c6891e039474b8ce2b2c0f318fe2dd053bac550b Author: Katura Harvey Date: Wed Oct 27 10:41:52 2021 -0400 http2_inspect: refactor decoded_headers_buffer for hpack decoding --- diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc index 6dad237f4..dcfa41049 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc @@ -51,13 +51,12 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_ // Set up HPACK decoding hpack_decoder = &session_data->hpack_decoder[source_id]; - decoded_headers = new uint8_t[MAX_OCTETS]; } Http2HeadersFrame::~Http2HeadersFrame() { - delete[] decoded_headers; + hpack_decoder->cleanup(); } void Http2HeadersFrame::clear() @@ -74,10 +73,9 @@ void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow, SourceI if (session_data->abort_flow[source_id]) return; - http1_header = hpack_decoder->get_decoded_headers(decoded_headers); + hpack_decoder->set_decoded_headers(http1_header); StreamBuffer stream_buf; - // http_inspect scan() of headers // If we're processing a header truncated immediately after the start line, http1_header will // be empty. Don't call scan on the empty buffer because it will create a cutter and the check diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h index 8a1b74eae..08f6b85d1 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.h +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h @@ -50,7 +50,6 @@ protected: void process_decoded_headers(HttpFlowData* http_flow, HttpCommon::SourceId hi_source_id); uint8_t get_flags_mask() const override; - uint8_t* decoded_headers = nullptr; // working buffer to store decoded headers Field http1_header; // finalized headers to be passed to NHI uint32_t xtradata_mask = 0; bool detection_required = false; 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 a86514221..e48db69e6 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc @@ -57,7 +57,7 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, 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, decoded_headers, start_line_generator, false)) + encoded_headers_length, start_line_generator, false)) { if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) { 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 e90a879b4..ccf9d3b67 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc @@ -59,7 +59,7 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer, 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, decoded_headers, nullptr, true)) + encoded_headers_length, nullptr, true)) { if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) { diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc index 6d23cd12d..f9c9c2b11 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack.cc +++ b/src/service_inspectors/http2_inspect/http2_hpack.cc @@ -370,7 +370,7 @@ bool Http2HpackDecoder::decode_header_line(const uint8_t* encoded_header_buffer, // does not output the start line or decoded headers - this function must be followed by calls to // generate_start_line() and get_decoded_headers() to generate and obtain these fields. bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers, - const uint32_t encoded_headers_length, uint8_t* decoded_headers, + const uint32_t encoded_headers_length, Http2StartLine *start_line_generator, bool trailers) { uint32_t total_bytes_consumed = 0; @@ -378,7 +378,7 @@ bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers, uint32_t line_bytes_written = 0; bool success = true; start_line = start_line_generator; - decoded_headers_size = 0; + decoded_headers = new uint8_t[MAX_OCTETS]; is_trailers = trailers; pseudo_headers_allowed = !is_trailers; @@ -425,7 +425,24 @@ void Http2HpackDecoder::settings_table_size_update(uint32_t new_size) decode_table.get_dynamic_table().update_size(new_size); } -Field Http2HpackDecoder::get_decoded_headers(const uint8_t* const decoded_headers) +void Http2HpackDecoder::set_decoded_headers(Field& http1_header) +{ + assert(decoded_headers); + http1_header.set(decoded_headers_size, decoded_headers, true); + + // The headers frame object now owns this buffer + decoded_headers = nullptr; + decoded_headers_size = 0; +} + +void Http2HpackDecoder::cleanup() { - return Field(decoded_headers_size, decoded_headers, false); + 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; + } } diff --git a/src/service_inspectors/http2_inspect/http2_hpack.h b/src/service_inspectors/http2_inspect/http2_hpack.h index 1a0b5f90a..9d0f8c82d 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack.h +++ b/src/service_inspectors/http2_inspect/http2_hpack.h @@ -46,7 +46,7 @@ public: session_data(flow_data), events(_events), infractions(_infractions), source_id(src_id), decode_table(flow_data) { } bool decode_headers(const uint8_t* encoded_headers, const uint32_t encoded_headers_length, - uint8_t* decoded_headers, Http2StartLine* start_line, bool trailers); + Http2StartLine* start_line, bool trailers); bool write_decoded_headers(const uint8_t* in_buffer, const uint32_t in_length, uint8_t* decoded_header_buffer, uint32_t decoded_header_length, uint32_t& bytes_written); bool decode_header_line(const uint8_t* encoded_header_buffer, @@ -80,15 +80,16 @@ public: uint32_t& bytes_written, Field& field); bool finalize_start_line(); - const Field* get_start_line(); - Field get_decoded_headers(const uint8_t* const decoded_headers); + void set_decoded_headers(Field&); bool are_pseudo_headers_allowed() { return pseudo_headers_allowed; } void settings_table_size_update(const uint32_t size); + void cleanup(); private: Http2StartLine* start_line; bool pseudo_headers_allowed; - uint32_t decoded_headers_size; + uint8_t* decoded_headers = nullptr; // working buffer to store decoded headers + uint32_t decoded_headers_size = 0; Http2FlowData* session_data; Http2EventGen* const events; Http2Infractions* const infractions; 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 f5afb6ecb..89b7841e0 100644 --- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc @@ -67,7 +67,7 @@ Http2PushPromiseFrame::Http2PushPromiseFrame(const uint8_t* header_buffer, // Decode headers if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() - - hpack_headers_offset, decoded_headers, start_line_generator, false)) + hpack_headers_offset, start_line_generator, false)) { if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE)) {