feat: allow selectors on `*_NAMES` collections #1143

blotus · 2024-09-04T12:32:26Z

Hello,

This PR aims to allow the use of rules such as SecRule &REQUEST_COOKIES_NAMES:JSESSIONID "@eq 0" "id:45" (supported by ModSecurity and also present as an example in the documentation of Coraza), which currently causes Coraza to crash due to an explicit panic call.

There are 3 main changes:

Check if a collection supports selectors during parsing time, instead of throwing an error at runtime.
Make collections.NamedCollectionNames implements collection.Keyed: this allows the use of a selector for the collections created with .Names()
Remove runtime panics calls: as Coraza is designed to be embedded in other software, calling panic is never a good idea.

Parser changes

I've embedded information about whether a collection can be selected or not in the internal/variables/variables.go file, as a comment for each collection that does support it (hopefully, I did not miss any), and added a CanBeSelected method that is called during parsing to check if the selector is allowed or not.

I don't know if I'm really happy with embedding information in comments, but it was the least intrusive way I found to handle this.

`collections.NamedCollectionNames` implements `collection.Keyed`

This one is straightforward, NamedCollectionNames now implements Get, FindString and FindRegex.
Because it's a named collection, the key and the value in the returned results will be the same: the name of the key.

Remove runtime `panic`

The first two were removed as part of making namedCollectionNames implements Keyed.

The other two (which are the ones that caused the crash mentioned at the beginning of this PR) have been replaced by an error log.
In theory, this log should never occur because selectability is now checked during parsing (in practice, it could happen if a collection is marked as selectable but does not implement Keyed).

… non-selectable collection

codecov · 2024-09-04T12:35:37Z

Codecov Report

Attention: Patch coverage is 59.37500% with 39 lines in your changes missing coverage. Please review.

Project coverage is 81.55%. Comparing base (cbf0aa7) to head (47be8bb).

Files with missing lines	Patch %	Lines
internal/variables/variablesmap.gen.go	50.00%	26 Missing ⚠️
internal/corazawaf/transaction.go	8.33%	11 Missing ⚠️
internal/collections/named.go	93.10%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1143      +/-   ##
==========================================
- Coverage   81.68%   81.55%   -0.13%     
==========================================
  Files         168      168              
  Lines        9762     9847      +85     
==========================================
+ Hits         7974     8031      +57     
- Misses       1537     1565      +28     
  Partials      251      251

Flag	Coverage Δ
coraza.rule.case_sensitive_args_keys	`81.51% <59.37%> (-0.13%)`	⬇️
coraza.rule.multiphase_valuation	`81.55% <59.37%> (-0.13%)`	⬇️
coraza.rule.no_regex_multiline	`81.49% <59.37%> (-0.13%)`	⬇️
default	`81.55% <59.37%> (-0.13%)`	⬇️
examples+	`16.34% <2.08%> (-0.22%)`	⬇️
examples+coraza.rule.case_sensitive_args_keys	`81.51% <59.37%> (-0.13%)`	⬇️
examples+coraza.rule.multiphase_valuation	`81.39% <59.37%> (-0.13%)`	⬇️
examples+coraza.rule.no_regex_multiline	`81.41% <59.37%> (-0.13%)`	⬇️
examples+memoize_builders	`81.51% <59.37%> (-0.13%)`	⬇️
examples+no_fs_access	`80.84% <59.37%> (-0.13%)`	⬇️
ftw	`81.55% <59.37%> (-0.13%)`	⬇️
memoize_builders	`81.64% <59.37%> (-0.13%)`	⬇️
no_fs_access	`81.01% <59.37%> (-0.13%)`	⬇️
tinygo	`81.52% <59.37%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jptosso · 2024-09-04T13:51:19Z

Interesting, thank you very much for your contribution

Im a bit worried about how the complexity of variables is growing. Maybe not for this PR, but we need to improve generation of code, even for this "selectable" feature

fzipi · 2024-09-08T22:22:45Z

internal/variables/variablesmap.gen.go

+// CanBeSelected returns true if the variable supports selection (ie, `:foobar`)
+func (v RuleVariable) CanBeSelected() bool {
+	switch v {


I don't think adding everything here makes sense. Just return true on those who can, and use the default otherwise.

Although for performance it makes sense, I believe this is easier to maintain and more readable

This is auto generated so I would not be concern about readability. @blotus could you do a quick benchmark on this matter i.e. adding everything or just true and all the rest on a default?

I agree that readability does not really matter here.
I've updated the code generation to only generate the cases where true is returned.

fzipi · 2024-09-08T22:26:08Z

Im a bit worried about how the complexity of variables is growing. Maybe not for this PR, but we need to improve generation of code, even for this "selectable" feature

Definitely not for this PR. We should create an issue to refactor generation then.

jptosso · 2024-09-18T16:26:21Z

LGTM in general, but I believe this lacks negative tests and its decreasing the general project coverage

jcchavezs · 2024-09-23T09:51:59Z

internal/collections/named.go

@@ -101,11 +101,41 @@ type NamedCollectionNames struct {
 }

 func (c *NamedCollectionNames) FindRegex(key *regexp.Regexp) []types.MatchData {
-	panic("selection operator not supported")
+	var res []types.MatchData


Is there any chance data is empty? if so I would handle the empty case before this allocation.

Yes, there are probably situations where data can be empty (I haven't tested, but I'd expect a collection like XML to have an empty data on a non-XML request )

AFAIK, declaring a slice like this does not perform any actual allocation (other than the header of the slice, which will be all zero), and the actual allocation will be performed the first time we append to it, but I can add a check on data if you are worried about it.

Also, from what I see, this check is not performed in the existing code (here for example)

internal/collections/named.go

jcchavezs · 2024-09-23T09:53:34Z

internal/collections/named.go

+	for k, data := range c.collection.Map.data {
+		if key.MatchString(k) {
+			for _, d := range data {
+				res = append(res, &corazarules.MatchData{


I wonder if MatchData is mutable, if so we probably want to reuse the pointer?

internal/variables/generator/variablesmap.go.tmpl

internal/collections/named.go

jptosso · 2024-11-13T09:14:35Z

internal/corazawaf/transaction.go

@@ -574,13 +574,15 @@ func (tx *Transaction) GetField(rv ruleVariableParams) []types.MatchData {
 		if m, ok := col.(collection.Keyed); ok {
 			matches = m.FindRegex(rv.KeyRx)
 		} else {
-			panic("attempted to use regex with non-selectable collection: " + rv.Variable.Name())
+			// This should probably never happen, selectability is checked at parsing time
+			tx.debugLogger.Error().Str("collection", rv.Variable.Name()).Msg("attempted to use regex with non-selectable collection")


We discussed time ago that panic is ok, as this is a low level issue and coraza should not run here

I'm not sure I agree with this: coraza is designed as a library, and in my point of view, this means that explicit panics must be avoided at all costs (with very little exceptions, if you can call panic, you can return an error), and not doing anything is almost always better than bringing down a production website.

If a function call can lead to a panic, it should be made very clear to the caller (either with an explicit function name (Must....) or, at the very least, with some documentation): I don't mind wrapping every call to coraza with a recover, but I need to be aware it's required.

For this specific case, it can only (AFAIK) be triggered by a configuration error, so this means it should be detected when parsing the configuration (and is now thanks to this PR), so the panic has become redundant.

blotus added 7 commits September 3, 2024 13:25

Emit a parsing time error when attempting to perform a selection on a…

a5b714e

… non-selectable collection

ENV / RESPONSE_ARGS can be selected

73a2508

make collections.NamedCollectionNames implement collection.Keyed

dd38d5f

remove panics in Transaction.GetField()

0e4f805

change type of *Names collections to collection.Keyed

8d4e521

comment

21e6aaa

add middleware test case on count of args names

e84a320

blotus requested a review from a team as a code owner September 4, 2024 12:32

blotus added 4 commits September 4, 2024 14:45

reenable tests disabled for debugging

f26a5ea

fix linting error

c6a4048

fix linting error

77f4159

remove debug spew call

280af44

fzipi reviewed Sep 8, 2024

View reviewed changes

fzipi changed the title ~~Allow selectors on *_NAMES collections~~ feat: allow selectors on *_NAMES collections Sep 8, 2024

Merge branch 'main' into no-panic-on-non-selectable-col

8dbefad

jcchavezs reviewed Sep 23, 2024

View reviewed changes

internal/collections/named.go Outdated Show resolved Hide resolved

jcchavezs reviewed Sep 23, 2024

View reviewed changes

internal/variables/generator/variablesmap.go.tmpl Outdated Show resolved Hide resolved

LaurenceJJones reviewed Oct 1, 2024

View reviewed changes

internal/collections/named.go Outdated Show resolved Hide resolved

jcchavezs and others added 6 commits October 2, 2024 15:11

Merge branch 'main' into no-panic-on-non-selectable-col

5ead3ff

tag more collections as being selectable

279c462

only generate code for selectable collections

5c8a2e2

early termination in FindRegex/FindString

e1d83b2

Merge branch 'main' into no-panic-on-non-selectable-col

1a8f57c

add test for getting non-existing item from named collection

f8e778f

jptosso reviewed Nov 13, 2024

View reviewed changes

Merge branch 'main' into no-panic-on-non-selectable-col

47be8bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow selectors on `*_NAMES` collections #1143

feat: allow selectors on `*_NAMES` collections #1143

blotus commented Sep 4, 2024

codecov bot commented Sep 4, 2024 •

edited

Loading

jptosso commented Sep 4, 2024

fzipi Sep 8, 2024

jptosso Sep 18, 2024

jcchavezs Sep 23, 2024

blotus Nov 7, 2024

fzipi commented Sep 8, 2024

jptosso commented Sep 18, 2024

jcchavezs Sep 23, 2024

blotus Nov 7, 2024

blotus Nov 7, 2024

jcchavezs Sep 23, 2024

jptosso Nov 13, 2024

blotus Nov 13, 2024

feat: allow selectors on *_NAMES collections #1143

Are you sure you want to change the base?

feat: allow selectors on *_NAMES collections #1143

Conversation

blotus commented Sep 4, 2024

Parser changes

collections.NamedCollectionNames implements collection.Keyed

Remove runtime panic

codecov bot commented Sep 4, 2024 • edited Loading

Codecov Report

jptosso commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzipi commented Sep 8, 2024

jptosso commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: allow selectors on `*_NAMES` collections #1143

feat: allow selectors on `*_NAMES` collections #1143

`collections.NamedCollectionNames` implements `collection.Keyed`

Remove runtime `panic`

codecov bot commented Sep 4, 2024 •

edited

Loading