8000 fix 933180 regex by theMiddleBlue · Pull Request #2303 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix 933180 regex #2303

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 5 commits into from
Closed

fix 933180 regex #2303

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
afa49ce
fix 933180 regex
theMiddleBlue Nov 10, 2021
f4b89aa
remove comments and whitespaces
theMiddleBlue Nov 10, 2021
9f1126b
replace comment // with \W{2,}
theMiddleBlue Nov 10, 2021
60f8ae2
remove the 2nd part of the regex
theMiddleBlue Nov 10, 2021
d748448
convert .+ to .* before ()
theMiddleBlue Nov 10, 2021
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
4 changes: 2 additions & 2 deletions rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]*|{.+}).*\(.*\)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"id:933180,\
phase:2,\
block,\
capture,\
t:none,\
t:none,t:compressWhitespace,t:removeWhitespace,t:removeCommentsChar,\
Copy link
Contributor

Choose a reason for hiding this comment

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

removeWhitespace makes compressWhitespace unnecessary, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not clear on modsec documentation if removeWhitespace removes also \n. we do know that compressWhitespace replace \n with a space \x20

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I wondered about that too :/

Copy link
Member
@RedXanadu RedXanadu Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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… 🙂

msg:'PHP Injection Attack: Variable Function Call Found',\
logdata:'Matched Data: %{TX.0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',\
tag:'application-multi',\
Expand Down
0