From: Mike Stepanek (mstepane) Date: Tue, 5 Nov 2019 14:25:08 +0000 (-0500) Subject: Merge pull request #1827 in SNORT/snort3 from ~KATHARVE/snort3:h2i_code_coverage... X-Git-Tag: 3.0.0-264~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6f5d202007f5a4ef829ff567d4f88f29543f0204;p=thirdparty%2Fsnort3.git Merge pull request #1827 in SNORT/snort3 from ~KATHARVE/snort3:h2i_code_coverage to master Squashed commit of the following: commit fb6ef30804b7463b132fac75af68005fe9fce16e Author: Katura Harvey Date: Mon Oct 28 14:53:03 2019 -0400 http2_inspect: fix bugs in splitting long data frames and padding --- diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.cc b/src/service_inspectors/http2_inspect/http2_flow_data.cc index 438c85b3b..a7a611088 100644 --- a/src/service_inspectors/http2_inspect/http2_flow_data.cc +++ b/src/service_inspectors/http2_inspect/http2_flow_data.cc @@ -82,8 +82,12 @@ Http2FlowData::~Http2FlowData() void Http2FlowData::clear_frame_data(HttpCommon::SourceId source_id) { - delete[] frame_header[source_id]; - frame_header[source_id] = nullptr; + // If there is more data to be inspected in the frame, leave the frame_header + if (leftover_data[source_id] == 0) + { + delete[] frame_header[source_id]; + frame_header[source_id] = nullptr; + } delete[] frame_data[source_id]; frame_data[source_id] = nullptr; frame_in_detection = false; diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc index 25b576d98..043abbae6 100644 --- a/src/service_inspectors/http2_inspect/http2_hpack.cc +++ b/src/service_inspectors/http2_inspect/http2_hpack.cc @@ -51,7 +51,7 @@ bool Http2Hpack::write_decoded_headers(Http2FlowData* session_data, HttpCommon:: if (in_length > decoded_header_length) { - length = MAX_OCTETS - session_data->raw_decoded_header_size[source_id]; + length = decoded_header_length; *session_data->infractions[source_id] += INF_DECODED_HEADER_BUFF_OUT_OF_SPACE; session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2); ret = false; @@ -82,9 +82,9 @@ bool Http2Hpack::decode_string_literal(Http2FlowData* session_data, HttpCommon:: } if (!decode_string.translate(encoded_header_buffer + encoded_header_offset, - encoded_header_length, encoded_bytes_consumed, decoded_header_buffer, - decoded_header_length, decoded_bytes_written, session_data->events[source_id], - session_data->infractions[source_id])) + encoded_header_length - encoded_header_offset, encoded_bytes_consumed, + decoded_header_buffer, decoded_header_length, decoded_bytes_written, + session_data->events[source_id], session_data->infractions[source_id])) { return false; } @@ -257,7 +257,10 @@ bool Http2Hpack::decode_literal_header_line(Http2FlowData* session_data, if (!Http2Hpack::decode_index(session_data, source_id, encoded_header_buffer, encoded_header_length, decode_int, false, partial_bytes_consumed, decoded_header_buffer, decoded_header_length, partial_bytes_written)) + { + bytes_written += partial_bytes_written; return false; + } } // literal field name else @@ -266,8 +269,10 @@ bool Http2Hpack::decode_literal_header_line(Http2FlowData* session_data, encoded_header_length, true, partial_bytes_consumed, decoded_header_buffer, decoded_header_length, partial_bytes_written)) + { + bytes_written += partial_bytes_written; return false; - + } // If this was a pseudo-header value, give it to the start-line. if (session_data->header_start_line[source_id] and session_data->header_start_line[source_id]->is_pseudo_name( @@ -298,7 +303,10 @@ bool Http2Hpack::decode_literal_header_line(Http2FlowData* session_data, false, partial_bytes_consumed, decoded_header_buffer + bytes_written, decoded_header_length - bytes_written, partial_bytes_written)) + { + bytes_written += partial_bytes_written; return false; + } // If this was a pseudo-header value, give it to the start-line. if (session_data->header_start_line[source_id] and @@ -401,55 +409,33 @@ bool Http2Hpack::decode_headers(Http2FlowData* session_data, HttpCommon::SourceI if (source_id == SRC_CLIENT) session_data->header_start_line[source_id] = new Http2RequestLine(session_data, source_id); - while (total_bytes_consumed < header_length) + while (success and total_bytes_consumed < header_length) { - if (!Http2Hpack::decode_header_line(session_data, source_id, + success = Http2Hpack::decode_header_line(session_data, source_id, encoded_header_buffer + total_bytes_consumed, header_length - total_bytes_consumed, line_bytes_consumed, session_data->raw_decoded_header[source_id] + session_data->raw_decoded_header_size[source_id], MAX_OCTETS - - session_data->raw_decoded_header_size[source_id], line_bytes_written)) - { - success = false; - break; - } + session_data->raw_decoded_header_size[source_id], line_bytes_written); total_bytes_consumed += line_bytes_consumed; session_data->raw_decoded_header_size[source_id] += line_bytes_written; } - if (!success) - { -#ifdef REG_TEST - if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2)) - { - fprintf(HttpTestManager::get_output_file(), "Error decoding headers.\n"); - if (session_data->header_start_line[source_id] and - session_data->header_start_line[source_id]->get_start_line().length() > 0) - session_data->header_start_line[source_id]->get_start_line(). - print(HttpTestManager::get_output_file(), "Decoded start-line"); - if (session_data->raw_decoded_header_size[source_id] > 0) - Field(session_data->raw_decoded_header_size[source_id], - session_data->raw_decoded_header[source_id]).print( - HttpTestManager::get_output_file(), "Partially decoded raw header"); - } -#endif - return false; - } - // If there were only pseudo-headers, finalize never got called, so create the start-line if (session_data->header_start_line[source_id] and !session_data->header_start_line[source_id]->is_finalized()) { - if (!session_data->header_start_line[source_id]->finalize()) - return false; + success &= session_data->header_start_line[source_id]->finalize(); } // write the last CRLF to end the header - if (!Http2Hpack::write_decoded_headers(session_data, source_id, (const uint8_t*)"\r\n", 2, - session_data->raw_decoded_header[source_id] + - session_data->raw_decoded_header_size[source_id], MAX_OCTETS - - session_data->raw_decoded_header_size[source_id], line_bytes_written)) - return false; - session_data->raw_decoded_header_size[source_id] += line_bytes_written; + if (success) + { + success = Http2Hpack::write_decoded_headers(session_data, source_id, + (const uint8_t*)"\r\n", 2, session_data->raw_decoded_header[source_id] + + session_data->raw_decoded_header_size[source_id], MAX_OCTETS - + session_data->raw_decoded_header_size[source_id], line_bytes_written); + session_data->raw_decoded_header_size[source_id] += line_bytes_written; + } // set http2_decoded_header to send to NHI session_data->http2_decoded_header[source_id] = new Field( @@ -458,14 +444,30 @@ bool Http2Hpack::decode_headers(Http2FlowData* session_data, HttpCommon::SourceI session_data->raw_decoded_header[source_id] + session_data->pseudo_header_fragment_size[source_id], false); + #ifdef REG_TEST if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2)) { - if (session_data->header_start_line[source_id]) - session_data->header_start_line[source_id]->get_start_line(). - print(HttpTestManager::get_output_file(), "Decoded start-line"); - session_data->http2_decoded_header[source_id]-> - print(HttpTestManager::get_output_file(), "Decoded header"); + if (success) + { + if (session_data->header_start_line[source_id]) + session_data->header_start_line[source_id]->get_start_line(). + print(HttpTestManager::get_output_file(), "Decoded start-line"); + session_data->http2_decoded_header[source_id]-> + print(HttpTestManager::get_output_file(), "Decoded header"); + } + else + { + fprintf(HttpTestManager::get_output_file(), "Error decoding headers.\n"); + if (session_data->header_start_line[source_id] and + session_data->header_start_line[source_id]->get_start_line().length() > 0) + session_data->header_start_line[source_id]->get_start_line(). + print(HttpTestManager::get_output_file(), "Decoded start-line"); + if (session_data->raw_decoded_header_size[source_id] > 0) + Field(session_data->raw_decoded_header_size[source_id], + session_data->raw_decoded_header[source_id]).print( + HttpTestManager::get_output_file(), "Partially decoded raw header"); + } } #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 cd250c3c6..9120a3259 100644 --- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc +++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc @@ -107,7 +107,8 @@ StreamSplitter::Status implement_scan(Http2FlowData* session_data, const uint8_t } // Have full inspection section, flush and update leftover - *flush_offset = session_data->inspection_section_length[source_id]; + *flush_offset = session_data->inspection_section_length[source_id] - + session_data->octets_seen[source_id]; session_data->leftover_data[source_id] -= session_data->inspection_section_length[source_id]; session_data->octets_seen[source_id] = 0; @@ -318,13 +319,22 @@ const StreamBuffer implement_reassemble(Http2FlowData* session_data, unsigned to frame_length = get_frame_length(session_data->frame_header[source_id] + session_data->frame_header_offset[source_id]); + + if (frame_length == 0) + break; + if (remaining_len == 0) + return frame_buf; + + // handle split long data frames + if (frame_length > total) + frame_length = total - data_pos; frame_flags = get_frame_flags(session_data->frame_header[source_id] + session_data->frame_header_offset[source_id]); if (frame_flags & PADDED) { frame_data_offset += 1; - pad_len = session_data->frame_data[source_id][0]; + pad_len = *(data + data_pos); } //FIXIT-M handle stream dependency and weight. For now just skip over if (frame_flags & PRIORITY)