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

Feature: Pm custom separator #349

Closed
M4tteoP opened this issue Aug 22, 2022 · 7 comments
Closed

Feature: Pm custom separator #349

M4tteoP opened this issue Aug 22, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@M4tteoP
Copy link
Member

M4tteoP commented Aug 22, 2022

Summary

Hi folks, I'm opening this issue in order to extend the conversation about providing a custom separator for the @pm operator.
It is an open feature request from such a long time inside the Modsecurity repo (see owasp-modsecurity/ModSecurity#682) and Wasm is yet another player that would take advantage of such a feature.

Basic example

I made a first PoC under ModSec repo: owasp-modsecurity/ModSecurity#2786.
This first proposal takes advantage of a fixed string at the beginning of the rule (PmCustomSeparator:) in order to provide and figure out that we are going to use a custom separator.
An overall example of rule syntax is the following:

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

Motivation

  • Make the @pm operator more flexible and able to handle some cases that currently can only be managed by PmFromFile.
  • Wasm environment that is lacking file system support would benefit from it.
  • Depending on the design agreed, it may lead also to looking for piped payloads (currently, to the best of my knowledge, it is not possible to match the pipe | character and do not interpret it as Suricata syntax)

Extra details

  • Even a different operator may be a way to go in order to avoid tweaking a known one and not messing with its default design.
  • If we agree on the usefulness of that feature and on a proper design, I would be happy to implement it on Coraza.

Thanks for any feedback!

cc: @jcchavezs

@jptosso jptosso added the enhancement New feature or request label Aug 22, 2022
@jptosso jptosso self-assigned this Aug 22, 2022
@jptosso
Copy link
Member

jptosso commented Aug 22, 2022

Hey @M4tteoP, thank you for your interest in Coraza. CRS has solved this problem by using text wrappers like |, but what you are talking about WASM is right, and it should concern us.
I have been thinking about a different solution for a while, creating seclang datasets like:

datasets.conf

SecDataset malicious_browsers `
hacker_browser
# this is a comment
hacker_browser_2
`
# or maybe
SecDataset malicious_browsers \
  "hacker_browser", \
  "hacker_browser_2" 

rules.conf

SecRule REQUEST_HEADERS:user-agent "@pmfromdict malicious_browsers" "..."
# or something like :
SecRule REQUEST_HEADERS:user-agent "@pm %{dict_to_pm malicious_browsers}" "..."

This would be consistent with the idea of helpers and code snippets for future versions, but your solution would be easier to implement and understand.
Looking forward to your feedback.

@M4tteoP
Copy link
Member Author

M4tteoP commented Aug 22, 2022

Hi @jptosso, thanks a lot for the prompt reply!
I feel that SecDataset solution mimics in a great way the PmFromFile logic. Thinking about supporting as many characters as possible I would prefer your first example:

SecDataset malicious_browsers `
hacker_browser
# this is a comment
hacker_browser_2
`

As far as I can see, it could permit to look also for % and " characters.

Summarizing:
SecDataset solution would be:

  • harder to implement, maybe a bit less intuitive (but something like PmFromDataset should easily remind a similar behaviour to PmFromFile)
  • a bit more complex. A simple match like "a b c" would still require writing both the SecDataset and the rule.
  • With more characters supported.
  • Working fine in a Wasm env just like PmCustomSeparator.

A couple of questions:

  1. What about The Snort-Suricata syntax, would you just mimic PmFromFile leaving that syntax only for the @pm directive?
  2. What about the implementation? Do you feel that it could have to deal with big problems in Coraza?

As a side note, I'm dreaming about an implementation that may also land on ModSecurity, but there it may be way more tricky...

@jptosso
Copy link
Member

jptosso commented Aug 22, 2022

  1. Snort-Suricata is not supported, but we haven't received any request or issue. I have monitored this repo in case we want to implement Suricata syntax, and we should be completely open to implementing it as long as it is compatible with WASM. Feel free to create an issue if you want.
  2. The only major change for the seclang processor would be the ``` wrappers, as we haven't touched the lex processor in more than a year, but we can handle it.

I will open this discussion for the August Monthly Meeting

@M4tteoP
Copy link
Member Author

M4tteoP commented Aug 25, 2022

Just a note that may add another element to the discussion: the same concept may be applied to ipMatch/ipMatchFromFile operators, having something like SecDataset with the list of IPs and an ipMatchFromDict operator. Here I can see that it would just be about readability instead of relying on a long list of inlined IPs.

@jptosso
Copy link
Member

jptosso commented Aug 31, 2022

Could we mark this as solved with the new SecDataset operator? #361

@fzipi
Copy link
Member

fzipi commented Sep 7, 2022

@M4tteoP ☝️ ?

@M4tteoP
Copy link
Member Author

M4tteoP commented Sep 8, 2022

Sorry to have kept you waiting, but #393 gave me pause wondering about overlaps in functionalities with SecDataset. In the end, I still see value in SecDataset (e.g. easy way to inline PmFromFile-like rules avoiding the need to recompile the Wasm plugin with the new rules embedded.)
I close it, thank you!

@M4tteoP M4tteoP closed this as completed Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants