Plugins may need to be able to hook into any of the rules files #3154

theseion · 2023-03-08T18:55:33Z

Initiated by this discussion: coreruleset/nextcloud-rule-exclusions-plugin#12.

Scenario
We want to modify allowed_methods (default: GET HEAD POST OPTIONS) in a plugin to include another method conditionally, e.g., to upload a file in a specific context. Modifying allowed_methods is great, because there's already a rule that takes care of blocking and anomaly scoring for us.

Problem
allowed_methods is initialised in REQUEST-901-INITIALIZATION.conf and checked for compliance in REQUEST-911-METHOD-ENFORCEMENT.conf. The plugin system can currently run rules either before all CRS rules, or after all CRS rules. Neither of those two options makes it possible to modify allowed_methods with the expected outcome.

We could work around this limitation by copying the rule from REQUEST-911-METHOD-ENFORCEMENT.conf to the plugin and then running the modification rule and the checking rule after the CRS rules. I don't believe that this is ideal though. We should try to prevent duplication of rules by plugins.

Instead, I think we need to (again) discuss the option to run plugin rules after REQUEST-901-INITIALIZATION.conf. Maybe we could even add "before" and "after" hooks to all of the CRS rules files (probably not necessary).

I don't think ModSecurity allows including other files from .conf files, which would make it easy to solve this problem.

The text was updated successfully, but these errors were encountered:

fzipi · 2023-03-08T19:31:54Z

Thanks Max. Agreed we need to review.

dune73 · 2023-03-08T20:40:48Z

901160 is only initializing allowed_methods if the variable is not yet existing. So if the plugin defines the variable in the plugin-config.conf, the problem is solved.

I agree there is a chance where you want to extend allowed_methods without looking at the default setting of allowed_methods, but I do not th 8000 ink we have to accommodate for this situation.

Why not: Because if a user want to modify allowed_methods without looking at the default value, he / she runs the risk of adding a method that is already defined in allowed_methods. So technically you would have to inspect allowed_methods and if the method in question is not yet part of the variable then you add it. But I think this is all overkill and I would take a pragmatic approach.

Doubling down on this: @theseion mentioned "upload a file in a specific context". That implies a given URL. And if you know the URL and that it's about an operation like file upload, there is no point in extending the existing allowed_methods variable. You simply set it to method you are expecting.

RedXanadu · 2023-03-09T09:29:40Z

We also still have a really nasty plugin architectural question open here which we need to address eventually:
coreruleset/nextcloud-rule-exclusions-plugin#2

May be related (I need to fully re-read both issues when I have the time…)

dune73 · 2023-03-09T09:36:59Z

This is the exact same problem.

It is an edge case and even if I may be downplaying it, I would rather not change the plugin architecture again before the release. I would rather roll it out, get some experience and then maybe adjust things for 4.1.

We can to bring our own recommended rules for 4.1 as well, so there is a chance we will look into variable initialization for 4.1 anyways and address this for good.

theseion · 2023-03-09T19:07:02Z

For allowed_methods it indeed seems we could just make plugin developers add the default value too. In the following rule, for example, GET HEAD POST OPTIONS would need to be added to allowd_methods:

# Nextcloud uses DAV methods with index.php and remote.php to do many things
# The default ones in ModSecurity are: GET HEAD POST OPTIONS
#
# Looking through the code, and via testing, I found these:
#
# File manager: PUT DELETE MOVE PROPFIND PROPPATCH
# Calendars: REPORT
# Others in the code or js files: PATCH MKCOL MOVE TRACE
# Others that I added just in case, and they seem related:
#   CHECKOUT COPY LOCK MERGE MKACTIVITY UNLOCK.
SecRule REQUEST_FILENAME "@rx /(?:remote|index|public)\.php/" \
    "id:9508130,\
    phase:1,\
    pass,\
    t:none,\
    nolog,\
    ver:'nextcloud-rule-exclusions-plugin/1.0.0',\
    setvar:'tx.allowed_methods=%{tx.allowed_methods} PUT PATCH CHECKOUT COPY DELETE LOCK MERGE MKACTIVITY MKCOL MOVE PROPFIND PROPPATCH SEARCH UNLOCK REPORT TRACE jsonp'"

But now imagine the same scenario with a variable like restricted_extensions. I just don't think it's a good idea to repeat that. We would need to check every plugin, each time we wanted to update a default variable. You are of course right in saying that if a rule modifies a variable, the author must have looked at the default. But here's a counter argument: if someone modifies the default in their installation, they shouldn't have to go and change all the "modifications" as well. The modification of the default should be picked up by all rules that modify the variable.

dune73 · 2023-03-10T09:16:02Z

Does not your allowed_methods example fall exactly into this trap?

restricted_extensions seems to be a better example where the need to change the variable is more frequent. But I think for the time being the workaround explained above would work and we buy time to gain more experience to overhaul the recommended rules, the variable initialization and plugin configuration together for 4.1.

I do not see us finding the elegant solution right now.

RedXanadu · 2023-03-20T18:37:53Z

We can to bring our own recommended rules for 4.1 as well, so there is a chance we will look into variable initialization for 4.1 anyways and address this for good.

After re-examining this topic (also re-reading the other identical open issue), this sounds like the best plan so far.

Here's a radical thought:

Why do we have redundant pairs of rules, for example one rule that does:

"id:900200,\
⋮
setvar:'tx.allowed_methods=GET HEAD POST OPTIONS'"

and then another, different rule that does:

"id:901160,\
⋮
setvar:'tx.allowed_methods=GET HEAD POST OPTIONS'"

If we're planning to make significant changes to crs-setup.conf for 4.1 anyway (and maybe rolling in our own version of modsecurity.conf-recommended as well, so massive changes ahead anyway), then why not move the variable initialisation step into crs-setup.conf, too?

As long as we clearly state what the default values are - that's a good thing about having a separate REQUEST-901-INITIALIZATION.conf file: you are guaranteed to be able to fall back to sensible/correct default values, no matter how much you mess up your setup.conf file…

Or maybe there's a very good reason for having those redundant rule pairs that I'm forgetting 😅

theseion · 2023-03-20T18:43:02Z

Fine by me. As the discussion is stalling a bit (for the second time) I seems to be more important to have a working solution, rather than the perfect one, for now.

Let's quickly talk about it in the chat tonight.

theseion · 2023-03-27T19:48:13Z

As decided in the the chat, we solve this for now by duplicating initialization rules in plugins to set defaults, if necessary.

theseion self-assigned this Mar 8, 2023

theseion mentioned this issue Mar 20, 2023

Monthly Chat Agenda March 2023 (2023-03-06 and 2022-03-20) #3039

Closed

theseion closed this as completed Mar 27, 2023

theseion mentioned this issue Mar 27, 2023

Variable expansion not working? coreruleset/nextcloud-rule-exclusions-plugin#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Plugins may need to be able to hook into any of the rules files #3154

Plugins may need to be able to hook into any of the rules files #3154

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Plugins may need to be able to hook into any of the rules files #3154

Plugins may need to be able to hook into any of the rules files #3154

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!