Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

REQUEST-944-APPLICATION-ATTACK-JAVA.conf #990

Merged
merged 43 commits into from
Mar 5, 2018
Merged

REQUEST-944-APPLICATION-ATTACK-JAVA.conf #990

merged 43 commits into from
Mar 5, 2018

Conversation

spartantri
Copy link
Contributor

Java attacks updated config

spartantri and others added 4 commits November 27, 2017 11:19
The rule will ignore non all upper case header names, which I don't think was the idea behind this rule.
3.2.  Header Fields
Each header field consists of a case-insensitive field name followed by a colon (":"), optional leading whitespace, the field value, and optional trailing whitespace.
@emphazer
Copy link
Contributor

@spartantri looks really hot! :-)
One question, does the REQUEST_BODY processor include the REQUEST_HEADERS?
Like Accept-Encoding for example.

@fzipi
Copy link
Contributor

fzipi commented Dec 29, 2017

@spartantri Sorry to tell you this, but it doesn't follow the contributing guidelines...

@spartantri
Copy link
Contributor Author

@fzipi changed the rules to match the actions order, @emphazer added the REQUEST_HEADERS as it is not part of REQUEST_BODY, and also added an additional PL4 rule including the different b64 encoded strings of interesting keywords

@emphazer
Copy link
Contributor

@spartantri great! Thats what we need with REQUEST_HEADERS! Thanks

@@ -0,0 +1,290 @@
# ------------------------------------------------------------------------
# OWASP ModSecurity Core Rule Set ver.3.1.0
# Copyright (c) 2006-2016 Trustwave and contributors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be 2018 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know but changed anyway

setvar:tx.anomaly_score=+%{tx.notice_anomaly_score},\
setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RCE-%{matched_var_name}=%{tx.0}'"

SecRule ARGS|ARGS_NAMES|REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|REQUEST_BODY|REQUEST_HEADERS|XML|XML:/* \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this normal indentation?

ver:'OWASP_CRS/3.1.0',\
severity:'CRITICAL',\
chain"
SecRule MATCHED_VARS "@rx (?:unmarshaller|base64data|java\.)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

This 'SecRule' should be aligned with the previous 'chain'

ver:'OWASP_CRS/3.1.0',\
severity:'NOTICE',\
chain"
SecRule MATCHED_VARS "@rx (?:runtime|processbuilder|clonetransformer|forclosure|instantiatefactory|instantiatetransformer|invokertransformer|prototypeclonefactory|prototypeserializationfactory|whileclosure)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

This 'SecRule' should be aligned with the previous 'chain'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, missed that one

ver:'OWASP_CRS/3.1.0',\
severity:'CRITICAL',\
chain"
SecRule MATCHED_VARS "@rx (?:runtime|processbuilder)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed all those, sorry about that, it is my personal identation style to distinguish short chains, anyway, removed all those extra 4 spaces to allign to contributing.md

@spartantri
Copy link
Contributor Author

mmm I don't know why File upload rules get mixed with this so I will move that file off to finish this PR and once done move it back to create a new PR

Copy link
Contributor Author

@spartantri spartantri left a comment

Choose a reason for hiding this comment

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

Updated sugested fixes

setvar:'tx.msg=%{rule.msg}',\
setvar:tx.rce_score=+%{tx.notice_anomaly_score},\
setvar:tx.anomaly_score=+%{tx.notice_anomaly_score},\
setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RCE-%{matched_var_name}=%{matched_var}'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how common this may be for legitimate purposes so let's start with 3 and lower it once we have some feedback.

ver:'OWASP_CRS/3.1.0',\
severity:'NOTICE',\
chain"
SecRule MATCHED_VARS "@rx (?:runtime|processbuilder|clonetransformer|forclosure|instantiatefactory|instantiatetransformer|invokertransformer|prototypeclonefactory|prototypeserializationfactory|whileclosure)" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, missed that one

setvar:'tx.msg=%{rule.msg}',\
setvar:tx.rce_score=+%{tx.notice_anomaly_score},\
setvar:tx.anomaly_score=+%{tx.notice_anomaly_score},\
setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RCE-%{matched_var_name}=%{matched_var}'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no there is no chain in this one is intended to complain on most common keywords to spawn a process

setvar:'tx.msg=%{rule.msg}',\
setvar:tx.rce_score=+%{tx.notice_anomaly_score},\
setvar:tx.anomaly_score=+%{tx.notice_anomaly_score},\
setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RCE-%{matched_var_name}=%{matched_var}'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the message to class lets add this into a new rule, maybe a new PR if required

setvar:'tx.msg=%{rule.msg}',\
setvar:tx.rce_score=+%{tx.notice_anomaly_score},\
setvar:tx.anomaly_score=+%{tx.notice_anomaly_score},\
setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/RCE-%{matched_var_name}=%{matched_var}'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to 944230 and class list into java-classes.data

@lifeforms
Copy link
Contributor

Confirming that 2 false negatives are now gone!

Only this one is left, I don't know why it is not detecting:

http://localhost/?redirect:$%7B%23matt%3d%20%23context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse'),%23matt.setContentType('text/plain'),%23matt.getWriter().println%20('successsuccess'),%23matt.getWriter().flush(),%23matt.getWriter().close()%7D

tag:'paranoia-level/1',\
ctl:forceRequestBodyVariable=On,\
rev:'1',\
ver:'OWASP_CRS/3.1.0'"
Copy link
Contributor

Choose a reason for hiding this comment

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

We leave this in for now and create a separate issue on how to handle this generally in setup/initialization file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lifeforms There are no false negative on my tests but some of the rules were on pass instead of block for testing, changed all back to block, please test again

http://localhost/?redirect:$%7B%23matt%3d%20%23context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse'),%23matt.setContentType('text/plain'),%23matt.getWriter().println%20('successsuccess'),%23matt.getWriter().flush(),%23matt.getWriter().close()%7D

[Tue Feb 06 19:04:15.778459 2018] [:error] [pid 13977] [client 127.0.0.1] ModSecurity: Warning. Matched phrase "com.opensymphony.xwork2" at ARGS_NAMES:redirect:${#matt= #context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse'),#matt.setContentType('text/plain'),#matt.getWriter().println ('successsuccess'),#matt.getWriter().flush(),#matt.getWriter().close()}. [file "/home/spartan/owasp-modsecurity-crs/rules/REQUEST-944-APPLICATION-ATTACK-JAVA.conf"] [line "160"] [id "944230"] [rev "1"] [msg "Suspicious Java class detected"] [data "Matched Data: redirect:${#matt= #context.get('com.opensymphony.xwork2.dispatcher.httpservletresponse'),#matt.setcontenttype('text/plain'),#matt.getwriter().println ('successsuccess'),#matt.getwriter().flush(),#matt.getwriter().close()} found within ARGS_NAMES:redirect:${#matt= #context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse'),#matt.setContentType('text/plain'),#matt.getWriter().println ('successsuccess'),#matt.getWriter().flush(),#matt.getWriter().close()}"] [severity "NOTICE"] [ver "OWASP_CRS/3.1.0"] [tag "application-multi"] [tag "l [hostname "localhost"] [uri "/"] [unique_id "WnnuH38AAQEAADaZIF4AAAAK"]

@lifeforms lifeforms self-assigned this Feb 5, 2018
@lifeforms
Copy link
Contributor

Decision: I will merge this after a final test run, Friday at the latest!

@spartantri
Copy link
Contributor Author

spartantri commented Feb 6, 2018

@lifeforms Some rules including pmf were in pass, changed back to block, please test again

@lifeforms
Copy link
Contributor

Thanks for the updates...

I now tried out the PR on a clean checkout, so I was using paranoia level 1, and I notice that there are no rules active in this level... Is that intended? Lol, due to this I've wasted a lot of time redoing my install... Hence my review is a bit shorter than I'd liked.

  1. Rule 944230 has a NOTICE severity but a critical score. I think the severity should be changed to CRITICAL.

  2. Personally, I think that most of the detections would be completely fine at paranoia level 1. It is what I did for most PHP rules having signatures which don't appear regularly in normal text — And the PHP rules rarely give users false positives problems now. I think it will be the same for the Java rules. It's a judgement call. But I would recommend to move all your PL2 rules to PL1 and make them active by default. You don't have to be shy, we have had much much worse rules in the default install ;)

  3. I also still feel that rule 944300 could be in PL1 (Java serialized object magic bytes detection, now PL3). It's been a common vulnerability and if an application wants to use this (discouraged) practice, manual user whitelisting would be advised. It looks that we disagree on the PL though, so do other people have insights/opinions?

  4. Some rules have severity NOTICE. This is pretty rare in the CRS, and I personally don't think that NOTICE rules are highly useful (I just want to block definitive exploits, and I want as little log noise as possible, also I want as little code as possible since maintenance all adds up). I would probably leave them out myself. But if you really love them and would feel they are useful in practice, then I definitely won't mind.

@dune73
Copy link
Contributor

dune73 commented Feb 9, 2018

I've stayed away from this discussion, but let me chime in on @lifeform's questions:

3: I do not think any special treatment with regards to PL is due. If FPs are nearly non-existent, then it should be PL1, if there are some, then it's a PL2, if they are frequent, then a PL3.

4: I'm generally all for CRITICAL in almost all cases.

@spartantri
Copy link
Contributor Author

well then I lowered the PLs and changed the severity and scores to critical, it should be ready now

@lifeforms
Copy link
Contributor

lifeforms commented Feb 18, 2018

My tests are succeeding!

I did notice though that you put the deserialization magic bytes rule (944200) in PL2. Did you find false positives on this one? If so, PL2 is the best place for it, but it seems so rare that I think PL1 would be possible. That said, I have never run many Java applications (and especially no enterprise ones) so I'm surely not qualified.

Apart from this final point, I have only love to spread. It works really nicely whatever kind of exploit I know that I throw at it :)

@spartantri
Copy link
Contributor Author

Hi @lifeforms, I let the 944200 at PL2 due that 4 bytes is not so long to avoid false positives. What can be done later is add an additional rule a bit more specific at PL1 to avoid false positive but yet grab serialized java objects.

@spartantri spartantri mentioned this pull request Mar 1, 2018
12 tasks
@lifeforms lifeforms merged commit a1cba01 into SpiderLabs:v3.1/dev Mar 5, 2018
@lifeforms
Copy link
Contributor

MERGED 💃
Thanks so much for this awesome work!!!

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

Successfully merging this pull request may close these issues.

5 participants