Skip to content

Commit

Permalink
feat: add support for case sensitive args (#1059)
Browse files Browse the repository at this point in the history
* feat: add support for case sensitive args

Signed-off-by: Felipe Zipitria <[email protected]>

* Change args names to be case sensitive tag (#1065)

* feat: adds build tag for case sensitive args keys.

* chore: updates license year.

* Update README with new build tag

---------

Signed-off-by: Felipe Zipitria <[email protected]>
Co-authored-by: José Carlos Chávez <[email protected]>
Co-authored-by: Matteo Pace <[email protected]>
  • Loading branch information
3 people authored May 28, 2024
1 parent bef0335 commit 711e4a4
Show file tree
Hide file tree
Showing 11 changed files with 256 additions and 201 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 8 additions & 0 deletions internal/corazawaf/casesensitive.go
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions internal/corazawaf/casesensitive_default.go
Original file line number Diff line number Diff line change
@@ -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
113 changes: 60 additions & 53 deletions internal/corazawaf/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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,
Expand All @@ -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)
Expand All @@ -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
}
}
Expand Down
22 changes: 22 additions & 0 deletions internal/corazawaf/rule_casesensitive_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
12 changes: 1 addition & 11 deletions internal/corazawaf/rule_test.go
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -279,16 +279,6 @@ func TestRuleNegativeVariablesEmtpyRule(t *testing.T) {
}
}

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 {
Expand Down
15 changes: 11 additions & 4 deletions internal/corazawaf/transaction.go
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -1717,11 +1717,18 @@ func NewTransactionVariables() *TransactionVariables {
// XML is a pointer to RequestXML
v.xml = v.requestXML

v.argsGet = collections.NewNamedCollection(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.NewNamedCollection(variables.ArgsPost)
v.argsPostNames = v.argsPost.Names(variables.ArgsPostNames)
v.argsPath = collections.NewNamedCollection(variables.ArgsPath)
v.argsCombinedSize = collections.NewSizeCollection(variables.ArgsCombinedSize, v.argsGet, v.argsPost)
v.args = collections.NewConcatKeyed(
variables.Args,
Expand Down
133 changes: 133 additions & 0 deletions internal/seclang/rules_casesensitive_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
}
Loading

0 comments on commit 711e4a4

Please sign in to comment.