From: Tom Peters (thopeter) Date: Fri, 23 Jul 2021 15:09:47 +0000 (+0000) Subject: Merge pull request #2978 in SNORT/snort3 from ~KATHARVE/snort3:h2i_hpack_fix to master X-Git-Tag: 3.1.9.0~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fe0b514169b03f458ee6d4eb498a59239e07ab69;p=thirdparty%2Fsnort3.git Merge pull request #2978 in SNORT/snort3 from ~KATHARVE/snort3:h2i_hpack_fix to master Squashed commit of the following: commit 2001a8e9a9d3fdb5417ae1b3d24aebc5806f07f0 Author: Katura Harvey Date: Wed Jul 7 16:49:55 2021 -0400 http2_inspect: fix HPACK dynamic table size update management --- diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h index e4fedee86..82b61a334 100644 --- a/src/service_inspectors/http2_inspect/http2_enum.h +++ b/src/service_inspectors/http2_inspect/http2_enum.h @@ -89,6 +89,9 @@ enum EventSid EVENT_INVALID_WINDOW_UPDATE_FRAME = 31, EVENT_WINDOW_UPDATE_FRAME_ZERO_INCREMENT = 32, EVENT_REQUEST_WITHOUT_METHOD = 33, + EVENT_TABLE_SIZE_UPDATE_NOT_AT_HEADER_START = 34, + EVENT_MORE_THAN_2_TABLE_SIZE_UPDATES = 35, + EVENT_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX = 36, EVENT__MAX_VALUE }; @@ -117,10 +120,10 @@ enum Infraction INF_INVALID_SETTINGS_FRAME = 18, INF_SETTINGS_FRAME_UNKN_PARAM = 19, INF_FRAME_SEQUENCE = 20, - INF_INVALID_TABLE_SIZE_UPDATE = 21, + INF_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX = 21, INF_DYNAMIC_TABLE_OVERFLOW = 22, - INF_TABLE_SIZE_UPDATE_WITHIN_HEADER = 23, - INF_TOO_MANY_TABLE_SIZE_UPDATES = 24, + INF_TABLE_SIZE_UPDATE_NOT_AT_HEADER_START = 23, + INF_MORE_THAN_2_TABLE_SIZE_UPDATES = 24, INF_INVALID_STARTLINE = 25, INF_INVALID_HEADER = 26, INF_PADDING_LEN = 27, diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.h b/src/service_inspectors/http2_inspect/http2_flow_data.h index 1c5bd6be6..d17c6a9fb 100644 --- a/src/service_inspectors/http2_inspect/http2_flow_data.h +++ b/src/service_inspectors/http2_inspect/http2_flow_data.h @@ -98,7 +98,7 @@ public: { return &hpack_decoder[source_id]; } Http2ConnectionSettings* get_my_connection_settings(const HttpCommon::SourceId source_id) { return &connection_settings[source_id]; } - Http2ConnectionSettings* get_recipient_connection_settings(const HttpCommon::SourceId source_id) + Http2ConnectionSettings* get_remote_connection_settings(const HttpCommon::SourceId source_id) { return &connection_settings[1 - source_id]; } // Used by payload injection to determine whether we are at a safe place to insert our own diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc index 9ab8fbe90..6d23cd12d 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack.cc +++ b/src/service_inspectors/http2_inspect/http2_hpack.cc @@ -255,6 +255,7 @@ bool Http2HpackDecoder::handle_dynamic_size_update(const uint8_t* encoded_header { uint64_t decoded_int; uint32_t encoded_bytes_consumed; + bool valid_update = true; bytes_consumed = 0; if (!decode_int5.translate(encoded_header_buffer, encoded_header_length, @@ -266,24 +267,31 @@ bool Http2HpackDecoder::handle_dynamic_size_update(const uint8_t* encoded_header bytes_consumed += encoded_bytes_consumed; // Table size update shenanigans are dangerous because we cannot be sure how the target will - // interpret them. + // interpret them. We generate alerts for illegal updates, but then ignore them and attempt to + // keep processing if (!table_size_update_allowed) { - *infractions += INF_TABLE_SIZE_UPDATE_WITHIN_HEADER; - return false; + *infractions += INF_TABLE_SIZE_UPDATE_NOT_AT_HEADER_START; + events->create_event(EVENT_TABLE_SIZE_UPDATE_NOT_AT_HEADER_START); + valid_update = false; } if (++num_table_size_updates > 2) { - *infractions += INF_TOO_MANY_TABLE_SIZE_UPDATES; - return false; + *infractions += INF_MORE_THAN_2_TABLE_SIZE_UPDATES; + events->create_event(EVENT_MORE_THAN_2_TABLE_SIZE_UPDATES); + valid_update = false; } - - if (!decode_table.hpack_table_size_update(decoded_int)) + if (decoded_int > session_data->get_remote_connection_settings(source_id)-> + get_param(SFID_HEADER_TABLE_SIZE)) { - *infractions += INF_INVALID_TABLE_SIZE_UPDATE; - return false; + *infractions += INF_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX; + events->create_event(EVENT_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX); + valid_update = false; } + if (valid_update) + decode_table.get_dynamic_table().update_size(decoded_int); + return true; } @@ -402,6 +410,21 @@ bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers, return success; } +void Http2HpackDecoder::settings_table_size_update(uint32_t new_size) +{ + if (new_size < decode_table.get_dynamic_table().get_max_size()) + { + // If the Settings parameter table size update changed the size of the table, an HPACK + // table size update is required. Remember the lowest value and validate we see a dynamic + // size update no greater than it. + expect_table_size_update = true; + if (new_size < min_settings_table_size_received) + min_settings_table_size_received = new_size; + } + + decode_table.get_dynamic_table().update_size(new_size); +} + Field Http2HpackDecoder::get_decoded_headers(const uint8_t* const decoded_headers) { return Field(decoded_headers_size, decoded_headers, false); diff --git a/src/service_inspectors/http2_inspect/http2_hpack.h b/src/service_inspectors/http2_inspect/http2_hpack.h index d545eb41c..07ea8ed6c 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack.h +++ b/src/service_inspectors/http2_inspect/http2_hpack.h @@ -43,7 +43,7 @@ class Http2HpackDecoder public: Http2HpackDecoder(Http2FlowData* flow_data, HttpCommon::SourceId src_id, Http2EventGen* const _events, Http2Infractions* const _infractions) : - session_data(flow_data), events(_events), infractions(_infractions), + session_data(flow_data), events(_events), infractions(_infractions), source_id(src_id), decode_table(flow_data, src_id) { } bool decode_headers(const uint8_t* encoded_headers, const uint32_t encoded_headers_length, uint8_t* decoded_headers, Http2StartLine* start_line, bool trailers); @@ -82,8 +82,8 @@ public: bool finalize_start_line(); const Field* get_start_line(); Field get_decoded_headers(const uint8_t* const decoded_headers); - HpackIndexTable* get_decode_table() { return &decode_table; } bool are_pseudo_headers_allowed() { return pseudo_headers_allowed; } + void settings_table_size_update(const uint32_t size); private: Http2StartLine* start_line; @@ -92,6 +92,7 @@ private: Http2FlowData* session_data; Http2EventGen* const events; Http2Infractions* const infractions; + const HttpCommon::SourceId source_id; static Http2HpackIntDecode decode_int7; static Http2HpackIntDecode decode_int6; @@ -103,6 +104,8 @@ private: bool table_size_update_allowed = true; uint8_t num_table_size_updates = 0; bool is_trailers = false; + bool expect_table_size_update = false; + uint32_t min_settings_table_size_received = UINT32_MAX; }; #endif diff --git a/src/service_inspectors/http2_inspect/http2_hpack_table.cc b/src/service_inspectors/http2_inspect/http2_hpack_table.cc index d23f4e727..094bc7206 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack_table.cc +++ b/src/service_inspectors/http2_inspect/http2_hpack_table.cc @@ -121,29 +121,3 @@ bool HpackIndexTable::add_index(const Field& name, const Field& value) { return dynamic_table.add_entry(name, value); } - -void HpackIndexTable::settings_table_size_update(uint32_t new_size) -{ - if (!encoder_set_max_size) - dynamic_table.update_size(new_size); - else if (new_size < dynamic_table.get_max_size()) - { - encoder_set_max_size = false; - dynamic_table.update_size(new_size); - } -} - -// A dynamic table size update sent in an HPACK encoder cannot be larger than last -// HEADER_TABLE_SIZE settings frame parameter sent by the decoder -bool HpackIndexTable::hpack_table_size_update(uint32_t new_size) -{ - encoder_set_max_size = true; - if (new_size <= session_data->get_recipient_connection_settings(source_id)-> - get_param(SFID_HEADER_TABLE_SIZE)) - { - dynamic_table.update_size(new_size); - return true; - } - else - return false; -} diff --git a/src/service_inspectors/http2_inspect/http2_hpack_table.h b/src/service_inspectors/http2_inspect/http2_hpack_table.h index c09ddbfbd..16ac1330f 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack_table.h +++ b/src/service_inspectors/http2_inspect/http2_hpack_table.h @@ -45,8 +45,7 @@ public: { } const HpackTableEntry* lookup(uint64_t index) const; bool add_index(const Field& name, const Field& value); - bool hpack_table_size_update(const uint32_t size); - void settings_table_size_update(const uint32_t size); + HpackDynamicTable& get_dynamic_table() { return dynamic_table; } const static uint8_t STATIC_MAX_INDEX = 61; const static uint8_t PSEUDO_HEADER_MAX_STATIC_INDEX = 14; @@ -56,6 +55,5 @@ private: HpackDynamicTable dynamic_table; Http2FlowData* session_data; HttpCommon::SourceId source_id; - bool encoder_set_max_size = false; }; #endif 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 171bc6e29..f5afb6ecb 100644 --- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc @@ -54,7 +54,7 @@ Http2PushPromiseFrame::Http2PushPromiseFrame(const uint8_t* header_buffer, session_data->events[source_id]->create_event(EVENT_INVALID_STREAM_ID); } - if (session_data->get_recipient_connection_settings(source_id)->get_param(SFID_ENABLE_PUSH) == 0) + if (session_data->get_remote_connection_settings(source_id)->get_param(SFID_ENABLE_PUSH) == 0) { session_data->events[source_id]->create_event(EVENT_PUSH_WHEN_PROHIBITED); *session_data->infractions[source_id] += INF_PUSH_WHEN_PROHIBITED; diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.cc b/src/service_inspectors/http2_inspect/http2_settings_frame.cc index 5bca2409a..ea3110918 100644 --- a/src/service_inspectors/http2_inspect/http2_settings_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_settings_frame.cc @@ -111,7 +111,7 @@ bool Http2SettingsFrame::handle_update(uint16_t id, uint32_t value) // Sending a table size parameter informs the receiver the maximum hpack dynamic // table size they may use. session_data->get_hpack_decoder((HttpCommon::SourceId) (1 - source_id))-> - get_decode_table()->settings_table_size_update(value); + settings_table_size_update(value); break; case SFID_ENABLE_PUSH: // Only values of 0 or 1 are allowed diff --git a/src/service_inspectors/http2_inspect/http2_tables.cc b/src/service_inspectors/http2_inspect/http2_tables.cc index e5a274dae..233da269e 100644 --- a/src/service_inspectors/http2_inspect/http2_tables.cc +++ b/src/service_inspectors/http2_inspect/http2_tables.cc @@ -64,6 +64,12 @@ const RuleMap Http2Module::http2_events[] = { EVENT_INVALID_WINDOW_UPDATE_FRAME, "invalid HTTP/2 window update frame" }, { EVENT_WINDOW_UPDATE_FRAME_ZERO_INCREMENT, "HTTP/2 window update frame with zero increment" }, { EVENT_REQUEST_WITHOUT_METHOD, "HTTP/2 request without a method" }, + { EVENT_TABLE_SIZE_UPDATE_NOT_AT_HEADER_START, + "HTTP/2 HPACK table size update not at the start of a header block" }, + { EVENT_MORE_THAN_2_TABLE_SIZE_UPDATES, + "More than two HTTP/2 HPACK table size updates in a single header block" }, + { EVENT_HPACK_TABLE_SIZE_UPDATE_EXCEEDS_MAX, + "HTTP/2 HPACK table size update exceeds max value set by decoder in SETTINGS frame" }, { 0, nullptr } };