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

feat: allow selectors on *_NAMES collections #1143

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -572,13 +572,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 576 in internal/corazawaf/transaction.go

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L576

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L583

Added line #L583 was not covered by tests
}
default:
matches = col.FindAll()
Expand Down Expand Up @@ -1586,11 +1588,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 @@ -1606,7 +1608,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 @@ -1626,10 +1628,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 @@ -1640,7 +1642,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 @@ -1754,7 +1756,7 @@
v.argsPost,
v.argsPath,
)
v.argsNames = collections.NewConcatCollection(
v.argsNames = collections.NewConcatKeyed(
variables.ArgsNames,
v.argsGetNames,
v.argsPostNames,
Expand Down Expand Up @@ -1980,7 +1982,7 @@
return v.multipartName
}

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L1985

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

Expand All @@ -2004,19 +2006,19 @@
return v.filesTmpContent
}

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2009

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2017

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2021

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

Expand All @@ -2036,15 +2038,15 @@
return v.resBodyProcessor
}

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2041

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2045

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/corazawaf/transaction.go#L2049

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

Expand Down
3 changes: 3 additions & 0 deletions internal/seclang/rule_parser.go
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
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
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
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