From 92971b54f7a6c65bc06679d9539eefaa42253204 Mon Sep 17 00:00:00 2001 From: Trim21 Date: Sat, 22 Jul 2023 07:36:58 +0800 Subject: [PATCH 1/6] fix(sec): `randomString` bias --- middleware/util.go | 39 ++++++++++++++++++++++++++++----------- middleware/util_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/middleware/util.go b/middleware/util.go index aa34d78f3..00ed66b1c 100644 --- a/middleware/util.go +++ b/middleware/util.go @@ -2,7 +2,7 @@ package middleware import ( "crypto/rand" - "fmt" + "io" "strings" ) @@ -55,17 +55,34 @@ func matchSubdomain(domain, pattern string) bool { return false } +const randomStringCharset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" +const randomStringCharsetLen = 52 // len(randomStringCharset) +const randomStringMaxByte = 255 - (256 % randomStringCharsetLen) + func randomString(length uint8) string { - charset := "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + b := make([]byte, length) + r := make([]byte, length+(length/3)) + var i uint8 = 0 - bytes := make([]byte, length) - _, err := rand.Read(bytes) - if err != nil { - // we are out of random. let the request fail - panic(fmt.Errorf("echo randomString failed to read random bytes: %w", err)) - } - for i, b := range bytes { - bytes[i] = charset[b%byte(len(charset))] + for { + n, err := io.ReadFull(rand.Reader, r) + if err != nil { + panic("unexpected error happened when reading from bufio.NewReader(crypto/rand.Reader)") + } + if n != len(r) { + panic("partial reads occurred when reading from bufio.NewReader(crypto/rand.Reader)") + } + for _, rb := range r { + if rb > randomStringMaxByte { + // Skip this number to avoid modulo bias. + continue + } + c := randomStringCharset[rb%randomStringCharsetLen] + b[i] = c + i++ + if i == length { + return string(b) + } + } } - return string(bytes) } diff --git a/middleware/util_test.go b/middleware/util_test.go index 7562d4a5f..d0f20bba6 100644 --- a/middleware/util_test.go +++ b/middleware/util_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_matchScheme(t *testing.T) { @@ -117,3 +118,31 @@ func TestRandomString(t *testing.T) { }) } } + +func TestRandomStringBias(t *testing.T) { + t.Parallel() + const slen = 33 + const loop = 100000 + + counts := make(map[rune]int) + var count int64 + + for i := 0; i < loop; i++ { + s := randomString(slen) + require.Equal(t, slen, len(s)) + for _, b := range s { + counts[b]++ + count++ + } + } + + require.Equal(t, randomStringCharsetLen, len(counts)) + + avg := float64(count) / float64(len(counts)) + for k, n := range counts { + diff := float64(n) / avg + if diff < 0.95 || diff > 1.05 { + t.Errorf("Bias on '%c': expected average %f, got %d", k, avg, n) + } + } +} From f32da2eb63458e7a9185071f2c13c35d34ee7168 Mon Sep 17 00:00:00 2001 From: Trim21 Date: Sat, 22 Jul 2023 07:41:32 +0800 Subject: [PATCH 2/6] Update util.go --- middleware/util.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/middleware/util.go b/middleware/util.go index 00ed66b1c..fb87a151a 100644 --- a/middleware/util.go +++ b/middleware/util.go @@ -77,8 +77,7 @@ func randomString(length uint8) string { // Skip this number to avoid modulo bias. continue } - c := randomStringCharset[rb%randomStringCharsetLen] - b[i] = c + b[i] = randomStringCharset[rb%randomStringCharsetLen] i++ if i == length { return string(b) From 68f8af3a5b986f06b41378b7b327d39c6b03ad2d Mon Sep 17 00:00:00 2001 From: Trim21 Date: Sat, 22 Jul 2023 07:48:48 +0800 Subject: [PATCH 3/6] Update util.go --- middleware/util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/middleware/util.go b/middleware/util.go index fb87a151a..54676d8ba 100644 --- a/middleware/util.go +++ b/middleware/util.go @@ -61,7 +61,7 @@ const randomStringMaxByte = 255 - (256 % randomStringCharsetLen) func randomString(length uint8) string { b := make([]byte, length) - r := make([]byte, length+(length/3)) + r := make([]byte, length+(length/4)) // perf: avoid read from rand.Reader many times var i uint8 = 0 for { @@ -74,7 +74,7 @@ func randomString(length uint8) string { } for _, rb := range r { if rb > randomStringMaxByte { - // Skip this number to avoid modulo bias. + // Skip this number to avoid bias. continue } b[i] = randomStringCharset[rb%randomStringCharsetLen] From 1c89a88434b392a71518943ea19d0df8e34f63b8 Mon Sep 17 00:00:00 2001 From: Trim21 Date: Sat, 22 Jul 2023 07:54:33 +0800 Subject: [PATCH 4/6] use pooled buffed random reader --- middleware/util.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/middleware/util.go b/middleware/util.go index 54676d8ba..dbdbdfae6 100644 --- a/middleware/util.go +++ b/middleware/util.go @@ -1,9 +1,11 @@ package middleware import ( + "bufio" "crypto/rand" "io" "strings" + "sync" ) func matchScheme(domain, pattern string) bool { @@ -55,17 +57,24 @@ func matchSubdomain(domain, pattern string) bool { return false } +var randomReaderPool = sync.Pool{New: func() any { + return bufio.NewReader(rand.Reader) +}} + const randomStringCharset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" const randomStringCharsetLen = 52 // len(randomStringCharset) const randomStringMaxByte = 255 - (256 % randomStringCharsetLen) func randomString(length uint8) string { + reader := randomReaderPool.Get().(*bufio.Reader) + defer randomReaderPool.Put(reader) + b := make([]byte, length) r := make([]byte, length+(length/4)) // perf: avoid read from rand.Reader many times var i uint8 = 0 for { - n, err := io.ReadFull(rand.Reader, r) + n, err := io.ReadFull(reader, r) if err != nil { panic("unexpected error happened when reading from bufio.NewReader(crypto/rand.Reader)") } From 4585ec222efde07a2c7a97fb1ad6f0a9b7aef3f4 Mon Sep 17 00:00:00 2001 From: Trim21 Date: Sat, 22 Jul 2023 07:56:50 +0800 Subject: [PATCH 5/6] Update util.go --- middleware/util.go | 1 + 1 file changed, 1 insertion(+) diff --git a/middleware/util.go b/middleware/util.go index dbdbdfae6..aab9990f8 100644 --- a/middleware/util.go +++ b/middleware/util.go @@ -57,6 +57,7 @@ func matchSubdomain(domain, pattern string) bool { return false } +// https://tip.golang.org/doc/go1.19#:~:text=Read%20no%20longer%20buffers%20random%20data%20obtained%20from%20the%20operating%20system%20between%20calls var randomReaderPool = sync.Pool{New: func() any { return bufio.NewReader(rand.Reader) }} From 479045b5707c0c413ae3f8c6603caaa5c6e41b2f Mon Sep 17 00:00:00 2001 From: Trim21 Date: Sat, 22 Jul 2023 08:21:48 +0800 Subject: [PATCH 6/6] Update util.go --- middleware/util.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/middleware/util.go b/middleware/util.go index aab9990f8..0aa0420fc 100644 --- a/middleware/util.go +++ b/middleware/util.go @@ -58,7 +58,7 @@ func matchSubdomain(domain, pattern string) bool { } // https://tip.golang.org/doc/go1.19#:~:text=Read%20no%20longer%20buffers%20random%20data%20obtained%20from%20the%20operating%20system%20between%20calls -var randomReaderPool = sync.Pool{New: func() any { +var randomReaderPool = sync.Pool{New: func() interface{} { return bufio.NewReader(rand.Reader) }} @@ -75,13 +75,10 @@ func randomString(length uint8) string { var i uint8 = 0 for { - n, err := io.ReadFull(reader, r) + _, err := io.ReadFull(reader, r) if err != nil { panic("unexpected error happened when reading from bufio.NewReader(crypto/rand.Reader)") } - if n != len(r) { - panic("partial reads occurred when reading from bufio.NewReader(crypto/rand.Reader)") - } for _, rb := range r { if rb > randomStringMaxByte { // Skip this number to avoid bias.