From f47a1c0e64da5f6c1c3b2e27d015efd9f05e0135 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Sun, 22 Oct 2023 14:21:31 +0200 Subject: [PATCH 1/4] chore: setvar minor fix, tests, added warning when missing variable --- examples/http-server/go.mod | 2 +- experimental/plugins/macro/macro.go | 2 + go.work.sum | 1 - internal/actions/setvar.go | 30 +++---- internal/actions/setvar_test.go | 118 +++++++++++++++++++++++++++- testing/coreruleset/go.mod | 6 +- testing/coreruleset/go.sum | 12 +-- 7 files changed, 140 insertions(+), 31 deletions(-) diff --git a/examples/http-server/go.mod b/examples/http-server/go.mod index 08acd1e8c..5423bbdf3 100644 --- a/examples/http-server/go.mod +++ b/examples/http-server/go.mod @@ -1,6 +1,6 @@ module github.com/corazawaf/coraza/v3/examples/http-server -go 1.18 +go 1.19 require github.com/corazawaf/coraza/v3 v3.0.0-20220914101451-05d352c89b24 diff --git a/experimental/plugins/macro/macro.go b/experimental/plugins/macro/macro.go index 6fc10ce91..ec756f0d8 100644 --- a/experimental/plugins/macro/macro.go +++ b/experimental/plugins/macro/macro.go @@ -82,6 +82,8 @@ func expandToken(tx plugintypes.TransactionState, token macroToken) string { } } + // If the variable is known (e.g. TX) but the key is not found, we return the original text + tx.DebugLogger().Warn().Str("variable", token.variable.Name()).Str("key", token.key).Msg("key not found in collection, returning the original text") return token.text } diff --git a/go.work.sum b/go.work.sum index 569ff9723..050519ddf 100644 --- a/go.work.sum +++ b/go.work.sum @@ -1,6 +1,5 @@ golang.org/x/crypto v0.14.0 h1:wBqGXzWJW6m1XrIKlAH0Hs1JJ7+9KBwnIO8v66Q9cHc= golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4= golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= -golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/term v0.13.0/go.mod h1:LTmsnFJwVN6bCy1rVCoS+qHT1HhALEFxKncY3WNNh4U= golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= diff --git a/internal/actions/setvar.go b/internal/actions/setvar.go index a372b4410..2c3635206 100644 --- a/internal/actions/setvar.go +++ b/internal/actions/setvar.go @@ -136,20 +136,19 @@ func (a *setvarFn) evaluateTxCollection(r plugintypes.RuleMetadata, tx plugintyp col.Remove(key) return } - res := "" + currentVal := "" if r := col.Get(key); len(r) > 0 { - res = r[0] + currentVal = r[0] } var err error switch { case len(value) == 0: // if nothing to input col.Set(key, []string{""}) - case value[0] == '+': - // if we want to sum - sum := 0 + case value[0] == '+', value[0] == '-': // Increment or decrement, arithmetical operations + val := 0 if len(value) > 1 { - sum, err = strconv.Atoi(value[1:]) + val, err = strconv.Atoi(value[1:]) if err != nil { tx.DebugLogger().Error(). Str("var_value", value). @@ -159,26 +158,23 @@ func (a *setvarFn) evaluateTxCollection(r plugintypes.RuleMetadata, tx plugintyp return } } - val := 0 - if res != "" { - val, err = strconv.Atoi(res) + currentValInt := 0 + if currentVal != "" { + currentValInt, err = strconv.Atoi(currentVal) if err != nil { tx.DebugLogger().Error(). - Str("var_key", res). + Str("var_key", currentVal). Int("rule_id", r.ID()). Err(err). Msg("Invalid value") return } } - col.Set(key, []string{strconv.Itoa(sum + val)}) - case value[0] == '-': - me, _ := strconv.Atoi(value[1:]) - txv, err := strconv.Atoi(res) - if err != nil { - return + if value[0] == '+' { + col.Set(key, []string{strconv.Itoa(currentValInt + val)}) + } else { + col.Set(key, []string{strconv.Itoa(currentValInt - val)}) } - col.Set(key, []string{strconv.Itoa(txv - me)}) default: col.Set(key, []string{value}) } diff --git a/internal/actions/setvar_test.go b/internal/actions/setvar_test.go index c512c83a6..8e4358dfc 100644 --- a/internal/actions/setvar_test.go +++ b/internal/actions/setvar_test.go @@ -4,19 +4,26 @@ package actions import ( + "bytes" + "strings" "testing" + + "github.com/corazawaf/coraza/v3/collection" + "github.com/corazawaf/coraza/v3/debuglog" + "github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes" + "github.com/corazawaf/coraza/v3/internal/corazawaf" ) type md struct { } -func (_ md) ID() int { +func (md) ID() int { return 0 } -func (_ md) ParentID() int { +func (md) ParentID() int { return 0 } -func (_ md) Status() int { +func (md) Status() int { return 0 } @@ -46,3 +53,108 @@ func TestSetvarInit(t *testing.T) { } }) } + +var invalidSyntaxAtoiError = "invalid syntax" +var warningKeyNotFoundInCollection = "key not found in collection" + +func TestSetvarEvaluateErrors(t *testing.T) { + tests := []struct { + name string + init string + init2 string + expectInvalidSyntaxError bool + expectNewVarValue string + }{ + { + name: "Numerical operation + with existing variable", + init: "TX.var=5", + init2: "TX.newvar=+%{tx.var}", + expectInvalidSyntaxError: false, + expectNewVarValue: "5", + }, + { + name: "Numerical operation - with existing variable", + init: "TX.var=5", + init2: "TX.newvar=-%{tx.var}", + expectInvalidSyntaxError: false, + expectNewVarValue: "-5", + }, + { + name: "Numerical operation + with missing (or non-numerical) variable", + init: "TX.newvar=+%{tx.missingvar}", + expectInvalidSyntaxError: true, + }, + { + name: "Numerical operation - with missing (or non-numerical) variable", + init: "TX.newvar=-%{tx.missingvar}", + expectInvalidSyntaxError: true, + }, + } + + for _, tt := range tests { + logsBuf := &bytes.Buffer{} + + logger := debuglog.Default().WithLevel(debuglog.LevelWarn).WithOutput(logsBuf) + + t.Run(tt.name, func(t *testing.T) { + defer logsBuf.Reset() + a := setvar() + metadata := &md{} + if err := a.Init(metadata, tt.init); err != nil { + t.Error("unexpected error during setvar init") + } + waf := corazawaf.NewWAF() + waf.Logger = logger + tx := waf.NewTransaction() + a.Evaluate(metadata, tx) + if tt.expectInvalidSyntaxError { + if logsBuf.Len() == 0 { + t.Fatal("expected error") + } + if !strings.Contains(logsBuf.String(), invalidSyntaxAtoiError) { + t.Errorf("expected error containing %q, got %q", invalidSyntaxAtoiError, logsBuf.String()) + } + if !strings.Contains(logsBuf.String(), warningKeyNotFoundInCollection) { + t.Errorf("expected error containing %q, got %q", warningKeyNotFoundInCollection, logsBuf.String()) + } + } + if logsBuf.Len() != 0 && !tt.expectInvalidSyntaxError { + t.Fatalf("unexpected error: %s", logsBuf.String()) + } + + if tt.init2 != "" { + if err := a.Init(metadata, tt.init2); err != nil { + t.Fatal("unexpected error during setvar init") + } + a.Evaluate(metadata, tx) + if logsBuf.Len() != 0 && !tt.expectInvalidSyntaxError { + t.Fatalf("unexpected error: %s", logsBuf.String()) + } + } + if tt.expectNewVarValue != "" { + checkCollectionValue(t, a.(*setvarFn), tx, "newvar", tt.expectNewVarValue) + } + }) + } +} + +func checkCollectionValue(t *testing.T, a *setvarFn, tx plugintypes.TransactionState, key string, expected string) { + t.Helper() + var col collection.Map + if c, ok := tx.Collection(a.collection).(collection.Map); !ok { + t.Fatal("collection in setvar is not a map") + return + } else { + col = c + } + if col == nil { + t.Fatal("collection in setvar is nil") + return + } + if col == nil { + t.Fatal("collection is nil") + } + if col.Get(key)[0] != expected { + t.Errorf("key %q: expected %q, got %q", key, expected, col.Get(key)) + } +} diff --git a/testing/coreruleset/go.mod b/testing/coreruleset/go.mod index e44a834eb..be4b7e831 100644 --- a/testing/coreruleset/go.mod +++ b/testing/coreruleset/go.mod @@ -35,9 +35,9 @@ require ( github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.1 // indirect github.com/yargevad/filepathx v1.0.0 // indirect - golang.org/x/crypto v0.12.0 // indirect - golang.org/x/net v0.14.0 // indirect - golang.org/x/sys v0.11.0 // indirect + golang.org/x/crypto v0.14.0 // indirect + golang.org/x/net v0.17.0 // indirect + golang.org/x/sys v0.13.0 // indirect golang.org/x/tools v0.6.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/testing/coreruleset/go.sum b/testing/coreruleset/go.sum index 8d30bf4db..ff20fd632 100644 --- a/testing/coreruleset/go.sum +++ b/testing/coreruleset/go.sum @@ -314,8 +314,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20190923035154-9ee001bba392/go.mod h1:/lpIB1dKB+9EgE3H3cr1v9wB50oz8l4C4h62xy7jSTY= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.12.0 h1:tFM/ta59kqch6LlvYnPa0yx5a83cL2nHflFhYKvv9Yk= -golang.org/x/crypto v0.12.0/go.mod h1:NF0Gs7EO5K4qLn+Ylc+fih8BSTeIjAP05siRnAh98yw= +golang.org/x/crypto v0.14.0 h1:wBqGXzWJW6m1XrIKlAH0Hs1JJ7+9KBwnIO8v66Q9cHc= +golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU= @@ -343,8 +343,8 @@ golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwY golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM= golang.org/x/net v0.0.0-20210410081132-afb366fc7cd1/go.mod h1:9tjilg8BloeKEkVJvy7fQ90B1CfIiPueXVOjqfkSzI8= -golang.org/x/net v0.14.0 h1:BONx9s002vGdD9umnlX1Po8vOZmrgH34qlHcD1MfK14= -golang.org/x/net v0.14.0/go.mod h1:PpSgVXXLK0OxS0F31C1/tv6XNguvCrnXIDrFMspZIUI= +golang.org/x/net v0.17.0 h1:pVaXccu2ozPjCXewfr1S7xza/zcXTity9cCdXQYSjIM= +golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -391,8 +391,8 @@ golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM= -golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE= +golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20181227161524-e6919f6577db/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= From 955938abbe14f6b6d07cd54ec92605b2a0782db2 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Sun, 22 Oct 2023 14:37:25 +0200 Subject: [PATCH 2/4] adds tests with numerical operation starting with a negative number --- internal/actions/setvar_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/actions/setvar_test.go b/internal/actions/setvar_test.go index 8e4358dfc..1471e8f23 100644 --- a/internal/actions/setvar_test.go +++ b/internal/actions/setvar_test.go @@ -79,6 +79,13 @@ func TestSetvarEvaluateErrors(t *testing.T) { expectInvalidSyntaxError: false, expectNewVarValue: "-5", }, + { + name: "Numerical operation - with existing negative variable", + init: "TX.newvar=-5", + init2: "TX.newvar=+5", + expectInvalidSyntaxError: false, + expectNewVarValue: "0", + }, { name: "Numerical operation + with missing (or non-numerical) variable", init: "TX.newvar=+%{tx.missingvar}", From 2c955da3ce07b31f564962905251e068caa59f18 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Sun, 22 Oct 2023 16:42:28 +0200 Subject: [PATCH 3/4] deprecating Logdata field in transaction --- internal/actions/logdata.go | 2 +- internal/corazawaf/rule.go | 2 +- internal/corazawaf/rule_test.go | 9 +++++---- internal/corazawaf/transaction.go | 1 + internal/corazawaf/waf.go | 2 +- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/actions/logdata.go b/internal/actions/logdata.go index 03a602d74..24fa05888 100644 --- a/internal/actions/logdata.go +++ b/internal/actions/logdata.go @@ -37,7 +37,7 @@ func (a *logdataFn) Init(r plugintypes.RuleMetadata, data string) error { } func (a *logdataFn) Evaluate(r plugintypes.RuleMetadata, tx plugintypes.TransactionState) { - tx.(*corazawaf.Transaction).Logdata = r.(*corazawaf.Rule).LogData.Expand(tx) + // logdata macro expansion is performed after all other actions have been evaluated (and potentially all the needed variables have been set) } func (a *logdataFn) Type() plugintypes.ActionType { diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go index 14016b957..2209d9ca6 100644 --- a/internal/corazawaf/rule.go +++ b/internal/corazawaf/rule.go @@ -342,7 +342,7 @@ func (r *Rule) doEvaluate(phase types.RulePhase, tx *Transaction, collectiveMatc } // Expansion of Msg and LogData is postponed here. It allows to run it only if the whole rule/chain - // matches and to rely on MATCHED_* variables updated by the chain, not just by the fist rule. + // matches and to rely on MATCHED_* variables updated by the chain, not just by the first rule. if !r.MultiMatch { if r.Msg != nil { matchedValues[0].(*corazarules.MatchData).Message_ = r.Msg.Expand(tx) diff --git a/internal/corazawaf/rule_test.go b/internal/corazawaf/rule_test.go index 472dcb27d..87b20b413 100644 --- a/internal/corazawaf/rule_test.go +++ b/internal/corazawaf/rule_test.go @@ -95,7 +95,8 @@ func (*dummyFlowAction) Init(_ plugintypes.RuleMetadata, _ string) error { } func (*dummyFlowAction) Evaluate(_ plugintypes.RuleMetadata, tx plugintypes.TransactionState) { - tx.(*Transaction).Logdata = "flow action triggered" + // SkipAfter is used in a improper way, just for testing purposes ensuring that the action has been enforced + tx.(*Transaction).SkipAfter = "flow action triggered" } func (*dummyFlowAction) Type() plugintypes.ActionType { @@ -116,7 +117,7 @@ func TestFlowActionIfDetectionOnlyEngine(t *testing.T) { if len(matchdata) != 1 { t.Errorf("Expected 1 matchdata, got %d", len(matchdata)) } - if tx.Logdata != "flow action triggered" { + if tx.SkipAfter != "flow action triggered" { t.Errorf("Expected flow action triggered with DetectionOnly engine") } } @@ -128,7 +129,7 @@ func (*dummyNonDisruptiveAction) Init(_ plugintypes.RuleMetadata, _ string) erro } func (*dummyNonDisruptiveAction) Evaluate(_ plugintypes.RuleMetadata, tx plugintypes.TransactionState) { - tx.(*Transaction).Logdata = "action enforced" + tx.(*Transaction).SkipAfter = "action enforced" } func (*dummyNonDisruptiveAction) Type() plugintypes.ActionType { @@ -142,7 +143,7 @@ func TestMatchVariableRunsActionTypeNondisruptive(t *testing.T) { action := &dummyNonDisruptiveAction{} _ = rule.AddAction("dummyNonDisruptiveAction", action) rule.matchVariable(tx, md) - if tx.Logdata != "action enforced" { + if tx.SkipAfter != "action enforced" { t.Errorf("Expected non disruptive action to be enforced during matchVariable") } } diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 9cb69cc82..b872f67a7 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -48,6 +48,7 @@ type Transaction struct { interruption *types.Interruption // This is used to store log messages + // Deprecated since Coraza 3.0.5: this variable is not used, logdata values are stored in the matched rules Logdata string // Rules will be skipped after a rule with this SecMarker is found diff --git a/internal/corazawaf/waf.go b/internal/corazawaf/waf.go index 7af329a10..8656110c1 100644 --- a/internal/corazawaf/waf.go +++ b/internal/corazawaf/waf.go @@ -153,7 +153,7 @@ func (w *WAF) newTransactionWithID(id string) *Transaction { tx.id = id tx.matchedRules = []types.MatchedRule{} tx.interruption = nil - tx.Logdata = "" + tx.Logdata = "" // Deprecated, this variable is not used. Logdata for each matched rule is stored in the MatchData field. tx.SkipAfter = "" tx.AuditEngine = w.AuditEngine tx.AuditLogParts = w.AuditLogParts From 7c72491ca45a3465f6a16396d5b5fdeefeae48d2 Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Mon, 30 Oct 2023 12:01:54 +0100 Subject: [PATCH 4/4] updates test name --- internal/actions/setvar_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/actions/setvar_test.go b/internal/actions/setvar_test.go index 1471e8f23..591d5a853 100644 --- a/internal/actions/setvar_test.go +++ b/internal/actions/setvar_test.go @@ -57,7 +57,7 @@ func TestSetvarInit(t *testing.T) { var invalidSyntaxAtoiError = "invalid syntax" var warningKeyNotFoundInCollection = "key not found in collection" -func TestSetvarEvaluateErrors(t *testing.T) { +func TestSetvarEvaluate(t *testing.T) { tests := []struct { name string init string