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.

Sign up for GitHub

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

feat: allow selectors on *_NAMES collections #1143

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

feat: allow selectors on *_NAMES collections #1143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a5b714e
Emit a parsing time error when attempting to perform a selection on a…
blotus Sep 3, 2024
73a2508
ENV / RESPONSE_ARGS can be selected
blotus Sep 3, 2024
dd38d5f
make collections.NamedCollectionNames implement collection.Keyed
blotus Sep 4, 2024
0e4f805
remove panics in Transaction.GetField()
blotus Sep 4, 2024
8d4e521
change type of *Names collections to collection.Keyed
blotus Sep 4, 2024
21e6aaa
comment
blotus Sep 4, 2024
e84a320
add middleware test case on count of args names
blotus Sep 4, 2024
f26a5ea
reenable tests disabled for debugging
blotus Sep 4, 2024
c6a4048
fix linting error
blotus Sep 4, 2024
77f4159
fix linting error
blotus Sep 4, 2024
280af44
remove debug spew call
blotus Sep 4, 2024
8dbefad
Merge branch 'main' into no-panic-on-non-selectable-col
piyushroshan Sep 18, 2024
5ead3ff
Merge branch 'main' into no-panic-on-non-selectable-col
jcchavezs Oct 2, 2024
279c462
tag more collections as being selectable
blotus Nov 7, 2024
5c8a2e2
only generate code for selectable collections
blotus Nov 7, 2024
e1d83b2
early termination in FindRegex/FindString
blotus Nov 7, 2024
1a8f57c
Merge branch 'main' into no-panic-on-non-selectable-col
blotus Nov 7, 2024
f8e778f
add test for getting non-existing item from named collection
blotus Nov 7, 2024
47be8bb
Merge branch 'main' into no-panic-on-non-selectable-col
jptosso Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions experimental/plugins/plugintypes/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,20 +100,20 @@ type TransactionVariables interface {
RequestHeaders() collection.Map
ResponseHeaders() collection.Map
MultipartName() collection.Map
MatchedVarsNames() collection.Collection
MatchedVarsNames() collection.Keyed
MultipartFilename() collection.Map
MatchedVars() collection.Map
FilesSizes() collection.Map
FilesNames() collection.Map
FilesTmpContent() collection.Map
ResponseHeadersNames() collection.Collection
RequestHeadersNames() collection.Collection
RequestCookiesNames() collection.Collection
ResponseHeadersNames() collection.Keyed
RequestHeadersNames() collection.Keyed
RequestCookiesNames() collection.Keyed
XML() collection.Map
RequestXML() collection.Map
ResponseXML() collection.Map
ArgsNames() collection.Collection
ArgsGetNames() collection.Collection
ArgsPostNames() collection.Collection
ArgsNames() collection.Keyed
ArgsGetNames() collection.Keyed
ArgsPostNames() collection.Keyed
MultipartStrictError() collection.Single
}
7 changes: 7 additions & 0 deletions http/middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,12 @@ func TestHttpServer(t *testing.T) {
expectedStatus: 403,
expectedRespHeadersKeys: expectedBlockingHeaders,
},
"deny based on number of post arguments matching a name": {
reqURI: "/hello?foobar=1&foobar=2",
expectedProto: "HTTP/1.1",
expectedStatus: 403,
expectedRespHeadersKeys: expectedBlockingHeaders,
},
}

logger := debuglog.Default().
Expand All @@ -358,6 +364,7 @@ func TestHttpServer(t *testing.T) {
SecRule RESPONSE_HEADERS:Foo "@pm bar" "id:199,phase:3,deny,t:lowercase,deny, status:401,msg:'Invalid response header',log,auditlog"
SecRule RESPONSE_BODY "@contains password" "id:200, phase:4,deny, status:403,msg:'Invalid response body',log,auditlog"
SecRule REQUEST_URI "/allow_me" "id:9,phase:1,allow,msg:'ALLOWED'"
SecRule &ARGS_GET_NAMES:foobar "@eq 2" "id:11,phase:1,deny, status:403,msg:'Invalid foobar',log,auditlog"
`).WithErrorCallback(errLogger(t)).WithDebugLogger(logger)
if l := tCase.reqBodyLimit; l > 0 {
conf = conf.WithRequestBodyAccess().WithRequestBodyLimit(l).WithRequestBodyInMemoryLimit(l)
Expand Down
38 changes: 35 additions & 3 deletions internal/collections/named.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
c.Map.Reset()
}

func (c *NamedCollection) Names(rv variables.RuleVariable) collection.Collection {
func (c *NamedCollection) Names(rv variables.RuleVariable) collection.Keyed {
return &NamedCollectionNames{
variable: rv,
collection: c,
Expand All @@ -101,11 +101,43 @@
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


for k, data := range c.collection.Map.data {
if !key.MatchString(k) {
continue
}
for _, d := range data {
res = append(res, &corazarules.MatchData{
Variable_: c.variable,
Key_: d.key,
Value_: d.key,
})
}
}
return res
}

func (c *NamedCollectionNames) FindString(key string) []types.MatchData {
panic("selection operator not supported")
var res []types.MatchData

for k, data := range c.collection.Map.data {
if k != key {
continue
}
for _, d := range data {
res = append(res, &corazarules.MatchData{
Variable_: c.variable,
Key_: d.key,
Value_: d.key,
})
}
}
return res
}

func (c *NamedCollectionNames) Get(key string) []string {
return c.collection.Map.Get(key)

Check warning on line 140 in internal/collections/named.go

View check run for this annotation

Codecov / codecov/patch

internal/collections/named.go#L139-L140 

Added lines #L139 - L140 were not covered by tests
}

func (c *NamedCollectionNames) FindAll() []types.MatchData {
Expand Down
36 changes: 36 additions & 0 deletions internal/collections/named_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,39 @@ func TestNamedCollection(t *testing.T) {
}

}

func TestNames(t *testing.T) {
c := NewNamedCollection(variables.ArgsPost)
if c.Name() != "ARGS_POST" {
t.Error("Error getting name")
}

c.SetIndex("key", 1, "value")
c.Set("key2", []string{"value2", "value3"})

names := c.Names(variables.ArgsPostNames)

r := names.FindString("key2")

if len(r) != 2 {
t.Errorf("Error finding string, got %d instead of 2", len(r))
}

r = names.FindString("nonexistent")

if len(r) != 0 {
t.Errorf("Error finding nonexistent, got %d instead of 0", len(r))
}

r = names.FindRegex(regexp.MustCompile("key.*"))

if len(r) != 3 {
t.Errorf("Error finding regex, got %d instead of 3", len(r))
}

r = names.FindRegex(regexp.MustCompile("nonexistent"))

if len(r) != 0 {
t.Errorf("Error finding nonexistent regex, got %d instead of 0", len(r))
}
}
36 changes: 19 additions & 17 deletions internal/corazawaf/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,13 +592,15 @@
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")

Check warning on line 596 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L595-L596 

Added lines #L595 - L596 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
case rv.KeyStr != "":
if m, ok := col.(collection.Keyed); ok {
matches = m.FindString(rv.KeyStr)
} else {
panic("attempted to use string 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 string with non-selectable collection")

Check warning on line 603 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L602-L603 

Added lines #L602 - L603 were not covered by tests
}
default:
matches = col.FindAll()
Expand Down Expand Up @@ -1633,11 +1635,11 @@
args *collections.ConcatKeyed
argsCombinedSize *collections.SizeCollection
argsGet *collections.NamedCollection
argsGetNames collection.Collection
argsNames *collections.ConcatCollection
argsGetNames collection.Keyed
argsNames *collections.ConcatKeyed
argsPath *collections.NamedCollection
argsPost *collections.NamedCollection
argsPostNames collection.Collection
argsPostNames collection.Keyed
duration *collections.Single
env *collections.Map
files *collections.Map
Expand All @@ -1653,7 +1655,7 @@
matchedVar *collections.Single
matchedVarName *collections.Single
matchedVars *collections.NamedCollection
matchedVarsNames collection.Collection
matchedVarsNames collection.Keyed
multipartDataAfter *collections.Single
multipartFilename *collections.Map
multipartName *collections.Map
Expand All @@ -1673,10 +1675,10 @@
requestBody *collections.Single
requestBodyLength *collections.Single
requestCookies *collections.NamedCollection
requestCookiesNames collection.Collection
requestCookiesNames collection.Keyed
requestFilename *collections.Single
requestHeaders *collections.NamedCollection
requestHeadersNames collection.Collection
requestHeadersNames collection.Keyed
requestLine *collections.Single
requestMethod *collections.Single
requestProtocol *collections.Single
Expand All @@ -1687,7 +1689,7 @@
responseContentLength *collections.Single
responseContentType *collections.Single
responseHeaders *collections.NamedCollection
responseHeadersNames collection.Collection
responseHeadersNames collection.Keyed
responseProtocol *collections.Single
responseStatus *collections.Single
responseXML *collections.Map
Expand Down Expand Up @@ -1819,7 +1821,7 @@
v.argsPost,
v.argsPath,
)
v.argsNames = collections.NewConcatCollection(
v.argsNames = collections.NewConcatKeyed(
variables.ArgsNames,
v.argsGetNames,
v.argsPostNames,
Expand Down Expand Up @@ -2045,7 +2047,7 @@
return v.multipartName
}

func (v *TransactionVariables) MatchedVarsNames() collection.Collection {
func (v *TransactionVariables) MatchedVarsNames() collection.Keyed {

Check warning on line 2050 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2050 

Added line #L2050 was not covered by tests
return v.matchedVarsNames
}

Expand All @@ -2069,19 +2071,19 @@
return v.filesTmpContent
}

func (v *TransactionVariables) ResponseHeadersNames() collection.Collection {
func (v *TransactionVariables) ResponseHeadersNames() collection.Keyed {

Check warning on line 2074 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2074 

Added line #L2074 was not covered by tests
return v.responseHeadersNames
}

func (v *TransactionVariables) ResponseArgs() collection.Map {
return v.responseArgs
}

func (v *TransactionVariables) RequestHeadersNames() collection.Collection {
func (v *TransactionVariables) RequestHeadersNames() collection.Keyed {

Check warning on line 2082 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2082 

Added line #L2082 was not covered by tests
return v.requestHeadersNames
}

func (v *TransactionVariables) RequestCookiesNames() collection.Collection {
func (v *TransactionVariables) RequestCookiesNames() collection.Keyed {

Check warning on line 2086 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2086 

Added line #L2086 was not covered by tests
return v.requestCookiesNames
}

Expand All @@ -2101,15 +2103,15 @@
return v.resBodyProcessor
}

func (v *TransactionVariables) ArgsNames() collection.Collection {
func (v *TransactionVariables) ArgsNames() collection.Keyed {

Check warning on line 2106 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2106 

Added line #L2106 was not covered by tests
return v.argsNames
}

func (v *TransactionVariables) ArgsGetNames() collection.Collection {
func (v *TransactionVariables) ArgsGetNames() collection.Keyed {

Check warning on line 2110 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2110 

Added line #L2110 was not covered by tests
return v.argsGetNames
}

func (v *TransactionVariables) ArgsPostNames() collection.Collection {
func (v *TransactionVariables) ArgsPostNames() collection.Keyed {

Check warning on line 2114 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2114 

Added line #L2114 was not covered by tests
return v.argsPostNames
}

Expand Down
3 changes: 3 additions & 0 deletions internal/seclang/rule_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ func (rp *RuleParser) ParseVariables(vars string) error {
if err != nil {
return err
}
if curr == 1 && !v.CanBeSelected() {
return fmt.Errorf("attempting to select a value inside a non-selectable collection: %s", string(curVar))
}
// fmt.Printf("(PREVIOUS %s) %s:%s (%t %t)\n", vars, curvar, curkey, iscount, isnegation)
if isquoted {
// if it is quoted we remove the last quote
Expand Down
11 changes: 11 additions & 0 deletions internal/seclang/rule_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,17 @@ func TestParseRule(t *testing.T) {
}
}

func TestNonSelectableCollection(t *testing.T) {
waf := corazawaf.NewWAF()
p := NewParser(waf)
err := p.FromString(`
SecRule REQUEST_URI:foo "bar" "id:1,phase:1"
`)
if err == nil {
t.Error("expected error")
}
}

func BenchmarkParseActions(b *testing.B) {
actionsToBeParsed := "id:980170,phase:5,pass,t:none,noauditlog,msg:'Anomaly Scores:Inbound Scores - Outbound Scores',tag:test"
for i := 0; i < b.N; i++ {
Expand Down
19 changes: 15 additions & 4 deletions internal/variables/generator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ import (
var variablesMapTmpl string

type VariablesMap struct {
Key string
Value string
Key string
Value string
CanBeSelected bool
}

func main() {
Expand Down Expand Up @@ -74,9 +75,19 @@ func main() {
value = "FILES_TMPNAMES"
}

canBeSelected := false
if v.Comment != nil {
for _, c := range v.Comment.List {
if strings.Contains(c.Text, "CanBeSelected") {
canBeSelected = true
}
}
}

directives = append(directives, VariablesMap{
Key: name.String(),
Value: value,
Key: name.String(),
Value: value,
CanBeSelected: canBeSelected,
})
}
}
Expand Down
14 changes: 14 additions & 0 deletions internal/variables/generator/variablesmap.go.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,20 @@ func (v RuleVariable) Name() string {
}
}

// CanBeSelected returns true if the variable supports selection (ie, `:foobar`)
func (v RuleVariable) CanBeSelected() bool {
switch v {
{{- range . }}
{{- if .CanBeSelected }}
case {{ .Key }}:
return true
{{- end }}
{{- end }}
default:
return false
}
}

var rulemapRev = map[string]RuleVariable{
{{range .}}"{{ .Value }}": {{ .Key }},
{{end}}
Expand Down
Loading
Loading