From 42e97ac847211df0419b0a4bf9e6126b774f71cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Zipitr=C3=ADa?= <3012076+fzipi@users.noreply.github.com> Date: Thu, 31 Oct 2024 03:22:14 -0300 Subject: [PATCH] feat: add SecRuleUpdateActionById directive (#1071) * feat: add SecRuleUpdateActionById directive Signed-off-by: Felipe Zipitria * fix: testing rule e2e Signed-off-by: Felipe Zipitria * fix: apply code review suggestions Signed-off-by: Felipe Zipitria --------- Signed-off-by: Felipe Zipitria --- internal/seclang/directives.go | 84 ++++++++++++++++++- internal/seclang/directives_test.go | 38 +++++++++ internal/seclang/directivesmap.gen.go | 3 +- .../seclang/generator/directivesmap.go.tmpl | 1 - testing/engine/directives_updateactions.go | 64 ++++++++++++++ 5 files changed, 187 insertions(+), 3 deletions(-) create mode 100644 testing/engine/directives_updateactions.go diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go index 2bf34a3e5..b45220c8c 100644 --- a/internal/seclang/directives.go +++ b/internal/seclang/directives.go @@ -357,7 +357,7 @@ func directiveSecRuleRemoveByID(options *DirectiveOptions) error { options.WAF.Rules.DeleteByID(id) } else { if idx == 0 { - return fmt.Errorf("SecRuleUpdateTargetById: invalid negative id: %s", idOrRange) + return fmt.Errorf("SecRuleRemoveById: invalid negative id: %s", idOrRange) } start, err := strconv.Atoi(idOrRange[:idx]) if err != nil { @@ -1046,6 +1046,88 @@ func updateTargetBySingleID(id int, variables string, options *DirectiveOptions) return rp.ParseVariables(strings.Trim(variables, "\"")) } +// Description: Updates the action list of the specified rule(s). +// Syntax: SecRuleUpdateActionById ID ACTIONLIST +// --- +// This directive will overwrite the action list of the specified rule with the actions provided in the second parameter. +// It has two limitations: it cannot be used to change the ID or phase of a rule. +// Only the actions that can appear only once are overwritten. +// The actions that are allowed to appear multiple times in a list, will be appended to the end of the list. +// The following example demonstrates how `SecAuditEngine` is used: +// ```apache +// SecRuleUpdateActionById 12345 "deny,status:403" +// ``` +func directiveSecRuleUpdateActionByID(options *DirectiveOptions) error { + if len(options.Opts) == 0 { + return errEmptyOptions + } + + idsOrRanges := strings.Fields(options.Opts) + idsOrRangesLen := len(idsOrRanges) + if idsOrRangesLen < 2 { + return errors.New("syntax error: SecRuleUpdateActionById id \"ACTION1,ACTION2,...\"") + } + // The last element is expected to be the actions(s) + actions := idsOrRanges[idsOrRangesLen-1] + for _, idOrRange := range idsOrRanges[:idsOrRangesLen-1] { + if idx := strings.Index(idOrRange, "-"); idx == -1 { + id, err := strconv.Atoi(idOrRange) + if err != nil { + return err + } + return updateActionBySingleID(id, actions, options) + } else { + if idx == 0 { + return fmt.Errorf("SecRuleUpdateActionById: invalid negative id: %s", idOrRange) + } + start, err := strconv.Atoi(idOrRange[:idx]) + if err != nil { + return err + } + + end, err := strconv.Atoi(idOrRange[idx+1:]) + if err != nil { + return err + } + if start == end { + return updateActionBySingleID(start, actions, options) + } + if start > end { + return fmt.Errorf("invalid range: %s", idOrRange) + } + + for _, rule := range options.WAF.Rules.GetRules() { + if rule.ID_ < start && rule.ID_ > end { + continue + } + rp := RuleParser{ + rule: &rule, + options: RuleOptions{}, + defaultActions: map[types.RulePhase][]ruleAction{}, + } + if err := rp.ParseActions(strings.Trim(actions, "\"")); err != nil { + return err + } + } + } + } + return nil +} + +func updateActionBySingleID(id int, actions string, options *DirectiveOptions) error { + + rule := options.WAF.Rules.FindByID(id) + if rule == nil { + return fmt.Errorf("SecRuleUpdateActionById: rule \"%d\" not found", id) + } + rp := RuleParser{ + rule: rule, + options: RuleOptions{}, + defaultActions: map[types.RulePhase][]ruleAction{}, + } + return rp.ParseActions(strings.Trim(actions, "\"")) +} + // Description: Updates the target (variable) list of the specified rule(s) by tag. // Syntax: SecRuleUpdateTargetByTag TAG TARGET1[|TARGET2|TARGET3] // --- diff --git a/internal/seclang/directives_test.go b/internal/seclang/directives_test.go index 1d828f4c4..b5b47b777 100644 --- a/internal/seclang/directives_test.go +++ b/internal/seclang/directives_test.go @@ -36,6 +36,31 @@ func Test_NonImplementedDirective(t *testing.T) { } } +func TestSecRuleUpdateActionByID(t *testing.T) { + waf := corazawaf.NewWAF() + rule, err := ParseRule(RuleOptions{ + Data: "REQUEST_URI \"^/test\" \"id:181,log\"", + WAF: waf, + WithOperator: true, + }) + if err != nil { + t.Error(err) + } + if err := waf.Rules.Add(rule); err != nil { + t.Error(err) + } + if waf.Rules.Count() != 1 { + t.Error("Failed to add rule") + } + if err := directiveSecRuleUpdateActionByID(&DirectiveOptions{ + WAF: waf, + Opts: "181 \"nolog\"", + }); err != nil { + t.Error(err) + } + +} + func TestSecRuleUpdateTargetByID(t *testing.T) { waf := corazawaf.NewWAF() rule, err := ParseRule(RuleOptions{ @@ -179,6 +204,19 @@ func TestDirectives(t *testing.T) { {"1 2", expectNoErrorOnDirective}, {"1 2 3-4", expectNoErrorOnDirective}, }, + "SecRuleUpdateActionById": { + {"", expectErrorOnDirective}, + {"a", expectErrorOnDirective}, + {"1-a", expectErrorOnDirective}, + {"a-2", expectErrorOnDirective}, + {"2-1", expectErrorOnDirective}, + {"1-a \"status:403\"", expectErrorOnDirective}, + {"a-2 \"status:403\"", expectErrorOnDirective}, + {"2-1 \"status:403\"", expectErrorOnDirective}, + {"-1 \"status:403\"", expectErrorOnDirective}, + {"1 2 3-4 \"status:403\"", expectNoErrorOnDirective}, + {"1 2 3-4 \"status:403,nolog\"", expectNoErrorOnDirective}, + }, "SecRuleUpdateTargetById": { {"", expectErrorOnDirective}, {"a", expectErrorOnDirective}, diff --git a/internal/seclang/directivesmap.gen.go b/internal/seclang/directivesmap.gen.go index 9e6fc118d..a0b2102d2 100644 --- a/internal/seclang/directivesmap.gen.go +++ b/internal/seclang/directivesmap.gen.go @@ -60,6 +60,7 @@ var ( _ directive = directiveSecDebugLog _ directive = directiveSecDebugLogLevel _ directive = directiveSecRuleUpdateTargetByID + _ directive = directiveSecRuleUpdateActionByID _ directive = directiveSecRuleUpdateTargetByTag _ directive = directiveSecIgnoreRuleCompilationErrors _ directive = directiveSecDataset @@ -121,6 +122,7 @@ var directivesMap = map[string]directive{ "secdebuglog": directiveSecDebugLog, "secdebugloglevel": directiveSecDebugLogLevel, "secruleupdatetargetbyid": directiveSecRuleUpdateTargetByID, + "secruleupdateactionbyid": directiveSecRuleUpdateActionByID, "secruleupdatetargetbytag": directiveSecRuleUpdateTargetByTag, "secignorerulecompilationerrors": directiveSecIgnoreRuleCompilationErrors, "secdataset": directiveSecDataset, @@ -130,7 +132,6 @@ var directivesMap = map[string]directive{ "secargumentseparator": directiveUnsupported, "seccookieformat": directiveUnsupported, "secruleupdatetargetbymsg": directiveUnsupported, - "secruleupdateactionbyid": directiveUnsupported, "secrulescript": directiveUnsupported, "secruleperftime": directiveUnsupported, "secunicodemap": directiveUnsupported, diff --git a/internal/seclang/generator/directivesmap.go.tmpl b/internal/seclang/generator/directivesmap.go.tmpl index 0e2956acf..069de4ef9 100644 --- a/internal/seclang/generator/directivesmap.go.tmpl +++ b/internal/seclang/generator/directivesmap.go.tmpl @@ -17,7 +17,6 @@ var directivesMap = map[string]directive{ "secargumentseparator": directiveUnsupported, "seccookieformat": directiveUnsupported, "secruleupdatetargetbymsg": directiveUnsupported, - "secruleupdateactionbyid": directiveUnsupported, "secrulescript": directiveUnsupported, "secruleperftime": directiveUnsupported, "secunicodemap": directiveUnsupported, diff --git a/testing/engine/directives_updateactions.go b/testing/engine/directives_updateactions.go new file mode 100644 index 000000000..e5fd692d1 --- /dev/null +++ b/testing/engine/directives_updateactions.go @@ -0,0 +1,64 @@ +// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +package engine + +import ( + "github.com/corazawaf/coraza/v3/testing/profile" +) + +var _ = profile.RegisterProfile(profile.Profile{ + Meta: profile.Meta{ + Author: "fzipi", + Description: "Test SecRuleUpdateActionById directives", + Enabled: true, + Name: "SecRuleUpdateActionById.yaml", + }, + Tests: []profile.Test{ + { + Title: "SecRuleUpdateActionById to pass", + Stages: []profile.Stage{ + // Phase 1 + { + Stage: profile.SubStage{ + Input: profile.StageInput{ + URI: "/phase1?param1=value1", + }, + Output: profile.ExpectedOutput{ + TriggeredRules: []int{ + 1004, + }, + }, + }, + }, + // Phase 2 + { + Stage: profile.SubStage{ + Input: profile.StageInput{ + URI: "/phase2?param2=value2", + }, + Output: profile.ExpectedOutput{ + TriggeredRules: []int{ + 1014, + }, + Interruption: &profile.ExpectedInterruption{ + Status: 302, + Data: "https://www.example.com/", + RuleID: 1014, + Action: "redirect", + }, + }, + }, + }, + }, + }, + }, + Rules: ` + SecRule ARGS "@contains value1" "phase:1,id:1004,deny" + SecRule ARGS "@contains value1" "phase:1,id:1005,log" + SecRuleUpdateActionById 1004 "pass" + + SecRule ARGS "@contains value2" "phase:2,id:1014,block,deny" + SecRuleUpdateActionById 1014 "redirect:'https://www.example.com/',status:302" + `, +})