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 <drchase@google.com>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/crypto/elliptic/elliptic.go b/src/crypto/elliptic/elliptic.go
index 01838dd86899768cb2e73b7d0163a25933a67362..8c0b60b8892d44c3360bc3e14d73b8959374bdae 100644 (file)
--- a/src/crypto/elliptic/elliptic.go
+++ b/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() {

diff --git a/src/crypto/elliptic/elliptic_test.go b/src/crypto/elliptic/elliptic_test.go
index 56756ba52ddc4f74170133115ab6f091e799c4a6..34d70f6a47310d06c22c4e39669f3e7efdfb0815 100644 (file)
--- a/src/crypto/elliptic/elliptic_test.go
+++ b/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")

diff --git a/src/crypto/elliptic/nistec.go b/src/crypto/elliptic/nistec.go
index 58c9c5c07cd465fd35cef1c784c5a7b8918ed22e..60d58720f3e4188b4e7eb417818bb6884057d530 100644 (file)
--- a/src/crypto/elliptic/nistec.go
+++ b/src/crypto/elliptic/nistec.go
@@ -6,7 +6,6 @@ package elliptic
 
 import (
        "crypto/elliptic/internal/nistec"
-       "crypto/rand"
        "errors"
        "math/big"
 )
@@ -173,31 +172,14 @@ 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))
 }
@@ -205,7 +187,7 @@ func (curve *nistCurve[Point]) Add(x1, y1, x2, y2 *big.Int) (*big.Int, *big.Int)
 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,7 +281,7 @@ 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
 }
@@ -307,7 +289,7 @@ func bigFromDecimal(s string) *big.Int {
 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
 }

diff --git a/src/crypto/elliptic/nistec_p256.go b/src/crypto/elliptic/nistec_p256.go
index 3e80a33131c1b7885939858d583f7303d9abb54f..205aaa12c7ee4233f77bf5d7371ad38b6cc96572 100644 (file)
--- a/src/crypto/elliptic/nistec_p256.go
+++ 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)
 }

diff --git a/src/crypto/elliptic/params.go b/src/crypto/elliptic/params.go
index 65176bf3522882d71f3079d271aae60148adc5e7..0ed929d61f9c7e13abd98685b0c6dbdc2c82b581 100644 (file)
--- a/src/crypto/elliptic/params.go
+++ b/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)