8000 Update Nextcloud rules by kam821 · Pull Request #1902 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update Nextcloud rules #1902

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

Update Nextcloud rules #1902

wants to merge 4 commits into from

Conversation

kam821
Copy link
@kam821 kam821 commented Oct 8, 2020
  • Fix issues during uploading e.g. PHP files (add PUT method to 9003106, 9003107 rules)
  • Keep same syntax for match /ocs/v[0-9].php
  • Add rule to allow Nextcloud users delete.
  • Add rule to allow configure file extensions list in Ransomware protection extension.

kam821 added 2 commits October 9, 2020 00:12
@kam821
Update Nextcloud rules
789df35 
- Fix issues during uploading e.g. PHP files (add PUT method to 9003106, 9003107 rules)
- Keep same syntax for match /ocs/v[0-9].php
- Add rule to allow Nextcloud users delete.
- Add rule to allow configure file extensions list in Ransomware protection extension.
@kam821
Nextcloud - add missing rule description
9a34f38
@azurit
Copy link
Member
azurit commented Oct 9, 2020

For the first commit.

Please use @pm instead of @rx:
"@pm PROPFIND PUT"

For rules 9003701 and 9003800, what do you think about second occurence of version number? Maybe it can be handled too?
SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/notifications/api/v2/notifications" \
SecRule REQUEST_FILENAME "@rx /ocs/v[0-9]+\.php/apps/files_sharing/api/v1/shares" \

@kam821
Nextcloud rules - reduced rx usage
ef03c0a 
- Reduced rx method usage to improve performance
- Spacing and comments corrected
@kam821 kam821 marked this pull request as draft October 9, 2020 18:55
@kam821
Copy link
Author
kam821 commented Oct 9, 2020 •
edited
Loading

Temp turned this pull request into draft.

@azurit
Thank you for your advice.
Could you tell me, what do you think about recent commit - ef03c0a
Some changes:
9003160 replaced -> contains 9003160, endsWith 9003420
9003333 removed => 9003332 contains -> pm
Is this the correct way? Is using pm in REQUEST_FILENAME match safe?

About Nextcloud ocs/v[0-9].php paths.
I don't think it is necessary. If I remember correctly, api version determines what applications are available, but to be sure, it would be necessary to review the documentation (unfortunately this one is awful)

@azurit
Copy link
Member
azurit commented Oct 9, 2020 •
edited
Loading

Looks very good!

I wouldn't use @pm for REQUEST_FILENAME because it prevents running an application inside a directory. For example, with your changes applied, the rule number 9003115 won't match this URL:
example.com/nextcloud/remote.php/dav/files/

I think for rules like 9003115, it is ok to keep @rx because it's used with second, chained, SecRule where most of the requests will be filtered out with first SecRule (so impact on performance will be minimal). If it's possible, never use @rx with first SecRule (but, sometimes, it's not possible, see 9003130).

@kam821
Copy link
Author
kam821 commented Oct 10, 2020 •
edited
Loading

@azurit
Are you sure, that @pm won't match subdirectories?
I tested rule 9003115 as separate, the only one rule for another VHost:
SecRule REQUEST_FILENAME "@pm /remote.php/dav/files/ /remote.php/dav/uploads/" "phase:2,id:'10',log,deny,status:403"
and it catches both queries to remote.php, whether it is placed directly in document root or in e.g nextcloud/ subdirectory.

Tested on Openlitespeed 1.6.16, mod_security 1.3.

If I understand correctly, the pm method is case insensitive (is it?), and that's what I asked about safety of using @pm for match REQUEST_FILENAME.

@kam821
Copy link
Author
kam821 commented Oct 10, 2020 •
edited
Loading

@azurit
One more thing - which approach will be more efficient - multiple @contains (let's say 2, maybe 3 max) or one @pm? (for simple, replaceable @pm rules, like 9003115, of course)
Is the best approach same for both primary and nested rules?
Regards

@azurit
Copy link
Member
azurit commented Oct 11, 2020

@azurit
Are you sure, that @pm won't match subdirectories?
I tested rule 9003115 as separate, the only one rule for another VHost:
SecRule REQUEST_FILENAME "@pm /remote.php/dav/files/ /remote.php/dav/uploads/" "phase:2,id:'10',log,deny,status:403"
and it catches both queries to remote.php, whether it is placed directly in document root or in e.g nextcloud/ subdirectory.

Tested on Openlitespeed 1.6.16, mod_security 1.3.

If I understand correctly, the pm method is case insensitive (is it?), and that's what I asked about safety of using @pm for match REQUEST_FILENAME.

Sorry, you are right. I don't recommend to use it with REQUEST_FILENAME.

@azurit
One more thing - which approach will be more efficient - multiple @contains (let's say 2, maybe 3 max) or one @pm? (for simple, replaceable @pm rules, like 9003115, of course)
Is the best approach same for both primary and nested rules?
Regards

I cannot tell for sure (should be tested for performance) but i would say that @pm will do better. Also, the rule will be easier to read and maintain.

@kam821
Nextcloud - polishing rules
2beca60 
- Removed pm method in REQUEST_FILENAME matching due to security concerns - coreruleset#1902
@kam821
Copy link
Author
kam821 commented Oct 11, 2020 •
edited
Loading

@azurit
I think the PR is ready.
It would be best to rewrite this file completely because of the mess, repetitive matches and general illegibility.
Anyway, could you please explain, what does failing regression test mean?
Is it happened due to a problem in my patch or somewhere else?

@azurit
Copy link
Member
azurit commented Oct 11, 2020

Anyway, could you please explain, what does failing regression test mean?
Is it happened due to a problem in my patch or somewhere else?

I re-run tests and it now passes ok, it was only some kind of random/temporary error, sorry for that.

@kam821 kam821 marked this pull request as ready for review October 11, 2020 16:35
@kam821 kam821 mentioned this pull request Oct 13, 2020
Closed
@mackov83
Copy link

@kam821
Thanks for the heads up. Please bear with me, as I mentioned in my post I am a little new to all of this type of thing. Can you please explain what your recommendation is? I assume you are suggesting I apply this 3.4/dev coreruleset?
TIA

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

@kam821 Can you explain why have you changed IDs of existing rules? Thank you!

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

@azurit : We have not heard from the contributor anymore. Would you mind picking this up and if need be fix it yourself in a new PR, so we can merge this eventually?

@azurit azurit mentioned this pull request Dec 7, 2020
Merged
@azurit
Copy link
Member
azurit commented Dec 7, 2020

See PL #1946 .

@dune73
Copy link
Member
dune73 commented Dec 7, 2020

Closing this in favor of #1946, which has been merged in the meantime. Thank you @kam821 for your original contribution.

@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.
4 participants
@kam821 @azurit @mackov83 @dune73
0