From dd1e03304e96fe8b8d5221626918d9250c392b92 Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Mon, 20 Jul 2020 14:06:50 +0000 Subject: [PATCH] Bug 1652609 - When generating a JWE, include `kid` of the target key if known. r=m_and_m,lina:m_and_m,:lina Differential Revision: https://phabricator.services.mozilla.com/D83426 --- services/crypto/modules/jwcrypto.jsm | 30 +++++++++++++-------- services/crypto/tests/unit/test_jwcrypto.js | 29 +++++++++++++++----- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/services/crypto/modules/jwcrypto.jsm b/services/crypto/modules/jwcrypto.jsm index bcfa3ce150d4..61f7d136e3c8 100644 --- a/services/crypto/modules/jwcrypto.jsm +++ b/services/crypto/modules/jwcrypto.jsm @@ -63,8 +63,6 @@ class JWCrypto { * from https://tools.ietf.org/html/rfc7516. The only supported content encryption * algorithm is enc="A256GCM" [1] and the only supported key encryption algorithm * is alg="ECDH-ES" [2]. - * The IV is generated randomly: if you are using long-lived keys you might be - * exposing yourself to a birthday attack. Please consult your nearest cryptographer. * * @param {Object} key Peer Public JWK. * @param {ArrayBuffer} data @@ -76,9 +74,18 @@ class JWCrypto { */ async generateJWE(key, data) { // Generate an ephemeral key to use just for this encryption. + // The public component gets embedded in the JWE header. const epk = await crypto.subtle.generateKey(ECDH_PARAMS, true, [ "deriveKey", ]); + const ownPublicJWK = await crypto.subtle.exportKey("jwk", epk.publicKey); + // Remove properties added by our WebCrypto implementation but that aren't typically + // used with JWE in the wild. This saves space in the resulting JWE, and makes it easier + // to re-import the resulting JWK. + delete ownPublicJWK.key_ops; + delete ownPublicJWK.ext; + let header = { alg: "ECDH-ES", enc: "A256GCM", epk: ownPublicJWK }; + // Import the peer's public key. const peerPublicKey = await crypto.subtle.importKey( "jwk", key, @@ -86,20 +93,20 @@ class JWCrypto { false, ["deriveKey"] ); - return this._generateJWE(epk, peerPublicKey, data); - } - - async _generateJWE(epk, peerPublicKey, data) { - let iv = crypto.getRandomValues(new Uint8Array(AES_GCM_IV_SIZE)); - const ownPublicJWK = await crypto.subtle.exportKey("jwk", epk.publicKey); - delete ownPublicJWK.key_ops; + if (key.hasOwnProperty("kid")) { + header.kid = key.kid; + } // Do ECDH agreement to get the content encryption key. const contentKey = await deriveECDHSharedAESKey( epk.privateKey, peerPublicKey, ["encrypt"] ); - let header = { alg: "ECDH-ES", enc: "A256GCM", epk: ownPublicJWK }; + // Encrypt with AES-GCM using the generated key. + // Note that the IV is generated randomly, which *in general* is not safe to do with AES-GCM because + // it's too short to guarantee uniqueness. But we know that the AES-GCM key itself is unique and will + // only be used for this single encryption, making a random IV safe to use for this particular use-case. + let iv = crypto.getRandomValues(new Uint8Array(AES_GCM_IV_SIZE)); // Yes, additionalData is the byte representation of the base64 representation of the stringified header. const additionalData = UTF8_ENCODER.encode( ChromeUtils.base64URLEncode(UTF8_ENCODER.encode(JSON.stringify(header)), { @@ -116,10 +123,11 @@ class JWCrypto { contentKey, data ); + // JWE needs the authentication tag as a separate string. const tagIdx = encrypted.byteLength - ((AES_TAG_LEN + 7) >> 3); let ciphertext = encrypted.slice(0, tagIdx); let tag = encrypted.slice(tagIdx); - // JWE serialization. + // JWE serialization in compact format. header = UTF8_ENCODER.encode(JSON.stringify(header)); header = ChromeUtils.base64URLEncode(header, { pad: false }); tag = ChromeUtils.base64URLEncode(tag, { pad: false }); diff --git a/services/crypto/tests/unit/test_jwcrypto.js b/services/crypto/tests/unit/test_jwcrypto.js index 9c90fa7a205e..2c51395bb3fc 100644 --- a/services/crypto/tests/unit/test_jwcrypto.js +++ b/services/crypto/tests/unit/test_jwcrypto.js @@ -43,8 +43,8 @@ const generateKeyPair = promisify(jwcrypto.generateKeyPair); const generateAssertion = promisify(jwcrypto.generateAssertion); add_task(async function test_jwe_roundtrip_ecdh_es_encryption() { - const data = crypto.getRandomValues(new Uint8Array(123)); - const localEpk = await crypto.subtle.generateKey( + const plaintext = crypto.getRandomValues(new Uint8Array(123)); + const remoteKey = await crypto.subtle.generateKey( { name: "ECDH", namedCurve: "P-256", @@ -52,7 +52,16 @@ add_task(async function test_jwe_roundtrip_ecdh_es_encryption() { true, ["deriveKey"] ); - const remoteEpk = await crypto.subtle.generateKey( + const remoteJWK = await crypto.subtle.exportKey("jwk", remoteKey.publicKey); + delete remoteJWK.key_ops; + const jwe = await jwcrypto.generateJWE(remoteJWK, plaintext); + const decrypted = await jwcrypto.decryptJWE(jwe, remoteKey.privateKey); + Assert.deepEqual(plaintext, decrypted); +}); + +add_task(async function test_jwe_header_includes_key_id() { + const plaintext = crypto.getRandomValues(new Uint8Array(123)); + const remoteKey = await crypto.subtle.generateKey( { name: "ECDH", namedCurve: "P-256", @@ -60,9 +69,17 @@ add_task(async function test_jwe_roundtrip_ecdh_es_encryption() { true, ["deriveKey"] ); - const jwe = await jwcrypto._generateJWE(localEpk, remoteEpk.publicKey, data); - const decryptedJWE = await jwcrypto.decryptJWE(jwe, remoteEpk.privateKey); - Assert.deepEqual(data, decryptedJWE); + const remoteJWK = await crypto.subtle.exportKey("jwk", remoteKey.publicKey); + delete remoteJWK.key_ops; + remoteJWK.kid = "key identifier"; + const jwe = await jwcrypto.generateJWE(remoteJWK, plaintext); + let [header /* other items deliberately ignored */] = jwe.split("."); + header = JSON.parse( + new TextDecoder().decode( + ChromeUtils.base64URLDecode(header, { padding: "reject" }) + ) + ); + Assert.equal(header.kid, "key identifier"); }); add_task(async function test_sanity() {