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

RCE: prevent bypass rule 930120 (PL3) #1136

Merged
merged 1 commit into from Aug 6, 2018

Conversation

Projects
None yet
5 participants
@theMiddleBlue
Copy link
Contributor

theMiddleBlue commented Jul 8, 2018

Referencing to #1131 this rule, added on owasp-modsecurity-crs/rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf, tries to detect wildcard ? and * usage in order to bypass rule 930120 (OS File Access Attempt) on Paranoia Lvel 3.

A POC was sent to security@coreruleset.org in order to not sharing on GitHub critical information about how to bypass the whole rule set to exploit a RCE.

At the time, I've placed this rule in PL3 because, unfortunately, it could lead to many false positives.

Thanks @lifeforms @spartantri @csanders-git and @fgsch for contributing.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 10, 2018

Should we include JSON too?

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor

theMiddleBlue commented Jul 11, 2018

It seems legit! Do you mean by adding REQUEST_BODY in the variables list? Should we add Headers Cookie for insecure deserialization that could lead to RCE? or is it useless because it's intercepted by others rules on PL3?

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 11, 2018

Well, I thought of ARGS|JSON|XML.

It is really hard to tell for me which additional variables should be checked. I think you need to think about the backends that could be affected. How are they typically reading and parsing cookies? And maybe check with other rules if they cover it too.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 12, 2018

This also needs a test added so that it can be merged. @theMiddleBlue can you help with that if you have questions I can walk you through how to add tests on IRC or. https://coreruleset.org/20171214/practical-ftw-testing-the-core-rule-set-or-any-other-waf/

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor

theMiddleBlue commented Jul 12, 2018

Thanks @csanders-git, I write to you on IRC because I've got a couple of questions about it.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 12, 2018

Thanks for the add.

@csanders-git
Copy link
Collaborator

csanders-git left a comment

I guess this means our most recent change didn't fix it right... :-/

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jul 12, 2018

@csanders-git no, this PR is not using the latest changes since it was not rebased.

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor

theMiddleBlue commented Jul 12, 2018

I rolled back to the previous version (without JSON variable) because it fails during the regression test. Am I missing something?

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Jul 12, 2018

There is no JSON variable when JSON body processor is being used it populates the ARGS collection

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jul 12, 2018

Good thinking. My bad. <leads himself out the door/>

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Aug 6, 2018

Thank you for your submission.

@csanders-git csanders-git merged commit 64ad6fb into SpiderLabs:v3.1/dev Aug 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment