From 8cfee35cbc84e0fbfa3f35decd686e5821284520 Mon Sep 17 00:00:00 2001 From: "Mike Stepanek (mstepane)" Date: Mon, 23 Nov 2020 19:01:27 +0000 Subject: [PATCH] Merge pull request #2618 in SNORT/snort3 from ~THOPETER/snort3:h2i17 to master Squashed commit of the following: commit 58296aa1e56005645325b178504e68f3278b7f0d Author: Tom Peters Date: Mon Nov 9 12:36:10 2020 -0500 http2_inspect: improve error handling --- .../http2_inspect/http2_data_cutter.cc | 25 ++-- .../http2_inspect/http2_frame.cc | 2 + .../http2_inspect/http2_headers_frame.cc | 9 +- .../http2_inspect/http2_settings_frame.cc | 2 +- .../http2_inspect/http2_stream.cc | 2 + .../http2_inspect/http2_stream_splitter.cc | 28 ---- .../http2_inspect/http2_stream_splitter.h | 4 - .../http2_stream_splitter_impl.cc | 125 +++++++++--------- 8 files changed, 81 insertions(+), 116 deletions(-) diff --git a/src/service_inspectors/http2_inspect/http2_data_cutter.cc b/src/service_inspectors/http2_inspect/http2_data_cutter.cc index 70cb4f770..31a378159 100644 --- a/src/service_inspectors/http2_inspect/http2_data_cutter.cc +++ b/src/service_inspectors/http2_inspect/http2_data_cutter.cc @@ -90,15 +90,8 @@ StreamSplitter::Status Http2DataCutter::scan(const uint8_t* data, uint32_t lengt { bytes_sent_http += cur_data; } - else if (scan_result == StreamSplitter::ABORT) - { - // FIXIT-E eventually need to implement continued processing. We cannot abort just - // because one stream went sideways. A better approach would be to put this one stream - // into a pass through mode while continuing to process other streams. As long as we - // can parse the framing and process most streams it is reasonable to continue. - session_data->stream_in_hi = NO_STREAM_ID; - return StreamSplitter::ABORT; - } + else + assert(false); } if (data_bytes_read == data_len) @@ -172,10 +165,11 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len) reassemble_state = GET_PADDING_LEN; else if (reassemble_data_len > 0) reassemble_state = SEND_DATA; - else if (frame_flags & END_STREAM) - reassemble_state = SEND_EMPTY_DATA; else - reassemble_state = GET_FRAME_HDR; + { + assert(frame_flags & END_STREAM); + reassemble_state = SEND_EMPTY_DATA; + } } break; } @@ -187,10 +181,11 @@ void Http2DataCutter::reassemble(const uint8_t* data, unsigned len) cur_data_offset++; if (reassemble_data_len > 0) reassemble_state = SEND_DATA; - else if (get_frame_flags(session_data->lead_frame_header[source_id]) & END_STREAM) - reassemble_state = SEND_EMPTY_DATA; else - reassemble_state = GET_FRAME_HDR; + { + assert(get_frame_flags(session_data->lead_frame_header[source_id]) & END_STREAM); + reassemble_state = SEND_EMPTY_DATA; + } break; } case SEND_EMPTY_DATA: diff --git a/src/service_inspectors/http2_inspect/http2_frame.cc b/src/service_inspectors/http2_inspect/http2_frame.cc index b18ad1236..05be984ff 100644 --- a/src/service_inspectors/http2_inspect/http2_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_frame.cc @@ -46,6 +46,8 @@ Http2Frame::Http2Frame(const uint8_t* header_buffer, const uint32_t header_len, // FIXIT-E want to refactor so that zero-length frames are not a special case if (data_buffer != nullptr) data.set(data_len, data_buffer, true); + else + data.set(0, new uint8_t[0], true); } Http2Frame* Http2Frame::new_frame(const uint8_t* header, const uint32_t header_len, diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc index 70d0b1f75..c3e40bbdb 100644 --- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc @@ -45,13 +45,6 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_ HttpCommon::SourceId source_id_, Http2Stream* stream_) : Http2Frame(header_buffer, header_len, data_buffer, data_len, session_data_, source_id_, stream_) { - // FIXIT-E zero length should not be a special case - if (data.length() <= 0) - { - process_frame = false; - return; - } - // Remove stream dependency if present if (get_flags() & PRIORITY) hpack_headers_offset = 5; @@ -146,7 +139,7 @@ const Field& Http2HeadersFrame::get_buf(unsigned id) { switch (id) { - // FIXIT-M need to add a buffer for the decoded start line + // FIXIT-E need to add a buffer for the decoded start line case HTTP2_BUFFER_DECODED_HEADER: return http1_header; default: diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.cc b/src/service_inspectors/http2_inspect/http2_settings_frame.cc index 8021fcb22..b0144f021 100644 --- a/src/service_inspectors/http2_inspect/http2_settings_frame.cc +++ b/src/service_inspectors/http2_inspect/http2_settings_frame.cc @@ -94,7 +94,7 @@ bool Http2SettingsFrame::sanity_check() // FIXIT-E this next check should possibly be moved to valid_sequence() if (get_stream_id() != 0) bad_frame = true; - else if (!ack and ((data.length() % 6) != 0)) + else if (!ack and ((data.length() <= 0) or ((data.length() % 6) != 0))) bad_frame = true; else if (ack and data.length() > 0) bad_frame = true; diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc index 599b19f06..5aed057ec 100644 --- a/src/service_inspectors/http2_inspect/http2_stream.cc +++ b/src/service_inspectors/http2_inspect/http2_stream.cc @@ -90,6 +90,8 @@ void Http2Stream::set_state(HttpCommon::SourceId source_id, StreamState new_stat { assert((STREAM_EXPECT_HEADERS <= new_state) && (new_state <= STREAM_ERROR)); assert(state[source_id] < new_state); + assert((new_state < STREAM_EXPECT_BODY) || (new_state > STREAM_BODY) || + (get_hi_flow_data() != nullptr)); state[source_id] = new_state; } diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc index e92f352aa..59dfcd1f0 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc @@ -263,31 +263,3 @@ bool Http2StreamSplitter::finish(Flow* flow) return need_reassemble; } -bool Http2StreamSplitter::init_partial_flush(Flow* flow) -{ - Profile profile(Http2Module::get_profile_stats()); - - if (source_id != SRC_SERVER) - { - assert(false); - return false; - } - - Http2FlowData* session_data = (Http2FlowData*)flow->get_flow_data(Http2FlowData::inspector_id); - assert(session_data != nullptr); - if (session_data->abort_flow[source_id]) - return false; - -#ifdef REG_TEST - if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2) && - !HttpTestManager::use_test_input(HttpTestManager::IN_HTTP2)) - { - printf("HTTP/2 partial flush from flow data %" PRIu64 "\n", session_data->seq_num); - fflush(stdout); - } -#endif - - // FIXIT-E not supported yet - return false; -} - diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.h b/src/service_inspectors/http2_inspect/http2_stream_splitter.h index 3ce51ea63..de8d04c7e 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter.h +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.h @@ -40,7 +40,6 @@ public: const snort::StreamBuffer reassemble(snort::Flow* flow, unsigned total, unsigned offset, const uint8_t* data, unsigned len, uint32_t flags, unsigned& copied) override; bool finish(snort::Flow* flow) override; - bool init_partial_flush(snort::Flow* flow) override; bool is_paf() override { return true; } // FIXIT-M should return actual packet buffer size @@ -51,9 +50,6 @@ private: 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 StreamSplitter::Status non_data_frame_header_checks( - Http2FlowData* session_data, HttpCommon::SourceId source_id, uint32_t frame_length, - uint8_t type); 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); 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 4e0b592a8..018e66e79 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc @@ -71,66 +71,34 @@ StreamSplitter::Status Http2StreamSplitter::data_frame_header_checks(Http2FlowDa if (!stream || !stream->is_open(source_id)) { - *session_data->infractions[source_id] += INF_FRAME_SEQUENCE; - session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE); - // FIXIT-E We should not be aborting here - return StreamSplitter::ABORT; - } - - HttpFlowData* const http_flow = (HttpFlowData*)stream->get_hi_flow_data(); - if (http_flow == nullptr) - { - *session_data->infractions[source_id] += INF_FRAME_SEQUENCE; - session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE); - return StreamSplitter::ABORT; - } - - if (http_flow->get_type_expected(source_id) != HttpEnums::SEC_BODY_H2) - { - // If 0 length frame and http isn't expecting body, flush without involving http - if (frame_length == 0) + if (!(stream && (stream->get_state(source_id) == STREAM_ERROR))) { - *flush_offset = data_offset; - session_data->header_octets_seen[source_id] = 0; - session_data->scan_state[source_id] = SCAN_FRAME_HEADER; - return StreamSplitter::FLUSH; + *session_data->infractions[source_id] += INF_FRAME_SEQUENCE; + session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE); + if (stream) + { + // FIXIT-E need to do this even if the stream does not exist yet + stream->set_state(source_id, STREAM_ERROR); + } } - - *session_data->infractions[source_id] += INF_FRAME_SEQUENCE; - session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE); - // FIXIT-E We should not be aborting here - return StreamSplitter::ABORT; + return StreamSplitter::SEARCH; } - if (frame_length > MAX_OCTETS) - return StreamSplitter::ABORT; - - return StreamSplitter::SEARCH; -} - -StreamSplitter::Status Http2StreamSplitter::non_data_frame_header_checks( - Http2FlowData* session_data, HttpCommon::SourceId source_id, uint32_t frame_length, - uint8_t type) -{ - // Compute frame section length once per frame - if (frame_length + FRAME_HEADER_LENGTH > MAX_OCTETS) - { - // FIXIT-M long non-data frame needs to be supported - return StreamSplitter::ABORT; - } + HttpFlowData* const http_flow = (HttpFlowData*)stream->get_hi_flow_data(); + assert(http_flow != nullptr); - if (type == FT_CONTINUATION and !session_data->continuation_expected[source_id]) + // 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)) { - *session_data->infractions[source_id] += INF_UNEXPECTED_CONTINUATION; - session_data->events[source_id]->create_event( - EVENT_UNEXPECTED_CONTINUATION); - return StreamSplitter::ABORT; + *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, uint32_t length, uint32_t* flush_offset, HttpCommon::SourceId source_id, uint8_t type, uint8_t frame_flags, uint32_t& data_offset) @@ -258,12 +226,26 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio 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; + } + 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 + // FIXIT-E need an alert and infraction + return StreamSplitter::ABORT; + } + assert(session_data->scan_remaining_frame_octets[source_id] == 0); session_data->scan_remaining_frame_octets[source_id] = frame_length; @@ -300,9 +282,6 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio status = data_frame_header_checks(session_data, flush_offset, source_id, frame_length, data_offset); } - else - status = non_data_frame_header_checks(session_data, source_id, frame_length, - type); break; } @@ -340,16 +319,44 @@ StreamSplitter::Status Http2StreamSplitter::implement_scan(Http2FlowData* sessio session_data->scan_frame_header[source_id]); const uint8_t frame_flags = get_frame_flags(session_data-> scan_frame_header[source_id]); - if (session_data->frame_type[source_id] == FT_DATA) + if (session_data->frame_type[source_id] != FT_DATA) { - status = session_data->data_cutter[source_id].scan( - data, length, flush_offset, data_offset, - frame_length - session_data->padding_length[source_id], frame_flags); + status = non_data_scan(session_data, length, flush_offset, source_id, type, + frame_flags, data_offset); } else { - status = non_data_scan(session_data, length, flush_offset, source_id, type, - frame_flags, data_offset); + Http2Stream* const stream = + session_data->find_stream(session_data->current_stream[source_id]); + if (stream && stream->is_open(source_id)) + { + status = session_data->data_cutter[source_id].scan( + data, length, flush_offset, data_offset, + frame_length - session_data->padding_length[source_id], frame_flags); + } + else + { + // Need to skip past Data frame in a bad stream + 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; + assert(!session_data->frame_lengths[source_id].empty()); + session_data->frame_lengths[source_id].pop(); + assert(session_data->frame_lengths[source_id].empty()); + } + + session_data->payload_discard[source_id] = true; + status = StreamSplitter::FLUSH; + } } assert(status != StreamSplitter::SEARCH or session_data->scan_state[source_id] != SCAN_EMPTY_DATA); @@ -381,8 +388,6 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess { if (offset == 0) { - session_data->frame_header_offset[source_id] = 0; - // This is the first reassemble() for this frame - allocate data buffer session_data->frame_data_size[source_id] = total - (session_data->frame_lengths[source_id].size() * FRAME_HEADER_LENGTH); -- 2.47.3