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

Use true random (crypto/rand) for Id #48

Closed
wants to merge 2 commits into from

Conversation

discordianfish
Copy link

Using math/rand for this seems to be insecure. See http://tools.ietf.org/html/rfc5452
It might also make sense to revisit the source port generation if it's random enough. I've added a similar patch to https://code.google.com/p/go/issues/detail?id=5767

func Id() uint16 {
return uint16(rand.Int()) ^ uint16(time.Now().Nanosecond())
id, err := rand.Int(rand.Reader, big.NewInt(0xFFFF))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the big.NewInt(0xFFFF) can be moved out of this function, or is it really necessary to allocate inside the Id() function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, it's not. I've made it a package wide variable.

@miekg
Copy link
Owner

miekg commented Jun 25, 2013

Can you create a benchmark function for sending packets with this change in? I'm curious (if any) what the effects are?

@discordianfish
Copy link
Author

Will do a benchmark tomorrow. But it's quite likely that it's slower.

@miekg
Copy link
Owner

miekg commented Jun 25, 2013

If its slower I'm hesitant about merging. godns already depends on the crypto package so that is not the issue, but being slower (for a small benefit, also see the golang-dev thread you link) makes me grince. The only secure DNS is DNSSEC or something completely different

@discordianfish
Copy link
Author

Unfortunately it's much slower:

[apollon~/dev] ➔ time ./miekg-crypto-rand

real 0m0.745s
user 0m0.586s
sys 0m0.180s
[apollon~/dev] ➔ time ./miekg-math-rand

real 0m0.080s
user 0m0.080s
sys 0m0.000s

"Bechmark": https://gist.github.com/discordianfish/5863154

I understand your concerns, but given someone writes a caching nameserver or any other application with predictable queries, it will be probably quite easy to exploit it. If you look at threads like https://groups.google.com/forum/#!topic/golang-nuts/_lXhTCvUVk4 people don't really care about how predictable math/rand is (which is fine). Anyway, I try to get some more insights into how practical an attack would be.

@miekg
Copy link
Owner

miekg commented Jun 26, 2013

Holy moly, that's a an order of magnitude slower! Still there are only 16 bit in the ID. A better ID might be http://tools.ietf.org/html/draft-vixie-dnsext-dns0x20-00, although not all servers support this.

@miekg
Copy link
Owner

miekg commented Jun 27, 2013

Closing, as I'm not going to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants