Repository: pion/sctp
Pull request #135 (2020-07-13 05:33:06)

Fix randoms
- Use seeded mathematical random where uniqueness is needed
  but not required to be cryptographic
- Use crypto random where used as a password or credential

### Reference issue
Related https://github.com/pion/ice/issues/203


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=h1) Report
> Merging [#135](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=desc) into [master](https://codecov.io/gh/pion/sctp/commit/87eef637d5d71e37f6280345774eeda0a112f2f2&el=desc) will **decrease** coverage by `0.23%`.
> The diff coverage is `60.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/sctp/pull/135/graphs/tree.svg?width=650&height=150&src=pr&token=AB8S3U6xR9)](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   78.94%   78.71%   -0.24%     
==========================================
  Files          43       43              
  Lines        2798     2795       -3     
==========================================
- Hits         2209     2200       -9     
- Misses        454      457       +3     
- Partials      135      138       +3     
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `78.71% <60.00%> (-0.24%)` | :arrow_down: |
| #wasm | `69.73% <60.00%> (-0.18%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [association.go](https://codecov.io/gh/pion/sctp/pull/135/diff?src=pr&el=tree#diff-YXNzb2NpYXRpb24uZ28=) | `82.73% <60.00%> (-0.36%)` | :arrow_down: |
| [param\_state\_cookie.go](https://codecov.io/gh/pion/sctp/pull/135/diff?src=pr&el=tree#diff-cGFyYW1fc3RhdGVfY29va2llLmdv) | `70.00% <60.00%> (-12.61%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=footer). Last update [87eef63...bfd9941](https://codecov.io/gh/pion/sctp/pull/135?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/association.go b/association.go
index 8130834..c8ca49e 100644
--- a/association.go
+++ b/association.go
@@ -5,16 +5,19 @@ import (
 	"fmt"
 	"io"
 	"math"
-	"math/rand"
 	"net"
 	"sync"
 	"sync/atomic"
 	"time"
 
 	"github.com/pion/logging"
+	"github.com/pion/randutil"
 	"github.com/pkg/errors"
 )
 
+// Use global random generator to properly seed by crypto grade random.
+var globalMathRandomGenerator = randutil.NewMathRandomGenerator()
+
 const (
 	receiveMTU          uint32 = 8192 // MTU for inbound packet (from DTLS)
 	initialMTU          uint32 = 1228 // initial MTU for outgoing packets (to DTLS)
@@ -229,9 +232,6 @@ func Client(config Config) (*Association, error) {
 }
 
 func createAssociation(config Config) *Association {
-	rs := rand.NewSource(time.Now().UnixNano())
-	r := rand.New(rs)
-
 	var maxReceiveBufferSize uint32
 	if config.MaxReceiveBufferSize == 0 {
 		maxReceiveBufferSize = initialRecvBufSize
@@ -239,7 +239,7 @@ func createAssociation(config Config) *Association {
 		maxReceiveBufferSize = config.MaxReceiveBufferSize
 	}
 
-	tsn := r.Uint32()
+	tsn := globalMathRandomGenerator.Uint32()
 	a := &Association{
 		netConn:                 config.NetConn,
 		maxReceiveBufferSize:    maxReceiveBufferSize,
@@ -251,7 +251,7 @@ func createAssociation(config Config) *Association {
 		controlQueue:            newControlQueue(),
 		mtu:                     initialMTU,
 		maxPayloadSize:          initialMTU - (commonHeaderSize + dataChunkHeaderSize),
-		myVerificationTag:       r.Uint32(),
+		myVerificationTag:       globalMathRandomGenerator.Uint32(),
 		myNextTSN:               tsn,
 		myNextRSN:               tsn,
 		minTSN2MeasureRTT:       tsn,
@@ -869,7 +869,10 @@ func (a *Association) handleInit(p *packet, i *chunkInit) ([]*packet, error) {
 	initAck.advertisedReceiverWindowCredit = a.maxReceiveBufferSize
 
 	if a.myCookie == nil {
-		a.myCookie = newRandomStateCookie()
+		var err error
+		if a.myCookie, err = newRandomStateCookie(); err != nil {
+			return nil, err
+		}
 	}
 
 	initAck.params = []param{a.myCookie}
diff --git a/go.mod b/go.mod
index 34221b7..cc2f41e 100644
--- a/go.mod
+++ b/go.mod
@@ -4,6 +4,7 @@ require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/kr/pretty v0.1.0 // indirect
 	github.com/pion/logging v0.2.2
+	github.com/pion/randutil v0.0.0
 	github.com/pion/transport v0.10.1
 	github.com/pkg/errors v0.9.1
 	github.com/stretchr/testify v1.6.1
diff --git a/go.sum b/go.sum
index e385f5a..3d3c363 100644
--- a/go.sum
+++ b/go.sum
@@ -9,6 +9,8 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/pion/logging v0.2.2 h1:M9+AIj/+pxNsDfAT64+MAVgJO0rsyLnoJKCqf//DoeY=
 github.com/pion/logging v0.2.2/go.mod h1:k0/tDVsRCX2Mb2ZEmTqNa7CWsQPc+YYCB7Q+5pahoms=
+github.com/pion/randutil v0.0.0 h1:aLWLVhTG2jzoD25F0OlW6nXvXrjoGwiXq2Sz7j7NzL0=
+github.com/pion/randutil v0.0.0/go.mod h1:XcJrSMMbbMRhASFVOlj/5hQial/Y8oH/HVo7TBZq+j8=
 github.com/pion/transport v0.10.1 h1:2W+yJT+0mOQ160ThZYUx5Zp2skzshiNgxrNE9GUfhJM=
 github.com/pion/transport v0.10.1/go.mod h1:PBis1stIILMiis0PewDw91WJeLJkyIMcEk+DwKOzf4A=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
diff --git a/param_state_cookie.go b/param_state_cookie.go
index 4476501..9681267 100644
--- a/param_state_cookie.go
+++ b/param_state_cookie.go
@@ -1,10 +1,8 @@
 package sctp
 
 import (
-	"encoding/binary"
+	"crypto/rand"
 	"fmt"
-	"math/rand"
-	"time"
 )
 
 type paramStateCookie struct {
@@ -12,21 +10,19 @@ type paramStateCookie struct {
 	cookie []byte
 }
 
-func newRandomStateCookie() *paramStateCookie {
-	rs := rand.NewSource(time.Now().UnixNano())
-	r := rand.New(rs)
+func newRandomStateCookie() (*paramStateCookie, error) {
 	randCookie := make([]byte, 32)
-	i := 0
-	for i < 4 {
-		binary.BigEndian.PutUint64(randCookie[i*4:], r.Uint64())
-		i++
+	_, err := rand.Read(randCookie)
+	// crypto/rand.Read returns n == len(b) if and only if err == nil.
+	if err != nil {
+		return nil, err
 	}
 
 	s := &paramStateCookie{
 		cookie: randCookie,
 	}
 
-	return s
+	return s, nil
 }
 
 func (s *paramStateCookie) marshal() ([]byte, error) {


Test patch:
diff --git a/chunk_test.go b/chunk_test.go
index 40f9fa7..bb44c12 100644
--- a/chunk_test.go
+++ b/chunk_test.go
@@ -100,7 +100,11 @@ func TestInitMarshalUnmarshal(t *testing.T) {
 	initAck.numInboundStreams = 1
 	initAck.initiateTag = 123
 	initAck.advertisedReceiverWindowCredit = 1024
-	initAck.params = []param{newRandomStateCookie()}
+	cookie, errRand := newRandomStateCookie()
+	if errRand != nil {
+		t.Fatal(errors.Wrap(errRand, "Failed to generate random state cookie"))
+	}
+	initAck.params = []param{cookie}
 
 	p.chunks = []chunk{initAck}
 	rawPkt, err := p.marshal()