From 846dddb066ff1c2bc8a1d70f5b0a3ce09dcdf24d Mon Sep 17 00:00:00 2001 From: "Mike Stepanek (mstepane)" Date: Mon, 23 Nov 2020 20:41:44 +0000 Subject: [PATCH] Merge pull request #2630 in SNORT/snort3 from ~MDAGON/snort3:h2i_err2 to master Squashed commit of the following: commit 89ff0a660518e90ad8cbf8dc7557d70d913b1490 Author: mdagon Date: Thu Nov 12 16:01:31 2020 -0500 http2_inspect: check for invalid flags --- .../http2_inspect/CMakeLists.txt | 1 + .../http2_inspect/http2_data_frame.cc | 2 + .../http2_inspect/http2_data_frame.h | 2 + .../http2_inspect/http2_enum.h | 7 ++- .../http2_inspect/http2_frame.cc | 32 +++++++++--- .../http2_inspect/http2_frame.h | 1 + .../http2_inspect/http2_headers_frame.cc | 3 ++ .../http2_inspect/http2_headers_frame.h | 1 + .../http2_inspect/http2_ping_frame.h | 44 +++++++++++++++++ .../http2_inspect/http2_push_promise_frame.cc | 10 ++-- .../http2_inspect/http2_push_promise_frame.h | 2 + .../http2_inspect/http2_settings_frame.cc | 6 +-- .../http2_inspect/http2_settings_frame.h | 4 +- .../http2_stream_splitter_impl.cc | 49 ++++++++++--------- .../http2_inspect/http2_tables.cc | 3 +- 15 files changed, 122 insertions(+), 45 deletions(-) create mode 100644 src/service_inspectors/http2_inspect/http2_ping_frame.h diff --git a/src/service_inspectors/http2_inspect/CMakeLists.txt b/src/service_inspectors/http2_inspect/CMakeLists.txt index b5bf70b4e..732d6cc74 100644 --- a/src/service_inspectors/http2_inspect/CMakeLists.txt +++ b/src/service_inspectors/http2_inspect/CMakeLists.txt @@ -35,6 +35,7 @@ set (FILE_LIST http2_inspect.h http2_module.cc http2_module.h + http2_ping_frame.h http2_push_promise_frame.cc http2_push_promise_frame.h http2_request_line.cc diff --git a/src/service_inspectors/http2_inspect/http2_data_frame.cc b/src/service_inspectors/http2_inspect/http2_data_frame.cc index 759edacd2..f0b8c487a 100644 --- a/src/service_inspectors/http2_inspect/http2_data_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_data_frame.cc @@ -101,6 +101,8 @@ void Http2DataFrame::update_stream_state() } } +uint8_t Http2DataFrame::get_flags_mask() const { return (END_STREAM|PADDED); } + #ifdef REG_TEST void Http2DataFrame::print_frame(FILE* output) { diff --git a/src/service_inspectors/http2_inspect/http2_data_frame.h b/src/service_inspectors/http2_inspect/http2_data_frame.h index 86c534328..2ca965487 100644 --- a/src/service_inspectors/http2_inspect/http2_data_frame.h +++ b/src/service_inspectors/http2_inspect/http2_data_frame.h @@ -48,6 +48,8 @@ private: Http2DataFrame(const uint8_t* header_buffer, const uint32_t header_len, const uint8_t* data_buffer_, const uint32_t data_length_, Http2FlowData* ssn_data, HttpCommon::SourceId src_id, Http2Stream* stream); + uint8_t get_flags_mask() const override; + const uint32_t data_length; const uint8_t* const data_buffer; uint32_t xtradata_mask = 0; diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h index 938a56c9b..efce2e58b 100644 --- a/src/service_inspectors/http2_inspect/http2_enum.h +++ b/src/service_inspectors/http2_inspect/http2_enum.h @@ -73,13 +73,12 @@ enum EventSid EVENT_PSEUDO_HEADER_IN_TRAILERS = 18, EVENT_INVALID_PSEUDO_HEADER = 19, EVENT_TRAILERS_NOT_END = 20, - EVENT_PADDING_ON_INVALID_FRAME = 21, + EVENT_PUSH_WHEN_PROHIBITED = 21, EVENT_PADDING_ON_EMPTY_FRAME = 22, EVENT_C2S_PUSH = 23, EVENT_INVALID_PUSH_FRAME = 24, EVENT_BAD_PUSH_SEQUENCE = 25, EVENT_BAD_SETTINGS_VALUE = 26, - EVENT_PUSH_WHEN_PROHIBITED = 27, EVENT__MAX_VALUE }; @@ -118,7 +117,7 @@ enum Infraction INF_PUSH_FRAME_TOO_SHORT = 28, INF_PSEUDO_HEADER_IN_TRAILERS = 29, INF_TRAILERS_NOT_END = 30, - INF_PADDING_ON_INVALID_FRAME = 31, + INF_CONNECT_WITH_SCHEME_OR_PATH = 31, INF_PADDING_ON_EMPTY_FRAME = 32, INF_BAD_PUSH_SEQUENCE = 33, INF_BAD_SETTINGS_PUSH_VALUE = 34, @@ -129,12 +128,12 @@ enum Infraction INF_TRUNCATED_HEADER_LINE = 39, INF_REQUEST_WITHOUT_METHOD = 40, INF_CONNECT_WITHOUT_AUTHORITY = 41, - INF_CONNECT_WITH_SCHEME_OR_PATH = 42, INF__MAX_VALUE }; enum HeaderFrameFlags { + ACK = 0x1, END_STREAM = 0x1, END_HEADERS = 0x4, PADDED = 0x8, diff --git a/src/service_inspectors/http2_inspect/http2_frame.cc b/src/service_inspectors/http2_inspect/http2_frame.cc index 05be984ff..bbeed292b 100644 --- a/src/service_inspectors/http2_inspect/http2_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_frame.cc @@ -28,6 +28,7 @@ #include "http2_flow_data.h" #include "http2_headers_frame_header.h" #include "http2_headers_frame_trailer.h" +#include "http2_ping_frame.h" #include "http2_push_promise_frame.h" #include "http2_settings_frame.h" #include "http2_stream.h" @@ -54,29 +55,48 @@ Http2Frame* Http2Frame::new_frame(const uint8_t* header, const uint32_t header_l const uint8_t* data, const uint32_t data_len, Http2FlowData* session_data, SourceId source_id, Http2Stream* stream) { + Http2Frame* frame = nullptr; + // FIXIT-E call the appropriate frame subclass constructor based on the type switch(session_data->frame_type[source_id]) { case FT_HEADERS: if (stream->get_state(source_id) == STREAM_EXPECT_HEADERS) - return new Http2HeadersFrameHeader(header, header_len, data, data_len, + frame = new Http2HeadersFrameHeader(header, header_len, data, data_len, session_data, source_id, stream); else - return new Http2HeadersFrameTrailer(header, header_len, data, data_len, + frame = new Http2HeadersFrameTrailer(header, header_len, data, data_len, session_data, source_id, stream); + break; case FT_SETTINGS: - return new Http2SettingsFrame(header, header_len, data, data_len, session_data, + frame = new Http2SettingsFrame(header, header_len, data, data_len, session_data, source_id, stream); + break; case FT_DATA: - return new Http2DataFrame(header, header_len, data, data_len, session_data, source_id, + frame = new Http2DataFrame(header, header_len, data, data_len, session_data, source_id, stream); + break; case FT_PUSH_PROMISE: - return new Http2PushPromiseFrame(header, header_len, data, data_len, session_data, + frame = new Http2PushPromiseFrame(header, header_len, data, data_len, session_data, source_id, stream); + break; + case FT_PING: + frame = new Http2PingFrame(header, header_len, data, data_len, session_data, + source_id, stream); + break; default: - return new Http2Frame(header, header_len, data, data_len, session_data, source_id, + frame = new Http2Frame(header, header_len, data, data_len, session_data, source_id, stream); } + + const uint8_t flags = frame->get_flags(); + if (flags != (flags & frame->get_flags_mask())) + { + *session_data->infractions[source_id] += INF_INVALID_FLAG; + session_data->events[source_id]->create_event(EVENT_INVALID_FLAG); + } + + return frame; } const Field& Http2Frame::get_buf(unsigned id) diff --git a/src/service_inspectors/http2_inspect/http2_frame.h b/src/service_inspectors/http2_inspect/http2_frame.h index 6a2789734..1bb4662ff 100644 --- a/src/service_inspectors/http2_inspect/http2_frame.h +++ b/src/service_inspectors/http2_inspect/http2_frame.h @@ -62,6 +62,7 @@ protected: HttpCommon::SourceId source_id, Http2Stream* stream); uint8_t get_flags(); uint32_t get_stream_id(); + virtual uint8_t get_flags_mask() const { return 0; } Field header; Field data; diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc index c3e40bbdb..d11e40036 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc @@ -147,6 +147,9 @@ const Field& Http2HeadersFrame::get_buf(unsigned id) } } +uint8_t Http2HeadersFrame::get_flags_mask() const +{ return (END_STREAM|END_HEADERS|PADDED|PRIORITY); } + #ifdef REG_TEST void Http2HeadersFrame::print_frame(FILE* output) { diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h index 5016bb6c0..94b4d37a6 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.h +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h @@ -48,6 +48,7 @@ protected: const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data, HttpCommon::SourceId src_id, Http2Stream* stream); 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 diff --git a/src/service_inspectors/http2_inspect/http2_ping_frame.h b/src/service_inspectors/http2_inspect/http2_ping_frame.h new file mode 100644 index 000000000..e7bacae16 --- /dev/null +++ b/src/service_inspectors/http2_inspect/http2_ping_frame.h @@ -0,0 +1,44 @@ +//-------------------------------------------------------------------------- +// Copyright (C) 2020-2020 Cisco and/or its affiliates. All rights reserved. +// +// This program is free software; you can redistribute it and/or modify it +// under the terms of the GNU General Public License Version 2 as published +// by the Free Software Foundation. You may not use, modify or distribute +// this program under any other version of the GNU General Public License. +// +// This program is distributed in the hope that it will be useful, but +// WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +// General Public License for more details. +// +// You should have received a copy of the GNU General Public License along +// with this program; if not, write to the Free Software Foundation, Inc., +// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +//-------------------------------------------------------------------------- +// http2_ping_frame.h author Maya Dagon +// FIXIT-E currently only supports flags validation + +#ifndef HTTP2_PING_FRAME_H +#define HTTP2_PING_FRAME_H + +#include "http2_frame.h" + +class Http2Frame; + +class Http2PingFrame : public Http2Frame +{ +public: + friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const uint32_t, const uint8_t*, + const uint32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream); + +private: + Http2PingFrame(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) : + Http2Frame(header_buffer, header_len, data_buffer, data_len, ssn_data, src_id, _stream) { } + + uint8_t get_flags_mask() const override + { return Http2Enums::ACK; } +}; +#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 35222c756..81a6ce47c 100644 --- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc @@ -60,13 +60,6 @@ Http2PushPromiseFrame::Http2PushPromiseFrame(const uint8_t* header_buffer, *session_data->infractions[source_id] += INF_PUSH_WHEN_PROHIBITED; } - // Push_promise frames only define the padded and end_headers flags - if (get_flags() & ~PADDED & ~END_HEADERS) - { - session_data->events[source_id]->create_event(EVENT_INVALID_FLAG); - *session_data->infractions[source_id] += INF_INVALID_FLAG; - } - start_line_generator = new Http2RequestLine(session_data->events[source_id], session_data->infractions[source_id]); @@ -161,6 +154,9 @@ uint32_t Http2PushPromiseFrame::get_promised_stream_id(Http2EventGen* const even return get_stream_id_from_buffer(data_buffer); } +uint8_t Http2PushPromiseFrame::get_flags_mask() const +{ return (END_HEADERS|PADDED); } + #ifdef REG_TEST void Http2PushPromiseFrame::print_frame(FILE* output) { diff --git a/src/service_inspectors/http2_inspect/http2_push_promise_frame.h b/src/service_inspectors/http2_inspect/http2_push_promise_frame.h index dc5838532..3dab88161 100644 --- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.h +++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.h @@ -57,6 +57,8 @@ private: Http2PushPromiseFrame(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); + uint8_t get_flags_mask() const override; + static const int32_t PROMISED_ID_LENGTH = 4; }; #endif diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.cc b/src/service_inspectors/http2_inspect/http2_settings_frame.cc index b0144f021..9447131fa 100644 --- a/src/service_inspectors/http2_inspect/http2_settings_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_settings_frame.cc @@ -58,7 +58,7 @@ Http2SettingsFrame::Http2SettingsFrame(const uint8_t* header_buffer, const uint3 return; } - if (SfAck & get_flags()) + if (ACK & get_flags()) return; parse_settings_frame(); @@ -89,7 +89,7 @@ void Http2SettingsFrame::parse_settings_frame() bool Http2SettingsFrame::sanity_check() { - const bool ack = SfAck & get_flags(); + const bool ack = ACK & get_flags(); // FIXIT-E this next check should possibly be moved to valid_sequence() if (get_stream_id() != 0) @@ -134,7 +134,7 @@ void Http2SettingsFrame::print_frame(FILE* output) if (bad_frame) fprintf(output, " Error in settings frame."); - else if (SfAck & get_flags()) + else if (ACK & get_flags()) fprintf(output, " ACK"); else fprintf(output, " Parameters in current frame - %d.", (data.length()/6)) ; diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.h b/src/service_inspectors/http2_inspect/http2_settings_frame.h index 8ebecff11..94afffe90 100644 --- a/src/service_inspectors/http2_inspect/http2_settings_frame.h +++ b/src/service_inspectors/http2_inspect/http2_settings_frame.h @@ -46,8 +46,10 @@ private: bool sanity_check(); bool handle_update(uint16_t id, uint32_t value); + uint8_t get_flags_mask() const override + { return Http2Enums::ACK; } + bool bad_frame = false; - static const uint8_t SfAck = 0x01; }; class Http2ConnectionSettings diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc index 018e66e79..e8bcd5313 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc @@ -219,26 +219,36 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio session_data->events[source_id]->create_event(EVENT_MISSING_CONTINUATION); return StreamSplitter::ABORT; } - + + const uint32_t frame_length = get_frame_length(session_data-> + scan_frame_header[source_id]); + session_data->frame_lengths[source_id].push(frame_length); + const uint8_t frame_flags = get_frame_flags(session_data-> + scan_frame_header[source_id]); + if (type != FT_CONTINUATION) { session_data->frame_type[source_id] = type; memcpy(session_data->lead_frame_header[source_id], session_data->scan_frame_header[source_id], FRAME_HEADER_LENGTH); } - else if (!session_data->continuation_expected[source_id]) - { - *session_data->infractions[source_id] += INF_UNEXPECTED_CONTINUATION; - session_data->events[source_id]->create_event(EVENT_UNEXPECTED_CONTINUATION); - return StreamSplitter::ABORT; + else + { + if (!session_data->continuation_expected[source_id]) + { + *session_data->infractions[source_id] += INF_UNEXPECTED_CONTINUATION; + session_data->events[source_id]->create_event(EVENT_UNEXPECTED_CONTINUATION); + return StreamSplitter::ABORT; + } + // Do flags check for continuation frame, since it is not saved + // as lead frame for later. + if ((frame_flags & END_HEADERS) != frame_flags) + { + *session_data->infractions[source_id] += INF_INVALID_FLAG; + session_data->events[source_id]->create_event(EVENT_INVALID_FLAG); + } } - const uint32_t frame_length = get_frame_length(session_data-> - scan_frame_header[source_id]); - session_data->frame_lengths[source_id].push(frame_length); - const uint8_t frame_flags = get_frame_flags(session_data-> - scan_frame_header[source_id]); - if ((type != FT_DATA) && (frame_length + FRAME_HEADER_LENGTH > MAX_OCTETS)) { // FIXIT-E long non-data frames may need to be supported @@ -249,16 +259,9 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio assert(session_data->scan_remaining_frame_octets[source_id] == 0); session_data->scan_remaining_frame_octets[source_id] = frame_length; - if (frame_flags & PADDED) + if ((frame_flags & PADDED) && + (type == FT_DATA || type == FT_HEADERS || type == FT_PUSH_PROMISE)) { - if (!(type == FT_DATA || type == FT_HEADERS || type == FT_PUSH_PROMISE)) - { - *session_data->infractions[source_id] += INF_PADDING_ON_INVALID_FRAME; - session_data->events[source_id]->create_event( - EVENT_PADDING_ON_INVALID_FRAME); - // FIXIT-E this is not a sufficient reason to abort - return StreamSplitter::ABORT; - } if (frame_length == 0) { *session_data->infractions[source_id] += INF_PADDING_ON_EMPTY_FRAME; @@ -429,7 +432,9 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess const uint8_t frame_flags = get_frame_flags(session_data->lead_frame_header[source_id]); - if ((frame_flags & PADDED) && !session_data->continuation_frame[source_id]) + const uint8_t type = session_data->frame_type[source_id]; + if ((frame_flags & PADDED) && !session_data->continuation_frame[source_id] && + (type == FT_HEADERS || type == FT_PUSH_PROMISE)) session_data->read_padding_len[source_id] = true; if (data_offset == len) diff --git a/src/service_inspectors/http2_inspect/http2_tables.cc b/src/service_inspectors/http2_inspect/http2_tables.cc index f4573b81c..05265966b 100644 --- a/src/service_inspectors/http2_inspect/http2_tables.cc +++ b/src/service_inspectors/http2_inspect/http2_tables.cc @@ -51,13 +51,12 @@ const RuleMap Http2Module::http2_events[] = { EVENT_PSEUDO_HEADER_IN_TRAILERS, "HTTP/2 pseudo-header in trailers" }, { EVENT_INVALID_PSEUDO_HEADER, "invalid HTTP/2 pseudo-header" }, { EVENT_TRAILERS_NOT_END, "HTTP/2 trailers without END_STREAM bit" }, - { EVENT_PADDING_ON_INVALID_FRAME, "padding flag set on invalid HTTP/2 frame type" }, + { EVENT_PUSH_WHEN_PROHIBITED, "HTTP/2 push promise frame sent when prohibited by receiver" }, { EVENT_PADDING_ON_EMPTY_FRAME, "padding flag set on HTTP/2 frame with zero length" }, { EVENT_C2S_PUSH, "HTTP/2 push promise frame in c2s direction" }, { EVENT_INVALID_PUSH_FRAME, "invalid HTTP/2 push promise frame" }, { EVENT_BAD_PUSH_SEQUENCE, "HTTP/2 push promise frame sent at invalid time" }, { EVENT_BAD_SETTINGS_VALUE, "invalid parameter value sent in HTTP/2 settings frame" }, - { EVENT_PUSH_WHEN_PROHIBITED, "HTTP/2 push promise frame sent when prohibited by receiver" }, { 0, nullptr } }; -- 2.47.3