v0.16.1

Fixes: - (#158) PII: make the PII scrubbing of In-App WAF attack events case-insensitive in order to correctly scrub transformed request parameters. - (#159) Monitoring: fix the content type and length monitoring of HTTP responses. - (#157) Gin middleware: use the request Go context instead of Gin's so that the agent can properly manage the request execution context, but also to correctly propagate values stored in the Go context before the middleware function.
sqreen · Oct 1, 2020 · 2678e0f · 2678e0f
2 parents d43b63d + 378102d
commit 2678e0f
Show file tree

Hide file tree

Showing 14 changed files with 458 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,18 @@
+# v0.16.1 - 30 September 2020
+
+## Fixes
+
+- (#158) PII: make the PII scrubbing of In-App WAF attack events
+  case-insensitive in order to correctly scrub transformed request parameters.
+
+- (#159) Monitoring: fix the content type and length monitoring of HTTP
+  responses.
+
+- (#157) Gin middleware: use the request Go context instead of Gin's so that the
+  agent can properly manage the request execution context, but also to correctly
+  propagate values stored in the Go context before the middleware function.
+
+
 # v0.16.0 - 22 September 2020
 
 ## New Feature

diff --git a/README.md b/README.md
@@ -1,5 +1,5 @@
 <p align="center">
-<img width="40%" src="doc/images/sqreen-gopher.png" alt="Sqreen for Go" title="Sqreen for Go" />
+<img width="30%" src="doc/images/sqreen-gopher.png" alt="Sqreen for Go" title="Sqreen for Go" />
 </p>
 
 # [Sqreen](https://www.sqreen.com/)'s Application Security Management for Go
@@ -9,7 +9,9 @@ security component into your app. Sqreen’s microagent automatically monitors
 sensitive app’s routines, blocks attacks and reports actionable infos to your
 dashboard.
 
-![Dashboard](https://sqreen-assets.s3-eu-west-1.amazonaws.com/miscellaneous/dashboard.gif)
+<p align="center">
+<img width="80%" src="https://sqreen-assets.s3-eu-west-1.amazonaws.com/miscellaneous/dashboard.gif" alt="Sqreen for Go" title="Sqreen for Go" />
+</p>
 
 Sqreen provides automatic defense against attacks:
 
@@ -90,4 +92,4 @@ Congratulations, your Go web application is now protected by Sqreen!
 
 Optionally, use the SDK to perform [user monitoring](https://docs.sqreen.com/go/user-monitoring/)
 or [custom security events](https://docs.sqreen.com/go/custom-events/) you would
-like to track and possibly block.
+like to track and possibly block.
diff --git a/internal/backend/api/api.go b/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
 				}

diff --git a/internal/backend/api/api_test.go b/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))
+		}
+	})
+}
diff --git a/internal/sqlib/sqsanitize/sanitize.go b/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)

diff --git a/internal/sqlib/sqsanitize/sanitize_test.go b/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")
 			})
 		})
 	})

diff --git a/internal/version/version.go b/internal/version/version.go
@@ -4,6 +4,6 @@
 
 package version
 
-const version = "0.16.0"
+const version = "0.16.1"
 
 func Version() string { return version }
diff --git a/sdk/middleware/sqecho/echo.go b/sdk/middleware/sqecho/echo.go
@@ -255,17 +255,22 @@ type observedResponse struct {
 }
 
 func newObservedResponse(r *responseWriterImpl) *observedResponse {
+	response := r.c.Response()
+
+	headers := response.Header()
+
 	// Content-Type will be not empty only when explicitly set.
 	// It could be guessed as net/http does. Not implemented for now.
-	ct := r.Header().Get("Content-Type")
-
-	response := r.c.Response()
+	ct := headers.Get("Content-Type")
 
-	// Content-Length is either explicitly set or the amount of written data.
+	// Content-Length is either explicitly set or the amount of written data. It's
+	// 0 by default with Echo.
 	cl := response.Size
-	if contentLength := r.Header().Get("Content-Length"); contentLength != "" {
-		if l, err := strconv.ParseInt(contentLength, 10, 0); err == nil {
-			cl = l
+	if cl == 0 {
+		if contentLength := headers.Get("Content-Length"); contentLength != "" {
+			if l, err := strconv.ParseInt(contentLength, 10, 0); err == nil {
+				cl = l
+			}
 		}
 	}