Skip to content
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.

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

added support for SecRuleUpdateTargetByTag and SecRuleUpdateTargetById #46

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

Xhoenix
Copy link
Member

@Xhoenix Xhoenix commented May 6, 2023

plugin-lint fails when SecRuleUpdateTargetByTag is used. This issue can be taken as an example.

@Xhoenix
Copy link
Member Author

Xhoenix commented May 8, 2023

  • variable "ID" was declared but not defined. SecRuleRemoveById fails for single rules.
  • added support for SecRuleUpdateTargetById

src/secrules_parsing/model/secrules.tx Outdated Show resolved Hide resolved
src/secrules_parsing/model/secrules.tx Outdated Show resolved Hide resolved
@fzipi
Copy link
Member

fzipi commented May 15, 2023

Also ID has a special meaning in the textX library.

@Xhoenix
Copy link
Member Author

Xhoenix commented May 15, 2023

In the code:-

IDRangeList:
    idlist+=ID | range=IDRange;

the idlist looks for an ID or an IDRange. But, this fails when trying to remove single rules with SecRuleRemoveById.

Is it okay if I change ID variable name to ruleID and use that in idlist+=ruleID? This will solve the problem with SecRuleRemoveById.

Update:- Using the RangeINT definition from TransientAction fixes everything. It uses the predefined INT definition. So, this is what I tested and it works:-

idlist+=INT

'SecRuleUpdateTargetById' ids=INT '"'? negated='!'? variables=Variable '"'?;

As SecRuleUpdateTargetById doesn't support Id range, I used a single ID.

image

@Xhoenix Xhoenix changed the title added support for SecRuleUpdateTargetByTag added support for SecRuleUpdateTargetByTag and SecRuleUpdateTargetById May 15, 2023
@airween airween self-assigned this May 15, 2023
Copy link
Contributor

@airween airween left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my "big" comment :)


SecRuleUpdateTargetByTag:
'SecRuleUpdateTargetByTag' '"'? tag=Tag '"'? '"'? negated='!'? variables=Variable '"'?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment for both new rule above:

Here are the references of these directives, both for mod_security2 and libmodsecurity3:

https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v2.x)#secruleupdatetargetbyid
https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v2.x)#secruleupdatetargetbytag
https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v3.x)#secruleupdatetargetbyid
https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-(v3.x)#secruleupdatetargetbytag

The syntax of two directives are almost the same, so we can take a look only for the first, for example.

Here are the syntax for mod_security2:

SecRuleUpdateTargetById RULEID TARGET1[,TARGET2,TARGET3] REPLACED_TARGET

and for libmodsecurity3:

SecRuleUpdateTargetById RULEID TARGET1[,TARGET2,TARGET3]

As you can see, the REPLACED_TARGET argument is gone.

IMHO without that this syntax makes no sense:

SecRuleUpdateTargetById 1234 "ARGS:foo"

Because you want to update a target - but for what? I mean there is no exclusion mark - with that, it makes sense:

SecRuleUpdateTargetById 1234 "!ARGS:foo"

This means you just want to remove this target.

If you want to be strict, you should allow one of these syntax:

SecRuleUpdateTargetById 1234 "!ARGS:foo"
SecRuleUpdateTargetById 1234 "ARGS:foo" "ARGS:bar"

But I'm sure this is not a critical thing, you can keep this as it is.

Also, both directives allows the list of TARGET:

... TARGET1[,TARGET2,TARGET3]

which is not allowed by this implementation. I suggest that you should add this syntax. Both mod_security2 and libmodsecurity3 allows this syntax.

Copy link
Member Author

@Xhoenix Xhoenix May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've come up with this.

First, I'll create a new variable like this:-

TARGET: '"'? negated='!'? variables=Variable '"'?;

Then I'll change the rules based upon this. For e.g,

SecRuleUpdateTargetById:
    'SecRuleUpdateTargetById' id=INT targets+=TARGET[',']?;

What do you think about this? I tested this and it works.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you pull this modification to your repository? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I pulled down, tested, and looks now it works as we expect.

added a new TARGET variable and updated rules accordingly
Copy link
Contributor

@airween airween left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.


SecRuleUpdateTargetByTag:
'SecRuleUpdateTargetByTag' '"'? tag=Tag '"'? '"'? negated='!'? variables=Variable '"'?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I pulled down, tested, and looks now it works as we expect.

@fzipi fzipi merged commit 988d2e8 into coreruleset:master Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants