8000 Amend regular expression pattern for rule 942440 by RedXanadu · Pull Request #2201 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Amend regular expression pattern for rule 942440 #2201

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.

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

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

RedXanadu
Copy link
Member

This should make the regular expression have the same end result with
both Apache/mod_security2 and nginx/libmodsecurity

@RedXanadu
Copy link
Member Author

This PR is intended to resolve the issue with the regular expression pattern with rule 942440, as discussed in issue #2179

@RedXanadu
Copy link
Member Author

Added an additional test for rule 942440: ensure that the URL encoded null byte example input is correctly caught

@theseion
Copy link
Contributor

If it makes sense, could you break up the expression and move it to a data file so we can generate it?

@dune73
Copy link
Member
dune73 commented Sep 22, 2021

We can also do an issue to make this as a separate step. But I agree, this rule ought to have a source-file with the individual regexes and we generate it. It might also become faster / optimized that way.

@RedXanadu
Copy link
Member Author
RedXanadu commented Sep 22, 2021

Okay, so can we look at that in a separate issue, then? IMO issue #2179 is already too complex/broad (that's my fault).

@theseion
Copy link
Contributor

Sure, put it in a separate issue.

@dune73
Copy link
Member
dune73 commented Sep 22, 2021

No worries @RedXanadu. We're happy you are solving it, step by step.

@RedXanadu
Copy link
Member Author

Okay: I looked into it and realised that regexp-assemble is not as complicated as I'd thought!

Could someone check the commits, please? I think I understand how to use regexp-assemble, but this is my first time properly using it…

I did notice that regexp-assemble.pl converts \x00 into a literal null character in the output. I've added a note to rule 942440 explaining this and stating that the \x00 must be added back in by hand after assembly.

(P.S. The REQUEST-920 regression tests are failing, just like in PR #2208. I'm still not sure why.)

@theseion
Copy link
Contributor

@RedXanadu I just glanced at the changes for regexp-assemble and they look good to me. However, the conversion of \\x00 to a literal null is probablly undesirable. And having to manually adapt the rule pretty much defeats the purpose of the script.

I've opened #2210 to fix the issue.

Does the test work when the expression contains a null? If so, please remove the documentation on manual manipulation. Otherwise leave the documentation in but open an issue to remove it again once #2210 has been fixed.

@RedXanadu
Copy link
Member Author

Thank you @theseion. It looked like a bug, but I wasn't certain: thanks for opening one :)

Does the test work when the expression contains a null?

I'm not sure I understand the question. Do you mean, does rule 942440 work correctly if the regular expression contains the literal null character that regexp-assemble.pl outputs to stdout? Like so:

image

ModSecurity refuses to start in that scenario:

AH00526: Syntax error on line 1259 of /etc/modsecurity.d/owasp-crs/rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf:
Error creating rule: Error compiling pattern (offset 73): missing )

@theseion
Copy link
Contributor

I'm not sure I understand the question. Do you mean, does rule 942440 work correctly if the regular expression contains the literal null character that regexp-assemble.pl outputs to stdout? Like so:

Yes. That's ok then. leave it like you have it know and I'll clean it up once the issue is fixed.

@lifeforms
Copy link
Member

The failing tests will probably succeed when you rebase the PR on v3.4/dev, could you try that please?

@franbuehler
Copy link
Contributor

Meeting decision Oct 4: a rebase should fix the failing tests.

This should make the regular expression have the same end result with
both Apache/mod_security2 and nginx/libmodsecurity
Adds a test for a SQL comment ending with a URL encoded null character.
Historically, mod_security2 on Apache would catch such input but
libmodsecurity would not: this test should now pass on both platforms
@RedXanadu
Copy link
Member Author

I believe this is good to go, and can be simplified once the new regex assembler is in place (for now, manual modifications are required, so the documentation on how to do that should stay for now).

@theseion
Copy link
Contributor

I think so too.

@fzipi
Copy link
Member
fzipi commented Oct 14, 2021

Excellent, I concur. Merging then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent representation of the backslash character in search patterns
6 participants
0