KS-IOF-F-02 Security audit. Collision of Hash Values: fix size of len…

…gth parts (#16)

* KS-IOF-F-02 Security audit. Collision of Hash Values: fix size of length parts

* revert Makefile change

* KS-IOF-F-02 Security audit. Collision of Hash Values: use common_test package in hash tests

* KS-IOF-F-02 Security audit. Collision of Hash Values: fix case of Hash(-a) != Hash(a), add tests

* KS-IOF-F-02 Security audit. Collision of Hash Values: fix race condition in Mul

* failing test fix

common/hash.go

@@ -10,8 +10,8 @@ import (
 	"crypto"
 	_ "crypto/sha512"
 	"encoding/binary"
+
 	big "github.com/binance-chain/tss-lib/common/int"
-	"strconv"
 )
 
 const (
@@ -29,20 +29,22 @@ func SHA512_256(in ...[]byte) []byte {
 	}
 	bzSize := 0
 	// prevent hash collisions with this prefix containing the block count
-	inLenBz := make([]byte, 64/8)
+	inLenBz := make([]byte, 8) // 64-bits
 	// converting between int and uint64 doesn't change the sign bit, but it may be interpreted as a larger value.
 	// this prefix is never read/interpreted, so that doesn't matter.
 	binary.LittleEndian.PutUint64(inLenBz, uint64(inLen))
 	for _, bz := range in {
 		bzSize += len(bz)
 	}
-	data = make([]byte, 0, len(inLenBz)+bzSize+inLen)
+	dataCap := len(inLenBz) + bzSize + inLen + (inLen * 8)
+	data = make([]byte, 0, dataCap)
 	data = append(data, inLenBz...)
 	for _, bz := range in {
 		data = append(data, bz...)
 		data = append(data, hashInputDelimiter) // safety delimiter
-		l := []byte(strconv.Itoa(len(bz)))
-		data = append(data, l...) // Security audit: length of each byte buffer should be added after
+		dataLen := make([]byte, 8)              // 64-bits
+		binary.LittleEndian.PutUint64(dataLen, uint64(len(bz)))
+		data = append(data, dataLen...) // Security audit: length of each byte buffer should be added after
 		// each security delimiters in order to enforce proper domain separation
 	}
 	// n < len(data) or an error will never happen.
@@ -63,7 +65,7 @@ func SHA512_256i(in ...*big.Int) *big.Int {
 	}
 	bzSize := 0
 	// prevent hash collisions with this prefix containing the block count
-	inLenBz := make([]byte, 64/8)
+	inLenBz := make([]byte, 8) // 64-bits
 	// converting between int and uint64 doesn't change the sign bit, but it may be interpreted as a larger value.
 	// this prefix is never read/interpreted, so that doesn't matter.
 	binary.LittleEndian.PutUint64(inLenBz, uint64(inLen))
@@ -72,13 +74,15 @@ func SHA512_256i(in ...*big.Int) *big.Int {
 		ptrs[i] = append(n.Bytes(), byte(n.Sign()))
 		bzSize += len(ptrs[i])
 	}
-	data = make([]byte, 0, len(inLenBz)+bzSize+inLen)
+	dataCap := len(inLenBz) + bzSize + inLen + (inLen * 8)
+	data = make([]byte, 0, dataCap)
 	data = append(data, inLenBz...)
 	for i := range in {
 		data = append(data, ptrs[i]...)
 		data = append(data, hashInputDelimiter) // safety delimiter
-		l := []byte(strconv.Itoa(len(ptrs[i])))
-		data = append(data, l...) // Security audit: length of each byte buffer should be added after
+		dataLen := make([]byte, 8)              // 64-bits
+		binary.LittleEndian.PutUint64(dataLen, uint64(len(ptrs[i])))
+		data = append(data, dataLen...) // Security audit: length of each byte buffer should be added after
 		// each security delimiters in order to enforce proper domain separation
 	}
 	// n < len(data) or an error will never happen.

common/hash_test.go

@@ -0,0 +1,140 @@
+package common_test
+
+import (
+	"testing"
+
+	. "github.com/binance-chain/tss-lib/common"
+	big "github.com/binance-chain/tss-lib/common/int"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSHA512_256(t *testing.T) {
+	input := [][]byte{[]byte("abc"), []byte("def"), []byte("ghi")}
+	input2 := [][]byte{[]byte("abc"), []byte("def"), []byte("gh")}
+	type args struct {
+		in [][]byte
+	}
+	tests := []struct {
+		name     string
+		args     args
+		want     []byte
+		wantDiff bool
+		wantLen  int
+	}{{
+		name:    "same inputs produce the same hash",
+		args:    args{input},
+		want:    SHA512_256(input...),
+		wantLen: 256 / 8,
+	}, {
+		name:     "different inputs produce a differing hash",
+		args:     args{input2},
+		want:     SHA512_256(input...),
+		wantDiff: true,
+		wantLen:  256 / 8,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := SHA512_256(tt.args.in...)
+			if tt.wantDiff {
+				if !assert.NotEqualf(t, tt.want, got, "SHA512_256(%v)", tt.args.in) {
+					t.Errorf("SHA512_256() = %v, do not want %v", got, tt.want)
+				}
+			} else {
+				if !assert.Equalf(t, tt.want, got, "SHA512_256(%v)", tt.args.in) {
+					t.Errorf("SHA512_256() = %v, want %v", got, tt.want)
+				}
+			}
+			if tt.wantLen != len(got) {
+				t.Errorf("SHA512_256() = bitlen %d, want %d", len(got), tt.wantLen)
+			}
+		})
+	}
+}
+
+func TestSHA512_256i(t *testing.T) {
+	input := ByteSlicesToBigInts([][]byte{[]byte("abc"), []byte("def"), []byte("ghi")})
+	input2 := ByteSlicesToBigInts([][]byte{[]byte("abc"), []byte("def"), []byte("gh")})
+	input3 := new(big.Int).SetBytes([]byte("abc"))
+	t.Logf("%d", input3.Int64())
+	t.Logf("%d", new(big.Int).Neg(input3).Int64())
+	type args struct {
+		in []*big.Int
+	}
+	tests := []struct {
+		name     string
+		args     args
+		want     *big.Int
+		wantDiff bool
+	}{{
+		name: "same inputs produce the same hash",
+		args: args{input},
+		want: SHA512_256i(input...),
+	}, {
+		name:     "different inputs produce a differing hash",
+		args:     args{input2},
+		want:     SHA512_256i(input...),
+		wantDiff: true,
+	}, {
+		name:     "different inputs produce a differing hash: Hash(-a) != Hash(a)",
+		args:     args{[]*big.Int{new(big.Int).Neg(input3)}},
+		want:     SHA512_256i(input3),
+		wantDiff: true,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := SHA512_256i(tt.args.in...)
+			if tt.wantDiff {
+				if !assert.NotEqualf(t, tt.want, got, "SHA512_256i(%v)", tt.args.in) {
+					t.Errorf("SHA512_256i() = %v, do not want %v", got, tt.want)
+				}
+			} else {
+				if !assert.Equalf(t, tt.want, got, "SHA512_256i(%v)", tt.args.in) {
+					t.Errorf("SHA512_256i() = %v, want %v", got, tt.want)
+				}
+			}
+		})
+	}
+}
+
+func TestSHA512_256iOne(t *testing.T) {
+	input := new(big.Int).SetBytes([]byte("abc"))
+	input2 := new(big.Int).SetBytes([]byte("ab"))
+	input3 := new(big.Int).SetBytes([]byte("cd"))
+	type args struct {
+		in *big.Int
+	}
+	tests := []struct {
+		name     string
+		args     args
+		want     *big.Int
+		wantDiff bool
+	}{{
+		name: "same inputs produce the same hash",
+		args: args{input},
+		want: SHA512_256iOne(input),
+	}, {
+		name:     "different inputs produce a differing hash",
+		args:     args{input2},
+		want:     SHA512_256iOne(input),
+		wantDiff: true,
+	}, {
+		name:     "different inputs produce a differing hash: Hash(-a) != Hash(a)",
+		args:     args{new(big.Int).Neg(input3)},
+		want:     SHA512_256i(input3),
+		wantDiff: true,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := SHA512_256iOne(tt.args.in)
+			if tt.wantDiff {
+				if !assert.NotEqualf(t, tt.want, got, "SHA512_256iOne(%v)", tt.args.in) {
+					t.Errorf("SHA512_256iOne() = %v, do not want %v", got, tt.want)
+				}
+			} else {
+				if !assert.Equalf(t, tt.want, got, "SHA512_256iOne(%v)", tt.args.in) {
+					t.Errorf("SHA512_256iOne() = %v, want %v", got, tt.want)
+				}
+			}
+		})
+	}
+}

common/int/bigint.go

@@ -79,8 +79,9 @@ func (z *Int) Clone() *Int {
 	z.ensureInitialized()
 	z.mutex.Lock()
 	defer z.mutex.Unlock()
-	z.i = z.i.Clone()
-	return z
+	cloned := new(Int)
+	cloned.i = z.i.Clone()
+	return cloned
 }
 func (z *Int) Resize(cap int) *Int {
 	z.ensureInitialized()
@@ -127,7 +128,8 @@ func (z *Int) Neg(x *Int) *Int {
 	x.ensureInitialized()
 	z.mutex.Lock()
 	defer z.mutex.Unlock()
-	z.i = x.i.Neg(1)
+	// allocate a new *Int otherwise we will mutate arg `x`
+	z.i = x.Clone().i.Neg(1)
 	return z
 }
 func (z *Int) SetNeg() *Int {

crypto/vss/feldman_vss_test.go

@@ -128,6 +128,6 @@ func TestReconstruct(t *testing.T) {
 	assert.NoError(t, err5)
 	assert.NotZero(t, secret5)
 
-	assert.EqualValues(t, secret, secret4, "secrets must be the same")
-	assert.EqualValues(t, secret4, secret5, "secrets must be the same")
+	assert.Equal(t, secret.Int64(), secret4.Int64(), "secrets must be the same")
+	assert.Equal(t, secret4.Int64(), secret5.Int64(), "secrets must be the same")
 }