Skip to content
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: make sure Dial returns random UDP port #5767

Closed
gopherbot opened this issue Jun 24, 2013 · 15 comments

Comments

Projects
None yet
6 participants
@gopherbot
Copy link

commented Jun 24, 2013

by fish@soundcloud.com:

Hello everyone,

I was digging around in the net package (to see how hard it would be to fix #5686) when
I realized that it's using math/rand for id randomization. math/rand is a pseudo-random
generator and, without being an crypto expert, this seems like it's predictable.

Since DNS security (without dnssec) depends solely on the randomness of dns ID and
source port, it's important for both to be random. This yield true especially since Dan
Kaminski's dns vulnerability discovery.

For the source port, net seems to depend on the kernel's source port randomization where
I'm not sure if they are unpredictable or whether go should explicitly generate a
random, unpredictable port as well.

Attached is a naive fix, replacing math/rand by crypto/rand. I'm happy for suggestions
in case there is a better way to fix it.

Attachments:

  1. replace-math-rand-by-crypto-rand-for-dns-id.patch (1312 bytes)
@gopherbot

This comment has been minimized.

Copy link
Author

commented Jun 24, 2013

Comment 1 by fish@soundcloud.com:

Sorry for the 'typo' "random id: %s". Here is an updated patch.

Attachments:

  1. replace-math-rand-by-crypto-rand-for-dns-id.patch (1338 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

Comment 2:

In practice the somewhat weak ID (note that it is rand ^ low-16-bits-of-nanotime, not
just rand) does not matter very much, because it is combined with a random UDP port on
most systems.
Specifically:
1) On Linux since 2.6.24, UDP without an explicit source gets a random port.
2) On my Mac (OS X 10.8.4), same. I don't know when it started.
3) Other systems should behave similarly.
Also in practice, this code never runs, because the default behavior is to use the C
library for DNS resolution; this code is only a fallback that is only built with
CGO_ENABLED=0 or on incomplete ports that do not yet support cgo.
It's somewhat counterproductive for net to pick random ports if the kernel is already
doing it. The kernel has a much better view of the situation and can do a better job. If
Go tries to do the job instead, the kernel cannot. 
It would be nice to know what operating systems do need "help". 
The attached program estimates the number of bits of entropy in the UDP port choice.
It's off by a small constant factor, so that on my Mac it reports 19.5 bits even though
the actual entropy appears to be around 13 bits. But if the ports were assigned in order
or something predictable like that it would say 0.3 bits or so, so it's fine.
If anyone wants to run the program and report back its determination and the OS name and
version, that'd be great.
I would strongly prefer net not to depend on crypto/rand.

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

Attachments:

  1. x.go (818 bytes)
@gopherbot

This comment has been minimized.

Copy link
Author

commented Jun 24, 2013

Comment 3 by fish@soundcloud.com:

Here (ubuntu 13.04) it reports 19.5 bits as well. But it seems like it's actually
hitting the go implementation (not libc) since changing the code (adding some debug
statement) actually works - although 'go env' shows CGO_ENABLED="1".
Anyway, if this is an issue it should be fixed in the go implementation as well.
Now regarding whether it's an issues in practice: It really depends on what you're
implementing. If an attacker can't predict when and what the victim is querying, it
shouldn't be a problem. But if the attacker can guess or even control it, like in the
case someone builds an caching dns resolver, some kind of proxy or just something
resolving the same name continuously, it becomes very feasible to exploit it.
Anyway, I personally would expect that both source port and id are unpredictable for any
dns (client) implementation. Ruby for example is using a secure random number for both
port and id: https://github.com/ruby/ruby/blob/trunk/lib/resolv.rb#L602
I would bet that's the same for almost all other dns client implementation.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Jun 25, 2013

Comment 4 by fish@soundcloud.com:

Hrm, any why did you rename the topic? The UDP port is another issue, but the main issue
is the predictable id. There is even an rfc about all that:
http://tools.ietf.org/html/rfc5452#section-9.1
@gopherbot

This comment has been minimized.

Copy link
Author

commented Jun 25, 2013

Comment 5 by fish@soundcloud.com:

BTW: There is no cgo for dns yet: https://golang.org/issue/5686?c=6
@minux

This comment has been minimized.

Copy link
Member

commented Jun 25, 2013

Comment 6:

Re #5, you misread my comment, what i mean is that SRV lookup doesn't use cgo (nor does
gai supports it).
normal DNS lookups are of course performed by cgo (if CGO_ENABLED=1).
@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 7:

I explained why I renamed the issue in my comment:
    In practice the somewhat weak ID (note that it is rand ^ 
    low-16-bits-of-nanotime, not just rand) does not matter very much, 
    because it is combined with a random UDP port on most systems.

Labels changed: added go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 8:

Labels changed: added feature.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2013

Comment 9:

Not for 1.2.

Labels changed: removed go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 10:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 11:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 12:

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 13:

Labels changed: added repo-main.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@danp

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

Can this be closed?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

Thanks, as far as I can tell it looks OK. Closing.

@golang golang locked and limited conversation to collaborators Jul 13, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.