diff --git a/cookie.go b/cookie.go index 8344208..09fa162 100644 --- a/cookie.go +++ b/cookie.go @@ -163,6 +163,7 @@ func (c *Cookie) SetPath(path string) { c.bufK = append(c.bufK[:0], path...) c.path = normalizePath(c.path, c.bufK) c.path = removeNewLines(c.path) + c.path = removeSemicolons(c.path) } // SetPathBytes sets cookie path. @@ -170,6 +171,7 @@ func (c *Cookie) SetPathBytes(path []byte) { c.bufK = append(c.bufK[:0], path...) c.path = normalizePath(c.path, c.bufK) c.path = removeNewLines(c.path) + c.path = removeSemicolons(c.path) } // Domain returns cookie domain. @@ -183,11 +185,13 @@ func (c *Cookie) Domain() []byte { // SetDomain sets cookie domain. func (c *Cookie) SetDomain(domain string) { c.domain = initHeaderValueString(c.domain, domain) + c.domain = removeSemicolons(c.domain) } // SetDomainBytes sets cookie domain. func (c *Cookie) SetDomainBytes(domain []byte) { c.domain = initHeaderValueBytes(c.domain, domain) + c.domain = removeSemicolons(c.domain) } // MaxAge returns the seconds until the cookie is meant to expire or 0 @@ -239,11 +243,13 @@ func (c *Cookie) Value() []byte { // SetValue sets cookie value. func (c *Cookie) SetValue(value string) { c.value = initHeaderValueString(c.value, value) + c.value = removeSemicolons(c.value) } // SetValueBytes sets cookie value. func (c *Cookie) SetValueBytes(value []byte) { c.value = initHeaderValueBytes(c.value, value) + c.value = removeSemicolons(c.value) } // Key returns cookie name. @@ -257,11 +263,13 @@ func (c *Cookie) Key() []byte { // SetKey sets cookie name. func (c *Cookie) SetKey(key string) { c.key = initHeaderValueString(c.key, key) + c.key = removeSemicolons(c.key) } // SetKeyBytes sets cookie name. func (c *Cookie) SetKeyBytes(key []byte) { c.key = initHeaderValueBytes(c.key, key) + c.key = removeSemicolons(c.key) } // Reset clears the cookie. @@ -467,6 +475,21 @@ func (c *Cookie) ParseBytes(src []byte) error { return nil } +// removeSemicolons replaces every ';' in raw with a space. +// +// ';' separates attributes inside a Set-Cookie header, so a ';' carried in a +// key, value, domain or path taken from untrusted input would let an attacker +// append arbitrary attributes (Domain, Path, Secure, ...) to the cookie. CR and +// LF are already neutralised by removeNewLines. +func removeSemicolons(raw []byte) []byte { + for i := range raw { + if raw[i] == ';' { + raw[i] = ' ' + } + } + return raw +} + func appendCookiePart(dst, key, value []byte) []byte { dst = append(dst, ';', ' ') dst = append(dst, key...) diff --git a/cookie_test.go b/cookie_test.go index e49f495..fb42cca 100644 --- a/cookie_test.go +++ b/cookie_test.go @@ -78,6 +78,81 @@ func TestCookieSettersSanitizeNewLines(t *testing.T) { } } +func TestCookieSettersSanitizeSemicolons(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + set func(c *Cookie) + get func(c *Cookie) []byte + }{ + { + name: "SetKey", + set: func(c *Cookie) { c.SetKey("sid; Secure") }, + get: (*Cookie).Key, + }, + { + name: "SetKeyBytes", + set: func(c *Cookie) { c.SetKeyBytes([]byte("sid; Secure")) }, + get: (*Cookie).Key, + }, + { + name: "SetValue", + set: func(c *Cookie) { c.SetValue("abc; Secure") }, + get: (*Cookie).Value, + }, + { + name: "SetValueBytes", + set: func(c *Cookie) { c.SetValueBytes([]byte("abc; Secure")) }, + get: (*Cookie).Value, + }, + { + name: "SetDomain", + set: func(c *Cookie) { c.SetDomain("example.com; Secure") }, + get: (*Cookie).Domain, + }, + { + name: "SetDomainBytes", + set: func(c *Cookie) { c.SetDomainBytes([]byte("example.com; Secure")) }, + get: (*Cookie).Domain, + }, + { + name: "SetPath", + set: func(c *Cookie) { c.SetPath("/account; Secure") }, + get: (*Cookie).Path, + }, + { + name: "SetPathBytes", + set: func(c *Cookie) { c.SetPathBytes([]byte("/account; Secure")) }, + get: (*Cookie).Path, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var c Cookie + c.SetKey("sid") + c.SetValue("abc") + c.SetDomain("example.com") + c.SetPath("/") + + tc.set(&c) + + if strings.Contains(string(tc.get(&c)), ";") { + t.Fatalf("%s left a ';' that can inject attributes: %q", tc.name, c.Cookie()) + } + + var got Cookie + if err := got.ParseBytes(c.Cookie()); err != nil { + t.Fatalf("cannot parse produced cookie %q: %v", c.Cookie(), err) + } + if got.Secure() { + t.Fatalf("%s injected a Secure attribute: %q", tc.name, c.Cookie()) + } + }) + } +} + func TestCookieParseSanitizesNewLines(t *testing.T) { t.Parallel()