From 400b5e3b88d985fc9b62ea6e6a41811293a17bbc Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 5 Dec 2019 13:00:45 -0800 Subject: [PATCH 1/3] Use crypto/rand for random id generation. Fixes #1043 and #1037 --- msg.go | 53 +++++++++++++---------------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/msg.go b/msg.go index e04fb5d77..cb3b1eba0 100644 --- a/msg.go +++ b/msg.go @@ -15,10 +15,9 @@ import ( "encoding/binary" "fmt" "math/big" - "math/rand" + mrand "math/rand" "strconv" "strings" - "sync" ) const ( @@ -74,52 +73,26 @@ var ( ) // Id by default, returns a 16 bits random number to be used as a -// message id. The random provided should be good enough. This being a -// variable the function can be reassigned to a custom function. -// For instance, to make it return a static value: +// message id. This being a variable the function can be reassigned +// to a custom function. For instance, to make it return a static value: // // dns.Id = func() uint16 { return 3 } +// +// Randomness comes from crypto/rand. In the rare case that there is an error +// getting bytes from that source, this function will silently fall back to +// the predictable math/rand. If your program has more stringent requirements, +// you should override Id to do something different (e.g., terminate). var Id = id -var ( - idLock sync.Mutex - idRand *rand.Rand -) - // id returns a 16 bits random number to be used as a // message id. The random provided should be good enough. func id() uint16 { - idLock.Lock() - - if idRand == nil { - // This (partially) works around - // https://github.com/golang/go/issues/11833 by only - // seeding idRand upon the first call to id. - - var seed int64 - var buf [8]byte - - if _, err := crand.Read(buf[:]); err == nil { - seed = int64(binary.LittleEndian.Uint64(buf[:])) - } else { - seed = rand.Int63() - } - - idRand = rand.New(rand.NewSource(seed)) + var v uint16 + err := binary.Read(crand.Reader, binary.BigEndian, &v) + if err != nil { + v = uint16(mrand.Uint32()) } - - // The call to idRand.Uint32 must be within the - // mutex lock because *rand.Rand is not safe for - // concurrent use. - // - // There is no added performance overhead to calling - // idRand.Uint32 inside a mutex lock over just - // calling rand.Uint32 as the global math/rand rng - // is internally protected by a sync.Mutex. - id := uint16(idRand.Uint32()) - - idLock.Unlock() - return id + return v } // MsgHdr is a a manually-unpacked version of (id, bits). From 445335556dd03bdbb6b6fcbf877d2ceb8e03e987 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Sat, 7 Dec 2019 18:43:05 -0800 Subject: [PATCH 2/3] Panic on rare crypto/rand error. --- msg.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/msg.go b/msg.go index cb3b1eba0..f4823d940 100644 --- a/msg.go +++ b/msg.go @@ -15,7 +15,6 @@ import ( "encoding/binary" "fmt" "math/big" - mrand "math/rand" "strconv" "strings" ) @@ -72,16 +71,12 @@ var ( ErrTime error = &Error{err: "bad time"} // ErrTime indicates a timing error in TSIG authentication. ) -// Id by default, returns a 16 bits random number to be used as a -// message id. This being a variable the function can be reassigned -// to a custom function. For instance, to make it return a static value: +// Id by default, returns a 16 bit random number (from crypto/rand) to +// be used as a message id. This being a variable the function can be +// reassigned to a custom function. For instance, to make it return a +// static value: // // dns.Id = func() uint16 { return 3 } -// -// Randomness comes from crypto/rand. In the rare case that there is an error -// getting bytes from that source, this function will silently fall back to -// the predictable math/rand. If your program has more stringent requirements, -// you should override Id to do something different (e.g., terminate). var Id = id // id returns a 16 bits random number to be used as a @@ -90,7 +85,7 @@ func id() uint16 { var v uint16 err := binary.Read(crand.Reader, binary.BigEndian, &v) if err != nil { - v = uint16(mrand.Uint32()) + panic(fmt.Sprintf("failed to get randomness: %s", err)) } return v } From a509d69e3fa2f68ffb86a49762a23a341611583a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 9 Dec 2019 12:39:41 -0800 Subject: [PATCH 3/3] Fixes in response to review. --- msg.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/msg.go b/msg.go index f4823d940..293813005 100644 --- a/msg.go +++ b/msg.go @@ -11,7 +11,7 @@ package dns //go:generate go run msg_generate.go import ( - crand "crypto/rand" + "crypto/rand" "encoding/binary" "fmt" "math/big" @@ -71,10 +71,10 @@ var ( ErrTime error = &Error{err: "bad time"} // ErrTime indicates a timing error in TSIG authentication. ) -// Id by default, returns a 16 bit random number (from crypto/rand) to -// be used as a message id. This being a variable the function can be -// reassigned to a custom function. For instance, to make it return a -// static value: +// Id by default returns a 16-bit random number to be used as a message id. The +// number is drawn from a cryptographically secure random number generator. +// This being a variable the function can be reassigned to a custom function. +// For instance, to make it return a static value for testing: // // dns.Id = func() uint16 { return 3 } var Id = id @@ -82,12 +82,12 @@ var Id = id // id returns a 16 bits random number to be used as a // message id. The random provided should be good enough. func id() uint16 { - var v uint16 - err := binary.Read(crand.Reader, binary.BigEndian, &v) + var output uint16 + err := binary.Read(rand.Reader, binary.BigEndian, &output) if err != nil { - panic(fmt.Sprintf("failed to get randomness: %s", err)) + panic("dns: reading random id failed: " + err.Error()) } - return v + return output } // MsgHdr is a a manually-unpacked version of (id, bits).