Bug in ECDHE Implementation

[kyoukaya]

I understand that this library is now deprecated, but perhaps this information will be useful for those who intend to fork this library.

Background

We run an internal fork of this library and noticed around a small percentage of applepay token decryptions were failing with a cipher: message authentication failed error in production. We tried to replicate the issue locally and managed to narrow it down to the ECDHE and KDF functions.

Issue

The combined use of the ecdheSharedSecret and deriveEncryptionKey functions do not correctly implement P256 ECDHE correctly. Notably, they fail to pad the X coordinate of the point to 32 bytes as specified in SEC 1, Version 2.0, Section 2.3.5.

Replication

This snippet (playground link) illustrates the difference between the ScalarMult and Bytes implementation with the correct crypto/ecdh implementation:

package main

import (
	"crypto/ecdh"
	"crypto/ecdsa"
	"crypto/elliptic"
	"fmt"
	"math/big"
)

func main() {
	k1 := []byte{42, 154, 0, 223, 84, 8, 55, 162, 117, 24, 142, 136, 128, 33, 143, 69, 194, 175, 251, 116, 183, 49, 190, 0, 228, 232, 21, 229, 203, 187, 226, 96}
	k2 := []byte{195, 5, 124, 117, 229, 83, 2, 62, 251, 203, 46, 195, 218, 215, 152, 7, 226, 64, 180, 63, 19, 183, 198, 37, 234, 194, 188, 222, 158, 85, 230, 191}

	privECDH, _ := ecdh.P256().NewPrivateKey(k1)
	privECDH2, _ := ecdh.P256().NewPrivateKey(k2)
	secretECDHE, _ := privECDH.ECDH(privECDH2.PublicKey())

	privECDSA := ecdhToECDSAPrivKey(privECDH)
	pubECDSA := ecdhToECDSAPublicKey(privECDH2.PublicKey())
	x, _ := privECDSA.Curve.ScalarMult(pubECDSA.X, pubECDSA.Y, privECDSA.D.Bytes())
	secretScalarMult := x.Bytes()
	secretMultFillBytes := make([]byte, 32)
	secretMultFillBytes = x.FillBytes(secretMultFillBytes)

	fmt.Printf("len:%d %x\n", len(secretECDHE), secretECDHE)                 // len:32 005363f5f75762476c04d7f32385629501368593df7e89aea4cdaebe4ae6c217
	fmt.Printf("len:%d %x\n", len(secretScalarMult), secretScalarMult)       // len:31 5363f5f75762476c04d7f32385629501368593df7e89aea4cdaebe4ae6c217
	fmt.Printf("len:%d %x\n", len(secretMultFillBytes), secretMultFillBytes) // len:32 005363f5f75762476c04d7f32385629501368593df7e89aea4cdaebe4ae6c217
}

func ecdhToECDSAPrivKey(key *ecdh.PrivateKey) *ecdsa.PrivateKey {
	return &ecdsa.PrivateKey{
		PublicKey: *ecdhToECDSAPublicKey(key.PublicKey()),
		D:         big.NewInt(0).SetBytes(key.Bytes()),
	}
}

func ecdhToECDSAPublicKey(key *ecdh.PublicKey) *ecdsa.PublicKey {
	rawKey := key.Bytes()
	return &ecdsa.PublicKey{
		Curve: elliptic.P256(),
		X:     big.NewInt(0).SetBytes(rawKey[1:33]),
		Y:     big.NewInt(0).SetBytes(rawKey[33:]),
	}
}

Fix

If you are using go1.20 or newer, I would recommend that you refactor the code to use crypto/ecdh instead. As at go1.20, (elliptic.Curve).ScalarMult contained a note advising against its use while recommending the then newly added crypto/ecdh package which correctly implements ECDHE. From go1.21 onwards, (elliptic.Curve).ScalarMult is marked as deprecated.

For versions less than go1.20, replacing the (*big.Int).Bytes() call in deriveEncryptionKey with (*big.Int).SetBytes on a 32 length byte slice would be an alternative.