coreruleset / nextcloud-rule-exclusions-plugin

Rule exclusion plugin for Nextcloud
Apache License 2.0
11 stars 7 forks source link

Rule 911100 triggered as a false positive when using ios app and dismissing a notification #87

Closed jessebot closed 1 month ago

jessebot commented 1 month ago

Hi friends!

This one might be a little more difficult to reproduce unless you own an iPad or iPhone, but I have the Nextcloud app installed on my iPad for uploading my drawings from procreate. Recently, when I tried to delete a file it was trying to upload, it showed an error that I didn't have network access and when I dismissed the error, I got the following in the logs:

{
  "transaction": {
    "client_ip": "192.168.1.1",
    "time_stamp": "Sun Jul 21 08:55:02 2024",
    "server_id": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
    "client_port": 17408,
    "host_ip": "xx.xx.xx.xx",
    "host_port": 443,
    "unique_id": "xxxxxxxxxxxx.xxxxxxxx",
    "request": {
      "method": "DELETE",
      "http_version": 2.0,
      "uri": "/ocs/v2.php/apps/notifications/api/v2/push",
      "body": "",
      "headers": {
        "host": "cloud.example.com",
        "authorization": "Basic xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
        "accept": "application/json",
        "content-type": "application/x-www-form-urlencoded",
        "ocs-apirequest": "true",
        "accept-language": "en-NL;q=1.0, nl-NL;q=0.9",
        "content-length": "0",
        "accept-encoding": "br;q=1.0, gzip;q=0.9, deflate;q=0.8",
        "user-agent": "Mozilla/5.0 (iOS) Nextcloud-iOS/5.4.1",
        "cookie": "__Host-nc_sameSiteCookielax=true; __Host-nc_sameSiteCookiestrict=true; oc_sessionPassphrase=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx; ocrkhwrly2jb=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
      }
    },
    "response": {
      "http_code": 403,
      "headers": {
        "Server": "",
        "Date": "Sun,21 Jul 2024 06:55:02 GMT",
        "Content-Length": "146",
        "Content-Type": "text/html",
        "Connection": "close",
        "Strict-Transport-Security": "max-age=31536000; includeSubDomains"
      }
    },
    "producer": {
      "modsecurity": "ModSecurity v3.0.12 (Linux)",
      "connector": "ModSecurity-nginx v1.0.3",
      "secrules_engine": "Enabled",
      "components": [
        "OWASP_CRS/4.4.0\""
      ]
    },
    "messages": [
      {
        "message": "Method is not allowed by policy",
        "details": {
          "match": "Matched \"Operator `Within' with parameter `GET HEAD POST OPTIONS' against variable `REQUEST_METHOD' (Value: `DELETE' )",
          "reference": "v0,6",
          "ruleId": "911100",
          "file": "/etc/nginx/owasp-modsecurity-crs/rules/REQUEST-911-METHOD-ENFORCEMENT.conf",
          "lineNumber": "28",
          "data": "DELETE",
          "severity": "2",
          "ver": "OWASP_CRS/4.4.0",
          "rev": "",
          "tags": [
            "application-multi",
            "language-multi",
            "platform-multi",
            "attack-generic",
            "paranoia-level/1",
            "OWASP_CRS",
            "capec/1000/210/272/220/274",
            "PCI/12.1"
          ],
          "maturity": "0",
          "accuracy": "0"
        }
      },
      {
        "message": "Inbound Anomaly Score Exceeded (Total Score: 5)",
        "details": {
          "match": "Matched \"Operator `Ge' with parameter `5' against variable `TX:BLOCKING_INBOUND_ANOMALY_SCORE' (Value: `5' )",
          "reference": "",
          "ruleId": "949110",
          "file": "/etc/nginx/owasp-modsecurity-crs/rules/REQUEST-949-BLOCKING-EVALUATION.conf",
          "lineNumber": "222",
          "data": "",
          "severity": "0",
          "ver": "OWASP_CRS/4.4.0",
          "rev": "",
          "tags": [
            "anomaly-evaluation",
            "OWASP_CRS"
          ],
          "maturity": "0",
          "accuracy": "0"
        }
      }
    ]
  }
}

Luckily, I don't think this actually breaks anything this time, just kind of clogs the logs a bit. What's weird is that this should already be caught, I think? Check out this section:

https://github.com/coreruleset/nextcloud-rule-exclusions-plugin/blob/27cee1604aa6a2c6e15dc563d23cdfeb697316a9/plugins/nextcloud-rule-exclusions-before.conf#L1889-L1916

I'm unable to reproduce the same issue from my android, web, or native macOS apps. 🤷 Maybe we need something like this?

    # Dismissing notifications from ios?
    SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/notifications/api/v[0-9]/notifications/push$" \
        "id:9508703,\
        phase:1,\
        pass,\
        t:none,\
        nolog,\
        ver:'nextcloud-rule-exclusions-plugin/1.2.0',\
        setvar:'tx.allowed_methods=%{tx.allowed_methods} DELETE'"
EsadCetiner commented 1 month ago

@jessebot Thanks for the report

Luckily, I don't think this actually breaks anything this time, just kind of clogs the logs a bit.

This false positive might break functionality for dismissing notifications, although that would be a relatively small annoyance. I'd still fix the false positive as it can be an issue in the future.

What's weird is that this should already be caught, I think? Check out this section:

It looks like that on first glance, but if you look closer the /push is what's causing the issue.

I'm unable to reproduce the same issue from my android, web, or native macOS apps. 🤷 Maybe we need something like this?

Yes that should work, but I'd modify the regex for 9508701 instead so you don't have to introduce a new rule

- SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/notifications/api/v[0-9]/notifications(?:/[0-9]+)?$" \ 
+ SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/notifications/api/v[0-9]/notifications(?:/[0-9]+|/push)?$" \ 

Try to avoid introducing unnecessary rules to prevent the number of rules from exploding, this plugin is already pretty big as it is.

Do you want to try opening a PR? if you do, please add some tests if you can.

jessebot commented 1 month ago

@EsadCetiner Thank you! Unfortunately, I don't have time in my schedule to fix this right now, so if you can do this, that would be great. Otherwise, I probably won't have time for a week or two, as I'm not really sure how to write the tests yet, so I'd have to spend some time with it.

EsadCetiner commented 1 month ago

@jessebot No problem, I just wanted to encourage you since you expressed interest in doing one yourself last time. I've opened a PR to fix the issue: #89

I see in the transaction log you provided that there's a content type header, I don't see any request body, are you able to provide one for the tests (No big deal if you can't provide one)? You might have to add SecAuditLogParts ABCDEFHIJZ to modsecurity.conf to enable request body logging.

jessebot commented 1 month ago

I appreciate that! 🙏 I will do my best to make time for this repo as soon as my schedule clears up a tiny bit.

I think Request Body is enabled by default in my setup, unless I'm wrong (totally possible 🤦 ), but I think there's no body in this case, as the body did get logged in the instance of the nextcloud cookbook app here, https://github.com/coreruleset/nextcloud-rule-exclusions-plugin/issues/88#issue-2421505618, and both log entries would have been spawned by the same instance of nginx, I think.

Here's my ingress-nginx modSecurity section which should be equivlent of modsecurity.conf: https://github.com/small-hack/argocd-apps/blob/c317ee116f6c1df2d865ac13f091e6a094583b6b/ingress-nginx/ingress-nginx_argocd_appset.yaml#L93-L141

Oh, btw, I'm a maintainer over at nextcloud/helm, and to help drive adoption of this plugin, I've created a discussion post to promote it here if you're interested (I also got my PR released so ingress-nginx is now using the latest version of CRS): https://github.com/nextcloud/helm/discussions/592

EsadCetiner commented 1 month ago

@jessebot

I think Request Body is enabled by default in my setup, unless I'm wrong (totally possible 🤦 ), but I think there's no body in this case, as the body did get logged in the instance of the nextcloud cookbook app here, https://github.com/coreruleset/nextcloud-rule-exclusions-plugin/issues/88#issue-2421505618, and both log entries would have been spawned by the same instance of nginx, I think.

Just making sure since there's a known issue on libModSecurity3 where the request body won't be logged in the default configuration(This is the default on ModSecurity2).

Here's my ingress-nginx modSecurity section which should be equivlent of modsecurity.conf: https://github.com/small-hack/argocd-apps/blob/c317ee116f6c1df2d865ac13f091e6a094583b6b/ingress-nginx/ingress-nginx_argocd_appset.yaml#L93-L141

I don't see a rule for https://github.com/owasp-modsecurity/ModSecurity/blob/3dda900ee9f86619354dd41eefd160794e1833dd/modsecurity.conf-recommended#L75, this rule should be triggered if a request with a content type is sent without a request body, that doesn't happen for you?

Oh, btw, I'm a maintainer over at nextcloud/helm, and to help drive adoption of this plugin, I've created a discussion post to promote it here if you're interested (I also got my PR released so ingress-nginx is now using the latest version of CRS): https://github.com/nextcloud/helm/discussions/592

Sounds awesome!