From: Mike Stepanek (mstepane) Date: Mon, 30 Mar 2020 15:36:59 +0000 (+0000) Subject: Merge pull request #2111 in SNORT/snort3 from ~KATHARVE/snort3:h2_headers to master X-Git-Tag: 3.0.1-1~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e1e44b89161f8a9c824ba8406757d89702a1d5fa;p=thirdparty%2Fsnort3.git Merge pull request #2111 in SNORT/snort3 from ~KATHARVE/snort3:h2_headers to master Squashed commit of the following: commit b076d151ec56be77b27a72904e68c9eae18e887b Author: Katura Harvey Date: Wed Mar 25 19:06:59 2020 -0400 http2_inspect: handle Cl and TE headers, and end_stream flags set on headers frames --- diff --git a/src/service_inspectors/http2_inspect/http2_data_cutter.cc b/src/service_inspectors/http2_inspect/http2_data_cutter.cc index f97e483cf..c98bc7d6a 100644 --- a/src/service_inspectors/http2_inspect/http2_data_cutter.cc +++ b/src/service_inspectors/http2_inspect/http2_data_cutter.cc @@ -154,7 +154,7 @@ StreamSplitter::Status Http2DataCutter::http_scan(const uint8_t* data, uint32_t* if (frame_flags & END_STREAM) { session_data->get_current_stream(source_id)->get_hi_flow_data()-> - set_http2_end_stream(source_id); + finish_h2_body(source_id); scan_result = session_data->hi_ss[source_id]->scan(&dummy_pkt, nullptr, 0, unused, &http_flush_offset); assert(scan_result == StreamSplitter::FLUSH); diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc index c2e474305..78c5a3e9e 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc @@ -44,6 +44,10 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const int32_t HttpCommon::SourceId source_id_) : Http2Frame(header_buffer, header_len, data_buffer, data_len, session_data_, source_id_) { + Http2Stream* const stream = session_data->find_stream(get_stream_id()); + if (get_flags() & END_STREAM) + stream->set_end_stream(source_id); + uint8_t hpack_headers_offset = 0; // Remove stream dependency if present diff --git a/src/service_inspectors/http2_inspect/http2_stream.h b/src/service_inspectors/http2_inspect/http2_stream.h index 5938614cf..e026ae97c 100644 --- a/src/service_inspectors/http2_inspect/http2_stream.h +++ b/src/service_inspectors/http2_inspect/http2_stream.h @@ -51,8 +51,10 @@ public: Http2DataCutter* get_data_cutter(HttpCommon::SourceId source_id); void set_data_cutter(Http2DataCutter* cutter, HttpCommon::SourceId source_id) - { data_cutter[source_id] = cutter; } + { data_cutter[source_id] = cutter; } + void set_end_stream(HttpCommon::SourceId source_id) { end_stream_set[source_id] = true; } + bool end_stream_is_set(HttpCommon::SourceId source_id) { return end_stream_set[source_id]; } #ifdef REG_TEST void print_frame(FILE* output); #endif @@ -64,6 +66,7 @@ private: HttpFlowData* hi_flow_data = nullptr; HttpMsgSection* hi_msg_section = nullptr; Http2DataCutter* data_cutter[2] = { nullptr, nullptr}; + bool end_stream_set[2] = { false, false }; }; #endif 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 3401d0ecf..16f7a064a 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc @@ -48,8 +48,8 @@ StreamSplitter::Status data_scan(Http2FlowData* session_data, const uint8_t* dat if (stream) http_flow = (HttpFlowData*)stream->get_hi_flow_data(); - if (!stream || !http_flow || (frame_length > 0 and - (http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2))) + if (!stream || !http_flow || stream->end_stream_is_set(source_id) || + (frame_length > 0 and (http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2))) { *session_data->infractions[source_id] += INF_FRAME_SEQUENCE; session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE); @@ -90,6 +90,14 @@ StreamSplitter::Status non_data_scan(Http2FlowData* session_data, session_data->scan_remaining_frame_octets[source_id] = frame_length; session_data->total_bytes_in_split[source_id] += FRAME_HEADER_LENGTH + frame_length; + + // If the stream object exists and the end_stream flag is set, save that state in the stream + // object. If this is the first headers frame in the current stream,the stream object has + // not been created yet. The end_stream flag will be handled in the headers frame processing + Http2Stream* const stream = session_data->find_stream( + session_data->current_stream[source_id]); + if (stream and frame_flags & END_STREAM) + stream->set_end_stream(source_id); } // If we don't have the full frame, keep scanning diff --git a/src/service_inspectors/http_inspect/http_cutter.cc b/src/service_inspectors/http_inspect/http_cutter.cc index 30c9bc273..0c4f0069d 100644 --- a/src/service_inspectors/http_inspect/http_cutter.cc +++ b/src/service_inspectors/http_inspect/http_cutter.cc @@ -21,6 +21,7 @@ #include "config.h" #endif +#include "http_common.h" #include "http_cutter.h" #include "http_enum.h" @@ -687,37 +688,60 @@ ScanResult HttpBodyChunkCutter::cut(const uint8_t* buffer, uint32_t length, return detain_this_packet ? SCAN_NOT_FOUND_DETAIN : SCAN_NOT_FOUND; } -ScanResult HttpBodyH2Cutter::cut(const uint8_t* buffer, uint32_t length, HttpInfractions*, - HttpEventGen*, uint32_t flow_target, bool stretch, bool h2_end_stream) +ScanResult HttpBodyH2Cutter::cut(const uint8_t* buffer, uint32_t length, + HttpInfractions* infractions, HttpEventGen* events, uint32_t flow_target, bool stretch, + bool h2_body_finished) { - //FIXIT-M detained inspection not yet supported for http2 + //FIXIT-E detained inspection not yet supported for http2 UNUSED(buffer); - // FIXIT-M stretch not yet supported for http2 message bodies + // FIXIT-E stretch not yet supported for http2 message bodies UNUSED(stretch); + // If the headers included a content length header (expected length >= 0), check it against the + // actual message body length. Alert if it does not match at the end of the message body, or if + // it overflows during the body (alert once then stop computing). + if (expected_body_length >= 0) + { + if ((total_octets_scanned + length) > expected_body_length) + { + *infractions += INF_H2_DATA_OVERRUNS_CL; + events->create_event(EVENT_H2_DATA_OVERRUNS_CL); + expected_body_length = HttpCommon::STAT_NOT_COMPUTE; + } + else if (h2_body_finished and ((total_octets_scanned + length) < expected_body_length)) + { + *infractions += INF_H2_DATA_UNDERRUNS_CL; + events->create_event(EVENT_H2_DATA_UNDERRUNS_CL); + } + } + if (flow_target == 0) { num_flush = length; + total_octets_scanned += length; return SCAN_DISCARD_PIECE; } - if (!h2_end_stream) + if (!h2_body_finished) { if (octets_seen + length < flow_target) { // Not enough data yet to create a message section octets_seen += length; + total_octets_scanned += length; return SCAN_NOT_FOUND; } else { num_flush = flow_target - octets_seen; + total_octets_scanned += num_flush; return SCAN_FOUND_PIECE; } } else { // For now if end_stream is set for scan, a zero-length buffer is always sent to flush + assert(length == 0); num_flush = 0; return SCAN_FOUND; } diff --git a/src/service_inspectors/http_inspect/http_cutter.h b/src/service_inspectors/http_inspect/http_cutter.h index 1ea351b24..ae3197829 100644 --- a/src/service_inspectors/http_inspect/http_cutter.h +++ b/src/service_inspectors/http_inspect/http_cutter.h @@ -36,7 +36,7 @@ public: virtual ~HttpCutter() = default; virtual HttpEnums::ScanResult cut(const uint8_t* buffer, uint32_t length, HttpInfractions* infractions, HttpEventGen* events, uint32_t flow_target, bool stretch, - bool h2_end_stream) = 0; + bool h2_body_finished) = 0; uint32_t get_num_flush() const { return num_flush; } uint32_t get_octets_seen() const { return octets_seen; } uint32_t get_num_excess() const { return num_crlf; } @@ -164,10 +164,14 @@ private: class HttpBodyH2Cutter : public HttpBodyCutter { public: - explicit HttpBodyH2Cutter(bool detained_inspection, HttpEnums::CompressId compression) : - HttpBodyCutter(detained_inspection, compression) {} + explicit HttpBodyH2Cutter(int64_t expected_length, bool detained_inspection, + HttpEnums::CompressId compression) : + HttpBodyCutter(detained_inspection, compression), expected_body_length(expected_length) {} HttpEnums::ScanResult cut(const uint8_t*, uint32_t, HttpInfractions*, HttpEventGen*, - uint32_t flow_target, bool stretch, bool h2_end_stream) override; + uint32_t flow_target, bool stretch, bool h2_body_finished) override; +private: + int64_t expected_body_length; + uint32_t total_octets_scanned = 0; }; diff --git a/src/service_inspectors/http_inspect/http_enum.h b/src/service_inspectors/http_inspect/http_enum.h index cbfce493f..3fc8a0106 100644 --- a/src/service_inspectors/http_inspect/http_enum.h +++ b/src/service_inspectors/http_inspect/http_enum.h @@ -227,6 +227,9 @@ enum Infraction INF_VERSION_NOT_UPPERCASE, INF_CHUNK_LEADING_WS, INF_BAD_HEADER_WHITESPACE, + INF_H2_NON_IDENTITY_TE, + INF_H2_DATA_OVERRUNS_CL, + INF_H2_DATA_UNDERRUNS_CL, INF__MAX_VALUE }; @@ -345,6 +348,9 @@ enum EventSid EVENT_BAD_HEADER_WHITESPACE, EVENT_GZIP_EARLY_END, // 248 EVENT_EXCESS_REPEAT_PARAMS, + EVENT_H2_NON_IDENTITY_TE, + EVENT_H2_DATA_OVERRUNS_CL, + EVENT_H2_DATA_UNDERRUNS_CL, EVENT__MAX_VALUE }; diff --git a/src/service_inspectors/http_inspect/http_flow_data.h b/src/service_inspectors/http_inspect/http_flow_data.h index a4527e472..e0068cb99 100644 --- a/src/service_inspectors/http_inspect/http_flow_data.h +++ b/src/service_inspectors/http_inspect/http_flow_data.h @@ -71,13 +71,13 @@ public: HttpEnums::SectionType get_type_expected(HttpCommon::SourceId source_id) { return type_expected[source_id]; } - void set_http2_end_stream(HttpCommon::SourceId source_id) - { http2_end_stream[source_id] = true; } + void finish_h2_body(HttpCommon::SourceId source_id) + { h2_body_finished[source_id] = true; } private: // HTTP/2 handling bool for_http2 = false; - bool http2_end_stream[2] = { false, false }; + bool h2_body_finished[2] = { false, false }; // Convenience routines void half_reset(HttpCommon::SourceId source_id); diff --git a/src/service_inspectors/http_inspect/http_msg_body_h2.cc b/src/service_inspectors/http_inspect/http_msg_body_h2.cc index 1f57873e7..191f2ecee 100644 --- a/src/service_inspectors/http_inspect/http_msg_body_h2.cc +++ b/src/service_inspectors/http_inspect/http_msg_body_h2.cc @@ -26,18 +26,13 @@ void HttpMsgBodyH2::update_flow() { session_data->body_octets[source_id] = body_octets; - if (session_data->http2_end_stream[source_id]) + if (session_data->h2_body_finished[source_id]) { - // FIXIT-E check content length header against bytes received - session_data->trailer_prep(source_id); - session_data->http2_end_stream[source_id] = false; + session_data->h2_body_finished[source_id] = false; } else - { - // FIXIT-E check have not exceeded content length update_depth(); - } } #ifdef REG_TEST diff --git a/src/service_inspectors/http_inspect/http_msg_header.cc b/src/service_inspectors/http_inspect/http_msg_header.cc index 362b386d5..6a5587158 100644 --- a/src/service_inspectors/http_inspect/http_msg_header.cc +++ b/src/service_inspectors/http_inspect/http_msg_header.cc @@ -141,7 +141,7 @@ const Field& HttpMsgHeader::get_true_ip_addr() void HttpMsgHeader::gen_events() { if ((get_header_count(HEAD_CONTENT_LENGTH) > 0) && - (get_header_count(HEAD_TRANSFER_ENCODING) > 0)) + (get_header_count(HEAD_TRANSFER_ENCODING) > 0) && !session_data->for_http2) { add_infraction(INF_BOTH_CL_AND_TE); create_event(EVENT_BOTH_CL_AND_TE); @@ -196,15 +196,35 @@ void HttpMsgHeader::update_flow() return; } + const Field& te_header = get_header_value_norm(HEAD_TRANSFER_ENCODING); + if (session_data->for_http2) { - // FIXIT-E check for transfer-encoding and content-length headers + // The only transfer-encoding header we should see for HTTP/2 traffic is "identity" + const int IDENTITY_SIZE = 8; + if ((te_header.length() > 0) && ( (te_header.length() != IDENTITY_SIZE) || + memcmp(te_header.start(), "identity", IDENTITY_SIZE) != 0)) + { + add_infraction(INF_H2_NON_IDENTITY_TE); + create_event(EVENT_H2_NON_IDENTITY_TE); + } + if (get_header_value_norm(HEAD_CONTENT_LENGTH).length() > 0) + { + const int64_t content_length = + norm_decimal_integer(get_header_value_norm(HEAD_CONTENT_LENGTH)); + if (content_length >= 0) + session_data->data_length[source_id] = content_length; + else + { + add_infraction(INF_BAD_CONTENT_LENGTH); + create_event(EVENT_BAD_CONTENT_LENGTH); + } + } session_data->type_expected[source_id] = SEC_BODY_H2; prepare_body(); return; } - const Field& te_header = get_header_value_norm(HEAD_TRANSFER_ENCODING); if ((te_header.length() > 0) && (version_id == VERS_1_0)) { // HTTP 1.0 should not be chunked and many browsers will ignore the TE header diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc b/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc index 1e6f19367..85c600baf 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc +++ b/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc @@ -87,6 +87,7 @@ HttpCutter* HttpStreamSplitter::get_cutter(SectionType type, session_data->compression[source_id]); case SEC_BODY_H2: return (HttpCutter*)new HttpBodyH2Cutter( + session_data->data_length[source_id], session_data->detained_inspection[source_id], session_data->compression[source_id]); default: @@ -227,7 +228,8 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data const ScanResult cut_result = cutter->cut(data, (length <= max_length) ? length : max_length, session_data->get_infractions(source_id), session_data->events[source_id], session_data->section_size_target[source_id], - session_data->stretch_section_to_packet[source_id], session_data->http2_end_stream[source_id]); + session_data->stretch_section_to_packet[source_id], + session_data->h2_body_finished[source_id]); switch (cut_result) { case SCAN_NOT_FOUND: diff --git a/src/service_inspectors/http_inspect/http_tables.cc b/src/service_inspectors/http_inspect/http_tables.cc index d13fe4ff0..145579d33 100644 --- a/src/service_inspectors/http_inspect/http_tables.cc +++ b/src/service_inspectors/http_inspect/http_tables.cc @@ -380,6 +380,11 @@ const RuleMap HttpModule::http_events[] = { EVENT_GZIP_EARLY_END, "gzip compressed data followed by unexpected non-gzip " "data" }, { EVENT_EXCESS_REPEAT_PARAMS, "excessive HTTP parameter key repeats" }, + { EVENT_H2_NON_IDENTITY_TE, "HTTP/2 Transfer-Encoding header other than identity" }, + { EVENT_H2_DATA_OVERRUNS_CL, "HTTP/2 message body overruns Content-Length header " + "value" }, + { EVENT_H2_DATA_UNDERRUNS_CL, "HTTP/2 message body smaller than Content-Length header " + "value" }, { 0, nullptr } };