From: Tom Peters (thopeter) Date: Thu, 16 Dec 2021 23:24:15 +0000 (+0000) Subject: Pull request #3203: http2_inspect: don't send data frames to the http stream splitter... X-Git-Tag: 3.1.20.0~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=00db044d6826d65576d5c74340ff31e0d3843048;p=thirdparty%2Fsnort3.git Pull request #3203: http2_inspect: don't send data frames to the http stream splitter when it's not expecting them Merge in SNORT/snort3 from ~KATHARVE/snort3:h2_unexpected_data_frames to master Squashed commit of the following: commit ca74f8065c003468325bfd4cfab69d3bb19de67e Author: Katura Harvey Date: Wed Dec 1 11:41:51 2021 -0500 http2_inspect: don't send data frames to the http stream splitter when it's not expecting them --- diff --git a/doc/reference/builtin_stubs.txt b/doc/reference/builtin_stubs.txt index a745668b9..18db0beee 100644 --- a/doc/reference/builtin_stubs.txt +++ b/doc/reference/builtin_stubs.txt @@ -1441,6 +1441,10 @@ More than two HTTP/2 HPACK table size updates in a single header block HTTP/2 HPACK table size update exceeds max value set by decoder in SETTINGS frame +121:37 + +Nonempty HTTP/2 Data frame where a message body was not expected. + 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_data_cutter.cc b/src/service_inspectors/http2_inspect/http2_data_cutter.cc index 3966be60d..800d01bce 100644 --- a/src/service_inspectors/http2_inspect/http2_data_cutter.cc +++ b/src/service_inspectors/http2_inspect/http2_data_cutter.cc @@ -37,10 +37,64 @@ Http2DataCutter::Http2DataCutter(Http2FlowData* _session_data, HttpCommon::Sourc session_data(_session_data), source_id(src_id) { } +StreamSplitter::Status Http2DataCutter::skip_over_frame(Http2Stream* const stream, uint32_t length, + uint32_t* flush_offset, uint32_t data_offset, uint8_t frame_flags) +{ + uint32_t& remaining = session_data->scan_remaining_frame_octets[source_id]; + if (length - data_offset < remaining) + { + *flush_offset = length; + remaining -= length - data_offset; + } + else + { + *flush_offset = data_offset + remaining; + remaining = 0; + session_data->header_octets_seen[source_id] = 0; + session_data->scan_state[source_id] = SCAN_FRAME_HEADER; + session_data->remaining_data_padding[source_id] = 0; + + if (!session_data->frame_lengths[source_id].empty()) + { + session_data->frame_lengths[source_id].pop(); + assert(session_data->frame_lengths[source_id].empty()); + } + + if (stream && ((frame_flags & FLAG_END_STREAM) != 0)) + { + stream->set_end_stream_on_data_flush(source_id); + discarded_frame_cleanup(stream); + } + } + + session_data->payload_discard[source_id] = true; + return StreamSplitter::FLUSH; +} + +bool Http2DataCutter::check_http_state(Http2Stream* const stream) +{ + HttpFlowData* const http_flow = stream->get_hi_flow_data(); + if ((http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2)) + { + stream->set_state(source_id, STREAM_ERROR); + if (data_len > 0) + { + *session_data->infractions[source_id] += INF_UNEXPECTED_DATA_FRAME; + session_data->events[source_id]->create_event(EVENT_UNEXPECTED_DATA_FRAME); + } + return false; + } + return true; +} + StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t length, uint32_t* flush_offset, uint32_t& data_offset, uint8_t frame_flags) { - const uint32_t cur_data_offset = data_offset; + Http2Stream* const stream = session_data->find_stream(session_data->current_stream[source_id]); + + if (!stream or !stream->is_open(source_id) or stream->is_discard_set(source_id)) + return skip_over_frame(stream, length, flush_offset, data_offset, frame_flags); + if (frame_bytes_seen == 0) { assert(session_data->frame_lengths[source_id].size() == 1); @@ -53,8 +107,15 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt data_len -= 1; frame_bytes_seen += 1; } + + if (!check_http_state(stream)) + { + return skip_over_frame(stream, length, flush_offset, data_offset, frame_flags); + } + } + const uint32_t cur_data_offset = data_offset; uint32_t cur_pos = data_offset; const uint32_t missing = data_len - data_bytes_read; const uint32_t cur_data = (missing <= (length - cur_pos)) ? missing : (length - cur_pos); @@ -65,8 +126,6 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt data_offset = cur_pos; *flush_offset = cur_pos; - session_data->stream_in_hi = session_data->current_stream[source_id]; - StreamSplitter::Status scan_result = StreamSplitter::SEARCH; if (cur_data > 0) @@ -75,32 +134,38 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt Http2DummyPacket dummy_pkt; dummy_pkt.flow = session_data->flow; uint32_t unused = 0; + session_data->stream_in_hi = session_data->current_stream[source_id]; if ((data_bytes_read == data_len) && (frame_flags & FLAG_END_STREAM)) { - Http2Stream* const stream = - session_data->find_stream(session_data->current_stream[source_id]); HttpFlowData* const hi_flow = stream->get_hi_flow_data(); hi_flow->set_h2_body_state(source_id, HttpEnums::H2_BODY_LAST_SEG); } scan_result = session_data->hi_ss[source_id]->scan(&dummy_pkt, data + cur_data_offset, cur_data, unused, &http_flush_offset); - - if (scan_result == StreamSplitter::FLUSH) + session_data->stream_in_hi = NO_STREAM_ID; + switch (scan_result) { + case StreamSplitter::FLUSH: + { bytes_sent_http += http_flush_offset; const uint32_t unused_input = cur_data - http_flush_offset; data_bytes_read -= unused_input; data_offset -= unused_input; *flush_offset -= unused_input; session_data->scan_remaining_frame_octets[source_id] -= http_flush_offset; - } - else if (scan_result == StreamSplitter::SEARCH) - { + break; + } + case StreamSplitter::SEARCH: bytes_sent_http += cur_data; session_data->scan_remaining_frame_octets[source_id] -= cur_data; - } - else + break; + default: + // Since we verified HttpStreamSplitter was expecting an H2 message body shouldn't get + // here assert(false); + stream->set_state(source_id, STREAM_ERROR); + return skip_over_frame(stream, length, flush_offset, data_offset, frame_flags); + } } if (data_bytes_read == data_len) @@ -112,22 +177,24 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt if (frame_flags & FLAG_END_STREAM) { - Http2Stream* const stream = session_data->find_stream( - session_data->current_stream[source_id]); assert(scan_result == StreamSplitter::FLUSH || data_len == 0); + session_data->stream_in_hi = session_data->current_stream[source_id]; stream->finish_msg_body(source_id, false, data_len == 0); + session_data->stream_in_hi = NO_STREAM_ID; // FIXIT-E this flag seems to mean both END_STREAM and the end of this frame stream->set_end_stream_on_data_flush(source_id); - session_data->stream_in_hi = NO_STREAM_ID; return StreamSplitter::FLUSH; } - else if (scan_result != StreamSplitter::FLUSH) + else if (scan_result == StreamSplitter::SEARCH) { - assert(scan_result == StreamSplitter::SEARCH); scan_result = StreamSplitter::FLUSH; if (cur_data > 0) + { + session_data->stream_in_hi = session_data->current_stream[source_id]; session_data->hi_ss[source_id]->prep_partial_flush(session_data->flow, 0); + session_data->stream_in_hi = NO_STREAM_ID; + } else { session_data->payload_discard[source_id] = true; @@ -193,7 +260,8 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len) reassemble_state = SEND_DATA; else { - assert(get_frame_flags(session_data->lead_frame_header[source_id]) & FLAG_END_STREAM); + assert(get_frame_flags(session_data->lead_frame_header[source_id]) & + FLAG_END_STREAM); reassemble_state = SEND_EMPTY_DATA; } break; @@ -244,9 +312,26 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len) return; } -void Http2DataCutter::discard_cleanup() +void Http2DataCutter::discarded_frame_cleanup(Http2Stream* const stream) { frame_bytes_seen = 0; reassemble_data_bytes_read = 0; reassemble_state = GET_FRAME_HDR; + + if (!stream->is_end_stream_on_data_flush(source_id)) + return; + + if(stream->get_state(source_id) != STREAM_ERROR) + { + if (session_data->concurrent_files > 0) + session_data->concurrent_files -= 1; + stream->set_state(source_id, STREAM_COMPLETE); + } + stream->check_and_cleanup_completed(); + if (session_data->delete_stream) + { + session_data->processing_stream_id = session_data->get_current_stream_id(source_id); + session_data->delete_processing_stream(); + session_data->processing_stream_id = NO_STREAM_ID; + } } diff --git a/src/service_inspectors/http2_inspect/http2_data_cutter.h b/src/service_inspectors/http2_inspect/http2_data_cutter.h index 55b750238..245f9b61d 100644 --- a/src/service_inspectors/http2_inspect/http2_data_cutter.h +++ b/src/service_inspectors/http2_inspect/http2_data_cutter.h @@ -26,6 +26,7 @@ #include "http2_enum.h" class Http2FlowData; +class Http2Stream; class Http2DataCutter { @@ -34,9 +35,13 @@ public: snort::StreamSplitter::Status scan(const uint8_t* data, uint32_t length, uint32_t* flush_offset, uint32_t& data_offset, uint8_t frame_flags); void reassemble(const uint8_t* data, unsigned len); - void discard_cleanup(); + void discarded_frame_cleanup(Http2Stream* const stream); private: + bool check_http_state(Http2Stream* const stream); + snort::StreamSplitter::Status skip_over_frame(Http2Stream* const stream, uint32_t length, + uint32_t* flush_offset, uint32_t data_offset, uint8_t frame_flags); + Http2FlowData* const session_data; const HttpCommon::SourceId source_id; diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h index 82b61a334..2edf82840 100644 --- a/src/service_inspectors/http2_inspect/http2_enum.h +++ b/src/service_inspectors/http2_inspect/http2_enum.h @@ -92,6 +92,7 @@ enum EventSid 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_UNEXPECTED_DATA_FRAME = 37, EVENT__MAX_VALUE }; @@ -147,6 +148,7 @@ enum Infraction INF_HEADER_UPPERCASE = 45, INF_INVALID_WINDOW_UPDATE_FRAME = 46, INF_WINDOW_UPDATE_FRAME_ZERO_INCREMENT = 47, + INF_UNEXPECTED_DATA_FRAME = 48, INF__MAX_VALUE }; diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc index 1aeab5241..0852fd143 100644 --- a/src/service_inspectors/http2_inspect/http2_stream.cc +++ b/src/service_inspectors/http2_inspect/http2_stream.cc @@ -127,6 +127,7 @@ bool Http2Stream::is_open(HttpCommon::SourceId source_id) return (state[source_id] == STREAM_EXPECT_BODY) || (state[source_id] == STREAM_BODY); } +// Caller must set session_data->stream_in_hi before calling this void Http2Stream::finish_msg_body(HttpCommon::SourceId source_id, bool expect_trailers, bool clear_partial_buffer) { diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.h b/src/service_inspectors/http2_inspect/http2_stream_splitter.h index 624aa70da..bdae9ebde 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter.h +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.h @@ -47,9 +47,8 @@ public: private: const HttpCommon::SourceId source_id; - static StreamSplitter::Status data_frame_header_checks(Http2FlowData* session_data, - uint32_t* flush_offset, HttpCommon::SourceId source_id, uint32_t frame_length, - uint32_t& data_offset); + static void data_frame_header_checks(Http2FlowData* session_data, + HttpCommon::SourceId source_id); static StreamSplitter::Status non_data_scan(Http2FlowData* session_data, uint32_t length, uint32_t* flush_offset, HttpCommon::SourceId source_id, uint8_t type, uint8_t frame_flags, uint32_t& data_offset); @@ -60,7 +59,6 @@ private: HttpCommon::SourceId source_id); static bool read_frame_hdr(Http2FlowData* session_data, const uint8_t* data, uint32_t length, HttpCommon::SourceId source_id, uint32_t& data_offset); - static void discarded_data_frame_cleanup(Http2FlowData* session_data, HttpCommon::SourceId source_id, Http2Stream* stream); }; 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 35cd94581..5d3519f89 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc @@ -63,9 +63,8 @@ static ValidationResult validate_preface(const uint8_t* data, const uint32_t len return V_GOOD; } -StreamSplitter::Status Http2StreamSplitter::data_frame_header_checks(Http2FlowData* session_data, - uint32_t* flush_offset, HttpCommon::SourceId source_id, uint32_t frame_length, - uint32_t& data_offset) +void Http2StreamSplitter::data_frame_header_checks(Http2FlowData* session_data, + HttpCommon::SourceId source_id) { Http2Stream* const stream = session_data->find_stream(session_data->current_stream[source_id]); @@ -81,22 +80,7 @@ StreamSplitter::Status Http2StreamSplitter::data_frame_header_checks(Http2FlowDa stream->set_state(source_id, STREAM_ERROR); } } - return StreamSplitter::SEARCH; - } - - HttpFlowData* const http_flow = (HttpFlowData*)stream->get_hi_flow_data(); - assert(http_flow != nullptr); - - // If 0 length frame and http_inspect isn't expecting body, flush without involving HI - if ((frame_length == 0) && (http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2)) - { - *flush_offset = data_offset; - session_data->header_octets_seen[source_id] = 0; - session_data->scan_state[source_id] = SCAN_FRAME_HEADER; - return StreamSplitter::FLUSH; } - - return StreamSplitter::SEARCH; } StreamSplitter::Status Http2StreamSplitter::non_data_scan(Http2FlowData* session_data, @@ -283,10 +267,7 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio } if (type == FT_DATA) - { - status = data_frame_header_checks(session_data, flush_offset, source_id, - frame_length, data_offset); - } + data_frame_header_checks(session_data, source_id); break; } @@ -330,46 +311,8 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio } else { - Http2Stream* const stream = - session_data->find_stream(session_data->current_stream[source_id]); - if (stream && stream->is_open(source_id) && !stream->is_discard_set(source_id)) - { - status = session_data->data_cutter[source_id].scan( - data, length, flush_offset, data_offset, frame_flags); - } - else - { - // Need to skip past Data frame in a bad stream or if passed flow depth - uint32_t& remaining = session_data->scan_remaining_frame_octets[source_id]; - if (length - data_offset < remaining) - { - *flush_offset = length; - remaining -= length - data_offset; - } - else - { - *flush_offset = data_offset + remaining; - remaining = 0; - session_data->header_octets_seen[source_id] = 0; - session_data->scan_state[source_id] = SCAN_FRAME_HEADER; - session_data->remaining_data_padding[source_id] = 0; - - if (!session_data->frame_lengths[source_id].empty()) - { - session_data->frame_lengths[source_id].pop(); - assert(session_data->frame_lengths[source_id].empty()); - } - - if (stream && ((frame_flags & FLAG_END_STREAM) != 0)) - { - stream->set_end_stream_on_data_flush(source_id); - discarded_data_frame_cleanup(session_data, source_id, stream); - } - } - - session_data->payload_discard[source_id] = true; - status = StreamSplitter::FLUSH; - } + status = session_data->data_cutter[source_id].scan(data, length, flush_offset, + data_offset, frame_flags); } assert(status != StreamSplitter::SEARCH or session_data->scan_state[source_id] != SCAN_EMPTY_DATA); @@ -525,7 +468,7 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess if (stream) { stream->set_discard(source_id); - discarded_data_frame_cleanup(session_data, source_id, stream); + session_data->data_cutter[source_id].discarded_frame_cleanup(stream); } } else @@ -539,24 +482,3 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess return frame_buf; } - -void Http2StreamSplitter::discarded_data_frame_cleanup(Http2FlowData* session_data, - HttpCommon::SourceId source_id, Http2Stream* stream) -{ - session_data->data_cutter[source_id].discard_cleanup(); - - if (stream->get_state(source_id) == STREAM_ERROR || - !stream->is_end_stream_on_data_flush(source_id)) - return; - - if (session_data->concurrent_files > 0) - session_data->concurrent_files -= 1; - stream->set_state(source_id, STREAM_COMPLETE); - stream->check_and_cleanup_completed(); - if (session_data->delete_stream) - { - session_data->processing_stream_id = session_data->get_current_stream_id(source_id); - session_data->delete_processing_stream(); - session_data->processing_stream_id = NO_STREAM_ID; - } -} diff --git a/src/service_inspectors/http2_inspect/http2_tables.cc b/src/service_inspectors/http2_inspect/http2_tables.cc index c34429e8c..88919dae4 100644 --- a/src/service_inspectors/http2_inspect/http2_tables.cc +++ b/src/service_inspectors/http2_inspect/http2_tables.cc @@ -70,6 +70,7 @@ const RuleMap Http2Module::http2_events[] = "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" }, + { EVENT_UNEXPECTED_DATA_FRAME, "Nonempty HTTP/2 Data frame where message body not expected" }, { 0, nullptr } }; diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc index 238db9471..db156ca3f 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc +++ b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc @@ -276,7 +276,7 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, (session_data->section_type[source_id] == SEC__NOT_COMPUTE)) { assert(session_data->type_expected[source_id] != SEC_ABORT); - // assert(session_data->section_type[source_id] != SEC__NOT_COMPUTE); // FIXIT-M H2I + assert(session_data->section_type[source_id] != SEC__NOT_COMPUTE); session_data->type_expected[source_id] = SEC_ABORT; return { nullptr, 0 }; }