8000 Nextcloud 20 false-positives by keachi · Pull Request #1975 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Nextcloud 20 false-positives #1975

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 8 commits into from
Closed

Nextcloud 20 false-positives #1975

wants to merge 8 commits into from

Conversation

keachi
Copy link
@keachi keachi commented Jan 9, 2021 •
edited
Loading

With Nextcloud 20.0.4 there are multiple false-positives. This PR will fix some of the false-positives.

The following setup is used to reproduce the false-positives:

  • Nginx v1.14.1
  • ModSecurity v3.0.4
  • CRS v3.3.0 with nextcloud exclusion v.3.4
  • Paranoia level setting 3
  • Operating System: CentOS 8.3.2011
  • Backend: Nextcloud 20.0.4 Container

Fixes:

Nextcloud Apps:

  • PDF viewer (default)
  • Theming (default)
  • Text (default)
  • User status (default)
  • First run wizard (default)
  • Two-Factor U2F
  • Two-Factor Webauthn

keachi added 7 commits January 10, 2021 11:31
@keachi
Allow URI with and without index.php
267d49e 
The official nextcloud docker container doesn't contain index.php in the
URI. Adapt patterns to allow those URIs too.
@keachi
Fix Personal info website set
13c3907 
Allow to set a url into the Personal info website field.
@keachi
Allow Nextcloud theming
8acfed9
@keachi
DAV uses chars below ascii 127
c817e8e
@keachi
Allow URI search/providers
37266e0
@keachi
Allow to edit source code with text app
1a1df9d
@keachi
Fix false-positives for nextcloud pdfviewer
5f93d62
@dune73
Copy link
Member
dune73 commented Jan 18, 2021

Wow, that's an impressive contribution. Thank you very much.

You are also rewriting several of the existing rule exclusions. Could you explain why you are doing that and is this rewriting of existing rules essential to your PR?

@keachi
Copy link
Author
keachi commented Jan 18, 2021

I rewrite those rules, because with the official container, which implements the pretty url configuration, the index.php isn't part of the URL. My rewrite should only modify the existing rules in that way. This is part of the #1973, see this comment.

@dune73 dune73 mentioned this pull request Feb 1, 2021
Closed
@azurit azurit self-assigned this Feb 3, 2021
@Magicrafter13
Copy link

Any chance this gets accepted in next release, or is it waiting on a more extensive review process of some kind?

@dune73 dune73 mentioned this pull request Mar 1, 2021
Closed
@dune73
Copy link
Member
dune73 commented Mar 1, 2021

This is very likely accepted into the next release and it is now waiting for a review that was self-assigned to @azurit.

Maybe we get an update if we ping him. :)

@dune73 dune73 mentioned this pull request Apr 2, 2021
Closed
@dune73
Copy link
Member
dune73 commented Apr 2, 2021

@azurit: Any news here? We'd like to merge this. So where is your review standing?

@franbuehler
Copy link
Contributor

Meeting decision April 5th (#2051 (comment)): Keep it assigned to @azurit and @dune73 tries to get in touch with @emphazer.

@dune73 dune73 mentioned this pull request Apr 16, 2021
Closed
@dune73
Copy link
Member
dune73 commented Apr 16, 2021

Status on the PR: We have a hard time with the review as we do not have an active CRS developer running NextCloud / OwnCloud with enough time at hand to this this.

@Magicrafter13
Copy link

So, with the current ruleset for Nextcloud, it doesn't work, period.
Which means the only solution, for users running NextCloud, is to not use the Nextcloud ruleset.
Is any harm done by merging this? (I'm working under the assumption that everything is isolated and that this would have no effect on anything that isn't Nextcloud / OwnCloud.)

@keachi
Copy link
Author
keachi commented Apr 18, 2021

@Magicrafter13 that depends on your setup. There are setups where the CRS works (if you don't use pretty urls) and this MR will mostly just fix this behaviour. It will not affect other platforms than Nextcloud / Owncloud (if the flag isn't set).
Depending on this apps you've installed you have to write your own exclude rules too.
But yes, this MR will fix a lot of false-positives if you use pretty urls and with pretty urls the current ruleset (3.3.0) will not work.

@dune73
Copy link
Member
dune73 commented Apr 19, 2021

@Magicrafter13 : Part of the review is to prove your assumption is correct. Other than that a review also makes sure the whole thing fits into our policy, follows our conventions more or less. I totally see your concerns of course and there is a real need for this to be merged. But I have a gatekeeping role here and there are other PRs (and issues) we are looking at in parallel. So it's unfortunate, our go to reviewer does not have time for it.

Now with that being said: Have you tried out the PR @Magicrafter13? Since if it works for you and for @keachi, then I could do a visual review of the rules, provide some feedback if any, @keachi fixes it and we're ready to fly.

@dune73 dune73 mentioned this pull request May 3, 2021
Closed
@franbuehler
Copy link
Contributor

Meeting decision May (#2053 (comment)):
We have to wait until we have tests and Nextcloud knowhow. See @dune73's comment above too.

This was linked to issues May 10, 2021
Nextcloud 20 App user_status #1974
Closed
Nextcloud 20 App firstrunwizard #1973
Closed
@franbuehler franbuehler mentioned this pull request May 10, 2021
Closed
@Magicrafter13
Copy link
Magicrafter13 commented May 11, 2021 •
edited
Loading

@Magicrafter13 : Part of the review is to prove your assumption is correct. Other than that a review also makes sure the whole thing fits into our policy, follows our conventions more or less. I totally see your concerns of course and there is a real need for this to be merged. But I have a gatekeeping role here and there are other PRs (and issues) we are looking at in parallel. So it's unfortunate, our go to reviewer does not have time for it.

Now with that being said: Have you tried out the PR @Magicrafter13? Since if it works for you and for @keachi, then I could do a visual review of the rules, provide some feedback if any, @keachi fixes it and we're ready to fly.

I tried it, and the keachi nextcloud_20 branch doesn't seem to work for me, but perhaps I have another issue contributing? The modsec audit log is rather abstract, but I think this is relevant:

---jyxs2mnI---H--
ModSecurity: Warning. Matched "Operator `Rx' with parameter `^0?$' against variable `REQUEST_HEADERS:content-length' (Value: `19' ) [file "/usr/local/coreruleset-nc/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf"] [line "161"] [id "920170"] [rev ""] [msg "GET or HEAD Request with Body Content"] [data "19"] [severity "2"] [ver "OWASP_CRS/3.3.0"] [maturity "0"] [accuracy "0"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-protocol"] [tag "paranoia-level/1"] [tag "OWASP_CRS"] [tag "capec/1000/210/272"] [hostname "my server's LAN address?"] [uri "/nextcloud/apps/user_status/heartbeat"] [unique_id "1620712135"] [ref "o0,3v0,3v508,2"]
ModSecurity: Access denied with code 200 (phase 2). Matched "Operator `Ge' with parameter `5' against variable `TX:ANOMALY_SCORE' (Value: `5' ) [file "/usr/local/coreruleset-nc/rules/REQUEST-949-BLOCKING-EVALUATION.conf"] [line "138"] [id "949110"] [rev ""] [msg "Inbound Anomaly Score Exceeded (Total Score: 5)"] [data ""] [severity "2"] [ver "OWASP_CRS/3.3.0"] [maturity "0"] [accuracy "0"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-generic"] [hostname "my server's LAN address?"] [uri "/nextcloud/apps/user_status/heartbeat"] [unique_id "1620712135"] [ref ""]

If there's anything else I can look into to help, please let me know. There's a lot of moving parts here...

Oh yeah, for the record, a lot of Nextcloud does work, but like, my to-do list and calendar for example, those don't load. Also my files and contacts.

@dune73 dune73 mentioned this pull request Oct 4, 2021
Closed
@dune73 dune73 mentioned this pull request Nov 1, 2021
Closed
@fzipi fzipi mentioned this pull request Dec 4, 2021
Closed
@fzipi fzipi mentioned this pull request Jan 2, 2022
Closed
@fzipi fzipi mentioned this pull request Feb 7, 2022
Closed
@fzipi fzipi mentioned this pull request Mar 6, 2022
Closed
@geppi
Copy link
geppi commented Apr 1, 2022

I'm just wondering if the regex in rule 9003130 of this pull request isn't a little generic?

"@rx /(?:remote\.php/|index\.php/|public\.php/)?" is IMHO equivalent to "@rx /" and will therefore catch every REQUEST_FILENAME.

This does at least render rule 9003140 obsolete, right?.

It also seems for me equivalent to allowing all these HTTP methods right from the beginning and could be implemented by a SecAction as proposed in crs-setup.conf.example action id:900200, which would hit performance less than a rule, right?

@geppi
Copy link
geppi commented Apr 1, 2022

There are also several regex in this pull request of the form "@rx /(?:index\.php)?/some/path" which substitute a @contains operator. It should be possible to keep the @contains operator in most cases and just remove the /index.php part from the match string, which would be preferable performance wise, right?

@fzipi fzipi mentioned this pull request Apr 4, 2022
Closed
@dune73 dune73 mentioned this pull request May 2, 2022
Closed
@fzipi fzipi mentioned this pull request Jun 5, 2022
Closed
@theseion theseion marked this pull request as draft June 8, 2022 18:20
@dune73 dune73 mentioned this pull request Jul 4, 2022
Closed
@fzipi fzipi mentioned this pull request Aug 1, 2022
Closed
@fzipi fzipi mentioned this pull request Sep 5, 2022
Closed
@fzipi fzipi mentioned this pull request Oct 2, 2022
Closed
@dune73
Copy link
Member
dune73 commented Apr 17, 2023

@azurit and @Xhoenix : Is this something you could address given your activities on the nextcloud rule exclusion plugin?

@Xhoenix
Copy link
Member
Xhoenix commented Apr 17, 2023 •
edited
Loading

I've already submitted fixes for all of the above mentioned except the Webauthn FP which I'm using in my installation but somehow forgot to add. The fixes were already merged.

I'm using pretty URLs, which I think is almost every NextCloud installation uses now-a-days.

@dune73
Copy link
Member
dune73 commented Apr 17, 2023

That's very good news. So if you address Webauthn we can then close this PR?

@Xhoenix
Copy link
Member
Xhoenix commented Apr 17, 2023

Yeah, will submit the fix tomorrow.

@Xhoenix
Copy link
Member
Xhoenix commented Apr 17, 2023

I just checked and there are several FPs for the Webauthn part and some of them are at PL2. I think I'll have to do some retesting tomorrow before opening a PR.

@Xhoenix
Copy link
Member
Xhoenix commented Apr 18, 2023

Actually, the WebAuthn implemented by this PR is based upon an extension which does not ship by default with Nextcloud. The WebAuthn I was talking about is quite different from this one.

@azurit
Copy link
Member
azurit commented Apr 19, 2023

@Xhoenix Ok, so, is everything fixed except that 3rd party plugin? Can this be closed?

@prghix
Copy link
prghix commented Apr 19, 2023

btw: @Xhoenix which Nextcloud version do you have?

I test against NC26.. which is the latest one.

@prghix
Copy link
prghix commented Apr 19, 2023

Got a ton of FPs from "Text" application.

@azurit
Copy link
Member
azurit commented Apr 19, 2023

@prghix Are you using newest version of Netxcloud plugin?

@Xhoenix
Copy link
Member
Xhoenix commented Apr 19, 2023 •
edited
Loading

Yes @azurit, this can be closed. Also, I just looked into the what @prghix is saying and found that there is still a few problems with the Nextcloud Plugin, more specifically the phase values for some rules need to be revised. Also, about the "Text" application @prghix is suggesting, it's about the "autosave" issue I created earlier. Fixing that will make the Nextcloud plugin completely PL 1 ready.

@azurit
Copy link
Member
azurit commented Apr 19, 2023

Thanks, closing!

@azurit azurit closed this Apr 19, 2023
@keachi keachi deleted the nextcloud_20 branch April 24, 2023 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@azurit azurit

Labels
👀 Needs action 🙏 help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nextcloud 20 App user_status Nextcloud 20 App firstrunwizard
8 participants
@keachi @dune73 @Magicrafter13 @franbuehler @geppi @Xhoenix @azurit @prghix
0