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

Add rule 933200 PHP Wrappers #1172

Merged
merged 4 commits into from Dec 3, 2018

Conversation

Projects
None yet
5 participants
@theMiddleBlue
Contributor

theMiddleBlue commented Aug 17, 2018

Referring to #1171 I'm testing this rule in production, and at the moment seems to works fine without FP in PL1. About the wrapper phar:// please refer to: https://github.com/s-n-t/presentations/blob/master/us-18-Thomas-It's-A-PHP-Unserialization-Vulnerability-Jim-But-Not-As-We-Know-It.pdf

theMiddleBlue added some commits Aug 17, 2018

@theMiddleBlue theMiddleBlue changed the title from Add rule 933200 PHP Wrappers to (draft: do not merge yet) Add rule 933200 PHP Wrappers Aug 17, 2018

@theMiddleBlue

This comment has been minimized.

Contributor

theMiddleBlue commented Aug 20, 2018

Referring to #1171 (comment) I've fixed the regex and added zip:// to the list of wrappers. As said in the issue, I've omitted http, https, data, file and ftp that could lead to many FPs.

@theMiddleBlue theMiddleBlue changed the title from (draft: do not merge yet) Add rule 933200 PHP Wrappers to Add rule 933200 PHP Wrappers Aug 20, 2018

@dune73

This comment has been minimized.

Collaborator

dune73 commented Oct 2, 2018

Nice and welcome PR. It's not going into 3.1, but we are open to merging into 3.2.

Open issues:

  • Is the capture and reference to tx.0 really needed? Can't you go with MATCHED_VAR.
  • Rule ID is OK, but why the position in the middle of the PL1 rules? Why not at the end of PL1? The order of the rule IDs is a bit weird. Putting it where it is now would be OK if it was because of grouping similar rules.
  • Link to talk by Sam Thomas at BlackHat USA 2018 would be welcome. Linking slides or blog post if available even more so.
@theMiddleBlue

This comment has been minimized.

Contributor

theMiddleBlue commented Oct 2, 2018

Thanks for reply @dune73

  • Is the capture and reference to tx.0 really needed? Can't you go with MATCHED_VAR.

yes, I think that the capture function and tx.0 var could be removed (I've just copied from the 933140) unless we want to log the specific wrapper blocked by rule (I mean with tx.1)

  • Rule ID is OK, but why the position in the middle of the PL1 rules? Why not at the end of PL1? The order of the rule IDs is a bit weird. Putting it where it is now would be OK if it was because of grouping similar rules.

yes, I've put it there for "continuity" from the 933140. What's the better approach? keep rules ordered by their id? maybe yes... it sounds more clean. What do you think about?

  • Link to talk by Sam Thomas at BlackHat USA 2018 would be welcome. Linking slides or blog post if available even more so.

Nice! I'll add the presentation available on the author's github repo.

Can I push a new commit with all changes on this PR?

@dune73

This comment has been minimized.

Collaborator

dune73 commented Oct 2, 2018

Yes, please go ahead with committing to this PR.

I think it's safe to remove the capture / TX.0. And the position makes sense, but maybe add a remark in the comments, that it is very close to the PHP I/O streams. Weird rule order till happen as we move along, so it's best to just live with it. A renumbering would be far worse.

# could lead to RCE as describled by Sam Thomas at BlackHat USA 2018, even wrappers like zlib://,
# glob://, rar://, zip://, etc... could lead to LFI and expect:// to RCE.
#
SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@rx (?i)(?:zlib|glob|phar|ssh2|rar|ogg|expect|zip)://([^/]+|/[^/]+)" \

This comment has been minimized.

@fgsch

fgsch Oct 2, 2018

Collaborator

can we drop the capture groups please?

This comment has been minimized.

@dune73

dune73 Oct 2, 2018

Collaborator

@theMiddleBlue just agreed to that.

This comment has been minimized.

@dune73

dune73 Oct 2, 2018

Collaborator

Well in fact he agreed to remove the capture.
@theMiddleBlue: Please don't forget to also fix the regex.

This comment has been minimized.

@fgsch

fgsch Oct 2, 2018

Collaborator

I believe you still need the capture for TX.0. Is this incorrect?

This comment has been minimized.

@theMiddleBlue

theMiddleBlue Oct 3, 2018

Contributor

Before push all changes, I write it here for your review:

  1. removed capture from actions
  2. replaced tx.0 with MATCHED_VAR, now the logdata action is: logdata:'Matched Data: %{MATCHED_VAR} found within %{MATCHED_VAR_NAME}'
  3. changed the regex to exclude all groups from being captured, now it's: @rx (?i)(?:zlib|glob|phar|ssh2|rar|ogg|expect|zip)://(?:[^/]+|/[^/]+)
  4. add Sam Thomas presentation github link on comments

result:

#
# [ PHP Wrappers ]
#
# PHP comes with many built-in wrappers for various URL-style protocols for use with the filesystem
# functions such as fopen(), copy(), file_exists() and filesize(). Abusing of PHP wrappers like phar://
# could lead to RCE as describled by Sam Thomas at BlackHat USA 2018 (https://bit.ly/2yaKV5X), even
# wrappers like zlib://, glob://, rar://, zip://, etc... could lead to LFI and expect:// to RCE.
#
SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@rx (?i)(?:zlib|glob|phar|ssh2|rar|ogg|expect|zip)://(?:[^/]+|/[^/]+)" \
    "id:933200,\
    phase:2,\
    block,\
    t:none,\
    msg:'PHP Injection Attack: Wrapper scheme detected',\
    logdata:'Matched Data: %{MATCHED_VAR} found within %{MATCHED_VAR_NAME}',\
    tag:'application-multi',\
    tag:'language-php',\
    tag:'platform-multi',\
    tag:'attack-injection-php',\
    tag:'OWASP_CRS/WEB_ATTACK/PHP_INJECTION',\
    tag:'OWASP_TOP_10/A1',\
    ctl:auditLogParts=+E,\
    ver:'OWASP_CRS/3.1.0',\
    severity:'CRITICAL',\
    setvar:'tx.msg=%{rule.msg}',\
    setvar:'tx.php_injection_score=+%{tx.critical_anomaly_score}',\
    setvar:'tx.anomaly_score_pl1=+%{tx.critical_anomaly_score}',\
    setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/PHP_INJECTION-%{MATCHED_VAR_NAME}=%{MATCHED_VAR}'"

Should I change the ver action too? I mean with ver:'OWASP_CRS/3.2.0' or something like?

This comment has been minimized.

@fgsch

fgsch Oct 3, 2018

Collaborator

LGTM.

@dune73 dune73 added the v3.2-dev label Oct 3, 2018

@spartantri

This comment has been minimized.

Collaborator

spartantri commented Oct 3, 2018

Hi, it may be good to add also some transforms to this to avoid common bypasses like t:none,t:utf8toUnicode,t:urlDecodeUni,t:removeNulls,t:cmdLine instead of t:none

@theMiddleBlue

This comment has been minimized.

Contributor

theMiddleBlue commented Oct 3, 2018

thanks @spartantri changed the trasformation functions list as you said, thanks @dune73 and @fgsch for all advice. I'm pushing the new version right now.

@lifeforms lifeforms merged commit 5f01890 into SpiderLabs:v3.2/dev Dec 3, 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