Owasp-modsecurity-crs: FP on 941120 (XSS Filter - Category 2: Event Handler Vector)

[jhaar]

Hi there

We run SimpleSAMLphp and SAML's use of BASE64 encoding large chunks of data in POSTs (and GETs) seems to trigger a fair amount of FPs - which completely BREAKS the application...

I put in #671 which shows the OWASP_CRS/3.0.0 ruleset 932140 catching an "if" inside a blob of BASE64 data - this one is the same - this time for "on". [Edit by @lifeforms: This FP concerns rule 941120.]

Attached is an alert showing that rule hit

modsec.txt

[dune73]

Nice find. Actually, I stumbled over this yesterday as well.
I have reopened #671 again and am assigning the usual tags to this one. Hope to fix this for 3.0.1.

[dune73]

Thank you for taking the time to report. It's much appreciated.

[vishaalgc]

@dune73 @jhaar facing the same issue, Is there any fix for it yet ?

[dune73]

Not officially. If you want a quick fix, to a simple but total rule exclusion:

SecRuleRemoveById 941120

If you want a more subtle approach, look up better techniques in the tutorials on https://netnea.com.

[franbuehler]

What do you think about explicitly listing the event handlers?
It's a finite number of commands:

onabort|onblur|onchange|onclick|onclose|oncontextmenu|ondblclick|onerror|onfocus|oninput|
onkeydown|onkeypress|onkeyup|onload|onmousedown|onmousemove|onmouseout|onmouseover|
onmouseup|onpointercancel|onpointerdown|onpointerenter|onpointerleave|
onpointermove|onpointerout|onpointerover|onpointerup|onreset|onresize|onscroll|
onselect|onselectstart|onsubmit|ontouchcancel|ontouchmove|ontouchstart

Or the same after running through a regexp optimizer:
on(?:pointer(?:(?:lea|mo)ve|o(?:ver|ut)|cancel|enter|down|up)|s(?:(?:elec(?:tstar)?|ubmi)t|croll)|c(?:l(?:ick|ose)|ontextmenu|hange)|mouse(?:o(?:ver|ut)|down|move|up)|touch(?:cancel|start|move)|key(?:press|down|up)|(?:abor|inpu)t|(?:erro|blu)r|res(?:ize|et)|dblclick|focus|load)

[dune73]

So the idea would be to replace the on\w+ in the regex with the pattern you suggest, @franbuehler?

[franbuehler]

Yes:

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|
REQUEST_HEADERS:User-Agent|REQUEST_HEADERS:Referer|ARGS_NAMES|ARGS|XML:/* 
"(?i)([\s\"'`;\/0-9\=\x0B\x09\x0C\x3B\x2C\x28\x3B]+on(?:pointer(?:(?:lea|mo)ve|
o(?:ver|ut)|cancel|enter|down|up)|s(?:(?:elec(?:tstar)?|ubmi)t|croll)|
c(?:l(?:ick|ose)|ontextmenu|hange)|mouse(?:o(?:ver|ut)|down|move|up)|
touch(?:cancel|start|move)|key(?:press|down|up)|(?:abor|inpu)t|
(?:erro|blu)r|res(?:ize|et)|dblclick|focus|load)[\s\x0B\x09\x0C\x3B\x2C\x28\x3B]*?=)" \
        "msg:'XSS Filter - Category 2: Event Handler Vector',\
        id:941120,\
        ...

[dune73]

Looks good.

@jhaar and @vishaalgc: Could you give this a shot and test it?

@franbuehler: When doing a PR eventually, I suggest you also update the documentation with the full list of events you are covering the source of the list.

[jhaar]

The new rule made the problem go away for me.

[lifeforms]

For 3.0.1 we revert to just checking letters e.g. on[a-zA-Z]+, which will most likely suppress all the base64 related FPs for this rule.

If FP still remains a problem we can use a blacklist and move the original onxxx rule to PL2.

[lifeforms]

@franbuehler is going to take a stab at a PR for this. :)

[dune73]

Closing this after fix in #729 has been merged.