net: dns needs to use unique request IDs

[gopherbot]

by whitetiger0990:

What steps will reproduce the problem?
1. _,error:=net.Dial("tcp","",host);
2. Have a 2Wire router (which is what I believe is causing the problem, but
may be wrong)

What is the expected output? What do you see instead?
I expect error==nil, but instead error=="dial tcp google.com:80: lookup
google.com. on 192.168.1.254:53: no answer from server"

What is your $GOOS?  $GOARCH?
linux amd64

Which revision are you using?  (hg identify)
cf1a9b1f9bee+ tip


Please provide any additional information below.
See attached for 4 packets associated with the execution of my test script.
Also attached is my test script. It sometimes works, outputting:

google.com:80
B <nil>

but then sometimes outputs:
google.com:80
B dial tcp google.com:80: lookup google.com. on 192.168.1.254:53: no answer
from server



I'm pretty sure it's something related to the 2Wire router because both my
mother and grandmother have one and it appears to be the same issue, and
nothing else on my computer seems to be experiencing any difficulty (but if
someone can provide some way to test that, I'll be glad to do it. I'm not
completely ruling out some problem with the computer).

The router doesn't even seem to respond several google.com queries, but it
responds right away to the incorrect google.com.gateway.2wire.net.

Thanks, if any more information is needed, just ask.

Attachments:

1. packets.txt (5198 bytes)
2. dialing.go (162 bytes)

[nictuku]

Comment 1:

Can you paste the contents of your /etc/resolv.conf file?
Also, see if the DNS server in 192.168.1.254 is properly configured. Maybe one of the 
servers it's pointing to is down.

[gopherbot]

Comment 2 by whitetiger0990:

josh@Eva:~$ cat /etc/resolv.conf 
domain gateway.2wire.net
search gateway.2wire.net
nameserver 192.168.1.254
The router is set up to get the name servers automatically, and the ones it has both
work and are pingable and everything.
josh@Eva:~$ dig "google.com" @68.94.156.1 +short
74.125.53.100
74.125.67.100
74.125.45.100
josh@Eva:~$ dig "google.com" @68.94.157.1 +short
74.125.67.100
74.125.45.100
74.125.53.100

[gopherbot]

Comment 3 by e.scott.daniels:

I have noticed this message occasionally; Linux, 386, cf1a9b1f9bee+ tip.  My current 
network environment is wireless connectivity to a NetGear router.  
I believe that the cause is occasional slow router response (> 1s). The default (pkg/
net/dnsconfig.go) is to make one attempt with a timeout of 1s.  I added the 
following line to /etc/resolv.conf to force multiple retries -- this seems to have 
eliminated these messages though I haven't done more than just a bit of testing. 
options attempts:5

[gopherbot]

Comment 4 by whitetiger0990:

Still doesn't seem to work. Does anything have to be restarted after changing
resolv.conf?
josh@Eva:/tmp$ ./6.out 
google.com:80
B dial tcp google.com:80: lookup google.com. on 192.168.1.254:53: no answer from server
josh@Eva:/tmp$ host google.com
google.com has address 74.125.67.100
google.com has address 74.125.53.100
google.com has address 74.125.45.100
google.com mail is handled by 10 google.com.s9a1.psmtp.com.
google.com mail is handled by 10 google.com.s9a2.psmtp.com.
google.com mail is handled by 10 google.com.s9b2.psmtp.com.
google.com mail is handled by 10 google.com.s9b1.psmtp.com.
josh@Eva:/tmp$ cat /etc/r
rarfiles.lst  rc1.d/        rc3.d/        rc5.d/        rc.local      readahead/   
resolv.conf   rpc           rsyslog.d/    
rc0.d/        rc2.d/        rc4.d/        rc6.d/        rcS.d/        resolvconf/  
rmt           rsyslog.conf  
josh@Eva:/tmp$ cat /etc/resolv.conf 
domain gateway.2wire.net
search gateway.2wire.net
nameserver 192.168.1.254
options attempts:5

[gopherbot]

Comment 5 by e.scott.daniels:

Other than restarting your programme, you shouldn't need to start anything else. The 
dns parse function in the net package should open and read resolv.conf and pick up 
the changes when you first attempt to make the connection.  
You could also try this in resolv.conf:
options attempts:5 timeout:10
That will set the timeout to 10s for each attempt.  
I did some playing and on my net it seems that once every so often the UDP packet to 
the DNS port is dropped (no response) which generates a retry or the error if I 
don't have retries turned on. 
Maybe something else is going on; sorry for the red herring if that's the case.

[gopherbot]

Comment 6 by charlie.c.kim:

Had the same problem.   You can work around it by setting resolve.conf to use
nameservers at 8.8.8.8/8.8.4.4 :-)
The salient clue is that that the first DNS request goes through -- subsequent ones
do not (at least for a while).
The current code uses a constant transaction id of x1234 (see _Exchange in
dnsclient.go).   The 2wire gateway seems not respond to duplicate requests.
Constant id (out.id=0x1234) needs to be replaced.   Not sure of direction of
dnsclient, but could start with incrementing global for now (as this solves an
immediate issue).   Probably best to  have a random start basis for each go program
(easy) or goproc (harder since it would need goproc specific context & is probably
overkill)  and it should be checked in the answer routine (this is just being safe,
but may not be necessary).
[Note: this has been an irritant for a while as the go build test suite fails unless
the resolve.conf is set away from a 2wire router]

[gopherbot]

Comment 7 by charlie.c.kim:

This should be sufficient for now -- use a random id for every DNS request.   ID is
already checked.
Other:
Source port randomization should also be checked.
Is this the best way to get 16 unsigned bits of random number: uint16(rand.Uint32())?
Other DNS spoof issues should be checked out.
diff -r bdfc3faa253a src/pkg/net/dnsclient.go
--- a/src/pkg/net/dnsclient.go  Fri Dec 04 21:58:32 2009 -0800
+++ b/src/pkg/net/dnsclient.go  Fri Dec 04 23:16:04 2009 -0800
@@ -17,6 +17,7 @@
 import (
        "once";
        "os";
+        "rand";
 )
 // DNSError represents a DNS lookup error.
@@ -44,7 +45,7 @@
                return nil, &DNSError{"name too long", name, ""}
        }
        out := new(_DNS_Msg);
-       out.id = 0x1234;
+       out.id = uint16(rand.Uint32());          // random to minimize spoof
        out.question = []_DNS_Question{
                _DNS_Question{name, _DNS_TypeA, _DNS_ClassINET},
        };

[gopherbot]

Comment 8 by cck3737:

Thinking about the id (should it increment, be random) lead to the notes in comment 7
with respect to DNS spoofing issues and some of the protections that a client should
take.  But the patch in Comment 7 is insufficient as the id will be non-random.   The
implementation in comment 7 calls rand without setting the seed and thus is a fixed
sequence (of random numbers :-)).   While often this will work around the 2wire
problem as most programs will result in multiple DNS requests--the 2wire will allow a
sequence like 0x1234, 0x1111, 0x1234, but not 0x1234, 0x1234; but there may still be
unexpected  failures.
The best solution for Linux and such would be to get some bits from /dev/urandom to
seed the rng but that's OS dependent code that doesn't belong here.
Something like below would leave things in a slightly better position, but is ugly
(and attackable if ts can be inferred to resolution); would likely be, at best, an
interim patch; and only satisfies one of several requirements needed to protect
against DNS spoofing (cf. draft-hubert-dns-anti-spoofing-00 - Measures to prevent DNS
spoofing).
diff -r 75168d77b51d src/pkg/net/dnsclient.go
--- a/src/pkg/net/dnsclient.go  Fri Dec 18 17:24:58 2009 -0800
+++ b/src/pkg/net/dnsclient.go  Fri Dec 18 21:25:38 2009 -0800
@@ -17,6 +17,8 @@
 import (
        "once"
        "os"
+       "rand"
+       "time"
 )
 // DNSError represents a DNS lookup error.
@@ -44,7 +46,18 @@
                return nil, &DNSError{"name too long", name, ""}
        }
        out := new(_DNS_Msg)
-       out.id = 0x1234
+       {
+               // fudge a seed by using low order bits of nanoseconds (mask upper by
shifting t2, fill out bottom using bits in the middle t3 )
+               // (less than ms resolution would likely cause this to fall over)
+               // not safe, not optimal; best to access /dev/urandom but
+               // that's not OS independent (probably belongs in a package like rand)
+               t := time.Nanoseconds()
+               t2 := time.Nanoseconds() % (100 * 1000 * 1000)
+               t3 := time.Nanoseconds() % (100 * 1000 * 1000) / 1000
+               sval := t + t2<<32 + t3
+               myrand := rand.New(rand.NewSource(sval))
+               out.id = uint16(myrand.Uint32()) // random to minimize spoof
+       }
        out.question = []_DNS_Question{
                _DNS_Question{name, _DNS_TypeA, _DNS_ClassINET},
        }

[gopherbot]

Comment 9 by charlie.c.kim:

Cleaner version:
diff -r affe0f093696 src/pkg/net/dnsclient.go
--- a/src/pkg/net/dnsclient.go  Wed Dec 23 22:08:49 2009 -0800
+++ b/src/pkg/net/dnsclient.go  Thu Dec 24 01:27:38 2009 -0800
@@ -17,6 +17,8 @@
 import (
        "once"
        "os"
+       "rand"
+/*     "time" */
 )
 // DNSError represents a DNS lookup error.
@@ -37,6 +39,84 @@
 const noSuchHost = "no such host"
+var drandStream *rand.Rand     // persist stream
+
+// return byte array of at least bits bits
+// Should a version of this exist in os.?
+func _GetRandBits(bits int) ([]byte, os.Error) {
+       bytcnt := ( bits + 7 ) / 8 // Get right # of bytes
+       randfile, err := os.Open("/dev/urandom", os.O_RDONLY, 0)
+       if err != nil {
+               // fallback to /dev/random for FreeBSD, etc.?
+               // maybe should disallow on Linux
+               // prob. need diff. path for Windows
+               if e, ok := err.(*os.PathError); ok && e.Error == os.ENOENT {
+                       randfile, err = os.Open("/dev/random", os.O_RDONLY, 0)
+                       if err != nil {
+                               return nil, err;
+                       }
+               } else {
+                       return nil, err
+               }
+       }
+       defer randfile.Close()
+       bytes := make([]byte, bytcnt)
+       n, err := randfile.Read(bytes)
+       if err != nil {
+               return nil, err
+       }
+       if n < bytcnt {
+               return nil, err
+       }
+       return bytes, nil
+}
+
+func getRandId() (uint16, os.Error) {
+       if drandStream == nil  {
+               var sval int64;
+               bytes, err := _GetRandBits(64)
+               if err != nil {
+/*
+// disable for now
+                       // could be fallback position - don't want to fail DNS due to
problems seeding rand
+                       // but few "supported" systems should have an issue with
GetRandBits...
+                       // note: this may be one of the few cases wehre it is better
to contiuously reseed
+                       // (so initial seed (which is guessable +/- some variance)
cannot be "matched"
+                       // bsed on observed valuse)
+                       // fudge a seed by using low order bits of nanoseconds (mask
upper by
+                       // shifting t2, fill out bottom using bits in the middle t3 )
+                       // (less than ms resolution would likely cause this to fall over)
+                       // not safe, not optimal; best to access /dev/urandom but
+                       // that's not OS independent (probably belongs in a package
like rand)
+                       t := time.Nanoseconds()
+                       t2 := time.Nanoseconds() % (100 * 1000 * 1000)
+                       t3 := time.Nanoseconds() % (100 * 1000 * 1000) / 1000
+                       sval = t + t2<<32 + t3
+*/
+                       return 0, err;
+               } else {
+                       // better way?
+                       sval = int64(bytes[0])<<56 | int64(bytes[1])<<48 |
int64(bytes[2])<<40 | int64(bytes[3])<<32 |
+                               int64(bytes[4])<<24 | int64(bytes[5])<<16 |
int64(bytes[6])<<8 | int64(bytes[7])
+
+               }
+               drandStream = rand.New(rand.NewSource(sval))
+       }
+       return uint16(drandStream.Uint32()), nil;
+}
+
+func compareQuestions(outq, inq []_DNS_Question) os.Error {
+       if (len(outq) != len(inq)) {
+               return os.EINVAL;
+       }
+       for i := 0; i < len(outq); i++ {
+               if inq[i].Name != outq[i].Name || inq[i].Qtype != outq[i].Qtype ||
inq[i].Qclass != outq[i].Qclass {
+                       return os.EINVAL;
+               }
+       }
+       return nil;
+}
+
 // Send a request on the connection and hope for a reply.
 // Up to cfg.attempts attempts.
 func _Exchange(cfg *_DNS_Config, c Conn, name string) (m *_DNS_Msg, err os.Error) {
@@ -44,7 +124,10 @@
                return nil, &DNSError{"name too long", name, ""}
        }
        out := new(_DNS_Msg)
-       out.id = 0x1234
+       out.id, err = getRandId()
+       if err != nil {
+               return nil, err;
+       }
        out.question = []_DNS_Question{
                _DNS_Question{name, _DNS_TypeA, _DNS_ClassINET},
        }
@@ -54,6 +137,8 @@
                return nil, &DNSError{"internal error - cannot pack message", name, ""}
        }
+       a := c.RemoteAddr()
+
        for attempt := 0; attempt < cfg.attempts; attempt++ {
                n, err := c.Write(msg)
                if err != nil {
@@ -63,7 +148,12 @@
                c.SetReadTimeout(1e9) // nanoseconds
                buf := make([]byte, 2000) // More than enough.
-               n, err = c.Read(buf)
+               var addr *UDPAddr;
+               uc, ok := c.(*UDPConn);
+               if !ok {
+                       return nil, os.EINVAL;
+               }
+               n, addr, err = uc.ReadFromUDP(buf)
                if isEAGAIN(err) {
                        err = nil
                        continue
@@ -76,10 +166,20 @@
                if !in.Unpack(buf) || in.id != out.id {
                        continue
                }
+               // must have remote addr available..
+               // must match incoming
+               if a == nil || c.RemoteAddr().String() != addr.String() {
+                       return nil, os.EINVAL; // toss error
+               }
+               // should compare question section
+               err = compareQuestions(out.question, in.question);
+               if err != nil {
+                       return nil, os.EINVAL;
+               }
                return in, nil
        }
        var server string
-       if a := c.RemoteAddr(); a != nil {
+       if a != nil {
                server = a.String()
        }
        return nil, &DNSError{"no answer from server", name, server}

[rsc]

Comment 10:

Owner changed to r...@golang.org.

Status changed to Accepted.

[gopherbot]

Comment 11 by stephenm@golang.org:

Fixed in revision 4b3115346f: http://code.google.com/p/go/source/detail?r=4b3115346f

Status changed to Fixed.