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

Rework Pullrequest #122, avoid xss false positives starting with 'on.*' #143

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

blappm
Copy link

@blappm blappm commented Nov 16, 2018

It is safer to use a list of event handlers than just matching strings > 5 chars

@lifeforms
Copy link

As a CRS maintainer, I agree that a fix for this problem would be very interesting. Our users regularly turn up false positives due to generic onfoo= matching. Some examples:

SpiderLabs/owasp-modsecurity-crs#820
SpiderLabs/owasp-modsecurity-crs#967

A discrete blacklist would solve this problem, although it may require more regular maintenance as new event handlers are added.

@blappm
Copy link
Author

blappm commented Nov 16, 2018

Looks like there were some eventhandlers missing. Adding them now.

@lifeforms
Copy link

build passed 🎉

@blappm
Copy link
Author

blappm commented Nov 16, 2018

Well :-) It's now sorted alphabetically. This makes it easier to add new event handlers.

@blappm
Copy link
Author

blappm commented Nov 19, 2018

We are now successfully using this patch in production. While we were seeing 20-30 FP per day before, the rate has now dropped to 1-2 per day.

One of the worst FP caused by this was 'online'.

@fgsch
Copy link

fgsch commented Jan 3, 2019

Is there anything holding this PR? It'd be great if it's merged.

@frankyhun
Copy link

@client9 is this project abandoned?

@zimmerle
Copy link
Contributor

@client9 is this project abandoned?

You may want to look here: libinjection/libinjection#7 we are giving o followup on that discussion there.

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

Successfully merging this pull request may close these issues.

5 participants