Skip to content

V3/collection re fix #2297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

V3/collection re fix #2297

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df9e453
Regex search in collection data case insensitive fix
airween Apr 22, 2020
ceb0e38
Replace regression test as it expected
airween Oct 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,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-920450.json
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect a different name for that file. Explained below.

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

TESTS+=test/test-cases/regression/secaction.json
TESTS+=test/test-cases/regression/secargumentslimit.json
TESTS+=test/test-cases/regression/sec_component_signature.json
Expand Down
2 changes: 1 addition & 1 deletion src/collection/backend/in_memory-per_process.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ void InMemoryPerProcess::resolveRegularExpression(const std::string& var,
//std::string name = std::string(var, var.find(":") + 2,
// var.size() - var.find(":") - 3);
//size_t keySize = col.size();
Utils::Regex r(var);
Utils::Regex r(var, true);

for (const auto& x : *this) {
//if (x.first.size() <= keySize + 1) {
Expand Down
2 changes: 1 addition & 1 deletion src/collection/backend/lmdb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ void LMDB::resolveRegularExpression(const std::string& var,
MDB_stat mst;
MDB_cursor *cursor;

Utils::Regex r(var);
Utils::Regex r(var, true);

rc = mdb_txn_begin(m_env, NULL, 0, &txn);
lmdb_debug(rc, "txn", "resolveRegularExpression");
Expand Down
8 changes: 6 additions & 2 deletions src/utils/regex.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,16 @@ namespace modsecurity {
namespace Utils {


Regex::Regex(const std::string& pattern_)
Regex::Regex(const std::string& pattern_, bool ignoreCase)
: pattern(pattern_.empty() ? ".*" : pattern_) {
const char *errptr = NULL;
int erroffset;
int flags = (PCRE_DOTALL|PCRE_MULTILINE);

m_pc = pcre_compile(pattern.c_str(), PCRE_DOTALL|PCRE_MULTILINE,
if (ignoreCase == true) {
flags |= PCRE_CASELESS;
}
m_pc = pcre_compile(pattern.c_str(), flags,
&errptr, &erroffset, NULL);

m_pce = pcre_study(m_pc, pcre_study_opt, &errptr);
Expand Down
2 changes: 1 addition & 1 deletion src/utils/regex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class SMatch {

class Regex {
public:
explicit Regex(const std::string& pattern_);
explicit Regex(const std::string& pattern_, bool ignoreCase = false);
~Regex();

// m_pc and m_pce can't be easily copied
Expand Down
87 changes: 87 additions & 0 deletions test/test-cases/regression/rule-920450.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
[
{
"enabled":1,
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a pull request that was well described in a issue i miss the github_issue tag.

As listed -
https://github.com/SpiderLabs/ModSecurity/blob/785958f9b5089b918c7d054cbcc2fe4a3c7b3788/test/test-cases/regression/issue-960.json#L7-L9

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for info. The last commit contains both 'url' and 'github_issue' keys, in both test cases.

Note: it would be good to make some wiki page about your expectations for contributors. I'm sure these keywords are part of the regression test structure, but after 17 commits I've met with them first.

"version_min":300000,
"title":"OWASP CRS id:920450",
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have a title with a more meaningful message. If the test "OWASP CRS id:920450" fails, what does it means to the user?

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in ceb0e3

"client":{
"ip":"200.249.12.31",
"port":123
},
"server":{
"ip":"200.249.12.31",
"port":80
},
"request":{
"headers":{
"TRANSLATE": "test"
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use real request headers, to mimic a real world example.

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

},
"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":{
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specially because the rules are complex, this test case could fail silent... as a combination of two (or more) failures. Better to also considers the debug and error log (when possible).

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the test cases: both of them block the request, and gives the error_log, which used in "expected" block.

"http_code":403
},
"rules":[
"SecRuleEngine On",
"SecRule &TX:restricted_headers \"@eq 0\" \"id:901165,phase:1,pass,nolog,setvar:'tx.restricted_headers=/proxy/ /lock-token/ /content-range/ /translate/ /if/'\"",
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same rule complexity issue explained below.

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

"SecRule REQUEST_HEADERS_NAMES \"@rx ^.*$\" \"id:920450,phase:2,capture,block,deny,t:lowercase,setvar:'tx.header_name_%{tx.0}=/%{tx.0}/',chain",
"SecRule TX:/^HEADER_NAME_/ \"@within %{tx.restricted_headers}\" \"\""
]
},
{
"enabled":1,
"version_min":300000,
"title":"OWASP CRS id:920450",
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fixed too.

"client":{
"ip":"200.249.12.31",
"port":123
},
"server":{
"ip":"200.249.12.31",
"port":80
},
"request":{
"headers":{
"Accept": "text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5",
"Accept-Charset": "ISO-8859-1,utf-8;q=0.7,*;q=0.7",
"Accept-Encoding": "gzip,deflate",
"Accept-Language": "en-us,en;q=0.5",
"Host": "localhost",
"Keep-Alive": "300",
"Proxy-Connection": "keep-alive",
"User-Agent": "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv"
},
"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":{
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment above. Use error and debug logs.

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed too.

"http_code":200
},
"rules":[
"SecRuleEngine On",
"SecRule &TX:restricted_headers \"@eq 0\" \"id:901165,phase:1,pass,nolog,setvar:'tx.restricted_headers=/proxy/ /lock-token/ /content-range/ /translate/ /if/'\"",
Copy link
Contributor

@zimmerle zimmerle Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a value of having complex rules on the regression tests as it increases surface area for testing features. However, I am afraid that together with the complex example, we need to have a very simple use case scenario. If/When this test fails, it was a consequence of a problem on the selection of variables using regular expression or elsewhere (e.g. setvar)?

Copy link
Member Author

@airween airween Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the rules in the new regression test file. I based @michaelgranzow-avi test cases, but there are only two cases, with same context: only the TX variable name format differ.

"SecRule REQUEST_HEADERS_NAMES \"@rx ^.*$\" \"id:920450,phase:2,capture,block,deny,t:lowercase,setvar:'tx.header_name_%{tx.0}=/%{tx.0}/',chain",
"SecRule TX:/^HEADER_NAME_/ \"@within %{tx.restricted_headers}\" \"\""
]
}
]