From e8bd2151f274acd6a1241f61dc47f39381fe620c Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Mon, 18 Jan 2021 14:58:56 -0300 Subject: [PATCH] Having _NAMES, variables proxied Some variables share content with others; that is the case for ARGS and ARGS_NAMES. Those are different in value, as ARGS_NAMES holds the key name as value. Instead of duplicating the strings for the different collections, this patch unifies the collection in radix, avoiding memory fragmentation. It is currently doing some fragmentation while resolving the variable, but to be mitigated by shared_ptr is VariableValues, a different change. TODO: place others variables such as COOKIE*NAMES to use the same proxy. --- .../anchored_set_variable_translation_proxy.h | 126 ++++++++++++++++++ headers/modsecurity/transaction.h | 18 +-- src/Makefile.am | 1 + src/transaction.cc | 4 - .../regression/offset-variable.json | 66 ++++++++- .../regression/variable-ARGS_NAMES.json | 52 ++++++++ .../regression/variable-ARGS_POST_NAMES.json | 108 ++++++++++++++- 7 files changed, 359 insertions(+), 16 deletions(-) create mode 100644 headers/modsecurity/anchored_set_variable_translation_proxy.h diff --git a/headers/modsecurity/anchored_set_variable_translation_proxy.h b/headers/modsecurity/anchored_set_variable_translation_proxy.h new file mode 100644 index 00000000..9314c154 --- /dev/null +++ b/headers/modsecurity/anchored_set_variable_translation_proxy.h @@ -0,0 +1,126 @@ +/* + * ModSecurity, http://www.modsecurity.org/ + * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/) + * + * You may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * If any of the files related to licensing are missing or if you have any + * other questions related to licensing please contact Trustwave Holdings, Inc. + * directly using the email address security@modsecurity.org. + * + */ + + +#ifdef __cplusplus +#include +#include +#include +#include +#include +#endif + +#include "modsecurity/variable_value.h" +#include "modsecurity/anchored_set_variable.h" + + +#ifndef HEADERS_MODSECURITY_ANCHORED_SET_VARIABLE_TRANSLATION_PROXY_H_ +#define HEADERS_MODSECURITY_ANCHORED_SET_VARIABLE_TRANSLATION_PROXY_H_ + +#ifdef __cplusplus + +namespace modsecurity { + + +class AnchoredSetVariableTranslationProxy { + public: + AnchoredSetVariableTranslationProxy( + const std::string &name, + AnchoredSetVariable *fount) + : m_name(name), + m_fount(fount) + { + m_translate = [](std::string *name, std::vector *l) { + for (int i = 0; i < l->size(); ++i) { + VariableValue *newVariableValue = new VariableValue(name, &l->at(i)->getKey()); + const VariableValue *oldVariableValue = l->at(i); + l->at(i) = newVariableValue; + for (auto &oldOrigin : oldVariableValue->getOrigin()) { + std::unique_ptr newOrigin(new VariableOrigin); + newOrigin->m_length = oldVariableValue->getKey().size(); + newOrigin->m_offset = oldOrigin->m_offset - oldVariableValue->getKey().size() - 1; + newVariableValue->addOrigin(std::move(newOrigin)); + } + delete oldVariableValue; + } + }; + } + + virtual ~AnchoredSetVariableTranslationProxy() + { } + + void resolve(std::vector *l) { + m_fount->resolve(l); + m_translate(&m_name, l); + } + + void resolve(std::vector *l, + variables::KeyExclusions &ke) { + m_fount->resolve(l, ke); + m_translate(&m_name, l); + } + + void resolve(const std::string &key, + std::vector *l) { + m_fount->resolve(key, l); + m_translate(&m_name, l); + }; + + void resolveRegularExpression(Utils::Regex *r, + std::vector *l) { + m_fount->resolveRegularExpression(r, l); + m_translate(&m_name, l); + }; + + void resolveRegularExpression(Utils::Regex *r, + std::vector *l, + variables::KeyExclusions &ke) { + m_fount->resolveRegularExpression(r, l, ke); + m_translate(&m_name, l); + }; + + std::unique_ptr resolveFirst(const std::string &key) { + std::vector l; + resolve(&l); + + if (l.empty()) { + return nullptr; + } + + std::unique_ptr ret(new std::string("")); + + ret->assign(l.at(0)->getValue()); + + while (!l.empty()) { + auto &a = l.back(); + l.pop_back(); + delete a; + } + + return ret; + } + + std::string m_name; + private: + AnchoredSetVariable *m_fount; + std::function *l)> m_translate; +}; + +} // namespace modsecurity + +#endif + + +#endif // HEADERS_MODSECURITY_ANCHORED_SET_VARIABLE_TRANSLATION_PROXY_H_ diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 00ea2528..6d9949db 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -48,6 +48,7 @@ typedef struct Rules_t RulesSet; #include "modsecurity/variable_value.h" #include "modsecurity/collection/collection.h" #include "modsecurity/variable_origin.h" +#include "modsecurity/anchored_set_variable_translation_proxy.h" #ifndef NO_LOGS @@ -121,10 +122,7 @@ class Operator; class TransactionAnchoredVariables { public: explicit TransactionAnchoredVariables(Transaction *t) - : m_variableArgsNames(t, "ARGS_NAMES"), - m_variableArgsGetNames(t, "ARGS_GET_NAMES"), - m_variableArgsPostNames(t, "ARGS_POST_NAMES"), - m_variableRequestHeadersNames(t, "REQUEST_HEADERS_NAMES"), + : m_variableRequestHeadersNames(t, "REQUEST_HEADERS_NAMES"), m_variableResponseContentType(t, "RESPONSE_CONTENT_TYPE"), m_variableResponseHeadersNames(t, "RESPONSE_HEADERS_NAMES"), m_variableARGScombinedSize(t, "ARGS_COMBINED_SIZE"), @@ -202,12 +200,12 @@ class TransactionAnchoredVariables { m_variableGeo(t, "GEO"), m_variableRequestCookiesNames(t, "REQUEST_COOKIES_NAMES"), m_variableFilesTmpNames(t, "FILES_TMPNAMES"), - m_variableOffset(0) + m_variableOffset(0), + m_variableArgsNames("ARGS_NAMES", &m_variableArgs), + m_variableArgsGetNames("ARGS_GET_NAMES", &m_variableArgsGet), + m_variableArgsPostNames("ARGS_POST_NAMES", &m_variableArgsPost) { } - AnchoredSetVariable m_variableArgsNames; - AnchoredSetVariable m_variableArgsGetNames; - AnchoredSetVariable m_variableArgsPostNames; AnchoredSetVariable m_variableRequestHeadersNames; AnchoredVariable m_variableResponseContentType; AnchoredSetVariable m_variableResponseHeadersNames; @@ -285,6 +283,10 @@ class TransactionAnchoredVariables { AnchoredSetVariable m_variableFilesTmpNames; int m_variableOffset; + + AnchoredSetVariableTranslationProxy m_variableArgsNames; + AnchoredSetVariableTranslationProxy m_variableArgsGetNames; + AnchoredSetVariableTranslationProxy m_variableArgsPostNames; }; class TransactionSecMarkerManagement { diff --git a/src/Makefile.am b/src/Makefile.am index 67dfc99f..63582010 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -35,6 +35,7 @@ MAINTAINERCLEANFILES = \ pkginclude_HEADERS = \ + ../headers/modsecurity/anchored_set_variable_translation_proxy.h \ ../headers/modsecurity/anchored_set_variable.h \ ../headers/modsecurity/anchored_variable.h \ ../headers/modsecurity/audit_log.h \ diff --git a/src/transaction.cc b/src/transaction.cc index a2ec6c26..822272c9 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -393,17 +393,13 @@ bool Transaction::addArgument(const std::string& orig, const std::string& key, return false; } - size_t k_offset = offset; offset = offset + key.size() + 1; m_variableArgs.set(key, value, offset); - m_variableArgsNames.set(key, key, k_offset); if (orig == "GET") { m_variableArgsGet.set(key, value, offset); - m_variableArgsGetNames.set(key, key, k_offset); } else if (orig == "POST") { m_variableArgsPost.set(key, value, offset); - m_variableArgsPostNames.set(key, key, k_offset); } m_ARGScombinedSizeDouble = m_ARGScombinedSizeDouble + \ diff --git a/test/test-cases/regression/offset-variable.json b/test/test-cases/regression/offset-variable.json index 99c9e19a..de633167 100644 --- a/test/test-cases/regression/offset-variable.json +++ b/test/test-cases/regression/offset-variable.json @@ -882,11 +882,11 @@ ] }, "expected":{ - "error_log":"o0,1v228,1t:lowercase" + "error_log":"o0,1v228,1t:lowercase,t:trim" }, "rules":[ "SecRequestBodyAccess On", - "SecRule REQUEST_COOKIES \"b\" \"id:1,phase:2,pass,t:lowercase,msg:'ops'\"" + "SecRule REQUEST_COOKIES \"b\" \"id:1,phase:2,pass,t:lowercase,t:trim,msg:'ops'\"" ] }, { @@ -1951,5 +1951,67 @@ "SecUploadDir /tmp", "SecRule MULTIPART_NAME \"fiasdfasdfledata\" \"id:1,phase:3,pass,t:trim,msg:'s'\"" ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Variable offset - ARGS n", + "request":{ + "headers":{ + "Host":"localhost", + "Content-Length": "27", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri":"/index.html?param01=5555&bbbbbbbmy_id=6", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "http_code": 403, + "error_log":"o0,1v42,1" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@contains 6\" \"id:1,phase:2,deny,status:403,log\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Variable offset - ARGS_NAMES n", + "request":{ + "headers":{ + "Host":"localhost", + "Content-Length": "27", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri":"/index.html?param01=5555&bbbbbbbmy_id=6", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "http_code": 403, + "error_log":"o7,5v29,12" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS_NAMES \"@contains my_id\" \"id:1,phase:2,deny,status:403,log\"" + ] } ] diff --git a/test/test-cases/regression/variable-ARGS_NAMES.json b/test/test-cases/regression/variable-ARGS_NAMES.json index 97c9aa80..3aaecb30 100644 --- a/test/test-cases/regression/variable-ARGS_NAMES.json +++ b/test/test-cases/regression/variable-ARGS_NAMES.json @@ -164,6 +164,58 @@ "SecRuleEngine On", "SecRule ARGS_NAMES \"@contains test \" \"id:1,phase:3,pass,t:trim\"" ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing Variables :: ARGS_POST_NAMES (3/x)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=0000", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "--0000\r", + "Content-Disposition: form-data; name=\"name1\"\r", + "\r", + "content1\r", + "--0000\r", + "Content-Disposition: form-data; name=\"name2\"\r", + "\r", + "content2\r", + "--0000--\r" + ] + }, + "response":{ + "headers":{ + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"Target value: \"name1\" \\(Variable: ARGS_NAMES\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule ARGS_NAMES \"@contains test \" \"id:1,phase:3,pass,t:trim\"" + ] } ] diff --git a/test/test-cases/regression/variable-ARGS_POST_NAMES.json b/test/test-cases/regression/variable-ARGS_POST_NAMES.json index 5c3fd404..fb0964f3 100644 --- a/test/test-cases/regression/variable-ARGS_POST_NAMES.json +++ b/test/test-cases/regression/variable-ARGS_POST_NAMES.json @@ -2,7 +2,7 @@ { "enabled":1, "version_min":300000, - "title":"Testing Variables :: ARGS_POST_NAMES (1/2)", + "title":"Testing Variables :: ARGS_POST_NAMES (1/x)", "client":{ "ip":"200.249.12.31", "port":123 @@ -46,7 +46,7 @@ { "enabled":1, "version_min":300000, - "title":"Testing Variables :: ARGS_POST_NAMES (2/2)", + "title":"Testing Variables :: ARGS_POST_NAMES (2/x)", "client":{ "ip":"200.249.12.31", "port":123 @@ -86,6 +86,110 @@ "SecRuleEngine On", "SecRule ARGS_POST_NAMES \"@contains test \" \"id:1,phase:3,pass,t:trim\"" ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing Variables :: ARGS_POST_NAMES (3/x)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=0000", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "--0000\r", + "Content-Disposition: form-data; name=\"name1\"\r", + "\r", + "content1\r", + "--0000\r", + "Content-Disposition: form-data; name=\"name2\"\r", + "\r", + "content2\r", + "--0000--\r" + ] + }, + "response":{ + "headers":{ + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"Target value: \"name1\" \\(Variable: ARGS_POST_NAMES\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule ARGS_POST_NAMES \"@contains test \" \"id:1,phase:3,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing Variables :: ARGS_POST_NAMES (3/x)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=0000", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "--0000\r", + "Content-Disposition: form-data; name=\"name1\"\r", + "\r", + "content1\r", + "--0000\r", + "Content-Disposition: form-data; name=\"name2\"\r", + "\r", + "content2\r", + "--0000--\r" + ] + }, + "response":{ + "headers":{ + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "error_log":"o0,5v206,5t:trim" + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule ARGS_POST_NAMES \"@contains name1\" \"id:1,phase:3,pass,t:trim\"" + ] } ]