From: Volker Dobler Date: Mon, 6 Mar 2017 09:07:25 +0000 (+0100) Subject: net/http/cookiejar: fix out-of-bounds errors on malformed domains X-Git-Tag: go1.9beta1~1065 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=d972dc2de9d6b2b85656654b4d3a01dd02e446ec;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Russ Cox --- diff --git a/src/net/http/cookiejar/jar.go b/src/net/http/cookiejar/jar.go index f89abbcd18..37694a24f1 100644 --- 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 3aa601586e..f7682e698a 100644 --- 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) + } + } +}