Issue 52100043: code review 52100043: net/http/cookiejar: reject malformed domains (Closed)
|
Created:
10 years, 10 months ago by volker.dobler Modified:
10 years, 10 months ago CC:
golang-codereviews Visibility:
Public. |
Descriptionnet/http/cookiejar: reject malformed domains
This CL rejects domain names with two trailing dots.
Such domain names are malformed but currently not
properly rejected on Linux. This is a stopgap
until the underlying issue 7122 is solved.
Patch Set 1 #Patch Set 2 : diff -r 93fde524b6ac https://code.google.com/p/go/ #Patch Set 3 : diff -r 93fde524b6ac https://code.google.com/p/go/ #
Total comments: 3
MessagesTotal messages: 8
Hello bradfitz@golang.org, nigeltao@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
I have no opinion on this. Seems fine if that's what the cookie spec(s) say. Nigel? On Tue, Jan 14, 2014 at 6:19 AM, <dr.volker.dobler@gmail.com> wrote: > Reviewers: bradfitz, nigeltao, > > Message: > Hello bradfitz@golang.org, nigeltao@golang.org (cc: > golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go/ > > > Description: > net/http/cookiejar: reject malformed domains > > This CL rejects domain names with two trailing dots. > Such domain names are malformed but currently not > properly rejected on Linux. This is a stopgap > until the underlying issue 7122 is solved. > > Please review this at https://codereview.appspot.com/52100043/ > > Affected files (+10, -0 lines): > M src/pkg/net/http/cookiejar/jar.go > > > Index: src/pkg/net/http/cookiejar/jar.go > =================================================================== > --- a/src/pkg/net/http/cookiejar/jar.go > +++ b/src/pkg/net/http/cookiejar/jar.go > @@ -312,6 +312,16 @@ > if strings.HasSuffix(host, ".") { > // Strip trailing dot from fully qualified domain names. > host = host[:len(host)-1] > + // TODO: Remove this test once issue > + // https://code.google.com/p/go/issues/detail?id=7122 is > resolved. > + if strings.HasSuffix(host, ".") { > + // Domain name resolution on Linux has a bug which > + // allows to resolve domain names with two trailing > + // dots like "www.google.com..". We do reject > + // cookies received from HTTP request to such > + // malformed domains. > + return "", errors.New("Rejected domain name with > two trailing dots.") > + } > } > return toASCII(host) > } > > >
Sign in to reply to this message.
RFC 6265 is a bit unclear about this as it assumes that stuff like "www.google.com.." is never a valid domain from which cookies could be received or sent to. The algorithm in RFC 6265 mandates the removal the one trailing dot in a FQDN but is silent on malformed domains. V. On Tue, Jan 14, 2014 at 9:57 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > I have no opinion on this. Seems fine if that's what the cookie spec(s) > say. Nigel? > > > > On Tue, Jan 14, 2014 at 6:19 AM, <dr.volker.dobler@gmail.com> wrote: > >> Reviewers: bradfitz, nigeltao, >> >> Message: >> Hello bradfitz@golang.org, nigeltao@golang.org (cc: >> golang-codereviews@googlegroups.com), >> >> I'd like you to review this change to >> https://code.google.com/p/go/ >> >> >> Description: >> net/http/cookiejar: reject malformed domains >> >> This CL rejects domain names with two trailing dots. >> Such domain names are malformed but currently not >> properly rejected on Linux. This is a stopgap >> until the underlying issue 7122 is solved. >> >> Please review this at https://codereview.appspot.com/52100043/ >> >> Affected files (+10, -0 lines): >> M src/pkg/net/http/cookiejar/jar.go >> >> >> Index: src/pkg/net/http/cookiejar/jar.go >> =================================================================== >> --- a/src/pkg/net/http/cookiejar/jar.go >> +++ b/src/pkg/net/http/cookiejar/jar.go >> @@ -312,6 +312,16 @@ >> if strings.HasSuffix(host, ".") { >> // Strip trailing dot from fully qualified domain names. >> host = host[:len(host)-1] >> + // TODO: Remove this test once issue >> + // https://code.google.com/p/go/issues/detail?id=7122 is >> resolved. >> + if strings.HasSuffix(host, ".") { >> + // Domain name resolution on Linux has a bug which >> + // allows to resolve domain names with two >> trailing >> + // dots like "www.google.com..". We do reject >> + // cookies received from HTTP request to such >> + // malformed domains. >> + return "", errors.New("Rejected domain name with >> two trailing dots.") >> + } >> } >> return toASCII(host) >> } >> >> >> > -- Dr. Volker Dobler
Sign in to reply to this message.
canonicalHostTests needs updating. https://codereview.appspot.com/52100043/diff/40001/src/pkg/net/http/cookiejar... File src/pkg/net/http/cookiejar/jar.go (right): https://codereview.appspot.com/52100043/diff/40001/src/pkg/net/http/cookiejar... src/pkg/net/http/cookiejar/jar.go:315: // TODO: Remove this test once issue I don't see why this test should ever be removed. Regardless of what the OS does, the PublicSuffixList interface's doc comments says "Domain is... without leading or trailing dots", but without this test, this code would (incorrectly) call PublicSuffix with a trailing dot. https://codereview.appspot.com/52100043/diff/40001/src/pkg/net/http/cookiejar... src/pkg/net/http/cookiejar/jar.go:323: return "", errors.New("Rejected domain name with two trailing dots.") errors.New("cookiejar: malformed domain name has multiple trailing dots") https://codereview.appspot.com/52100043/diff/40001/src/pkg/net/http/cookiejar... src/pkg/net/http/cookiejar/jar.go:326: return toASCII(host) The PublicSuffixList doc comments also say no leading dots, but you don't seem to enforce that (yet).
Sign in to reply to this message.
Maybe I am understanding Nigel now. Please excuse the
slow progress.
I assumed that Jar might rely on valid url.URLs being
passed to its SetCookies and Cookies methods. "Valid" in
the sense of URL.Host contains a valid domain name or an
IP address, both with an optional port. I assumed that
a malformed Host in the passed URL cannot happen for
SetCookies as this method is called after a successful
request to this URL and doesn't matter for Cookies as
the following request to the malformed domain will fail.
Nigel, if I understand your objection correct than this
assumption is plain false: Once the acceptable format
for PublicSuffix is defined, Jar (as a caller) must ensure
this format in each and every case, i.e. also in code
like this
u := &url.URL{
Scheme: "http",
Host: "--definitively..invalid!.hostname...",
Path: "/"
}
jar.SetCookies(u, someHttpCookies)
and Jar must not rely on http.Get failing on malformed domain
names. (I was under the impression that calling SetCookies
with such a Host in u was a programming error, almost like
calling SetCookies with a nil u.)
Is this case this CL is complete nonsense as Jar is totally
broken! Jar implements domain name canonicalization and
cookie domain handling as required by RFC 6265 which
assumes that the request-host is _not_ malformed, Jar makes
the same assumption and assumes URL.Host is not malformed.
If this assumption is false, than Jar must do its own, full-fletched
request-host validation (would be a different, much larger CL)
and not just a workaround until HTTP requests to malformed
host fail reliable; regardless which format PublicSuffix
expects or how Linux treats slightly malformed domain names.
V.
On Wed, Jan 15, 2014 at 10:32 PM, <nigeltao@golang.org> wrote:
> canonicalHostTests needs updating.
>
>
> https://codereview.appspot.com/52100043/diff/40001/src/
> pkg/net/http/cookiejar/jar.go
> File src/pkg/net/http/cookiejar/jar.go (right):
>
> https://codereview.appspot.com/52100043/diff/40001/src/
> pkg/net/http/cookiejar/jar.go#newcode315
> src/pkg/net/http/cookiejar/jar.go:315: // TODO: Remove this test once
> issue
> I don't see why this test should ever be removed. Regardless of what the
> OS does, the PublicSuffixList interface's doc comments says "Domain
> is... without leading or trailing dots", but without this test, this
> code would (incorrectly) call PublicSuffix with a trailing dot.
>
> https://codereview.appspot.com/52100043/diff/40001/src/
> pkg/net/http/cookiejar/jar.go#newcode323
> src/pkg/net/http/cookiejar/jar.go:323: return "", errors.New("Rejected
>
> domain name with two trailing dots.")
> errors.New("cookiejar: malformed domain name has multiple trailing
> dots")
>
> https://codereview.appspot.com/52100043/diff/40001/src/
> pkg/net/http/cookiejar/jar.go#newcode326
> src/pkg/net/http/cookiejar/jar.go:326: return toASCII(host)
> The PublicSuffixList doc comments also say no leading dots, but you
> don't seem to enforce that (yet).
>
Simply because (false assumption above) I thought that
it will never be called with leading dots as ".google.com"
is not a valid domain name.
>
> https://codereview.appspot.com/52100043/
>
Sign in to reply to this message.
On Thu, Jan 16, 2014 at 10:01 PM, Volker Dobler <dr.volker.dobler@gmail.com> wrote: > Nigel, if I understand your objection correct than this > assumption is plain false: Once the acceptable format > for PublicSuffix is defined, Jar (as a caller) must ensure > this format in each and every case, i.e. also in code > like this > u := &url.URL{ > Scheme: "http", > Host: "--definitively..invalid!.hostname...", > Path: "/" > } > jar.SetCookies(u, someHttpCookies) > and Jar must not rely on http.Get failing on malformed domain > names. (I was under the impression that calling SetCookies > with such a Host in u was a programming error, almost like > calling SetCookies with a nil u.) Yes, it is a design question (and hence the TODO removed in https://codereview.appspot.com/47560044/) whether or not such a Host in u was a programming error. Importantly, a nil u would crash early, but a malformed u.Host would not, and could potentially be a silent and subtle security issue. In retrospect, maybe http.CookieJar and cookiejar.PublicSuffixList should return (T, error) instead of T. This is part of http://golang.org/issue/5465 For example, the program below, a sketch of a primitive cookie-aware web crawler, will call PublicSuffix("--definitively..invalid!.hostname..") for the given third party HTML. That HTML fragment is hard-coded in the program below but conceptually could be HTML content from anywhere on the internet. This program is buggy with what you call a programming error, in that it calls PublicSuffix with a non-canonical host name, but it doesn't crash. The bug is also not obvious from the source code, and it's not obvious where the fix should be: in this program, or in the standard libraries, or in the go.net sub-repo. On the flip side, you could make the publicsuffix implementation defensive (allowing non-canonical hostnames), and the cookiejar implementation, and the net/http implementation, and the net implementation, but having different layers of the stack do their own ad hoc canonicalization can also lead to problems. Furthermore, RFC 6265 and its hostname assumptions deal with cookie jars, but Go's net and net/http packages do more than just cookie jars. Ideally, we'd have a top-down design to hostname canonicalization. This is http://golang.org/issue/7125 -------- package main import ( "fmt" "io" "log" "net/http" "net/http/cookiejar" "os" "strings" "code.google.com/p/go.net/html" "code.google.com/p/go.net/publicsuffix" ) const thirdPartyHTML = `A link to <a href="http://--definitively..invalid!.hostname.../foo">bar</a>.` type psl struct{} func (psl) PublicSuffix(domain string) string { s := publicsuffix.List.PublicSuffix(domain) fmt.Printf("PublicSuffix(%q) = %q\n", domain, s) return s } func (psl) String() string { return "psl" } var client *http.Client func main() { jar, err := cookiejar.New(&cookiejar.Options{ PublicSuffixList: psl{}, }) if err != nil { log.Fatal(err) } client = &http.Client{ Jar: jar, } doc, err := html.Parse(strings.NewReader(thirdPartyHTML)) if err != nil { log.Fatal(err) } walk(doc) } func walk(n *html.Node) { if n.Type == html.ElementNode && n.Data == "a" { for _, a := range n.Attr { if a.Key == "href" { crawl(a.Val) } } } for c := n.FirstChild; c != nil; c = c.NextSibling { walk(c) } } func crawl(href string) { res, err := client.Get(href) if err != nil { log.Fatal(err) } defer res.Body.Close() io.Copy(os.Stdout, res.Body) } --------
Sign in to reply to this message.
I think we have a proper (and shared) understanding of the
problem space. But how could a solution look like for Go 1.x?
How could the suggested top-down approach to domain
canonicalization solve/simplify the interplay of net/http,
net/http/cookiejar and go.net/publicsuffix ?
For Go 2 some fancy type like
type Host struct { /* unexported stuff */ }
could be used instead of strings and provide canonicalization.
But for Go 1 where URLs, and all functions work on naked
strings? (Something I really do like.)
It boils down to:
- Document the expected behaviour and valid arguments.
May fail in non-obvious ways and be a security issue as
you pointed out.
- Be defensive everywhere.
Package net and net/http already are defensive in the sense
that malformed domains are rejected (during resolving in
the OS level). Leaves cookiejar and publicsuffix.
I definitely thinks that our implementation of http.PublicSuffix
should be more defensive (and more convenient).
Should this defensive handling of arguments be required
in the interface for all implementations?
Proper rejection of malformed domains in package
cookiejar is doable.
How do we reach a consensus here?
API proposals like
package net
// ValidHost checks if a network address of the form "host:port"
// or "host" has a valid host which is either a well-formed domain
// name or an IPv4 or an IPv6 address.
ValidHost(hostport string) bool
?
V.
On Mon, Jan 20, 2014 at 5:46 AM, Nigel Tao <nigeltao@golang.org> wrote:
> On Thu, Jan 16, 2014 at 10:01 PM, Volker Dobler
> <dr.volker.dobler@gmail.com> wrote:
> > Nigel, if I understand your objection correct than this
> > assumption is plain false: Once the acceptable format
> > for PublicSuffix is defined, Jar (as a caller) must ensure
> > this format in each and every case, i.e. also in code
> > like this
> > u := &url.URL{
> > Scheme: "http",
> > Host: "--definitively..invalid!.hostname...",
> > Path: "/"
> > }
> > jar.SetCookies(u, someHttpCookies)
> > and Jar must not rely on http.Get failing on malformed domain
> > names. (I was under the impression that calling SetCookies
> > with such a Host in u was a programming error, almost like
> > calling SetCookies with a nil u.)
>
> Yes, it is a design question (and hence the TODO removed in
> https://codereview.appspot.com/47560044/) whether or not such a Host
> in u was a programming error. Importantly, a nil u would crash early,
> but a malformed u.Host would not, and could potentially be a silent
> and subtle security issue. In retrospect, maybe http.CookieJar and
> cookiejar.PublicSuffixList should return (T, error) instead of T. This
> is part of http://golang.org/issue/5465
>
> For example, the program below, a sketch of a primitive cookie-aware
> web crawler, will call
> PublicSuffix("--definitively..invalid!.hostname..") for the given
> third party HTML. That HTML fragment is hard-coded in the program
> below but conceptually could be HTML content from anywhere on the
> internet. This program is buggy with what you call a programming
> error, in that it calls PublicSuffix with a non-canonical host name,
> but it doesn't crash. The bug is also not obvious from the source
> code, and it's not obvious where the fix should be: in this program,
> or in the standard libraries, or in the go.net sub-repo.
>
> On the flip side, you could make the publicsuffix implementation
> defensive (allowing non-canonical hostnames), and the cookiejar
> implementation, and the net/http implementation, and the net
> implementation, but having different layers of the stack do their own
> ad hoc canonicalization can also lead to problems. Furthermore, RFC
> 6265 and its hostname assumptions deal with cookie jars, but Go's net
> and net/http packages do more than just cookie jars. Ideally, we'd
> have a top-down design to hostname canonicalization. This is
> http://golang.org/issue/7125
>
> --------
> package main
>
> import (
> "fmt"
> "io"
> "log"
> "net/http"
> "net/http/cookiejar"
> "os"
> "strings"
>
> "code.google.com/p/go.net/html"
> "code.google.com/p/go.net/publicsuffix"
> )
>
> const thirdPartyHTML = `A link to <a
> href="http://--definitively..invalid!.hostname.../foo">bar</a>.`
>
> type psl struct{}
>
> func (psl) PublicSuffix(domain string) string {
> s := publicsuffix.List.PublicSuffix(domain)
> fmt.Printf("PublicSuffix(%q) = %q\n", domain, s)
> return s
> }
>
> func (psl) String() string { return "psl" }
>
> var client *http.Client
>
> func main() {
> jar, err := cookiejar.New(&cookiejar.Options{
> PublicSuffixList: psl{},
> })
> if err != nil {
> log.Fatal(err)
> }
> client = &http.Client{
> Jar: jar,
> }
> doc, err := html.Parse(strings.NewReader(thirdPartyHTML))
> if err != nil {
> log.Fatal(err)
> }
> walk(doc)
> }
>
> func walk(n *html.Node) {
> if n.Type == html.ElementNode && n.Data == "a" {
> for _, a := range n.Attr {
> if a.Key == "href" {
> crawl(a.Val)
> }
> }
> }
> for c := n.FirstChild; c != nil; c = c.NextSibling {
> walk(c)
> }
> }
>
> func crawl(href string) {
> res, err := client.Get(href)
> if err != nil {
> log.Fatal(err)
> }
> defer res.Body.Close()
> io.Copy(os.Stdout, res.Body)
> }
> --------
>
--
Dr. Volker Dobler
Sign in to reply to this message.
|
