New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: reject Lookup("www.google.com..") on all systems #7122

Open
vdobler opened this Issue Jan 14, 2014 · 8 comments

Comments

Projects
None yet
6 participants
@vdobler
Contributor

vdobler commented Jan 14, 2014

On Linux domain look-up is too liberal and accepts domain
names with two trailing dots like "www.google.com..".
Most probably this is an issue on the OS level as
gethostbyname(3) shows this behavior also.

Allowing two trailing dots instead of none or
one (for a FQDN) seems harmless but it messes with the
domain name canonicalization done in net/http/cookiejar
and the determination of the public suffix of a domain.

I'd like to argue that malformed domains should be
rejected on look-up on any OS; at least no request
should be made to such malformed domain names.

This issue is not present on Windows and OSX.
Domains with three trailing dots are properly rejected
on Linux, Windows and OSX.
Other OS are untested.

The following program demonstrates the issue:
On Linux neither net.LookupHost nor http.Get fail.

--------------
package main

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

func main() {
    addrs, err := net.LookupHost("www.google.com..")
    if err != nil {
        log.Fatal(err)
    }
    for _, a := range addrs {
        log.Println(a)
    }
    res, err := http.Get("http://www.google.com../foo")
    if err != nil {
        log.Fatal(err)
    }
    defer res.Body.Close()
    io.Copy(os.Stdout, res.Body)
}
@mikioh

This comment has been minimized.

Contributor

mikioh commented Jan 15, 2014

Comment 1:

Perhaps we should ask rsc (or iant?) why did you guys choose hostname-like
platform-dependent (but plain and  flexible) namespace instead of host subcomponent of
URI namespace. Anyway, the current net package takes both namespaces and never
recognises the difference btw those twos. I personally hope that packages that have to
take account of their property's namespace would handle this issue at each package.
@nigeltao

This comment has been minimized.

Contributor

nigeltao commented Jan 20, 2014

Comment 2:

This is possibly a glibc bug. In any case, I filed
https://sourceware.org/bugzilla/show_bug.cgi?id=16469
@rsc

This comment has been minimized.

Contributor

rsc commented Jan 22, 2014

Comment 3:

I do not understand the question in #1.
I agree that net.LookupHost("www.google.com..") should be made to fail everywhere.
@mikioh

This comment has been minimized.

Contributor

mikioh commented Jan 22, 2014

Comment 4:

I'm not good at this section but what if we have some entries in local database (e.g.
/etc/hosts) like the following:
::1  www.google.com..
127.0.0.1  www.google.com..
getaddrinfo (or similar kernel/system library service) on some implementation works as
not only a DNS resolver but mDNS or some network instance (that is represented in
net-unicode, see http://tools.ietf.org/html/rfc6762#appendix-F) resolver.
# ping6 www.google.com..
PING6(56=40+8+8 bytes) ::1 --> ::1
16 bytes from ::1, icmp_seq=0 hlim=64 time=0.092 ms
# ping www.google.com..
PING www.google.com.. (127.0.0.1): 56 data bytes
64 bytes from 127.0.0.1: icmp_seq=0 ttl=64 time=0.059 ms
I have no concrete opinion about this issue, whether we should normalize network
instance/node namespace in the package net API level.
@rsc

This comment has been minimized.

Contributor

rsc commented Mar 3, 2014

Comment 5:

Labels changed: added release-go1.3.

Status changed to Accepted.

@rsc

This comment has been minimized.

Contributor

rsc commented May 9, 2014

Comment 6:

Labels changed: added release-none, suggested, removed release-go1.3.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Oct 1, 2014

Comment 7:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

gopherbot commented Mar 6, 2017

CL https://golang.org/cl/37843 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 21, 2017

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment