net/http/cookiejar: fix out-of-bounds errors on malformed domains

The old implementation of Jar made the assumption that the host names
in the URLs given to SetCookies() and Cookies() methods are well-formed.
This is not an unreasonable assumption as malformed host names do not
trigger calls to SetCookies or Cookies (at least not from net/http)
as the HTTP request themselves are not executed. But there can be other
invocations of these methods and at least on Linux it was possible to
make DNS lookup to domain names with two trailing dots (see issue #7122).

This is an old bug and this CL revives an old change (see
https://codereview.appspot.com/52100043) to fix the issue. The discussion
around 52100043 focused on the interplay between the jar and the
public suffix list and who is responsible for which type if domain name
canonicalization. The new bug report in issue #19384 used a nil public
suffix list which demonstrates that the package cookiejar alone exhibits
this problem and any solution cannot be fully delegated to the
implementation of the used PublicSuffixList: Package cookiejar itself
needs to protect against host names of the form ".." which triggered an
out-of-bounds error.

This CL does not address the issue of host name canonicalization and
the question who is responsible for it. This CL just prevents the
out-of-bounds error: It is a very conservative change, i.e. one might
still set and retrieve cookies for host names like "weird.stuf...".
Several more test cases document how the current code works.

Fixes #19384.

Change-Id: I14be080e8a2a0b266ced779f2aeb18841b730610
Reviewed-on: https://go-review.googlesource.com/37843
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/net/http/cookiejar/jar.go b/src/net/http/cookiejar/jar.go
index f89abbcd186077e5963747021acba08f2f6e845e..37694a24f14166cc839621fbdb353027e98080bb 100644 (file)
--- a/src/net/http/cookiejar/jar.go
+++ b/src/net/http/cookiejar/jar.go
@@ -331,7 +331,7 @@ func jarKey(host string, psl PublicSuffixList) string {
        var i int
        if psl == nil {
                i = strings.LastIndex(host, ".")
-               if i == -1 {
+               if i <= 0 {
                        return host
                }
        } else {

diff --git a/src/net/http/cookiejar/jar_test.go b/src/net/http/cookiejar/jar_test.go
index 3aa601586e3f2f1b8943f41ccd54291682a6e08d..f7682e698abf400b85c6179e51f484555b71beca 100644 (file)
--- a/src/net/http/cookiejar/jar_test.go
+++ b/src/net/http/cookiejar/jar_test.go
@@ -125,6 +125,17 @@ var canonicalHostTests = map[string]string{
        "[2001:4860:0:::68]:8080": "2001:4860:0:::68",
        "www.bücher.de":           "www.xn--bcher-kva.de",
        "www.example.com.":        "www.example.com",
+       // TODO: Fix canonicalHost so that all of the following malformed
+       // domain names trigger an error. (This list is not exhaustive, e.g.
+       // malformed internationalized domain names are missing.)
+       ".":                       "",
+       "..":                      ".",
+       "...":                     "..",
+       ".net":                    ".net",
+       ".net.":                   ".net",
+       "a..":                     "a.",
+       "b.a..":                   "b.a.",
+       "weird.stuff...":          "weird.stuff..",
        "[bad.unmatched.bracket:": "error",
 }
 
@@ -133,7 +144,7 @@ func TestCanonicalHost(t *testing.T) {
                got, err := canonicalHost(h)
                if want == "error" {
                        if err == nil {
-                               t.Errorf("%q: got nil error, want non-nil", h)
+                               t.Errorf("%q: got %q and nil error, want non-nil", h, got)
                        }
                        continue
                }
@@ -176,6 +187,15 @@ var jarKeyTests = map[string]string{
        "co.uk":               "co.uk",
        "uk":                  "uk",
        "192.168.0.5":         "192.168.0.5",
+       // The following are actual outputs of canonicalHost for
+       // malformed inputs to canonicalHost (see above).
+       "":              "",
+       ".":             ".",
+       "..":            ".",
+       ".net":          ".net",
+       "a.":            "a.",
+       "b.a.":          "a.",
+       "weird.stuff..": ".",
 }
 
 func TestJarKey(t *testing.T) {
@@ -197,6 +217,15 @@ var jarKeyNilPSLTests = map[string]string{
        "co.uk":               "co.uk",
        "uk":                  "uk",
        "192.168.0.5":         "192.168.0.5",
+       // The following are actual outputs of canonicalHost for
+       // malformed inputs to canonicalHost.
+       "":              "",
+       ".":             ".",
+       "..":            "..",
+       ".net":          ".net",
+       "a.":            "a.",
+       "b.a.":          "a.",
+       "weird.stuff..": "stuff..",
 }
 
 func TestJarKeyNilPSL(t *testing.T) {
@@ -1265,3 +1294,18 @@ func TestDomainHandling(t *testing.T) {
                test.run(t, jar)
        }
 }
+
+func TestIssue19384(t *testing.T) {
+       cookies := []*http.Cookie{{Name: "name", Value: "value"}}
+       for _, host := range []string{"", ".", "..", "..."} {
+               jar, _ := New(nil)
+               u := &url.URL{Scheme: "http", Host: host, Path: "/"}
+               if got := jar.Cookies(u); len(got) != 0 {
+                       t.Errorf("host %q, got %v", host, got)
+               }
+               jar.SetCookies(u, cookies)
+               if got := jar.Cookies(u); len(got) != 1 || got[0].Value != "value" {
+                       t.Errorf("host %q, got %v", host, got)
+               }
+       }
+}