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 11 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
92 changes: 28 additions & 64 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 Down Expand Up @@ -138,57 +139,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

@holiman holiman Apr 1, 2020

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

@fjl fjl Apr 1, 2020

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

@holiman holiman Apr 2, 2020

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

@fjl fjl Apr 2, 2020

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 nearest 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 Down Expand Up @@ -266,15 +249,7 @@ func Encrypt(rand io.Reader, pub *PublicKey, m, s1, s2 []byte) (ct []byte, err e
if err != nil {
return
}
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)

em, err := symEncrypt(rand, params, Ke, m)
if err != nil || len(em) <= params.BlockSize {
Expand Down Expand Up @@ -341,26 +316,15 @@ func (prv *PrivateKey) Decrypt(c, s1, s2 []byte) (m []byte, err error) {

z, err := prv.GenerateShared(R, 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()
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
}

m, err = symDecrypt(params, Ke, c[mStart:mEnd])
return
}
36 changes: 25 additions & 11 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 @@ -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
}