Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Avoid embedded anchors in CRS rule 942330 #1668

Merged

Conversation

allanrbo
Copy link
Contributor

Rule 942330 has an embedded start anchor. Specifically this part:

(?:^[\"'`\\\\]*?(?:[\d\"'`]+|[^\"'`]+[\"'`]))+

has a ^ inside a +-group. The Hyperscan regex engine does not support this, so I'm sharing this fix for others also experimenting with Hyperscan.

It looks like the purpose of the group is to find lines that begin with quotes or digits. The + makes it consume multiple of such lines if there are multiple. But even without the +, we would always find one if one is present.

To get rid of the embedded start anchor, I suggest removing the + from behind the group. It's only effect was to allow multiple matches of the group, but in this case it's enough to just match the last instance of the group for this rule to trigger. We can then also remove the (?:) as there is no longer any purpose of having this expression be a group. The part in question then becomes:

^[\"'`\\\\]*?(?:[\d\"'`]+|[^\"'`]+[\"'`])

The rule will trigger on the same input, but the matched data is now a little bit less. %{TX.0} will now only contain the last instance of the group above. I think this is not a problem. Especially since the logdata action also contains %{MATCHED_VAR}, which contains the entire variable that triggered this matched (not just the matched portion).

All the above also applies to the other similar part

(?:^[\"'`\\\\]*?[^\"'`]+[\"'`])+

@fzipi
Copy link
Contributor

fzipi commented Feb 9, 2020

@fgsch @dune73 What do you think about this change?

@dune73
Copy link
Contributor

dune73 commented Feb 10, 2020

I see the reasoning and the way @allanrbo does this. And I agree that the original rule looks odd with the anchor embedded. But let's way for @fgsch before we merge.

@dune73
Copy link
Contributor

dune73 commented Feb 11, 2020

Meeting decision: @franbuehler will review this and merge afterwards if viable.

#1671 (comment)

Copy link
Contributor

@franbuehler franbuehler left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. I'll merge it now. Thank you @allanrbo !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants