From 81f411b9853ab67434033fb8e5a68fbf81eac049 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 15 Jun 2023 01:43:34 +0000 Subject: [PATCH 1/6] chore: use robust RNG in `cryptorand` --- cryptorand/errors_test.go | 30 ------ cryptorand/numbers.go | 196 ++++++------------------------------- cryptorand/numbers_test.go | 117 +--------------------- cryptorand/strings.go | 23 ++--- 4 files changed, 37 insertions(+), 329 deletions(-) diff --git a/cryptorand/errors_test.go b/cryptorand/errors_test.go index c03b6064e48d0..36ce2faf6beab 100644 --- a/cryptorand/errors_test.go +++ b/cryptorand/errors_test.go @@ -30,31 +30,6 @@ func TestRandError(t *testing.T) { require.ErrorIs(t, err, io.ErrShortBuffer, "expected Int63 error") }) - t.Run("Uint64", func(t *testing.T) { - _, err := cryptorand.Uint64() - require.ErrorIs(t, err, io.ErrShortBuffer, "expected Uint64 error") - }) - - t.Run("Int31", func(t *testing.T) { - _, err := cryptorand.Int31() - require.ErrorIs(t, err, io.ErrShortBuffer, "expected Int31 error") - }) - - t.Run("Int31n", func(t *testing.T) { - _, err := cryptorand.Int31n(100) - require.ErrorIs(t, err, io.ErrShortBuffer, "expected Int31n error") - }) - - t.Run("Uint32", func(t *testing.T) { - _, err := cryptorand.Uint32() - require.ErrorIs(t, err, io.ErrShortBuffer, "expected Uint32 error") - }) - - t.Run("Int", func(t *testing.T) { - _, err := cryptorand.Int() - require.ErrorIs(t, err, io.ErrShortBuffer, "expected Int error") - }) - t.Run("Intn_32bit", func(t *testing.T) { _, err := cryptorand.Intn(100) require.ErrorIs(t, err, io.ErrShortBuffer, "expected Intn error") @@ -70,11 +45,6 @@ func TestRandError(t *testing.T) { require.ErrorIs(t, err, io.ErrShortBuffer, "expected Float64 error") }) - t.Run("Float32", func(t *testing.T) { - _, err := cryptorand.Float32() - require.ErrorIs(t, err, io.ErrShortBuffer, "expected Float32 error") - }) - t.Run("StringCharset", func(t *testing.T) { _, err := cryptorand.HexString(10) require.ErrorIs(t, err, io.ErrShortBuffer, "expected HexString error") diff --git a/cryptorand/numbers.go b/cryptorand/numbers.go index c030fce31ae01..e718a556a7c35 100644 --- a/cryptorand/numbers.go +++ b/cryptorand/numbers.go @@ -3,194 +3,56 @@ package cryptorand import ( "crypto/rand" "encoding/binary" - "time" - - "golang.org/x/xerrors" + insecurerand "math/rand" ) -// Most of this code is inspired by math/rand, so shares similar -// functions and implementations, but uses crypto/rand to generate -// random Int63 data. - -// Int64 returns a non-negative random 63-bit integer as a int64. -func Int63() (int64, error) { - var i int64 - err := binary.Read(rand.Reader, binary.BigEndian, &i) - if err != nil { - return 0, xerrors.Errorf("read binary: %w", err) - } - - if i < 0 { - return -i, nil - } - return i, nil +type cryptoSource struct { + err error } -// Uint64 returns a random 64-bit integer as a uint64. -func Uint64() (uint64, error) { - upper, err := Int63() - if err != nil { - return 0, xerrors.Errorf("read upper: %w", err) - } - - lower, err := Int63() - if err != nil { - return 0, xerrors.Errorf("read lower: %w", err) - } - - return uint64(lower)>>31 | uint64(upper)<<32, nil +func (*cryptoSource) Seed(seed int64) { + // Intentionally disregard seed } -// Int31 returns a non-negative random 31-bit integer as a int32. -func Int31() (int32, error) { - i, err := Int63() +func (c *cryptoSource) Int63() int64 { + var seed int64 + err := binary.Read(rand.Reader, binary.BigEndian, &seed) if err != nil { - return 0, err + c.err = err } - - return int32(i >> 32), nil + return 0 } -// Uint32 returns a 32-bit value as a uint32. -func Uint32() (uint32, error) { - i, err := Int63() +func (c *cryptoSource) Uint64() uint64 { + var seed uint64 + err := binary.Read(rand.Reader, binary.BigEndian, &seed) if err != nil { - return 0, err + c.err = err } - - return uint32(i >> 31), nil -} - -// Int returns a non-negative random integer as a int. -func Int() (int, error) { - i, err := Int63() - if err != nil { - return 0, err - } - - if i < 0 { - return int(-i), nil - } - return int(i), nil -} - -// Int63n returns a non-negative random integer in [0,max) as a int64. -func Int63n(max int64) (int64, error) { - if max <= 0 { - panic("invalid argument to Int63n") - } - - trueMax := int64((1 << 63) - 1 - (1<<63)%uint64(max)) - i, err := Int63() - if err != nil { - return 0, err - } - - for i > trueMax { - i, err = Int63() - if err != nil { - return 0, err - } - } - - return i % max, nil + return 0 } -// Int31n returns a non-negative integer in [0,max) as a int32. -func Int31n(max int32) (int32, error) { - i, err := Uint32() - if err != nil { - return 0, err - } - - return UnbiasedModulo32(i, max) +// secureRand returns a cryptographically secure random number generator. +func secureRand() (*insecurerand.Rand, *cryptoSource) { + var cs cryptoSource + //nolint:gosec + return insecurerand.New(&cs), &cs } -// UnbiasedModulo32 uniformly modulos v by n over a sufficiently large data -// set, regenerating v if necessary. n must be > 0. All input bits in v must be -// fully random, you cannot cast a random uint8/uint16 for input into this -// function. -// -//nolint:varnamelen -func UnbiasedModulo32(v uint32, n int32) (int32, error) { - prod := uint64(v) * uint64(n) - low := uint32(prod) - if low < uint32(n) { - thresh := uint32(-n) % uint32(n) - for low < thresh { - var err error - v, err = Uint32() - if err != nil { - return 0, err - } - prod = uint64(v) * uint64(n) - low = uint32(prod) - } - } - return int32(prod >> 32), nil +// Int64 returns a non-negative random 63-bit integer as a int64. +func Int63() (int64, error) { + rng, cs := secureRand() + return rng.Int63(), cs.err } -// Intn returns a non-negative integer in [0,max) as a int. +// Intn returns a non-negative integer in [0,max) as an int. func Intn(max int) (int, error) { - if max <= 0 { - panic("n must be a positive nonzero number") - } - - if max <= 1<<31-1 { - i, err := Int31n(int32(max)) - if err != nil { - return 0, err - } - - return int(i), nil - } - - i, err := Int63n(int64(max)) - if err != nil { - return 0, err - } - - return int(i), nil + rng, cs := secureRand() + return rng.Intn(max), cs.err } // Float64 returns a random number in [0.0,1.0) as a float64. func Float64() (float64, error) { -again: - i, err := Int63n(1 << 53) - if err != nil { - return 0, err - } - - f := (float64(i) / (1 << 53)) - if f == 1 { - goto again - } - - return f, nil -} - -// Float32 returns a random number in [0.0,1.0) as a float32. -func Float32() (float32, error) { -again: - i, err := Float64() - if err != nil { - return 0, err - } - - f := float32(i) - if f == 1 { - goto again - } - - return f, nil -} - -// Duration returns a random time.Duration value -func Duration() (time.Duration, error) { - i, err := Int63() - if err != nil { - return time.Duration(0), err - } - - return time.Duration(i), nil + rng, cs := secureRand() + return rng.Float64(), cs.err } diff --git a/cryptorand/numbers_test.go b/cryptorand/numbers_test.go index 24c8aeeb684b3..58fa2aa364c9e 100644 --- a/cryptorand/numbers_test.go +++ b/cryptorand/numbers_test.go @@ -1,8 +1,6 @@ package cryptorand_test import ( - "crypto/rand" - "encoding/binary" "testing" "github.com/stretchr/testify/require" @@ -21,96 +19,6 @@ func TestInt63(t *testing.T) { } } -func TestUint64(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Uint64() - require.NoError(t, err, "unexpected error from Uint64") - t.Logf("value: %v <- random?", v) - } -} - -func TestInt31(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Int31() - require.NoError(t, err, "unexpected error from Int31") - t.Logf("value: %v <- random?", v) - require.True(t, v >= 0, "values must be positive") - } -} - -func TestUnbiasedModulo32(t *testing.T) { - t.Parallel() - const mod = 7 - dist := [mod]uint32{} - - _, err := cryptorand.UnbiasedModulo32(0, mod) - require.NoError(t, err) - - for i := 0; i < 1000; i++ { - b := [4]byte{} - _, _ = rand.Read(b[:]) - v, err := cryptorand.UnbiasedModulo32(binary.BigEndian.Uint32(b[:]), mod) - require.NoError(t, err, "unexpected error from UnbiasedModulo32") - dist[v]++ - } - - t.Logf("dist: %+v <- evenly distributed?", dist) -} - -func TestUint32(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Uint32() - require.NoError(t, err, "unexpected error from Uint32") - t.Logf("value: %v <- random?", v) - } -} - -func TestInt(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Int() - require.NoError(t, err, "unexpected error from Int") - t.Logf("value: %v <- random?", v) - require.True(t, v >= 0, "values must be positive") - } -} - -func TestInt63n(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Int63n(1 << 35) - require.NoError(t, err, "unexpected error from Int63n") - t.Logf("value: %v <- random?", v) - require.True(t, v >= 0, "values must be positive") - require.True(t, v < 1<<35, "values must be less than 1<<35") - } - - // Expect a panic if max is negative - require.PanicsWithValue(t, "invalid argument to Int63n", func() { - cryptorand.Int63n(0) - }) -} - -func TestInt31n(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Int31n(100) - require.NoError(t, err, "unexpected error from Int31n") - t.Logf("value: %v <- random?", v) - require.True(t, v >= 0, "values must be positive") - require.True(t, v < 100, "values must be less than 100") - } -} - func TestIntn(t *testing.T) { t.Parallel() @@ -127,7 +35,7 @@ func TestIntn(t *testing.T) { require.NoError(t, err, "expected Intn to work for 64-bit int") // Expect a panic if max is negative - require.PanicsWithValue(t, "n must be a positive nonzero number", func() { + require.PanicsWithValue(t, "invalid argument to Intn", func() { cryptorand.Intn(0) }) } @@ -143,26 +51,3 @@ func TestFloat64(t *testing.T) { require.True(t, v < 1.0, "values must be less than 1.0") } } - -func TestFloat32(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Float32() - require.NoError(t, err, "unexpected error from Float32") - t.Logf("value: %v <- random?", v) - require.True(t, v >= 0.0, "values must be positive") - require.True(t, v < 1.0, "values must be less than 1.0") - } -} - -func TestDuration(t *testing.T) { - t.Parallel() - - for i := 0; i < 20; i++ { - v, err := cryptorand.Duration() - require.NoError(t, err, "unexpected error from Duration") - t.Logf("value: %v <- random?", v) - require.True(t, v >= 0.0, "values must be positive") - } -} diff --git a/cryptorand/strings.go b/cryptorand/strings.go index fd16ba0af7e0e..066602f16631f 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -1,9 +1,9 @@ package cryptorand import ( - "crypto/rand" - "encoding/binary" "strings" + + "golang.org/x/xerrors" ) // Charsets @@ -36,32 +36,23 @@ const ( func StringCharset(charSetStr string, size int) (string, error) { charSet := []rune(charSetStr) - if len(charSet) == 0 || size == 0 { + if size == 0 { return "", nil } - // This buffer facilitates pre-emptively creation of random uint32s - // to reduce syscall overhead. - ibuf := make([]byte, 4*size) - - _, err := rand.Read(ibuf) - if err != nil { - return "", err + if len(charSet) == 0 { + return "", xerrors.Errorf("charSetStr must not be empty") } var buf strings.Builder buf.Grow(size) for i := 0; i < size; i++ { - count, err := UnbiasedModulo32( - binary.BigEndian.Uint32(ibuf[i*4:(i+1)*4]), - int32(len(charSet)), - ) + ci, err := Intn(len(charSet)) if err != nil { return "", err } - - _, _ = buf.WriteRune(charSet[count]) + _, _ = buf.WriteRune(charSet[ci]) } return buf.String(), nil From 5950a9adfc2c243f9753aff2ef022bf3a83ce297 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 15 Jun 2023 01:54:24 +0000 Subject: [PATCH 2/6] Fix sign bit --- cryptorand/numbers.go | 16 +++++++++------- cryptorand/numbers_test.go | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/cryptorand/numbers.go b/cryptorand/numbers.go index e718a556a7c35..aa5046ae8e17f 100644 --- a/cryptorand/numbers.go +++ b/cryptorand/numbers.go @@ -10,26 +10,28 @@ type cryptoSource struct { err error } -func (*cryptoSource) Seed(seed int64) { +func (*cryptoSource) Seed(_ int64) { // Intentionally disregard seed } func (c *cryptoSource) Int63() int64 { - var seed int64 - err := binary.Read(rand.Reader, binary.BigEndian, &seed) + var n int64 + err := binary.Read(rand.Reader, binary.BigEndian, &n) if err != nil { c.err = err } - return 0 + // The sign bit must be cleared to ensure the final value is non-negative. + n &= 0x7fffffffffffffff + return n } func (c *cryptoSource) Uint64() uint64 { - var seed uint64 - err := binary.Read(rand.Reader, binary.BigEndian, &seed) + var n uint64 + err := binary.Read(rand.Reader, binary.BigEndian, &n) if err != nil { c.err = err } - return 0 + return n } // secureRand returns a cryptographically secure random number generator. diff --git a/cryptorand/numbers_test.go b/cryptorand/numbers_test.go index 58fa2aa364c9e..0ffedf78b9d9e 100644 --- a/cryptorand/numbers_test.go +++ b/cryptorand/numbers_test.go @@ -26,7 +26,7 @@ func TestIntn(t *testing.T) { v, err := cryptorand.Intn(100) require.NoError(t, err, "unexpected error from Intn") t.Logf("value: %v <- random?", v) - require.True(t, v >= 0, "values must be positive") + require.GreaterOrEqual(t, v, 0, "values must be positive") require.True(t, v < 100, "values must be less than 100") } From 96cf3defa25d52bef38a72a00cbd1ff5339607e4 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 15 Jun 2023 02:10:56 +0000 Subject: [PATCH 3/6] Bring back modulo --- cryptorand/strings.go | 44 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/cryptorand/strings.go b/cryptorand/strings.go index 066602f16631f..31dff677808c7 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -1,6 +1,8 @@ package cryptorand import ( + "crypto/rand" + "encoding/binary" "strings" "golang.org/x/xerrors" @@ -32,23 +34,57 @@ const ( Human = "23456789abcdefghjkmnpqrstuvwxyz" ) +// unbiasedModulo32 uniformly modulos v by n over a sufficiently large data +// set, regenerating v if necessary. n must be > 0. All input bits in v must be +// fully random, you cannot cast a random uint8/uint16 for input into this +// function. +// +//nolint:varnamelen +func unbiasedModulo32(v uint32, n int32) (int32, error) { + prod := uint64(v) * uint64(n) + low := uint32(prod) + if low < uint32(n) { + thresh := uint32(-n) % uint32(n) + for low < thresh { + err := binary.Read(rand.Reader, binary.BigEndian, &v) + if err != nil { + return 0, err + } + prod = uint64(v) * uint64(n) + low = uint32(prod) + } + } + return int32(prod >> 32), nil +} + // StringCharset generates a random string using the provided charset and size func StringCharset(charSetStr string, size int) (string, error) { - charSet := []rune(charSetStr) - if size == 0 { return "", nil } - if len(charSet) == 0 { + if len(charSetStr) == 0 { return "", xerrors.Errorf("charSetStr must not be empty") } + charSet := []rune(charSetStr) + + // We pre-allocate the entropy to amortize the crypto/rand syscall overhead. + entropy := make([]byte, 4*size) + + _, err := rand.Read(entropy) + if err != nil { + return "", err + } + var buf strings.Builder buf.Grow(size) for i := 0; i < size; i++ { - ci, err := Intn(len(charSet)) + ci, err := unbiasedModulo32( + binary.BigEndian.Uint32(entropy[i*4:(i+1)*4]), + int32(len(charSet)), + ) if err != nil { return "", err } From 45a2da83e66bb1a7f2db031fdb2129d3cccec49c Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 15 Jun 2023 02:18:19 +0000 Subject: [PATCH 4/6] cleanup code --- cryptorand/strings.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cryptorand/strings.go b/cryptorand/strings.go index 31dff677808c7..35a4ff7157580 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -81,13 +81,17 @@ func StringCharset(charSetStr string, size int) (string, error) { buf.Grow(size) for i := 0; i < size; i++ { + r := binary.BigEndian.Uint32(entropy[:4]) + entropy = entropy[4:] + ci, err := unbiasedModulo32( - binary.BigEndian.Uint32(entropy[i*4:(i+1)*4]), + r, int32(len(charSet)), ) if err != nil { return "", err } + _, _ = buf.WriteRune(charSet[ci]) } From a6fe2c41d8c37f3d4ebd761c10a448d312bb5d47 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 15 Jun 2023 14:59:59 +0000 Subject: [PATCH 5/6] period --- cryptorand/strings.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cryptorand/strings.go b/cryptorand/strings.go index 35a4ff7157580..aa613d24128cb 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -57,7 +57,7 @@ func unbiasedModulo32(v uint32, n int32) (int32, error) { return int32(prod >> 32), nil } -// StringCharset generates a random string using the provided charset and size +// StringCharset generates a random string using the provided charset and size. func StringCharset(charSetStr string, size int) (string, error) { if size == 0 { return "", nil From ee663ce69532d89940cb93131ae70bcb155e96e5 Mon Sep 17 00:00:00 2001 From: Ammar Bandukwala Date: Thu, 15 Jun 2023 15:00:22 +0000 Subject: [PATCH 6/6] fixup! period --- cryptorand/strings.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cryptorand/strings.go b/cryptorand/strings.go index aa613d24128cb..69e9d529d5993 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -39,6 +39,9 @@ const ( // fully random, you cannot cast a random uint8/uint16 for input into this // function. // +// See more details on this algorithm here: +// https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/ +// //nolint:varnamelen func unbiasedModulo32(v uint32, n int32) (int32, error) { prod := uint64(v) * uint64(n)