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/2421Reviewed-by: Adam Langley <agl@golang.org>

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
 }
 

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