From: Adrian Mamolea (admamole) Date: Wed, 16 Aug 2023 14:42:41 +0000 (+0000) Subject: Pull request #3946: http2_inspect: update connection settings on ack X-Git-Tag: 3.1.69.0~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=03dffc1e5bf911068f10d15f79c0f5ae5d616d40;p=thirdparty%2Fsnort3.git Pull request #3946: http2_inspect: update connection settings on ack Merge in SNORT/snort3 from ~ADMAMOLE/snort3:settings_ack to master Squashed commit of the following: commit 28a58b0433ba324da53fcf14398c2cdd205dd0b3 Author: Adrian Mamolea Date: Tue Jul 25 16:03:20 2023 -0400 http2_inspect: update connection settings on ack --- diff --git a/doc/reference/builtin_stubs.txt b/doc/reference/builtin_stubs.txt index 1cef38c44..ec9e52da7 100644 --- a/doc/reference/builtin_stubs.txt +++ b/doc/reference/builtin_stubs.txt @@ -1458,6 +1458,14 @@ Invalid HTTP/2 PRIORITY frame. Stream ID is 0 or length is not 5. Invalid HTTP/2 GOAWAY frame. R bit is set or stream ID is not 0 or length is less than 8. +121:42 + +More than 6 unacknowledged settings frames. + +121:43 + +Unexpected settings ACK. + 122:1 Basic one host to one host TCP portscan where multiple TCP ports are scanned on diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h index b84f1d725..586e57730 100644 --- a/src/service_inspectors/http2_inspect/http2_enum.h +++ b/src/service_inspectors/http2_inspect/http2_enum.h @@ -97,6 +97,8 @@ enum EventSid EVENT_LOSS_OF_SYNC = 39, EVENT_INVALID_PRIORITY_FRAME = 40, EVENT_INVALID_GOAWAY_FRAME = 41, + EVENT_SETTINGS_QUEUE_OVERFLOW = 42, + EVENT_SETTINGS_QUEUE_UNDERFLOW = 43, EVENT__MAX_VALUE }; @@ -159,6 +161,8 @@ enum Infraction INF_BAD_GOAWAY_FRAME_STREAM_ID = 52, INF_BAD_GOAWAY_FRAME_LENGTH = 53, INF_BAD_GOAWAY_FRAME_R_BIT = 54, + INF_SETTINGS_QUEUE_OVERFLOW = 55, + INF_SETTINGS_QUEUE_UNDERFLOW = 56, INF__MAX_VALUE }; diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.h b/src/service_inspectors/http2_inspect/http2_flow_data.h index 01fa764d9..7d3d43d5f 100644 --- a/src/service_inspectors/http2_inspect/http2_flow_data.h +++ b/src/service_inspectors/http2_inspect/http2_flow_data.h @@ -149,6 +149,7 @@ protected: // Used in eval() Http2ConnectionSettings connection_settings[2]; + Http2ConnectionSettingsQueue settings_queue[2]; Http2HpackDecoder hpack_decoder[2]; std::list streams; uint32_t concurrent_files = 0; diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.cc b/src/service_inspectors/http2_inspect/http2_settings_frame.cc index 9927fd568..60669d839 100644 --- a/src/service_inspectors/http2_inspect/http2_settings_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_settings_frame.cc @@ -59,16 +59,29 @@ Http2SettingsFrame::Http2SettingsFrame(const uint8_t* header_buffer, const uint3 } if (FLAG_ACK & get_flags()) - return; - - if (src_id == HttpCommon::SRC_SERVER && !ssn_data->was_server_settings_received()) - ssn_data->set_server_settings_received(); + apply_settings(); + else + { + if (src_id == HttpCommon::SRC_SERVER && !ssn_data->was_server_settings_received()) + ssn_data->set_server_settings_received(); - parse_settings_frame(); + queue_settings(); + } } -void Http2SettingsFrame::parse_settings_frame() +void Http2SettingsFrame::queue_settings() { + auto& settings_queue = session_data->settings_queue[source_id]; + // Insert new settings in the queue (duplicating latest queued or current) + if (not settings_queue.extend(session_data->connection_settings[source_id])) + { + session_data->events[source_id]->create_event(EVENT_SETTINGS_QUEUE_OVERFLOW); + *session_data->infractions[source_id] += INF_SETTINGS_QUEUE_OVERFLOW; + } + + // Update new settings values based on received frame + Http2ConnectionSettings& settings = settings_queue.back(); + int32_t data_pos = 0; while (data_pos < data.length()) @@ -82,11 +95,14 @@ void Http2SettingsFrame::parse_settings_frame() { session_data->events[source_id]->create_event(EVENT_SETTINGS_FRAME_UNKN_PARAM); *session_data->infractions[source_id] += INF_SETTINGS_FRAME_UNKN_PARAM; - continue; } - - if (handle_update(parameter_id, parameter_value)) - session_data->connection_settings[source_id].set_param(parameter_id, parameter_value); + else if (parameter_id == SFID_ENABLE_PUSH and parameter_value > 1) + { + session_data->events[source_id]->create_event(EVENT_BAD_SETTINGS_VALUE); + *session_data->infractions[source_id] += INF_BAD_SETTINGS_PUSH_VALUE; + } + else + settings.set_param(parameter_id, parameter_value); } } @@ -103,25 +119,40 @@ bool Http2SettingsFrame::sanity_check() return !(bad_frame); } +void Http2SettingsFrame::apply_settings() +{ + // Apply settings to direction opposite to current ACK frame. + auto settings_source_id = 1 - source_id; + assert(settings_source_id == HttpCommon::SRC_CLIENT || settings_source_id == HttpCommon::SRC_SERVER); + auto& settings_queue = session_data->settings_queue[settings_source_id]; + if (settings_queue.size() == 0) + { + session_data->events[source_id]->create_event(EVENT_SETTINGS_QUEUE_UNDERFLOW); + *session_data->infractions[source_id] += INF_SETTINGS_QUEUE_UNDERFLOW; + return; + } + + auto& next_settings = settings_queue.front(); + auto& current_settings = session_data->connection_settings[settings_source_id]; + + for (uint16_t parameter_id = SFID_HEADER_TABLE_SIZE; parameter_id <= SFID_MAX_HEADER_LIST_SIZE; ++parameter_id) + if (next_settings.get_param(parameter_id) != current_settings.get_param(parameter_id)) + handle_update(parameter_id, next_settings.get_param(parameter_id)); + + current_settings = next_settings; + settings_queue.pop(); +} + bool Http2SettingsFrame::handle_update(uint16_t id, uint32_t value) { switch (id) { case SFID_HEADER_TABLE_SIZE: // 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))-> + // table size they may use. The receiver is the sender of this ack. + session_data->get_hpack_decoder((HttpCommon::SourceId) (source_id))-> settings_table_size_update(value); break; - case SFID_ENABLE_PUSH: - // Only values of 0 or 1 are allowed - if (!(value == 0 or value == 1)) - { - session_data->events[source_id]->create_event(EVENT_BAD_SETTINGS_VALUE); - *session_data->infractions[source_id] += INF_BAD_SETTINGS_PUSH_VALUE; - return false; - } - break; default: break; } @@ -136,7 +167,8 @@ void Http2SettingsFrame::print_frame(FILE* output) if (bad_frame) fprintf(output, " Error in settings frame."); else if (FLAG_ACK & get_flags()) - fprintf(output, " ACK"); + fprintf(output, " ACK, Header Table Size: %d.", + session_data->connection_settings[1 - source_id].get_param(SFID_HEADER_TABLE_SIZE)); else fprintf(output, " Parameters in current frame - %d.", (data.length()/6)) ; @@ -160,3 +192,33 @@ void Http2ConnectionSettings::set_param(uint16_t id, uint32_t value) parameters[id - 1] = value; } + +void Http2ConnectionSettingsQueue::pop() +{ + assert(size()); + queue->erase(queue->begin()); + if (queue->size() == 0) + { + delete queue; + queue = nullptr; + } +} + +bool Http2ConnectionSettingsQueue::init(Http2ConnectionSettings& item) +{ + queue = new std::vector(); + queue->reserve(SETTINGS_QUEUE_MAX); + queue->push_back(item); + return true; +} + +bool Http2ConnectionSettingsQueue::extend() +{ + if (size() == SETTINGS_QUEUE_MAX) + // to stay in sync, do an implicit tail drop + return false; + + auto& item = back(); + queue->push_back(item); + return true; +} diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.h b/src/service_inspectors/http2_inspect/http2_settings_frame.h index fd546d861..13ddd09a7 100644 --- a/src/service_inspectors/http2_inspect/http2_settings_frame.h +++ b/src/service_inspectors/http2_inspect/http2_settings_frame.h @@ -42,8 +42,9 @@ private: const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data, HttpCommon::SourceId src_id, Http2Stream* stream); - void parse_settings_frame(); + void queue_settings(); bool sanity_check(); + void apply_settings(); bool handle_update(uint16_t id, uint32_t value); uint8_t get_flags_mask() const override @@ -71,4 +72,22 @@ private: 4294967295 // Max header list size }; }; + +class Http2ConnectionSettingsQueue +{ +public: + ~Http2ConnectionSettingsQueue() { delete queue; } + auto size() { return queue ? queue->size() : 0; } + bool extend(Http2ConnectionSettings& item) { return size() ? extend() : init(item); } + void pop(); + auto& front() { assert(size()); return queue->front(); } + auto& back() { assert(size()); return queue->back(); } + +private: + bool init(Http2ConnectionSettings& item); + bool extend(); + + static const uint8_t SETTINGS_QUEUE_MAX = 6; + std::vector* queue = nullptr; +}; #endif diff --git a/src/service_inspectors/http2_inspect/http2_tables.cc b/src/service_inspectors/http2_inspect/http2_tables.cc index 674dd6e19..f9574c5fe 100644 --- a/src/service_inspectors/http2_inspect/http2_tables.cc +++ b/src/service_inspectors/http2_inspect/http2_tables.cc @@ -75,6 +75,8 @@ const RuleMap Http2Module::http2_events[] = { EVENT_LOSS_OF_SYNC, "not HTTP/2 traffic or unrecoverable HTTP/2 protocol error" }, { EVENT_INVALID_PRIORITY_FRAME, "invalid HTTP/2 PRIORITY frame" }, { EVENT_INVALID_GOAWAY_FRAME, "invalid HTTP/2 GOAWAY frame" }, + { EVENT_SETTINGS_QUEUE_OVERFLOW, "too many unacknowledged settings" }, + { EVENT_SETTINGS_QUEUE_UNDERFLOW, "setting acknowledgment without actual settings" }, { 0, nullptr } };