search

coreruleset / nextcloud-rule-exclusions-plugin

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

FP when uploading files via desktop client #46

Closed mhastu closed 8 months ago

mhastu commented 8 months ago

Uploading files from the (windows) desktop client produces false positives with the log:

ModSecurity: Warning. String match within "/accept-charset/ /content-encoding/ /proxy/ /lock-token/ /content-range/ /if/" at TX:header_name_if. [file "/etc/modsecurity/crs/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf"] [line "1128"] [id "920450"] [msg "HTTP header is restricted by policy (/if/)"] [data "Restricted header detected: /if/"] [severity "CRITICAL"] [ver "OWASP_CRS/3.3.5"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-protocol"] [tag "paranoia-level/1"] [tag "OWASP_CRS"] [tag "capec/1000/210/272"] [tag "PCI/12.1"] [hostname "example.org"] [uri "/remote.php/dav/uploads/user/4195284221/.file"]

I managed to fix it on my setup by removing /if form the restricted header list for requests uris of this form, with the following rule:

SecRule REQUEST_FILENAME "@rx /remote\.php/dav/uploads/[^/]+/[0-9]+/.file$" \
    "id:9999999999,\
    phase:1,\
    pass,\
    t:none,\
    nolog,\
    ver:'nextcloud-rule-exclusions-plugin/1.0.0',\
    setvar:'tx.restricted_headers=/accept-charset/ /content-encoding/ /proxy/ /lock-token/ /content-range/'"

I think that removing /if from the current value of tx.restricted_headers might be more future-proof, but I don't know how to do that, hence I didn't open a pull-request.

Desktop client version: 3.11.0 (might be relevant, since this issue did not occur before)

EsadCetiner commented 8 months ago

@mhastu I agree that setting the restricted_headers variable per rule isn't a good solution for allowing the if header. It needs to be kept in sync with any changes made in CRS and doesn't respect any changes made within crs-setup.conf.

The better option would be to chain another rule and check for the existance of the if header, then disable 920450 like I've done here: #47

Then, any changes made within crs-setup.conf is respected and you don't have to keep it in sync.

also, is .file meant to be a filename?

I'm personally on 3.11.0 on Windows 11 with Nextcloud 27.1.5 and I haven't encountered this issue

mhastu commented 8 months ago

Oh yes, using a chained SecRule &REQUEST_HEADERS is a much better solution.

I did not change the uri in the log, the sync client actually uses .file at the end.

This might only occur for large files >700MB in my case, I guess the sync client then creates a temporary file called .file.