Repository: labstack/echo
Pull request #2492 (2023-07-21 23:44:46)

fix(sec): `randomString` bias
security issue added by #2490

`len("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz")==52`, and `256 = 52 * 4 + 48`, so the possibility of each characters generated by `randomString` is not equal.

A-Za-v: 5/256
wxyz: 4/256

also perfermance improve, in newer (>=1.19) go version, `rand.Reader` is not buffered any more, so it's suggested to wrap `rand.Reader` with bufio if the data read from reader is small.

https://tip.golang.org/doc/go1.19#:~:text=Read%20no%20longer%20buffers%20random%20data%20obtained%20from%20the%20operating%20system%20between%20calls

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/labstack/echo/pull/2492?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) Report
Patch coverage: **`90.47`**% and project coverage change: **`+0.03`** :tada:
> Comparison is base [(`626f13e`)](https://app.codecov.io/gh/labstack/echo/commit/626f13e33830665e08d9d40e333dd13d9de8e672?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) 92.77% compared to head [(`479045b`)](https://app.codecov.io/gh/labstack/echo/pull/2492?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) 92.81%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #2492      +/-   ##
==========================================
+ Coverage   92.77%   92.81%   +0.03%     
==========================================
  Files          39       39              
  Lines        4635     4646      +11     
==========================================
+ Hits         4300     4312      +12     
+ Misses        243      242       -1     
  Partials       92       92              
```


| [Impacted Files](https://app.codecov.io/gh/labstack/echo/pull/2492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) | Coverage Δ | |
|---|---|---|
| [middleware/util.go](https://app.codecov.io/gh/labstack/echo/pull/2492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack#diff-bWlkZGxld2FyZS91dGlsLmdv) | `90.76% <90.47%> (+3.73%)` | :arrow_up: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/labstack/echo/pull/2492?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).


Patch:
diff --git a/middleware/util.go b/middleware/util.go
index aa34d78..0aa0420 100644
--- a/middleware/util.go
+++ b/middleware/util.go
@@ -1,9 +1,11 @@
 package middleware
 
 import (
+	"bufio"
 	"crypto/rand"
-	"fmt"
+	"io"
 	"strings"
+	"sync"
 )
 
 func matchScheme(domain, pattern string) bool {
@@ -55,17 +57,38 @@ 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() interface{} {
+	return bufio.NewReader(rand.Reader)
+}}
+
+const randomStringCharset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
+const randomStringCharsetLen = 52 // len(randomStringCharset)
+const randomStringMaxByte = 255 - (256 % randomStringCharsetLen)
+
 func randomString(length uint8) string {
-	charset := "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
+	reader := randomReaderPool.Get().(*bufio.Reader)
+	defer randomReaderPool.Put(reader)
 
-	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))]
+	b := make([]byte, length)
+	r := make([]byte, length+(length/4)) // perf: avoid read from rand.Reader many times
+	var i uint8 = 0
+
+	for {
+		_, err := io.ReadFull(reader, r)
+		if err != nil {
+			panic("unexpected error happened when reading from bufio.NewReader(crypto/rand.Reader)")
+		}
+		for _, rb := range r {
+			if rb > randomStringMaxByte {
+				// Skip this number to avoid bias.
+				continue
+			}
+			b[i] = randomStringCharset[rb%randomStringCharsetLen]
+			i++
+			if i == length {
+				return string(b)
+			}
+		}
 	}
-	return string(bytes)
 }


Test patch:
diff --git a/middleware/util_test.go b/middleware/util_test.go
index 7562d4a..d0f20bb 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)
+		}
+	}
+}