Fix ECDH-Curve25519 private key masking (#40)

openpgp/ecdh/ecdh_test.go

@@ -20,6 +20,7 @@ var (
 )
 
 
+// TODO: Improve this.
 func TestEncryptDecrypt(t *testing.T) {
 	kdf := KDF{
 		Hash:   algorithm.SHA512,

openpgp/ecdh/x25519.go

@@ -11,13 +11,18 @@ import (
 	"io"
 	"math/big"
 
-	"golang.org/x/crypto/openpgp/aes/keywrap"
 	"golang.org/x/crypto/curve25519"
+	"golang.org/x/crypto/openpgp/aes/keywrap"
 	"golang.org/x/crypto/openpgp/internal/ecc"
 )
 
-func X25519GenerateParams(rand io.Reader) (priv [32]byte, x [32]byte, err error) {
-	var n, helper = new (big.Int), new (big.Int)
+// Generates a private-public key-pair.
+// 'priv' is a private key; a scalar belonging to the set
+// 2^{254} + 8 * [0, 2^{251}), in order to avoid the small subgroup of the
+// curve. 'pub' is simply 'priv' * G where G is the base point.
+// See https://cr.yp.to/ecdh.html and RFC7748, sec 5.
+func x25519GenerateKeyPairBytes(rand io.Reader) (priv [32]byte, pub [32]byte, err error) {
+	var n, helper = new(big.Int), new(big.Int)
 	n.SetUint64(1)
 	n.Lsh(n, 252)
 	helper.SetString("27742317777372353535851937790883648493", 10)
@@ -28,26 +33,32 @@ func X25519GenerateParams(rand io.Reader) (priv [32]byte, x [32]byte, err error)
 		if err != nil {
 			return
 		}
-		// This is because, in tests, rand will return all zeros and we don't
-		// want to get the point at infinity and loop forever.
-		priv[1] ^= 0x42
+		// The following ensures that the private key is a number of the form
+		// 2^{254} + 8 * [0, 2^{251}), in order to avoid the small subgroup of
+		// of the curve.
+		priv[0] &= 248
+		priv[31] &= 127
+		priv[31] |= 64
 
 		// If the scalar is out of range, sample another random number.
 		if new(big.Int).SetBytes(priv[:]).Cmp(n) >= 0 {
 			continue
 		}
 
-		curve25519.ScalarBaseMult(&x, &priv)
+		curve25519.ScalarBaseMult(&pub, &priv)
 		return
 	}
 	return
 }
 
+// X25519GenerateKey samples the key pair according to the correct distribution.
+// It also sets the given key-derivation function and returns the *PrivateKey
+// object along with an error.
 func X25519GenerateKey(rand io.Reader, kdf KDF) (priv *PrivateKey, err error) {
 	ci := ecc.FindByName("Curve25519")
 	priv = new(PrivateKey)
 	priv.PublicKey.Curve = ci.Curve
-	d, pubKey, err := X25519GenerateParams(rand)
+	d, pubKey, err := x25519GenerateKeyPairBytes(rand)
 	if err != nil {
 		return nil, err
 	}
@@ -57,26 +68,28 @@ func X25519GenerateKey(rand io.Reader, kdf KDF) (priv *PrivateKey, err error) {
 	priv.PublicKey.CurveType = ci.CurveType
 	priv.PublicKey.Curve = ci.Curve
 	/*
-	 * Note that ECPoint.point differs from the definition of public keys in [Curve25519] in two ways:
-	 * (1) the byte-ordering is big-endian, which is more uniform with how big integers are represented in TLS, and
-	 * (2) there is an additional length byte (so ECpoint.point is actually 33 bytes), again for uniformity (and extensibility).
+	 * Note that ECPoint.point differs from the definition of public keys in
+	 * [Curve25519] in two ways: (1) the byte-ordering is big-endian, which is
+	 * more uniform with how big integers are represented in TLS, and (2) there
+	 * is an additional length byte (so ECpoint.point is actually 33 bytes),
+	 * again for uniformity (and extensibility).
 	 */
 	var encodedKey = make([]byte, 33)
 	encodedKey[0] = 0x40
 	copy(encodedKey[1:], pubKey[:])
-	priv.PublicKey.X = new (big.Int).SetBytes(encodedKey[:])
-	priv.PublicKey.Y = new (big.Int)
+	priv.PublicKey.X = new(big.Int).SetBytes(encodedKey[:])
+	priv.PublicKey.Y = new(big.Int)
 	return priv, nil
 }
 
 func X25519Encrypt(random io.Reader, pub *PublicKey, msg, curveOID, fingerprint []byte) (vsG, c []byte, err error) {
-	d, ephemeralKey, err := X25519GenerateParams(random)
+	d, ephemeralKey, err := x25519GenerateKeyPairBytes(random)
 	if err != nil {
 		return nil, nil, err
 	}
 	var pubKey [32]byte
 
-	if pub.X.BitLen() > 33 * 264 {
+	if pub.X.BitLen() > 33*264 {
 		return nil, nil, errors.New("ecdh: invalid key")
 	}
 	copy(pubKey[:], pub.X.Bytes()[1:])
@@ -102,9 +115,9 @@ func X25519Encrypt(random io.Reader, pub *PublicKey, msg, curveOID, fingerprint
 }
 
 func X25519Decrypt(priv *PrivateKey, vsG, m, curveOID, fingerprint []byte) (msg []byte, err error) {
-	var zb, d, ephemeralKey[32]byte
+	var zb, d, ephemeralKey [32]byte
 	if len(vsG) != 33 || vsG[0] != 0x40 {
-		return nil,  errors.New("ecdh: invalid key")
+		return nil, errors.New("ecdh: invalid key")
 	}
 	copy(ephemeralKey[:], vsG[1:33])
 
@@ -141,4 +154,4 @@ func copyReversed(out []byte, in []byte) {
 	for i := 0; i < l; i++ {
 		out[i] = in[l-i-1]
 	}
-}
+}

openpgp/ecdh/x25519_test.go

@@ -0,0 +1,35 @@
+// Copyright 2019 ProtonTech AG.
+//
+
+package ecdh
+
+import (
+	"testing"
+	"crypto/rand"
+)
+
+// Some OpenPGP implementations, such as gpg 2.2.12, do not accept ECDH private
+// keys if they're not masked. This is because they're not of the proper form,
+// cryptographically, and they don't mask input keys during crypto operations.
+// This test checks if the keys that this library stores or outputs are
+// properly masked.
+func TestGenerateMaskedPrivateKeyX25519(t *testing.T) {
+	priv, _, err := x25519GenerateKeyPairBytes(rand.Reader)
+	if err != nil  {
+		t.Fatal(err)
+	}
+
+	// Check masking
+	// 3 lsb are 0
+	if priv[0]<<5 != 0 {
+		t.Fatalf("Priv. key is not masked (3 lsb should be unset): %X", priv)
+	}
+	// MSB is 0
+	if priv[31]>>7 != 0 {
+		t.Fatalf("Priv. key is not masked (MSB should be unset): %X", priv)
+	}
+	// Second-MSB is 1
+	if priv[31]>>6 != 1 {
+		t.Fatalf("Priv. key is not masked (second MSB should be set): %X", priv)
+	}
+}

openpgp/packet/private_key.go

@@ -225,21 +225,14 @@ func (pk *PrivateKey) Serialize(w io.Writer) (err error) {
 	return
 }
 
+// TODO: Write a description
 // SerializeUnEncrypted ..
 func (pk *PrivateKey) SerializeUnEncrypted(w io.Writer) (err error) {
 	buf := bytes.NewBuffer(nil)
 	buf.Write([]byte{uint8(S2KNON)} /* no encryption */)
-	switch priv := pk.PrivateKey.(type) {
-	case *rsa.PrivateKey:
-		err = serializeRSAPrivateKey(buf, priv)
-	case *dsa.PrivateKey:
-		err = serializeDSAPrivateKey(buf, priv)
-	case *elgamal.PrivateKey:
-		err = serializeElGamalPrivateKey(buf, priv)
-	case *ecdsa.PrivateKey:
-		err = serializeECDSAPrivateKey(buf, priv)
-	default:
-		err = errors.InvalidArgumentError("unknown private key type")
+	err = pk.serializePrivateKey(buf)
+	if err != nil {
+		return err
 	}
 	privateKeyBytes := buf.Bytes()
 	if pk.sha1Checksum {