PII: fix In-App WAF attack sanitization

internal/backend/api/api.go

@@ -6,9 +6,10 @@ package api
 
 import (
 	"encoding/json"
-	"strings"
+	"regexp"
 	"time"
 
+	"github.com/sqreen/go-agent/internal/sqlib/sqerrors"
 	"github.com/sqreen/go-agent/internal/sqlib/sqsanitize"
 )
 
@@ -451,16 +452,22 @@ func (i *WAFAttackInfo) Scrub(scrubber *sqsanitize.Scrubber, info sqsanitize.Inf
 	// were scrubbed. The caller must have stored into info the values scrubbed
 	// from the request.
 	redactedString := scrubber.RedactedValueMask()
-	for e := range wafInfo {
+	for sanitized := range info {
+		re, err := regexp.Compile(`(?i)`+regexp.QuoteMeta(sanitized))
+		if err != nil {
+			return false, sqerrors.Wrapf(err, "could not ")
+		}
+
+		for e := range wafInfo {
 		for f := range wafInfo[e].Filter {
-			for v := range info {
 				resolvedValue := wafInfo[e].Filter[f].ResolvedValue
-				newStr := strings.ReplaceAll(resolvedValue, v, redactedString)
+
+				newStr := re.ReplaceAllString(resolvedValue, redactedString)
 				if newStr != resolvedValue {
 					// The string was changed
 					wafInfo[e].Filter[f].ResolvedValue = newStr
 					if wafInfo[e].Filter[f].MatchStatus != "" {
-						wafInfo[e].Filter[f].MatchStatus = strings.ReplaceAll(wafInfo[e].Filter[f].MatchStatus, v, redactedString)
+						wafInfo[e].Filter[f].MatchStatus = re.ReplaceAllString(wafInfo[e].Filter[f].MatchStatus, redactedString)
 					}
 					scrubbed = true
 				}

internal/backend/api/api_test.go

@@ -0,0 +1,111 @@
+package api_test
+
+import (
+	"encoding/json"
+	"regexp"
+	"strings"
+	"testing"
+
+	"github.com/sqreen/go-agent/internal/backend/api"
+	"github.com/sqreen/go-agent/internal/sqlib/sqsanitize"
+	"github.com/stretchr/testify/require"
+)
+
+func TestWAFAttackInfo_Scrub(t *testing.T) {
+	t.Run("AGO-137", func(t *testing.T) {
+		// Test case covering issue https://sqreen.atlassian.net/browse/AGO-137
+
+		// The idea of the following mess is to mock a WAF attack with a lowercase
+		// transformation of the request parameters. The fix should correctly scrub
+		// the request parameters in the attack.
+
+		// Set of PII values used in this test. They are uppercase here while they
+		// are lowercased in the attack information.
+		pii := map[string]string{
+			"access_token":  "PASSWORD_1",
+			"api_key":       "PASSWORD_2",
+			"apikey":        "PASSWORD_3",
+			"authorization": "PASSWORD_4",
+		}
+
+		// Create test data including the resolved values of the WAF using lowercase
+		// parameters and the request parameters including a separate attack
+		// parameter.
+		resolvedValues := make(map[string]string, len(pii))
+		params := make(map[string][]interface{}, len(pii))
+		for k, v := range pii {
+			resolvedValues[k] = strings.ToLower(v)
+			params[k] = []interface{}{v}
+		}
+		resolvedValues["attack"] = "java.lang.processbuilder"
+
+		// Prepare the WAF data json string
+		resolvedValuesJSON, err := json.Marshal(resolvedValues)
+		require.NoError(t, err)
+		resolvedValuesJSONStr, err := json.Marshal(string(resolvedValuesJSON))
+		require.NoError(t, err)
+		wafData := []byte(`[
+                         {
+                             "ret_code": 1,
+                             "flow": "shell_injection-monitoring",
+                             "step": "start",
+                             "rule": "rule_944100",
+                             "filter": [
+                                 {
+                                     "operator": "@rx",
+                                     "operator_value": "java\\.lang\\.(?:runtime|processbuilder)",
+                                     "binding_accessor": "#.Request.Body.String",
+                                     "resolved_value": ` + string(resolvedValuesJSONStr) + `,
+                                     "match_status": "java.lang.processbuilder"
+                                 }
+                             ]
+                         }
+                     ]`)
+
+		// Create a fake request record with the interesting parts for this test
+		// only
+		record := api.RequestRecord{
+			Request: api.RequestRecord_Request{
+				Parameters: api.RequestRecord_Request_Parameters{
+					Params: params,
+				},
+			},
+			Observed: api.RequestRecord_Observed{
+				Attacks: []*api.RequestRecord_Observed_Attack{
+					{Info: api.WAFAttackInfo{WAFData: wafData}},
+				},
+			},
+		}
+
+		// Create a scrubber of the PII values
+		keyRE := regexp.MustCompile(`(?i)(passw(((or)?d))|(phrase))|(secret)|(authorization)|(api_?key)|((access_?)?token)`)
+		valueRE := regexp.MustCompile(`(?:\d[ -]*?){13,16}`)
+		redactionString := "Redacted by Test"
+		scrubber := sqsanitize.NewScrubber(keyRE, valueRE, redactionString)
+
+		// Scrub the request record
+		info := sqsanitize.Info{}
+		scrubbed, err := record.Scrub(scrubber, info)
+
+		// It shouldn't fail and it should have scrubbed
+		require.NoError(t, err)
+		require.True(t, scrubbed)
+
+
+		scrubbedWAFData := string(record.Observed.Attacks[0].Info.(api.WAFAttackInfo).WAFData)
+
+		// Check that the count of redactions in the WAF info string is correct:
+		// one per PII value.
+		require.Equal(t, len(pii), strings.Count(scrubbedWAFData, redactionString))
+
+		// For each PII value
+		for k, v := range pii {
+			// Check that the returned scrubbed values contain the PII value
+			require.Contains(t, info, v)
+			// Check that the WAF data has been scrubbed
+			require.NotContains(t, scrubbedWAFData, v)
+			// Check that the request parameter has been scrubbed
+			require.Equal(t, redactionString, record.Request.Parameters.Params[k][0].(string))
+		}
+	})
+}

internal/sqlib/sqsanitize/sanitize.go

@@ -100,9 +100,7 @@ walk:
 		v = v.Elem()
 		goto walk
 
-	case reflect.Array:
-		fallthrough
-	case reflect.Slice:
+	case reflect.Array, reflect.Slice:
 		return s.scrubSlice(v, info)
 
 	case reflect.Map:
@@ -212,6 +210,8 @@ func (s *Scrubber) scrubSlice(v reflect.Value, info Info) (scrubbed bool) {
 }
 
 func (s *Scrubber) scrubMap(v reflect.Value, info Info) (scrubbed bool) {
+	var scrubEverything *Scrubber
+
 	vt := v.Type().Elem()
 	hasInterfaceValueType := vt.Kind() == reflect.Interface
 	hasStringKeyType := v.Type().Key().Kind() == reflect.String
@@ -223,9 +223,12 @@ func (s *Scrubber) scrubMap(v reflect.Value, info Info) (scrubbed bool) {
 		// value regular expression.
 		key := iter.Key()
 		if hasStringKeyType && !s.scrubEveryString && matchString(s.keyRegexp, key.String()) {
-			scrubber = new(Scrubber)
-			*scrubber = *s
-			scrubber.scrubEveryString = true
+			if scrubEverything == nil {
+				scrubEverything = new(Scrubber)
+				*scrubEverything = *scrubber
+				scrubEverything.scrubEveryString = true
+			}
+			scrubber = scrubEverything
 		}
 
 		// Map entries cannot be set. We therefore create a new value in order
@@ -247,9 +250,10 @@ func (s *Scrubber) scrubMap(v reflect.Value, info Info) (scrubbed bool) {
 		// the scrubber.
 		newVal := reflect.New(valT).Elem()
 		newVal.Set(val)
+
 		// Scrub it
-		if scrubbedElement := scrubber.scrubValue(newVal, info); scrubbedElement {
-			// Set it
+		if scrubber.scrubValue(newVal, info) {
+			// Replace it
 			v.SetMapIndex(key, newVal)
 			scrubbed = true
 		}
@@ -258,6 +262,8 @@ func (s *Scrubber) scrubMap(v reflect.Value, info Info) (scrubbed bool) {
 }
 
 func (s *Scrubber) scrubStruct(v reflect.Value, info Info) (scrubbed bool) {
+	var scrubEverything *Scrubber
+
 	l := v.NumField()
 	vt := v.Type()
 	for i := 0; i < l; i++ {
@@ -269,9 +275,12 @@ func (s *Scrubber) scrubStruct(v reflect.Value, info Info) (scrubbed bool) {
 
 		scrubber := s
 		if !s.scrubEveryString && matchString(s.keyRegexp, ft.Name) {
-			scrubber = new(Scrubber)
-			*scrubber = *s
-			scrubber.scrubEveryString = true
+			if scrubEverything == nil {
+				scrubEverything = new(Scrubber)
+				*scrubEverything = *scrubber
+				scrubEverything.scrubEveryString = true
+			}
+			scrubber = scrubEverything
 		}
 
 		f := v.Field(i)

internal/sqlib/sqsanitize/sanitize_test.go

@@ -1247,8 +1247,16 @@ func TestScrubber(t *testing.T) {
 				fuzzer.Fuzz(&multipartForm)
 
 				// Insert some values forbidden by the regular expression
-				postForm.Add("password", "1234")
-				postForm.Add("password", "5678")
+				postForm.Add("password", "password10")
+				postForm.Add("password", "password11")
+				postForm.Add("password", "password12")
+				postForm.Add("passwd", "password1")
+				postForm.Add("api_key", "password2")
+				postForm.Add("apikey", "password3")
+				postForm.Add("authorization", "password4")
+				postForm.Add("access_token", "password5")
+				postForm.Add("secret", "password6")
+
 				messageFormat := "here is my credit card number %s."
 				stringWithCreditCardNb := fmt.Sprintf(messageFormat, "4533-3432-3234-3334")
 				form.Add("message", stringWithCreditCardNb)
@@ -1276,12 +1284,23 @@ func TestScrubber(t *testing.T) {
 				require.True(t, scrubbed)
 
 				// Check values were scrubbed
-				require.Equal(t, []string{expectedMask, expectedMask}, req.PostForm["password"])
+				require.Equal(t, []string{expectedMask, expectedMask, expectedMask}, req.PostForm["password"])
+				require.Equal(t, []string{expectedMask}, req.PostForm["passwd"])
+				require.Equal(t, []string{expectedMask}, req.PostForm["api_key"])
+				require.Equal(t, []string{expectedMask}, req.PostForm["apikey"])
+				require.Equal(t, []string{expectedMask}, req.PostForm["authorization"])
+				require.Equal(t, []string{expectedMask}, req.PostForm["access_token"])
+				require.Equal(t, []string{expectedMask}, req.PostForm["secret"])
 				require.Equal(t, []string{fmt.Sprintf(messageFormat, expectedMask)}, req.Form["message"])
 
 				require.Contains(t, info, stringWithCreditCardNb)
-				require.Contains(t, info, "1234")
-				require.Contains(t, info, "5678")
+				require.Contains(t, info, "password10")
+				require.Contains(t, info, "password11")
+				require.Contains(t, info, "password2")
+				require.Contains(t, info, "password3")
+				require.Contains(t, info, "password4")
+				require.Contains(t, info, "password5")
+				require.Contains(t, info, "password6")
 			})
 		})
 	})