From 11d153dfaf99deb4a824bfa7c5ea7bc84f92012d Mon Sep 17 00:00:00 2001
From: Atsushi Watanabe <atsushi.w@ieee.org>
Date: Mon, 13 Jul 2020 14:37:03 +0900
Subject: [PATCH] Fix session ID generation

Use crypto random according to
https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-26#section-5.2.1
---
 go.mod       |  5 ++++-
 go.sum       |  2 ++
 jsep.go      | 14 +++++++++++---
 util.go      | 13 ++++++++-----
 util_test.go | 26 ++++++++++++++++++++++++++
 5 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/go.mod b/go.mod
index 87840d2..d697d8e 100644
--- a/go.mod
+++ b/go.mod
@@ -2,4 +2,7 @@ module github.com/pion/sdp/v2
 
 go 1.12
 
-require github.com/stretchr/testify v1.6.1
+require (
+	github.com/pion/randutil v0.1.0
+	github.com/stretchr/testify v1.6.1
+)
diff --git a/go.sum b/go.sum
index afe7890..43316cb 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/pion/randutil v0.1.0 h1:CFG1UdESneORglEsnimhUjf33Rwjubwj6xfiOXBa3mA=
+github.com/pion/randutil v0.1.0/go.mod h1:XcJrSMMbbMRhASFVOlj/5hQial/Y8oH/HVo7TBZq+j8=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
diff --git a/jsep.go b/jsep.go
index a10a8dd..86b70bb 100644
--- a/jsep.go
+++ b/jsep.go
@@ -50,12 +50,20 @@ var extMapURI = map[int]string{
 
 // NewJSEPSessionDescription creates a new SessionDescription with
 // some settings that are required by the JSEP spec.
-func NewJSEPSessionDescription(identity bool) *SessionDescription {
+//
+// Note: Since v2.4.0, session ID has been fixed to use crypto random according to
+//       JSEP spec, so that NewJSEPSessionDescription now returns error as a second
+//       return value.
+func NewJSEPSessionDescription(identity bool) (*SessionDescription, error) {
+	sid, err := newSessionID()
+	if err != nil {
+		return nil, err
+	}
 	d := &SessionDescription{
 		Version: 0,
 		Origin: Origin{
 			Username:       "-",
-			SessionID:      newSessionID(),
+			SessionID:      sid,
 			SessionVersion: uint64(time.Now().Unix()),
 			NetworkType:    "IN",
 			AddressType:    "IP4",
@@ -80,7 +88,7 @@ func NewJSEPSessionDescription(identity bool) *SessionDescription {
 		d.WithPropertyAttribute(AttrKeyIdentity)
 	}
 
-	return d
+	return d, nil
 }
 
 // WithPropertyAttribute adds a property attribute 'a=key' to the session description
diff --git a/util.go b/util.go
index e82e70f..6c79f4d 100644
--- a/util.go
+++ b/util.go
@@ -5,12 +5,11 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"math/rand"
 	"sort"
 	"strconv"
 	"strings"
 
-	"time"
+	"github.com/pion/randutil"
 )
 
 const (
@@ -49,9 +48,13 @@ func (t ConnectionRole) String() string {
 	}
 }
 
-func newSessionID() uint64 {
-	r := rand.New(rand.NewSource(time.Now().UnixNano()))
-	return uint64(r.Uint32()*2) >> 2
+func newSessionID() (uint64, error) {
+	// https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-26#section-5.2.1
+	// Session ID is recommended to be constructed by generating a 64-bit
+	// quantity with the highest bit set to zero and the remaining 63-bits
+	// being cryptographically random.
+	id, err := randutil.CryptoUint64()
+	return id & (^(uint64(1) << 63)), err
 }
 
 // Codec represents a codec
diff --git a/util_test.go b/util_test.go
index df9a0ef..cc3737b 100644
--- a/util_test.go
+++ b/util_test.go
@@ -142,3 +142,29 @@ func TestGetCodecForPayloadType(t *testing.T) {
 		}
 	}
 }
+
+func TestNewSessionID(t *testing.T) {
+	min := uint64(0x7FFFFFFFFFFFFFFF)
+	max := uint64(0)
+	for i := 0; i < 10000; i++ {
+		r, err := newSessionID()
+		if err != nil {
+			t.Fatal(err)
+		}
+		if r > (1<<63)-1 {
+			t.Fatalf("Session ID must be less than 2**64-1, got %d", r)
+		}
+		if r < min {
+			min = r
+		}
+		if r > max {
+			max = r
+		}
+	}
+	if min > 0x1000000000000000 {
+		t.Error("Value around lower boundary was not generated")
+	}
+	if max < 0x7000000000000000 {
+		t.Error("Value around upper boundary was not generated")
+	}
+}