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

Jump to bottom

feat: allow selectors on *_NAMES collections #1143

Open
wants to merge 18 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 11 commits
Commits
Show all changes
18 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
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
36 changes: 33 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,41 @@
}

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) {
blotus marked this conversation as resolved.
Show resolved Hide resolved
for _, d := range data {
res = append(res, &corazarules.MatchData{
Copy link
Member

Choose a reason for hiding this comment

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

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

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 {
blotus marked this conversation as resolved.
Show resolved Hide resolved
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 138 in internal/collections/named.go

View check run for this annotation

Codecov / codecov/patch

internal/collections/named.go#L137-L138 

Added lines #L137 - L138 were not covered by tests
}

func (c *NamedCollectionNames) FindAll() []types.MatchData {
Expand Down
24 changes: 24 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,27 @@ 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.FindRegex(regexp.MustCompile("key.*"))

if len(r) != 3 {
t.Errorf("Error finding regex, got %d instead of 3", 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 @@ -571,13 +571,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 575 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L575 

Added line #L575 was 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 582 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L582 

Added line #L582 was not covered by tests
}
default:
matches = col.FindAll()
Expand Down Expand Up @@ -1570,11 +1572,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 @@ -1590,7 +1592,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 @@ -1610,10 +1612,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 @@ -1624,7 +1626,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 @@ -1738,7 +1740,7 @@
v.argsPost,
v.argsPath,
)
v.argsNames = collections.NewConcatCollection(
v.argsNames = collections.NewConcatKeyed(
variables.ArgsNames,
v.argsGetNames,
v.argsPostNames,
Expand Down Expand Up @@ -1964,7 +1966,7 @@
return v.multipartName
}

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L1969 

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

Expand All @@ -1988,19 +1990,19 @@
return v.filesTmpContent
}

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L1993 

Added line #L1993 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 2001 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2001 

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2005 

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

Expand All @@ -2020,15 +2022,15 @@
return v.resBodyProcessor
}

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2025 

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2029 

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2033 

Added line #L2033 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
11 changes: 11 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,17 @@ func (v RuleVariable) Name() string {
}
}

//CanBeSelected returns true if the variable supports selection (ie, `:foobar`)
blotus marked this conversation as resolved.
Show resolved Hide resolved
func (v RuleVariable) CanBeSelected() bool {
switch v {
{{range .}}case {{ .Key }}:
return {{ .CanBeSelected }}
{{end}}
default:
return false
}
}

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