From a5b714ead0e76db4dd092f4e895dcfefbad9f8ef Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Tue, 3 Sep 2024 13:25:16 +0200 Subject: [PATCH 01/16] Emit a parsing time error when attempting to perform a selection on a non-selectable collection --- internal/seclang/rule_parser.go | 3 + internal/seclang/rule_parser_test.go | 11 + internal/variables/generator/main.go | 19 +- .../variables/generator/variablesmap.go.tmpl | 11 + internal/variables/variables.go | 44 ++-- internal/variables/variablesmap.gen.go | 201 ++++++++++++++++++ 6 files changed, 263 insertions(+), 26 deletions(-) diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go index 77b61ab99..61f8a762e 100644 --- a/internal/seclang/rule_parser.go +++ b/internal/seclang/rule_parser.go @@ -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 diff --git a/internal/seclang/rule_parser_test.go b/internal/seclang/rule_parser_test.go index fe30e1230..f2ff3701f 100644 --- a/internal/seclang/rule_parser_test.go +++ b/internal/seclang/rule_parser_test.go @@ -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++ { diff --git a/internal/variables/generator/main.go b/internal/variables/generator/main.go index 07fb0d7b2..f17017c48 100644 --- a/internal/variables/generator/main.go +++ b/internal/variables/generator/main.go @@ -23,8 +23,9 @@ import ( var variablesMapTmpl string type VariablesMap struct { - Key string - Value string + Key string + Value string + CanBeSelected bool } func main() { @@ -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, }) } } diff --git a/internal/variables/generator/variablesmap.go.tmpl b/internal/variables/generator/variablesmap.go.tmpl index 3b1e09355..95ed7f2ce 100644 --- a/internal/variables/generator/variablesmap.go.tmpl +++ b/internal/variables/generator/variablesmap.go.tmpl @@ -22,6 +22,17 @@ func (v RuleVariable) Name() string { } } +//CanBeSelected returns true if the variable supports selection (ie, `: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}} diff --git a/internal/variables/variables.go b/internal/variables/variables.go index 717b18f8b..9a8e9610f 100644 --- a/internal/variables/variables.go +++ b/internal/variables/variables.go @@ -112,21 +112,21 @@ const ( // the beginning of the transaction until this point Duration // ResponseHeadersNames contains the names of the response headers - ResponseHeadersNames + ResponseHeadersNames //CanBeSelected // RequestHeadersNames contains the names of the request headers - RequestHeadersNames + RequestHeadersNames //CanBeSelected // Args contains copies of ArgsGet and ArgsPost - Args + Args //CanBeSelected // ArgsGet contains the GET (URL) arguments - ArgsGet + ArgsGet //CanBeSelected // ArgsPost contains the POST (BODY) arguments - ArgsPost + ArgsPost //CanBeSelected // ArgsPath contains the url path parts ArgsPath // FilesSizes contains the sizes of the uploaded files FilesSizes // FilesNames contains the names of the uploaded files - FilesNames + FilesNames //CanBeSelected // FilesTmpContent is not supported FilesTmpContent // MultipartFilename contains the multipart data from field FILENAME @@ -135,43 +135,43 @@ const ( MultipartName // MatchedVarsNames is similar to MATCHED_VAR_NAME except that it is // a collection of all matches for the current operator check. - MatchedVarsNames + MatchedVarsNames //CanBeSelected // MatchedVars is similar to MATCHED_VAR except that it is a collection // of all matches for the current operator check - MatchedVars + MatchedVars //CanBeSelected // Files contains a collection of original file names // (as they were called on the remote user’s filesys- tem). // Available only on inspected multipart/form-data requests. Files // RequestCookies is a collection of all of request cookies (values only - RequestCookies + RequestCookies //CanBeSelected // RequestHeaders can be used as either a collection of all of the request // headers or can be used to inspect selected headers - RequestHeaders + RequestHeaders //CanBeSelected // ResponseHeaders can be used as either a collection of all of the response // headers or can be used to inspect selected headers - ResponseHeaders + ResponseHeaders //CanBeSelected // ReseBodyProcessor contains the name of the response body processor used, // no default ResBodyProcessor // Geo contains the location information of the client Geo // RequestCookiesNames contains the names of the request cookies - RequestCookiesNames + RequestCookiesNames //CanBeSelected // FilesTmpNames contains the names of the uploaded temporal files - FilesTmpNames + FilesTmpNames //CanBeSelected // ArgsNames contains the names of the arguments (POST and GET) - ArgsNames + ArgsNames //CanBeSelected // ArgsGetNames contains the names of the GET arguments - ArgsGetNames + ArgsGetNames //CanBeSelected // ArgsPostNames contains the names of the POST arguments - ArgsPostNames + ArgsPostNames //CanBeSelected // TX contains transaction specific variables created with setvar - TX + TX //CanBeSelected // Rule contains rule metadata Rule // JSON does not provide any data, might be removed - JSON + JSON //CanBeSelected // Env contains the process environment variables Env // UrlencodedError equals 1 if we failed to parse de URL @@ -180,13 +180,13 @@ const ( // ResponseArgs contains the response parsed arguments ResponseArgs // ResponseXML contains the response parsed XML - ResponseXML + ResponseXML //CanBeSelected // RequestXML contains the request parsed XML - RequestXML + RequestXML //CanBeSelected // XML is a pointer to ResponseXML - XML + XML //CanBeSelected // MultipartPartHeaders contains the multipart headers - MultipartPartHeaders + MultipartPartHeaders //CanBeSelected // Unsupported variables diff --git a/internal/variables/variablesmap.gen.go b/internal/variables/variablesmap.gen.go index 8b707aced..9f6707ad4 100644 --- a/internal/variables/variablesmap.gen.go +++ b/internal/variables/variablesmap.gen.go @@ -212,6 +212,207 @@ func (v RuleVariable) Name() string { } } +// CanBeSelected returns true if the variable supports selection (ie, `:foobar`) +func (v RuleVariable) CanBeSelected() bool { + switch v { + case Unknown: + return false + case ResponseContentType: + return false + case UniqueID: + return false + case ArgsCombinedSize: + return false + case FilesCombinedSize: + return false + case FullRequestLength: + return false + case InboundDataError: + return false + case MatchedVar: + return false + case MatchedVarName: + return false + case MultipartDataAfter: + return false + case OutboundDataError: + return false + case QueryString: + return false + case RemoteAddr: + return false + case RemoteHost: + return false + case RemotePort: + return false + case ReqbodyError: + return false + case ReqbodyErrorMsg: + return false + case ReqbodyProcessorError: + return false + case ReqbodyProcessorErrorMsg: + return false + case ReqbodyProcessor: + return false + case RequestBasename: + return false + case RequestBody: + return false + case RequestBodyLength: + return false + case RequestFilename: + return false + case RequestLine: + return false + case RequestMethod: + return false + case RequestProtocol: + return false + case RequestURI: + return false + case RequestURIRaw: + return false + case ResponseBody: + return false + case ResponseContentLength: + return false + case ResponseProtocol: + return false + case ResponseStatus: + return false + case ServerAddr: + return false + case ServerName: + return false + case ServerPort: + return false + case HighestSeverity: + return false + case StatusLine: + return false + case Duration: + return false + case ResponseHeadersNames: + return true + case RequestHeadersNames: + return true + case Args: + return true + case ArgsGet: + return true + case ArgsPost: + return true + case ArgsPath: + return false + case FilesSizes: + return false + case FilesNames: + return true + case FilesTmpContent: + return false + case MultipartFilename: + return false + case MultipartName: + return false + case MatchedVarsNames: + return true + case MatchedVars: + return true + case Files: + return false + case RequestCookies: + return true + case RequestHeaders: + return true + case ResponseHeaders: + return true + case ResBodyProcessor: + return false + case Geo: + return false + case RequestCookiesNames: + return true + case FilesTmpNames: + return true + case ArgsNames: + return true + case ArgsGetNames: + return true + case ArgsPostNames: + return true + case TX: + return true + case Rule: + return false + case JSON: + return true + case Env: + return false + case UrlencodedError: + return false + case ResponseArgs: + return false + case ResponseXML: + return true + case RequestXML: + return true + case XML: + return true + case MultipartPartHeaders: + return true + case AuthType: + return false + case FullRequest: + return false + case MultipartBoundaryQuoted: + return false + case MultipartBoundaryWhitespace: + return false + case MultipartCrlfLfLines: + return false + case MultipartDataBefore: + return false + case MultipartFileLimitExceeded: + return false + case MultipartHeaderFolding: + return false + case MultipartInvalidHeaderFolding: + return false + case MultipartInvalidPart: + return false + case MultipartInvalidQuoting: + return false + case MultipartLfLine: + return false + case MultipartMissingSemicolon: + return false + case MultipartStrictError: + return false + case MultipartUnmatchedBoundary: + return false + case PathInfo: + return false + case Sessionid: + return false + case Userid: + return false + case IP: + return false + case ResBodyError: + return false + case ResBodyErrorMsg: + return false + case ResBodyProcessorError: + return false + case ResBodyProcessorErrorMsg: + return false + + default: + return false + } +} + var rulemapRev = map[string]RuleVariable{ "UNKNOWN": Unknown, "RESPONSE_CONTENT_TYPE": ResponseContentType, From 73a25089ad74b7c4808a1234280677c5e229e961 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Tue, 3 Sep 2024 13:31:04 +0200 Subject: [PATCH 02/16] ENV / RESPONSE_ARGS can be selected --- internal/variables/variables.go | 4 ++-- internal/variables/variablesmap.gen.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/variables/variables.go b/internal/variables/variables.go index 9a8e9610f..1b7394750 100644 --- a/internal/variables/variables.go +++ b/internal/variables/variables.go @@ -173,12 +173,12 @@ const ( // JSON does not provide any data, might be removed JSON //CanBeSelected // Env contains the process environment variables - Env + Env //CanBeSelected // UrlencodedError equals 1 if we failed to parse de URL // It applies for URL query part and urlencoded post body UrlencodedError // ResponseArgs contains the response parsed arguments - ResponseArgs + ResponseArgs //CanBeSelected // ResponseXML contains the response parsed XML ResponseXML //CanBeSelected // RequestXML contains the request parsed XML diff --git a/internal/variables/variablesmap.gen.go b/internal/variables/variablesmap.gen.go index 9f6707ad4..dc5d3c3cc 100644 --- a/internal/variables/variablesmap.gen.go +++ b/internal/variables/variablesmap.gen.go @@ -348,11 +348,11 @@ func (v RuleVariable) CanBeSelected() bool { case JSON: return true case Env: - return false + return true case UrlencodedError: return false case ResponseArgs: - return false + return true case ResponseXML: return true case RequestXML: From dd38d5f07dee40dab1773adc0a58c10b7ec92146 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 13:02:59 +0200 Subject: [PATCH 03/16] make collections.NamedCollectionNames implement collection.Keyed --- internal/collections/named.go | 36 +++++++++++++++++++++++++++--- internal/collections/named_test.go | 27 ++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/internal/collections/named.go b/internal/collections/named.go index 88d9166fd..c6d9c55e6 100644 --- a/internal/collections/named.go +++ b/internal/collections/named.go @@ -80,7 +80,7 @@ func (c *NamedCollection) Reset() { 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, @@ -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 + + for k, data := range c.collection.Map.data { + if key.MatchString(k) { + 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 { + 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) } func (c *NamedCollectionNames) FindAll() []types.MatchData { diff --git a/internal/collections/named_test.go b/internal/collections/named_test.go index 0a773b4d4..e65d138cf 100644 --- a/internal/collections/named_test.go +++ b/internal/collections/named_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/corazawaf/coraza/v3/types/variables" + "github.com/davecgh/go-spew/spew" ) func TestNamedCollection(t *testing.T) { @@ -54,6 +55,8 @@ func TestNamedCollection(t *testing.T) { t.Errorf("want %q, have %q", want, have) } + spew.Dump(names) + assertUnorderedValuesMatch(t, names.FindAll(), "key", "key2") if want, have := "ARGS_POST_NAMES: key,key2", fmt.Sprint(names); want != have { if want := "ARGS_POST_NAMES: key2,key"; want != have { @@ -87,3 +90,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)) + } +} From 0e4f805787f71896c5b95d1bbb48d376ede73eaa Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 13:13:10 +0200 Subject: [PATCH 04/16] remove panics in Transaction.GetField() --- internal/corazawaf/transaction.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 880c6220c..9cf3bd74d 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -571,13 +571,13 @@ 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()) + tx.debugLogger.Error().Str("collection", rv.Variable.Name()).Msg("attempted to use regex with non-selectable collection") } 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()) + tx.debugLogger.Error().Str("collection", rv.Variable.Name()).Msg("attempted to use string with non-selectable collection") } default: matches = col.FindAll() From 8d4e521fe9720abc50cd4c3a7c91ee284eaeb0c3 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 13:20:06 +0200 Subject: [PATCH 05/16] change type of *Names collections to collection.Keyed --- .../plugins/plugintypes/transaction.go | 14 ++++----- internal/corazawaf/transaction.go | 30 +++++++++---------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/experimental/plugins/plugintypes/transaction.go b/experimental/plugins/plugintypes/transaction.go index a34a4732a..092030d0c 100644 --- a/experimental/plugins/plugintypes/transaction.go +++ b/experimental/plugins/plugintypes/transaction.go @@ -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 } diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 9cf3bd74d..cb39b9507 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -1570,11 +1570,11 @@ type TransactionVariables struct { 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 @@ -1590,7 +1590,7 @@ type TransactionVariables struct { matchedVar *collections.Single matchedVarName *collections.Single matchedVars *collections.NamedCollection - matchedVarsNames collection.Collection + matchedVarsNames collection.Keyed multipartDataAfter *collections.Single multipartFilename *collections.Map multipartName *collections.Map @@ -1610,10 +1610,10 @@ type TransactionVariables struct { 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 @@ -1624,7 +1624,7 @@ type TransactionVariables struct { responseContentLength *collections.Single responseContentType *collections.Single responseHeaders *collections.NamedCollection - responseHeadersNames collection.Collection + responseHeadersNames collection.Keyed responseProtocol *collections.Single responseStatus *collections.Single responseXML *collections.Map @@ -1738,7 +1738,7 @@ func NewTransactionVariables() *TransactionVariables { v.argsPost, v.argsPath, ) - v.argsNames = collections.NewConcatCollection( + v.argsNames = collections.NewConcatKeyed( variables.ArgsNames, v.argsGetNames, v.argsPostNames, @@ -1964,7 +1964,7 @@ func (v *TransactionVariables) MultipartName() collection.Map { return v.multipartName } -func (v *TransactionVariables) MatchedVarsNames() collection.Collection { +func (v *TransactionVariables) MatchedVarsNames() collection.Keyed { return v.matchedVarsNames } @@ -1988,7 +1988,7 @@ func (v *TransactionVariables) FilesTmpContent() collection.Map { return v.filesTmpContent } -func (v *TransactionVariables) ResponseHeadersNames() collection.Collection { +func (v *TransactionVariables) ResponseHeadersNames() collection.Keyed { return v.responseHeadersNames } @@ -1996,11 +1996,11 @@ func (v *TransactionVariables) ResponseArgs() collection.Map { return v.responseArgs } -func (v *TransactionVariables) RequestHeadersNames() collection.Collection { +func (v *TransactionVariables) RequestHeadersNames() collection.Keyed { return v.requestHeadersNames } -func (v *TransactionVariables) RequestCookiesNames() collection.Collection { +func (v *TransactionVariables) RequestCookiesNames() collection.Keyed { return v.requestCookiesNames } @@ -2020,15 +2020,15 @@ func (v *TransactionVariables) ResponseBodyProcessor() collection.Single { return v.resBodyProcessor } -func (v *TransactionVariables) ArgsNames() collection.Collection { +func (v *TransactionVariables) ArgsNames() collection.Keyed { return v.argsNames } -func (v *TransactionVariables) ArgsGetNames() collection.Collection { +func (v *TransactionVariables) ArgsGetNames() collection.Keyed { return v.argsGetNames } -func (v *TransactionVariables) ArgsPostNames() collection.Collection { +func (v *TransactionVariables) ArgsPostNames() collection.Keyed { return v.argsPostNames } From 21e6aaa837235844fb3da8a4b992b55b7b53d45c Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 13:21:05 +0200 Subject: [PATCH 06/16] comment --- internal/corazawaf/transaction.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index cb39b9507..2e9ddeaec 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -571,12 +571,14 @@ func (tx *Transaction) GetField(rv ruleVariableParams) []types.MatchData { if m, ok := col.(collection.Keyed); ok { matches = m.FindRegex(rv.KeyRx) } else { + //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") } case rv.KeyStr != "": if m, ok := col.(collection.Keyed); ok { matches = m.FindString(rv.KeyStr) } else { + //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") } default: From e84a3201d719682e8f546b4793ceb5a5576a9809 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 13:48:30 +0200 Subject: [PATCH 07/16] add middleware test case on count of args names --- http/middleware_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/http/middleware_test.go b/http/middleware_test.go index 249f2f661..19c65971d 100644 --- a/http/middleware_test.go +++ b/http/middleware_test.go @@ -249,7 +249,7 @@ var expectedBlockingHeaders = []string{"Content-Length", "Date"} func TestHttpServer(t *testing.T) { tests := map[string]httpTest{ - "no blocking": { + /*"no blocking": { reqURI: "/hello", expectedProto: "HTTP/1.1", expectedStatus: 201, @@ -331,6 +331,12 @@ func TestHttpServer(t *testing.T) { expectedProto: "HTTP/1.1", 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, }, } @@ -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) From f26a5ea8ca45ac95679af09b245f603a67e33873 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 14:45:11 +0200 Subject: [PATCH 08/16] reenable tests disabled for debugging --- http/middleware_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/middleware_test.go b/http/middleware_test.go index 19c65971d..89ea3f56b 100644 --- a/http/middleware_test.go +++ b/http/middleware_test.go @@ -249,7 +249,7 @@ var expectedBlockingHeaders = []string{"Content-Length", "Date"} func TestHttpServer(t *testing.T) { tests := map[string]httpTest{ - /*"no blocking": { + "no blocking": { reqURI: "/hello", expectedProto: "HTTP/1.1", expectedStatus: 201, @@ -331,7 +331,7 @@ func TestHttpServer(t *testing.T) { expectedProto: "HTTP/1.1", expectedStatus: 403, expectedRespHeadersKeys: expectedBlockingHeaders, - },*/ + }, "deny based on number of post arguments matching a name": { reqURI: "/hello?foobar=1&foobar=2", expectedProto: "HTTP/1.1", From c6a404859130d997b6fd68c0ccb811adef340546 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 14:46:50 +0200 Subject: [PATCH 09/16] fix linting error --- internal/variables/variables.go | 48 ++++++++++++++++----------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/internal/variables/variables.go b/internal/variables/variables.go index 1b7394750..17ee14747 100644 --- a/internal/variables/variables.go +++ b/internal/variables/variables.go @@ -112,21 +112,21 @@ const ( // the beginning of the transaction until this point Duration // ResponseHeadersNames contains the names of the response headers - ResponseHeadersNames //CanBeSelected + ResponseHeadersNames // CanBeSelected // RequestHeadersNames contains the names of the request headers - RequestHeadersNames //CanBeSelected + RequestHeadersNames // CanBeSelected // Args contains copies of ArgsGet and ArgsPost - Args //CanBeSelected + Args // CanBeSelected // ArgsGet contains the GET (URL) arguments - ArgsGet //CanBeSelected + ArgsGet // CanBeSelected // ArgsPost contains the POST (BODY) arguments - ArgsPost //CanBeSelected + ArgsPost // CanBeSelected // ArgsPath contains the url path parts ArgsPath // FilesSizes contains the sizes of the uploaded files FilesSizes // FilesNames contains the names of the uploaded files - FilesNames //CanBeSelected + FilesNames // CanBeSelected // FilesTmpContent is not supported FilesTmpContent // MultipartFilename contains the multipart data from field FILENAME @@ -135,58 +135,58 @@ const ( MultipartName // MatchedVarsNames is similar to MATCHED_VAR_NAME except that it is // a collection of all matches for the current operator check. - MatchedVarsNames //CanBeSelected + MatchedVarsNames // CanBeSelected // MatchedVars is similar to MATCHED_VAR except that it is a collection // of all matches for the current operator check - MatchedVars //CanBeSelected + MatchedVars // CanBeSelected // Files contains a collection of original file names // (as they were called on the remote user’s filesys- tem). // Available only on inspected multipart/form-data requests. Files // RequestCookies is a collection of all of request cookies (values only - RequestCookies //CanBeSelected + RequestCookies // CanBeSelected // RequestHeaders can be used as either a collection of all of the request // headers or can be used to inspect selected headers - RequestHeaders //CanBeSelected + RequestHeaders // CanBeSelected // ResponseHeaders can be used as either a collection of all of the response // headers or can be used to inspect selected headers - ResponseHeaders //CanBeSelected + ResponseHeaders // CanBeSelected // ReseBodyProcessor contains the name of the response body processor used, // no default ResBodyProcessor // Geo contains the location information of the client Geo // RequestCookiesNames contains the names of the request cookies - RequestCookiesNames //CanBeSelected + RequestCookiesNames // CanBeSelected // FilesTmpNames contains the names of the uploaded temporal files - FilesTmpNames //CanBeSelected + FilesTmpNames // CanBeSelected // ArgsNames contains the names of the arguments (POST and GET) - ArgsNames //CanBeSelected + ArgsNames // CanBeSelected // ArgsGetNames contains the names of the GET arguments - ArgsGetNames //CanBeSelected + ArgsGetNames // CanBeSelected // ArgsPostNames contains the names of the POST arguments - ArgsPostNames //CanBeSelected + ArgsPostNames // CanBeSelected // TX contains transaction specific variables created with setvar - TX //CanBeSelected + TX // CanBeSelected // Rule contains rule metadata Rule // JSON does not provide any data, might be removed - JSON //CanBeSelected + JSON // CanBeSelected // Env contains the process environment variables - Env //CanBeSelected + Env // CanBeSelected // UrlencodedError equals 1 if we failed to parse de URL // It applies for URL query part and urlencoded post body UrlencodedError // ResponseArgs contains the response parsed arguments - ResponseArgs //CanBeSelected + ResponseArgs // CanBeSelected // ResponseXML contains the response parsed XML - ResponseXML //CanBeSelected + ResponseXML // CanBeSelected // RequestXML contains the request parsed XML - RequestXML //CanBeSelected + RequestXML // CanBeSelected // XML is a pointer to ResponseXML - XML //CanBeSelected + XML // CanBeSelected // MultipartPartHeaders contains the multipart headers - MultipartPartHeaders //CanBeSelected + MultipartPartHeaders // CanBeSelected // Unsupported variables From 77f4159ccfc2f88e5e9c8cc94cd978bcdbdfc40a Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 14:51:34 +0200 Subject: [PATCH 10/16] fix linting error --- internal/corazawaf/transaction.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 2e9ddeaec..3e8b7e438 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -571,14 +571,14 @@ func (tx *Transaction) GetField(rv ruleVariableParams) []types.MatchData { if m, ok := col.(collection.Keyed); ok { matches = m.FindRegex(rv.KeyRx) } else { - //This should probably never happen, selectability is checked at parsing time + // 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") } case rv.KeyStr != "": if m, ok := col.(collection.Keyed); ok { matches = m.FindString(rv.KeyStr) } else { - //This should probably never happen, selectability is checked at parsing time + // 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") } default: From 280af44df443c1467692ef32e22a719cb5eb6175 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Wed, 4 Sep 2024 14:55:28 +0200 Subject: [PATCH 11/16] remove debug spew call --- internal/collections/named_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/collections/named_test.go b/internal/collections/named_test.go index e65d138cf..67494409d 100644 --- a/internal/collections/named_test.go +++ b/internal/collections/named_test.go @@ -9,7 +9,6 @@ import ( "testing" "github.com/corazawaf/coraza/v3/types/variables" - "github.com/davecgh/go-spew/spew" ) func TestNamedCollection(t *testing.T) { @@ -55,8 +54,6 @@ func TestNamedCollection(t *testing.T) { t.Errorf("want %q, have %q", want, have) } - spew.Dump(names) - assertUnorderedValuesMatch(t, names.FindAll(), "key", "key2") if want, have := "ARGS_POST_NAMES: key,key2", fmt.Sprint(names); want != have { if want := "ARGS_POST_NAMES: key2,key"; want != have { From 279c462eb9db8ee3aa0e2f61813a822475495765 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Thu, 7 Nov 2024 13:21:54 +0100 Subject: [PATCH 12/16] tag more collections as being selectable --- internal/variables/variables.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/variables/variables.go b/internal/variables/variables.go index 17ee14747..1376e8b5e 100644 --- a/internal/variables/variables.go +++ b/internal/variables/variables.go @@ -122,17 +122,17 @@ const ( // ArgsPost contains the POST (BODY) arguments ArgsPost // CanBeSelected // ArgsPath contains the url path parts - ArgsPath + ArgsPath // CanBeSelected // FilesSizes contains the sizes of the uploaded files - FilesSizes + FilesSizes // CanBeSelected // FilesNames contains the names of the uploaded files FilesNames // CanBeSelected // FilesTmpContent is not supported - FilesTmpContent + FilesTmpContent // CanBeSelected // MultipartFilename contains the multipart data from field FILENAME - MultipartFilename + MultipartFilename // CanBeSelected // MultipartName contains the multipart data from field NAME. - MultipartName + MultipartName // CanBeSelected // MatchedVarsNames is similar to MATCHED_VAR_NAME except that it is // a collection of all matches for the current operator check. MatchedVarsNames // CanBeSelected @@ -142,7 +142,7 @@ const ( // Files contains a collection of original file names // (as they were called on the remote user’s filesys- tem). // Available only on inspected multipart/form-data requests. - Files + Files // CanBeSelected // RequestCookies is a collection of all of request cookies (values only RequestCookies // CanBeSelected // RequestHeaders can be used as either a collection of all of the request @@ -155,7 +155,7 @@ const ( // no default ResBodyProcessor // Geo contains the location information of the client - Geo + Geo // CanBeSelected // RequestCookiesNames contains the names of the request cookies RequestCookiesNames // CanBeSelected // FilesTmpNames contains the names of the uploaded temporal files @@ -169,7 +169,7 @@ const ( // TX contains transaction specific variables created with setvar TX // CanBeSelected // Rule contains rule metadata - Rule + Rule // CanBeSelected // JSON does not provide any data, might be removed JSON // CanBeSelected // Env contains the process environment variables From 5c8a2e25eee7c0b414e9434424cb750b5f7ca61f Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Thu, 7 Nov 2024 13:22:56 +0100 Subject: [PATCH 13/16] only generate code for selectable collections --- .../variables/generator/variablesmap.go.tmpl | 15 +- internal/variables/variablesmap.gen.go | 145 ------------------ 2 files changed, 9 insertions(+), 151 deletions(-) diff --git a/internal/variables/generator/variablesmap.go.tmpl b/internal/variables/generator/variablesmap.go.tmpl index 95ed7f2ce..5bea2ba1e 100644 --- a/internal/variables/generator/variablesmap.go.tmpl +++ b/internal/variables/generator/variablesmap.go.tmpl @@ -22,14 +22,17 @@ func (v RuleVariable) Name() string { } } -//CanBeSelected returns true if the variable supports selection (ie, `:foobar`) +// CanBeSelected returns true if the variable supports selection (ie, `:foobar`) func (v RuleVariable) CanBeSelected() bool { switch v { - {{range .}}case {{ .Key }}: - return {{ .CanBeSelected }} - {{end}} - default: - return false + {{- range . }} + {{- if .CanBeSelected }} + case {{ .Key }}: + return true + {{- end }} + {{- end }} + default: + return false } } diff --git a/internal/variables/variablesmap.gen.go b/internal/variables/variablesmap.gen.go index dc5d3c3cc..c75e937ca 100644 --- a/internal/variables/variablesmap.gen.go +++ b/internal/variables/variablesmap.gen.go @@ -215,84 +215,6 @@ func (v RuleVariable) Name() string { // CanBeSelected returns true if the variable supports selection (ie, `:foobar`) func (v RuleVariable) CanBeSelected() bool { switch v { - case Unknown: - return false - case ResponseContentType: - return false - case UniqueID: - return false - case ArgsCombinedSize: - return false - case FilesCombinedSize: - return false - case FullRequestLength: - return false - case InboundDataError: - return false - case MatchedVar: - return false - case MatchedVarName: - return false - case MultipartDataAfter: - return false - case OutboundDataError: - return false - case QueryString: - return false - case RemoteAddr: - return false - case RemoteHost: - return false - case RemotePort: - return false - case ReqbodyError: - return false - case ReqbodyErrorMsg: - return false - case ReqbodyProcessorError: - return false - case ReqbodyProcessorErrorMsg: - return false - case ReqbodyProcessor: - return false - case RequestBasename: - return false - case RequestBody: - return false - case RequestBodyLength: - return false - case RequestFilename: - return false - case RequestLine: - return false - case RequestMethod: - return false - case RequestProtocol: - return false - case RequestURI: - return false - case RequestURIRaw: - return false - case ResponseBody: - return false - case ResponseContentLength: - return false - case ResponseProtocol: - return false - case ResponseStatus: - return false - case ServerAddr: - return false - case ServerName: - return false - case ServerPort: - return false - case HighestSeverity: - return false - case StatusLine: - return false - case Duration: - return false case ResponseHeadersNames: return true case RequestHeadersNames: @@ -303,34 +225,18 @@ func (v RuleVariable) CanBeSelected() bool { return true case ArgsPost: return true - case ArgsPath: - return false - case FilesSizes: - return false case FilesNames: return true - case FilesTmpContent: - return false - case MultipartFilename: - return false - case MultipartName: - return false case MatchedVarsNames: return true case MatchedVars: return true - case Files: - return false case RequestCookies: return true case RequestHeaders: return true case ResponseHeaders: return true - case ResBodyProcessor: - return false - case Geo: - return false case RequestCookiesNames: return true case FilesTmpNames: @@ -343,14 +249,10 @@ func (v RuleVariable) CanBeSelected() bool { return true case TX: return true - case Rule: - return false case JSON: return true case Env: return true - case UrlencodedError: - return false case ResponseArgs: return true case ResponseXML: @@ -361,53 +263,6 @@ func (v RuleVariable) CanBeSelected() bool { return true case MultipartPartHeaders: return true - case AuthType: - return false - case FullRequest: - return false - case MultipartBoundaryQuoted: - return false - case MultipartBoundaryWhitespace: - return false - case MultipartCrlfLfLines: - return false - case MultipartDataBefore: - return false - case MultipartFileLimitExceeded: - return false - case MultipartHeaderFolding: - return false - case MultipartInvalidHeaderFolding: - return false - case MultipartInvalidPart: - return false - case MultipartInvalidQuoting: - return false - case MultipartLfLine: - return false - case MultipartMissingSemicolon: - return false - case MultipartStrictError: - return false - case MultipartUnmatchedBoundary: - return false - case PathInfo: - return false - case Sessionid: - return false - case Userid: - return false - case IP: - return false - case ResBodyError: - return false - case ResBodyErrorMsg: - return false - case ResBodyProcessorError: - return false - case ResBodyProcessorErrorMsg: - return false - default: return false } From e1d83b287b699c2b5b5293f535489e88278e2e7c Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Thu, 7 Nov 2024 13:26:35 +0100 Subject: [PATCH 14/16] early termination in FindRegex/FindString --- internal/collections/named.go | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/internal/collections/named.go b/internal/collections/named.go index c6d9c55e6..55a5d3db7 100644 --- a/internal/collections/named.go +++ b/internal/collections/named.go @@ -104,14 +104,15 @@ func (c *NamedCollectionNames) FindRegex(key *regexp.Regexp) []types.MatchData { var res []types.MatchData for k, data := range c.collection.Map.data { - if key.MatchString(k) { - for _, d := range data { - res = append(res, &corazarules.MatchData{ - Variable_: c.variable, - Key_: d.key, - Value_: d.key, - }) - } + 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 @@ -121,14 +122,15 @@ func (c *NamedCollectionNames) FindString(key string) []types.MatchData { 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, - }) - } + if k != key { + continue + } + for _, d := range data { + res = append(res, &corazarules.MatchData{ + Variable_: c.variable, + Key_: d.key, + Value_: d.key, + }) } } return res From f8e778f9949e0de483409362251bfe0ef72e7d98 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Thu, 7 Nov 2024 14:06:22 +0100 Subject: [PATCH 15/16] add test for getting non-existing item from named collection --- internal/collections/named_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/collections/named_test.go b/internal/collections/named_test.go index 67494409d..8138d3407 100644 --- a/internal/collections/named_test.go +++ b/internal/collections/named_test.go @@ -105,9 +105,21 @@ func TestNames(t *testing.T) { 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)) + } } From 6f9d99048e9a8039a18ce194b42764483f9ed9a2 Mon Sep 17 00:00:00 2001 From: soujanyanmbri <54130357+soujanyanmbri@users.noreply.github.com> Date: Wed, 22 Jan 2025 21:20:26 +0530 Subject: [PATCH 16/16] Add sync.map and optimise --- internal/corazawaf/rule.go | 23 ++++++++--------------- internal/corazawaf/rule_test.go | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go index 4e04ff8de..5e5ce8a59 100644 --- a/internal/corazawaf/rule.go +++ b/internal/corazawaf/rule.go @@ -5,7 +5,9 @@ package corazawaf import ( "fmt" + "hash/fnv" "regexp" + "strconv" "strings" "sync" "unsafe" @@ -585,23 +587,14 @@ func (r *Rule) AddVariableNegation(v variables.RuleVariable, key string) error { return nil } -var transformationIDToName = []string{""} -var transformationNameToID = map[string]int{"": 0} -var transformationIDsLock = sync.Mutex{} +var transformationNameToID sync.Map func transformationID(currentID int, transformationName string) int { - transformationIDsLock.Lock() - defer transformationIDsLock.Unlock() - - currName := transformationIDToName[currentID] - nextName := fmt.Sprintf("%s+%s", currName, transformationName) - if id, ok := transformationNameToID[nextName]; ok { - return id - } - - id := len(transformationIDToName) - transformationIDToName = append(transformationIDToName, nextName) - transformationNameToID[nextName] = id + nextName := strconv.Itoa(currentID) + "+" + transformationName + hasher := fnv.New64a() + hasher.Write([]byte(nextName)) + id := int(hasher.Sum64()) + transformationNameToID.LoadOrStore(id, nextName) return id } diff --git a/internal/corazawaf/rule_test.go b/internal/corazawaf/rule_test.go index d999bb0c8..2a1ea1070 100644 --- a/internal/corazawaf/rule_test.go +++ b/internal/corazawaf/rule_test.go @@ -616,3 +616,20 @@ func TestExpandMacroAfterWholeRuleEvaluation(t *testing.T) { t.Errorf("Expected ArgsGet-data, got %s", matchdata[0].Data()) } } + +func BenchmarkAddTransformationUnique(b *testing.B) { + transformation := func(input string) (string, bool, error) { + return "Test", true, nil + } + b.ResetTimer() + b.RunParallel(func(p *testing.PB) { + rule := NewRule() + for p.Next() { + transformationName := "transformation" + b.Name() + err := rule.AddTransformation(transformationName, transformation) + if err != nil { + b.Fatalf("Failed to add a transformation: %s", err.Error()) + } + } + }) +}