fix: Disables implicit Cookies url decoding (#928)

* Cookie not urldecoded * adds test * chore: adds comment * makes const private * QueryUnescape minor refactor * private doParseQuery avoiding public booleans * removes also comment, makes QueryUnescape private * go mod tidy * adds a couple of QueryUnescape test cases * godoc format * untrack go.work.sum, drops constants * fix comment
corazawaf · Dec 11, 2023 · 968dc71 · 968dc71
1 parent c0b5f7a
commit 968dc71
Show file tree

Hide file tree

Showing 11 changed files with 109 additions and 185 deletions.
diff --git a/.gitignore b/.gitignore
@@ -18,3 +18,5 @@ coraza-waf
 __debug_bin
 
 build/
+
+go.work.sum
diff --git a/go.sum b/go.sum
@@ -32,8 +32,6 @@ golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLL
 golang.org/x/net v0.0.0-20190923162816-aa69164e4478/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
 golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
 golang.org/x/net v0.0.0-20210726213435-c6fcb2dbf985/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
-golang.org/x/net v0.18.0 h1:mIYleuAkSbHh0tCv7RvjL3F6ZVbLjq4+R7zbOn3Kokg=
-golang.org/x/net v0.18.0/go.mod h1:/czyP5RqHAH4odGYxBJ1qz0+CE5WZ+2j1YgoEo8F2jQ=
 golang.org/x/net v0.19.0 h1:zTwKpTd2XuCqf8huc7Fo2iSy+4RHPd10s4KzeTnVr1c=
 golang.org/x/net v0.19.0/go.mod h1:CfAk/cbD4CthTvqiEl8NpboMuiuOYsAr/7NOjZJtv1U=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
@@ -49,8 +47,6 @@ golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q=
-golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/sys v0.15.0 h1:h48lPFYpsTvQJZF4EKyI4aLHaev3CxivZmv7yZig9pc=
 golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=

diff --git a/go.work.sum b/go.work.sum
diff --git a/http/middleware_test.go b/http/middleware_test.go
@@ -101,8 +101,7 @@ SecRule &REQUEST_HEADERS:Transfer-Encoding "!@eq 0" "id:1,phase:1,deny"
 	}
 	if it == nil {
 		t.Fatal("Expected interruption")
-	}
-	if it.RuleID != 1 {
+	} else if it.RuleID != 1 {
 		t.Fatalf("Expected rule 1 to be triggered, got rule %d", it.RuleID)
 	}
 	if err := tx.Close(); err != nil {

diff --git a/internal/bodyprocessors/urlencoded.go b/internal/bodyprocessors/urlencoded.go
@@ -10,7 +10,7 @@ import (
 
 	"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
 	"github.com/corazawaf/coraza/v3/internal/collections"
-	"github.com/corazawaf/coraza/v3/internal/url"
+	urlutil "github.com/corazawaf/coraza/v3/internal/url"
 )
 
 type urlencodedBodyProcessor struct {
@@ -23,7 +23,7 @@ func (*urlencodedBodyProcessor) ProcessRequest(reader io.Reader, v plugintypes.T
 	}
 
 	b := buf.String()
-	values := url.ParseQuery(b, '&')
+	values := urlutil.ParseQuery(b, '&')
 	argsCol := v.ArgsPost()
 	for k, vs := range values {
 		argsCol.Set(k, vs)

diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
@@ -321,7 +321,8 @@ func (tx *Transaction) AddRequestHeader(key string, value string) {
 		}
 	case "cookie":
 		// Cookies use the same syntax as GET params but with semicolon (;) separator
-		values := urlutil.ParseQuery(value, ';')
+		// WithoutUnescape is used to avoid implicitly performing an URL decode on the cookies
+		values := urlutil.ParseQueryWithoutUnescape(value, ';')
 		for k, vr := range values {
 			for _, v := range vr {
 				tx.variables.requestCookies.Add(k, v)
@@ -535,7 +536,7 @@ func (tx *Transaction) GetStopWatch() string {
 }
 
 // GetField Retrieve data from collections applying exceptions
-// In future releases we may remove de exceptions slice and
+// In future releases we may remove the exceptions slice and
 // make it easier to use
 func (tx *Transaction) GetField(rv ruleVariableParams) []types.MatchData {
 	col := tx.Collection(rv.Variable)

diff --git a/internal/corazawaf/transaction_test.go b/internal/corazawaf/transaction_test.go
@@ -806,6 +806,35 @@ func TestHeaderSetters(t *testing.T) {
 	}
 }
 
+func TestCookiesNotUrldecoded(t *testing.T) {
+	waf := NewWAF()
+	tx := waf.NewTransaction()
+	fullCookie := "abc=%7Bd+e+f%7D;hij=%7Bklm%7D"
+	expectedUrlencodedAbcCookieValue := "%7Bd+e+f%7D"
+	unexpectedUrldencodedAbcCookieValue := "{d e f}"
+	tx.AddRequestHeader("cookie", fullCookie)
+	c := tx.variables.requestCookies.Get("abc")[0]
+	if c != expectedUrlencodedAbcCookieValue {
+		if c == unexpectedUrldencodedAbcCookieValue {
+			t.Errorf("failed to set cookie, unexpected urldecoding. Got: %q, expected: %q", unexpectedUrldencodedAbcCookieValue, expectedUrlencodedAbcCookieValue)
+		} else {
+			t.Errorf("failed to set cookie, got %q", c)
+		}
+	}
+	if tx.variables.requestHeaders.Get("cookie")[0] != fullCookie {
+		t.Errorf("failed to set request header, got: %q, expected: %q", tx.variables.requestHeaders.Get("cookie")[0], fullCookie)
+	}
+	if !utils.InSlice("cookie", collectionValues(t, tx.variables.requestHeadersNames)) {
+		t.Error("failed to set header name", collectionValues(t, tx.variables.requestHeadersNames))
+	}
+	if !utils.InSlice("abc", collectionValues(t, tx.variables.requestCookiesNames)) {
+		t.Error("failed to set cookie name")
+	}
+	if err := tx.Close(); err != nil {
+		t.Error(err)
+	}
+}
+
 func collectionValues(t *testing.T, col collection.Collection) []string {
 	t.Helper()
 	var values []string
Original file line number	Diff line number	Diff line change
Expand Up		@@ -18,3 +18,5 @@ coraza-waf
		__debug_bin

		build/

		go.work.sum