8000 Fix false positives with rule 942360 by Taiki-San · Pull Request #1817 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix false positives with rule 942360 #1817

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 18, 2021
Merged

Fix false positives with rule 942360 #1817

merged 5 commits into from
Jan 18, 2021

Conversation

Taiki-San
Copy link
Contributor

Address issue #1816.
This PR focus on two changes:

  • Expand the fix from Fix FP with create with 942360 #1675 to the patterns suffering from the same issue
  • Make sure the patterns from the query don't start in the middle of another token

@Taiki-San Taiki-San mentioned this pull request Jun 19, 2020
Closed
@Taiki-San
Copy link
Contributor Author

Any idea what's up with the CI?

@theMiddleBlue
Copy link
Contributor

Why we want to not trigger this rule by sending a=(select as www form url encoded request body?

@Taiki-San
Copy link
Contributor Author

It was part of a patterns of false positives and I went too quickly over it, thanks for catching it, I'm removing it and going over the other exemples to make sure I haven't missed anything.
In practice, we want to catch more than just \Wselect as it's fairly common as "action" parameters.
Blurbs of text are also a source of false positive here ("Select this" and then we can do that) but I feel like this is something for another issue and PR.

@fzipi
Copy link
Member
fzipi commented Jun 19, 2020

@Taiki-San This is what I'm seeing in the logs:

| modsec2-apache_1  | AH00526: Syntax error on line 444 of /etc/modsecurity.d/owasp-crs/rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf:
| modsec2-apache_1  | Error creating rule: Error compiling pattern (offset 1255): missing )

@Taiki-San
Copy link
Contributor Author

Thanks for the log @fzipi! That's weird, this look like an escaping issue but I can't see anything new in the commit (a4c9494) where is started to fail. The new part (at the end) is an existing pattern that I duplicated.

@fzipi
Copy link
Member
fzipi commented Jun 19, 2020

Weird. The line doesn't match.... I'm having a second look at it....

@fzipi
Copy link
Member
fzipi commented Jun 19, 2020

Ok, don't look for a problem yet. I'm testing again but surely will need more time for it.

@Taiki-San
Copy link
Contributor Author

No problem, thanks for spending the time!
In the meantime, I'm running this regex on our systems to gather some data. The original regex was too FP prone to be enabled by default but the new one appear to be much better.

@fzipi fzipi mentioned this pull request Jul 4, 2020
Closed
@lifeforms lifeforms self-assigned this Jul 6, 2020
@lifeforms lifeforms added this to the CRS v3.4.0 milestone Jul 6, 2020
@lifeforms
Copy link
Member
lifeforms commented Jul 6, 2020 •
edited
Loading

Émil-Hugo and I will work on this PR in the next month, with the goal to merge it on the next meeting.

Note to self from the Slack chat:

for instance, one might send: "truncate tablename", would that be covered still? (edited)
the pattern was ^[\W\d]+\s*?truncate\b but is now ^[\W\d]+\s*?truncate\s+\w+
I think this problem might only appear with truncate but would have to look at SQL documentation for a bit :)
we could just leave the truncate at the old pattern of course, but I’m willing to do a deeper check on this PR if wanted!

@Taiki-San
Copy link
Contributor Author

My suggestion is to replace the \w+ I introduced by [\"'`\w]+.
This second pattern is already used by [\d\W]\s+as\b\s*[\"'`\w]+\s*\bfrom and cover the few edge cases you brought up where the table name can be in quotes.

@dune73
Copy link
Member
dune73 commented Aug 3, 2020

Hi guys, chat is up tonight. Any progress here?

@dune73 dune73 mentioned this pull request Aug 3, 2020
Closed
@dune73
Copy link
Member
dune73 commented Sep 7, 2020

Ping

@franbuehler franbuehler mentioned this pull request Sep 7, 2020
Closed
@franbuehler franbuehler self-assigned this Sep 7, 2020
@franbuehler
Copy link
Contributor

Monthly chat meeting September: @franbuehler volounteers to help here.
#1869 (comment)

@dune73 dune73 mentioned this pull request Oct 5, 2020
Closed
@Taiki-San
Copy link
Contributor Author

Ran a few tests on the rule with a wide variety of production trafic, it's now much less sensitive to free text blob, while still catching many malicious queries.
If we're okay with this lower sensitivity, I'll remove the tests we're no longer catching.

@dune73 dune73 mentioned this pull request Nov 2, 2020
Closed
@franbuehler franbuehler mentioned this pull request Dec 5, 2020
Closed
@dune73
Copy link
Member
dune73 commented Dec 7, 2020

Hey @Taiki-San. Thank you for your work on this issue. This has been lingering much too long.

Am I getting you right, that we have less FPs now with the updated pattern, but we also miss a few attacks? So we traded false positives for false negatives? (This is not necessarily bad, I just want to be sure we have it black on white).

Also would you mind rebasing this to v3.4 (And sorry taking so long for the review).

@Taiki-San Taiki-San force-pushed the false_positives/942360 branch from 3adcdf6 to 051dcb0 Compare December 8, 2020 09:38
@Taiki-San Taiki-San changed the base branch from v3.3/dev to v3.4/dev December 8, 2020 09:41
@Taiki-San
Copy link
Contributor Author

Hey @dune73, no problem, hadn't had much time to push it harder.

I think you got it. I suspect if we end up deciding that those triggers were worthy, a forked rule could be introduced in >= PL2

@dune73
Copy link
Member
dune73 commented Dec 8, 2020

I think what we would be doing in this case would be to push the existing 942360 to PL2 as 942361 (stricter sibling) and take your updated version as the new 942360.

Could you draw up a brief list of the patterns your version still detects and which ones it does not?

@Taiki-San
Copy link
Contributor Author

Sounds great!
I'll probably have the time to work on the split tomorrow, any special consideration?
If so, I can dump those patterns here, as you prefer.

@dune73
Copy link
Member
dune73 commented Dec 9, 2020

Thank you for your support.

But I would prefer to talk about the patterns here first. In order to get an overview before you spring to action with all the splitting work.

@Taiki-San
Copy link
Contributor Author

Sure, here are a few samples:

  1. 1 union select 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,CONCAT('vbulletin','rce',@@version)...
  2. (SELECT 4440 FROM(SELECT COUNT(*),CONCAT(0x716b627a71,(SELECT (ELT(4440=4440,1))),0x7170716271,FLOOR...
  3. 2759399466.1534185336 -6863 union all select 1,1,1,1,1,1,1,1,1,CONCAT
  4. ���) UNION ALL SELECT NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL--
  5. (CONVERT(INT,(SELECT CHAR(113)+CHAR(118)+CHAR(112)+CHAR(113)+CHAR(113)+(SELECT (CASE WHEN (6557=6557
  6. -1 UNION SELECT null,123456,null,null,null,null--
  7. x"; SELECT LOAD_FILE('\
  8. 2020-03-01 UNION ALL SELECT CONCAT

Triggers we no longer detect:

  1. ||(SELECT(DBMS_LDAP.INIT('169.1.1.1',19))FROM(DUAL))/investigate
  2. '||(select(pg_sleep(15))where(true))||'/investigate
  3. UNION ALL SELECT NULL,NULL,CONCAT(CONCAT('qqkjq','mxTSrPILRz'),'qvxvq')-- sqCV

@dune73 dune73 mentioned this pull request Dec 11, 2020
Closed
@Taiki-San Taiki-San force-pushed the false_positives/942360 branch from 051dcb0 to cf0bdfa Compare December 14, 2020 15:56
@Taiki-San
Copy link
Contributor Author

Renumbered the original rule to 942362 (942361 was already taken)
Removed the following tests from 942360 (still here for 942362)

942360-6
942360-7
942360-8
942360-10
942360-12
942360-14
942360-16
942360-17
942360-18
942360-19
942360-20
942360-22
942360-23
942360-25
942360-26
942360-29

franbuehler
franbuehler reviewed Dec 14, 2020
View reviewed changes
Copy link
Contributor
@franbuehler franbuehler left a comment

Choose a reason for hiding this comment

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

This PR looks good to me from what I can understand.
I regenerated the regexes and ran the tests (I know CI runs them too :) ).
I found one small error with the PL tag, see below. And asked one question. See below.
I see and understand @Taiki-San explanations what we are introducing with this change. Less FP, but also possibly more false negatives at PL1...
I cannot fully assess the consequences because I'm not an SQLi pro. But as far as I can tell, this PR is clean work and looks good to me.

util/regexp-assemble/regexp-942362.data
^[\W\d]+\s*?select\b
^[\W\d]+\s*?truncate\b
^[\W\d]+\s*?update\b
^[\W\d]+\s*?alter\s*aggregate\b
Copy link
Contributor
@franbuehler franbuehler Dec 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we duplicate these regexes from here downwards, I mean these alter..., into the rule 942362?
And also line 32 (load_file), line 33 (regexp) and line 35 (create) while they don't change.
Don't we just want to duplicate the regexes into 942362 that get stricter?
Just asking, I'm not sure...

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 strong preferences here, I just kept the old 942360 as is in order to keep the review simple(r).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Personally, I wouldn't duplicate them. But also no strong preferences here. Other opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original suggestion to duplicate the old rule came from @dune73:
#1817 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Initially I thought this would play out better with strict siblings, but in fact it does not really bring any advantages here. On top the lower PL rule is executed anyway. So I think it can be skipped.

But maybe we can have @lifeforms opinion as well. I would like to make sure we get this right.

@dune73
Copy link
Member
dune73 commented Jan 18, 2021

Sorry, but I overlooked your PR in December. So this PR should not have had the label "needs work" anymore and it ought to have been discussed on Jan 4. Given reviewer @franbuehler deemed it ready for merging when said PL fix is done, I think we can merge this one.

Thank you for your welcome contribution @Taiki-San.

@dune73 dune73 merged commit 05ba7a7 into coreruleset:v3.4/dev Jan 18, 2021
@Taiki-San Taiki-San deleted the false_positives/942360 branch January 18, 2021 19:32
@dune73 dune73 mentioned this pull request Jan 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dune73 dune73 dune73 left review comments

@franbuehler franbuehler franbuehler left review comments

Assignees

@lifeforms lifeforms

@franbuehler franbuehler

Labels
None yet
Projects
None yet
Milestone
CRS v3.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Taiki-San @theMiddleBlue @fzipi @lifeforms @dune73 @franbuehler
0