Skip to content
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.

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

Problems with ECDH deriveBits/deriveKey operations #193

Open
NeilMadden opened this issue Dec 12, 2017 · 13 comments
Open

Problems with ECDH deriveBits/deriveKey operations #193

NeilMadden opened this issue Dec 12, 2017 · 13 comments

Comments

@NeilMadden
Copy link

The ECDH deriveBits operation is defined as just returning the raw bits of the secret that is output from the ECDH primitive operation (converted to an octet string). It is not clear from the text whether the compact output of RFC6090 Section 4 is being used (this should be clarified), but that is typical. There are at least two issues with using this value directly as the output of deriveBits:

  1. It is limited to outputting only as many bits as there are in the x-coordinate of the point, e.g. for P-256 then this is 256 bits maximum.
  2. Those bits are not uniformly distributed (some x-coordinates have multiple points on the curve, others have 1 or none), and so not suitable to be used directly for cryptographic purposes.

Point 2 is quite serious as the deriveKey operation is defined as taking the raw output of deriveBits and feeding it into the importKey of the intended derived key algorithm. This is completely unsuitable for most of those key types. This appears in examples such as https://github.com/diafygi/webcrypto-examples#ecdh---derivekey which feeds the output of ECDH deriveKey into creating an AES-CTR key. The resulting key will be biased, which is absolutely unacceptable for a cryptographic key.

The only safe key type to output the ECDH secret into would be HKDF, which will convert the raw secret into a uniformly-distributed bit sequence (of any length!). It also the allows mixing in crucial context information, such as the public keys involved in the key agreement, which is an essential step in authentication protocols built on top of ECDH.

In my opinion, the deriveBits operation should be specified to produce uniformly distributed bit output. The ECDH algorithm should then not be defined with deriveBits. Instead it should only have deriveKey and be further restricted to only derive keys that have usage deriveBits (and optionally also deriveKey) - i.e., basically just HKDF or PBKDF2.

If it is too late to change this, then it would be beneficial to attach a note to the ECDH part of the spec recommending strongly that it is only used to derive a HKDF key which can then be used to derive further keys/bits.

@LiraNuna
Copy link

LiraNuna commented Jun 6, 2018

JOSE JWE recommends using CONCAT KDF instead - by making this always output using HKDF you're effectively making ECDH-ES key exchange not support JOSE/JWE.

@NeilMadden
Copy link
Author

Right, I’d be happy with ConcatKDF too. The suggestion is just that the output should always pass through some KDF, and it just happens that WebCrypto only defines HKDF or PBKDF2.

@LiraNuna
Copy link

LiraNuna commented Jun 8, 2018

You can implement HKDF and CONCAT using primitives available in webcrypto. For example, here's my implementation of CONCAT using webcrypto and TypeScript, made for use with JWE for ECDH-ES:

async function genConcatKDF(
    Z: ArrayBuffer | ArrayBufferView,
    algorithmID: JwaEncAlg,
    keyLengthBits: number,
    partyUInfo: Uint8Array = new Uint8Array([]),
    partyVInfo: Uint8Array = new Uint8Array([]),
): Promise<Uint8Array> {
    function be32bitInteger(n: number): Uint8Array {
        const encoded = new Uint8Array(4);
        new DataView(encoded.buffer).setInt32(0, n);
        return encoded;
    }

    function lengthPrefixed(data: Uint8Array): Uint8Array {
        return concatBuffers(be32bitInteger(data.byteLength), data);
    }

    const commonBody = concatBuffers(
        Z,
        lengthPrefixed(encodeToBytes(algorithmID)),
        lengthPrefixed(partyUInfo),
        lengthPrefixed(partyVInfo),
        be32bitInteger(keyLengthBits),
    );

    const rounds = Math.round(keyLengthBits / 256);
    const prehashedChunks = Array.from(new Array(rounds), function(_, round: number): Uint8Array {
        return concatBuffers(be32bitInteger(round + 1), commonBody);
    });

    const chunks = await Promise.all(prehashedChunks.map(async function(chunk: Uint8Array): Promise<Uint8Array> {
        const hashed = await subtle.digest({name: 'SHA-256'}, chunk);
        return new Uint8Array(hashed);
    }));

    return concatBuffers(...chunks).slice(0, keyLengthBits / 8);
}

@NeilMadden
Copy link
Author

Yes, you can. (I wrote the ECDH-ES implementation for my employer's JOSE library). You can implement HMAC-SHA-256 from a SHA-256 primitive too. But you generally shouldn't. Implementing a standard KDF is at the lower end of "don't roll your own crypto", but there are still many things you can get wrong. Ideally as soon as an ECDH key agreement is done, the shared secret should be fed into a KDF and then immediately scrubbed from memory.

I don't think a good API design should make it so easy to do the wrong thing (using an ECDH shared secret directly as a key) just in case somebody wants to use a custom KDF. I'd rather that WebCrypto added the few KDF's in common use, or provided an alternative API to write your own, or provided an unsafeRawSecretKDF that returns the raw bytes but makes it clear that you are doing something unusual.

@LiraNuna
Copy link

LiraNuna commented Jun 8, 2018

Oh, I totally agree with you - I was trying to be friendly and provide some code :)
I believe CONCAT used to be in webcrypto, then later removed. I suspect HKDF would suffer a similar fate since currently no browser implements it

@NeilMadden
Copy link
Author

I’m not sure about the comment about HKDF. Certainly Chrome implements it: https://www.chromium.org/blink/webcrypto#TOC-Supported-algorithms-as-of-Chrome-53-

@LiraNuna
Copy link

LiraNuna commented Jun 8, 2018

I don't think that page is correct. Visiting https://diafygi.github.io/webcrypto-examples/ using latest chrome shows HKDF-CTR as not supported. Same with latest Firefox.

@NeilMadden
Copy link
Author

HKDF-CTR was dropped from the spec in favour of plain HKDF. diafygi/webcrypto-examples#46

@LiraNuna
Copy link

LiraNuna commented Jun 8, 2018

Oh! My bad! I thought they were the same thing!

@jimsch
Copy link
Collaborator

jimsch commented Jun 10, 2018

The group of people who originally authored the document look at this issue extensively. The big two reasons that I can remember are a) what do you do if your KDF function is not supported - without getting the bits then you cannot ever do a key agreement. b) There are cases where you want to derive multiple keys, or things which are not keys, from the same key agreement answer. The ability to do this without having to have multiple access to the user's private key (with potential UI) is considered to be a good thing to do.

This decision is reflected in the fact that ECDH only supports dervieBits and not deriveKey.

@LiraNuna
Copy link

This decision is reflected in the fact that ECDH only supports dervieBits and not deriveKey.

Interesting. Both Chrome and Firefox support deriveKey for ECDH. Is that an off-spec extention behavior?

@jimsch
Copy link
Collaborator

jimsch commented Jun 10, 2018

Arggggg. No its not. It has just been too long. deriveKey is written in terms of deriveBits. That means that it is not listed as a supported operation for ECDH, but gets indirectly supported. Stupid way to write the document.

@themikefuller
Copy link

As @jimsch pointed out, the deriveKey function is based on the deriveBits function. The reason that you may want to use deriveBits instead of deriveKey is that you may want to further iterate over (stretch) the shared secret (byteArray) before you import the bits as a shared key.

An example:

var mike = await crypto.subtle.generateKey({
"name":"ECDH",
"namedCurve":"P-256"
},true,['deriveBits']);

var ghost = await crypto.subtle.generateKey({
"name":"ECDH",
"namedCurve":"P-256"
},true,['deriveBits']);

var sharedBits = await crypto.subtle.deriveBits({
"name":"ECDH",
"public":ghost.publicKey
},mike.privateKey,256);

var salt = new Uint8Array(sharedBits).slice(0,16);
var password = new Uint8Array(sharedBits).slice(16,32);

var passKey = await crypto.subtle.importKey('raw',password,{
"name":"PBKDF2"
},false,['deriveBits', 'deriveKey']);

var sharedKey = crypto.subtle.deriveKey({
"name":"PBKDF2",
"salt": salt,
"iterations": 100000,
"hash": {"name":"SHA-256"}
},passKey,{
"name":"AES-GCM",
"length": 256
},false,['encrypt','decrypt']);

You could also use deriveBits (instead of deriveKey) and then import those bits as a raw key, which is essentially what the deriveKey function is doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants