search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.64k stars 17.61k forks source link

net/http/cookiejar: Domain matching against IP address #12610

Closed sebcat closed 2 years ago

sebcat commented 9 years ago

Hello,

The following test fails:

package foo 

import (
    "net/http"
    "net/http/cookiejar"
    "net/url"
    "testing"
)

func TestIPDomain(t *testing.T) {
    u, err := url.Parse("http://127.0.0.1/")
    if err != nil {
        t.Fatal(err)
    }   

    jar, err := cookiejar.New(nil)
    if err != nil {
        t.Fatal(err)
    }   

    c := &http.Cookie{Name: "foo", Value: "bar", Domain: "127.0.0.1"}
    jar.SetCookies(u, []*http.Cookie{c})
    cs := jar.Cookies(u)
    if len(cs) != 1 { 
        t.Fatalf("Got %v cookies, expected 1\n", len(cs))
    } else if cs[0].Name != "foo" || cs[0].Value != "bar" {
        t.Fatal("Invalid cookie name/value")
    }   
}

Further inspection shows that it fails with errNoHostname here: https://github.com/golang/go/blob/7f0be1f781a523b5acafb527ca123f5b0eac1536/src/net/http/cookiejar/jar.go#L447-L452

I believe the comment refers to RFC6265 5.1.3. It states that the domain string must either be identical with the string it's matched against, OR that all of the three subsequently listed conditions are met, one of which are that the string being matched must be a host name.

In the case of the matched string being an IP address and the domain string being the same IP address, the first condition is met. I therefor believe that L447-L452 should be removed.

vdobler commented 9 years ago

The situation is not really simple: RFC 6265 is not clear whether to allow or deny domain cookies on IP addresses and browsers handle this case differently. That was the reason for the TODO in line 450 [1] of the cookiejar implementation. I have the impression that half a decade ago domain cookies on IP addresses where commonly accepted, sometimes just to match the behavior of Internet Explorer (see e.g. [2] [3] [4] [10]).

The current implementation in Chromium rejects domain cookies in IP addresses [5] lines 457-477. If I my understanding of the Mozilla code [6] is correct then Firefox silently converts a domain cookie for an IP address to a host cookie. (I have not done tests on any browsers recently.)

My gut feeling is that cookies with an IP address as the domain-attribute should be either rejected (an IP address is not a domain) or be converted to host cookie (i.e. pretending the domain-attribute was not set, a forgivable user error).

RFC 6265 is not really clear here. I read the following

The string is a host name (i.e., not an IP address).

RFC 6265 section 5.1.3, second bullet, third star [7] as a definition of host name in the sense of: "a host name is not an IP address".

Therefore my understanding of the algorithm to produce a canonicalized host name

Convert the host name to a sequence of individual domain name labels.

RFC 6265 section 5.1.2 step 1 [8] is that an IP address does not qualify as a canonicalized host name. As an IP address is not a canonicalized host name I think that RFC 6265 section 5.3 step 6 first case [9] applies:

If the canonicalized request-host does not domain-match the domain-attribute: Ignore the cookie entirely and abort these steps.

and the cookie should be dropped as it is done currently.

[1] https://github.com/golang/go/blob/7f0be1f781a523b5acafb527ca123f5b0eac1536/src/net/http/cookiejar/jar.go#L447-L452 [2] https://code.google.com/p/chromium/issues/detail?id=3699 [3] https://codereview.chromium.org/18657/diff/1/net/base/cookie_monster_unittest.cc [4] https://bugzilla.mozilla.org/show_bug.cgi?id=125344 [5] http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_store_unittest.h [6] https://hg.mozilla.org/mozilla-central/file/3e8dde8f8c17/netwerk/cookie/nsCookieService.cpp#l3502

[7] http://tools.ietf.org/html/rfc6265#section-5.1.3 [8] http://tools.ietf.org/html/rfc6265#section-5.1.2 [9] http://tools.ietf.org/html/rfc6265#section-5.3 [10] https://bugzilla.mozilla.org/show_bug.cgi?id=125344

sebcat commented 9 years ago

The Chromium test case you linked as [5] shows that Chromium allows a cookie to be set with a domain attribute if that attribute is an exact match (L470-L472 in the same file).

RFC 6265 is not really clear here. I read the following

The string is a host name (i.e., not an IP address).

RFC 6265 section 5.1.3, second bullet, third star [7] as a definition of host name in the sense of: "a host name is not an IP address".

The third star is so that suffix matching is only applied on host names. 5.1.3 also states that if the two strings matches each other exactly, they domain-match.

Regarding RFC6265 5.3:

If the canonicalized request-host does not domain-match the domain-attribute: Ignore the cookie entirely and abort these steps.

If I visit the URL http://127.0.0.1/ in my browser, it is my understanding that "the canonicalized request-host" is "127.0.0.1". If the domain-attribute in the Set-Cookie header is set to 127.0.0.1, the canonicalized request-host domain-matches the domain-attribute under the rules of 5.1.3 (exact match).

vdobler commented 9 years ago

The comment in the Chromium test case is interesting: "This matches IE/Firefox, even though it seems a bit wrong." Maybe that behaviour is wrong.

One more from the Chromium test case: Chromium treats domain=.1.2.3.4 as an illegal domain attribute. But if you follow RFC 6265 (as package cookiejar tries to) you would chop off the leading trail first (see 5.3.2), even before domain matching it. Chromiums behaviour here is not RFC 6265 compliant no matter if RFC 6265 allows domain cookies on IP addresses or not.

As I said RFC 6265 is not clear here.

I think silently converting the domain cookie to a host cookie is okay, but I do not think that domain cookies on IP addresses make sense at all.

Would you have time to test the behaviour on Chrome, FF, IE, Safari and curl? I thinks showing that all these "browsers" handle domain attributes equal to the request IP address the same would add considerable weight to the proposed change.

sebcat commented 9 years ago

For the following web service:

package main

import (
    "net/http"
    "log"
)

func CookieHandler(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Set-Cookie", "foo=bar; domain=127.0.0.1")
    w.Write([]byte("foo"))
}

func main() {
    http.HandleFunc("/", CookieHandler)
    log.Fatal(http.ListenAndServe(":8080", nil))
}

curl 7.40.0 Google Chrome 47.0.2503.0 dev Mozilla Firefox 40.0.3

sets the cookie "foo" for 127.0.0.1

I will be able to test IE, Safari in the beginning of next week.

sebcat commented 9 years ago

Safari 9.0 (11601.1.56) IE 10.0.9200.17492 (Update version 10.0.31, Windows 2012 Server) *

sets the cookie "foo" for 127.0.0.1 for the previously mentioned test case.

*: Added Expires attribute to make sure a persistent cookie file was created in "Temporary Internet Files".

dcormier commented 5 years ago

This certainly makes it challenging to use httptest.Server and try to set cookies with the Domain attribute set to the httptest.Server's address.

jongiddy commented 5 years ago

Having spent some time working out why a Go test behaved differently to a browser test, I ended up at this discussion. Even after following the referenced links, I find the argument for the current behavior to be unconvincing.

RFC 6265 section 5.1.3 states that there is a match if "the domain string and the string are identical". The only way to make this exclude IP addresses is to infer a definition of the term "string" to mean "host name (i.e., not an IP address)". But if "string" is defined this way for the entire section, then why would the RFC need these exact words as a requirement of the alternative branch? It would be tautological.

Combined with the other evidence that browsers do support exact matches on IP addresses (the Chromium tests, Firefox code, and the last comment by @sebcat), my opinion is that this should be fixed in the Go library.

jayateertha043 commented 3 years ago

Please Golang allow ip address, domain setting cookie in next release .I had to comment out this whole part: https://github.com/golang/go/blob/7f0be1f781a523b5acafb527ca123f5b0eac1536/src/net/http/cookiejar/jar.go#L447-L452 to make my code run.

Most of the browsers allow ip address for cookie setting, kindly try to relax that behaviour or atleast mention that somewhere,It took me nearly 1 day to figure out what was going bad.

https://stackoverflow.com/questions/67872836/use-cookies-txt-as-cookies-jar-client-side-in-golang?noredirect=1#comment119971669_67872836

vdobler commented 3 years ago

@jayateertha043

You seem to imply that domain-cookies should unconditional be setable on IP addresses if the cookie's domain-attribute is an IP address and equal to the host which also must be an IP address?

Do you consider such cookies domain- or host-cookies?

You mention "Most of the browsers allow ip address for cookie setting". Could you provide evidence of the details, especially:

jongiddy commented 3 years ago

Do you consider such cookies domain- or host-cookies?

  • Do browsers treat such cookies a domain-cookies or host-cookies? Does this matter? Do all browsers behave the same?

It turns out that it doesn't matter - for IP addresses, host and domain cookies are the same.

RFC 6265 Sec 2.3 says:

The request-host is the name of the host, as known by the user agent, to which the user agent is sending an HTTP request or from which it is receiving an HTTP response (i.e., the name of the host to which it sent the corresponding HTTP request)."

This does not appear to exclude IP addresses, since a HTTP request can be sent to an IP address and the IP address would be the only name known by the user agent. This section also refers to request-uri from https://datatracker.ietf.org/doc/html/rfc2616#section-5.1.2 which, via https://datatracker.ietf.org/doc/html/rfc2396, ultimately defines its host component as hostname | IPv4address.

https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.2 defines a "canonicalized host name" and refers to domain name RFC's. So although it is not definitive, we could probably say that this refers to a domain name, and specifically not an IP address. This is backed up by section 5.1.3 which says "The string is a host name (i.e., not an IP address)"

So in RFC 6265 "request-host" includes IP addresses and "host name" does not.

When considering Host cookies (cookies without a Domain attribute):

The phrase "canonicalized request-host" isn't defined, but it is a "request-host" and not just a "host name". The most sensible way to interpret it is as a request-host that has been canonicalized if it is a host name.

So we get a cookie with host-only-flag=true and domain=canonicalized request-host (in this case an IP address).

When we come to send the cookie Sec 5.4(1) says, since the host-only-flag is true, we send the cookie when the canonicalized request-host is identical to the cookie's domain (when sending to the same IP address).

When considering a Domain cookie (cookie with a Domain attribute) returned from an IP address, running through the same sections, since the second part of sec 5.1.3 is inoperable if the request-host is an IP address, the cookie is ignored unless the cookie domain is identical to the request-host, both for the receiving and sending of the cookie. So a cookie from an IP address with the domain set to the IP address is equivalent to an IP address host cookie.

  • What if only one of the cookie-domain-attribute and the host is an IP address but the IP address ist that of the domain name on the other? What do browsers do in such cases? Do all browsers behave the same way?
  • On which requests do browsers return cookies set with an IP address as the domain-attribute? Only tho the IP address? Or also to the domain name resolving tho this IP?

Domain name resolution does not occur for cookie management. This is also true for domain names: https://stackoverflow.com/questions/10020987/cname-and-cookies

  • Are there any security considerations to take care of? If not: Why is the behaviour unproblematic?

The security considerations appear to be the same as those for domain names. e.g. there are potential problems if you use a request-host that reaches a different host on different networks. But the lack of true domain cookies for IP addresses eliminates one class of problems: accidentally giving access to an untrusted subdomain.

vdobler commented 3 years ago

@jongiddy Thanks for the detailed analysis. Please note that the problem are cookies with a domain-attribute only. Host-cookies and IP addresses work well, see e.g. the test in https://github.com/golang/go/blob/7f0be1f781a523b5acafb527ca123f5b0eac1536/src/net/http/cookiejar/jar_test.go#L259. RFC 6265 is pretty clear here and this was never in question, so let's focus the discussion on cookies with a domain-attribute which is a IP address only.

I think we agree the other two cases (host-cookies) and cookies with a domain name as domain-attribute) work correctly.

It turns out that it doesn't matter - for IP addresses, host and domain cookies are the same

Basically yes, but:

  1. Are we sure all browsers actually do treat them as "the same" (i.e. not only same by reading RFC 6265 which is not absolutely clear.) While adhering to RFC 6265 is important the main issue here is that browsers do something differently with this type of cookies than net/http/cookiejar.

  2. From when on are these "the same":

Variant A

If on a Set-Cookie header the domain-attribute is a IP address:
    If domain-attribute != request-host:
        ignore the cookie
    Else:
        treat the domain attribute as absent and process it to the current rules for host-cookies.

Variant B

If on a Set-Cookie header the domain-attribute is a IP address:
        treat the domain attribute as absent and
        process it to the current rules for host-cookies
        that is just set it unconditional in the request-host (be it a domain or an IP)

I think you seem to read RFC 6265 as Variant A which is fine, but again: Is this what browsers do?

It seems browser and net/http/cookiejar treat Set-Cookies with a domain-attribute which is an IP address differently. I think RFC 6265 is not crystal clear here and I agree that your interpretation is possible (and your arguments are a bit more convincing than mine in https://github.com/golang/go/issues/12610#issuecomment-140642614). But any change here should be okay with RFC 6265, and (more import) compliant with how browsers treat these cookies.

I do not think that Variant B would be what RFC 6265 intends, but it could be that browsers do behave that way. And in this case we probably should do B and not A.

I do not think that interpretations of RFC 6265's text are helpful in making any progress here. RFC 6258 is not clear here. This is unfortunate. Interpreting it both ways is possible. Progress here will be made once we understand how browsers handle these cookies. Does anybody have insights here? Do we have to make a test here?

neild commented 3 years ago

I don't see any ambiguity in RFC 6265.

5.3, step 6 clearly rejects a cookie only if the canonicalized request-host does not domain-match a non-empty domain-attribute. 5.1.3 clearly states that a string domain-matches a domain string if the two are identical.

The request-host is the name of the host to which the user agent sent the HTTP request (2.3). Canonicalization applies to DNS names, not IP addresses, so it is not explicitly stated that the request-host for a request to http://127.0.0.1/ is 127.0.0.1, however it is clear that this is the intent. (If this is not the case, then 5.6, step 6 makes no sense in the case where the domain-attribute is empty: "Set the cookie's domain to the canonicalized request-host.")

A request to http://127.0.0.1/ resulting in a cookie with a Domain attribute of 127.0.0.1 will therefore set the request-host to 127.0.0.1 (2.3), the domain-attribute to 127.0.0.1 (5.3, step 4). These strings are identical and therefore domain-match (5.1.3), so the cookie's host-only-flag is set to false and the domain is set to 127.0.0.1 (5.3, step 6).

Possibly contemporary browsers do something different, but the behavior as detailed by RFC 6265 clearly indicates that a Domain attribute containing an IP address will match on a request URL containing that exact IP address. (Not, of course, a request URL containing a DNS name that resolves to that address.)

vdobler commented 3 years ago

@neild And what conclusion do you draw?

Should we just change Jar and allow "IP-Domain-Cookies"?

Do it and call it a day?

Rating the ambiguity of RFC 6265 on a scale from "crystal clear" to "inconsistent" is a nice intellectual exercise but does it (alone) help in deciding what to do with net/http/cookiejar.Jar? I understand your last statement

Possibly contemporary browsers do something different, but the behavior as detailed by RFC 6265 clearly indicates that a Domain attribute containing an IP address will match on a request URL containing that exact IP address.

that you argue to change Jar, no matter what contemporary browsers do?

Please do not get me wrong: I'm basically convinced that Jar should be changed in regard to "IP-Domain-Cookies", but I'd like to see more evidence that it will do good to existing (and future) users of net/http/cookiejar.Jar than just "RFC 6265 is clear here". Jar is "broken" since several years and complaints about that "bug" are rare. I just want to see any reason why "fixing" Jar won't introduce other problems for existing users.

I'll retry sebcats experiments on 2021 browses. If their behaviour matches your reading of RFC 6265 I'll prepare a CL.

vdobler commented 3 years ago

Testcode

package main

import (
    "fmt"
    "log"
    "net/http"
    "os"
)

var ipaddr = "127.0.0.1"

func CookieHandler(w http.ResponseWriter, r *http.Request) {
    w.Header().Add("Set-Cookie", "hostcookie=123")
    w.Header().Add("Set-Cookie", "legal=456; domain="+ipaddr)
    w.Header().Add("Set-Cookie", "illegal=xxx; domain=1.2.3.4")
    w.WriteHeader(200)
    for _, c := range r.Cookies() {
        fmt.Fprintln(w, c.Name, " --> ", c.Value)
    }
}

func main() {
    if ip := os.Getenv("IP_ADDR"); ip != "" {
        ipaddr = ip
    }
    http.HandleFunc("/", CookieHandler)
    log.Fatal(http.ListenAndServe(":80", nil))
}

Systems checked

Mac

Windows

Outcome

All show

hostcookie  -->  123
legal  -->  456

on the second request.

Chrome developer tools and curl's jar file hint that the legal cookie is treated as / has been converted to a host-cookie (like hostcookie).

neild commented 3 years ago

And what conclusion do you draw?

In the absence of evidence that browsers behave differently, net/http/cookiejar should accept cookies with a IP-address Domain equal to the request-host.

By my read, RFC 6265 states that these cookies should have host-only-flag set to false, but it looks like your empirical tests indicate that browsers set it to true. Following the browser behavior seems okay, although I don't see how following the RFC behavior would result in in different behavior visible to clients.

The internal (*entry).domainMatch function claims to implement RFC 6265 domain-match, but it doesn't include the check to verify that the host is not an IP address. If we do accept cookies with HostOnly=true and an IP address in Domain, that function needs updating.

vdobler commented 3 years ago

@neild

The internal (*entry).domainMatch function claims to implement RFC 6265 domain-match, but it doesn't include the check to verify that the host is not an IP address. If we do accept cookies with HostOnly=true and an IP address in Domain, that function needs updating.

Why?

If we accept a cookie Set-Cookie: a=1; domain=127.0.0.1 from 127.0.0.1 and process this as if it was a host cookie Set-Cookie: a=1 then e.HostOnly==true, e.Domain=="127.0.0.1" and domainMatch is basically e.Domain == host which is correct.

Domain matching in RFC 6265 section 5.1.3 is the OR of two bullet points. The second bullet contains the condition "The string is a host name (i.e., not an IP address)" but the whole second bullet is there only for actual domain cookies which should be sent back to subdomains. RFC 6265 is overly complicated here (again) in treating domain-cookies and host-cookies the same.

Or do you have any example which would trigger some misbehaviour in (*entry).domainMatch if we do not include the check?

Note that I intend to reject a cookie Set-Cookie: a=1; domain=.127.0.0.1 from 127.0.0.1 if the domain-attribute has leading dot as browsers reject them (albeit curl and IE11 don't).

gopherbot commented 3 years ago

Change https://golang.org/cl/326689 mentions this issue: net/http/cookiejar: allow cookies with an IP address in the domain attribute

neild commented 3 years ago

Why?

Sorry; I got the flag backwards. I meant to say the opposite: If we accept cookies with HostOnly=false and an IP address in Domain, then domainMatch would need updating to implement the full domain-match algorithm and verify that the request-host string is not an IP address.

I agree that setting HostOnly=true on these cookies is equivalent from a user perspective and requires the fewest code changes.

robx commented 2 years ago

If changes are made here, it might be a good idea to consider IPv6 addresses. I.e., URIs of the form http://[::1]/ (see RFC 3986) and whether such host names would translate into domain=::1 or domain=[::1].