fix 933180 regex #2303
fix 933180 regex #2303
Conversation
4bb7479 to
f4b89aa
Compare
|
converted to draft because of #2301 (comment) |
|
I've removed a significant part of the regex in order to fix it... all tests pass but it might be more inclusive than before. Any thoughts? |
| "id:933180,\ | ||
| phase:2,\ | ||
| block,\ | ||
| capture,\ | ||
| t:none,\ | ||
| t:none,t:compressWhitespace,t:removeWhitespace,t:removeCommentsChar,\ |
There was a problem hiding this comment.
removeWhitespace makes compressWhitespace unnecessary, right?
There was a problem hiding this comment.
it's not clear on modsec documentation if removeWhitespace removes also \n. we do know that compressWhitespace replace \n with a space \x20
There was a problem hiding this comment.
Yeah, I wondered about that too :/
There was a problem hiding this comment.
I'm not sure if this will be helpful anymore, but maybe it will be 😄
removeWhitespace in ModSec 2:
if (isspace(input[i])||(input[i] == NBSP))
removeWhitespace in ModSec 3:
if (std::isspace(static_cast<unsigned char>(value[i]))
|| (value[i] == nonBreakingSpaces)
|| value[i] == nonBreakingSpaces2)
The isspace function: “Checks if the given character is a whitespace character, i.e. either space (0x20), form feed (0x0c), line feed (0x0a), carriage return (0x0d), horizontal tab (0x09) or vertical tab (0x0b).”
So removeWhitespace appears to function on the characters listed in the previous sentence, plus one or two non-breaking spaces 🙂
P.S. I wish the ModSecurity Reference Manual could accept PRs, as this info should be on there… 🙂
| @@ -436,12 +436,12 @@ SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|REQUEST_H | |||
| # \(.*\) | |||
| # Parentheses optionally containing function parameters | |||
| # | |||
| SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|REQUEST_FILENAME|ARGS_NAMES|ARGS|XML:/* "@rx \$+(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*|\s*{.+})(?:\s|\[.+\]|{.+}|/\*.*\*/|//.*|#.*)*\(.*\)" \ | |||
| SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|REQUEST_FILENAME|ARGS_NAMES|ARGS|XML:/* "@rx \$+(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*|{.+}).*\(.*\)" \ | |||
There was a problem hiding this comment.
Removing the comment markers forces you to match .* instead of (?:\[.+\]|{.+}|/\*.*\*/|//.*|#.*) (notice that I removed the \s from that expression). Does that really make sense? Yes, backtracking is better but the regex becomes much more open and prone to FPs. In combination with the removal of spaces you can easily produce FPS: iget10$anhour(no,idon'tgetpaidforovertime). Besides, the regex is vulnerable to DOSing anyway.
I would compress spaces but not remove them, then you can match against single spaces at least.
There was a problem hiding this comment.
Besides, the regex is vulnerable to DOSing anyway.
uhm why? @airween checked it but no redos were found anymore
I would compress spaces but not remove them, then you can match against single spaces at least.
yep, I can try to fix it without removing whitespace. I'm going to send a new version tnx
There was a problem hiding this comment.
uhm why? @airween checked it but no redos were found anymore
You're right. I had made a mistake when copying around different versions. Your version is fine.
|
Any status here, guys? |
|
Assembly file? ;) |
|
the regex has changed a lot from when I've opened this PR. The regex is no more affected by ReDoS: previous regex: \$+(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*|\s*{.+})(?:\s|\[.+\]|{.+}|/\*.*\*/|//.*|#.*)*\(.*\)
Pattern: \$+(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*|\s*{.+})(?:\s|\[.+\]|{.+}|/\*.*\*/|//.*|#.*)*\(.*\)
---
Redos(starriness=11, prefix_sequence=SEQ{ [24:$]{1+} [5f:_,[a-z],[A-Z],[7f-ff]] [5f:_,[a-z],[0-9],[A-Z],[7f-ff]]{0+} }, redos_sequence=SEQ{ [2f:/]{2+}{0+} [28:(] }, repeated_character=[2f:/], killer=None)
Worst-case complexity: 11 ⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐ (exponential)
Repeated character: [2f:/]
Example: '$a' + '/' * 3456
Redos(starriness=11, prefix_sequence=SEQ{ [24:$]{1+} [5f:_,[a-z],[A-Z],[7f-ff]] [5f:_,[a-z],[0-9],[A-Z],[7f-ff]]{0+} }, redos_sequence=SEQ{ [23:#]{1+}{0+} [28:(] }, repeated_character=[23:#], killer=None)
Worst-case complexity: 11 ⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐ (exponential)
Repeated character: [23:#]
Example: '$a' + '#' * 3456
Redos(starriness=11, prefix_sequence=SEQ{ [24:$]{1+} [SPACE]{0+} [7b:{] .{1+} [7d:}] }, redos_sequence=SEQ{ [2f:/]{2+}{0+} [28:(] }, repeated_character=[2f:/], killer=None)
Worst-case complexity: 11 ⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐ (exponential)
Repeated character: [2f:/]
Example: '${0}' + '/' * 3456
Redos(starriness=11, prefix_sequence=SEQ{ [24:$]{1+} [SPACE]{0+} [7b:{] .{1+} [7d:}] }, redos_sequence=SEQ{ [23:#]{1+}{0+} [28:(] }, repeated_character=[23:#], killer=None)
Worst-case complexity: 11 ⭐⭐⭐⭐⭐⭐⭐⭐⭐⭐ (exponential)
Repeated character: [23:#]
Example: '${0}' + '#' * 3456now \$+(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*|{.+}).*\(.*\)
No ReDoS found.closing |
|
Procrastination for the win! :) |
same fix made on #2301