Fix random generators

Seeding random generator each time limits number of generated sequence
to 31-bits, and caused collision.
Use global random generator seeded by crypto grade random.

Use crypto/rand for cryptographic values,
and math/rand for unique identifier.
- Use UUIDv4 for mDNS name
- Use crypto/rand for ICE pwd and user fragment
- Use properly seeded math/rand for UDP port,
  tie breaker and candidate ID

agent.go

@@ -4,7 +4,6 @@ package ice
 
 import (
 	"context"
-	"math/rand"
 	"net"
 	"strings"
 	"sync"
@@ -218,7 +217,7 @@ func NewAgent(config *AgentConfig) (*Agent, error) {
 	startedCtx, startedFn := context.WithCancel(context.Background())
 
 	a := &Agent{
-		tieBreaker:       rand.New(rand.NewSource(time.Now().UnixNano())).Uint64(),
+		tieBreaker:       globalMathRandomGenerator.Uint64(),
 		lite:             config.Lite,
 		gatheringState:   GatheringStateNew,
 		connectionState:  ConnectionStateNew,
@@ -991,10 +990,18 @@ func (a *Agent) SetRemoteCredentials(remoteUfrag, remotePwd string) error {
 // a user must then call GatherCandidates explicitly to start generating new ones
 func (a *Agent) Restart(ufrag, pwd string) error {
 	if ufrag == "" {
-		ufrag = randSeq(16)
+		var err error
+		ufrag, err = generateUFrag()
+		if err != nil {
+			return err
+		}
 	}
 	if pwd == "" {
-		pwd = randSeq(32)
+		var err error
+		pwd, err = generatePwd()
+		if err != nil {
+			return err
+		}
 	}
 
 	if len([]rune(ufrag))*8 < 24 {

candidate_host.go

@@ -26,11 +26,7 @@ func NewCandidateHost(config *CandidateHostConfig) (*CandidateHost, error) {
 	candidateID := config.CandidateID
 
 	if candidateID == "" {
-		var err error
-		candidateID, err = generateCandidateID()
-		if err != nil {
-			return nil, err
-		}
+		candidateID = globalCandidateIDGenerator.Generate()
 	}
 
 	c := &CandidateHost{

candidate_peer_reflexive.go

@@ -34,11 +34,7 @@ func NewCandidatePeerReflexive(config *CandidatePeerReflexiveConfig) (*Candidate
 
 	candidateID := config.CandidateID
 	if candidateID == "" {
-		var err error
-		candidateID, err = generateCandidateID()
-		if err != nil {
-			return nil, err
-		}
+		candidateID = globalCandidateIDGenerator.Generate()
 	}
 
 	return &CandidatePeerReflexive{

candidate_relay.go

@@ -28,11 +28,7 @@ func NewCandidateRelay(config *CandidateRelayConfig) (*CandidateRelay, error) {
 	candidateID := config.CandidateID
 
 	if candidateID == "" {
-		var err error
-		candidateID, err = generateCandidateID()
-		if err != nil {
-			return nil, err
-		}
+		candidateID = globalCandidateIDGenerator.Generate()
 	}
 
 	ip := net.ParseIP(config.Address)

candidate_server_reflexive.go

@@ -1,3 +1,5 @@
+// Package ice ...
+//nolint:dupl
 package ice
 
 import "net"
@@ -32,10 +34,7 @@ func NewCandidateServerReflexive(config *CandidateServerReflexiveConfig) (*Candi
 
 	candidateID := config.CandidateID
 	if candidateID == "" {
-		candidateID, err = generateCandidateID()
-		if err != nil {
-			return nil, err
-		}
+		candidateID = globalCandidateIDGenerator.Generate()
 	}
 
 	return &CandidateServerReflexive{

go.mod

@@ -3,6 +3,7 @@ module github.com/pion/ice/v2
 go 1.13
 
 require (
+	github.com/google/uuid v1.1.1
 	github.com/pion/dtls/v2 v2.0.1
 	github.com/pion/logging v0.2.2
 	github.com/pion/mdns v0.0.4

go.sum

@@ -1,5 +1,7 @@
 github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
+github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/pion/dtls/v2 v2.0.1 h1:ddE7+V0faYRbyh4uPsRZ2vLdRrjVZn+wmCfI7jlBfaA=
 github.com/pion/dtls/v2 v2.0.1/go.mod h1:uMQkz2W0cSqY00xav7WByQ4Hb+18xeQh2oH2fRezr5U=
 github.com/pion/logging v0.2.2 h1:M9+AIj/+pxNsDfAT64+MAVgJO0rsyLnoJKCqf//DoeY=

mdns.go

@@ -3,6 +3,7 @@ package ice
 import (
 	"net"
 
+	"github.com/google/uuid"
 	"github.com/pion/logging"
 	"github.com/pion/mdns"
 	"golang.org/x/net/ipv4"
@@ -24,7 +25,10 @@ const (
 )
 
 func generateMulticastDNSName() (string, error) {
-	return generateRandString("", ".local")
+	// https://tools.ietf.org/id/draft-ietf-rtcweb-mdns-ice-candidates-02.html#gathering
+	// The unique name MUST consist of a version 4 UUID as defined in [RFC4122], followed by “.local”.
+	u, err := uuid.NewRandom()
+	return u.String() + ".local", err
 }
 
 func createMulticastDNS(mDNSMode MulticastDNSMode, mDNSName string, log logging.LeveledLogger) (*mdns.Conn, MulticastDNSMode, error) {

mdns_test.go

@@ -4,6 +4,7 @@ package ice
 
 import (
 	"context"
+	"regexp"
 	"testing"
 	"time"
 
@@ -131,3 +132,17 @@ func TestMulticastDNSStaticHostName(t *testing.T) {
 	<-correctHostName.Done()
 	assert.NoError(t, agent.Close())
 }
+
+func TestGenerateMulticastDNSName(t *testing.T) {
+	name, err := generateMulticastDNSName()
+	if err != nil {
+		t.Fatal(err)
+	}
+	isMDNSName := regexp.MustCompile(
+		`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}.local+$`,
+	).MatchString
+
+	if !isMDNSName(name) {
+		t.Fatalf("mDNS name must be UUID v4 + \".local\" suffix, got %s", name)
+	}
+}

rand.go

@@ -0,0 +1,111 @@
+package ice
+
+import (
+	crand "crypto/rand"
+	"encoding/binary"
+	"math/big"
+	mrand "math/rand" // used for non-crypto unique ID and random port selection
+	"sync"
+	"time"
+)
+
+const (
+	runesAlpha                 = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
+	runesDigit                 = "0123456789"
+	runesCandidateIDFoundation = runesAlpha + runesDigit + "+/"
+
+	lenUFrag = 16
+	lenPwd   = 32
+)
+
+// Seeding random generator each time limits number of generated sequence to 31-bits,
+// and causes collision on low time accuracy environments.
+// Use global random generator seeded by crypto grade random.
+var globalMathRandomGenerator = newMathRandomGenerator()
+var globalCandidateIDGenerator = candidateIDGenerator{globalMathRandomGenerator}
+
+// mathRandomGenerator is a random generator for non-crypto usage.
+type mathRandomGenerator struct {
+	r  *mrand.Rand
+	mu sync.Mutex
+}
+
+func newMathRandomGenerator() *mathRandomGenerator {
+	var seed int64
+	if err := binary.Read(crand.Reader, binary.LittleEndian, &seed); err != nil {
+		// crypto/rand is unavailable. Fallback to seed by time.
+		seed = time.Now().UnixNano()
+	}
+
+	return &mathRandomGenerator{r: mrand.New(mrand.NewSource(seed))}
+}
+
+func (g *mathRandomGenerator) Intn(n int) int {
+	g.mu.Lock()
+	v := g.r.Intn(n)
+	g.mu.Unlock()
+	return v
+}
+
+func (g *mathRandomGenerator) Uint64() uint64 {
+	g.mu.Lock()
+	v := g.r.Uint64()
+	g.mu.Unlock()
+	return v
+}
+
+func (g *mathRandomGenerator) GenerateString(n int, runes string) string {
+	letters := []rune(runes)
+	b := make([]rune, n)
+	for i := range b {
+		b[i] = letters[g.Intn(len(letters))]
+	}
+	return string(b)
+}
+
+// candidateIDGenerator is a random candidate ID generator.
+// Candidate ID is used in SDP and always shared to the other peer.
+// It doesn't require cryptographic random.
+type candidateIDGenerator struct {
+	*mathRandomGenerator
+}
+
+func newCandidateIDGenerator() *candidateIDGenerator {
+	return &candidateIDGenerator{
+		newMathRandomGenerator(),
+	}
+}
+
+func (g *candidateIDGenerator) Generate() string {
+	// https://tools.ietf.org/html/rfc5245#section-15.1
+	// candidate-id = "candidate" ":" foundation
+	// foundation   = 1*32ice-char
+	// ice-char     = ALPHA / DIGIT / "+" / "/"
+	return "candidate:" + g.mathRandomGenerator.GenerateString(32, runesCandidateIDFoundation)
+}
+
+// generateCryptoRandomString generates a random string for crypto usage.
+func generateCryptoRandomString(n int, runes string) (string, error) {
+	letters := []rune(runes)
+	b := make([]rune, n)
+	for i := range b {
+		v, err := crand.Int(crand.Reader, big.NewInt(int64(len(letters))))
+		if err != nil {
+			return "", err
+		}
+		b[i] = letters[v.Int64()]
+	}
+	return string(b), nil
+}
+
+// generatePwd generates ICE pwd.
+// This internally uses generateCryptoRandomString.
+func generatePwd() (string, error) {
+	return generateCryptoRandomString(lenPwd, runesAlpha)
+}
+
+// generateUFrag generates ICE user fragment.
+// This internally uses generateCryptoRandomString.
+func generateUFrag() (string, error) {
+	return generateCryptoRandomString(lenUFrag, runesAlpha)
+}

rand_test.go

@@ -0,0 +1,110 @@
+package ice
+
+import (
+	"regexp"
+	"sync"
+	"testing"
+)
+
+func TestMathRandomGenerator(t *testing.T) {
+	g := newMathRandomGenerator()
+	isLetter := regexp.MustCompile(`^[a-zA-Z]+$`).MatchString
+
+	for i := 0; i < 10000; i++ {
+		s := g.GenerateString(10, runesAlpha)
+		if len(s) != 10 {
+			t.Error("Generator returned invalid length")
+		}
+		if !isLetter(s) {
+			t.Errorf("Generator returned unexpected character: %s", s)
+		}
+	}
+}
+
+func TestCryptoRandomGenerator(t *testing.T) {
+	isLetter := regexp.MustCompile(`^[a-zA-Z]+$`).MatchString
+
+	for i := 0; i < 10000; i++ {
+		s, err := generateCryptoRandomString(10, runesAlpha)
+		if err != nil {
+			t.Error(err)
+		}
+		if len(s) != 10 {
+			t.Error("Generator returned invalid length")
+		}
+		if !isLetter(s) {
+			t.Errorf("Generator returned unexpected character: %s", s)
+		}
+	}
+}
+
+func TestRandomGeneratorCollision(t *testing.T) {
+	candidateIDGen := newCandidateIDGenerator()
+
+	testCases := map[string]struct {
+		gen func(t *testing.T) string
+	}{
+		"CandidateID": {
+			gen: func(t *testing.T) string {
+				return candidateIDGen.Generate()
+			},
+		},
+		"PWD": {
+			gen: func(t *testing.T) string {
+				s, err := generatePwd()
+				if err != nil {
+					t.Fatal(err)
+				}
+				return s
+			},
+		},
+		"Ufrag": {
+			gen: func(t *testing.T) string {
+				s, err := generateUFrag()
+				if err != nil {
+					t.Fatal(err)
+				}
+				return s
+			},
+		},
+	}
+
+	const N = 1000
+	const iteration = 100
+
+	for name, testCase := range testCases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			for iter := 0; iter < iteration; iter++ {
+				var wg sync.WaitGroup
+				var mu sync.Mutex
+
+				rands := make([]string, 0, N)
+
+				for i := 0; i < N; i++ {
+					wg.Add(1)
+					go func() {
+						r := testCase.gen(t)
+						mu.Lock()
+						rands = append(rands, r)
+						mu.Unlock()
+						wg.Done()
+					}()
+				}
+				wg.Wait()
+
+				if len(rands) != N {
+					t.Fatal("Failed to generate randoms")
+				}
+
+				for i := 0; i < N; i++ {
+					for j := i + 1; j < N; j++ {
+						if rands[i] == rands[j] {
+							t.Fatalf("generateRandString caused collision: %s == %s", rands[i], rands[j])
+						}
+					}
+				}
+			}
+		})
+	}
+}

util.go

@@ -2,7 +2,6 @@ package ice
 
 import (
 	"fmt"
-	"math/rand"
 	"net"
 	"sync/atomic"
 	"time"
@@ -44,17 +43,6 @@ func isZeros(ip net.IP) bool {
 	return true
 }
 
-// RandSeq generates a random alpha numeric sequence of the requested length
-func randSeq(n int) string {
-	r := rand.New(rand.NewSource(time.Now().UnixNano()))
-	letters := []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")
-	b := make([]rune, n)
-	for i := range b {
-		b[i] = letters[r.Intn(len(letters))]
-	}
-	return string(b)
-}
-
 func parseAddr(in net.Addr) (net.IP, int, NetworkType, bool) {
 	switch addr := in.(type) {
 	case *net.UDPAddr:
@@ -79,19 +67,6 @@ func addrEqual(a, b net.Addr) bool {
 	return aType == bType && aIP.Equal(bIP) && aPort == bPort
 }
 
-func generateCandidateID() (string, error) {
-	return generateRandString("candidate:", "")
-}
-
-func generateRandString(prefix, sufix string) (string, error) {
-	b := make([]byte, 16)
-	if _, err := rand.New(rand.NewSource(time.Now().UnixNano())).Read(b); err != nil {
-		return "", err
-	}
-
-	return fmt.Sprintf("%s%X-%X-%X-%X-%X%s", prefix, b[0:4], b[4:6], b[6:8], b[8:10], b[10:], sufix), nil
-}
-
 // getXORMappedAddr initiates a stun requests to serverAddr using conn, reads the response and returns
 // the XORMappedAddress returned by the stun server.
 //
@@ -227,7 +202,7 @@ func listenUDPInPortRange(vnet *vnet.Net, log logging.LeveledLogger, portMax, po
 		return nil, ErrPort
 	}
 
-	portStart := rand.Intn(j-i+1) + i
+	portStart := globalMathRandomGenerator.Intn(j-i+1) + i
 	portCurrent := portStart
 	for {
 		laddr = &net.UDPAddr{IP: laddr.IP, Port: portCurrent}

util_test.go

@@ -1,17 +0,0 @@
-package ice
-
-import (
-	"regexp"
-	"testing"
-)
-
-func TestRandSeq(t *testing.T) {
-	if len(randSeq(10)) != 10 {
-		t.Errorf("randSeq return invalid length")
-	}
-
-	var isLetter = regexp.MustCompile(`^[a-zA-Z]+$`).MatchString
-	if !isLetter(randSeq(10)) {
-		t.Errorf("randSeq should be AlphaNumeric only")
-	}
-}