diff --git a/cmd/crowdsec-cli/cliitem/hubappsec.go b/cmd/crowdsec-cli/cliitem/hubappsec.go index dd8b032b397..577790c7a27 100644 --- a/cmd/crowdsec-cli/cliitem/hubappsec.go +++ b/cmd/crowdsec-cli/cliitem/hubappsec.go @@ -138,8 +138,8 @@ func NewAppsecRule(cfg csconfig.Getter) *cliItem { for _, ruleType := range appsec_rule.SupportedTypes() { fmt.Fprintf(os.Stdout, "\n%s format:\n", cases.Title(language.Und, cases.NoLower).String(ruleType)) - for _, rule := range appsecRule.Rules { - convertedRule, _, err := rule.Convert(ruleType, appsecRule.Name, appsecRule.Description) + for i, rule := range appsecRule.Rules { + convertedRule, _, err := rule.Convert(ruleType, appsecRule.Name, appsecRule.Description, i) if err != nil { return fmt.Errorf("unable to convert rule %s: %w", rule.Name, err) } diff --git a/pkg/acquisition/modules/appsec/appsec_hooks_test.go b/pkg/acquisition/modules/appsec/appsec_hooks_test.go index 38334f4f1fa..6dfd1d31d7e 100644 --- a/pkg/acquisition/modules/appsec/appsec_hooks_test.go +++ b/pkg/acquisition/modules/appsec/appsec_hooks_test.go @@ -527,7 +527,7 @@ func TestAppsecPreEvalHooks(t *testing.T) { }, }, pre_eval: []appsec.Hook{ - {Apply: []string{"RemoveInBandRuleByID(1516470898)"}}, //rule ID is generated at runtime. If you change rule, it will break the test (: + {Apply: []string{"RemoveInBandRuleByID(407789510)"}}, //rule ID is generated at runtime. If you change rule, it will break the test (: }, input_request: appsec.ParsedRequest{ RemoteAddr: "1.2.3.4", @@ -752,7 +752,7 @@ func TestAppsecPreEvalHooks(t *testing.T) { }, }, pre_eval: []appsec.Hook{ - {Apply: []string{"SetRemediationByID(1516470898, 'foobar')"}}, //rule ID is generated at runtime. If you change rule, it will break the test (: + {Apply: []string{"SetRemediationByID(407789510, 'foobar')"}}, //rule ID is generated at runtime. If you change rule, it will break the test (: }, input_request: appsec.ParsedRequest{ RemoteAddr: "1.2.3.4", diff --git a/pkg/acquisition/modules/appsec/appsec_runner.go b/pkg/acquisition/modules/appsec/appsec_runner.go index e410b353a61..40e0cba8b7f 100644 --- a/pkg/acquisition/modules/appsec/appsec_runner.go +++ b/pkg/acquisition/modules/appsec/appsec_runner.go @@ -5,6 +5,7 @@ import ( "fmt" "maps" "os" + "regexp" "slices" "strings" "time" @@ -35,21 +36,26 @@ type AppsecRunner struct { appsecAllowlistsClient *allowlists.AppsecAllowlist } +// ruleIDDirective matches the generated id so dedup can ignore it: rules that +// differ only by id (same content, different position) are duplicates. +var ruleIDDirective = regexp.MustCompile(`"id:\d+,phase:`) + func (*AppsecRunner) MergeDedupRules(collections []appsec.AppsecCollection, logger *log.Entry) string { var rulesArr []string dedupRules := make(map[string]struct{}) discarded := 0 for _, collection := range collections { - // Dedup *our* rules + // Dedup *our* rules, ignoring the generated id. for _, rule := range collection.Rules { - if _, ok := dedupRules[rule]; ok { + key := ruleIDDirective.ReplaceAllString(rule, `"id:,phase:`) + if _, ok := dedupRules[key]; ok { discarded++ logger.Debugf("Discarding duplicate rule : %s", rule) continue } rulesArr = append(rulesArr, rule) - dedupRules[rule] = struct{}{} + dedupRules[key] = struct{}{} } // Don't mess up with native modsec rules rulesArr = append(rulesArr, collection.NativeRules...) diff --git a/pkg/acquisition/modules/appsec/appsec_test.go b/pkg/acquisition/modules/appsec/appsec_test.go index ed1780d01ad..72ee7141e6a 100644 --- a/pkg/acquisition/modules/appsec/appsec_test.go +++ b/pkg/acquisition/modules/appsec/appsec_test.go @@ -96,7 +96,7 @@ func testAppSecEngine(t *testing.T, test appsecRuleTest) { //build rules for ridx, rule := range test.inband_rules { - strRule, _, err := rule.Convert(appsec_rule.ModsecurityRuleType, rule.Name, "test-rule") + strRule, _, err := rule.Convert(appsec_rule.ModsecurityRuleType, rule.Name, "test-rule", ridx) if err != nil { t.Fatalf("failed compilation of rule %d/%d of %s : %s", ridx, len(test.inband_rules), test.name, err) } @@ -106,7 +106,7 @@ func testAppSecEngine(t *testing.T, test appsecRuleTest) { nativeInbandRules = append(nativeInbandRules, test.inband_native_rules...) nativeOutofbandRules = append(nativeOutofbandRules, test.outofband_native_rules...) for ridx, rule := range test.outofband_rules { - strRule, _, err := rule.Convert(appsec_rule.ModsecurityRuleType, rule.Name, "test-rule") + strRule, _, err := rule.Convert(appsec_rule.ModsecurityRuleType, rule.Name, "test-rule", ridx) if err != nil { t.Fatalf("failed compilation of rule %d/%d of %s : %s", ridx, len(test.outofband_rules), test.name, err) } diff --git a/pkg/appsec/appsec_rule/appsec_rule.go b/pkg/appsec/appsec_rule/appsec_rule.go index 3ad94e7b0ec..fd5a647b4b8 100644 --- a/pkg/appsec/appsec_rule/appsec_rule.go +++ b/pkg/appsec/appsec_rule/appsec_rule.go @@ -46,7 +46,9 @@ type CustomRule struct { BodyType string `yaml:"body_type,omitempty"` } -func (v *CustomRule) Convert(ruleType string, appsecRuleName string, appsecRuleDescription string) (string, []uint32, error) { +// Convert renders the rule; ruleIndex is its position in the collection, used +// to keep ids unique across rules that share identical leaves. +func (v *CustomRule) Convert(ruleType string, appsecRuleName string, appsecRuleDescription string, ruleIndex int) (string, []uint32, error) { if v.Zones == nil && v.And == nil && v.Or == nil { return "", nil, errors.New("no zones defined") } @@ -62,7 +64,7 @@ func (v *CustomRule) Convert(ruleType string, appsecRuleName string, appsecRuleD switch ruleType { case ModsecurityRuleType: r := ModsecurityRule{} - return r.Build(v, appsecRuleName, appsecRuleDescription) + return r.Build(v, appsecRuleName, appsecRuleDescription, ruleIndex) default: return "", nil, fmt.Errorf("unknown rule format '%s'", ruleType) } diff --git a/pkg/appsec/appsec_rule/modsec_rule_test.go b/pkg/appsec/appsec_rule/modsec_rule_test.go index d8d6ddf662f..1df56126efd 100644 --- a/pkg/appsec/appsec_rule/modsec_rule_test.go +++ b/pkg/appsec/appsec_rule/modsec_rule_test.go @@ -24,7 +24,7 @@ func TestVPatchRuleString(t *testing.T) { Match: Match{Type: "eq", Value: "1"}, Transform: []string{"count"}, }, - expected: `SecRule &ARGS_GET:foo "@eq 1" "id:853070236,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Collection count',tag:'cs-custom-rule',severity:'emergency'"`, + expected: `SecRule &ARGS_GET:foo "@eq 1" "id:784839152,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Collection count',tag:'cs-custom-rule',severity:'emergency'"`, }, { name: "Base Rule", @@ -36,7 +36,7 @@ func TestVPatchRuleString(t *testing.T) { Match: Match{Type: "regex", Value: "[^a-zA-Z]"}, Transform: []string{"lowercase"}, }, - expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:2203944045,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Base Rule',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, + expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:426728273,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Base Rule',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, }, { name: "One zone, multi var", @@ -48,7 +48,7 @@ func TestVPatchRuleString(t *testing.T) { Match: Match{Type: "regex", Value: "[^a-zA-Z]"}, Transform: []string{"lowercase"}, }, - expected: `SecRule ARGS_GET:foo|ARGS_GET:bar "@rx [^a-zA-Z]" "id:385719930,phase:2,deny,log,msg:'test rule',tag:'crowdsec-One zone, multi var',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, + expected: `SecRule ARGS_GET:foo|ARGS_GET:bar "@rx [^a-zA-Z]" "id:3295043918,phase:2,deny,log,msg:'test rule',tag:'crowdsec-One zone, multi var',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, }, { name: "Base Rule #2", @@ -58,7 +58,7 @@ func TestVPatchRuleString(t *testing.T) { Zones: []string{"METHOD"}, Match: Match{Type: "startsWith", Value: "toto"}, }, - expected: `SecRule REQUEST_METHOD "@beginsWith toto" "id:2759779019,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Base Rule #2',tag:'cs-custom-rule',severity:'emergency'"`, + expected: `SecRule REQUEST_METHOD "@beginsWith toto" "id:106084967,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Base Rule #2',tag:'cs-custom-rule',severity:'emergency'"`, }, { name: "Base Negative Rule", @@ -68,7 +68,7 @@ func TestVPatchRuleString(t *testing.T) { Zones: []string{"METHOD"}, Match: Match{Type: "startsWith", Value: "toto", Not: true}, }, - expected: `SecRule REQUEST_METHOD "!@beginsWith toto" "id:3966251995,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Base Negative Rule',tag:'cs-custom-rule',severity:'emergency'"`, + expected: `SecRule REQUEST_METHOD "!@beginsWith toto" "id:1561051063,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Base Negative Rule',tag:'cs-custom-rule',severity:'emergency'"`, }, { name: "Multiple Zones", @@ -80,7 +80,7 @@ func TestVPatchRuleString(t *testing.T) { Match: Match{Type: "regex", Value: "[^a-zA-Z]"}, Transform: []string{"lowercase"}, }, - expected: `SecRule ARGS_GET:foo|ARGS_POST:foo "@rx [^a-zA-Z]" "id:3387135861,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Multiple Zones',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, + expected: `SecRule ARGS_GET:foo|ARGS_POST:foo "@rx [^a-zA-Z]" "id:1176299393,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Multiple Zones',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, }, { name: "Multiple Zones Multi Var", @@ -92,7 +92,7 @@ func TestVPatchRuleString(t *testing.T) { Match: Match{Type: "regex", Value: "[^a-zA-Z]"}, Transform: []string{"lowercase"}, }, - expected: `SecRule ARGS_GET:foo|ARGS_GET:bar|ARGS_POST:foo|ARGS_POST:bar "@rx [^a-zA-Z]" "id:1119773585,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Multiple Zones Multi Var',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, + expected: `SecRule ARGS_GET:foo|ARGS_GET:bar|ARGS_POST:foo|ARGS_POST:bar "@rx [^a-zA-Z]" "id:2142736261,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Multiple Zones Multi Var',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, }, { name: "Multiple Zones No Vars", @@ -103,7 +103,7 @@ func TestVPatchRuleString(t *testing.T) { Match: Match{Type: "regex", Value: "[^a-zA-Z]"}, Transform: []string{"lowercase"}, }, - expected: `SecRule ARGS_GET|ARGS_POST "@rx [^a-zA-Z]" "id:2020110336,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Multiple Zones No Vars',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, + expected: `SecRule ARGS_GET|ARGS_POST "@rx [^a-zA-Z]" "id:3150922420,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Multiple Zones No Vars',tag:'cs-custom-rule',severity:'emergency',t:lowercase"`, }, { name: "Basic AND", @@ -125,8 +125,8 @@ func TestVPatchRuleString(t *testing.T) { }, }, }, - expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:988489239,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic AND',tag:'cs-custom-rule',severity:'emergency',t:lowercase,chain" -SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:4145519614,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic AND',tag:'cs-custom-rule',t:lowercase"`, + expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:3951337643,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic AND',tag:'cs-custom-rule',severity:'emergency',t:lowercase,chain" +SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:1797871498,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic AND',tag:'cs-custom-rule',t:lowercase"`, }, { name: "Basic OR", @@ -148,8 +148,8 @@ SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:4145519614,phase:2,deny,log,msg:'test r }, }, }, - expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:4061834901,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic OR',tag:'cs-custom-rule',severity:'emergency',t:lowercase,skip:1" -SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:651140804,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic OR',tag:'cs-custom-rule',t:lowercase"`, + expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:2861427001,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic OR',tag:'cs-custom-rule',severity:'emergency',t:lowercase,skip:1" +SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:2619082328,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Basic OR',tag:'cs-custom-rule',t:lowercase"`, }, { // leaf(foo) AND (foo OR bar) → DNF: (foo AND foo) OR (foo AND bar) → 2 groups of 2 @@ -180,10 +180,10 @@ SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:651140804,phase:2,deny,log,msg:'test ru }, }, }, - expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:1061846204,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',severity:'emergency',t:lowercase,chain" -SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:2595762349,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',t:lowercase,skip:2" -SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:1714963250,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',t:lowercase,chain" -SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:1519945803,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',t:lowercase"`, + expected: `SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:1202606032,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',severity:'emergency',t:lowercase,chain" +SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:359710097,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',t:lowercase,skip:2" +SecRule ARGS_GET:foo "@rx [^a-zA-Z]" "id:3120021894,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',t:lowercase,chain" +SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:2229098023,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR AND mix',tag:'cs-custom-rule',t:lowercase"`, }, { // (A OR B) AND C → DNF: (A AND C) OR (B AND C) @@ -214,10 +214,10 @@ SecRule ARGS_GET:bar "@rx [^a-zA-Z]" "id:1519945803,phase:2,deny,log,msg:'test r }, }, }, - expected: `SecRule ARGS_GET:a "@rx x" "id:92468354,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule',severity:'emergency',t:lowercase,chain" -SecRule REQUEST_METHOD "@streq GET" "id:247410534,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule',skip:2" -SecRule ARGS_GET:b "@rx x" "id:1259128012,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule',t:lowercase,chain" -SecRule REQUEST_METHOD "@streq GET" "id:1682421760,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule'"`, + expected: `SecRule ARGS_GET:a "@rx x" "id:1800648982,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule',severity:'emergency',t:lowercase,chain" +SecRule REQUEST_METHOD "@streq GET" "id:1587988738,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule',skip:2" +SecRule ARGS_GET:b "@rx x" "id:3192999968,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule',t:lowercase,chain" +SecRule REQUEST_METHOD "@streq GET" "id:3752698388,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR inside AND',tag:'cs-custom-rule'"`, }, { // (A OR B) AND (C OR D) → DNF: (A,C) OR (A,D) OR (B,C) OR (B,D) @@ -256,14 +256,14 @@ SecRule REQUEST_METHOD "@streq GET" "id:1682421760,phase:2,deny,log,msg:'test ru }, }, }, - expected: `SecRule ARGS_GET:a "@rx x" "id:1377665195,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',severity:'emergency',chain" -SecRule REQUEST_HEADERS:c "@contains y" "id:943662042,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',skip:6" -SecRule ARGS_GET:a "@rx x" "id:2583268057,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',chain" -SecRule REQUEST_HEADERS:d "@contains y" "id:4169811748,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',skip:4" -SecRule ARGS_GET:b "@rx x" "id:3596628535,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',chain" -SecRule REQUEST_HEADERS:c "@contains y" "id:1569541606,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',skip:2" -SecRule ARGS_GET:b "@rx x" "id:3129875829,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',chain" -SecRule REQUEST_HEADERS:d "@contains y" "id:2899769488,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule'"`, + expected: `SecRule ARGS_GET:a "@rx x" "id:4076736575,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',severity:'emergency',chain" +SecRule REQUEST_HEADERS:c "@contains y" "id:3029228238,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',skip:6" +SecRule ARGS_GET:a "@rx x" "id:3671513773,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',chain" +SecRule REQUEST_HEADERS:d "@contains y" "id:2068477400,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',skip:4" +SecRule ARGS_GET:b "@rx x" "id:2234139251,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',chain" +SecRule REQUEST_HEADERS:c "@contains y" "id:1870138418,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',skip:2" +SecRule ARGS_GET:b "@rx x" "id:1039790593,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule',chain" +SecRule REQUEST_HEADERS:d "@contains y" "id:3758321836,phase:2,deny,log,msg:'test rule',tag:'crowdsec-OR cross product',tag:'cs-custom-rule'"`, }, { // Same level and+or: POST AND (a OR b) @@ -290,10 +290,10 @@ SecRule REQUEST_HEADERS:d "@contains y" "id:2899769488,phase:2,deny,log,msg:'tes }, }, }, - expected: `SecRule REQUEST_METHOD "@streq POST" "id:2771836947,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule',severity:'emergency',chain" -SecRule ARGS_GET:a "@rx x" "id:477818162,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule',skip:2" -SecRule REQUEST_METHOD "@streq POST" "id:278067945,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule',chain" -SecRule ARGS_GET:b "@rx x" "id:1867070580,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule'"`, + expected: `SecRule REQUEST_METHOD "@streq POST" "id:3395404975,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule',severity:'emergency',chain" +SecRule ARGS_GET:a "@rx x" "id:3880988910,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule',skip:2" +SecRule REQUEST_METHOD "@streq POST" "id:3808442053,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule',chain" +SecRule ARGS_GET:b "@rx x" "id:1637823048,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Same level AND OR',tag:'cs-custom-rule'"`, }, { // A AND (B OR (C AND D)) → DNF: (A,B) OR (A,C,D) @@ -331,11 +331,11 @@ SecRule ARGS_GET:b "@rx x" "id:1867070580,phase:2,deny,log,msg:'test rule',tag:' }, }, }, - expected: `SecRule REQUEST_METHOD "@streq POST" "id:1367877911,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',severity:'emergency',chain" -SecRule REQUEST_FILENAME "@contains /admin" "id:889480160,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',t:lowercase,skip:3" -SecRule REQUEST_METHOD "@streq POST" "id:3660599789,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',chain" -SecRule ARGS_GET:cmd "@rx exec" "id:713704559,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',chain" -SecRule REQUEST_HEADERS:x-debug "@streq true" "id:625013554,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule'"`, + expected: `SecRule REQUEST_METHOD "@streq POST" "id:4151628139,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',severity:'emergency',chain" +SecRule REQUEST_FILENAME "@contains /admin" "id:2534092724,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',t:lowercase,skip:3" +SecRule REQUEST_METHOD "@streq POST" "id:220760465,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',chain" +SecRule ARGS_GET:cmd "@rx exec" "id:2515501355,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule',chain" +SecRule REQUEST_HEADERS:x-debug "@streq true" "id:1447329030,phase:2,deny,log,msg:'test rule',tag:'crowdsec-Deep nesting',tag:'cs-custom-rule'"`, }, { name: "all transforms", @@ -350,7 +350,7 @@ SecRule REQUEST_HEADERS:x-debug "@streq true" "id:625013554,phase:2,deny,log,msg for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - actual, _, err := tt.rule.Convert(ModsecurityRuleType, tt.name, tt.description) + actual, _, err := tt.rule.Convert(ModsecurityRuleType, tt.name, tt.description, 0) if err != nil { t.Errorf("Error converting rule: %s", err) } @@ -392,8 +392,46 @@ func TestDNFExpansionLimit(t *testing.T) { }, } - _, _, err := rule.Convert(ModsecurityRuleType, "test", "test") + _, _, err := rule.Convert(ModsecurityRuleType, "test", "test", 0) if err == nil { t.Error("Expected error for excessive DNF expansion, got nil") } } + +// TestRuleIDUniqueAcrossRulesInCollection: an identical leaf shared by two +// rules of the same collection must not collide on the same id (positions +// restart at 0 per rule). Regression test for crowdsecurity/vpatch-CVE-2024-34102. +func TestRuleIDUniqueAcrossRulesInCollection(t *testing.T) { + const collection = "crowdsecurity/test" + + // identical zone/match/transform in both rules + sharedLeaf := CustomRule{ + Zones: []string{"URI"}, + Transform: []string{"lowercase"}, + Match: Match{Type: "contains", Value: "/admin"}, + } + + rule0 := CustomRule{And: []CustomRule{ + {Zones: []string{"METHOD"}, Match: Match{Type: "equals", Value: "POST"}}, + sharedLeaf, + }} + rule1 := CustomRule{And: []CustomRule{ + {Zones: []string{"METHOD"}, Match: Match{Type: "equals", Value: "GET"}}, + sharedLeaf, + }} + + _, ids0, err := rule0.Convert(ModsecurityRuleType, collection, "", 0) + require.NoError(t, err) + + _, ids1, err := rule1.Convert(ModsecurityRuleType, collection, "", 1) + require.NoError(t, err) + + seen := make(map[uint32]bool, len(ids0)) + for _, id := range ids0 { + seen[id] = true + } + + for _, id := range ids1 { + require.Falsef(t, seen[id], "id %d generated by two different rules of the same collection", id) + } +} diff --git a/pkg/appsec/appsec_rule/modsecurity.go b/pkg/appsec/appsec_rule/modsecurity.go index e935a779d23..22904a4c5a1 100644 --- a/pkg/appsec/appsec_rule/modsecurity.go +++ b/pkg/appsec/appsec_rule/modsecurity.go @@ -12,6 +12,10 @@ import ( type ModsecurityRule struct { ids []uint32 + // ruleIndex is the rule's position in its collection, mixed into ids so + // identical leaves of different rules don't collide (positions restart at 0 + // per rule). + ruleIndex int } var zonesMap = map[string]string{ @@ -94,7 +98,9 @@ var bodyTypeMatch = map[string]string{ const maxDNFGroups = 50 -func (m *ModsecurityRule) Build(rule *CustomRule, appsecRuleName string, appsecRuleDescription string) (string, []uint32, error) { +func (m *ModsecurityRule) Build(rule *CustomRule, appsecRuleName string, appsecRuleDescription string, ruleIndex int) (string, []uint32, error) { + m.ruleIndex = ruleIndex + if rule.Severity == "" { rule.Severity = cztypes.RuleSeverityEmergency.String() } @@ -223,6 +229,8 @@ func (m *ModsecurityRule) generateRuleID(rule *CustomRule, appsecRuleName string h.Write([]byte(rule.Match.Value)) h.Write([]byte(fmt.Sprintf("%d", position))) + h.Write([]byte(fmt.Sprintf("rule:%d", m.ruleIndex))) + for _, zone := range rule.Zones { h.Write([]byte(zone)) } diff --git a/pkg/appsec/appsec_rules_collection.go b/pkg/appsec/appsec_rules_collection.go index 41c0b122982..c36a02d432d 100644 --- a/pkg/appsec/appsec_rules_collection.go +++ b/pkg/appsec/appsec_rules_collection.go @@ -117,9 +117,9 @@ func LoadCollection(pattern string, logger *log.Entry, hub *cwhub.Hub) ([]Appsec } if appsecRule.Rules != nil { - for _, rule := range appsecRule.Rules { + for i, rule := range appsecRule.Rules { rule.Severity = appsecRule.Severity - strRule, rulesId, err := rule.Convert(appsec_rule.ModsecurityRuleType, appsecRule.Name, appsecRule.Description) + strRule, rulesId, err := rule.Convert(appsec_rule.ModsecurityRuleType, appsecRule.Name, appsecRule.Description, i) if err != nil { logger.Errorf("unable to convert rule %s : %s", appsecRule.Name, err) return nil, err @@ -128,19 +128,20 @@ func LoadCollection(pattern string, logger *log.Entry, hub *cwhub.Hub) ([]Appsec logger.Debugf("Adding rule %s", strRule) appsecCol.Rules = append(appsecCol.Rules, strRule) - // We only take the first id, as it's the one of the "main" rule - if _, ok := AppsecRulesDetails[int(rulesId[0])]; !ok { - AppsecRulesDetails[int(rulesId[0])] = RulesDetails{ - LogLevel: log.InfoLevel, - Hash: appsecRule.hash, - Version: appsecRule.version, - Name: appsecRule.Name, + // An `or` expands into several SecRules; register every id (not + // just the first) so the name resolves whichever branch matches. + for _, id := range rulesId { + if _, ok := AppsecRulesDetails[int(id)]; !ok { + AppsecRulesDetails[int(id)] = RulesDetails{ + LogLevel: log.InfoLevel, + Hash: appsecRule.hash, + Version: appsecRule.version, + Name: appsecRule.Name, + } + } else { + logger.Warnf("conflicting id %d for rule %s !", id, appsecRule.Name) } - } else { - logger.Warnf("conflicting id %d for rule %s !", rulesId[0], rule.Name) - } - for _, id := range rulesId { SetRuleDebug(int(id), appsecRule.Debug) } } diff --git a/pkg/appsec/appsec_rules_collection_test.go b/pkg/appsec/appsec_rules_collection_test.go new file mode 100644 index 00000000000..3e0edac8ece --- /dev/null +++ b/pkg/appsec/appsec_rules_collection_test.go @@ -0,0 +1,113 @@ +package appsec + +import ( + "testing" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" + + "github.com/crowdsecurity/crowdsec/pkg/appsec/appsec_rule" + "github.com/crowdsecurity/crowdsec/pkg/cwhub" +) + +// withCollection swaps in a fresh set of package globals for the duration of a +// test and restores them afterwards. LoadCollection reads appsecRules and +// writes AppsecRulesDetails, both of which are package-level state. +func withCollection(t *testing.T, cfg AppsecCollectionConfig) { + t.Helper() + + savedRules := appsecRules + savedDetails := AppsecRulesDetails + + t.Cleanup(func() { + appsecRules = savedRules + AppsecRulesDetails = savedDetails + }) + + appsecRules = map[string]AppsecCollectionConfig{cfg.Name: cfg} + AppsecRulesDetails = make(map[int]RulesDetails) +} + +// TestLoadCollectionRegistersAllRuleIDs: every SecRule from an `or` rule must +// be registered, so any matching branch resolves to the rule name instead of +// being mistaken for a native seclang rule. Regression test for #4340. +func TestLoadCollectionRegistersAllRuleIDs(t *testing.T) { + orRule := appsec_rule.CustomRule{ + Or: []appsec_rule.CustomRule{ + {Zones: []string{"ARGS"}, Variables: []string{"foo"}, Match: appsec_rule.Match{Type: "equals", Value: "toto"}}, + {Zones: []string{"ARGS"}, Variables: []string{"bar"}, Match: appsec_rule.Match{Type: "equals", Value: "tutu"}}, + {Zones: []string{"ARGS"}, Variables: []string{"baz"}, Match: appsec_rule.Match{Type: "equals", Value: "titi"}}, + }, + } + + cfg := AppsecCollectionConfig{ + Name: "test/or-rule", + Description: "my or rule description", + Rules: []appsec_rule.CustomRule{orRule}, + hash: "deadbeef", + version: "1.2.3", + } + + // Compute the full set of generated ids independently of LoadCollection. + // orRule is the first (index 0) rule of the collection. + _, expectedIDs, err := orRule.Convert(appsec_rule.ModsecurityRuleType, cfg.Name, cfg.Description, 0) + require.NoError(t, err) + // An `or` with three branches must produce more than one rule, otherwise + // this test would not actually exercise the bug. + require.Greater(t, len(expectedIDs), 1) + + withCollection(t, cfg) + + _, err = LoadCollection(cfg.Name, log.NewEntry(log.StandardLogger()), &cwhub.Hub{}) + require.NoError(t, err) + + // Every generated id (not just the first) must resolve to the rule details. + for _, id := range expectedIDs { + details, ok := AppsecRulesDetails[int(id)] + require.Truef(t, ok, "id %d not registered in AppsecRulesDetails", id) + require.Equal(t, cfg.Name, details.Name) + require.Equal(t, cfg.hash, details.Hash) + require.Equal(t, cfg.version, details.Version) + } +} + +// TestLoadCollectionNoCrossRuleIDCollision reproduces +// crowdsecurity/vpatch-CVE-2024-34102: a collection whose two rules share an +// identical leaf must still generate distinct, registered ids (no "conflicting +// id" warning). +func TestLoadCollectionNoCrossRuleIDCollision(t *testing.T) { + sharedLeaf := appsec_rule.CustomRule{ + Zones: []string{"URI"}, + Transform: []string{"lowercase"}, + Match: appsec_rule.Match{Type: "contains", Value: "/admin"}, + } + + cfg := AppsecCollectionConfig{ + Name: "test/shared-leaf", + Rules: []appsec_rule.CustomRule{ + {And: []appsec_rule.CustomRule{ + {Zones: []string{"METHOD"}, Match: appsec_rule.Match{Type: "equals", Value: "POST"}}, + sharedLeaf, + }}, + {And: []appsec_rule.CustomRule{ + {Zones: []string{"METHOD"}, Match: appsec_rule.Match{Type: "equals", Value: "GET"}}, + sharedLeaf, + }}, + }, + hash: "cafe", + version: "1.0.0", + } + + withCollection(t, cfg) + + _, err := LoadCollection(cfg.Name, log.NewEntry(log.StandardLogger()), &cwhub.Hub{}) + require.NoError(t, err) + + // Both rules generate 2 leaves each. The shared URI leaf must not collapse + // two of them into one id. + require.Len(t, AppsecRulesDetails, 4, "expected 4 distinct ids across the collection's two rules") + + for id, details := range AppsecRulesDetails { + require.Equalf(t, cfg.Name, details.Name, "id %d resolved to wrong rule name", id) + } +}