From e670416dccb79cad99d2d317a2524aba1a99f3d2 Mon Sep 17 00:00:00 2001 From: Atsushi Watanabe Date: Sat, 27 Jun 2020 14:13:28 +0900 Subject: [PATCH] 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 | 15 +++-- candidate_host.go | 6 +- candidate_peer_reflexive.go | 6 +- candidate_relay.go | 6 +- candidate_server_reflexive.go | 7 +-- go.mod | 1 + go.sum | 2 + mdns.go | 6 +- mdns_test.go | 15 +++++ rand.go | 111 ++++++++++++++++++++++++++++++++++ rand_test.go | 110 +++++++++++++++++++++++++++++++++ util.go | 27 +-------- util_test.go | 17 ------ 13 files changed, 262 insertions(+), 67 deletions(-) create mode 100644 rand.go create mode 100644 rand_test.go delete mode 100644 util_test.go diff --git a/agent.go b/agent.go index 0ce004c..948d310 100644 --- a/agent.go +++ b/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 { diff --git a/candidate_host.go b/candidate_host.go index 6a80198..93280f5 100644 --- a/candidate_host.go +++ b/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{ diff --git a/candidate_peer_reflexive.go b/candidate_peer_reflexive.go index b9417c2..c2c04d3 100644 --- a/candidate_peer_reflexive.go +++ b/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{ diff --git a/candidate_relay.go b/candidate_relay.go index 0cc01ff..8865872 100644 --- a/candidate_relay.go +++ b/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) diff --git a/candidate_server_reflexive.go b/candidate_server_reflexive.go index e50e5e3..d7563f7 100644 --- a/candidate_server_reflexive.go +++ b/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{ diff --git a/go.mod b/go.mod index 196a98f..177c9ab 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index c5e290c..ec67e4e 100644 --- a/go.sum +++ b/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= diff --git a/mdns.go b/mdns.go index 0895355..5a431d1 100644 --- a/mdns.go +++ b/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) { diff --git a/mdns_test.go b/mdns_test.go index e984dce..cb13703 100644 --- a/mdns_test.go +++ b/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) + } +} diff --git a/rand.go b/rand.go new file mode 100644 index 0000000..8455b07 --- /dev/null +++ b/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) +} diff --git a/rand_test.go b/rand_test.go new file mode 100644 index 0000000..63d49f1 --- /dev/null +++ b/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]) + } + } + } + } + }) + } +} diff --git a/util.go b/util.go index d39be68..f9d6cd3 100644 --- a/util.go +++ b/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} diff --git a/util_test.go b/util_test.go deleted file mode 100644 index 83f1ca9..0000000 --- a/util_test.go +++ /dev/null @@ -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") - } -}