feat: Implements SecRuleUpdateTargetByTag, extends ByID with ranges (#1020)

* Implements SecRuleUpdateTargetByTag, extends ByTag with ranges

* tag might be quoted

* nit: space

* address review

Author: M4tteoP

internal/corazawaf/rule.go

@@ -471,6 +471,9 @@ func (r *Rule) AddAction(name string, action plugintypes.Action) error {
 // it will be used to match the variable, in case of string it will
 // be a fixed match, in case of nil it will match everything
 func (r *Rule) AddVariable(v variables.RuleVariable, key string, iscount bool) error {
+	if r == nil {
+		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]

internal/corazawaf/rulegroup.go

@@ -8,7 +8,7 @@ import (
 	"time"
 
 	"github.com/corazawaf/coraza/v3/internal/corazatypes"
-	"github.com/corazawaf/coraza/v3/internal/strings"
+	utils "github.com/corazawaf/coraza/v3/internal/strings"
 	"github.com/corazawaf/coraza/v3/types"
 	"github.com/corazawaf/coraza/v3/types/variables"
 )
@@ -102,7 +102,7 @@ func (rg *RuleGroup) DeleteByMsg(msg string) {
 func (rg *RuleGroup) DeleteByTag(tag string) {
 	var kept []Rule
 	for _, r := range rg.rules {
-		if !strings.InSlice(tag, r.Tags_) {
+		if !utils.InSlice(tag, r.Tags_) {
 			kept = append(kept, r)
 		}
 	}

internal/seclang/directives.go

@@ -354,6 +354,9 @@ func directiveSecRuleRemoveByID(options *DirectiveOptions) error {
 
 			options.WAF.Rules.DeleteByID(id)
 		} else {
+			if idx == 0 {
+				return fmt.Errorf("SecRuleUpdateTargetById: invalid negative id: %s", idOrRange)
+			}
 			start, err := strconv.Atoi(idOrRange[:idx])
 			if err != nil {
 				return err
@@ -952,22 +955,111 @@ func directiveSecDebugLogLevel(options *DirectiveOptions) error {
 	return options.WAF.SetDebugLogLevel(debuglog.Level(lvl))
 }
 
+// Description: Updates the target (variable) list of the specified rule(s).
+// Syntax: SecRuleUpdateTargetById ID TARGET1[|TARGET2|TARGET3]
+// ---
+// This directive will append variables to the specified rule with the targets provided in the second parameter.
+// The rule ID can be single IDs or ranges of IDs. The targets are separated by a pipe character.
 func directiveSecRuleUpdateTargetByID(options *DirectiveOptions) error {
-	idStr, v, ok := strings.Cut(options.Opts, " ")
-	if !ok {
+	if len(options.Opts) == 0 {
+		return errEmptyOptions
+	}
+
+	idsOrRanges := strings.Fields(options.Opts)
+	length := len(idsOrRanges)
+	if length < 2 {
 		return errors.New("syntax error: SecRuleUpdateTargetById id \"VARIABLES\"")
 	}
-	id, err := strconv.Atoi(idStr)
-	if err != nil {
-		return err
+	// The last element is expected to be the variable(s)
+	variables := idsOrRanges[length-1]
+	for _, idOrRange := range idsOrRanges[:length-1] {
+		if idx := strings.Index(idOrRange, "-"); idx == -1 {
+			id, err := strconv.Atoi(idOrRange)
+			if err != nil {
+				return err
+			}
+			return updateTargetBySingleID(id, variables, options)
+		} else {
+			if idx == 0 {
+				return fmt.Errorf("SecRuleUpdateTargetById: 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 updateTargetBySingleID(start, variables, 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 {
+					rp := RuleParser{
+						rule:           &rule,
+						options:        RuleOptions{},
+						defaultActions: map[types.RulePhase][]ruleAction{},
+					}
+					if err := rp.ParseVariables(strings.Trim(variables, "\"")); err != nil {
+						return err
+					}
+				}
+			}
+		}
 	}
+	return nil
+}
+
+func updateTargetBySingleID(id int, variables string, options *DirectiveOptions) error {
+
 	rule := options.WAF.Rules.FindByID(id)
+	if rule == nil {
+		return fmt.Errorf("SecRuleUpdateTargetById: rule \"%d\" not found", id)
+	}
 	rp := RuleParser{
 		rule:           rule,
 		options:        RuleOptions{},
 		defaultActions: map[types.RulePhase][]ruleAction{},
 	}
-	return rp.ParseVariables(strings.Trim(v, "\""))
+	return rp.ParseVariables(strings.Trim(variables, "\""))
+}
+
+// Description: Updates the target (variable) list of the specified rule(s) by tag.
+// Syntax: SecRuleUpdateTargetByTag TAG TARGET1[|TARGET2|TARGET3]
+// ---
+// As an alternative to `SecRuleUpdateTargetById`, this directive will append variables to the specified rule
+// with the targets provided in the second parameter. It can be handy for updating an entire group of rules.
+// Matching is by case-sensitive string equality.
+// This directive will append variables to the specified rule with the targets provided in the second parameter.
+// The rule ID can be single IDs or ranges of IDs. The targets are separated by a pipe character.
+// Note: OWASP CRS has a list of supported tags https://coreruleset.org/docs/rules/metadata/
+func directiveSecRuleUpdateTargetByTag(options *DirectiveOptions) error {
+	tagAndvars := strings.Fields(options.Opts)
+	if len(tagAndvars) != 2 {
+		return errors.New("syntax error: SecRuleUpdateTargetByTag tag \"VARIABLES\"")
+	}
+
+	for _, rule := range options.WAF.Rules.GetRules() {
+		inputTag := strings.Trim(tagAndvars[0], "\"")
+		if utils.InSlice(inputTag, rule.Tags_) {
+			rp := RuleParser{
+				rule:           &rule,
+				options:        RuleOptions{},
+				defaultActions: map[types.RulePhase][]ruleAction{},
+			}
+			inputVars := strings.Trim(tagAndvars[1], "\"")
+			if err := rp.ParseVariables(inputVars); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
 }
 
 func directiveSecIgnoreRuleCompilationErrors(options *DirectiveOptions) error {

internal/seclang/directives_test.go

@@ -4,6 +4,7 @@
 package seclang
 
 import (
+	"regexp"
 	"strings"
 	"testing"
 
@@ -34,7 +35,7 @@ func Test_NonImplementedDirective(t *testing.T) {
 	}
 }
 
-func TestSecRuleUpdateTargetBy(t *testing.T) {
+func TestSecRuleUpdateTargetByID(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	rule, err := ParseRule(RuleOptions{
 		Data:         "REQUEST_URI \"^/test\" \"id:181,tag:test\"",
@@ -158,6 +159,7 @@ func TestDirectives(t *testing.T) {
 		},
 		"SecRuleRemoveByTag": {
 			{"", expectErrorOnDirective},
+			{"attack-sqli", expectNoErrorOnDirective},
 		},
 		"SecRuleRemoveByMsg": {
 			{"", expectErrorOnDirective},
@@ -168,10 +170,41 @@ func TestDirectives(t *testing.T) {
 			{"1-a", expectErrorOnDirective},
 			{"a-2", expectErrorOnDirective},
 			{"2-1", expectErrorOnDirective},
+			{"-1", expectErrorOnDirective},
+			{"-5--1", expectErrorOnDirective},
+			{"5--1", expectErrorOnDirective},
 			{"1", expectNoErrorOnDirective},
 			{"1 2", expectNoErrorOnDirective},
 			{"1 2 3-4", expectNoErrorOnDirective},
 		},
+		"SecRuleUpdateTargetById": {
+			{"", expectErrorOnDirective},
+			{"a", expectErrorOnDirective},
+			{"1-a", expectErrorOnDirective},
+			{"a-2", expectErrorOnDirective},
+			{"2-1", expectErrorOnDirective},
+			{"1-a \"ARGS:wp_post\"", expectErrorOnDirective},
+			{"a-2 \"ARGS:wp_post\"", expectErrorOnDirective},
+			{"2-1 \"ARGS:wp_post\"", expectErrorOnDirective},
+			{"-1 \"ARGS:wp_post\"", expectErrorOnDirective},
+			{"-5--1 \"ARGS:wp_post\"", expectErrorOnDirective},
+			{"5--1 \"ARGS:wp_post\"", expectErrorOnDirective},
+			// Variables has also to be provided to the directive
+			{"1", expectErrorOnDirective},
+			{"1 \"ARGS:wp_post\"", expectNoErrorOnDirective},
+			{"7-7 \"ARGS:wp_post\"", expectNoErrorOnDirective},
+			{"1 2 \"ARGS:wp_post\"", expectNoErrorOnDirective},
+			{"1 2 3-4 \"ARGS:wp_post\"", expectNoErrorOnDirective},
+			{"1 \"REQUEST_BODY|ARGS:wp_post\"", expectNoErrorOnDirective},
+			{"1 2 3-4 \"ARGS:wp_post|RESPONSE_HEADERS\"", expectNoErrorOnDirective},
+		},
+		"SecRuleUpdateTargetByTag": {
+			{"", expectErrorOnDirective},
+			{"a", expectErrorOnDirective},
+			{"tag-1 \"ARGS:wp_post\"", expectNoErrorOnDirective},
+			{"tag-1 tag-2 \"ARGS:wp_post\"", expectErrorOnDirective}, // Multiple tags in line is not supported
+			{"tag-2 \"ARGS:wp_post|RESPONSE_HEADERS|!REQUEST_BODY\"", expectNoErrorOnDirective},
+		},
 		"SecResponseBodyMimeTypesClear": {
 			{"", func(w *corazawaf.WAF) bool { return len(w.ResponseBodyMimeTypes) == 0 }},
 			{"x", expectErrorOnDirective},
@@ -262,7 +295,12 @@ func TestDirectives(t *testing.T) {
 						}
 					} else {
 						if err != nil {
-							t.Errorf("unexpected error: %s", err.Error())
+							match, _ := regexp.MatchString(`rule "\d+" not found`, err.Error())
+							// Logical errors are not checked by this test, therefore this specific pattern is allowed here
+							if !match {
+								// Syntax errors are checked
+								t.Errorf("unexpected error: %s", err.Error())
+							}
 						}
 
 						if !tCase.check(waf) {

internal/seclang/directivesmap.gen.go

@@ -16,7 +16,6 @@ var directivesMap = map[string]directive{
 	// Unsupported directives
 	"secargumentseparator":     directiveUnsupported,
 	"seccookieformat":          directiveUnsupported,
-	"secruleupdatetargetbytag": directiveUnsupported,
 	"secruleupdatetargetbymsg": directiveUnsupported,
 	"secruleupdateactionbyid":  directiveUnsupported,
 	"secrulescript":            directiveUnsupported,

internal/seclang/generator/directivesmap.go.tmpl

@@ -120,7 +120,7 @@ func TestSecRuleUpdateTargetVariableNegation(t *testing.T) {
 		SecRule REQUEST_URI|REQUEST_COOKIES "abc" "id:9,phase:2"
 		SecRuleUpdateTargetById 99 "!REQUEST_HEADERS:xyz"
 	`)
-	expectedErr = errors.New("cannot create a variable exception for an undefined rule")
+	expectedErr = errors.New("SecRuleUpdateTargetById: rule \"99\" not found")
 	if errors.Unwrap(err).Error() != expectedErr.Error() {
 		t.Fatalf("unexpected error, want %q, have %q", expectedErr, errors.Unwrap(err).Error())
 	}

internal/seclang/rule_parser_test.go

@@ -0,0 +1,94 @@
+// Copyright 2022 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:      "M4tteoP",
+		Description: "Test SecRuleUpdateTarget directives",
+		Enabled:     true,
+		Name:        "SecRuleUpdateTarget.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "SecRuleUpdateTarget",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:    "/index.php?t1=aaa&t2=bbb",
+							Method: "POST",
+							Headers: map[string]string{
+								"content-type": "application/x-www-form-urlencoded",
+								"Cookie":       "cookie=aaa",
+							},
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								20,
+								30,
+								50,
+								62,
+								51,
+							},
+							NonTriggeredRules: []int{
+								10,
+								12,
+								16,
+								40,
+								60,
+								61,
+							},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+	# ARGS:t1 is removed by SecRuleUpdateTargetById, rule 10 should not be triggered
+	SecRule ARGS:t1 "@rx aaa" "id:10,phase:1,log"
+	SecRuleUpdateTargetById 10 "!ARGS:t1"
+
+	# ARGS:t2 is removed by SecRuleUpdateTargetById, rule 12 (in range ids 11-13) should not be triggered
+	SecRule ARGS:t2 "@rx bbb" "id:12,phase:1,log"
+	SecRuleUpdateTargetById 11-13 "!ARGS:t2"
+
+	# ARGS:t2 is removed by SecRuleUpdateTargetById, rule 16 (in range) should not be triggered
+	SecRule ARGS:t2 "@rx bbb" "id:16,phase:1,log"
+	SecRuleUpdateTargetById 13-15 16 18 "!ARGS:t2"
+	
+	# ARGS:t1 is removed by SecRuleUpdateTargetById, but REQUEST_COOKIES should still trigger rule 20
+	SecRule ARGS:t1|REQUEST_COOKIES "@rx aaa" "id:20,phase:1,log"
+	SecRuleUpdateTargetById 20 "!ARGS:t1"
+
+	# ARGS:t1 is added by SecRuleUpdateTargetById, it should trigger rule 30
+	SecRule REQUEST_BODY "@rx aaa" "id:30,phase:1,log"
+	SecRuleUpdateTargetById 30 "ARGS:t1"
+
+	# ARGS:t19999 is added by SecRuleUpdateTargetById, it should not trigger rule 40
+	SecRule REQUEST_BODY "@rx aaa" "id:40,phase:1,log"
+	SecRuleUpdateTargetById 40 "ARGS:t19999"
+
+	# ARGS:t1 is NOT removed by SecRuleUpdateTargetByTag, rule 50 should be triggered
+	SecRule ARGS:t1 "@rx aaa" "id:50,phase:1,log,tag:tag-1"
+	SecRuleUpdateTargetByTag tag-1111 "!ARGS:t1"
+
+	# ARGS:t1 is NOT removed by SecRuleUpdateTargetByTag. Because case sensitive matching, rule 51 should be triggered
+	SecRule ARGS:t1 "@rx aaa" "id:51,phase:1,log,tag:tag-1b"
+	SecRuleUpdateTargetByTag tAg-1b "!ARGS:t1"
+
+	# ARGS:t1 is removed by SecRuleUpdateTargetByTag, rule 60,61 should not be triggered.
+	SecRule ARGS "@rx aaa" "id:60,phase:1,log,tag:tag-2"
+	SecRule ARGS:t2 "@rx bbb" "id:61,phase:1,log,tag:tag-2"
+	SecRule ARGS:t1|REQUEST_COOKIES "@rx aaa" "id:62,phase:1,log,tag:tag-2"
+	# The tag might also be wrapped in double quotes
+	SecRuleUpdateTargetByTag "tag-2" "!ARGS:t1|!ARGS:t2"
+
+	`,
+})