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 12 commits into
base: main
Choose a base branch
from
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.


for k, data := range c.collection.Map.data {
if key.MatchString(k) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use early termination to reduce the indentation.

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 {
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
}
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`)
Copy link
Member

@jcchavezs jcchavezs Sep 23, 2024

Choose a reason for hiding this comment

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

Suggested change
//CanBeSelected returns true if the variable supports selection (ie, `:foobar`)
// CanBeSelected returns true if the variable supports selection (e.g. `:foobar`)

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