Skip to content

Conversation

marcstern
Copy link

There's a check for (rule->actionset->id != NULL) that doesn't check for target exceptions in case a rule has no id. This must be a copy-paste from the code checking exceptions by id.
This PR removes that check.
A little enhancement could be to remove the "id=NULL" from the debug log, but this wasn't done here.

@marcstern marcstern changed the title ctl:removeByTag isn't executed if no rule id is present in the rule ctl:ruleRemoveByTag isn't executed if no rule id is present in the rule Nov 2, 2023
@martinhsv
Copy link
Contributor

Hello @marcstern ,

Could you please provide a use case where this PR would result in a modification of functionality?

@marcstern
Copy link
Author

Very basic (illustrative) example:

SecRule REQUEST_HEADERS:Content-Type "json" "phase:1,ctl:ruleRemoveByTag=JSON"
[...]
SecRule REQUEST_BODY "^{" "phase:2,tag:JSON,deny,msg:'JSON detected in body'"

A correctly typed JSON is (incorrectly) blocked.
If we add an id to the second rule, the exception is taken into account and the request is not blocked.
With the PR, the request is accepted even if the rule has no id.

@martinhsv
Copy link
Contributor

If I try to include to include your sample rule:

SecRule REQUEST_HEADERS:Content-Type "json" "phase:1,ctl:ruleRemoveByTag=JSON"

... then Apache will fail to start and the error message is produced: 'ModSecurity: No action id present within the rule'

Do you experience something different?

@martinhsv martinhsv added the 2.x Related to ModSecurity version 2.x label Nov 3, 2023
@marcstern
Copy link
Author

It has to be compiled with "./configure --disable-rule-id-validation"

@marcstern marcstern merged commit 710cc99 into owasp-modsecurity:v2/master Feb 1, 2024
@marcstern marcstern deleted the v2/mst/except_noid branch May 29, 2024 10:57
@marcstern marcstern restored the v2/mst/except_noid branch June 7, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants