From 20c4076ec8d682bb132e104a5db543094fe3faf3 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Fri, 3 May 2024 18:39:04 -0300 Subject: [PATCH 1/3] feat: add support for case sensitive args Signed-off-by: Felipe Zipitria --- internal/corazawaf/rule.go | 113 ++++++++++++++++-------------- internal/corazawaf/rule_test.go | 10 +++ internal/corazawaf/transaction.go | 6 +- internal/seclang/rules_test.go | 78 ++++++++++----------- testing/engine/chains.go | 2 +- 5 files changed, 111 insertions(+), 98 deletions(-) diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go index 6e972e53a..b1f1f0e79 100644 --- a/internal/corazawaf/rule.go +++ b/internal/corazawaf/rule.go @@ -456,6 +456,44 @@ func (r *Rule) AddAction(name string, action plugintypes.Action) error { return nil } +// hasRegex checks the received key to see if it is between forward slashes. +// if it is, it will return true and the content of the regular expression inside the slashes. +// otherwise it will return false and the same key. +func hasRegex(key string) (bool, string) { + if len(key) > 2 && key[0] == '/' && key[len(key)-1] == '/' { + return true, key[1 : len(key)-1] + } + return false, key +} + +// caseSensitiveVariable returns true if the variable is case sensitive +func caseSensitiveVariable(v variables.RuleVariable) bool { + res := false + switch v { + case variables.Args, variables.ArgsNames, + variables.ArgsGet, variables.ArgsPost, + variables.ArgsGetNames, variables.ArgsPostNames: + res = true + } + return res +} + +// newRuleVariableParams creates a new ruleVariableParams +// knows if a key needs to be lowercased. This probably should not be here, +// but the knowledge of the type of the Map it not here also, so let's start with this. +func newRuleVariableParams(v variables.RuleVariable, key string, re *regexp.Regexp, iscount bool) ruleVariableParams { + if !caseSensitiveVariable(v) { + key = strings.ToLower(key) + } + return ruleVariableParams{ + Count: iscount, + Variable: v, + KeyStr: key, + KeyRx: re, + Exceptions: []ruleVariableException{}, + } +} + // AddVariable adds a variable to the rule // The key can be a regexp.Regexp, a string or nil, in case of regexp // it will be used to match the variable, in case of string it will @@ -465,10 +503,8 @@ func (r *Rule) AddVariable(v variables.RuleVariable, key string, iscount bool) e return fmt.Errorf("cannot add a variable to an undefined rule") } var re *regexp.Regexp - if len(key) > 2 && key[0] == '/' && key[len(key)-1] == '/' { - key = key[1 : len(key)-1] - - if vare, err := memoize.Do(key, func() (interface{}, error) { return regexp.Compile(key) }); err != nil { + if isRegex, rx := hasRegex(key); isRegex { + if vare, err := memoize.Do(rx, func() (interface{}, error) { return regexp.Compile(rx) }); err != nil { return err } else { re = vare.(*regexp.Regexp) @@ -478,53 +514,29 @@ func (r *Rule) AddVariable(v variables.RuleVariable, key string, iscount bool) e if multiphaseEvaluation { // Splitting Args variable into ArgsGet and ArgsPost if v == variables.Args { - r.variables = append(r.variables, ruleVariableParams{ - Count: iscount, - Variable: variables.ArgsGet, - KeyStr: strings.ToLower(key), - KeyRx: re, - Exceptions: []ruleVariableException{}, - }) - - r.variables = append(r.variables, ruleVariableParams{ - Count: iscount, - Variable: variables.ArgsPost, - KeyStr: strings.ToLower(key), - KeyRx: re, - Exceptions: []ruleVariableException{}, - }) + r.variables = append(r.variables, newRuleVariableParams(variables.ArgsGet, key, re, iscount)) + r.variables = append(r.variables, newRuleVariableParams(variables.ArgsPost, key, re, iscount)) return nil } // Splitting ArgsNames variable into ArgsGetNames and ArgsPostNames if v == variables.ArgsNames { - r.variables = append(r.variables, ruleVariableParams{ - Count: iscount, - Variable: variables.ArgsGetNames, - KeyStr: strings.ToLower(key), - KeyRx: re, - Exceptions: []ruleVariableException{}, - }) - - r.variables = append(r.variables, ruleVariableParams{ - Count: iscount, - Variable: variables.ArgsPostNames, - KeyStr: strings.ToLower(key), - KeyRx: re, - Exceptions: []ruleVariableException{}, - }) + r.variables = append(r.variables, newRuleVariableParams(variables.ArgsGetNames, key, re, iscount)) + r.variables = append(r.variables, newRuleVariableParams(variables.ArgsPostNames, key, re, iscount)) return nil } } - r.variables = append(r.variables, ruleVariableParams{ - Count: iscount, - Variable: v, - KeyStr: strings.ToLower(key), - KeyRx: re, - Exceptions: []ruleVariableException{}, - }) + r.variables = append(r.variables, newRuleVariableParams(v, key, re, iscount)) return nil } +// needToSplitConcatenatedVariable returns true if the variable v is Args or ArgsNames and the +// variable ve is ArgsGet, ArgsPost, ArgsGetNames or ArgsPostNames +func needToSplitConcatenatedVariable(v variables.RuleVariable, ve variables.RuleVariable) bool { + return (v == variables.Args || v == variables.ArgsNames) && + (ve == variables.ArgsGet || ve == variables.ArgsPost || + ve == variables.ArgsGetNames || ve == variables.ArgsPostNames) +} + // AddVariableNegation adds an exception to a variable // It passes through if the variable is not used // It returns an error if the selector is empty, @@ -535,9 +547,8 @@ func (r *Rule) AddVariable(v variables.RuleVariable, key string, iscount bool) e // ERROR: SecRule !ARGS: "..." func (r *Rule) AddVariableNegation(v variables.RuleVariable, key string) error { var re *regexp.Regexp - if len(key) > 2 && key[0] == '/' && key[len(key)-1] == '/' { - key = key[1 : len(key)-1] - if vare, err := memoize.Do(key, func() (interface{}, error) { return regexp.Compile(key) }); err != nil { + if isRegex, rx := hasRegex(key); isRegex { + if vare, err := memoize.Do(rx, func() (interface{}, error) { return regexp.Compile(rx) }); err != nil { return err } else { re = vare.(*regexp.Regexp) @@ -548,19 +559,15 @@ func (r *Rule) AddVariableNegation(v variables.RuleVariable, key string) error { return fmt.Errorf("cannot create a variable exception for an undefined rule") } for i, rv := range r.variables { - // Splitting Args and ArgsNames variables - if multiphaseEvaluation && v == variables.Args && (rv.Variable == variables.ArgsGet || rv.Variable == variables.ArgsPost) { - rv.Exceptions = append(rv.Exceptions, ruleVariableException{strings.ToLower(key), re}) - r.variables[i] = rv - continue - } - if multiphaseEvaluation && v == variables.ArgsNames && (rv.Variable == variables.ArgsGetNames || rv.Variable == variables.ArgsPostNames) { - rv.Exceptions = append(rv.Exceptions, ruleVariableException{strings.ToLower(key), re}) + // Even when Args and ArgsNames are one map, the exceptions must be created for the individual maps the + // Concat Map contains in order for exceptions to apply in the corresponding phase. + if multiphaseEvaluation && needToSplitConcatenatedVariable(v, rv.Variable) { + rv.Exceptions = append(rv.Exceptions, ruleVariableException{key, re}) r.variables[i] = rv continue } if rv.Variable == v { - rv.Exceptions = append(rv.Exceptions, ruleVariableException{strings.ToLower(key), re}) + rv.Exceptions = append(rv.Exceptions, ruleVariableException{key, re}) r.variables[i] = rv } } diff --git a/internal/corazawaf/rule_test.go b/internal/corazawaf/rule_test.go index 74af3efd4..06ce56eac 100644 --- a/internal/corazawaf/rule_test.go +++ b/internal/corazawaf/rule_test.go @@ -257,6 +257,16 @@ func TestRuleNegativeVariablesEmtpyRule(t *testing.T) { } } +func TestArgsVariableKeysAreCaseSensitive(t *testing.T) { + rule := NewRule() + if err := rule.AddVariable(variables.ArgsGet, "Som3ThinG", false); err != nil { + t.Error(err) + } + if rule.variables[0].KeyStr != "Som3ThinG" { + t.Error("variable key is not case insensitive") + } +} + func TestVariableKeysAreCaseInsensitive(t *testing.T) { rule := NewRule() if err := rule.AddVariable(variables.RequestURI, "Som3ThinG", false); err != nil { diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 83459d0d4..7ea59a82b 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -1689,11 +1689,11 @@ func NewTransactionVariables() *TransactionVariables { // XML is a pointer to RequestXML v.xml = v.requestXML - v.argsGet = collections.NewNamedCollection(variables.ArgsGet) + v.argsGet = collections.NewCaseSensitiveNamedCollection(variables.ArgsGet) v.argsGetNames = v.argsGet.Names(variables.ArgsGetNames) - v.argsPost = collections.NewNamedCollection(variables.ArgsPost) + v.argsPost = collections.NewCaseSensitiveNamedCollection(variables.ArgsPost) v.argsPostNames = v.argsPost.Names(variables.ArgsPostNames) - v.argsPath = collections.NewNamedCollection(variables.ArgsPath) + v.argsPath = collections.NewCaseSensitiveNamedCollection(variables.ArgsPath) v.argsCombinedSize = collections.NewSizeCollection(variables.ArgsCombinedSize, v.argsGet, v.argsPost) v.args = collections.NewConcatKeyed( variables.Args, diff --git a/internal/seclang/rules_test.go b/internal/seclang/rules_test.go index ab59c06cf..b96eaf55c 100644 --- a/internal/seclang/rules_test.go +++ b/internal/seclang/rules_test.go @@ -683,41 +683,19 @@ func TestArgumentsCaseSensitive(t *testing.T) { if err != nil { t.Error(err) } - if it == nil { - t.Errorf("failed to test arguments value match: Upper case argument name, %+v\n", tx.MatchedRules()) - } - - tx = waf.NewTransaction() - tx.ProcessRequestHeaders() - tx.AddPostRequestArgument("test1", "Xyz") - it, err = tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } - if it == nil { - t.Errorf("failed to test arguments value match: Lower case argument name, %+v\n", tx.MatchedRules()) - } - - tx = waf.NewTransaction() - tx.ProcessRequestHeaders() - tx.AddPostRequestArgument("test1", "xyz") - it, err = tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } if it != nil { - t.Error("failed to test arguments value: different value case") + t.Errorf("failed to test arguments value match: argument is matching a different case, %+v\n", tx.MatchedRules()) } tx = waf.NewTransaction() tx.ProcessRequestHeaders() - tx.AddPostRequestArgument("test1", "XYZ") + tx.AddPostRequestArgument("Test1", "XYZ") it, err = tx.ProcessRequestBody() if err != nil { t.Error(err) } if it != nil { - t.Error("failed to test arguments value: different value case") + t.Errorf("failed to test arguments value match: argument is matching a different case, %+v\n", tx.MatchedRules()) } } @@ -982,7 +960,7 @@ func TestURIQueryParamCaseSensitive(t *testing.T) { } if len(tx.MatchedRules()) == 1 { - if len(tx.MatchedRules()[0].MatchedDatas()) != 3 { + if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { t.Errorf("failed to test uri query param. Found matches: %d, %+v\n", len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) } @@ -995,17 +973,12 @@ func TestURIQueryParamCaseSensitive(t *testing.T) { } } -/* func TestURIQueryParamNameCaseSensitive(t *testing.T) { - waf := coraza.NewWAF() + waf := corazawaf.NewWAF() rules := `SecRule ARGS_NAMES "Test1" "id:3, phase:2, log, pass"` - parser, err := NewParser(waf) - if err != nil { - t.Error(err) - return - } + parser := NewParser(waf) - err = parser.FromString(rules) + err := parser.FromString(rules) if err != nil { t.Error() return @@ -1013,17 +986,18 @@ func TestURIQueryParamNameCaseSensitive(t *testing.T) { tx := waf.NewTransaction() tx.ProcessURI("/url?Test1='SQLI", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() _, err = tx.ProcessRequestBody() if err != nil { t.Error(err) } if len(tx.MatchedRules()) == 1 { - if len(tx.MatchedRules()[0].MatchedDatas) != 1 { + if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { t.Errorf("failed to test uri query param. Expected: 1, Found matches: %d, %+v\n", - len(tx.MatchedRules()[0].MatchedDatas), tx.MatchedRules()) + len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) } - if !isMatchData(tx.MatchedRules()[0].MatchedDatas, "Test1") { + if !isMatchData(tx.MatchedRules()[0].MatchedDatas(), "Test1") { t.Error("Key did not match: Test1 !=", tx.MatchedRules()[0]) } } else { @@ -1033,17 +1007,18 @@ func TestURIQueryParamNameCaseSensitive(t *testing.T) { tx = waf.NewTransaction() tx.ProcessURI("/test?test1='SQLI&Test1='SQLI&TEST1='SQLI", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() _, err = tx.ProcessRequestBody() if err != nil { t.Error(err) } if len(tx.MatchedRules()) == 1 { - if len(tx.MatchedRules()[0].MatchedDatas) != 1 { + if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { t.Errorf("Failed to test uri query param. Expected: 1, Found matches: %d, %+v\n", - len(tx.MatchedRules()[0].MatchedDatas), tx.MatchedRules()) + len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) } - if !isMatchData(tx.MatchedRules()[0].MatchedDatas, "Test1") { + if !isMatchData(tx.MatchedRules()[0].MatchedDatas(), "Test1") { t.Error("Key did not match: Test1 !=", tx.MatchedRules()[0]) } } else { @@ -1051,7 +1026,28 @@ func TestURIQueryParamNameCaseSensitive(t *testing.T) { len(tx.MatchedRules())) } } -*/ + +func TestRuleMatchRegexCaseSensitivity(t *testing.T) { + waf := corazawaf.NewWAF() + parser := NewParser(waf) + err := parser.FromString(` + SecRuleEngine On + SecRule ARGS:/^Key/ "@streq my-value" "id:1028,phase:1,deny,status:403,msg:'ARGS:key matched.'" + `) + if err != nil { + t.Error(err.Error()) + } + tx := waf.NewTransaction() + tx.ProcessURI("https://asdf.com/index.php?t1=aaa&T1=zzz&t2=bbb&t3=ccc&Keyless=my-value&a=test&jsessionid=74B0CB414BD77D17B5680A6386EF1666", "GET", "HTTP/1.1") + tx.ProcessConnection("127.0.0.1", 0, "", 0) + tx.ProcessRequestHeaders() + if len(tx.MatchedRules()) != 1 { + t.Errorf("failed to match rules with %d", len(tx.MatchedRules())) + } + if tx.Interruption() == nil { + t.Fatal("failed to interrupt transaction") + } +} func isMatchData(mds []types.MatchData, key string) (result bool) { result = false diff --git a/testing/engine/chains.go b/testing/engine/chains.go index bd31d7e16..fa74364d3 100644 --- a/testing/engine/chains.go +++ b/testing/engine/chains.go @@ -119,7 +119,7 @@ SecRule ARGS_GET "@rx prepayloadpost" "id:200, phase:2, log, msg:'Rule Parent 2 SecRule MATCHED_VAR "@rx pre" "chain" SecRule MATCHED_VAR "@rx post" -SecRule ARGS_GET:var3 "@rx pre3payloadpost" "id:300, phase:2, log, msg:'Rule Parent 300', \ +SecRule ARGS_GET:Var3 "@rx pre3payloadpost" "id:300, phase:2, log, msg:'Rule Parent 300', \ logdata:'Matched Data: %{TX.0} found within %{TX.300_MATCHED_VAR_NAME}: %{MATCHED_VAR}',\ setvar:'tx.300_matched_var_name=%{MATCHED_VAR_NAME}',\ chain" From cf7e286a0ffd64c5a0c7a4ef289e61d294777d2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Thu, 16 May 2024 15:47:32 -0500 Subject: [PATCH 2/3] Change args names to be case sensitive tag (#1065) * feat: adds build tag for case sensitive args keys. * chore: updates license year. --- internal/corazawaf/casesensitive.go | 8 ++ internal/corazawaf/casesensitive_default.go | 8 ++ internal/corazawaf/rule_casesensitive_test.go | 22 +++ internal/corazawaf/rule_test.go | 22 +-- internal/corazawaf/transaction.go | 15 +- internal/seclang/rules_casesensitive_test.go | 133 ++++++++++++++++++ internal/seclang/rules_test.go | 123 +--------------- magefile.go | 4 + 8 files changed, 188 insertions(+), 147 deletions(-) create mode 100644 internal/corazawaf/casesensitive.go create mode 100644 internal/corazawaf/casesensitive_default.go create mode 100644 internal/corazawaf/rule_casesensitive_test.go create mode 100644 internal/seclang/rules_casesensitive_test.go diff --git a/internal/corazawaf/casesensitive.go b/internal/corazawaf/casesensitive.go new file mode 100644 index 000000000..fdd68181d --- /dev/null +++ b/internal/corazawaf/casesensitive.go @@ -0,0 +1,8 @@ +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +//go:build coraza.rule.case_sensitive_args_keys + +package corazawaf + +var shouldUseCaseSensitiveNamedCollection = true diff --git a/internal/corazawaf/casesensitive_default.go b/internal/corazawaf/casesensitive_default.go new file mode 100644 index 000000000..47515ed15 --- /dev/null +++ b/internal/corazawaf/casesensitive_default.go @@ -0,0 +1,8 @@ +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +//go:build !coraza.rule.case_sensitive_args_keys + +package corazawaf + +var shouldUseCaseSensitiveNamedCollection = false diff --git a/internal/corazawaf/rule_casesensitive_test.go b/internal/corazawaf/rule_casesensitive_test.go new file mode 100644 index 000000000..96e23cdc9 --- /dev/null +++ b/internal/corazawaf/rule_casesensitive_test.go @@ -0,0 +1,22 @@ +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +//go:build coraza.rule.case_sensitive_args_keys + +package corazawaf + +import ( + "testing" + + "github.com/corazawaf/coraza/v3/types/variables" +) + +func TestCaseSensitiveArgsVariableKeys(t *testing.T) { + rule := NewRule() + if err := rule.AddVariable(variables.ArgsGet, "Som3ThinG", false); err != nil { + t.Error(err) + } + if rule.variables[0].KeyStr != "Som3ThinG" { + t.Error("variable key is not case insensitive") + } +} diff --git a/internal/corazawaf/rule_test.go b/internal/corazawaf/rule_test.go index 1135f3ba1..ccb3c18c0 100644 --- a/internal/corazawaf/rule_test.go +++ b/internal/corazawaf/rule_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 package corazawaf @@ -279,26 +279,6 @@ func TestRuleNegativeVariablesEmtpyRule(t *testing.T) { } } -func TestArgsVariableKeysAreCaseSensitive(t *testing.T) { - rule := NewRule() - if err := rule.AddVariable(variables.ArgsGet, "Som3ThinG", false); err != nil { - t.Error(err) - } - if rule.variables[0].KeyStr != "Som3ThinG" { - t.Error("variable key is not case insensitive") - } -} - -func TestVariableKeysAreCaseInsensitive(t *testing.T) { - rule := NewRule() - if err := rule.AddVariable(variables.RequestURI, "Som3ThinG", false); err != nil { - t.Error(err) - } - if rule.variables[0].KeyStr != "som3thing" { - t.Error("variable key is not case insensitive") - } -} - func TestVariablesRxAreCaseSensitive(t *testing.T) { rule := NewRule() if err := rule.AddVariable(variables.ArgsGet, "/Som3ThinG/", false); err != nil { diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 2689d7efa..fe8aa825a 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -1,4 +1,4 @@ -// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 package corazawaf @@ -1717,11 +1717,18 @@ func NewTransactionVariables() *TransactionVariables { // XML is a pointer to RequestXML v.xml = v.requestXML - v.argsGet = collections.NewCaseSensitiveNamedCollection(variables.ArgsGet) + if shouldUseCaseSensitiveNamedCollection { + v.argsGet = collections.NewCaseSensitiveNamedCollection(variables.ArgsGet) + v.argsPost = collections.NewCaseSensitiveNamedCollection(variables.ArgsPost) + v.argsPath = collections.NewCaseSensitiveNamedCollection(variables.ArgsPath) + } else { + v.argsGet = collections.NewNamedCollection(variables.ArgsGet) + v.argsPost = collections.NewNamedCollection(variables.ArgsPost) + v.argsPath = collections.NewNamedCollection(variables.ArgsPath) + } + v.argsGetNames = v.argsGet.Names(variables.ArgsGetNames) - v.argsPost = collections.NewCaseSensitiveNamedCollection(variables.ArgsPost) v.argsPostNames = v.argsPost.Names(variables.ArgsPostNames) - v.argsPath = collections.NewCaseSensitiveNamedCollection(variables.ArgsPath) v.argsCombinedSize = collections.NewSizeCollection(variables.ArgsCombinedSize, v.argsGet, v.argsPost) v.args = collections.NewConcatKeyed( variables.Args, diff --git a/internal/seclang/rules_casesensitive_test.go b/internal/seclang/rules_casesensitive_test.go new file mode 100644 index 000000000..b06e83659 --- /dev/null +++ b/internal/seclang/rules_casesensitive_test.go @@ -0,0 +1,133 @@ +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +//go:build coraza.rule.case_sensitive_args_keys + +package seclang + +import ( + "testing" + + "github.com/corazawaf/coraza/v3/internal/corazawaf" +) + +func TestCaseSensitiveRuleMatchRegex(t *testing.T) { + waf := corazawaf.NewWAF() + parser := NewParser(waf) + err := parser.FromString(` + SecRuleEngine On + SecRule ARGS:/^Key/ "@streq my-value" "id:1028,phase:1,deny,status:403,msg:'ARGS:key matched.'" + `) + if err != nil { + t.Error(err.Error()) + } + tx := waf.NewTransaction() + tx.ProcessURI("https://asdf.com/index.php?t1=aaa&T1=zzz&t2=bbb&t3=ccc&Keyless=my-value&a=test&jsessionid=74B0CB414BD77D17B5680A6386EF1666", "GET", "HTTP/1.1") + tx.ProcessConnection("127.0.0.1", 0, "", 0) + tx.ProcessRequestHeaders() + if len(tx.MatchedRules()) != 1 { + t.Errorf("failed to match rules with %d", len(tx.MatchedRules())) + } + if tx.Interruption() == nil { + t.Fatal("failed to interrupt transaction") + } +} + +func TestCaseSensitiveArguments(t *testing.T) { + waf := corazawaf.NewWAF() + rules := `SecRule ARGS:Test1 "Xyz" "id:3, phase:2, log, deny"` + parser := NewParser(waf) + + err := parser.FromString(rules) + if err != nil { + t.Error() + return + } + + tx := waf.NewTransaction() + tx.ProcessRequestHeaders() + tx.AddPostRequestArgument("Test1", "Xyz") + it, err := tx.ProcessRequestBody() + if err != nil { + t.Error(err) + } + if it == nil { + t.Errorf("failed to test arguments value match: Same case argument name, %+v\n", tx.MatchedRules()) + } + + tx = waf.NewTransaction() + tx.ProcessRequestHeaders() + tx.AddPostRequestArgument("TEST1", "Xyz") + it, err = tx.ProcessRequestBody() + if err != nil { + t.Error(err) + } + if it != nil { + t.Errorf("failed to test arguments value match: argument is matching a different case, %+v\n", tx.MatchedRules()) + } + + tx = waf.NewTransaction() + tx.ProcessRequestHeaders() + tx.AddPostRequestArgument("Test1", "XYZ") + it, err = tx.ProcessRequestBody() + if err != nil { + t.Error(err) + } + if it != nil { + t.Errorf("failed to test arguments value match: argument is matching a different case, %+v\n", tx.MatchedRules()) + } +} + +func TestCaseSensitiveURIQueryParam(t *testing.T) { + waf := corazawaf.NewWAF() + rules := `SecRule ARGS:Test1 "@contains SQLI" "id:3, phase:2, log, pass"` + parser := NewParser(waf) + + err := parser.FromString(rules) + if err != nil { + t.Error() + return + } + + tx := waf.NewTransaction() + tx.ProcessURI("/url?Test1='SQLI", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + _, err = tx.ProcessRequestBody() + if err != nil { + t.Error(err) + } + + if len(tx.MatchedRules()) == 1 { + if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { + t.Errorf("failed to test uri query param. Found matches: %d, %+v\n", + len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) + } + if !isMatchData(tx.MatchedRules()[0].MatchedDatas(), "Test1") { + t.Error("Key did not match: Test1 !=", tx.MatchedRules()[0]) + } + } else { + t.Errorf("failed to test uri query param: Same case arg name: %d, %+v\n", + len(tx.MatchedRules()), tx.MatchedRules()) + } + + tx = waf.NewTransaction() + tx.ProcessURI("/test?test1='SQLI&Test1='SQLI&TEST1='SQLI", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + _, err = tx.ProcessRequestBody() + if err != nil { + t.Error(err) + } + + if len(tx.MatchedRules()) == 1 { + if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { + t.Errorf("failed to test uri query param. Found matches: %d, %+v\n", + len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) + } + if !isMatchData(tx.MatchedRules()[0].MatchedDatas(), "Test1") { + t.Error("Key did not match: Test1 !=", tx.MatchedRules()[0]) + } + } else { + t.Errorf("failed to test qparam pollution: Multiple arg different case: %d, %+v\n", + len(tx.MatchedRules()), tx.MatchedRules()) + } +} diff --git a/internal/seclang/rules_test.go b/internal/seclang/rules_test.go index b96eaf55c..7390d45f1 100644 --- a/internal/seclang/rules_test.go +++ b/internal/seclang/rules_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 package seclang @@ -654,51 +654,6 @@ func TestArgumentNamesCaseSensitive(t *testing.T) { */ } -func TestArgumentsCaseSensitive(t *testing.T) { - waf := corazawaf.NewWAF() - rules := `SecRule ARGS:Test1 "Xyz" "id:3, phase:2, log, deny"` - parser := NewParser(waf) - - err := parser.FromString(rules) - if err != nil { - t.Error() - return - } - - tx := waf.NewTransaction() - tx.ProcessRequestHeaders() - tx.AddPostRequestArgument("Test1", "Xyz") - it, err := tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } - if it == nil { - t.Errorf("failed to test arguments value match: Same case argument name, %+v\n", tx.MatchedRules()) - } - - tx = waf.NewTransaction() - tx.ProcessRequestHeaders() - tx.AddPostRequestArgument("TEST1", "Xyz") - it, err = tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } - if it != nil { - t.Errorf("failed to test arguments value match: argument is matching a different case, %+v\n", tx.MatchedRules()) - } - - tx = waf.NewTransaction() - tx.ProcessRequestHeaders() - tx.AddPostRequestArgument("Test1", "XYZ") - it, err = tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } - if it != nil { - t.Errorf("failed to test arguments value match: argument is matching a different case, %+v\n", tx.MatchedRules()) - } -} - func TestCookiesCaseSensitive(t *testing.T) { waf := corazawaf.NewWAF() rules := `SecRule REQUEST_COOKIES:Test1 "Xyz" "id:3, phase:2, log, deny"` @@ -919,60 +874,6 @@ SecRule ARGS:test1 "ZZZZ" "id:4, phase:2, log, pass"` } } -func TestURIQueryParamCaseSensitive(t *testing.T) { - waf := corazawaf.NewWAF() - rules := `SecRule ARGS:Test1 "@contains SQLI" "id:3, phase:2, log, pass"` - parser := NewParser(waf) - - err := parser.FromString(rules) - if err != nil { - t.Error() - return - } - - tx := waf.NewTransaction() - tx.ProcessURI("/url?Test1='SQLI", "POST", "HTTP/1.1") - tx.ProcessRequestHeaders() - _, err = tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } - - if len(tx.MatchedRules()) == 1 { - if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { - t.Errorf("failed to test uri query param. Found matches: %d, %+v\n", - len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) - } - if !isMatchData(tx.MatchedRules()[0].MatchedDatas(), "Test1") { - t.Error("Key did not match: Test1 !=", tx.MatchedRules()[0]) - } - } else { - t.Errorf("failed to test uri query param: Same case arg name: %d, %+v\n", - len(tx.MatchedRules()), tx.MatchedRules()) - } - - tx = waf.NewTransaction() - tx.ProcessURI("/test?test1='SQLI&Test1='SQLI&TEST1='SQLI", "POST", "HTTP/1.1") - tx.ProcessRequestHeaders() - _, err = tx.ProcessRequestBody() - if err != nil { - t.Error(err) - } - - if len(tx.MatchedRules()) == 1 { - if len(tx.MatchedRules()[0].MatchedDatas()) != 1 { - t.Errorf("failed to test uri query param. Found matches: %d, %+v\n", - len(tx.MatchedRules()[0].MatchedDatas()), tx.MatchedRules()) - } - if !isMatchData(tx.MatchedRules()[0].MatchedDatas(), "Test1") { - t.Error("Key did not match: Test1 !=", tx.MatchedRules()[0]) - } - } else { - t.Errorf("failed to test qparam pollution: Multiple arg different case: %d, %+v\n", - len(tx.MatchedRules()), tx.MatchedRules()) - } -} - func TestURIQueryParamNameCaseSensitive(t *testing.T) { waf := corazawaf.NewWAF() rules := `SecRule ARGS_NAMES "Test1" "id:3, phase:2, log, pass"` @@ -1027,28 +928,6 @@ func TestURIQueryParamNameCaseSensitive(t *testing.T) { } } -func TestRuleMatchRegexCaseSensitivity(t *testing.T) { - waf := corazawaf.NewWAF() - parser := NewParser(waf) - err := parser.FromString(` - SecRuleEngine On - SecRule ARGS:/^Key/ "@streq my-value" "id:1028,phase:1,deny,status:403,msg:'ARGS:key matched.'" - `) - if err != nil { - t.Error(err.Error()) - } - tx := waf.NewTransaction() - tx.ProcessURI("https://asdf.com/index.php?t1=aaa&T1=zzz&t2=bbb&t3=ccc&Keyless=my-value&a=test&jsessionid=74B0CB414BD77D17B5680A6386EF1666", "GET", "HTTP/1.1") - tx.ProcessConnection("127.0.0.1", 0, "", 0) - tx.ProcessRequestHeaders() - if len(tx.MatchedRules()) != 1 { - t.Errorf("failed to match rules with %d", len(tx.MatchedRules())) - } - if tx.Interruption() == nil { - t.Fatal("failed to interrupt transaction") - } -} - func isMatchData(mds []types.MatchData, key string) (result bool) { result = false for _, m := range mds { diff --git a/magefile.go b/magefile.go index eb375de94..a445d96f5 100644 --- a/magefile.go +++ b/magefile.go @@ -127,6 +127,10 @@ func Test() error { return err } + if err := sh.RunV("go", "test", "-tags=coraza.rule.case_sensitive_args_keys", "-run=^TestCaseSensitive", "./..."); err != nil { + return err + } + return nil } From 1dd665addd960f024320b7c5dfe4f1438a94005a Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Tue, 28 May 2024 00:00:48 +0200 Subject: [PATCH 3/3] Update README with new build tag --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 1c7668fda..288b17b0f 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,7 @@ only the phase the rule is defined for. dictionaries to reduce memory consumption in deployments that launch several coraza instances. For more context check [this issue](https://github.com/corazawaf/coraza-caddy/issues/76) * `no_fs_access` - indicates that the target environment has no access to FS in order to not leverage OS' filesystem related functionality e.g. file body buffers. +* `coraza.rule.case_sensitive_args_keys` - enables case-sensitive matching for ARGS keys, aligning Coraza behavior with RFC 3986 specification. It will be enabled by default in the next major version. ## E2E Testing