From: Adam Langley <agl@golang.org>
Date: Mon, 21 Dec 2015 22:40:23 +0000 (-0800)
Subject: crypto/x509: handle ECC private keys with the wrong length.
X-Git-Tag: go1.6beta2~142
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=9338f39459adc0d62277e141495cc5f7886b5896;p=gostls13.git

crypto/x509: handle ECC private keys with the wrong length.

SEC-1 says: “The component privateKey is the private key defined to be
the octet string of length ⌊log₂(n)/8⌋ (where n is the order of the
curve)”.

Previously the code for parsing ECC private keys would panic (on
non-amd64) when the private was too long. It would also pass a too-short
private key to crypto/elliptic, possibly resulting in undesirable
behaviour.

This change makes the parsing function handle both too much and too
little padding because GnuTLS does the former and OpenSSL did the latter
until 30cd4ff294252c4b6a4b69cbef6a5b4117705d22. It also causes
serialisation to pad private keys correctly.

Fixes #13699

Change-Id: If9c2faeaeb45af8a4d7770d784f3d2633e7f8290
Reviewed-on: https://go-review.googlesource.com/18094
Run-TryBot: Adam Langley <agl@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
---

diff --git a/src/crypto/x509/sec1.go b/src/crypto/x509/sec1.go
index c4d7ab68f7..f484b6d042 100644
--- a/src/crypto/x509/sec1.go
+++ b/src/crypto/x509/sec1.go
@@ -39,9 +39,14 @@ func MarshalECPrivateKey(key *ecdsa.PrivateKey) ([]byte, error) {
 	if !ok {
 		return nil, errors.New("x509: unknown elliptic curve")
 	}
+
+	privateKeyBytes := key.D.Bytes()
+	paddedPrivateKey := make([]byte, (key.Curve.Params().N.BitLen() + 7) / 8)
+	copy(paddedPrivateKey[len(paddedPrivateKey) - len(privateKeyBytes):], privateKeyBytes)
+
 	return asn1.Marshal(ecPrivateKey{
 		Version:       1,
-		PrivateKey:    key.D.Bytes(),
+		PrivateKey:    paddedPrivateKey,
 		NamedCurveOID: oid,
 		PublicKey:     asn1.BitString{Bytes: elliptic.Marshal(key.Curve, key.X, key.Y)},
 	})
@@ -71,13 +76,30 @@ func parseECPrivateKey(namedCurveOID *asn1.ObjectIdentifier, der []byte) (key *e
 	}
 
 	k := new(big.Int).SetBytes(privKey.PrivateKey)
-	if k.Cmp(curve.Params().N) >= 0 {
+	curveOrder := curve.Params().N
+	if k.Cmp(curveOrder) >= 0 {
 		return nil, errors.New("x509: invalid elliptic curve private key value")
 	}
 	priv := new(ecdsa.PrivateKey)
 	priv.Curve = curve
 	priv.D = k
-	priv.X, priv.Y = curve.ScalarBaseMult(privKey.PrivateKey)
+
+	privateKey := make([]byte, (curveOrder.BitLen() + 7) / 8)
+
+	// Some private keys have leading zero padding. This is invalid
+	// according to [SEC1], but this code will ignore it.
+	for len(privKey.PrivateKey) > len(privateKey) {
+		if privKey.PrivateKey[0] != 0 {
+			return nil, errors.New("x509: invalid private key length")
+		}
+		privKey.PrivateKey = privKey.PrivateKey[1:]
+	}
+
+	// Some private keys remove all leading zeros, this is also invalid
+	// according to [SEC1] but since OpenSSL used to do this, we ignore
+	// this too.
+	copy(privateKey[len(privateKey) - len(privKey.PrivateKey):], privKey.PrivateKey)
+	priv.X, priv.Y = curve.ScalarBaseMult(privateKey)
 
 	return priv, nil
 }
diff --git a/src/crypto/x509/sec1_test.go b/src/crypto/x509/sec1_test.go
index 95f18e77de..5e9ded54d9 100644
--- a/src/crypto/x509/sec1_test.go
+++ b/src/crypto/x509/sec1_test.go
@@ -10,21 +10,35 @@ import (
 	"testing"
 )
 
-// Generated using:
-//   openssl ecparam -genkey -name secp384r1 -outform PEM
-var ecPrivateKeyHex = `3081a40201010430bdb9839c08ee793d1157886a7a758a3c8b2a17a4df48f17ace57c72c56b4723cf21dcda21d4e1ad57ff034f19fcfd98ea00706052b81040022a16403620004feea808b5ee2429cfcce13c32160e1c960990bd050bb0fdf7222f3decd0a55008e32a6aa3c9062051c4cba92a7a3b178b24567412d43cdd2f882fa5addddd726fe3e208d2c26d733a773a597abb749714df7256ead5105fa6e7b3650de236b50`
+var ecKeyTests = []struct{
+	derHex string
+	shouldReserialize bool
+}{
+	// Generated using:
+	//   openssl ecparam -genkey -name secp384r1 -outform PEM
+	{"3081a40201010430bdb9839c08ee793d1157886a7a758a3c8b2a17a4df48f17ace57c72c56b4723cf21dcda21d4e1ad57ff034f19fcfd98ea00706052b81040022a16403620004feea808b5ee2429cfcce13c32160e1c960990bd050bb0fdf7222f3decd0a55008e32a6aa3c9062051c4cba92a7a3b178b24567412d43cdd2f882fa5addddd726fe3e208d2c26d733a773a597abb749714df7256ead5105fa6e7b3650de236b50", true},
+	// This key was generated by GnuTLS and has illegal zero-padding of the
+	// private key. See https://github.com/golang/go/issues/13699.
+	{"3078020101042100f9f43a04b9bdc3ab01f53be6df80e7a7bc3eaf7b87fc24e630a4a0aa97633645a00a06082a8648ce3d030107a1440342000441a51bc318461b4c39a45048a16d4fc2a935b1ea7fe86e8c1fa219d6f2438f7c7fd62957d3442efb94b6a23eb0ea66dda663dc42f379cda6630b21b7888a5d3d", false},
+	// This was generated using an old version of OpenSSL and is missing a
+	// leading zero byte in the private key that should be present.
+	{"3081db0201010441607b4f985774ac21e633999794542e09312073480baa69550914d6d43d8414441e61b36650567901da714f94dffb3ce0e2575c31928a0997d51df5c440e983ca17a00706052b81040023a181890381860004001661557afedd7ac8d6b70e038e576558c626eb62edda36d29c3a1310277c11f67a8c6f949e5430a37dcfb95d902c1b5b5379c389873b9dd17be3bdb088a4774a7401072f830fb9a08d93bfa50a03dd3292ea07928724ddb915d831917a338f6b0aecfbc3cf5352c4a1295d356890c41c34116d29eeb93779aab9d9d78e2613437740f6", false},
+}
 
 func TestParseECPrivateKey(t *testing.T) {
-	derBytes, _ := hex.DecodeString(ecPrivateKeyHex)
-	key, err := ParseECPrivateKey(derBytes)
-	if err != nil {
-		t.Errorf("failed to decode EC private key: %s", err)
-	}
-	serialized, err := MarshalECPrivateKey(key)
-	if err != nil {
-		t.Fatalf("failed to encode EC private key: %s", err)
-	}
-	if !bytes.Equal(serialized, derBytes) {
-		t.Fatalf("serialized key differs: got %x, want %x", serialized, derBytes)
+	for i, test := range ecKeyTests {
+		derBytes, _ := hex.DecodeString(test.derHex)
+		key, err := ParseECPrivateKey(derBytes)
+		if err != nil {
+			t.Fatalf("#%d: failed to decode EC private key: %s", i, err)
+		}
+		serialized, err := MarshalECPrivateKey(key)
+		if err != nil {
+			t.Fatalf("#%d: failed to encode EC private key: %s", i, err)
+		}
+		matches := bytes.Equal(serialized, derBytes)
+		if matches != test.shouldReserialize {
+			t.Fatalf("#%d: when serializing key: matches=%t, should match=%t: original %x, reserialized %x", i, matches, test.shouldReserialize, serialized, derBytes)
+		}
 	}
 }