Skip to content
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

Prevent bypass 933180 PHP Variable Function on PL1 #1294

Merged
merged 2 commits into from Feb 4, 2019

Conversation

Projects
None yet
6 participants
@theMiddleBlue
Copy link
Contributor

theMiddleBlue commented Jan 26, 2019

Referring to #1274 this new rule (933210) prevents to bypass rule 933180 using PHP syntax like the following:

- (system)(ls)
- (/**/system)(ls/**/);
- (['system'])[0]('uname');
- (++[++system++][++0++])++{/*dsasd*/0}++(++ls++);
- (sy.(st).em)('uname')
- (string)"system"('uname')
- define('x', 'sys' . 'tem');(x)/* comment */('uname');
- $y = 'sys'.'tem';($y)('uname')
- define('z', [['sys' .'tem']]);(z)[0][0]('uname');
etc...

I propose to put this rule in PL1 because I think it shouldn't be prone to many FPs. I've created a script that try to catch some FPs and this is the result:

image

As you can see, this regex avoids to block values like:

- [ACME] this is a test (just a test)
- [ACME]: this is, a test. (just a test)!
- [ACME]: this is, (another) test (foo)bar or foo(bar).
- (foo)bar or foo(bar) !or! [foo]bar or foo[bar]
- Test (with two) rounded (brackets)

I've used the following transformation functions:

  • urlDecode (obviously)
  • compressWhitespace (useful for something like (+++system+++))
  • replaceComments (remove C-like comments (system/*foo*/)(/*bar*/ls))

Questions:

  • What do you think about?
  • Is there anyone who wants to test others payloads or FPs?
  • Is 933210 good for the CRS rule numbering policy?
@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jan 26, 2019

Wow, thank you. Very nice contribution @theMiddleBlue.

Addressing you questions:

  • I really like this. You mentioned working on this, and I was not sure how you would go about it.
  • Testing for FPs is a necessity, I think. Maybe we will have to shift it to a higher PL, but we could merge at PL1 as we are still quite far from an eventual 3.2 release and thus time to shift.
  • Given 933200 is taken, I think the rule id 933210 is just fine.

Issues

  • The comments about payloads and FPs that are covered should be transformed into FTW tests before merging. Thank you for having prepared this so nicely.
  • Somebody needs to really think hard about the regular expression.
@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Jan 28, 2019

You don't need to use \x notation for all characters, it makes it really hard to read.
Like \x28[\x20]*string[\x20]*\x29[\x20] VS \(\s+string\s+\)

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 28, 2019

thanks @spartantri I thought that regex would be more readable if round brackets inside groups are converted to hex. Same thing for square brackets... If not I convert all hex sequence to chars

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jan 29, 2019

I'm actually siding with @spartantri, here. The way it is, it is very hard to read.

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 29, 2019

thanks @dune73 I'm changing the regex with less hex escape sequences as possible. I'll push it after doing some tests. Thank you all for all the hints!

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 29, 2019

  • removed all hex escape sequences (except double quotes and single quote).
  • rebased all commits to a single one.

Could you guys check if it's ok?

Thanks!

@lifeforms lifeforms self-assigned this Jan 29, 2019

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jan 29, 2019

Looks awesome, I'll give it a spin. Possible to add any tests maybe? ;)

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 29, 2019

thanks @lifeforms

Possible to add any tests maybe? ;)

It would be great! Send me your payloads and I'll add them to my script :)

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jan 29, 2019

I mean as YAML tests in the commit :)
Sure I'll try to add more.

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 29, 2019

I mean as YAML tests in the commit :)

oh sorry, thought you referring to the screenshot :)

Yeah, I can add a YAML file for this rule. Should I create a new PR or should I add it to this one?

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jan 29, 2019

Just add it to this one! Together they make a single unit. So it's nice to have them combined.

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 29, 2019

added a first draft of 933210.yaml. I've tested on my nginx and it seems ok:

image

anyone could check?

@spartantri
Copy link
Collaborator

spartantri left a comment

Nice work!

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jan 30, 2019

Sorry to nose in late in the game but why are you using hex values for ' and "?

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 30, 2019

why are you using hex values for ' and "?

just because I've got some problems to make it works with double and single quotes outside character class in my test script (don't know why), but I've just seen that the regex works well in modsecurity without encoding them.

I'm updating this PR removing \x22 and \x27 with " and '.

thanks!

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jan 30, 2019

Cool. Don't forget to update the comment :)

@theMiddleBlue

This comment has been minimized.

Copy link
Contributor Author

theMiddleBlue commented Jan 30, 2019

OMG 😭 I absolutely need to sleep more. Sorry, updating...

@csanders-git csanders-git merged commit 97969ea into SpiderLabs:v3.2/dev Feb 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fgsch fgsch referenced this pull request Mar 8, 2019

Closed

PHP function name bypasses #1274

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.