From: Russ Combs Date: Tue, 19 May 2015 15:10:48 +0000 (-0400) Subject: tom: new_http_inspect start line parsing updates X-Git-Tag: 3.0.0-233~972 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=594a2db4c472a00902a54e697c649ed8d2a44f4e;p=thirdparty%2Fsnort3.git tom: new_http_inspect start line parsing updates --- diff --git a/ChangeLog b/ChangeLog index 994927664..738de2e3c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,7 +1,8 @@ Pending - build 153 +-- new_http_inspect parsing updates -- use buckets for user seglist --- add -L u2 +-- add -A u2 -- fix u2 to output data only packets -- added DAQs for socket, user, and file in extras -- changed -K to -L (log type) diff --git a/src/service_inspectors/nhttp_inspect/nhttp_enum.h b/src/service_inspectors/nhttp_inspect/nhttp_enum.h index 743ecb8ef..cc4cdf80f 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_enum.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_enum.h @@ -125,6 +125,12 @@ enum Infraction INF_ENDLESS_HEADER, INF_LF_WITHOUT_CR, INF_NOT_HTTP, + INF_NO_URI, + INF_REQUEST_WS, + INF_REQUEST_TAB, + INF_STATUS_WS, + INF_STATUS_TAB, + INF_URI_SPACE, }; // Formats for output from a header normalization function @@ -193,11 +199,17 @@ enum EventSid EVENT_LOSS_OF_SYNC, EVENT_NOT_HTTP, EVENT_WS_BETWEEN_MSGS, + EVENT_URI_MISSING, + EVENT_CTRL_IN_REASON, + EVENT_IMPROPER_WS, + EVENT_BAD_VERS, + EVENT_UNKNOWN_VERS, EVENT_MAXVALUE }; extern const int8_t as_hex[256]; extern const bool token_char[256]; +extern const bool is_sp_tab[256]; } // end namespace NHttpEnums #endif diff --git a/src/service_inspectors/nhttp_inspect/nhttp_msg_request.cc b/src/service_inspectors/nhttp_inspect/nhttp_msg_request.cc index c2948c266..63990ea89 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_msg_request.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_msg_request.cc @@ -40,51 +40,48 @@ NHttpMsgRequest::NHttpMsgRequest(const uint8_t* buffer, const uint16_t buf_size, void NHttpMsgRequest::parse_start_line() { - // FIXIT-M this needs to be redesigned to parse a truncated request line and extract the method - // and URI. The current implementation just gives up if the " HTTP/X.Y" isn't in its proper - // place at the end of the line. - - // There should be exactly two spaces. One following the method and one before "HTTP/". - // Additional spaces located within the URI are not allowed by RFC but we will tolerate it - // HTTP/X.Y - if (start_line.start[start_line.length-9] != ' ') + // Check the version field + if ((start_line.length < 10) || !is_sp_tab[start_line.start[start_line.length-9]] || + memcmp(start_line.start + start_line.length - 8, "HTTP/", 5)) { - // space before "HTTP" missing or in wrong place infractions += INF_BAD_REQ_LINE; + events.create_event(EVENT_NOT_HTTP); return; } - int32_t space; - for (space = 0; space < start_line.length-9; space++) - { - if (start_line.start[space] == ' ') - break; - } - if (space >= start_line.length-9) - { - // leading space or no space - infractions += INF_BAD_REQ_LINE; - return; - } + // The splitter guarantees there will be a non-whitespace at octet 1 and a whitespace within + // octets 2-81. The following algorithm uses those assumptions. + + int32_t first_space; // first whitespace in request line + for (first_space = 1; !is_sp_tab[start_line.start[first_space]]; first_space++); + + int32_t first_end; // last whitespace in first clump of whitespace + for (first_end = first_space+1; is_sp_tab[start_line.start[first_end]]; first_end++); + first_end--; + + int32_t last_begin; // first whitespace in clump of whitespace before version + for (last_begin = start_line.length - 10; is_sp_tab[start_line.start[last_begin]]; + last_begin--); + last_begin++; method.start = start_line.start; - method.length = space; - derive_method_id(); - uri = new NHttpUri(start_line.start + method.length + 1, - start_line.length - method.length - 10, method_id); + method.length = first_space; + method_id = (MethodId)str_to_code(method.start, method.length, method_list); + version.start = start_line.start + (start_line.length - 8); version.length = 8; - assert (start_line.length == method.length + uri->get_uri().length + version.length + 2); -} + derive_version_id(); -void NHttpMsgRequest::derive_method_id() -{ - if (method.length <= 0) + if (first_end < last_begin) { - method_id = METH__NOSOURCE; - return; + uri = new NHttpUri(start_line.start + first_end + 1, last_begin - first_end - 1, + method_id); + } + else + { + infractions += INF_NO_URI; + events.create_event(EVENT_URI_MISSING); } - method_id = (MethodId)str_to_code(method.start, method.length, method_list); } const Field& NHttpMsgRequest::get_uri() @@ -107,6 +104,43 @@ const Field& NHttpMsgRequest::get_uri_norm_legacy() void NHttpMsgRequest::gen_events() { + if (infractions && INF_BAD_REQ_LINE) + return; + + if ((start_line.start[method.length] == '\t') || + (start_line.start[start_line.length - 9] == '\t')) + { + infractions += INF_REQUEST_TAB; + events.create_event(EVENT_APACHE_WS); + } + + for (int k = method.length + 1; k < start_line.length - 9; k++) + { + if (is_sp_tab[start_line.start[k]]) + { + if (uri && (uri->get_uri().start <= start_line.start + k) && + (start_line.start + k < uri->get_uri().start + uri->get_uri().length)) + { + // inside the URI + if (start_line.start[k] == ' ') + { + infractions += INF_URI_SPACE; + events.create_event(EVENT_UNESCAPED_SPACE_URI); + } + } + else + { + infractions += INF_REQUEST_WS; + events.create_event(EVENT_IMPROPER_WS); + if (start_line.start[k] == '\t') + { + infractions += INF_REQUEST_TAB; + events.create_event(EVENT_APACHE_WS); + } + } + } + } + if (method_id == METH__OTHER) events.create_event(EVENT_UNKNOWN_METHOD); diff --git a/src/service_inspectors/nhttp_inspect/nhttp_msg_request.h b/src/service_inspectors/nhttp_inspect/nhttp_msg_request.h index 16e199dc6..1c7c7c945 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_msg_request.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_msg_request.h @@ -47,7 +47,6 @@ private: static const StrCode method_list[]; void parse_start_line() override; - void derive_method_id(); Field method; NHttpUri* uri = nullptr; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc b/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc index a728e3842..7ec126618 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_msg_start.cc @@ -31,27 +31,15 @@ void NHttpMsgStart::analyze() start_line.start = msg_text.start; start_line.length = msg_text.length; parse_start_line(); - derive_version_id(); } void NHttpMsgStart::derive_version_id() { - if (version.length <= 0) - { - version_id = VERS__NOSOURCE; - return; - } - if (version.length != 8) - { - version_id = VERS__PROBLEMATIC; - infractions += INF_BAD_VERSION; - return; - } - - if (memcmp(version.start, "HTTP/", 5) || (version.start[6] != '.')) + if (version.start[6] != '.') { version_id = VERS__PROBLEMATIC; infractions += INF_BAD_VERSION; + events.create_event(EVENT_BAD_VERS); } else if ((version.start[5] == '1') && (version.start[7] == '1')) { @@ -70,11 +58,13 @@ void NHttpMsgStart::derive_version_id() { version_id = VERS__OTHER; infractions += INF_UNKNOWN_VERSION; + events.create_event(EVENT_UNKNOWN_VERS); } else { version_id = VERS__PROBLEMATIC; infractions += INF_BAD_VERSION; + events.create_event(EVENT_BAD_VERS); } } diff --git a/src/service_inspectors/nhttp_inspect/nhttp_msg_status.cc b/src/service_inspectors/nhttp_inspect/nhttp_msg_status.cc index 5487ec963..ff4ad6425 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_msg_status.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_msg_status.cc @@ -38,61 +38,59 @@ NHttpMsgStatus::NHttpMsgStatus(const uint8_t* buffer, const uint16_t buf_size, transaction->set_status(this); } -// All the header processing that is done for every message (i.e. not just-in-time) is done here. -void NHttpMsgStatus::analyze() -{ - NHttpMsgStart::analyze(); - derive_status_code_num(); -} - void NHttpMsgStatus::parse_start_line() { - // FIXIT-M need to be able to parse a truncated status line and extract version and status - // code. + // Splitter guarantees line begins with "HTTP/" - // Eventually we may need to cater to certain format errors, but for now exact match or treat - // as error. HTTP/X.Y### - if ((start_line.length < 13) || (start_line.start[8] != ' ') || (start_line.start[12] != ' ')) + if ((start_line.length < 12) || !is_sp_tab[start_line.start[8]]) { infractions += INF_BAD_STAT_LINE; + events.create_event(EVENT_NOT_HTTP); return; } - version.start = start_line.start; - version.length = 8; - status_code.start = start_line.start + 9; - status_code.length = 3; - reason_phrase.start = start_line.start + 13; - reason_phrase.length = start_line.length - 13; - for (int32_t k = 0; k < reason_phrase.length; k++) + + int32_t first_end; // last whitespace in first clump of whitespace + for (first_end = 9; is_sp_tab[start_line.start[first_end]] && (first_end < start_line.length); + first_end++); + first_end--; + + if (start_line.length < first_end + 4) { - if ((reason_phrase.start[k] <= 31) || (reason_phrase.start[k] >= 127)) - { - // Illegal character in reason phrase - infractions += INF_BAD_PHRASE; - break; - } + infractions += INF_BAD_STAT_LINE; + events.create_event(EVENT_NOT_HTTP); + return; } - assert (start_line.length == version.length + status_code.length + reason_phrase.length + 2); -} -void NHttpMsgStatus::derive_status_code_num() -{ - if (status_code.length <= 0) + if ((start_line.length > first_end + 4) && !is_sp_tab[start_line.start[first_end + 4]]) { - status_code_num = STAT_NOSOURCE; + infractions += INF_BAD_STAT_LINE; + events.create_event(EVENT_NOT_HTTP); return; } - if (status_code.length != 3) + + version.start = start_line.start; + version.length = 8; + derive_version_id(); + + status_code.start = start_line.start + first_end + 1; + status_code.length = 3; + derive_status_code_num(); + + if (start_line.length > first_end + 5) { - status_code_num = STAT_PROBLEMATIC; - return; + reason_phrase.start = start_line.start + first_end + 5; + reason_phrase.length = start_line.length - first_end - 5; } +} +void NHttpMsgStatus::derive_status_code_num() +{ if ((status_code.start[0] < '0') || (status_code.start[0] > '9') || (status_code.start[1] < '0') || (status_code.start[1] > '9') || (status_code.start[2] < '0') || (status_code.start[2] > '9')) { infractions += INF_BAD_STAT_CODE; + events.create_event(EVENT_INVALID_STATCODE); status_code_num = STAT_PROBLEMATIC; return; } @@ -101,10 +99,50 @@ void NHttpMsgStatus::derive_status_code_num() if ((status_code_num < 100) || (status_code_num > 599)) { infractions += INF_BAD_STAT_CODE; + events.create_event(EVENT_INVALID_STATCODE); } } -void NHttpMsgStatus::gen_events() { } +void NHttpMsgStatus::gen_events() +{ + if (infractions && INF_BAD_STAT_LINE) + return; + + if (status_code.start > start_line.start + 9) + { + infractions += INF_STATUS_WS; + events.create_event(EVENT_IMPROPER_WS); + } + + for (int k = 8; k < status_code.start - start_line.start; k++) + { + if (start_line.start[k] == '\t') + { + infractions += INF_STATUS_TAB; + events.create_event(EVENT_APACHE_WS); + } + } + + if (status_code.start - start_line.start + 3 < start_line.length) + { + if (status_code.start[3] == '\t') + { + infractions += INF_STATUS_TAB; + events.create_event(EVENT_APACHE_WS); + } + } + + for (int k=0; k < reason_phrase.length; k++) + { + if ((reason_phrase.start[k] <= 31) || (reason_phrase.start[k] >= 127)) + { + // Illegal character in reason phrase + infractions += INF_BAD_PHRASE; + events.create_event(EVENT_CTRL_IN_REASON); + break; + } + } +} void NHttpMsgStatus::print_section(FILE* output) { diff --git a/src/service_inspectors/nhttp_inspect/nhttp_msg_status.h b/src/service_inspectors/nhttp_inspect/nhttp_msg_status.h index b8f836e5b..70dc1d252 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_msg_status.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_msg_status.h @@ -32,7 +32,6 @@ class NHttpMsgStatus : public NHttpMsgStart public: NHttpMsgStatus(const uint8_t* buffer, const uint16_t buf_size, NHttpFlowData* session_data_, NHttpEnums::SourceId source_id_, bool buf_owner); - void analyze() override; void print_section(FILE* output) override; void gen_events() override; void update_flow() override; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_splitter.cc b/src/service_inspectors/nhttp_inspect/nhttp_splitter.cc index f4a3c3a3a..8c4defda8 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_splitter.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_splitter.cc @@ -31,9 +31,9 @@ ScanResult NHttpStartSplitter::split(const uint8_t* buffer, uint32_t length, // If we have seen nothing but white space so far ... if (num_crlf == octets_seen + k) { - if ((buffer[k] == 32) || ((buffer[k] >= 9) && (buffer[k] <= 13))) + if ((buffer[k] == ' ') || ((buffer[k] >= '\t') && (buffer[k] <= '\r'))) { - if ((buffer[k] != 10) && (buffer[k] != 13)) + if ((buffer[k] != '\n') && (buffer[k] != '\r')) { // tab, VT, FF, or space between messages infractions += INF_WS_BETWEEN_MSGS; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_splitter.h b/src/service_inspectors/nhttp_inspect/nhttp_splitter.h index 13ce2846d..51875776b 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_splitter.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_splitter.h @@ -40,7 +40,6 @@ public: uint32_t get_octets_seen() const { return octets_seen; } virtual uint32_t get_num_excess() const { return 0; } virtual uint32_t get_num_head_lines() const { return 0; } - virtual bool valid() const { return true; } protected: // number of octets processed by previous split() calls that returned NOTFOUND @@ -56,7 +55,6 @@ public: NHttpEnums::ScanResult split(const uint8_t* buffer, uint32_t length, NHttpInfractions& infractions, NHttpEventGen& events) override; uint32_t get_num_excess() const override { return (num_flush > 0) ? num_crlf : 0; } - bool valid() const override { return validated; } protected: enum ValidationResult { V_GOOD, V_BAD, V_TBD }; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc b/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc index 5025aedb7..717dcdb71 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_stream_splitter.cc @@ -164,7 +164,11 @@ StreamSplitter::Status NHttpStreamSplitter::scan(Flow* flow, const uint8_t* data return StreamSplitter::FLUSH; } data = test_data; - assert(session_data->type_expected[source_id] != SEC_ABORT); + if (session_data->type_expected[source_id] == SEC_ABORT) + { + session_data = new NHttpFlowData; + flow->set_application_data(session_data); + } } else if (NHttpTestManager::use_test_output()) { @@ -405,13 +409,15 @@ bool NHttpStreamSplitter::finish(Flow* flow) assert(session_data != nullptr); session_data->tcp_close[source_id] = true; // If there is leftover data for which we returned PAF_SEARCH and never flushed, we need to set - // up to process because it is about to go to reassemble(). + // up to process because it is about to go to reassemble(). But we don't support partial start + // lines. if ((session_data->section_type[source_id] == SEC__NOTCOMPUTE) && (session_data->splitter[source_id] != nullptr) && (session_data->splitter[source_id]->get_octets_seen() > 0) && (session_data->type_expected[source_id] != SEC_ABORT)) { - if (!session_data->splitter[source_id]->valid()) + if ((session_data->type_expected[source_id] == SEC_REQUEST) || + (session_data->type_expected[source_id] == SEC_STATUS)) { return false; } diff --git a/src/service_inspectors/nhttp_inspect/nhttp_tables.cc b/src/service_inspectors/nhttp_inspect/nhttp_tables.cc index a308b8d73..e29d5121f 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_tables.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_tables.cc @@ -303,7 +303,11 @@ const RuleMap NHttpModule::nhttp_events[] = { EVENT_LOSS_OF_SYNC, "HTTP misformatted or not really HTTP" }, { EVENT_NOT_HTTP, "Input apparently not HTTP" }, { EVENT_WS_BETWEEN_MSGS, "White space before or between messages" }, - + { EVENT_URI_MISSING, "Request message without URI" }, + { EVENT_CTRL_IN_REASON, "Control character in reason phrase" }, + { EVENT_IMPROPER_WS, "Illegal extra whitespace in start line" }, + { EVENT_BAD_VERS, "Corrupted HTTP version" }, + { EVENT_UNKNOWN_VERS, "Unknown HTTP version" }, { 0, nullptr } }; @@ -334,7 +338,6 @@ const int8_t NHttpEnums::as_hex[256] = -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1 }; - const bool NHttpEnums::token_char[256] = { false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, @@ -362,3 +365,30 @@ const bool NHttpEnums::token_char[256] = false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false }; +const bool NHttpEnums::is_sp_tab[256] = +{ + false, false, false, false, false, false, false, false, false, true, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + true, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, + false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false +}; + diff --git a/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt b/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt index 823ff0a63..e318d4fa2 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt +++ b/src/service_inspectors/nhttp_inspect/nhttp_test_msgs.txt @@ -280,6 +280,41 @@ HTTP/1.0 401 illegal nontext character in reason\xFFphrase delete\r\n\r\n @response HTTP/1.0 401 \x08illegal nontext character in reason phrase backspace\r\n\r\n +@2029 +@break +@response +HTTP/1.0 310 Excessive white space\r\n\r\n + +@2030 +@break +@response +HTTP/1.0 310 Excessive white space\r\n\r\n + +@2031 +@break +@response +HTTP/1.1\t310 Tab instead of space\r\n\r\n + +@2032 +@break +@response +HTTP/1.0 310\tTab instead of space\r\n\r\n + +@2033 +@break +@response +HTTP/1.0 \t310 Excessive white space and tab\r\n\r\n + +@2034 +@break +@response +HTTP/1.0 700 \tTab is in reason phrase\r\n\r\n + +@2035 +@break +@response +HTTP/1.0\t\t\t310 Excessive white space consisting of tabs\r\n\r\n + # *********************************************************************************************** # Valid request start lines @3001 @@ -392,6 +427,11 @@ GET HtTpS://1.2.3.4.5.a:6/abcdef/ghijklmnop/qrstuvwxyz/?thequery?fieldcontinues? @request \t\t\t\tBIND /1234567890?abcdef HTTP/1.1\r\n\r\n +@3023 +@break +@request +GET12345678901234567890123456789012345678901234567890123456789012345678901234567 http://hostname.com/?# HTTP/1.1\r\n\r\n + # *********************************************************************************************** # Invalid request start lines @4001 @@ -474,6 +514,36 @@ GE;T http://hostname.com/?# HTTP/1.1\r\n\r\n @request GET\xAA http://hostname.com/?# HTTP/1.1\r\n\r\n +@4016 +@break +@request +GET /white/space/abuse HTTP/1.1\r\n\r\n + +@4017 +@break +@request +GET /white/space/abuse HTTP/1.1\r\n\r\n + +@4018 +@break +@request +GET\t/white/space/abuse HTTP/1.1\r\n\r\n + +@4019 +@break +@request +GET /white/space/abuse\tHTTP/1.1\r\n\r\n + +@4020 +@break +@request +GET \t /white/space/abuse HTTP/1.1\r\n\r\n + +@4021 +@break +@request +GET /white/space/abuse \tHTTP/1.1\r\n\r\n + # *********************************************************************************************** # Valid headers without body @5001