w3c / webcrypto

The W3C Web Cryptography API
https://w3c.github.io/webcrypto/
Other
266 stars 55 forks source link

Problems with ECDH deriveBits/deriveKey operations #193

Open NeilMadden opened 6 years ago

NeilMadden commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

HKDF-CTR was dropped from the spec in favour of plain HKDF. https://github.com/diafygi/webcrypto-examples/issues/46

LiraNuna commented 6 years ago

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

jimsch commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.