crypto/elliptic: don't unmarshal points that are off the curve

At present, Unmarshal does not check that the point it unmarshals
is actually *on* the curve. (It may be on the curve's twist.)

This can, as Daniel Bernstein has pointed out at great length,
lead to quite devastating attacks. And 3 out of the 4 curves
supported by crypto/elliptic have twists with cofactor != 1;
P-224, in particular, has a sufficiently large cofactor that it
is likely that conventional dlog attacks might be useful.

This closes #2445, filed by Watson Ladd.

To explain why this was (partially) rejected before being accepted:

In the general case, for curves with cofactor != 1, verifying subgroup
membership is required. (This is expensive and hard-to-implement.)
But, as recent discussion during the CFRG standardization process
has brought out, small-subgroup attacks are much less damaging than
a twist attack.

Change-Id: I284042eb9954ff9b7cde80b8b693b1d468c7e1e8
Reviewed-on: https://go-review.googlesource.com/2421
Reviewed-by: Adam Langley <agl@golang.org>

diff --git a/src/crypto/elliptic/elliptic.go b/src/crypto/elliptic/elliptic.go
index 396bb5caf5157fb5f76bd60245ba206277d5705b..f3b84e1eac8d1b527c312ceac0fc6056fda2fe2d 100644 (file)
--- a/src/crypto/elliptic/elliptic.go
+++ b/src/crypto/elliptic/elliptic.go
@@ -308,7 +308,8 @@ func Marshal(curve Curve, x, y *big.Int) []byte {
        return ret
 }
 
-// Unmarshal converts a point, serialized by Marshal, into an x, y pair. On error, x = nil.
+// Unmarshal converts a point, serialized by Marshal, into an x, y pair.
+// It is an error if the point is not on the curve. On error, x = nil.
 func Unmarshal(curve Curve, data []byte) (x, y *big.Int) {
        byteLen := (curve.Params().BitSize + 7) >> 3
        if len(data) != 1+2*byteLen {
@@ -319,6 +320,9 @@ func Unmarshal(curve Curve, data []byte) (x, y *big.Int) {
        }
        x = new(big.Int).SetBytes(data[1 : 1+byteLen])
        y = new(big.Int).SetBytes(data[1+byteLen:])
+       if !curve.Params().IsOnCurve(x, y) {
+               x, y = nil, nil
+       }
        return
 }
 

diff --git a/src/crypto/elliptic/elliptic_test.go b/src/crypto/elliptic/elliptic_test.go
index 4dc27c92bf43eaf7264f5c695b0ffdcbdbffb219..7e27913dcd0505001411377f2f4fab3ebfb21d65 100644 (file)
--- a/src/crypto/elliptic/elliptic_test.go
+++ b/src/crypto/elliptic/elliptic_test.go
@@ -19,6 +19,19 @@ func TestOnCurve(t *testing.T) {
        }
 }
 
+func TestOffCurve(t *testing.T) {
+       p224 := P224()
+       x, y := new(big.Int).SetInt64(1), new(big.Int).SetInt64(1)
+       if p224.IsOnCurve(x, y) {
+               t.Errorf("FAIL: point off curve is claimed to be on the curve")
+       }
+       b := Marshal(p224, x, y)
+       x1, y1 := Unmarshal(p224, b)
+       if x1 != nil || y1 != nil {
+               t.Errorf("FAIL: unmarshalling a point not on the curve succeeded")
+       }
+}
+
 type baseMultTest struct {
        k    string
        x, y string