Use crypto/rand for Id generation

[taruti]

Too many implementations have had problems with poor random ids and DNS spoofing attacks.

[andrewtj]

In case you haven't seen it, this came up previously in #48.

[taruti]

This is somewhat faster than #48, but of course still slower. Numbers and benchmark below.

PASS
BenchmarkOriginal   50000000            48.8 ns/op
BenchmarkDiscordianfish  5000000           645 ns/op
BenchmarkTaruti 10000000           270 ns/op

package main

import (
    cr "crypto/rand"
    "math/big"
    mr "math/rand"
    "testing"
    "time"
)

func BenchmarkOriginal(b *testing.B) {
    var sum int
    for i := 0; i < b.N; i++ {
        sum += int(uint16(mr.Int()) ^ uint16(time.Now().Nanosecond()))
    }
    _ = sum
}

func BenchmarkDiscordianfish(b *testing.B) {
    var sum int
    for i := 0; i < b.N; i++ {
        id, e := cr.Int(cr.Reader, big.NewInt(0xFFFF))
        if e != nil {
            panic(e)
        }
        sum += int(uint16(id.Uint64()))
    }
    _ = sum
}

func BenchmarkTaruti(b *testing.B) {
    var sum int
    for i := 0; i < b.N; i++ {
        var buf [2]byte
        _, e := cr.Read(buf[:])
        if e != nil {
            panic(e)
        }
        sum += int(uint16(buf[0]) | (uint16(buf[1]) << 8))
    }
    _ = sum
}

[miekg]

Thanks.

But as I mentioned in #48 I'm not convinced: is the current random so bad?

Also the real solution against spoofing is DNSSEC, the rest are tricks that
make it slightly harder.
On 24 Jul 2014 09:56, "Taru Karttunen" [email protected] wrote:

This is somewhat faster than #48 #48,
but of course still slower. Numbers and benchmark below.

PASS
BenchmarkOriginal 50000000 48.8 ns/op
BenchmarkDiscordianfish 5000000 645 ns/op
BenchmarkTaruti 10000000 270 ns/op

package main
import (
cr "crypto/rand"
"math/big"
mr "math/rand"
"testing"
"time")
func BenchmarkOriginal(b *testing.B) {
var sum int
for i := 0; i < b.N; i++ {
sum += int(uint16(mr.Int()) ^ uint16(time.Now().Nanosecond()))
}
_ = sum}
func BenchmarkDiscordianfish(b *testing.B) {
var sum int
for i := 0; i < b.N; i++ {
id, e := cr.Int(cr.Reader, big.NewInt(0xFFFF))
if e != nil {
panic(e)
}
sum += int(uint16(id.Uint64()))
}
_ = sum}
func BenchmarkTaruti(b *testing.B) {
var sum int
for i := 0; i < b.N; i++ {
var buf [2]byte
_, e := cr.Read(buf[:])
if e != nil {
panic(e)
}
sum += int(uint16(buf[0]) | (uint16(buf[1]) << 8))
}
_ = sum}

—
Reply to this email directly or view it on GitHub
#102 (comment).

[taruti]

Yes, it is that bad.

math/rand is not seeded with a random value unless the user does so explicitly.

math/rand has 2**31-1 states (i.e. less than four bytes).

Typically system clock nanosecond counts will be somewhat predictable depending on the os.

[miekg]

I'm not convinced... If you care about spoofing DNSSEC is your friend anything else is a bandaid. I'm inclined to close this PR.

[taruti]

Ok, no problem. Will just use a fork for our code.

[andrewtj]

If there's not too much overhead, making Id a variable might be a reasonable compromise. (Can't bench it myself for 3~4 hours.)

[miekg]

Or a boolean in client, SecureId to flick it to use this? Or allow it to be
a function you can redefine.
On 29 Jul 2014 08:40, "andrewtj" [email protected] wrote:

If there's not too much overhead, making Id a variable might be a
reasonable compromise. (Can't bench it myself for 3~4 hours.)

—
Reply to this email directly or view it on GitHub
#102 (comment).

[omribahumi]

@miekg did you check what random sources other DNS servers are using?
Two issues on this subject, it might really be a security concern. Not everyone are using DNSSEC...

[miekg]

No I did not check other servers. The 16! bit id is way too small to give
you any protection against spoofing at all.
Having said that I'm willing to consider to "this is slightly better"
option in a dns client. But this will not be the default.
On 29 Jul 2014 09:28, "Omri Bahumi" [email protected] wrote:

@miekg https://github.com/miekg did you check what random sources other
DNS servers are using?
Two issues on this subject, it might really be a security concern. Not
everyone are using DNSSEC...

—
Reply to this email directly or view it on GitHub
#102 (comment).

[miekg]

On second, second thought we use math.Rand, so if nobody can show me: we could spoof Go dns in x seconds and with crypto.Rand it took 2x seconds I'm sceptical.

[taruti]

Actually with using a random source port for every request (this can be done with the library out of box without any extra patches) one gets slightly more randomness (like 31 bits in total in a (w)lan, if going through nat to the name server it is reduced back...). Which makes the attacks somewhat harder. Of course DNSSEC (or having a nameserver in a VPN/over IPSEC/...) is the right solution, but that is not available in many cases.

For example BIND8 had a weak PRNG and that did make spoofing easier (they analysed the outputs and made the attack based on the prng) and it was made somewhat better in BIND9. There were papers about that 5-10 years ago.

[miekg]

If go dns does not randomize the source port that would be a bug. Honestly
I've never checked if it does.

Re: Id function. This night I figured we can make it a variable pointing to
a function. Which defaults to the current Id function. This should be easy
to overrule. This isn't an api change and saves you from running you're own
fork.
On 30 Jul 2014 06:22, "Taru Karttunen" [email protected] wrote:

Actually with using a random source port for every request (this can be
done with the library out of box without any extra patches) one gets
slightly more randomness (like 31 bits in total in a (w)lan, if going
through nat to the name server it is reduced back...). Which makes the
attacks somewhat harder. Of course DNSSEC (or having a nameserver in a
VPN/over IPSEC/...) is the right solution, but that is not available in
many cases.

For example BIND8 had a weak PRNG and that did make spoofing easier (they
analysed the outputs and made the attack based on the prng) and it was made
somewhat better in BIND9. There were papers about that 5-10 years ago.

—
Reply to this email directly or view it on GitHub
#102 (comment).

[miekg]

Actually I just pushed a change where Id is a variable which can reassigned to different function.