code review 47560044: net/http/cookiejar: document format of domain in Public...

net/http/cookiejar: document format of domain in PublicSuffix

Document what values a PublicSuffixList must accept as
a domain in a call to PublicSuffix.

[volker.dobler, 2014-01-06 11:06:33]

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/

[bradfitz, 2014-01-06 17:59:59]

LGTM



On Mon, Jan 6, 2014 at 3:06 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: document format of domain in PublicSuffix
>
> Document what values a PublicSuffixList must accept as
> a domain in a call to PublicSuffix.
>
> Please review this at https://codereview.appspot.com/47560044/
>
> Affected files (+3, -3 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
> @@ -34,9 +34,9 @@
>  type PublicSuffixList interface {
>         // PublicSuffix returns the public suffix of domain.
>         //
> -       // TODO: specify which of the caller and callee is responsible for
> IP
> -       // addresses, for leading and trailing dots, for case sensitivity,
> and
> -       // for IDN/Punycode.
> +       // Domain is a lowercase punycoded domain name (not an IP address)
> +       // without leading or trailing dots. The returned value is in the
> +       // same form.
>         PublicSuffix(domain string) string
>
>         // String returns a description of the source of this public suffix
>
>
>

[bradfitz, 2014-01-06 18:00:47]

*** Submitted as https://code.google.com/p/go/source/detail?r=40a37153a550 ***

net/http/cookiejar: document format of domain in PublicSuffix

Document what values a PublicSuffixList must accept as
a domain in a call to PublicSuffix.

R=bradfitz, nigeltao
CC=golang-codereviews
https://codereview.appspot.com/47560044

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[nigeltao, 2014-01-06 22:54:17]

OK, but this means that e.g., in the code.google.com/p/go.net/publicsuffix
implementation, it's perfectly valid thatPublicSuffix("example.com.au") is
"com.au" but PublicSuffix("EXAMPLE.COM.AU") is "AU".

What happens if, somewhere out there in the wild internet, a anchor's href
contains a domain in ALL.CAPS? A naive HTTP client implementation might end up
associating any returned cookies with the wrong public suffix, unless somewhere
along the line the domains are canonicalized to lower case. Is this a cookie
security problem, and a particularly insidious one since it 'works' 99.999% of
the time?

This particular CL says that the PublicSuffixList is not responsible for
canonicalization (lower-casing, punycode), but the bigger question of who *is*
responsible is left unanswered. Having different parts of a program each do
their own ad hoc canonicalization can be precarious, e.g. if some separate part
of the program then lower-cases the "AU" to "au". It's not exactly related to
cookie jars, but I am reminded of
http://labs.spotify.com/2013/06/18/creative-usernames/

Personally, I would not have removed the TODO without first building consensus
(presumably as a new discussion on golang-dev) about how to approach domain
canonicalization and cookies so that it's always right, not just almost always
right.

[bradfitz, 2014-01-06 22:55:14]

I will open a new bug with this concern and revert the CL with a reference
to said bug.



On Mon, Jan 6, 2014 at 2:54 PM, <nigeltao@golang.org> wrote:

> OK, but this means that e.g., in the
> code.google.com/p/go.net/publicsuffix implementation, it's perfectly
> valid thatPublicSuffix("example.com.au") is "com.au" but
> PublicSuffix("EXAMPLE.COM.AU") is "AU".
>
> What happens if, somewhere out there in the wild internet, a anchor's
> href contains a domain in ALL.CAPS? A naive HTTP client implementation
> might end up associating any returned cookies with the wrong public
> suffix, unless somewhere along the line the domains are canonicalized to
> lower case. Is this a cookie security problem, and a particularly
> insidious one since it 'works' 99.999% of the time?
>
> This particular CL says that the PublicSuffixList is not responsible for
> canonicalization (lower-casing, punycode), but the bigger question of
> who *is* responsible is left unanswered. Having different parts of a
> program each do their own ad hoc canonicalization can be precarious,
> e.g. if some separate part of the program then lower-cases the "AU" to
> "au". It's not exactly related to cookie jars, but I am reminded of
> http://labs.spotify.com/2013/06/18/creative-usernames/
>
> Personally, I would not have removed the TODO without first building
> consensus (presumably as a new discussion on golang-dev) about how to
> approach domain canonicalization and cookies so that it's always right,
> not just almost always right.
>
> https://codereview.appspot.com/47560044/
>

[nigeltao, 2014-01-06 23:22:32]

On Tue, Jan 7, 2014 at 9:54 AM,  <nigeltao@golang.org> wrote:
> canonicalization (lower-casing, punycode)

Mostly a note to self, before I forget: doing canonicalization
properly can be subtle. Lower-casing and Punycode-decoding are not
commutative. Punycode-decoding is not idempotent.