From: Tom Peters (thopeter) Date: Tue, 19 Oct 2021 20:05:15 +0000 (+0000) Subject: Merge pull request #3105 in SNORT/snort3 from ~THOPETER/snort3:nhttp160 to master X-Git-Tag: 3.1.15.0~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b61d7b430dfd3bfb378b8cd1160c77a6f61222e0;p=thirdparty%2Fsnort3.git Merge pull request #3105 in SNORT/snort3 from ~THOPETER/snort3:nhttp160 to master Squashed commit of the following: commit d2e095d8a54d8e358a6b0b8fb0c5f1f9c16afd31 Author: Tom Peters Date: Mon Oct 4 16:26:34 2021 -0400 http_inspect: hardening --- diff --git a/src/protocols/packet.h b/src/protocols/packet.h index 4a90e7454..34942e3ac 100644 --- a/src/protocols/packet.h +++ b/src/protocols/packet.h @@ -56,8 +56,7 @@ class SFDAQInstance; #define PKT_PDU_TAIL 0x00000200 /* end of PDU */ #define PKT_DETECT_LIMIT 0x00000400 /* alt_dsize is valid */ -#define PKT_ALLOW_MULTIPLE_DETECT 0x00000800 /* packet has either pipelined mime attachments - or pipeline http requests */ +#define PKT_ALLOW_MULTIPLE_DETECT 0x00000800 /* packet has multiple PDUs */ #define PKT_PAYLOAD_OBFUSCATE 0x00001000 #define PKT_STATELESS 0x00002000 /* Packet has matched a stateless rule */ diff --git a/src/service_inspectors/http_inspect/http_inspect.cc b/src/service_inspectors/http_inspect/http_inspect.cc index 25249d555..d9a65dc61 100755 --- a/src/service_inspectors/http_inspect/http_inspect.cc +++ b/src/service_inspectors/http_inspect/http_inspect.cc @@ -461,15 +461,23 @@ void HttpInspect::eval(Packet* p) const SourceId source_id = p->is_from_client() ? SRC_CLIENT : SRC_SERVER; HttpFlowData* session_data = http_get_flow_data(p->flow); + if (session_data == nullptr) + { + assert(false); + return; + } if (!session_data->for_http2) HttpModule::increment_peg_counts(PEG_TOTAL_BYTES, p->dsize); - // FIXIT-E Workaround for unexpected eval() calls. Convert to asserts when possible. + // FIXIT-M Workaround for unexpected eval() calls. Convert to asserts when possible. if ((session_data->section_type[source_id] == SEC__NOT_COMPUTE) || (session_data->type_expected[source_id] == SEC_ABORT) || (session_data->octets_reassembled[source_id] != p->dsize)) { + // assert(session_data->type_expected[source_id] != SEC_ABORT); + // assert(session_data->section_type[source_id] != SEC__NOT_COMPUTE); + // assert(session_data->octets_reassembled[source_id] == p->dsize); session_data->type_expected[source_id] = SEC_ABORT; return; } @@ -606,8 +614,11 @@ void HttpInspect::clear(Packet* p) HttpFlowData* const session_data = http_get_flow_data(p->flow); - if ( session_data == nullptr ) + if (session_data == nullptr) + { + // assert(false); // FIXIT-M something wrong with H2I Push Promise triggers this. return; + } Http2FlowData* h2i_flow_data = nullptr; if (Http2FlowData::inspector_id != 0) @@ -625,18 +636,19 @@ void HttpInspect::clear(Packet* p) else current_section = HttpContextData::clear_snapshot(p->context); - // FIXIT-M This test is necessary because sometimes we get extra clears - // Convert to assert when that gets fixed. if ( current_section == nullptr ) + { + // assert(false); // FIXIT-M this happens a lot return; + } current_section->clear(); HttpTransaction* current_transaction = current_section->get_transaction(); const SourceId source_id = current_section->get_source_id(); - //FIXIT-M This check may not apply to the transaction attached to the packet - //in case of offload. + // FIXIT-M This check may not apply to the transaction attached to the packet + // in case of offload. if (session_data->detection_status[source_id] == DET_DEACTIVATING) { if (source_id == SRC_CLIENT) diff --git a/src/service_inspectors/http_inspect/http_stream_splitter.h b/src/service_inspectors/http_inspect/http_stream_splitter.h index 0903ac255..cfd1090fa 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter.h +++ b/src/service_inspectors/http_inspect/http_stream_splitter.h @@ -43,7 +43,7 @@ public: const snort::StreamBuffer reassemble(snort::Flow* flow, unsigned total, unsigned, const uint8_t* data, unsigned len, uint32_t flags, unsigned& copied) override; bool finish(snort::Flow* flow) override; - bool prep_partial_flush(snort::Flow* flow, uint32_t num_flush); + void prep_partial_flush(snort::Flow* flow, uint32_t num_flush); bool is_paf() override { return true; } static StreamSplitter::Status status_value(StreamSplitter::Status ret_val, bool http2 = false); diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_finish.cc b/src/service_inspectors/http_inspect/http_stream_splitter_finish.cc index d0d799f12..15adf44aa 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter_finish.cc +++ b/src/service_inspectors/http_inspect/http_stream_splitter_finish.cc @@ -45,7 +45,10 @@ bool HttpStreamSplitter::finish(Flow* flow) HttpFlowData* session_data = HttpInspect::http_get_flow_data(flow); if (!session_data) + { + // assert(false); // FIXIT-M this should not be possible but currently it is return false; + } #ifdef REG_TEST if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP)) @@ -70,6 +73,13 @@ bool HttpStreamSplitter::finish(Flow* flow) return false; } + if (session_data->tcp_close[source_id]) + { + // assert(false); // FIXIT-M this should not happen but it does + session_data->type_expected[source_id] = SEC_ABORT; + return false; + } + session_data->tcp_close[source_id] = true; if (session_data->section_type[source_id] != SEC__NOT_COMPUTE) return true; @@ -203,11 +213,12 @@ bool HttpStreamSplitter::finish(Flow* flow) return false; } -bool HttpStreamSplitter::prep_partial_flush(Flow* flow, uint32_t num_flush) +void HttpStreamSplitter::prep_partial_flush(Flow* flow, uint32_t num_flush) { Profile profile(HttpModule::get_profile_stats()); HttpFlowData* session_data = HttpInspect::http_get_flow_data(flow); + assert(session_data != nullptr); #ifdef REG_TEST if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP) && @@ -226,6 +237,5 @@ bool HttpStreamSplitter::prep_partial_flush(Flow* flow, uint32_t num_flush) session_data->cutter[source_id]->get_num_good_chunks(), session_data->cutter[source_id]->get_octets_seen() - num_flush); session_data->partial_flush[source_id] = true; - return true; } 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 8ba5be460..21e75fcac 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc +++ b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc @@ -229,12 +229,14 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, { Profile profile(HttpModule::get_profile_stats()); - StreamBuffer http_buf { nullptr, 0 }; - copied = len; HttpFlowData* session_data = HttpInspect::http_get_flow_data(flow); - assert(session_data != nullptr); + if (session_data == nullptr) + { + assert(false); + return { nullptr, 0 }; + } #ifdef REG_TEST if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP)) @@ -243,7 +245,7 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, { if (!(flags & PKT_PDU_TAIL)) { - return http_buf; + return { nullptr, 0 }; } bool tcp_close; uint8_t* test_buffer; @@ -258,7 +260,7 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, { // Source ID does not match test data, no test data was flushed, preparing for a // TCP connection close, or there is no more test data - return http_buf; + return { nullptr, 0 }; } data = test_buffer; } @@ -272,54 +274,50 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, } #endif - assert(session_data->type_expected[source_id] != SEC_ABORT); - if (session_data->section_type[source_id] == SEC__NOT_COMPUTE) + if ((session_data->type_expected[source_id] == SEC_ABORT) || + (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 + session_data->type_expected[source_id] = SEC_ABORT; return { nullptr, 0 }; } // Sometimes it is necessary to reassemble zero bytes when a connection is closing to trigger // proper clean up. But even a zero-length buffer cannot be processed with a nullptr lest we // get in trouble with memcpy() (undefined behavior) or some library. - assert((data != nullptr) || (len == 0)); if (data == nullptr) + { + if (len != 0) + { + assert(false); + session_data->type_expected[source_id] = SEC_ABORT; + return { nullptr, 0 }; + } data = (const uint8_t*)""; + } uint8_t*& partial_buffer = session_data->partial_buffer[source_id]; uint32_t& partial_buffer_length = session_data->partial_buffer_length[source_id]; uint32_t& partial_raw_bytes = session_data->partial_raw_bytes[source_id]; assert(partial_raw_bytes + total <= MAX_OCTETS); - // FIXIT-E this is a precaution/workaround for stream issues. When they are fixed replace this - // block with an assert. if ((session_data->section_offset[source_id] == 0) && (session_data->octets_expected[source_id] != partial_raw_bytes + total)) { assert(!session_data->for_http2); - - if (session_data->octets_expected[source_id] == 0) - { - // FIXIT-E This is a known problem. No data was scanned and yet somehow stream can - // give us data when we ask for an empty message section. Dropping the unexpected data - // enables us to send the HTTP headers through detection as originally planned. - total = 0; - len = 0; - } - else - { -#ifdef REG_TEST - // FIXIT-M: known case: if session clears w/o a flush point, - // stream_tcp will flush to paf max which could be well below what - // has been scanned so far. since no flush point was specified, - // NHI should just deal with what it gets. - //assert(false); -#endif - return http_buf; - } + assert(total == 0); // FIXIT-L this special exception for total of zero is needed for now + session_data->type_expected[source_id] = SEC_ABORT; + return { nullptr, 0 }; } session_data->running_total[source_id] += len; - assert(session_data->running_total[source_id] <= total); + if (session_data->running_total[source_id] > total) + { + assert(false); + session_data->type_expected[source_id] = SEC_ABORT; + return { nullptr, 0 }; + } // FIXIT-P stream should be enhanced to do discarding for us. For now flush-then-discard here // is how scan() handles things we don't need to examine. @@ -356,7 +354,7 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, } } } - return http_buf; + return { nullptr, 0 }; } HttpModule::increment_peg_counts(PEG_REASSEMBLE); @@ -405,10 +403,17 @@ const StreamBuffer HttpStreamSplitter::reassemble(Flow* flow, unsigned total, chunk_spray(session_data, buffer, data, len); } + StreamBuffer http_buf { nullptr, 0 }; + if (flags & PKT_PDU_TAIL) { uint32_t& running_total = session_data->running_total[source_id]; - assert(running_total == total); + if (running_total != total) + { + assert(false); + session_data->type_expected[source_id] = SEC_ABORT; + return { nullptr, 0 }; + } running_total = 0; const uint32_t buf_size = session_data->section_offset[source_id] - session_data->num_excess[source_id]; 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 bf529f9de..21f6b233a 100644 --- a/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc +++ b/src/service_inspectors/http_inspect/http_stream_splitter_scan.cc @@ -128,9 +128,7 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data { Profile profile(HttpModule::get_profile_stats()); - assert(length <= MAX_OCTETS); - - Flow* flow = pkt->flow; + Flow* const flow = pkt->flow; // This is the session state information we share with HttpInspect and store with stream. A // session is defined by a TCP connection. Since scan() is the first to see a new TCP @@ -143,8 +141,6 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data HttpModule::increment_peg_counts(PEG_FLOW); } - SectionType type = session_data->type_expected[source_id]; - #ifdef REG_TEST if (HttpTestManager::use_test_input(HttpTestManager::IN_HTTP)) { @@ -160,9 +156,18 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data } #endif + SectionType& type = session_data->type_expected[source_id]; + if (type == SEC_ABORT) return status_value(StreamSplitter::ABORT); + if (length > MAX_OCTETS) + { + assert(false); + type = SEC_ABORT; + return status_value(StreamSplitter::ABORT); + } + #ifdef REG_TEST if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP) && !HttpTestManager::use_test_input(HttpTestManager::IN_HTTP)) @@ -178,6 +183,13 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data } #endif + if (session_data->tcp_close[source_id]) + { + // assert(false); // FIXIT-L This currently happens. Add assert back when problem resolved. + type = SEC_ABORT; + return status_value(StreamSplitter::ABORT); + } + // If the last request was a CONNECT and we have not yet seen the response, this is early C2S // traffic. If there has been a pipeline overflow or underflow we cannot match requests to // responses, so there is no attempt to track early C2S traffic. @@ -203,8 +215,6 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data session_data->last_request_was_connect = false; } - assert(!session_data->tcp_close[source_id]); - HttpModule::increment_peg_counts(PEG_SCAN); if (session_data->detection_status[source_id] == DET_REACTIVATING) @@ -227,7 +237,6 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data // 0.9 response is a body that runs to connection end with no headers. HttpInspect does // not support no headers. Processing this imaginary status line and empty headers allows // us to overcome this limitation and reuse the entire HTTP infrastructure. - type = SEC_BODY_OLD; prepare_flush(session_data, nullptr, SEC_STATUS, 14, 0, 0, false, 0, 14); my_inspector->process((const uint8_t*)"HTTP/0.9 200 .", 14, flow, SRC_SERVER, false); session_data->transaction[SRC_SERVER]->clear_section(); @@ -258,7 +267,7 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data // should be an unconditional EVENT_LOSS_OF_SYNC. session_data->events[source_id]->generate_misformatted_http(data, length); // FIXIT-E need to process this data not just discard it. - session_data->type_expected[source_id] = SEC_ABORT; + type = SEC_ABORT; delete cutter; cutter = nullptr; return status_value(StreamSplitter::ABORT); @@ -277,7 +286,7 @@ StreamSplitter::Status HttpStreamSplitter::scan(Packet* pkt, const uint8_t* data // Wait patiently for more data return status_value(StreamSplitter::SEARCH); case SCAN_ABORT: - session_data->type_expected[source_id] = SEC_ABORT; + type = SEC_ABORT; delete cutter; cutter = nullptr; return status_value(StreamSplitter::ABORT);