From ce6ad3c4c9224073debe86b9933954089bd60ed2 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Wed, 6 Feb 2019 22:10:34 +0000 Subject: [PATCH 1/5] Aligned Cookie parsing method to ModSecurity2 style --- src/transaction.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/transaction.cc b/src/transaction.cc index 693bed6b90..97911c5167 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -571,6 +571,14 @@ int Transaction::addRequestHeader(const std::string& key, m_variableRequestCookies.set(s[0], s[1], localOffset); localOffset = localOffset + s[1].size() + 2; } + else { + if (s[0].at(0) == ' ') { + s[0].erase(0, 1); + } + m_variableRequestCookiesNames.set(s[0], + s[0], localOffset); + m_variableRequestCookies.set(s[0], "", localOffset); + } } } /** From 93bea7bb4522cbce262d9f3e3c308677afb9ee98 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 12 Feb 2019 20:54:15 +0000 Subject: [PATCH 2/5] Handle possible empty vector case if cookie is not wellformed --- src/transaction.cc | 20 ++++------ test/test-cases/regression/rule-942480.json | 41 +++++++++++++++++++++ 2 files changed, 49 insertions(+), 12 deletions(-) create mode 100644 test/test-cases/regression/rule-942480.json diff --git a/src/transaction.cc b/src/transaction.cc index 97911c5167..a6198593ef 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -560,24 +560,20 @@ int Transaction::addRequestHeader(const std::string& key, for (const std::string &c : cookies) { std::vector s = utils::string::split(c, '='); - if (s.size() > 1) { - if (s[0].at(0) == ' ') { + if (s.size() > 0) { + while (s[0].at(0) == ' ') { s[0].erase(0, 1); } m_variableRequestCookiesNames.set(s[0], s[0], localOffset); - localOffset = localOffset + s[0].size() + 1; - m_variableRequestCookies.set(s[0], s[1], localOffset); - localOffset = localOffset + s[1].size() + 2; - } - else { - if (s[0].at(0) == ' ') { - s[0].erase(0, 1); + if (s.size() > 1) { + m_variableRequestCookies.set(s[0], s[1], localOffset); + localOffset = localOffset + s[1].size() + 2; + } + else { + m_variableRequestCookies.set(s[0], "", localOffset); } - m_variableRequestCookiesNames.set(s[0], - s[0], localOffset); - m_variableRequestCookies.set(s[0], "", localOffset); } } } diff --git a/test/test-cases/regression/rule-942480.json b/test/test-cases/regression/rule-942480.json new file mode 100644 index 0000000000..5602c5771f --- /dev/null +++ b/test/test-cases/regression/rule-942480.json @@ -0,0 +1,41 @@ +[ + { + "enabled":1, + "version_min":300000, + "title":"Testing setvar :: OWASP CRS id:942480", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "Cookie": "'msdasql'" + }, + "uri":"/", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "http_code":400 + }, + "rules":[ + "SecRuleEngine On", + "SecDefaultAction \"phase:2,deny,block,status:400,log\"", + "SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|!REQUEST_COOKIES:/_pk_ref/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* \"@rx (?i:(?:\\b(?:(?:s(?:elect\\b.{1,100}?\\b(?:(?:(?:length|count)\\b.{1,100}?|.*?\\bdump\\b.*)\\bfrom|to(?:p\\b.{1,100}?\\bfrom|_(?:numbe|cha)r)|(?:from\\b.{1,100}?\\bwher|data_typ)e|instr)|ys_context)|in(?:to\\b\\W*?\\b(?:dump|out)file|sert\\b\\W*?\\binto|ner\\b\\W*?\\bjoin)|u(?:nion\\b.{1,100}?\\bselect|tl_inaddr)|group\\b.*?\\bby\\b.{1,100}?\\bhaving|d(?:elete\\b\\W*?\\bfrom|bms_\\w+\\.)|load\\b\\W*?\\bdata\\b.*?\\binfile)\\b|print\\b\\W*?\\@\\@)|(?:;\\W*?\\b(?:shutdown|drop)|collation\\W*?\\(a|\\@\\@version)\\b|'(?:s(?:qloledb|a)|msdasql|dbo)'))\" \"id:942480,phase:2,block,capture,t:none,t:urlDecodeUni,msg:'SQL Injection Attack',logdata:'Matched Data: %{TX.0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',ctl:auditLogParts=+E,ver:'OWASP_CRS/3.1.0',severity:'CRITICAL',setvar:'tx.msg=%{rule.msg}',setvar:'tx.sql_injection_score=+%{tx.critical_anomaly_score}', setvar:'tx.anomaly_score_pl2=+%{tx.critical_anomaly_score}',setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SQL_INJECTION-%{MATCHED_VAR_NAME}=%{tx.0}'\"" + ] + } +] From 16ccab40572898fc43c33d1d35f1ef89eb2210d3 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 19 Feb 2019 13:16:17 +0000 Subject: [PATCH 3/5] Added more cookie pattern --- test/test-cases/regression/rule-942480.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/test-cases/regression/rule-942480.json b/test/test-cases/regression/rule-942480.json index 5602c5771f..4871b0c5ca 100644 --- a/test/test-cases/regression/rule-942480.json +++ b/test/test-cases/regression/rule-942480.json @@ -14,7 +14,7 @@ "request":{ "headers":{ "Host":"localhost", - "Cookie": "'msdasql'" + "Cookie": "'msdasql'; key1=val1; key2=val2=val2a; key3 = val3 " }, "uri":"/", "method":"GET" @@ -30,7 +30,8 @@ ] }, "expected":{ - "http_code":400 + "http_code":400, + "debug_log":"'msdasql' with value: 'msdasql'" }, "rules":[ "SecRuleEngine On", From 4c35ff2ed8b99f7602d35cb20431220d1bcf0df1 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 19 Feb 2019 14:10:33 +0000 Subject: [PATCH 4/5] Changed cookie parsing method --- src/transaction.cc | 50 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/src/transaction.cc b/src/transaction.cc index a6198593ef..0c32e67982 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -556,25 +556,45 @@ int Transaction::addRequestHeader(const std::string& key, if (keyl == "cookie") { size_t localOffset = m_variableOffset; + size_t pos1, pos2; std::vector cookies = utils::string::ssplit(value, ';'); for (const std::string &c : cookies) { - std::vector s = utils::string::split(c, - '='); - if (s.size() > 0) { - while (s[0].at(0) == ' ') { - s[0].erase(0, 1); - } - m_variableRequestCookiesNames.set(s[0], - s[0], localOffset); - localOffset = localOffset + s[0].size() + 1; - if (s.size() > 1) { - m_variableRequestCookies.set(s[0], s[1], localOffset); - localOffset = localOffset + s[1].size() + 2; - } - else { - m_variableRequestCookies.set(s[0], "", localOffset); + pos1 = c.find_first_of("=", 0); + std::string ckey = ""; + std::string cval = ""; + + // if the cookie doesn't contains '=', its just a key + if (pos1 == std::string::npos) { + ckey = c; + } + // else split to two substrings + else { + ckey = c.substr(0, pos1); + cval = c.substr(pos1+1, c.length()); + } + + // remove leading spaces from key + while (ckey.at(0) == ' ') { + ckey.erase(0, 1); + localOffset++; + } + // replace remained spaces with underscore in key + pos2 = 0; + while(pos2 < ckey.length()) { + if (ckey.at(pos2) == ' ') { + ckey.replace(pos2, 1, "_"); } + pos2++; + } + if (ckey.length() > 0) { + // set cookie name + m_variableRequestCookiesNames.set(ckey, + ckey, localOffset); + localOffset = localOffset + ckey.length() + 1; + // set cookie value + m_variableRequestCookies.set(ckey, cval, localOffset); } + localOffset = localOffset + cval.length() + 1; } } /** From d3917a58eb44475cd014e10c91b91628e4cc9239 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Sat, 1 Jun 2019 10:17:03 +0000 Subject: [PATCH 5/5] Added regression test to Makefile.am --- Makefile.am | 1 + test/test-cases/regression/rule-942480.json | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index be32f7bef7..b6a22ac66a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -176,6 +176,7 @@ TESTS+=test/test-cases/regression/request-body-parser-xml-validade-dtd.json TESTS+=test/test-cases/regression/rule-920120.json TESTS+=test/test-cases/regression/rule-920200.json TESTS+=test/test-cases/regression/rule-920274.json +TESTS+=test/test-cases/regression/rule-942480.json TESTS+=test/test-cases/regression/secaction.json TESTS+=test/test-cases/regression/sec_component_signature.json TESTS+=test/test-cases/regression/secmarker.json diff --git a/test/test-cases/regression/rule-942480.json b/test/test-cases/regression/rule-942480.json index 4871b0c5ca..dc588a8be6 100644 --- a/test/test-cases/regression/rule-942480.json +++ b/test/test-cases/regression/rule-942480.json @@ -2,7 +2,7 @@ { "enabled":1, "version_min":300000, - "title":"Testing setvar :: OWASP CRS id:942480", + "title":"Testing cookie parser :: OWASP CRS id:942480", "client":{ "ip":"200.249.12.31", "port":123