diff --git a/internal/actions/severity.go b/internal/actions/severity.go index 6dd404cd7..f6855063b 100644 --- a/internal/actions/severity.go +++ b/internal/actions/severity.go @@ -46,6 +46,7 @@ func (a *severityFn) Init(r plugintypes.RuleMetadata, data string) error { return err } r.(*corazawaf.Rule).Severity_ = sev + r.(*corazawaf.Rule).HasSeverity_ = true return nil } diff --git a/internal/corazarules/rule.go b/internal/corazarules/rule.go index 09822481e..b0f11278d 100644 --- a/internal/corazarules/rule.go +++ b/internal/corazarules/rule.go @@ -20,14 +20,17 @@ type RuleMetadata struct { Line_ int Rev_ string Severity_ types.RuleSeverity - Version_ string - Tags_ []string - Maturity_ int - Accuracy_ int - Operator_ string - Phase_ types.RulePhase - Raw_ string - SecMark_ string + // HasSeverity_ reports whether the rule explicitly set a severity action. + // This distinguishes an unset value from RuleSeverityEmergency (0). + HasSeverity_ bool + Version_ string + Tags_ []string + Maturity_ int + Accuracy_ int + Operator_ string + Phase_ types.RulePhase + Raw_ string + SecMark_ string // Contains the Id of the parent rule if you are inside // a chain. Otherwise, it will be 0 ParentID_ int diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index d868b51cf..af0daf452 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -555,10 +555,19 @@ func (tx *Transaction) MatchRule(r *Rule, mds []types.MatchData) { } // set highest_severity - hs := tx.variables.highestSeverity - maxSeverity, _ := types.ParseRuleSeverity(hs.Get()) - if r.Severity_ > maxSeverity { - hs.Set(strconv.Itoa(r.Severity_.Int())) + // Only update when severity was explicitly set via the severity action. + // This mirrors ModSecurity v3 behavior (m_severity > -1 check in transaction.cc). + if r.HasSeverity_ { + hs := tx.variables.highestSeverity + currentVal := defaultHighestSeverity + if v := hs.Get(); v != "" { + if parsed, err := strconv.Atoi(v); err == nil { + currentVal = parsed + } + } + if r.Severity_.Int() < currentVal { + hs.Set(strconv.Itoa(r.Severity_.Int())) + } } mr := &corazarules.MatchedRule{ diff --git a/internal/corazawaf/waf.go b/internal/corazawaf/waf.go index 54e28eaac..f72f07889 100644 --- a/internal/corazawaf/waf.go +++ b/internal/corazawaf/waf.go @@ -32,6 +32,14 @@ var wafIDCounter atomic.Uint64 const ( // DefaultRequestBodyJsonDepthLimit is the default limit for the depth of JSON objects in the request body DefaultRequestBodyJsonDepthLimit = 1024 + + // defaultHighestSeverity is the default value for HIGHEST_SEVERITY when no rules + // with severity have been matched. Value 255 aligns with ModSecurity behavior: + // - ModSec v2: apache2/msc_util.c highest_severity initialized to 255 + // https://github.com/owasp-modsecurity/ModSecurity/blob/v2/master/apache2/msc_util.c + // - ModSec v3: src/transaction.cc m_highestSeverity initialized to 255 + // https://github.com/owasp-modsecurity/ModSecurity/blob/v3/master/src/transaction.cc + defaultHighestSeverity = 255 ) // WAF instance is used to store configurations and rules @@ -259,7 +267,7 @@ func (w *WAF) newTransaction(opts Options) *Transaction { tx.variables.reqbodyProcessorError.Set("0") tx.variables.requestBodyLength.Set("0") tx.variables.duration.Set("0") - tx.variables.highestSeverity.Set("0") + tx.variables.highestSeverity.Set(strconv.Itoa(defaultHighestSeverity)) tx.variables.uniqueID.Set(tx.id) tx.setTimeVariables() diff --git a/testing/engine/rulemetadata.go b/testing/engine/rulemetadata.go index 8ba4f6d3d..34bf211c9 100644 --- a/testing/engine/rulemetadata.go +++ b/testing/engine/rulemetadata.go @@ -33,3 +33,88 @@ SecAction "id:1, log, severity:5" SecRule HIGHEST_SEVERITY "@eq 5" "id:2, log" `, }) + +var _ = profile.RegisterProfile(profile.Profile{ + Meta: profile.Meta{ + Author: "majiayu000", + Description: "Test HIGHEST_SEVERITY defaults to 255 when no rules with severity fire", + Enabled: true, + Name: "rulemetadata_default_severity.yaml", + }, + Tests: []profile.Test{ + { + Title: "highest_severity_default", + Stages: []profile.Stage{ + { + Stage: profile.SubStage{ + Output: profile.ExpectedOutput{ + TriggeredRules: []int{1}, + }, + }, + }, + }, + }, + }, + Rules: ` +SecRule HIGHEST_SEVERITY "@eq 255" "id:1, log" +`, +}) + +// Regression: a rule without severity must not poison HIGHEST_SEVERITY. +// Without the HasSeverity_ guard, the no-severity rule's zero value (0) would +// win over a later explicit severity:5, leaving HIGHEST_SEVERITY stuck at 0. +var _ = profile.RegisterProfile(profile.Profile{ + Meta: profile.Meta{ + Author: "majiayu000", + Description: "Test that rules without severity do not poison HIGHEST_SEVERITY", + Enabled: true, + Name: "rulemetadata_no_severity_poison.yaml", + }, + Tests: []profile.Test{ + { + Title: "no_severity_then_explicit", + Stages: []profile.Stage{ + { + Stage: profile.SubStage{ + Output: profile.ExpectedOutput{ + TriggeredRules: []int{1, 2, 3}, + }, + }, + }, + }, + }, + }, + Rules: ` +SecAction "id:1, log" +SecAction "id:2, log, severity:5" +SecRule HIGHEST_SEVERITY "@eq 5" "id:3, log" +`, +}) + +var _ = profile.RegisterProfile(profile.Profile{ + Meta: profile.Meta{ + Author: "majiayu000", + Description: "Test HIGHEST_SEVERITY with multiple severities keeps the lowest number", + Enabled: true, + Name: "rulemetadata_highest_severity.yaml", + }, + Tests: []profile.Test{ + { + Title: "highest_severity_multiple", + Stages: []profile.Stage{ + { + Stage: profile.SubStage{ + Output: profile.ExpectedOutput{ + TriggeredRules: []int{1, 2, 3}, + }, + }, + }, + }, + }, + }, + Rules: ` +SecAction "id:1, log, severity:5" +SecAction "id:2, log, severity:2" +SecRule HIGHEST_SEVERITY "@eq 2" "id:3, log" +`, +})