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

932140 (Remote Command Execution: Windows FOR/IF Command Found) FP on SAML data #671

Closed
jhaar opened this issue Dec 12, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@jhaar
Copy link

commented Dec 12, 2016

Hi there

I'm running SimpleSAMLPhp as a SAML IdP behind mod_security-2.9.1 with the OWASP_CRS/3.0.0 ruleset and 932140 triggers on some SAML transactions

SAML involves large digitally signed PEM encoded blobs flying through via POST and I think the simple matching on 2chars really makes this rule likely to FP

Attached is an alert showing that rule hit some base64 encoded chars "if3q"

saml-fp.txt

@csanders-git

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2016

Hey @jhaar this is an interesting issue. SAML is typically presented as XML although this doesn't look like that. Would it be possible to get the auditlog result?

@jhaar

This comment has been minimized.

Copy link
Author

commented Dec 13, 2016

here you go

modsec_audit.txt

@lifeforms

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2016

Wow, that is a crazy false positive. Seems like we should be able to nip this in the bud with the information you gave. Thanks a lot for submitting it!

Are you okay for now, or would you like an example to exclude this rule for your app?

@jhaar

This comment has been minimized.

Copy link
Author

commented Jan 3, 2017

nah - all good. BTW I've got another one - I've put that in separately - #675

@jhaar jhaar closed this Jan 3, 2017

@dune73

This comment has been minimized.

Copy link
Collaborator

commented Jan 5, 2017

Let's keep this open until we have really fixed it.

@dune73 dune73 reopened this Jan 5, 2017

@dune73

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2017

So what do we do with this and with #675? I get the feeling, these issues are part of a bigger group of false positives that involve 1-2 dozens of rules, some of them PL1, that may trigger on (seemingly) random data.

Seemingly random data exists in the form of Base64 encoded payloads, client-side encryption of request parameters, etc.

I think we need to solve this at least for PL1. Whatever solving really means. With FOR/IF it looks really tough.

@lifeforms lifeforms added this to the CRS v3.0.1 milestone Apr 3, 2017

@lifeforms

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2017

Going to take a look at this next week.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2017

A fix is in #738 and will be included in CRS 3.0.1 maintenance release. Thanks for submitting the logs!

@lifeforms lifeforms closed this Apr 23, 2017

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.