From 4f96909eec7f79f13290c52c05b2925a25bd9a8d Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Fri, 2 Oct 2026 16:37:02 +0200 Subject: [PATCH] fix(corazawaf): key transformation cache on the argument value The per-phase transformation cache was keyed on the argument key's string pointer and its index in the GetField result. Repeated keys share one key string, and map-backed collections iterate in random order, so a rule could get the cached transformation of a different value with the same key. Key the cache on the value's data pointer and length instead: a transformation is a pure function of the value. Same approach as upstream corazawaf/coraza#1682. Co-Authored-By: Claude Opus 5.5 --- internal/corazawaf/rule.go | 20 ++++++++++---------- internal/corazawaf/rule_test.go | 33 ++++++++++++++++++++++++++------- internal/corazawaf/rulegroup.go | 19 ++++++++++--------- 3 files changed, 46 insertions(+), 26 deletions(-) diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go index 0d5e81127..5b47ab48f 100644 --- a/internal/corazawaf/rule.go +++ b/internal/corazawaf/rule.go @@ -265,12 +265,12 @@ func (r *Rule) doEvaluate(logger debuglog.Logger, phase types.RulePhase, tx *Tra args := make([]string, 1) var errs []error var argsLen int - for i, arg := range values { + for _, arg := range values { if r.MultiMatch { args, errs = r.transformMultiMatchArg(arg) argsLen = len(args) } else { - args[0], errs = r.transformArg(arg, i, cache) + args[0], errs = r.transformArg(arg, cache) argsLen = 1 } if len(errs) > 0 { @@ -424,7 +424,7 @@ func (r *Rule) transformMultiMatchArg(arg types.MatchData) ([]string, []error) { return r.executeTransformationsMultimatch(arg.Value()) } -func (r *Rule) transformArg(arg types.MatchData, argIdx int, cache map[transformationKey]transformationValue) (string, []error) { +func (r *Rule) transformArg(arg types.MatchData, cache map[transformationKey]transformationValue) (string, []error) { switch { case len(r.transformations) == 0: return arg.Value(), nil @@ -434,20 +434,20 @@ func (r *Rule) transformArg(arg types.MatchData, argIdx int, cache map[transform return arg, errs default: // NOTE: See comment on transformationKey struct to understand this hacky code - argKey := arg.Key() - argKeyPtr := unsafe.StringData(argKey) + argValue := arg.Value() + argValuePtr := unsafe.StringData(argValue) // Search from longest prefix (full chain) backwards for a cache hit. // Best case: full chain cached → single map lookup, done. // Typical case: shared prefix cached → start computing from there. startIdx := 0 - value := arg.Value() + value := argValue var errs []error for i := len(r.transformationPrefixIDs) - 1; i >= 0; i-- { key := transformationKey{ - argKey: argKeyPtr, - argIndex: argIdx, + argValue: argValuePtr, + argValueLen: len(argValue), argVariable: arg.Variable(), transformationsID: r.transformationPrefixIDs[i], } @@ -474,8 +474,8 @@ func (r *Rule) transformArg(arg types.MatchData, argIdx int, cache map[transform } key := transformationKey{ - argKey: argKeyPtr, - argIndex: argIdx, + argValue: argValuePtr, + argValueLen: len(argValue), argVariable: arg.Variable(), transformationsID: r.transformationPrefixIDs[i], } diff --git a/internal/corazawaf/rule_test.go b/internal/corazawaf/rule_test.go index 2ada3ce0f..e93c80a33 100644 --- a/internal/corazawaf/rule_test.go +++ b/internal/corazawaf/rule_test.go @@ -623,7 +623,7 @@ func TestTransformArgSimple(t *testing.T) { rule := NewRule() _ = rule.AddTransformation("AppendA", transformationAppendA) _ = rule.AddTransformation("AppendB", transformationAppendB) - arg, errs := rule.transformArg(md, 0, transformationCache) + arg, errs := rule.transformArg(md, transformationCache) if errs != nil { t.Fatalf("Unexpected errors executing transformations: %v", errs) } @@ -635,7 +635,7 @@ func TestTransformArgSimple(t *testing.T) { t.Errorf("Expected 2 transformations in cache (one per step), got %d", len(transformationCache)) } // Repeating the same transformation, expecting still two elements in the cache (cache hit) - arg, errs = rule.transformArg(md, 0, transformationCache) + arg, errs = rule.transformArg(md, transformationCache) if errs != nil { t.Fatalf("Unexpected errors executing transformations: %v", errs) } @@ -656,7 +656,7 @@ func TestTransformArgNoCacheForTXVariable(t *testing.T) { } rule := NewRule() _ = rule.AddTransformation("AppendA", transformationAppendA) - arg, errs := rule.transformArg(md, 0, transformationCache) + arg, errs := rule.transformArg(md, transformationCache) if errs != nil { t.Fatalf("Unexpected errors executing transformations: %v", errs) } @@ -686,7 +686,7 @@ func TestTransformArgPrefixSharing(t *testing.T) { _ = rule2.AddTransformation("AppendB", transformationAppendB) // Evaluate rule1 first — caches the AppendA intermediate - arg1, errs := rule1.transformArg(md, 0, transformationCache) + arg1, errs := rule1.transformArg(md, transformationCache) if errs != nil { t.Fatalf("Unexpected errors: %v", errs) } @@ -698,7 +698,7 @@ func TestTransformArgPrefixSharing(t *testing.T) { } // Evaluate rule2 — should reuse the cached AppendA result and only compute AppendB - arg2, errs := rule2.transformArg(md, 0, transformationCache) + arg2, errs := rule2.transformArg(md, transformationCache) if errs != nil { t.Fatalf("Unexpected errors: %v", errs) } @@ -711,6 +711,25 @@ func TestTransformArgPrefixSharing(t *testing.T) { } } +// Not a profile: whether a profile reaches the collision depends on map iteration order. +func TestTransformArgRepeatedKeyDifferentValues(t *testing.T) { + transformationCache := map[transformationKey]transformationValue{} + // Repeated keys share one key string, as when args are added from a map[string][]string. + key := "q" + benign := &corazarules.MatchData{Variable_: variables.ArgsGet, Key_: key, Value_: "benign"} + evil := &corazarules.MatchData{Variable_: variables.ArgsGet, Key_: key, Value_: "evil"} + + rule := NewRule() + _ = rule.AddTransformation("AppendA", transformationAppendA) + + if arg, _ := rule.transformArg(benign, transformationCache); arg != "benignA" { + t.Errorf("Expected \"benignA\", got %q", arg) + } + if arg, _ := rule.transformArg(evil, transformationCache); arg != "evilA" { + t.Errorf("Expected \"evilA\", got %q (cached result of another value)", arg) + } +} + func TestClearTransformationsResetsID(t *testing.T) { transformationCache := map[transformationKey]transformationValue{} md := &corazarules.MatchData{ @@ -730,8 +749,8 @@ func TestClearTransformationsResetsID(t *testing.T) { _ = ruleB.AddTransformation("AppendA", transformationAppendA) _ = ruleB.AddTransformation("AppendB", transformationAppendB) - argA, _ := ruleA.transformArg(md, 0, transformationCache) - argB, _ := ruleB.transformArg(md, 0, transformationCache) + argA, _ := ruleA.transformArg(md, transformationCache) + argB, _ := ruleB.transformArg(md, transformationCache) if argA != "testB" { t.Errorf("Rule A (t:none resets): expected \"testB\", got %q", argA) diff --git a/internal/corazawaf/rulegroup.go b/internal/corazawaf/rulegroup.go index 88134f922..a83772e20 100644 --- a/internal/corazawaf/rulegroup.go +++ b/internal/corazawaf/rulegroup.go @@ -288,15 +288,16 @@ func NewRuleGroup() RuleGroup { } type transformationKey struct { - // TODO(anuraaga): This is a big hack to support performance on TinyGo. TinyGo - // cannot efficiently compute a hashcode for a struct if it has embedded non-fixed - // size fields, for example string as we'd prefer to use here. A pointer is usable, - // and it works for us since we know that the arg key string is populated once per - // transaction phase and we would never have different string pointers with the same - // content, or more problematically same pointer for different content, as the strings - // will be alive throughout the phase. - argKey *byte - argIndex int + // Transformations are a pure function of the value, so the cache is keyed on + // the value itself. A pointer is used instead of the string because TinyGo + // cannot efficiently compute a hashcode for a struct with non-fixed size fields + // such as string. The pointer also keeps the value alive, so its address cannot + // be reused for different content while the entry is cached. The length is part + // of the key because two substrings of one buffer can share a start address. + // The argument key and its position must not be used: repeated keys share the + // same key string, and collection iteration order is not stable. + argValue *byte + argValueLen int argVariable variables.RuleVariable transformationsID int }