crypto/elliptic: panic when operating on invalid points

Fixes #50975
For #52182

Change-Id: I4a98d965436c7034877b8c0146bb0bd5b802d6fa
Reviewed-on: https://go-review.googlesource.com/c/go/+/382995
Reviewed-by: David Chase <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: FiloSottile

src/crypto/elliptic/elliptic.go

@@ -72,6 +72,8 @@ func GenerateKey(curve Curve, rand io.Reader) (priv []byte, x, y *big.Int, err e
 // SEC 1, Version 2.0, Section 2.3.3. If the point is not on the curve (or is
 // the conventional point at infinity), the behavior is undefined.
 func Marshal(curve Curve, x, y *big.Int) []byte {
+	panicIfNotOnCurve(curve, x, y)
+
 	byteLen := (curve.Params().BitSize + 7) / 8
 
 	ret := make([]byte, 1+2*byteLen)
@@ -87,6 +89,7 @@ func Marshal(curve Curve, x, y *big.Int) []byte {
 // specified in SEC 1, Version 2.0, Section 2.3.3. If the point is not on the
 // curve (or is the conventional point at infinity), the behavior is undefined.
 func MarshalCompressed(curve Curve, x, y *big.Int) []byte {
+	panicIfNotOnCurve(curve, x, y)
 	byteLen := (curve.Params().BitSize + 7) / 8
 	compressed := make([]byte, 1+byteLen)
 	compressed[0] = byte(y.Bit(0)) | 2
@@ -168,6 +171,18 @@ func UnmarshalCompressed(curve Curve, data []byte) (x, y *big.Int) {
 	return
 }
 
+func panicIfNotOnCurve(curve Curve, x, y *big.Int) {
+	// (0, 0) is the point at infinity by convention. It's ok to operate on it,
+	// although IsOnCurve is documented to return false for it. See Issue 37294.
+	if x.Sign() == 0 && y.Sign() == 0 {
+		return
+	}
+
+	if !curve.IsOnCurve(x, y) {
+		panic("crypto/elliptic: attempted operation on invalid point")
+	}
+}
+
 var initonce sync.Once
 
 func initAll() {

src/crypto/elliptic/elliptic_test.go

@@ -61,7 +61,13 @@ func TestOffCurve(t *testing.T) {
 		if curve.IsOnCurve(x, y) {
 			t.Errorf("point off curve is claimed to be on the curve")
 		}
-		b := Marshal(curve, x, y)
+
+		byteLen := (curve.Params().BitSize + 7) / 8
+		b := make([]byte, 1+2*byteLen)
+		b[0] = 4 // uncompressed point
+		x.FillBytes(b[1 : 1+byteLen])
+		y.FillBytes(b[1+byteLen : 1+2*byteLen])
+
 		x1, y1 := Unmarshal(curve, b)
 		if x1 != nil || y1 != nil {
 			t.Errorf("unmarshaling a point not on the curve succeeded")

src/crypto/elliptic/nistec.go

@@ -6,7 +6,6 @@ package elliptic
 
 import (
 	"crypto/elliptic/internal/nistec"
-	"crypto/rand"
 	"errors"
 	"math/big"
 )
@@ -173,39 +172,22 @@ func (curve *nistCurve[Point]) pointToAffine(p Point) (x, y *big.Int) {
 	return x, y
 }
 
-// randomPoint returns a random point on the curve. It's used when Add,
-// Double, or ScalarMult are fed a point not on the curve, which is undefined
-// behavior. Originally, we used to do the math on it anyway (which allows
-// invalid curve attacks) and relied on the caller and Unmarshal to avoid this
-// happening in the first place. Now, we just can't construct a nistec Point
-// for an invalid pair of coordinates, because that API is safer. If we panic,
-// we risk introducing a DoS. If we return nil, we risk a panic. If we return
-// the input, ecdsa.Verify might fail open. The safest course seems to be to
-// return a valid, random point, which hopefully won't help the attacker.
-func (curve *nistCurve[Point]) randomPoint() (x, y *big.Int) {
-	_, x, y, err := GenerateKey(curve, rand.Reader)
-	if err != nil {
-		panic("crypto/elliptic: failed to generate random point")
-	}
-	return x, y
-}
-
 func (curve *nistCurve[Point]) Add(x1, y1, x2, y2 *big.Int) (*big.Int, *big.Int) {
 	p1, err := curve.pointFromAffine(x1, y1)
 	if err != nil {
-		return curve.randomPoint()
+		panic("crypto/elliptic: Add was called on an invalid point")
 	}
 	p2, err := curve.pointFromAffine(x2, y2)
 	if err != nil {
-		return curve.randomPoint()
+		panic("crypto/elliptic: Add was called on an invalid point")
 	}
 	return curve.pointToAffine(p1.Add(p1, p2))
 }
 
 func (curve *nistCurve[Point]) Double(x1, y1 *big.Int) (*big.Int, *big.Int) {
 	p, err := curve.pointFromAffine(x1, y1)
 	if err != nil {
-		return curve.randomPoint()
+		panic("crypto/elliptic: Double was called on an invalid point")
 	}
 	return curve.pointToAffine(p.Double(p))
 }
@@ -228,12 +210,12 @@ func (curve *nistCurve[Point]) normalizeScalar(scalar []byte) []byte {
 func (curve *nistCurve[Point]) ScalarMult(Bx, By *big.Int, scalar []byte) (*big.Int, *big.Int) {
 	p, err := curve.pointFromAffine(Bx, By)
 	if err != nil {
-		return curve.randomPoint()
+		panic("crypto/elliptic: ScalarMult was called on an invalid point")
 	}
 	scalar = curve.normalizeScalar(scalar)
 	p, err = p.ScalarMult(p, scalar)
 	if err != nil {
-		panic("elliptic: nistec rejected normalized scalar")
+		panic("crypto/elliptic: nistec rejected normalized scalar")
 	}
 	return curve.pointToAffine(p)
 }
@@ -242,7 +224,7 @@ func (curve *nistCurve[Point]) ScalarBaseMult(scalar []byte) (*big.Int, *big.Int
 	scalar = curve.normalizeScalar(scalar)
 	p, err := curve.newPoint().ScalarBaseMult(scalar)
 	if err != nil {
-		panic("elliptic: nistec rejected normalized scalar")
+		panic("crypto/elliptic: nistec rejected normalized scalar")
 	}
 	return curve.pointToAffine(p)
 }
@@ -253,16 +235,16 @@ func (curve *nistCurve[Point]) CombinedMult(Px, Py *big.Int, s1, s2 []byte) (x,
 	s1 = curve.normalizeScalar(s1)
 	q, err := curve.newPoint().ScalarBaseMult(s1)
 	if err != nil {
-		panic("elliptic: nistec rejected normalized scalar")
+		panic("crypto/elliptic: nistec rejected normalized scalar")
 	}
 	p, err := curve.pointFromAffine(Px, Py)
 	if err != nil {
-		return curve.randomPoint()
+		panic("crypto/elliptic: CombinedMult was called on an invalid point")
 	}
 	s2 = curve.normalizeScalar(s2)
 	p, err = p.ScalarMult(p, s2)
 	if err != nil {
-		panic("elliptic: nistec rejected normalized scalar")
+		panic("crypto/elliptic: nistec rejected normalized scalar")
 	}
 	return curve.pointToAffine(p.Add(p, q))
 }
@@ -299,15 +281,15 @@ func (curve *nistCurve[Point]) UnmarshalCompressed(data []byte) (x, y *big.Int)
 func bigFromDecimal(s string) *big.Int {
 	b, ok := new(big.Int).SetString(s, 10)
 	if !ok {
-		panic("invalid encoding")
+		panic("crypto/elliptic: internal error: invalid encoding")
 	}
 	return b
 }
 
 func bigFromHex(s string) *big.Int {
 	b, ok := new(big.Int).SetString(s, 16)
 	if !ok {
-		panic("invalid encoding")
+		panic("crypto/elliptic: internal error: invalid encoding")
 	}
 	return b
 }

src/crypto/elliptic/nistec_p256.go

@@ -23,7 +23,7 @@ func (c p256Curve) Inverse(k *big.Int) *big.Int {
 	scalar := k.FillBytes(make([]byte, 32))
 	inverse, err := nistec.P256OrdInverse(scalar)
 	if err != nil {
-		panic("elliptic: nistec rejected normalized scalar")
+		panic("crypto/elliptic: nistec rejected normalized scalar")
 	}
 	return new(big.Int).SetBytes(inverse)
 }

src/crypto/elliptic/params.go

@@ -97,6 +97,8 @@ func (curve *CurveParams) Add(x1, y1, x2, y2 *big.Int) (*big.Int, *big.Int) {
 	if specific, ok := matchesSpecificCurve(curve); ok {
 		return specific.Add(x1, y1, x2, y2)
 	}
+	panicIfNotOnCurve(curve, x1, y1)
+	panicIfNotOnCurve(curve, x2, y2)
 
 	z1 := zForAffine(x1, y1)
 	z2 := zForAffine(x2, y2)
@@ -187,6 +189,7 @@ func (curve *CurveParams) Double(x1, y1 *big.Int) (*big.Int, *big.Int) {
 	if specific, ok := matchesSpecificCurve(curve); ok {
 		return specific.Double(x1, y1)
 	}
+	panicIfNotOnCurve(curve, x1, y1)
 
 	z1 := zForAffine(x1, y1)
 	return curve.affineFromJacobian(curve.doubleJacobian(x1, y1, z1))
@@ -259,6 +262,7 @@ func (curve *CurveParams) ScalarMult(Bx, By *big.Int, k []byte) (*big.Int, *big.
 	if specific, ok := matchesSpecificCurve(curve); ok {
 		return specific.ScalarMult(Bx, By, k)
 	}
+	panicIfNotOnCurve(curve, Bx, By)
 
 	Bz := new(big.Int).SetInt64(1)
 	x, y, z := new(big.Int), new(big.Int), new(big.Int)