8000 Monitoring tools exclusion by theMiddleBlue · Pull Request #1915 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Monitoring tools exclusion #1915

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
Closed
wants to merge 1 commit into from
Closed

Monitoring tools exclusion #1915

wants to merge 1 commit into from

Conversation

theMiddleBlue
Copy link
Contributor

Rule Logic:

A user must uncomment SecAction 900980 from crs-setup.conf to enable the exclusion. The SecAction has 2 variables that users must configure:

  • tx.trusted_ips: a comma-separated list of IPs or classes
  • tx.trusted_ua: a whitespace-separated list of User-Agent (partial match)

The exclusion occurs only for GET and HEAD methods, and it removes the following rules:

  • id: 911100 (allowed methods)
  • id: 913100,913110,913120,913101,913102 (scan detection)
  • id: 920280 (missing/empty host header)
  • id: 920350 (IP address in host header)
  • tag: attack-dos (DoS protection)
  • tag: attack-disclosure (all RESPONSE-*-DATA-LEAKAGES rules)

@airween
Copy link
Contributor
airween commented Oct 26, 2020

Just a quick note: as I see in v3.4/dev branch we use OWASP_CRS/3.3.0 for ver action, not OWASP_CRS/3.4.0.

@lifeforms
Copy link
Member
lifeforms commented Oct 26, 2020 •
edited
Loading

@airween Nice spotted. We have all our ver actions still at 3.3.0. I think it would be a good idea if we change our release procedure, to increase the versions directly after a main branch. Then it's consistent, and it's also useful for debugging (e.g see if a machine is running release version or master branch). Would you agree? Then I'll open a separate PR for that.

@airween
Copy link
Contributor
airween commented Oct 26, 2020

I think it would be a good idea if we change our release procedure, to increase the versions directly after a main branch.

This is how we're doing now, or isn't it?

Then it's consistent, and it's also useful for debugging (e.g see if a machine is running release version or master branch).

I just thought the 3.3.0 would be better here to keep this consistent state.

Would you agree? Then I'll open a separate PR for that.

I agree - but open a PR for what? :)

@lifeforms
8000
Copy link
Member
lifeforms commented Oct 26, 2020 •
edited
Loading

This is how we're doing now, or isn't it?

No, all our ver actions are still at 3.3.0. Right now in the https://github.com/coreruleset/coreruleset/wiki/Release-Procedure we bump all versions before the next release. I think it would be better to bump them after a release. (And also check if they're correct before the next release).

@airween
Copy link
Contributor
airween commented Oct 26, 2020

No, all our ver actions are still at 3.3.0.

Ah, thanks for pointing that. I think we must discuss it at the next monthly chat.

Note, I checked the wiki page above, and found an another interesting part:

Update copyright in all the files if there is a new year

I think we should discuss about another context of copyrights.

@dune73 dune73 mentioned this pull request Nov 2, 2020
Closed
@azurit
Copy link
Member
azurit commented Nov 2, 2020 •
edited
Loading

Here is full UA for Zabbix version 5.0.4 using key web.page.get:
Zabbix 5.0.4

and for Web monitoring feature (don't know why them doesn't match):
Zabbix

@theMiddleBlue
Copy link
Contributor Author

Zabbix 5.0.4

idk if I get it well, the partial match "Zabbix" didn't match "Zabbix 5.0.4"?

(maybe I missed something, but anyone knows what's wrong with the PR test?)

@azurit
Copy link
Member
azurit commented Nov 12, 2020

idk if I get it well, the partial match "Zabbix" didn't match "Zabbix 5.0.4"?

It will match, sorry i confused you. I just wanted to be precise so i pointed out that the same version of Zabbix will use different UA while using different methods of accessing HTTP resource.

@theMiddleBlue
Copy link
Contributor Author

It will match, sorry i confused you. I just wanted to be precise so i pointed out that the same version of Zabbix will use different UA while using different methods of accessing HTTP resource.

ahh I see! Indeed it's a good point, I'm wondering: what if a user wants to allow a specific UA and not match something in it? Should I edit the matching UA rule (901480) replacing pm with rx operator? I mean, by doing it a user could set the tx.trusted_ua with something like .*Nagios.*|Zabbix 5\.0\.4 ?

@azurit
Copy link
Member
azurit commented Nov 12, 2020

ahh I see! Indeed it's a good point, I'm wondering: what if a user wants to allow a specific UA and not match something in it? Should I edit the matching UA rule (901480) replacing pm with rx operator? I mean, by doing it a user could set the tx.trusted_ua with something like .*Nagios.*|Zabbix 5\.0\.4 ?

I would keep the pm because:

  1. Request needs to match also allowed IPs so there's a very little chance of bypassing using UA.
  2. Asking users to write regexpes as part of software configuration seems to be a little hardcore.

@azurit
Copy link
Member
azurit commented Nov 12, 2020

In case you want to allow using very specific UAs, then i suggest creating two methods of specifying UA: pm and also rx (using two different configuration rules).

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

OK. So this has been lingering too long with failing tests.

I'm sorry to announce that both, the ipMatch as well as the pm do not support macros. This means the variable expansion via the %{tx...} is not going to work.

We usually do within instead of pm. So that could work. However, I do not see a solution for ipMatch outside of working with ipMatchFromFile and that looks a bit ugly. Please be aware that the target file of ipMatchFromFile has to exist. Which brings us to the whole "foo.conf.example" problem for an optional feature (the rule is active from the beginning, so the file has to be existing from the beginning too, yet we have to make sure we are not overwriting it).

All in all, I do not see a clear way forward and will close this PR. Mainly because I want to clean up the queue. Feel free to re-submit @theMiddleBlue when you have a solution. Thank you for the nice idea, it would have been so sweet if this would work.

@dune73 dune73 closed this Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theMiddleBlue @airween @lifeforms @azurit @dune73
0