From: Tom Peters Date: Thu, 16 Oct 2014 18:58:49 +0000 (-0400) Subject: code review in progress X-Git-Tag: 3.0.0-233~1357^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=f40f7d0c75a9c672ea2bab3634a5fceda358f7eb;p=thirdparty%2Fsnort3.git code review in progress --- diff --git a/src/service_inspectors/nhttp_inspect/nhttp_inspect.cc b/src/service_inspectors/nhttp_inspect/nhttp_inspect.cc index ded9080cf..c40e5bd5e 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_inspect.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_inspect.cc @@ -75,7 +75,8 @@ bool NHttpInspect::get_buf(unsigned id, Packet*, InspectionBuffer& b) return true; } -ProcessResult NHttpInspect::process(const uint8_t* data, const uint16_t dsize, Flow* const flow, SourceId source_id, bool buf_owner) +ProcessResult NHttpInspect::process(const uint8_t* data, const uint16_t dsize, Flow* const flow, SourceId source_id, + bool buf_owner) { NHttpFlowData* session_data = (NHttpFlowData*)flow->get_application_data(NHttpFlowData::nhttp_flow_id); assert(session_data != nullptr); diff --git a/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc b/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc index c66d2c5fe..5e37fd80e 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc @@ -75,10 +75,10 @@ void NHttpMsgStart::gen_events() {} ProcessResult NHttpMsgStart::worth_detection() { // We combine the start line with the headers for sending to detection if they are already available and we will - // not exceed paf_max. + // not exceed maximum size. if ((session_data->header_octets_visible[source_id] > 0) && (session_data->type_expected[source_id] == SEC_HEADER) && - (msg_text.length + session_data->header_octets_visible[source_id]) <= 63780) { + (msg_text.length + session_data->header_octets_visible[source_id]) <= MAXOCTETS) { return RES_AGGREGATE; } else { diff --git a/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc b/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc index 82db37c55..687f76824 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc @@ -177,7 +177,7 @@ StreamSplitter::Status NHttpStreamSplitter::scan (Flow* flow, const uint8_t* dat } } case SEC_BODY: { - prepare_flush(session_data, flush_offset, source_id, type, + prepare_flush(session_data, flush_offset, source_id, SEC_BODY, tcp_close && (length <= session_data->data_length[source_id]), 0, session_data->data_length[source_id], length, 0); return StreamSplitter::FLUSH; @@ -236,7 +236,7 @@ const StreamBuffer* NHttpStreamSplitter::reassemble(Flow* flow, unsigned /* tota return nullptr; } - bool is_chunk = session_data->section_type[source_id] == SEC_CHUNK; + bool is_chunk = (session_data->section_type[source_id] == SEC_CHUNK); uint8_t*& chunk_buffer = session_data->chunk_buffer[source_id]; int32_t& chunk_buffer_length = session_data->chunk_buffer_length[source_id]; @@ -258,8 +258,8 @@ const StreamBuffer* NHttpStreamSplitter::reassemble(Flow* flow, unsigned /* tota if (!is_chunk) { // start line/headers/body individual section processing with aggregation prior to being sent to detection // only the last section added to the buffer goes to the inspector - send_to_detection = my_inspector->process(buffer + buffer_length, offset + len - num_excess, flow, source_id, - buffer_length == 0); + send_to_detection = my_inspector->process(buffer + buffer_length, offset + len - num_excess, flow, + source_id, buffer_length == 0); } else { // small chunks are aggregated before processing and are kept here until the buffer is full (paf_max) diff --git a/src/service_inspectors/nhttp_inspect/nhttp_test_input.cc b/src/service_inspectors/nhttp_inspect/nhttp_test_input.cc index ba1b9a8ae..437cd2dc6 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_test_input.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_test_input.cc @@ -19,17 +19,14 @@ // nhttp_test_input.cc author Tom Peters #include -#include -#include #include -#include #include "nhttp_test_manager.h" #include "nhttp_test_input.h" using namespace NHttpEnums; -NHttpTestInput::NHttpTestInput(const char *file_name) { +NHttpTestInput::NHttpTestInput(const char* file_name) { if ((test_data_file = fopen(file_name, "r")) == nullptr) throw std::runtime_error("Cannot open test input file"); } @@ -37,7 +34,7 @@ NHttpTestInput::~NHttpTestInput() { fclose(test_data_file); } -// Read from the test data file and present to PAF. +// Read from the test data file and present to StreamSplitter. // In the process we may need to skip comments, execute simple commands, and handle escape sequences. // The best way to understand this function is to read the comments at the top of the file of test cases. void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, bool &tcp_close, bool &need_break) { @@ -52,27 +49,27 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, need_break = false; if (just_flushed) { - // PAF just flushed and it has all been sent to inspection. There may or may not be leftover data from the - // last segment that was not flushed. + // StreamSplitter just flushed and it has all been sent by reassemble. There may or may not be leftover data + // from the last paragraph that was not flushed. just_flushed = false; data = msg_buf; length = end_offset - flush_octets; // this is the leftover data previous_offset = 0; end_offset = length; if (length > 0) { - // Must present unflushed leftovers to PAF again. - // If we don't take this opportunity to left justify our data in the buffer we may "walk" to the right until we run out of buffer space + // Must present unflushed leftovers to StreamSplitter again. If we don't take this opportunity to left + // justify our data in the buffer we may "walk" to the right until we run out of buffer space. memmove(msg_buf, msg_buf+flush_octets, length); tcp_close = tcp_closed; flush_octets = 0; return; } - // If we reach here then PAF has already flushed all the data we have read so far. + // If we reach here then StreamSplitter has already flushed all the data we have read so far. tcp_closed = false; flush_octets = 0; } else { - // The data we gave PAF last time was not flushed + // The data we gave StreamSplitter last time was not flushed length = 0; previous_offset = end_offset; data = msg_buf + previous_offset; @@ -80,7 +77,7 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, // Now we need to move forward by reading more data from the file int new_char; - typedef enum { WAITING, COMMENT, COMMAND, SECTION, ESCAPE, HEXVAL } State; + typedef enum { WAITING, COMMENT, COMMAND, PARAGRAPH, ESCAPE, HEXVAL } State; State state = WAITING; bool ending = false; int command_length = 0; @@ -104,7 +101,7 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, ending = false; } else if (new_char != '\n') { - state = SECTION; + state = PARAGRAPH; ending = false; data[length++] = (uint8_t) new_char; } @@ -117,6 +114,8 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, case COMMAND: if (new_char == '\n') { state = WAITING; + // FIXIT-L should not change direction with unflushed data remaining from previous paragraph. At the + // minimum need to test for this and assert. if ((command_length == strlen("request")) && !memcmp(command_value, "request", strlen("request"))) { source_id = last_source_id = SRC_CLIENT; } @@ -158,14 +157,14 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, } } break; - case SECTION: + case PARAGRAPH: if (new_char == '\\') { state = ESCAPE; ending = false; } else if (new_char == '\n') { if (ending) { - // Found the blank line that ends the section. + // Found the second consecutive blank line that ends the paragraph. end_offset = previous_offset + length; return; } @@ -178,15 +177,15 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, break; case ESCAPE: switch (new_char) { - case 'n': state = SECTION; data[length++] = '\n'; break; - case 'r': state = SECTION; data[length++] = '\r'; break; - case 't': state = SECTION; data[length++] = '\t'; break; - case '#': state = SECTION; data[length++] = '#'; break; - case '@': state = SECTION; data[length++] = '@'; break; - case '\\': state = SECTION; data[length++] = '\\'; break; + case 'n': state = PARAGRAPH; data[length++] = '\n'; break; + case 'r': state = PARAGRAPH; data[length++] = '\r'; break; + case 't': state = PARAGRAPH; data[length++] = '\t'; break; + case '#': state = PARAGRAPH; data[length++] = '#'; break; + case '@': state = PARAGRAPH; data[length++] = '@'; break; + case '\\': state = PARAGRAPH; data[length++] = '\\'; break; case 'x': case 'X': state = HEXVAL; hex_val = 0; num_digits = 0; break; - default: assert(0); state = SECTION; break; + default: assert(0); state = PARAGRAPH; break; } break; case HEXVAL: @@ -196,12 +195,12 @@ void NHttpTestInput::scan(uint8_t*& data, uint32_t &length, SourceId &source_id, else assert(0); if (++num_digits == 2) { data[length++] = hex_val; - state = SECTION; + state = PARAGRAPH; } break; } // Don't allow a buffer overrun. - if (previous_offset + length >= sizeof(msg_buf)) assert(0); + assert(previous_offset + length < sizeof(msg_buf)); } // End-of-file. Return everything we have so far. end_offset = previous_offset + length; @@ -213,8 +212,7 @@ void NHttpTestInput::flush(uint32_t length) { flushed = true; } - -void NHttpTestInput::reassemble(uint8_t **buffer, unsigned &length, SourceId source_id, const NHttpFlowData* session_data, +void NHttpTestInput::reassemble(uint8_t** buffer, unsigned& length, SourceId source_id, const NHttpFlowData* session_data, bool& tcp_close) { if (!flushed || (source_id != last_source_id)) { *buffer = nullptr; @@ -232,8 +230,8 @@ void NHttpTestInput::reassemble(uint8_t **buffer, unsigned &length, SourceId sou else { // We need to generate additional data to fill out the body or chunk section. We may come through here // multiple times as we generate all the maximum size body sections needed for a single flush. - unsigned paf_max = 16384 - session_data->chunk_buffer_length[source_id]; tcp_close = false; + const unsigned paf_max = DATABLOCKSIZE - session_data->chunk_buffer_length[source_id]; length = (flush_octets <= paf_max) ? flush_octets : paf_max; for (uint32_t k = end_offset; k < length; k++) { msg_buf[k] = 'A' + k % 26; @@ -247,6 +245,3 @@ void NHttpTestInput::reassemble(uint8_t **buffer, unsigned &length, SourceId sou } } - - - diff --git a/src/service_inspectors/nhttp_inspect/nhttp_test_input.h b/src/service_inspectors/nhttp_inspect/nhttp_test_input.h index 175256a9a..a2ecb8dfc 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_test_input.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_test_input.h @@ -21,6 +21,8 @@ #ifndef NHTTP_TEST_INPUT_H #define NHTTP_TEST_INPUT_H +#include + #include "nhttp_enum.h" #include "nhttp_flow_data.h" @@ -36,13 +38,27 @@ public: private: FILE* test_data_file; uint8_t msg_buf[2 * NHttpEnums::MAXOCTETS]; + + // data has been flushed and must be sent by reassemble() before more data may be given to scan() bool flushed = false; - NHttpEnums::SourceId last_source_id = NHttpEnums::SRC_CLIENT; // current direction of traffic flow. Toggled by commands in file. - bool just_flushed = true; // all octets sent to inspection and must resume reading the file - bool tcp_closed = false; // so we can keep presenting a TCP close to PAF until all the remaining octets are consumed and flushed - uint32_t flush_octets = 0; // number of octets that have been flushed and must go to inspection - uint32_t previous_offset = 0; // last character in the buffer shown to PAF but not flushed yet - uint32_t end_offset = 0; // last read character in the buffer + + // current direction of traffic flow. Toggled by commands in file. + NHttpEnums::SourceId last_source_id = NHttpEnums::SRC_CLIENT; + + // reassemble just completed and all flushed octets forwarded, time to resume scan() + bool just_flushed = true; + + // TCP connection directional close at end of current paragraph + bool tcp_closed = false; + + // number of octets that have been flushed and must be sent by reassemble + uint32_t flush_octets = 0; + + // last character in the buffer previously shown to PAF but not flushed yet + uint32_t previous_offset = 0; + + // last read character in the buffer + uint32_t end_offset = 0; }; #endif diff --git a/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt b/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt index 3b7b40d89..c4e86aec3 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt +++ b/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt @@ -805,6 +805,25 @@ Transfer-Encoding: chunked\r\n 0\r\n\r\n +@13006 +@break +@request + +GET /wall/fruitwall_file.txt HTTP/1.1 + +\r\nHost: www.hektik.org\r\n\r\n + +@response + +HTTP/1.1 200 OK\r\n +Date: Fri, 01 Aug 2003 21:22:16 GMT\r\n +Server: Apache/1.3.27 (Unix) mod_ssl/2.8.12 OpenSSL/0.9.7-beta3 PHP/4.3.0\r\n +Last-Modified: Fri, 01 Aug 2003 18:08:10 GMT\r\n +ETag: "295682-9-3f2aac8a"\r\n +Accept-Ranges: bytes\r\n +Content-Length: 9\r\n +Content-Type: text/plain\r\n\r\n +596254978 # *********************************************************************************************** # Alerts