From: Russ Combs (rucombs) Date: Thu, 14 Jan 2016 23:20:27 +0000 (-0500) Subject: Merge pull request #207 in SNORT/snort3 from nhttp33 to master X-Git-Tag: 3.0.0-233~662 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=580d788da2f8c8ead4b67e3470cd38dc986dfe12;p=thirdparty%2Fsnort3.git Merge pull request #207 in SNORT/snort3 from nhttp33 to master Squashed commit of the following: commit 71155a9f008c0c58e2594bac2fd30a84ab17b754 Author: Tom Peters Date: Fri Jan 8 15:12:09 2016 -0500 URI normalization memory reduction and performance enhancements --- diff --git a/src/service_inspectors/nhttp_inspect/nhttp_enum.h b/src/service_inspectors/nhttp_inspect/nhttp_enum.h index 0549544ce..681cdc6bb 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_enum.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_enum.h @@ -132,7 +132,7 @@ enum Infraction INF_BAD_PHRASE, INF_BAD_URI, INF_UNUSED, - INF_URI_NEED_NORM, + INF_UNUSED2, INF_URI_PERCENT_NORMAL, INF_URI_PERCENT_ASCII, INF_URI_PERCENT_UTF8, @@ -172,6 +172,10 @@ enum Infraction INF_GZIP_OVERRUN, INF_GZIP_FAILURE, INF_GZIP_EARLY_END, + INF_URI_NEED_NORM_PATH, + INF_URI_NEED_NORM_HOST, + INF_URI_NEED_NORM_QUERY, + INF_URI_NEED_NORM_FRAGMENT, INF__MAX_VALUE }; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_field.cc b/src/service_inspectors/nhttp_inspect/nhttp_field.cc index 420eba626..d9aee0c51 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_field.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_field.cc @@ -29,6 +29,15 @@ using namespace NHttpEnums; const Field Field::FIELD_NULL { STAT_NO_SOURCE }; +void Field::set(int32_t length_, const uint8_t* start_) +{ + assert(length == STAT_NOT_COMPUTE); + assert(start == nullptr); + assert(start_ != nullptr); + assert(length_ >= 0); + start = start_; + length = length_; +} #ifdef REG_TEST void Field::print(FILE* output, const char* name) const diff --git a/src/service_inspectors/nhttp_inspect/nhttp_field.h b/src/service_inspectors/nhttp_inspect/nhttp_field.h index b50c75cbf..0ec96c7ff 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_field.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_field.h @@ -40,6 +40,7 @@ public: Field(int32_t length_, const uint8_t* start_) : length(length_), start(start_) { } explicit Field(int32_t length_) : length(length_) { assert(length<=0); } Field() = default; + void set(int32_t length_, const uint8_t* start_); #ifdef REG_TEST void print(FILE* output, const char* name) const; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_uri.cc b/src/service_inspectors/nhttp_inspect/nhttp_uri.cc index 927b39af0..671593dd6 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_uri.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_uri.cc @@ -28,6 +28,12 @@ using namespace NHttpEnums; +NHttpUri::~NHttpUri() +{ + if (classic_norm_allocated) + delete[] classic_norm.start; +} + void NHttpUri::parse_uri() { // Four basic types of HTTP URI @@ -132,6 +138,11 @@ void NHttpUri::parse_abs_path() query.start = abs_path.start + path.length + 1; for (query.length = 0; (query.start[query.length] != '#') && (query.length < abs_path.length - path.length - 1); query.length++); + if (abs_path.length - path.length - 1 - query.length == 0) + { + fragment.length = STAT_NOT_PRESENT; + return; + } fragment.start = query.start + query.length + 1; fragment.length = abs_path.length - path.length - 1 - query.length - 1; } @@ -153,88 +164,101 @@ void NHttpUri::normalize() parse_authority(); parse_abs_path(); - // Normalize the individual components. We don't do anything with scheme or port. - if (path.length >= 0) + // Almost all HTTP requests are honest and rarely need expensive normalization processing. We + // do a quick scan for red flags and only perform normalization if something comes up. + // Otherwise we set the normalized fields to point at the raw values. + if ((host.length > 0) && UriNormalizer::need_norm_no_path(host)) + infractions += INF_URI_NEED_NORM_HOST; + if ((path.length > 0) && UriNormalizer::need_norm_path(path)) + infractions += INF_URI_NEED_NORM_PATH; + if ((query.length > 0) && UriNormalizer::need_norm_no_path(query)) + infractions += INF_URI_NEED_NORM_QUERY; + if ((fragment.length > 0) && UriNormalizer::need_norm_no_path(fragment)) + infractions += INF_URI_NEED_NORM_FRAGMENT; + + if (!((infractions & INF_URI_NEED_NORM_PATH) || (infractions & INF_URI_NEED_NORM_HOST) || + (infractions & INF_URI_NEED_NORM_QUERY) || (infractions & INF_URI_NEED_NORM_FRAGMENT))) { - UriNormalizer::normalize(path, path_norm, true, scratch_pad, infractions, events); + host_norm = host; + path_norm = path; + query_norm = query; + fragment_norm = fragment; + classic_norm = uri; + return; } - if (host.length >= 0) + + // Create a new buffer containing the normalized URI by normalizing each individual piece. + const uint32_t total_length = uri.length + UriNormalizer::URI_NORM_EXPANSION; + uint8_t* const new_buf = new uint8_t[total_length]; + uint8_t* current = new_buf; + if (scheme.length >= 0) { - UriNormalizer::normalize(host, host_norm, false, scratch_pad, infractions, events); + memcpy(current, scheme.start, scheme.length); + current += scheme.length; + memcpy(current, "://", 3); + current += 3; } - if (query.length >= 0) + if (host.length > 0) { - UriNormalizer::normalize(query, query_norm, false, scratch_pad, infractions, events); + if (infractions & INF_URI_NEED_NORM_HOST) + UriNormalizer::normalize(host, host_norm, false, current, infractions, events); + else + { + // The host component is not changing but other parts of the URI are being normalized. + // We need a copy of the raw host to provide that part of the normalized URI buffer we + // are assembling. But the normalized component will refer to the original raw buffer + // on the chance that the data retention policy in use might keep it longer. + memcpy(current, host.start, host.length); + host_norm = host; + } + current += host_norm.length; } - if (fragment.length >= 0) + if (port.length >= 0) { - UriNormalizer::normalize(fragment, fragment_norm, false, scratch_pad, - infractions, events); + memcpy(current, ":", 1); + current += 1; + memcpy(current, port.start, port.length); + current += port.length; } - - // We can reuse the raw URI for the normalized URI if no normalization is required - if (!(infractions & INF_URI_NEED_NORM)) + if (path.length > 0) { - classic_norm.start = uri.start; - classic_norm.length = uri.length; - return; - } - - // Glue normalized URI pieces back together - const uint32_t total_length = ((scheme.length >= 0) ? scheme.length + 3 : 0) + - ((host_norm.length >= 0) ? host_norm.length : 0) + - ((port.length >= 0) ? port.length + 1 : 0) + - ((path_norm.length >= 0) ? path_norm.length : 0) + - ((query_norm.length >= 0) ? query_norm.length + 1 : 0) + - ((fragment_norm.length >= 0) ? fragment_norm.length + 1 : 0); - uint8_t* const scratch = scratch_pad.request(total_length); - if (scratch != nullptr) - { - uint8_t* current = scratch; - if (scheme.length >= 0) - { - memcpy(current, scheme.start, scheme.length); - current += scheme.length; - memcpy(current, "://", 3); - current += 3; - } - if (host_norm.length >= 0) - { - memcpy(current, host_norm.start, host_norm.length); - current += host_norm.length; - } - if (port.length >= 0) - { - memcpy(current, ":", 1); - current += 1; - memcpy(current, port.start, port.length); - current += port.length; - } - if (path_norm.length >= 0) + if (infractions & INF_URI_NEED_NORM_PATH) + UriNormalizer::normalize(path, path_norm, true, current, infractions, events); + else { - memcpy(current, path_norm.start, path_norm.length); - current += path_norm.length; + memcpy(current, path.start, path.length); + path_norm = path; } - if (query_norm.length >= 0) + current += path_norm.length; + } + if (query.length >= 0) + { + memcpy(current, "?", 1); + current += 1; + if (infractions & INF_URI_NEED_NORM_QUERY) + UriNormalizer::normalize(query, query_norm, false, current, infractions, events); + else { - memcpy(current, "?", 1); - current += 1; - memcpy(current, query_norm.start, query_norm.length); - current += query_norm.length; + memcpy(current, query.start, query.length); + query_norm = query; } - if (fragment_norm.length >= 0) + current += query_norm.length; + } + if (fragment.length >= 0) + { + memcpy(current, "#", 1); + current += 1; + if (infractions & INF_URI_NEED_NORM_FRAGMENT) + UriNormalizer::normalize(fragment, fragment_norm, false, current, infractions, events); + else { - memcpy(current, "#", 1); - current += 1; - memcpy(current, fragment_norm.start, fragment_norm.length); - current += fragment_norm.length; + memcpy(current, fragment.start, fragment.length); + fragment_norm = fragment; } - assert(total_length == current - scratch); - scratch_pad.commit(current - scratch); - classic_norm.start = scratch; - classic_norm.length = current - scratch; + current += fragment_norm.length; } - else - classic_norm.length = STAT_INSUF_MEMORY; + assert(current - new_buf <= total_length); + classic_norm.set(current - new_buf, new_buf); + classic_norm_allocated = true; } diff --git a/src/service_inspectors/nhttp_inspect/nhttp_uri.h b/src/service_inspectors/nhttp_inspect/nhttp_uri.h index 4333f3c58..9c30debb2 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_uri.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_uri.h @@ -20,7 +20,6 @@ #ifndef NHTTP_URI_H #define NHTTP_URI_H -#include "nhttp_scratch_pad.h" #include "nhttp_str_to_code.h" #include "nhttp_uri_norm.h" #include "nhttp_field.h" @@ -36,8 +35,9 @@ class NHttpUri public: NHttpUri(const uint8_t* start, int32_t length, NHttpEnums::MethodId method, NHttpInfractions& infractions_, NHttpEventGen& events_) : - uri(length, start), method_id(method), infractions(infractions_), events(events_), - scratch_pad(2*length+200) { normalize(); } + uri(length, start), method_id(method), infractions(infractions_), events(events_) + { normalize(); } + ~NHttpUri(); const Field& get_uri() const { return uri; } NHttpEnums::UriType get_uri_type() { return uri_type; } const Field& get_scheme() { return scheme; } @@ -76,15 +76,12 @@ private: Field query_norm; Field fragment_norm; Field classic_norm; + bool classic_norm_allocated = false; void normalize(); void parse_uri(); void parse_authority(); void parse_abs_path(); - - // FIXIT-P there is an enormous memory waste that this is always allocated. It is only needed - // when the URI requires normalization. Most of the time the raw URI is already in normal form. - ScratchPad scratch_pad; }; #endif diff --git a/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.cc b/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.cc index 6d7514b92..da3b13ba7 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.cc +++ b/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.cc @@ -25,76 +25,52 @@ using namespace NHttpEnums; -void UriNormalizer::normalize(const Field& input, Field& result, bool do_path, - ScratchPad& scratch_pad, NHttpInfractions& infractions, NHttpEventGen& events) +void UriNormalizer::normalize(const Field& input, Field& result, bool do_path, uint8_t* buffer, + NHttpInfractions& infractions, NHttpEventGen& events) { - // Almost all HTTP requests are honest and rarely need expensive normalization processing. We - // do a quick scan for red flags and only perform normalization if something comes up. - // Otherwise we set the normalized field to point at the raw value. - if ( ( do_path && path_check(input.start, input.length, infractions, events)) || - (!do_path && no_path_check(input.start, input.length, infractions, events))) - { - result.start = input.start; - result.length = input.length; - return; - } - - // Add an extra byte because normalization on rare occasions adds an extra character - // We need working space for two copies to do multiple passes. - // Round up to multiple of eight so that both copies are 64-bit aligned. - const int32_t buffer_length = input.length + 1 + (8-(input.length+1)%8)%8; - uint8_t* const scratch = scratch_pad.request(2 * buffer_length); - if (scratch == nullptr) - { - result.length = STAT_INSUF_MEMORY; - return; - } - uint8_t* const front_half = scratch; - uint8_t* const back_half = scratch + buffer_length; + // Normalize character escape sequences + int32_t data_length = norm_char_clean(input.start, input.length, buffer, infractions, events); - int32_t data_length; - data_length = norm_char_clean(input.start, input.length, front_half, infractions, events); + // Normalize path directory traversals if (do_path) { - data_length = norm_backslash(front_half, data_length, back_half, infractions, events); - data_length = norm_path_clean(back_half, data_length, front_half, infractions, events); + norm_backslash(buffer, data_length, infractions, events); + data_length = norm_path_clean(buffer, data_length, infractions, events); } - scratch_pad.commit(data_length); - result.start = front_half; - result.length = data_length; + result.set(data_length, buffer); } -bool UriNormalizer::no_path_check(const uint8_t* in_buf, int32_t in_length, - NHttpInfractions& infractions, NHttpEventGen&) +bool UriNormalizer::need_norm_no_path(const Field& uri_component) { - for (int32_t k = 0; k < in_length; k++) + const int32_t& length = uri_component.length; + const uint8_t* const & buf = uri_component.start; + for (int32_t k = 0; k < length; k++) { - if ((uri_char[in_buf[k]] == CHAR_NORMAL) || (uri_char[in_buf[k]] == CHAR_PATH)) + if ((uri_char[buf[k]] == CHAR_NORMAL) || (uri_char[buf[k]] == CHAR_PATH)) continue; - infractions += INF_URI_NEED_NORM; - return false; + return true; } - return true; + return false; } -bool UriNormalizer::path_check(const uint8_t* in_buf, int32_t in_length, - NHttpInfractions& infractions, NHttpEventGen&) +bool UriNormalizer::need_norm_path(const Field& uri_component) { - for (int32_t k = 0; k < in_length; k++) + const int32_t& length = uri_component.length; + const uint8_t* const & buf = uri_component.start; + for (int32_t k = 0; k < length; k++) { - if (uri_char[in_buf[k]] == CHAR_NORMAL) + if (uri_char[buf[k]] == CHAR_NORMAL) continue; - if ((in_buf[k] == '/') && ((k == 0) || (in_buf[k-1] != '/'))) + if ((buf[k] == '/') && ((k == 0) || (buf[k-1] != '/'))) continue; - if ( (in_buf[k] == '.') && - ((k == 0) || (uri_char[in_buf[k-1]] == CHAR_NORMAL)) && - ((k == in_length-1) || (uri_char[in_buf[k+1]] == CHAR_NORMAL))) + if ( (buf[k] == '.') && + ((k == 0) || (uri_char[buf[k-1]] == CHAR_NORMAL)) && + ((k == length-1) || (uri_char[buf[k+1]] == CHAR_NORMAL))) continue; - infractions += INF_URI_NEED_NORM; - return false; + return true; } - return true; + return false; } int32_t UriNormalizer::norm_char_clean(const uint8_t* in_buf, int32_t in_length, uint8_t* out_buf, @@ -177,25 +153,22 @@ int32_t UriNormalizer::norm_char_clean(const uint8_t* in_buf, int32_t in_length, } // Convert URI backslashes to slashes -int32_t UriNormalizer::norm_backslash(const uint8_t* in_buf, int32_t in_length, uint8_t* out_buf, - NHttpInfractions& infractions, NHttpEventGen& events) +void UriNormalizer::norm_backslash(uint8_t* buf, int32_t length, NHttpInfractions& infractions, + NHttpEventGen& events) { - for (int32_t k = 0; k < in_length; k++) + for (int32_t k = 0; k < length; k++) { - if (in_buf[k] != '\\') - out_buf[k] = in_buf[k]; - else + if (buf[k] == '\\') { - out_buf[k] = '/'; + buf[k] = '/'; infractions += INF_URI_BACKSLASH; events.create_event(EVENT_IIS_BACKSLASH); } } - return in_length; } // Caution: worst case output length is one greater than input length -int32_t UriNormalizer::norm_path_clean(const uint8_t* in_buf, int32_t in_length, uint8_t* out_buf, +int32_t UriNormalizer::norm_path_clean(uint8_t* buf, const int32_t in_length, NHttpInfractions& infractions, NHttpEventGen& events) { int32_t length = 0; @@ -205,19 +178,19 @@ int32_t UriNormalizer::norm_path_clean(const uint8_t* in_buf, int32_t in_length, for (int32_t k = 0; k <= in_length; k++) { // Pass through all non-slash characters and also the leading slash - if (((k < in_length) && (in_buf[k] != '/')) || (k == 0)) + if (((k < in_length) && (buf[k] != '/')) || (k == 0)) { - out_buf[length++] = in_buf[k]; + buf[length++] = buf[k]; } // Ignore this slash if it directly follows another slash - else if ((k < in_length) && (length >= 1) && (out_buf[length-1] == '/')) + else if ((k < in_length) && (length >= 1) && (buf[length-1] == '/')) { infractions += INF_URI_MULTISLASH; events.create_event(EVENT_MULTI_SLASH); } // This slash is the end of a /./ pattern, ignore this slash and remove the period from the // output - else if ((length >= 2) && (out_buf[length-1] == '.') && (out_buf[length-2] == '/')) + else if ((length >= 2) && (buf[length-1] == '.') && (buf[length-2] == '/')) { infractions += INF_URI_SLASH_DOT; events.create_event(EVENT_SELF_DIR_TRAV); @@ -225,8 +198,8 @@ int32_t UriNormalizer::norm_path_clean(const uint8_t* in_buf, int32_t in_length, } // This slash is the end of a /../ pattern, normalization depends on whether there is a // previous directory that we can remove - else if ((length >= 3) && (out_buf[length-1] == '.') && (out_buf[length-2] == '.') && - (out_buf[length-3] == '/')) + else if ((length >= 3) && (buf[length-1] == '.') && (buf[length-2] == '.') && + (buf[length-3] == '/')) { infractions += INF_URI_SLASH_DOT_DOT; events.create_event(EVENT_DIR_TRAV); @@ -236,24 +209,23 @@ int32_t UriNormalizer::norm_path_clean(const uint8_t* in_buf, int32_t in_length, // pretend slash after the end of the buffer. That is intentional so that the normal // form of "/../../../.." is "/../../../../" if ( (length == 3) || - ((length >= 6) && (out_buf[length-4] == '.') && (out_buf[length-5] == '.') && - (out_buf[length-6] == '/'))) + ((length >= 6) && (buf[length-4] == '.') && (buf[length-5] == '.') && + (buf[length-6] == '/'))) { infractions += INF_URI_ROOT_TRAV; events.create_event(EVENT_WEBROOT_DIR); - out_buf[length++] = '/'; + buf[length++] = '/'; } // Remove the previous directory from the output. "/foo/bar/../" becomes "/foo/" else { - for (length -= 3; out_buf[length-1] != '/'; length--) - ; + for (length -= 3; buf[length-1] != '/'; length--); } } // Pass through an ordinary slash else if (k < in_length) { - out_buf[length++] = '/'; + buf[length++] = '/'; } } return length; diff --git a/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.h b/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.h index 81e85c17d..1426b054d 100644 --- a/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.h +++ b/src/service_inspectors/nhttp_inspect/nhttp_uri_norm.h @@ -20,7 +20,6 @@ #ifndef NHTTP_URI_NORM_H #define NHTTP_URI_NORM_H -#include "nhttp_scratch_pad.h" #include "nhttp_field.h" #include "nhttp_infractions.h" #include "nhttp_event_gen.h" @@ -29,21 +28,21 @@ class UriNormalizer { public: - static void normalize(const Field& input, Field& result, bool do_path, ScratchPad& scratch_pad, + static void normalize(const Field& input, Field& result, bool do_path, uint8_t* buffer, NHttpInfractions& infractions, NHttpEventGen& events); + static bool need_norm_path(const Field& uri_component); + static bool need_norm_no_path(const Field& uri_component); + static const unsigned URI_NORM_EXPANSION = 1; private: static const NHttpEnums::CharAction uri_char[256]; static const bool good_percent[256]; - static bool no_path_check(const uint8_t* in_buf, int32_t in_length, - NHttpInfractions& infractions, NHttpEventGen& events); - static bool path_check(const uint8_t* in_buf, int32_t in_length, - NHttpInfractions& infractions, NHttpEventGen& events); - static NormFunc norm_char_clean; - static NormFunc norm_backslash; - static NormFunc norm_path_clean; + static void norm_backslash(uint8_t* buf, int32_t length, NHttpInfractions& infractions, + NHttpEventGen& events); + static int32_t norm_path_clean(uint8_t* buf, const int32_t in_length, + NHttpInfractions& infractions, NHttpEventGen& events); }; #endif