Inconsistent use of SecArgumentsLimit in Recommended Rules

[studersi]

Describe the bug

The limit SecArgumentsLimit is inconsistently used an documented.

	v2.9.8	v3.0.13
Supported (code)	x	? (could not find it in source code, only in recommended rules)
Supported (documentation)	x (2.9.7)	x (3.0.5)
ModSecurity Recommended Rules	-	x

Logs and dumps

Not applicable.

To Reproduce

Not applicable.

Expected behavior

I would expect the recommended rules for v2.9.8 to also include the SecArgumentsLimit configuration, like 3.0.13.

Server (please complete the following information):

• ModSecurity version (and connector): v2.9.8
• WebServer: Not applicable.
• OS (and distro): Not applicable.

Rule Set (please complete the following information):

• Running any public or commercial rule set? ModSecurity Recommended Rules
• What is the version number? v2.9.8

Additional context

[airween]

Hi @studersi,

thanks for reporting this. You're completely right, it would be much better to make the both versions' default configuration files consistent.

For the record: in libmodsecurity3 the initial set step of that variable is here. (This is a "compiled" code (by Bison), the original part is here).

Would you mind to send a PR for v2 to fix this?

My side note: I'm not sure this rule makes sense; if the engine is in DetectionOnly mode (which is the default...!) then this rule does nothing... But of course we can start to discuss, what would be the nice solution.

[dune73]

I'm not sure I get your argument about the engine. All the blocking recommended rules do nothing unless you put the engine in - well - blocking mode. So what's the difference here?

[airween]

There is no difference, I just think it's a bit confuse that in the "recommended" configuration file we set the engine to "DetectionOnly" and add a rule without any notification that won't work after installation.

My other comment regarding this rule that rule 200002 does (almost) the same (but in another way). If the SecArgumentsLimit is set (and it is here), then if the number of arguments has reached the limit, then it sets the REQBODY_ERROR, which triggers rule 200002.

I really don't know that rule 200007 is necessary. But not really disturbing, just my 2 cents.

[airween]

@studersi, @dune73, @marcstern - what do you think guys, do we need rule 200007 in recommended modsecurity.conf file? As I explained above I don't think it's necessary, but I don't want to decide alone. I will gladly create a PR if it's really need.

[dune73]

I see the redundancy and keeping multiple values in sync is annoying.

If we recommend 200007, then I think the comment for SecArgumentsLimit should make it clear that a violation will lead to a REQBODY_ERROR and thus 200002 being triggered.

Also, I am not sure about the implications of the comment If SecArgumentsLimit has been set, you probably want to reject any request body that has only been partly parsed.. What does this really mean?

And is there any influence of the different body parsers on reqbody error and the argument limit behavior?

[marcstern]

SecArgumentsLimit + 200002 is better than 200007 because the former will stop the JSON parsing after X args (and JSON parsing is CPU-intensive). For other parsers, there's no difference (currently).
200007 is totally redundant, but less powerful. We should remove it.

[airween]

And is there any influence of the different body parsers on reqbody error and the argument limit behavior?

Yes, for eg. JSON parser will stop too if it reaches the arglimit. (And the planned XML parser (under #3178) will follow this behavior too.)

@marcstern is right, rule 200007 would wait until the engine counts the number of arguments (which never occurs because rule 200002 prevents that).

[studersi]

I agree that 200002 is probably sufficient for most cases and 200007 is not strictly necessary when it comes to blocking the request.

I wonder, whether this was added to log additional context for these requests as to why the request was blocked. If I remember correctly SecArgumentsLimit itself does not log any relevant information and you only get a generic body parsing error in the logs with no mention of SecArgumentsLimit.

Having a dedicated error message might be very helpful when debugging these issues.

[dune73]

That's a good thought. Would make sense.

[marcstern]

The REQBODY_ERROR is populated with the right error message ("SecArgumentsLimit exceeded" or "More than %ld JSON keys"). This variable should always be logged in case it's populated.

[studersi]

In that case, I don't think I see any reason to keep 200007 either.