Feature: Pm custom separator

[M4tteoP]

Context

@pm currently implicitly relies only on the (space) as the separator. For any more complex scenario has been suggested to rely on rx (less performant) or on pmFromFile (not available for some environments e.g. Wasm).
The possibility of choosing a custom delimiter has been requested for such a long time (see #682) and iterated over time by some different issues (E.g. #2699).

Proposal

This PR proposes a little extension of the pm operator capability permitting the specification of a custom separator.
It :

• To the best of my knowledge does not impact the default behaviour of the pm operator.
• Allows writing pm rules against strings with spaces (e.g. "a b c").
• Allows writing pm rules against piped commands (just like PmFromFile, this proposal bypasses the Snort-Suricata format parsing, therefore the pipe symbol can be used as a separator or matched as any other character).

Notes:

• Some of the limitations pointed out by @liudongmiao are still in place: @pm still can not contain %, " and can not end with \.

Example Rule syntax:

SecRule REQUEST_BODY "@Pm PmCustomSeparator:| single_word|<this> <is> <a> <string>|trailing_space " "id:999,phase:2,t:lowercase,deny"

Open to any discussion about this functionality and to further work on it for any better design agreed.

Closes #682.

[marcstern]

Thanks a lot for this PM. I hope it will be accepted.

About the syntax, why not simplifying it:
"@pm| single_word| |trailing_space "

Also, would this be supported?
"@Pm/ single_word/ /Snort-Suricata binary |22|"

[M4tteoP]

Hi @marcstern, happy to see you still around!

Syntax

For the first proposal, I gave a go to a more explicit syntax in order to:

• Provide a first working solution as PoC with a small codebase change.
• Reduce the risk of impacting the default pm behaviour as much as possible.
• Provide a more verbose operator to make it clear to any reader what is going on with that rule.

Furthermore, I'm gonna try to dig into how the rules are parsed by ModSecurity, but I'm guessing that a space character between the operator name (@pm) and anything else is needed in order to make the parse able to understand which operator we are going to use.
Looking for any guidance about it 👀 (@martinhsv, @zimmerle).

It would be easy to do something like "@pm \ single_word\trailing_space " but then it can lead to misunderstandings (Is the single initial character a custom separator or it is a single character that we want to look for?).

Making Custom delimiters and Suricata syntax coexist

As of now, in your example ("@Pm/ single_word/ /Snort-Suricata binary |22|"), the |22| would not interpret as a snort-Suricata binary.
I bypassed on purpose the Snort-Suricata format parsing because:

• I mainly tried to mimic the PmFromFile behaviour and, as far as I understand, here the Snort-Suricata format parsing is not done
• It is a design tradeoff between enabling the match of piped payloads and Snort-Suricata. I thought that: if you wish Snort-Suricata you can go with the default pm separator, otherwise, if you need PmFromFile/CustomSeparator you are not going to have Snort-Suricata working. It would lead to overall increased flexibility in terms of what Pm is capable of.
• I gave a go to Snort-Suricata syntax, but I end up with memory errors. They may be related to the memory leaks pointed out here. I'm wondering, is this parsing safe to use?

Just wanted to share some thoughts behind my implementation, happy to keep this conversation going on and implement an agreed and technically doable design!

[M4tteoP]

FYI: @marcstern, I extended the conversation also with Coraza's folks. I would be happy to see your point of view also about their proposals corazawaf/coraza#349

[marcstern]

Indeed, a space is mandatory after the operator

[martinhsv]

Overall I think there are quite a few things to like about this proposal.

A couple of things that gave me pause:

One thing that's somewhat different from existing functionality is with the operator's parameters. Almost all ModSecurity operators take exactly one parameter, and that (mostly) makes them simple to use. We do have at least one exception to this: fuzzyHash takes 2 parms. But we don't have anything currently that takes either one or two parameters. That doesn't need to be a show-stopper but it is something to be aware of.

This does mean implementing a "magic string" that has a special meaning, when that same string might have had an entirely different meaning prior to this implementation. (Might someone have wanted to try to match 'PmCustomSeparator:' against ARGS?) That can be a bit of a red flag when building a syntax. If we were concerned about this, one alternative could be to implement an entirely separate operator (@pmWithSeparator ?) to support the new functionality. On balance, I think I still prefer your approach here for what is likely to be, in practice, only a theoretical downside.

On a more detail level, I think there are still some things to consider mostly from the perspective of clarity and usability. I'll use your example

"@Pm PmCustomSeparator:| htaccess|This is an error string| hi"

• the parsing of the operator-parameters appears to consume the character after the ':' regardless of what it is. Should the code perhaps enforce that it is a space?
• the solution anticipates that a string-to-be-matched could begin with a space character. What if that is the case for the first string? Having two space characters in a row (the first space being the delimiter after ':', while the second space is the first character of the first string) is awkward visually and makes it easy to make an error. Is it worth considering something to handle this -- like maybe the separator is also required at the beginning and end? E.g.

"@Pm PmCustomSeparator:| |htaccess|This is an error string| hi|"

Just some initial thoughts. I do think it's a pretty good solution overall. It expands functionality without meaningful downside to existing handling.

[M4tteoP]

Hello @martinhsv, thank you for your feedback on this PR!

1. One thing that's somewhat different from existing functionality is with the operator's parameters.

The "magic string" would become sort of an optional parameter. The only way I see to not impact the default pm behaviour would be making the second parameter mandatory in a separate operator (just like you pointed out with @pmWithSeparator).
Do you feel there could be a better "magic string" to replace PmCustomSeparator:?

2. Should the code perhaps enforce that it is a space?

I'm not sure what you mean by this. I guess it is about enforcing that it is not a space. I'm sorry if I misunderstood. If the guess is right, yes, I agree, I implemented it. Now if it is just PmCustomSeparator:, not followed by a non-space character, the behaviour will be the usual one, trying to match PmCustomSeparator:. It may even be a trick to make the "magic string" downside more theoretical than before.

3. like maybe the separator is also required at the beginning and end?

I agree, some corner cases without separators at the beginning and end can be definitely awkward. I implemented it by adding some logic to parsing the string: it is about looking for the initial and final separator and stripping all the rest.

[marcstern]

Can we be future-proof and find a standard for additional parameters?
Like "@operator @param1:value1 @Param2:value2 target..."
Examples:

• "@pm @PmCustomSeparator:| htaccess|This is an error string| hi"
• "@contains @nocase:yes select * from *"
• "@rsub @RsubCustomSeparator:| s|//|%2f%2f|"

Also, I already 2 use cases for a custom separator; why not standardizing "@CustomSeparator:" and implementing it in the common code.
Actually, the parsing of all @params should be integrated in the base code I think. When calling the parser, a function could register the parameters they want to be returned like (..., "@PmCustomSeparator", &separator).