Merge pull request #626 from eljobe/fix/curvepoint-equal-mutation

Fix curvePoint.Equal() mutating its operands

Author: pierluca

group/p256/curve.go

@@ -31,13 +31,14 @@ func (P *curvePoint) Equal(P2 kyber.Point) bool {
 
 	// Make sure both coordinates are normalized.
 	// Apparently Go's elliptic curve code doesn't always ensure this.
+	// Use temporary big.Ints to avoid mutating the operands.
 	M := P.c.p.P
-	P.x.Mod(P.x, M)
-	P.y.Mod(P.y, M)
-	cp2.x.Mod(cp2.x, M)
-	cp2.y.Mod(cp2.y, M)
+	x1 := new(big.Int).Mod(P.x, M)
+	y1 := new(big.Int).Mod(P.y, M)
+	x2 := new(big.Int).Mod(cp2.x, M)
+	y2 := new(big.Int).Mod(cp2.y, M)
 
-	return P.x.Cmp(cp2.x) == 0 && P.y.Cmp(cp2.y) == 0
+	return x1.Cmp(x2) == 0 && y1.Cmp(y2) == 0
 }
 
 func (P *curvePoint) Null() kyber.Point {
@@ -47,8 +48,8 @@ func (P *curvePoint) Null() kyber.Point {
 }
 
 func (P *curvePoint) Base() kyber.Point {
-	P.x = P.c.p.Gx
-	P.y = P.c.p.Gy
+	P.x = new(big.Int).Set(P.c.p.Gx)
+	P.y = new(big.Int).Set(P.c.p.Gy)
 	return P
 }
 
@@ -271,13 +272,17 @@ func (P *curvePoint) Set(A kyber.Point) kyber.Point {
 	if !ok {
 		panic(ErrTypeCast)
 	}
-	P.x = aCurvePoint.x
-	P.y = aCurvePoint.y
+	P.x = new(big.Int).Set(aCurvePoint.x)
+	P.y = new(big.Int).Set(aCurvePoint.y)
 	return P
 }
 
 func (P *curvePoint) Clone() kyber.Point {
-	return &curvePoint{x: P.x, y: P.y, c: P.c}
+	return &curvePoint{
+		x: new(big.Int).Set(P.x),
+		y: new(big.Int).Set(P.y),
+		c: P.c,
+	}
 }
 
 // Return the order of this curve: the prime N in the curve parameters.

group/p256/equal_mutation_test.go

@@ -0,0 +1,86 @@
+//go:build !constantTime
+
+package p256
+
+import (
+	"math/big"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestEqualDoesNotMutate verifies that Equal does not modify either operand.
+// Regression test for https://github.com/dedis/kyber/issues/625
+func TestEqualDoesNotMutate(t *testing.T) {
+	suite := NewBlakeSHA256P256()
+
+	a := suite.Point().Pick(suite.RandomStream()).(*curvePoint)
+	b := suite.Point().Pick(suite.RandomStream())
+
+	origX := new(big.Int).Set(a.x)
+
+	// Make x non-normalized by adding the field modulus.
+	// Mathematically equivalent (mod P), but numerically different.
+	a.x.Add(a.x, a.c.p.P)
+	require.NotEqual(t, 0, a.x.Cmp(origX), "sanity: x should differ after adding P")
+
+	a.Equal(b)
+
+	require.NotEqual(t, 0, a.x.Cmp(origX),
+		"Equal() should not normalize the receiver's coordinates")
+}
+
+// TestEqualDoesNotMutateArgument verifies Equal doesn't modify the argument.
+func TestEqualDoesNotMutateArgument(t *testing.T) {
+	suite := NewBlakeSHA256P256()
+
+	a := suite.Point().Pick(suite.RandomStream())
+	b := suite.Point().Pick(suite.RandomStream()).(*curvePoint)
+
+	origBX := new(big.Int).Set(b.x)
+	b.x.Add(b.x, b.c.p.P)
+
+	a.Equal(b)
+
+	require.NotEqual(t, 0, b.x.Cmp(origBX),
+		"Equal() should not normalize the argument's coordinates")
+}
+
+// TestSetDeepCopies verifies that Set copies coordinate values, not pointers.
+func TestSetDeepCopies(t *testing.T) {
+	suite := NewBlakeSHA256P256()
+
+	a := suite.Point().Pick(suite.RandomStream()).(*curvePoint)
+	b := suite.Point().(*curvePoint)
+	b.c = a.c
+	b.Set(a)
+
+	require.NotSame(t, a.x, b.x, "Set should deep-copy x, not alias the pointer")
+	require.NotSame(t, a.y, b.y, "Set should deep-copy y, not alias the pointer")
+	require.True(t, a.Equal(b), "Set copy should be equal to original")
+}
+
+// TestCloneDeepCopies verifies that Clone copies coordinate values, not pointers.
+func TestCloneDeepCopies(t *testing.T) {
+	suite := NewBlakeSHA256P256()
+
+	a := suite.Point().Pick(suite.RandomStream()).(*curvePoint)
+	b := a.Clone().(*curvePoint)
+
+	require.NotSame(t, a.x, b.x, "Clone should deep-copy x, not alias the pointer")
+	require.NotSame(t, a.y, b.y, "Clone should deep-copy y, not alias the pointer")
+	require.True(t, a.Equal(b), "Clone should be equal to original")
+}
+
+// TestBaseDeepCopies verifies that Base does not alias the curve's global
+// generator coordinates.
+func TestBaseDeepCopies(t *testing.T) {
+	suite := NewBlakeSHA256P256()
+
+	base := suite.Point().Base().(*curvePoint)
+
+	require.NotSame(t, base.c.p.Gx, base.x,
+		"Base should deep-copy Gx, not alias the curve parameter")
+	require.NotSame(t, base.c.p.Gy, base.y,
+		"Base should deep-copy Gy, not alias the curve parameter")
+}