[WAF] do not iterate over all transaction variables for nothing // swap alert generation + event send order (#3884)

Author: blotus

pkg/acquisition/modules/appsec/appsec_hooks_test.go

@@ -361,10 +361,10 @@ func TestAppsecOnMatchHooks(t *testing.T) {
 				require.Equal(t, http.StatusOK, statusCode)
 				// We have both an event an overflow
 				require.Len(t, events, 2)
-				require.Equal(t, types.LOG, events[0].Type)
-				require.Equal(t, types.APPSEC, events[1].Type)
-				require.Nil(t, events[0].Overflow.Alert)
-				require.NotNil(t, events[1].Overflow.Alert)
+				require.Equal(t, types.APPSEC, events[0].Type)
+				require.Equal(t, types.LOG, events[1].Type)
+				require.Nil(t, events[1].Overflow.Alert)
+				require.NotNil(t, events[0].Overflow.Alert)
 			},
 		},
 		{

pkg/acquisition/modules/appsec/appsec_runner.go

@@ -320,11 +320,10 @@ func (r *AppsecRunner) handleOutBandInterrupt(request *appsec.ParsedRequest) {
 			r.logger.Errorf("unable to process OnMatch rules: %s", err)
 			return
 		}
-		// Should the match trigger an event ?
-		if r.AppsecRuntime.Response.SendEvent {
-			r.outChan <- evt
-		}
 
+		// The alert needs to be sent first:
+		// The event and the alert share the same internal map (parsed, meta, ...)
+		// The event can be modified by the parsers, which might cause a concurrent map read/write
 		// Should the match trigger an overflow ?
 		if r.AppsecRuntime.Response.SendAlert {
 			appsecOvlfw, err := AppsecEventGeneration(evt, request.HTTPRequest)
@@ -336,6 +335,11 @@ func (r *AppsecRunner) handleOutBandInterrupt(request *appsec.ParsedRequest) {
 				r.outChan <- *appsecOvlfw
 			}
 		}
+
+		// Should the match trigger an event ?
+		if r.AppsecRuntime.Response.SendEvent {
+			r.outChan <- evt
+		}
 	}
 }
 

pkg/acquisition/modules/appsec/utils.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"slices"
 	"strings"
 	"time"
 
@@ -34,6 +35,18 @@ var excludedMatchCollections = []string{
 	"TX",                // Score has been exceeded
 }
 
+var CRSAnomalyScores = []string{
+	"sql_injection_score",
+	"xss_score",
+	"rfi_score",
+	"lfi_score",
+	"rce_score",
+	"php_injection_score",
+	"http_violation_score",
+	"session_fixation_score",
+	"anomaly_score",
+}
+
 func AppsecEventGenerationGeoIPEnrich(src *models.Source) error {
 
 	if src == nil || src.Scope == nil || *src.Scope != types.Ip {
@@ -79,9 +92,9 @@ func formatCRSMatch(vars map[string]string, hasInBandMatches bool, hasOutBandMat
 	case hasOutBandMatches:
 		msg += "out-of-band: "
 	}
-	for _, var_name := range appsec.CRSAnomalyScores {
-		if val, ok := vars[var_name]; ok && val != "0" {
-			msg += fmt.Sprintf("%s: %s, ", strings.Replace(strings.Replace(var_name, "TX.", "", 1), "_score", "", 1), val)
+	for _, var_name := range CRSAnomalyScores {
+		if val, ok := vars["TX."+var_name]; ok && val != "0" {
+			msg += fmt.Sprintf("%s: %s, ", strings.Replace(var_name, "_score", "", 1), val)
 		}
 	}
 	return msg
@@ -349,31 +362,43 @@ func (r *AppsecRunner) AccumulateTxToEvent(evt *types.Event, req *appsec.ParsedR
 		evt.Appsec.Vars = map[string]string{}
 	}
 
-	req.Tx.Variables().All(func(v variables.RuleVariable, col collection.Collection) bool {
-		for _, variable := range col.FindAll() {
-			r.logger.Tracef("variable: %s.%s = %s\n", variable.Variable().Name(), variable.Key(), variable.Value())
-			key := variable.Variable().Name()
-			if variable.Key() != "" {
-				key += "." + variable.Key()
-			}
+	txCollection := req.Tx.Variables().TX()
 
-			if variable.Value() == "" {
-				continue
-			}
+	txMatchedData := txCollection.FindAll()
+
+	for _, match := range txMatchedData {
+		if slices.Contains(CRSAnomalyScores, match.Key()) {
+			evt.Appsec.Vars["TX."+match.Key()] = match.Value()
+		}
+	}
 
-			for _, collectionToKeep := range r.AppsecRuntime.CompiledVariablesTracking {
-				match := collectionToKeep.MatchString(key)
-				if match {
-					evt.Appsec.Vars[key] = variable.Value()
-					r.logger.Debugf("%s.%s = %s", variable.Variable().Name(), variable.Key(), variable.Value())
-				} else {
-					r.logger.Debugf("%s.%s != %s (%s) (not kept)", variable.Variable().Name(), variable.Key(), collectionToKeep, variable.Value())
+	if len(r.AppsecRuntime.CompiledVariablesTracking) > 0 {
+		req.Tx.Variables().All(func(v variables.RuleVariable, col collection.Collection) bool {
+			for _, variable := range col.FindAll() {
+				r.logger.Tracef("variable: %s.%s = %s\n", variable.Variable().Name(), variable.Key(), variable.Value())
+				key := variable.Variable().Name()
+				if variable.Key() != "" {
+					key += "." + variable.Key()
+				}
+
+				if variable.Value() == "" {
+					continue
+				}
+
+				for _, collectionToKeep := range r.AppsecRuntime.CompiledVariablesTracking {
+					match := collectionToKeep.MatchString(key)
+					if match {
+						evt.Appsec.Vars[key] = variable.Value()
+						r.logger.Debugf("%s.%s = %s", variable.Variable().Name(), variable.Key(), variable.Value())
+					} else {
+						r.logger.Debugf("%s.%s != %s (%s) (not kept)", variable.Variable().Name(), variable.Key(), collectionToKeep, variable.Value())
+					}
 				}
 			}
-		}
 
-		return true
-	})
+			return true
+		})
+	}
 
 	for _, rule := range req.Tx.MatchedRules() {
 		// Drop the rule if it has no message (it's likely a CRS setup rule)

pkg/appsec/appsec.go

@@ -38,18 +38,6 @@ const (
 	AllowRemediation   = "allow"
 )
 
-var CRSAnomalyScores = []string{
-	"TX.sql_injection_score",
-	"TX.xss_score",
-	"TX.rfi_score",
-	"TX.lfi_score",
-	"TX.rce_score",
-	"TX.php_injection_score",
-	"TX.http_violation_score",
-	"TX.session_fixation_score",
-	"TX.anomaly_score",
-}
-
 func (h *Hook) Build(hookStage int) error {
 	ctx := map[string]interface{}{}
 
@@ -227,9 +215,6 @@ func (wc *AppsecConfig) LoadByPath(file string) error {
 		wc.VariablesTracking = append(wc.VariablesTracking, tmp.VariablesTracking...)
 	}
 
-	// In all cases, track the CRS anomaly scores
-	wc.VariablesTracking = append(wc.VariablesTracking, CRSAnomalyScores...)
-
 	// override other options
 	wc.LogLevel = tmp.LogLevel