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

SQLi bypass detection: ticks and backticks #1335

Merged
merged 9 commits into from
Jul 16, 2019
Merged

SQLi bypass detection: ticks and backticks #1335

merged 9 commits into from
Jul 16, 2019

Conversation

franbuehler
Copy link
Contributor

This PR resolves issue #1181 by adding a new rule 942510 at PL2 with severity CRITICAL.
Two ticks and backticks are detected.
Please have a look at issue #1181 for more information.

I am not sure if this new rule leads to false positives. I'm thankful for tests.

@emphazer
Copy link
Contributor

emphazer commented Apr 2, 2019

wow, now you give full throttle :-)
thats great!
i will test it for FPs.

@dune73
Copy link
Contributor

dune73 commented Apr 2, 2019

Very welcome PR. Thanks @franbuehler.

Unfortunately, Travis got the hickups:

Sending build context to Docker daemon  8.704kB
Step 1/13 : FROM owasp/modsecurity:v2-ubuntu-apache
manifest for owasp/modsecurity:v2-ubuntu-apache not found

@theMiddleBlue
Copy link
Contributor

awesome! I can't match anything on my logs with that regex, and this sounds good to me :D maybe it could be suitable for PL1?

@franbuehler
Copy link
Contributor Author

Thank you!!
Yes, I know. The tag v2-ubuntu-apache is missing.
It would be cool to move this rule to PL1. We would cover this bypass in the default installation.
I was afraid of false positive. If we don't have FP, I will move this rule to PL1 🎉

@emphazer
Copy link
Contributor

emphazer commented Apr 3, 2019

@franbuehler pls wait with moving to PL1 until i finished my tests tommorow.

@dune73
Copy link
Contributor

dune73 commented Apr 24, 2019

Any news here @emphazer?

@franbuehler
Copy link
Contributor Author

In an English text FP are possible:

Let's do this. Bla bla bla bla bla bla. We'll see....

That's why I added {2.25}.
The minimum text between the ticks must be 2 (if, for example) and a maximum of 25. 25 is a compromise: The lower this number (25), the lower the probability of FP and the higher the probability of false negatives.
@emphazer also added Base64-encoded strings between the ticks to avoid evasions as described in #369.
Thank you for working with me on this rule, @emphazer!

@spartantri
Copy link
Contributor

Hi, php may not need the tailing = to decode properly, why not extracting what looks like base64 and decode it and then do the checks, also this one is pretty good candidate for a false positive nightmare, it matches
'my dog'
'your cat'

@emphazer
Copy link
Contributor

emphazer commented Apr 29, 2019

@spartantri we want to match base64 encoded stuff and don't care about whats inside.
for example 'EihwkakiFuosyONxLKpFVTtihFQTeorKbgWAOoip' or 'PD9waHAgCmVycm9yX3JlcG9ydGluZygwKTtzZXRfdGltZV9saW1pdCgwKTskYT1iYXNlNjRfZGVjb2RlKCJZIi4iWCIuIk4iLiJ6Ii4iWiIuIlgiLiJKIi4iMCIpOyRhKEAkeyJfUCIuIk8iLiJTIi4iVCJ9W3Jvc2VdKTs/Pg=='
stuff like that is not very common in a normal CMS etc...

@franbuehler
could you raise the max number to 29 pls?
in my tests i get the best results here with a minimum of FPs

@dune73
Copy link
Contributor

dune73 commented May 5, 2019

Adding "needs action" label. Either raise as requests, or explain why 25 is OK.

Other than that this look like something we could merge.

@emphazer
Copy link
Contributor

any updates here?

@franbuehler franbuehler self-assigned this Jul 1, 2019
@franbuehler
Copy link
Contributor Author

Finally updated the rule and added a comment.
@emphazer, could you please review again and merge if you think the PR is ok?
Sorry it took so long.

@franbuehler franbuehler removed their assignment Jul 3, 2019
@emphazer
Copy link
Contributor

emphazer commented Jul 4, 2019

@franbuehler i test this PR in production on several server. looks good so far

@franbuehler
Copy link
Contributor Author

Thank you, @emphazer.

@dune73
Copy link
Contributor

dune73 commented Jul 10, 2019

Thank you for the update. Almost ready to be merged. Raising the PL1/PL2 and FP question on slack before an eventual merge.

@lifeforms
Copy link
Contributor

lifeforms commented Jul 10, 2019

If I remember correctly, I actually considered but removed the backticks pattern from my earlier rules on RCE prevention. Back then I was discouraged, as I used a public dataset of Reddit comments, and used that to produce a measure of false positive risk. And Reddit turns out to have quite a few instances of people using backticks in markdown, so I hit many false positives and felt held back to write a rule for it.

That said, I'm willing to see how much of a false positive problem actually occurs in practice. It's probably not so common! And backticks are now not only used in SQLi, code injection, but also in "XSS without parentheses" which is a niche form of XSS that we don't detect well yet.

So I think the upside of detecting them is higher now. And if you have a site that accepts markdown, you will likely need to exclude those markdown endpoints from the CRS entirely anyway because of other false positives (keywords, other meta characters etc.). I'd like to try it at PL1.

@dune73 dune73 merged commit 5200552 into SpiderLabs:v3.2/dev Jul 16, 2019
@dune73
Copy link
Contributor

dune73 commented Jul 16, 2019

And merged at PL1 after waiting for additional comments on slack with regards to desired PL. Thank you for the PR @franbuehler. Most welcome.

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

Successfully merging this pull request may close these issues.

6 participants