Skip to content

crypto/ecies: improve concatKDF #20836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Apr 3, 2020
Merged

crypto/ecies: improve concatKDF #20836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5b6118e
ecies: Fix reps calculation
lukechampine Mar 30, 2020
753f927
crypto/ecies: avoid big integer math in overflow check
fjl Mar 31, 2020
5b70f57
crypto/ecies: fix build
fjl Mar 31, 2020
830d138
ecies: overhaul concatKDF
lukechampine Mar 31, 2020
c863c97
ecies: Append hash.Sum to k directly
lukechampine Mar 31, 2020
9c5b79b
ecies: Add missing binary import
lukechampine Mar 31, 2020
b921ff6
crypto/ecies: fix test
fjl Apr 1, 2020
870f40f
crypto/ecies: improve KDF test
fjl Apr 1, 2020
3b81617
crypto/ecies: move hash.Reset call to end of loop
fjl Apr 1, 2020
81823d4
crypto/ecies: avoid overallocation of key buffer
fjl Apr 1, 2020
026172a
crypto/ecies: extract KDF call into shared function
fjl Apr 1, 2020
2215cdf
crypto/ecies: fix doc comment for roundup
fjl Apr 1, 2020
c8d0688
crypto/ecies: add check on keyLen in ECIESParams
fjl Apr 2, 2020
623f49a
crypto/ecies: remove redundant on-curve check in Decrypt
fjl Apr 2, 2020
15c72be
crypto/ecies: oops
fjl Apr 2, 2020
d23792f
crypto/ecies: fix typo in comment
fjl Apr 2, 2020
2775eed
crypto/ecies: fix error message
fjl Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 47 additions & 96 deletions crypto/ecies/ecies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"crypto/elliptic"
"crypto/hmac"
"crypto/subtle"
"encoding/binary"
"fmt"
"hash"
"io"
Expand All @@ -44,7 +45,6 @@ import (
var (
ErrImport = fmt.Errorf("ecies: failed to import key")
ErrInvalidCurve = fmt.Errorf("ecies: invalid elliptic curve")
ErrInvalidParams = fmt.Errorf("ecies: invalid ECIES parameters")
ErrInvalidPublicKey = fmt.Errorf("ecies: invalid public key")
ErrSharedKeyIsPointAtInfinity = fmt.Errorf("ecies: shared key is point at infinity")
ErrSharedKeyTooBig = fmt.Errorf("ecies: shared key params are too big")
Expand Down Expand Up @@ -138,57 +138,39 @@ func (prv *PrivateKey) GenerateShared(pub *PublicKey, skLen, macLen int) (sk []b
}

var (
ErrKeyDataTooLong = fmt.Errorf("ecies: can't supply requested key data")
ErrSharedTooLong = fmt.Errorf("ecies: shared secret is too long")
ErrInvalidMessage = fmt.Errorf("ecies: invalid message")
)

var (
big2To32 = new(big.Int).Exp(big.NewInt(2), big.NewInt(32), nil)
big2To32M1 = new(big.Int).Sub(big2To32, big.NewInt(1))
)

func incCounter(ctr []byte) {
if ctr[3]++; ctr[3] != 0 {
return
}
if ctr[2]++; ctr[2] != 0 {
return
}
if ctr[1]++; ctr[1] != 0 {
return
}
if ctr[0]++; ctr[0] != 0 {
return
}
}

// NIST SP 800-56 Concatenation Key Derivation Function (see section 5.8.1).
func concatKDF(hash hash.Hash, z, s1 []byte, kdLen int) (k []byte, err error) {
if s1 == nil {
s1 = make([]byte, 0)
}

reps := ((kdLen + 7) * 8) / (hash.BlockSize() * 8)
if big.NewInt(int64(reps)).Cmp(big2To32M1) > 0 {
fmt.Println(big2To32M1)
return nil, ErrKeyDataTooLong
}

counter := []byte{0, 0, 0, 1}
k = make([]byte, 0)

for i := 0; i <= reps; i++ {
hash.Write(counter)
func concatKDF(hash hash.Hash, z, s1 []byte, kdLen int) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this method could return an error, ErrKeyDataTooLong. So what would happen now when the same input that would trigger ErrKeyDataTooLong is fed into this one? Is that error only an implementation flaw that went away?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error would be returned when the requested amount of key data was larger than 2^32-1. This error could never actually happen because the concatKDF is only used internally in crypto/ecies and we always request exactly 32 bytes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds odd. So you're saying that if kdLen > 2^32-1, then the result is not correct, but it doesn't matter because we always have it at 32?
Why don't we hardcode it at 32 then, and drop the params? Alternatively, set kdLen to uint8 or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is unlikely anyone will ever request gigabytes of secret key data from this function. If kdLen is very large, the result isn't necessarily incorrect, it just doesn't correspond to the NIST SP 800-56 spec.

counterBytes := make([]byte, 4)
k := make([]byte, 0, roundup(kdLen, hash.Size()))
for counter := uint32(1); len(k) < kdLen; counter++ {
binary.BigEndian.PutUint32(counterBytes, counter)
hash.Reset()
hash.Write(counterBytes)
hash.Write(z)
hash.Write(s1)
k = append(k, hash.Sum(nil)...)
hash.Reset()
incCounter(counter)
k = hash.Sum(k)
}
return k[:kdLen]
}

k = k[:kdLen]
return
// roundup rounds size up to the next multiple of blocksize.
func roundup(size, blocksize int) int {
return size + blocksize - (size % blocksize)
}

// deriveKeys creates the encryption and MAC keys using concatKDF.
func deriveKeys(hash hash.Hash, z, s1 []byte, keyLen int) (Ke, Km []byte) {
K := concatKDF(hash, z, s1, 2*keyLen)
Ke = K[:keyLen]
Km = K[keyLen:]
hash.Reset()
hash.Write(Km)
Km = hash.Sum(Km[:0])
return Ke, Km
}

// messageTag computes the MAC of a message (called the tag) as per
Expand All @@ -209,7 +191,6 @@ func generateIV(params *ECIESParams, rand io.Reader) (iv []byte, err error) {
}

// symEncrypt carries out CTR encryption using the block cipher specified in the
// parameters.
func symEncrypt(rand io.Reader, params *ECIESParams, key, m []byte) (ct []byte, err error) {
c, err := params.Cipher(key)
if err != nil {
Expand Down Expand Up @@ -249,36 +230,27 @@ func symDecrypt(params *ECIESParams, key, ct []byte) (m []byte, err error) {
// ciphertext. s1 is fed into key derivation, s2 is fed into the MAC. If the
// shared information parameters aren't being used, they should be nil.
func Encrypt(rand io.Reader, pub *PublicKey, m, s1, s2 []byte) (ct []byte, err error) {
params := pub.Params
if params == nil {
if params = ParamsFromCurve(pub.Curve); params == nil {
err = ErrUnsupportedECIESParameters
return
}
params, err := pubkeyParams(pub)
if err != nil {
return nil, err
}

R, err := GenerateKey(rand, pub.Curve, params)
if err != nil {
return
return nil, err
}

hash := params.Hash()
z, err := R.GenerateShared(pub, params.KeyLen, params.KeyLen)
if err != nil {
return
}
K, err := concatKDF(hash, z, s1, params.KeyLen+params.KeyLen)
if err != nil {
return
return nil, err
}
Ke := K[:params.KeyLen]
Km := K[params.KeyLen:]
hash.Write(Km)
Km = hash.Sum(nil)
hash.Reset()

hash := params.Hash()
Ke, Km := deriveKeys(hash, z, s1, params.KeyLen)

em, err := symEncrypt(rand, params, Ke, m)
if err != nil || len(em) <= params.BlockSize {
return
return nil, err
}

d := messageTag(params.Hash, Km, em, s2)
Expand All @@ -288,21 +260,19 @@ func Encrypt(rand io.Reader, pub *PublicKey, m, s1, s2 []byte) (ct []byte, err e
copy(ct, Rb)
copy(ct[len(Rb):], em)
copy(ct[len(Rb)+len(em):], d)
return
return ct, nil
}

// Decrypt decrypts an ECIES ciphertext.
func (prv *PrivateKey) Decrypt(c, s1, s2 []byte) (m []byte, err error) {
if len(c) == 0 {
return nil, ErrInvalidMessage
}
params := prv.PublicKey.Params
if params == nil {
if params = ParamsFromCurve(prv.PublicKey.Curve); params == nil {
err = ErrUnsupportedECIESParameters
return
}
params, err := pubkeyParams(&prv.PublicKey)
if err != nil {
return nil, err
}

hash := params.Hash()

var (
Expand All @@ -316,12 +286,10 @@ func (prv *PrivateKey) Decrypt(c, s1, s2 []byte) (m []byte, err error) {
case 2, 3, 4:
rLen = (prv.PublicKey.Curve.Params().BitSize + 7) / 4
if len(c) < (rLen + hLen + 1) {
err = ErrInvalidMessage
return
return nil, ErrInvalidMessage
}
default:
err = ErrInvalidPublicKey
return
return nil, ErrInvalidPublicKey
}

mStart = rLen
Expand All @@ -331,36 +299,19 @@ func (prv *PrivateKey) Decrypt(c, s1, s2 []byte) (m []byte, err error) {
R.Curve = prv.PublicKey.Curve
R.X, R.Y = elliptic.Unmarshal(R.Curve, c[:rLen])
if R.X == nil {
err = ErrInvalidPublicKey
return
}
if !R.Curve.IsOnCurve(R.X, R.Y) {
err = ErrInvalidCurve
return
return nil, ErrInvalidPublicKey
}

z, err := prv.GenerateShared(R, params.KeyLen, params.KeyLen)
if err != nil {
return
return nil, err
}

K, err := concatKDF(hash, z, s1, params.KeyLen+params.KeyLen)
if err != nil {
return
}

Ke := K[:params.KeyLen]
Km := K[params.KeyLen:]
hash.Write(Km)
Km = hash.Sum(nil)
hash.Reset()
Ke, Km := deriveKeys(hash, z, s1, params.KeyLen)

d := messageTag(params.Hash, Km, c[mStart:mEnd], s2)
if subtle.ConstantTimeCompare(c[mEnd:], d) != 1 {
err = ErrInvalidMessage
return
return nil, ErrInvalidMessage
}

m, err = symDecrypt(params, Ke, c[mStart:mEnd])
return
return symDecrypt(params, Ke, c[mStart:mEnd])
}
40 changes: 27 additions & 13 deletions crypto/ecies/ecies_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,23 @@ import (
"github.com/ethereum/go-ethereum/crypto"
)

// Ensure the KDF generates appropriately sized keys.
func TestKDF(t *testing.T) {
msg := []byte("Hello, world")
h := sha256.New()

k, err := concatKDF(h, msg, nil, 64)
if err != nil {
t.Fatal(err)
}
if len(k) != 64 {
t.Fatalf("KDF: generated key is the wrong size (%d instead of 64\n", len(k))
tests := []struct {
length int
output []byte
}{
{6, decode("858b192fa2ed")},
{32, decode("858b192fa2ed4395e2bf88dd8d5770d67dc284ee539f12da8bceaa45d06ebae0")},
{48, decode("858b192fa2ed4395e2bf88dd8d5770d67dc284ee539f12da8bceaa45d06ebae0700f1ab918a5f0413b8140f9940d6955")},
{64, decode("858b192fa2ed4395e2bf88dd8d5770d67dc284ee539f12da8bceaa45d06ebae0700f1ab918a5f0413b8140f9940d6955f3467fd6672cce1024c5b1effccc0f61")},
}

for _, test := range tests {
h := sha256.New()
k := concatKDF(h, []byte("input"), nil, test.length)
if !bytes.Equal(k, test.output) {
t.Fatalf("KDF: generated key %x does not match expected output %x", k, test.output)
}
}
}

Expand Down Expand Up @@ -293,8 +299,8 @@ func TestParamSelection(t *testing.T) {

func testParamSelection(t *testing.T, c testCase) {
params := ParamsFromCurve(c.Curve)
if params == nil && c.Expected != nil {
t.Fatalf("%s (%s)\n", ErrInvalidParams.Error(), c.Name)
if params == nil {
t.Fatal("ParamsFromCurve returned nil")
} else if params != nil && !cmpParams(params, c.Expected) {
t.Fatalf("ecies: parameters should be invalid (%s)\n", c.Name)
}
Expand Down Expand Up @@ -401,7 +407,7 @@ func TestSharedKeyStatic(t *testing.T) {
t.Fatal(ErrBadSharedKeys)
}

sk, _ := hex.DecodeString("167ccc13ac5e8a26b131c3446030c60fbfac6aa8e31149d0869f93626a4cdf62")
sk := decode("167ccc13ac5e8a26b131c3446030c60fbfac6aa8e31149d0869f93626a4cdf62")
if !bytes.Equal(sk1, sk) {
t.Fatalf("shared secret mismatch: want: %x have: %x", sk, sk1)
}
Expand All @@ -414,3 +420,11 @@ func hexKey(prv string) *PrivateKey {
}
return ImportECDSA(key)
}

func decode(s string) []byte {
bytes, err := hex.DecodeString(s)
if err != nil {
panic(err)
}
return bytes
}
19 changes: 19 additions & 0 deletions crypto/ecies/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,14 @@ var (
DefaultCurve = ethcrypto.S256()
ErrUnsupportedECDHAlgorithm = fmt.Errorf("ecies: unsupported ECDH algorithm")
ErrUnsupportedECIESParameters = fmt.Errorf("ecies: unsupported ECIES parameters")
ErrInvalidKeyLen = fmt.Errorf("ecies: invalid key size (> %d) in ECIESParams", maxKeyLen)
)

// KeyLen is limited to prevent overflow of the counter
// in concatKDF. While the theoretical limit is much higher,
// no known cipher uses keys larger than 512 bytes.
const maxKeyLen = 512

type ECIESParams struct {
Hash func() hash.Hash // hash function
hashAlgo crypto.Hash
Expand Down Expand Up @@ -115,3 +121,16 @@ func AddParamsForCurve(curve elliptic.Curve, params *ECIESParams) {
func ParamsFromCurve(curve elliptic.Curve) (params *ECIESParams) {
return paramsFromCurve[curve]
}

func pubkeyParams(key *PublicKey) (*ECIESParams, error) {
params := key.Params
if params == nil {
if params = ParamsFromCurve(key.Curve); params == nil {
return nil, ErrUnsupportedECIESParameters
}
}
if params.KeyLen > maxKeyLen {
return nil, ErrInvalidKeyLen
}
return params, nil
}