From 8ee1cfc4b91d1812439ac6607bac5144e7de3f5f Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Wed, 17 Nov 2021 16:41:51 +0100 Subject: [PATCH 1/8] WIP --- src/config/config.js | 3 + src/crypto/pkcs1.js | 31 ++++-- src/message.js | 104 +++++++++++++----- .../public_key_encrypted_session_key.js | 20 +++- 4 files changed, 116 insertions(+), 42 deletions(-) diff --git a/src/config/config.js b/src/config/config.js index 035243c9f..adbb3a4a2 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -140,6 +140,9 @@ export default { */ allowInsecureVerificationWithReformattedKeys: false, + constantTimePKCS1Decryption: false, + // TODO add option to specify ciphers to decrypt with (default to AES ones) + /** * @memberof module:config * @property {Integer} minBytesForWebCrypto The minimum amount of bytes for which to use native WebCrypto APIs when available diff --git a/src/crypto/pkcs1.js b/src/crypto/pkcs1.js index 7773b62cf..ac6a48c75 100644 --- a/src/crypto/pkcs1.js +++ b/src/crypto/pkcs1.js @@ -98,18 +98,31 @@ export async function emeEncode(message, keyLength) { * Decode a EME-PKCS1-v1_5 padded message * @see {@link https://tools.ietf.org/html/rfc4880#section-13.1.2|RFC 4880 13.1.2} * @param {Uint8Array} encoded - Encoded message bytes - * @returns {Uint8Array} Message. + * @param {Uint8Array} randomPayload - Data to return in case of decoding error (needed for constant-time processing) + * @returns {Uint8Array} decoded data or `randomPayload` (on error, if given) + * @throws {Error} on decoding failure, unless `randomPayload` is provided */ -export function emeDecode(encoded) { - let i = 2; - while (encoded[i] !== 0 && i < encoded.length) { - i++; +export function emeDecode(encoded, randomPayload) { + // encoded format: 0x00 0x02 0x00 + let offset = 2; + let separatorNotFound = 1; + for (let j = offset; j < encoded.length; j++) { + separatorNotFound &= encoded[j] !== 0; + offset += separatorNotFound; } - const psLen = i - 2; - const separator = encoded[i++]; - if (encoded[0] === 0 && encoded[1] === 2 && psLen >= 8 && separator === 0) { - return encoded.subarray(i); + + const psLen = offset - 2; + const payload = encoded.subarray(offset + 1); // discar the 0x00 separator + const isValidPadding = encoded[0] === 0 & encoded[1] === 2 & psLen >= 8 & !separatorNotFound; + + if (randomPayload) { + return isValidPadding ? payload : randomPayload; } + + if (isValidPadding) { + return payload; + } + throw new Error('Decryption error'); } diff --git a/src/message.js b/src/message.js index d94c6e809..8fb570754 100644 --- a/src/message.js +++ b/src/message.js @@ -107,7 +107,7 @@ export class Message { * @async */ async decrypt(decryptionKeys, passwords, sessionKeys, date = new Date(), config = defaultConfig) { - const sessionKeyObjs = sessionKeys || await this.decryptSessionKeys(decryptionKeys, passwords, date, config); + const sessionKeyObjects = sessionKeys || await this.decryptSessionKeys(decryptionKeys, passwords, date, config); const symEncryptedPacketlist = this.packets.filterByTag( enums.packet.symmetricallyEncryptedData, @@ -121,7 +121,7 @@ export class Message { const symEncryptedPacket = symEncryptedPacketlist[0]; let exception = null; - const decryptedPromise = Promise.all(sessionKeyObjs.map(async ({ algorithm: algorithmName, data }) => { + const decryptedPromise = Promise.all(sessionKeyObjects.map(async ({ algorithm: algorithmName, data }) => { if (!util.isUint8Array(data) || !util.isString(algorithmName)) { throw new Error('Invalid session key for decryption.'); } @@ -162,36 +162,36 @@ export class Message { * @async */ async decryptSessionKeys(decryptionKeys, passwords, date = new Date(), config = defaultConfig) { - let keyPackets = []; + let decryptedSessionKeyPackets = []; let exception; if (passwords) { - const symESKeyPacketlist = this.packets.filterByTag(enums.packet.symEncryptedSessionKey); - if (symESKeyPacketlist.length === 0) { + const skeskPackets = this.packets.filterByTag(enums.packet.symEncryptedSessionKey); + if (skeskPackets.length === 0) { throw new Error('No symmetrically encrypted session key packet found.'); } await Promise.all(passwords.map(async function(password, i) { let packets; if (i) { - packets = await PacketList.fromBinary(symESKeyPacketlist.write(), allowedSymSessionKeyPackets, config); + packets = await PacketList.fromBinary(skeskPackets.write(), allowedSymSessionKeyPackets, config); } else { - packets = symESKeyPacketlist; + packets = skeskPackets; } - await Promise.all(packets.map(async function(keyPacket) { + await Promise.all(packets.map(async function(skeskPacket) { try { - await keyPacket.decrypt(password); - keyPackets.push(keyPacket); + await skeskPacket.decrypt(password); + decryptedSessionKeyPackets.push(skeskPacket); } catch (err) { util.printDebugError(err); } })); })); } else if (decryptionKeys) { - const pkESKeyPacketlist = this.packets.filterByTag(enums.packet.publicKeyEncryptedSessionKey); - if (pkESKeyPacketlist.length === 0) { + const pkeskPackets = this.packets.filterByTag(enums.packet.publicKeyEncryptedSessionKey); + if (pkeskPackets.length === 0) { throw new Error('No public key encrypted session key packet found.'); } - await Promise.all(pkESKeyPacketlist.map(async function(keyPacket) { + await Promise.all(pkeskPackets.map(async function(pkeskPacket) { await Promise.all(decryptionKeys.map(async function(decryptionKey) { let algos = [ enums.symmetric.aes256, // Old OpenPGP.js default fallback @@ -207,7 +207,7 @@ export class Message { } catch (e) {} // do not check key expiration to allow decryption of old messages - const decryptionKeyPackets = (await decryptionKey.getDecryptionKeys(keyPacket.publicKeyID, null, undefined, config)).map(key => key.keyPacket); + const decryptionKeyPackets = (await decryptionKey.getDecryptionKeys(pkeskPacket.publicKeyID, null, undefined, config)).map(key => key.keyPacket); await Promise.all(decryptionKeyPackets.map(async function(decryptionKeyPacket) { if (!decryptionKeyPacket || decryptionKeyPacket.isDummy()) { return; @@ -215,30 +215,78 @@ export class Message { if (!decryptionKeyPacket.isDecrypted()) { throw new Error('Decryption key is not decrypted.'); } - try { - await keyPacket.decrypt(decryptionKeyPacket); - if (!algos.includes(keyPacket.sessionKeyAlgorithm)) { - throw new Error('A non-preferred symmetric algorithm was used.'); + + // To hinder CCA attacks against RSA PKCS1, we carry out a constant-time decryption flow if the `constantTimePKCS1Decryption` config option is set. + const doConstantTimeDecryption = config.constantTimePKCS1Decryption && ( + pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaEncrypt || + pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaEncryptSign || + pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaSign + ); + + if (doConstantTimeDecryption) { + // The goal is to not reveal whether PKESK decryption failed, hence, we always proceed to decrypt the message, either with the successfully decrypted session key, + // or with a randomly generated one of the same type. + // Since the SEIP/AEAD's symmetric algorithm and key size are stored in the encrypted portion of the PKESK, and the execution flow cannot depend on + // the decrypted payload, we always assume the message to be AES-encrypted, and try to decrypt the message using all possible AES key sizes (i.e. 128, 192, 256): + // - If the PKESK decryption succeeds, and the session key cipher is AES, then we try to decrypt the data with the decrypted session key as well as the + // randomly generated keys of the two remaining key sizes. + // - If the PKESK decryptions fails, or if it succeeds but the cipher is not AES, then we discard the session key and try to decrypt the data using only the randomly + // generated session keys. + // + // NB: if the data is encrypted with a non-AES cipher, decryption will always fail. The code could be extended to support additional non-AES ciphers, + // but that would considerably slow down decryption. + const sessionKeysOfEachAlgo = await Promise.all(['aes128', 'aes192', 'aes256'].map(async algoName => { + const randomPKESK = new PublicKeyEncryptedSessionKeyPacket(); + randomPKESK.publicKeyAlgorithm = pkeskPacket.publicKeyAlgorithm; + randomPKESK.sessionKeyAlgorithm = enums.write(enums.symmetric, algoName); + randomPKESK.sessionKey = await crypto.generateSessionKey(algoName); + return randomPKESK; + })); + + try { + // PKESK decryption is not constant time and it will throw on failure, but the time variability is small enough + await pkeskPacket.decrypt(decryptionKeyPacket); + if (!algos.includes(enums.write(enums.symmetric, pkeskPacket.sessionKeyAlgorithm))) { + throw new Error('A non-preferred symmetric algorithm was used.'); + } + // If the successfully decrypted session key is AES, use it instead of the randomly generated AES key of the same size. + // If it is non-AES, discard it and proceed with only the randomly generated session keys. + + // TODO this can be simplified with pkesk.decrypt being aware of constant timeness: + // - clone pkesk and have it decrypted with each random key passed as `randomSessionKey` + // - use the result as-is, unless errors are thrown, then discard the pkesk (as already done) + const algoIndex = sessionKeysOfEachAlgo.findIndex(({ sessionKeyAlgorithm }) => sessionKeyAlgorithm === pkeskPacket.sessionKeyAlgorithm); + sessionKeysOfEachAlgo[algoIndex] = (algoIndex >= 0) ? pkeskPacket : sessionKeysOfEachAlgo[algoIndex]; + } finally { + decryptedSessionKeyPackets.push(...sessionKeysOfEachAlgo); + } + + } else { + try { + await pkeskPacket.decrypt(decryptionKeyPacket); + if (!algos.includes(enums.write(enums.symmetric, pkeskPacket.sessionKeyAlgorithm))) { + throw new Error('A non-preferred symmetric algorithm was used.'); + } + decryptedSessionKeyPackets.push(pkeskPacket); + } catch (err) { + util.printDebugError(err); + exception = err; } - keyPackets.push(keyPacket); - } catch (err) { - util.printDebugError(err); - exception = err; } })); })); - stream.cancel(keyPacket.encrypted); // Don't keep copy of encrypted data in memory. - keyPacket.encrypted = null; + stream.cancel(pkeskPacket.encrypted); // Don't keep copy of encrypted data in memory. + pkeskPacket.encrypted = null; })); } else { throw new Error('No key or password specified.'); } - if (keyPackets.length) { + if (decryptedSessionKeyPackets.length > 0) { // Return only unique session keys - if (keyPackets.length > 1) { + if (decryptedSessionKeyPackets.length > 1) { const seen = new Set(); - keyPackets = keyPackets.filter(item => { + decryptedSessionKeyPackets = decryptedSessionKeyPackets.filter(item => { const k = item.sessionKeyAlgorithm + util.uint8ArrayToString(item.sessionKey); if (seen.has(k)) { return false; @@ -248,7 +296,7 @@ export class Message { }); } - return keyPackets.map(packet => ({ + return decryptedSessionKeyPackets.map(packet => ({ data: packet.sessionKey, algorithm: enums.read(enums.symmetric, packet.sessionKeyAlgorithm) })); diff --git a/src/packet/public_key_encrypted_session_key.js b/src/packet/public_key_encrypted_session_key.js index c0bedee7f..a22102510 100644 --- a/src/packet/public_key_encrypted_session_key.js +++ b/src/packet/public_key_encrypted_session_key.js @@ -116,19 +116,29 @@ class PublicKeyEncryptedSessionKeyPacket { * @throws {Error} if decryption failed * @async */ - async decrypt(key) { + async decrypt(key, randomSessionKey) { // check that session key algo matches the secret key algo if (this.publicKeyAlgorithm !== key.algorithm) { throw new Error('Decryption error'); } + + // TODO: + // - add algo + checksum to random session key and pass down the data to be used as randomPayload for emeDecode + // - if checksum is valid, also check that decoded.algo and decoded.length match random session key data (ie use it as "expected session key type") + // if yes, use decoded data as session key data + algo, otherwise, use random session key data + algo + // only throw decryption error if random session key is not passed. + // NB: it is safe to throw some errors regardless of random session key presence (eg rsa decryption failure not due to decoding, or unknown pubkey algo etc). const decoded = await crypto.publicKeyDecrypt(this.publicKeyAlgorithm, key.publicParams, key.privateParams, this.encrypted, key.getFingerprintBytes()); const checksum = decoded.subarray(decoded.length - 2); const sessionKey = decoded.subarray(1, decoded.length - 2); - if (!util.equalsUint8Array(checksum, util.writeChecksum(sessionKey))) { - throw new Error('Decryption error'); - } else { + const symmetricAlgoByte = decoded[0]; + + const isValidPayload = util.equalsUint8Array(checksum, util.writeChecksum(sessionKey)) && enums.symmetric[symmetricAlgoByte] !== undefined && enums.symmetric[symmetricAlgoByte] === randomSessionKey.sessionKeyAlgorithm; + if (isValidPayload) { this.sessionKey = sessionKey; - this.sessionKeyAlgorithm = enums.write(enums.symmetric, decoded[0]); + this.sessionKeyAlgorithm = enums.write(enums.symmetric, symmetricAlgoByte); + } else { + throw new Error('Decryption error'); } } } From 8dd495ad7a0b009db6fd7f16b5d3e87ecf64d7d0 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Tue, 30 Nov 2021 18:21:51 +0100 Subject: [PATCH 2/8] Implement constant time decryption flow for RSA and ElGamal PKESK --- openpgp.d.ts | 2 + src/config/config.js | 18 +++- src/crypto/crypto.js | 9 +- src/crypto/public_key/elgamal.js | 7 +- src/crypto/public_key/rsa.js | 18 ++-- src/message.js | 62 ++++++------- .../public_key_encrypted_session_key.js | 40 +++++--- test/general/openpgp.js | 91 +++++++++++++++++++ 8 files changed, 185 insertions(+), 62 deletions(-) diff --git a/openpgp.d.ts b/openpgp.d.ts index ad7497545..c1b7bc831 100644 --- a/openpgp.d.ts +++ b/openpgp.d.ts @@ -326,6 +326,8 @@ interface Config { versionString: string; commentString: string; allowInsecureDecryptionWithSigningKeys: boolean; + constantTimePKCS1Decryption: boolean; + constantTimePKCS1DecryptionSupportedSymmetricAlgorithms: Set; v5Keys: boolean; preferredAEADAlgorithm: enums.aead; aeadChunkSizeByte: number; diff --git a/src/config/config.js b/src/config/config.js index adbb3a4a2..4ed4050a8 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -140,8 +140,24 @@ export default { */ allowInsecureVerificationWithReformattedKeys: false, + /** + * Enable constant-time decryption of RSA- and ElGamal-encrypted session keys, to hinder Bleichenbacher-like attacks (https://link.springer.com/chapter/10.1007/BFb0055716). + * This setting has measurable performance impact and it is only helpful in application scenarios where both of the following conditions apply: + * - new/incoming messages are automatically decrypted (without user interaction); + * - an attacker can determine how long it takes to decrypt each message (e.g. due to decryption errors being logged remotely). + * See also `constantTimePKCS1DecryptionSupportedSymmetricAlgorithms`. + * @memberof module:config + * @property {Boolean} constantTimePKCS1Decryption + */ constantTimePKCS1Decryption: false, - // TODO add option to specify ciphers to decrypt with (default to AES ones) + /** + * This setting is only meaningful if `constantTimePKCS1Decryption` is enabled. + * Decryption of RSA- and ElGamal-encrypted session keys of symmetric algorithms different from the ones specified here will fail. + * However, the more algorithms are added, the slower the decryption procedure becomes. + * @memberof module:config + * @property {Set} constantTimePKCS1DecryptionSupportedSymmetricAlgorithms {@link module:enums.symmetric} + */ + constantTimePKCS1DecryptionSupportedSymmetricAlgorithms: new Set([enums.symmetric.aes128, enums.symmetric.aes192, enums.symmetric.aes256]), /** * @memberof module:config diff --git a/src/crypto/crypto.js b/src/crypto/crypto.js index ed950d43e..498047337 100644 --- a/src/crypto/crypto.js +++ b/src/crypto/crypto.js @@ -76,23 +76,26 @@ export async function publicKeyEncrypt(algo, publicParams, data, fingerprint) { * @param {Object} privateKeyParams - Algorithm-specific private key parameters * @param {Object} sessionKeyParams - Encrypted session key parameters * @param {Uint8Array} fingerprint - Recipient fingerprint + * @param {Uint8Array} [randomPayload] - Data to return on decryption error, instead of throwing + * (needed for constant-time processing in RSA and ElGamal) * @returns {Promise} Decrypted data. + * @throws {Error} on sensitive decryption error, unless `randomPayload` is given * @async */ -export async function publicKeyDecrypt(algo, publicKeyParams, privateKeyParams, sessionKeyParams, fingerprint) { +export async function publicKeyDecrypt(algo, publicKeyParams, privateKeyParams, sessionKeyParams, fingerprint, randomPayload) { switch (algo) { case enums.publicKey.rsaEncryptSign: case enums.publicKey.rsaEncrypt: { const { c } = sessionKeyParams; const { n, e } = publicKeyParams; const { d, p, q, u } = privateKeyParams; - return publicKey.rsa.decrypt(c, n, e, d, p, q, u); + return publicKey.rsa.decrypt(c, n, e, d, p, q, u, randomPayload); } case enums.publicKey.elgamal: { const { c1, c2 } = sessionKeyParams; const p = publicKeyParams.p; const x = privateKeyParams.x; - return publicKey.elgamal.decrypt(c1, c2, p, x); + return publicKey.elgamal.decrypt(c1, c2, p, x, randomPayload); } case enums.publicKey.ecdh: { const { oid, Q, kdfParams } = publicKeyParams; diff --git a/src/crypto/public_key/elgamal.js b/src/crypto/public_key/elgamal.js index 5819324fd..6969ef00a 100644 --- a/src/crypto/public_key/elgamal.js +++ b/src/crypto/public_key/elgamal.js @@ -59,10 +59,13 @@ export async function encrypt(data, p, g, y) { * @param {Uint8Array} c2 * @param {Uint8Array} p * @param {Uint8Array} x + * @param {Uint8Array} randomPayload - Data to return on unpadding error, instead of throwing + * (needed for constant-time processing) * @returns {Promise} Unpadded message. + * @throws {Error} on decryption error, unless `randomPayload` is given * @async */ -export async function decrypt(c1, c2, p, x) { +export async function decrypt(c1, c2, p, x, randomPayload) { const BigInteger = await util.getBigInteger(); c1 = new BigInteger(c1); c2 = new BigInteger(c2); @@ -70,7 +73,7 @@ export async function decrypt(c1, c2, p, x) { x = new BigInteger(x); const padded = c1.modExp(x, p).modInv(p).imul(c2).imod(p); - return emeDecode(padded.toUint8Array('be', p.byteLength())); + return emeDecode(padded.toUint8Array('be', p.byteLength()), randomPayload); } /** diff --git a/src/crypto/public_key/rsa.js b/src/crypto/public_key/rsa.js index 72f2fe835..4ad34d37c 100644 --- a/src/crypto/public_key/rsa.js +++ b/src/crypto/public_key/rsa.js @@ -133,14 +133,17 @@ export async function encrypt(data, n, e) { * @param {Uint8Array} p - RSA private prime p * @param {Uint8Array} q - RSA private prime q * @param {Uint8Array} u - RSA private coefficient + * @param {Uint8Array} randomPayload - Data to return on decryption error, instead of throwing + * (needed for constant-time processing) * @returns {Promise} RSA Plaintext. + * @throws {Error} on decryption error, unless `randomPayload` is given * @async */ -export async function decrypt(data, n, e, d, p, q, u) { +export async function decrypt(data, n, e, d, p, q, u, randomPayload) { if (util.getNodeCrypto()) { - return nodeDecrypt(data, n, e, d, p, q, u); + return nodeDecrypt(data, n, e, d, p, q, u, randomPayload); } - return bnDecrypt(data, n, e, d, p, q, u); + return bnDecrypt(data, n, e, d, p, q, u, randomPayload); } /** @@ -439,7 +442,7 @@ async function bnEncrypt(data, n, e) { return data.modExp(e, n).toUint8Array('be', n.byteLength()); } -async function nodeDecrypt(data, n, e, d, p, q, u) { +async function nodeDecrypt(data, n, e, d, p, q, u, randomPayload) { const { default: BN } = await import('bn.js'); const pBNum = new BN(p); @@ -473,11 +476,14 @@ async function nodeDecrypt(data, n, e, d, p, q, u) { try { return new Uint8Array(nodeCrypto.privateDecrypt(key, data)); } catch (err) { + if (randomPayload) { + return randomPayload; + } throw new Error('Decryption error'); } } -async function bnDecrypt(data, n, e, d, p, q, u) { +async function bnDecrypt(data, n, e, d, p, q, u, randomPayload) { const BigInteger = await util.getBigInteger(); data = new BigInteger(data); n = new BigInteger(n); @@ -506,7 +512,7 @@ async function bnDecrypt(data, n, e, d, p, q, u) { result = result.mul(unblinder).mod(n); - return emeDecode(result.toUint8Array('be', n.byteLength())); + return emeDecode(result.toUint8Array('be', n.byteLength()), randomPayload); } /** Convert Openpgp private key params to jwk key according to diff --git a/src/message.js b/src/message.js index 8fb570754..ed9d60dd0 100644 --- a/src/message.js +++ b/src/message.js @@ -216,50 +216,42 @@ export class Message { throw new Error('Decryption key is not decrypted.'); } - // To hinder CCA attacks against RSA PKCS1, we carry out a constant-time decryption flow if the `constantTimePKCS1Decryption` config option is set. + // To hinder CCA attacks against PKCS1, we carry out a constant-time decryption flow if the `constantTimePKCS1Decryption` config option is set. const doConstantTimeDecryption = config.constantTimePKCS1Decryption && ( pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaEncrypt || pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaEncryptSign || - pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaSign + pkeskPacket.publicKeyAlgorithm === enums.publicKey.rsaSign || + pkeskPacket.publicKeyAlgorithm === enums.publicKey.elgamal ); if (doConstantTimeDecryption) { - // The goal is to not reveal whether PKESK decryption failed, hence, we always proceed to decrypt the message, either with the successfully decrypted session key, - // or with a randomly generated one of the same type. + // The goal is to not reveal whether PKESK decryption (specifically the PKCS1 decoding step) failed, hence, we always proceed to decrypt the message, + // either with the successfully decrypted session key, or with a randomly generated one of the same type. // Since the SEIP/AEAD's symmetric algorithm and key size are stored in the encrypted portion of the PKESK, and the execution flow cannot depend on - // the decrypted payload, we always assume the message to be AES-encrypted, and try to decrypt the message using all possible AES key sizes (i.e. 128, 192, 256): - // - If the PKESK decryption succeeds, and the session key cipher is AES, then we try to decrypt the data with the decrypted session key as well as the - // randomly generated keys of the two remaining key sizes. - // - If the PKESK decryptions fails, or if it succeeds but the cipher is not AES, then we discard the session key and try to decrypt the data using only the randomly + // the decrypted payload, we always assume the message to be encrypted with one of the symmetric algorithms specified in `config.constantTimePKCS1DecryptionSupportedSymmetricAlgorithms`: + // - If the PKESK decryption succeeds, and the session key cipher is in the supported set, then we try to decrypt the data with the decrypted session key as well as the + // randomly generated keys of the remaining key types. + // - If the PKESK decryptions fails, or if it succeeds but support for the cipher is not enabled, then we discard the session key and try to decrypt the data using only the randomly // generated session keys. - // - // NB: if the data is encrypted with a non-AES cipher, decryption will always fail. The code could be extended to support additional non-AES ciphers, - // but that would considerably slow down decryption. - const sessionKeysOfEachAlgo = await Promise.all(['aes128', 'aes192', 'aes256'].map(async algoName => { - const randomPKESK = new PublicKeyEncryptedSessionKeyPacket(); - randomPKESK.publicKeyAlgorithm = pkeskPacket.publicKeyAlgorithm; - randomPKESK.sessionKeyAlgorithm = enums.write(enums.symmetric, algoName); - randomPKESK.sessionKey = await crypto.generateSessionKey(algoName); - return randomPKESK; - })); - - try { - // PKESK decryption is not constant time and it will throw on failure, but the time variability is small enough - await pkeskPacket.decrypt(decryptionKeyPacket); - if (!algos.includes(enums.write(enums.symmetric, pkeskPacket.sessionKeyAlgorithm))) { - throw new Error('A non-preferred symmetric algorithm was used.'); + // NB: as a result, if the data is encrypted with a non-suported cipher, decryption will always fail. + + const serialisedPKESK = pkeskPacket.write(); // make copies to be able to decrypt the PKESK packet multiple times + await Promise.all(Array.from(config.constantTimePKCS1DecryptionSupportedSymmetricAlgorithms).map(async sessionKeyAlgorithm => { + const pkeskPacketCopy = new PublicKeyEncryptedSessionKeyPacket(); + pkeskPacketCopy.read(serialisedPKESK); + const randomSessionKey = { + sessionKeyAlgorithm, + sessionKey: await crypto.generateSessionKey(sessionKeyAlgorithm) + }; + try { + await pkeskPacketCopy.decrypt(decryptionKeyPacket, randomSessionKey); + decryptedSessionKeyPackets.push(pkeskPacketCopy); + } catch (err) { + // `decrypt` can still throw some non-security-sensitive errors + util.printDebugError(err); + exception = err; } - // If the successfully decrypted session key is AES, use it instead of the randomly generated AES key of the same size. - // If it is non-AES, discard it and proceed with only the randomly generated session keys. - - // TODO this can be simplified with pkesk.decrypt being aware of constant timeness: - // - clone pkesk and have it decrypted with each random key passed as `randomSessionKey` - // - use the result as-is, unless errors are thrown, then discard the pkesk (as already done) - const algoIndex = sessionKeysOfEachAlgo.findIndex(({ sessionKeyAlgorithm }) => sessionKeyAlgorithm === pkeskPacket.sessionKeyAlgorithm); - sessionKeysOfEachAlgo[algoIndex] = (algoIndex >= 0) ? pkeskPacket : sessionKeysOfEachAlgo[algoIndex]; - } finally { - decryptedSessionKeyPackets.push(...sessionKeysOfEachAlgo); - } + })); } else { try { diff --git a/src/packet/public_key_encrypted_session_key.js b/src/packet/public_key_encrypted_session_key.js index a22102510..645b3c947 100644 --- a/src/packet/public_key_encrypted_session_key.js +++ b/src/packet/public_key_encrypted_session_key.js @@ -110,10 +110,11 @@ class PublicKeyEncryptedSessionKeyPacket { } /** - * Decrypts the session key (only for public key encrypted session key - * packets (tag 1) + * Decrypts the session key (only for public key encrypted session key packets (tag 1) * @param {SecretKeyPacket} key - decrypted private key - * @throws {Error} if decryption failed + * @param {Object} [randomSessionKey] - Bogus session key to use in case of sensitive decryption error, or if the decrypted session key is of a different type/size. + * This is needed for constant-time processing. Expected object of the form: { sessionKey: Uint8Array, sessionKeyAlgorithm: enums.symmetric } + * @throws {Error} if decryption failed, unless `randomSessionKey` is given * @async */ async decrypt(key, randomSessionKey) { @@ -122,23 +123,32 @@ class PublicKeyEncryptedSessionKeyPacket { throw new Error('Decryption error'); } - // TODO: - // - add algo + checksum to random session key and pass down the data to be used as randomPayload for emeDecode - // - if checksum is valid, also check that decoded.algo and decoded.length match random session key data (ie use it as "expected session key type") - // if yes, use decoded data as session key data + algo, otherwise, use random session key data + algo - // only throw decryption error if random session key is not passed. - // NB: it is safe to throw some errors regardless of random session key presence (eg rsa decryption failure not due to decoding, or unknown pubkey algo etc). - const decoded = await crypto.publicKeyDecrypt(this.publicKeyAlgorithm, key.publicParams, key.privateParams, this.encrypted, key.getFingerprintBytes()); + const randomPayload = randomSessionKey ? util.concatUint8Array([ + new Uint8Array([randomSessionKey.sessionKeyAlgorithm]), + randomSessionKey.sessionKey, + util.writeChecksum(randomSessionKey.sessionKey) + ]) : null; + const decoded = await crypto.publicKeyDecrypt(this.publicKeyAlgorithm, key.publicParams, key.privateParams, this.encrypted, key.getFingerprintBytes(), randomPayload); const checksum = decoded.subarray(decoded.length - 2); const sessionKey = decoded.subarray(1, decoded.length - 2); const symmetricAlgoByte = decoded[0]; + const isValidChecksum = util.equalsUint8Array(checksum, util.writeChecksum(sessionKey)); + + if (randomSessionKey) { + // We must not leak info about the validity of the decrypted checksum or cipher algo. + // The decrypted session key must be of the same algo and size as the random session key, otherwise we discard it and use the random data. + const isValidPayload = isValidChecksum & symmetricAlgoByte === randomSessionKey.sessionKeyAlgorithm & sessionKey.length === randomSessionKey.sessionKey.length; + this.sessionKey = isValidPayload ? sessionKey : randomSessionKey.sessionKey; + this.sessionKeyAlgorithm = isValidPayload ? symmetricAlgoByte : randomSessionKey.sessionKeyAlgorithm; - const isValidPayload = util.equalsUint8Array(checksum, util.writeChecksum(sessionKey)) && enums.symmetric[symmetricAlgoByte] !== undefined && enums.symmetric[symmetricAlgoByte] === randomSessionKey.sessionKeyAlgorithm; - if (isValidPayload) { - this.sessionKey = sessionKey; - this.sessionKeyAlgorithm = enums.write(enums.symmetric, symmetricAlgoByte); } else { - throw new Error('Decryption error'); + const isValidPayload = isValidChecksum && enums.read(enums.symmetric, symmetricAlgoByte); + if (isValidPayload) { + this.sessionKey = sessionKey; + this.sessionKeyAlgorithm = symmetricAlgoByte; + } else { + throw new Error('Decryption error'); + } } } } diff --git a/test/general/openpgp.js b/test/general/openpgp.js index acdad24b8..9c571b774 100644 --- a/test/general/openpgp.js +++ b/test/general/openpgp.js @@ -833,6 +833,22 @@ Be4ubVrj5KjhX2PVNEJd3XZRzaXZE2aAMQ== =ZeAz -----END PGP PUBLIC KEY BLOCK-----`; +const eccPrivateKey = `-----BEGIN PGP PRIVATE KEY BLOCK----- + +xVgEYaYskRYJKwYBBAHaRw8BAQdAlHT6jzgvcng/qDvb+LH+nA4+AWrMLUYf +aNJIuJRUjXMAAP9llTr5+fNSY78FNnpx53muMtyeDINkeUGGwgqAfxj9lhEV +zRN0ZXN0IDx0ZXN0QHRlc3QuaXQ+wowEEBYKAB0FAmGmLJEECwkHCAMVCAoE +FgACAQIZAQIbAwIeAQAhCRBvJAzR+vGyExYhBCaNeWwMzRW97WhAq28kDNH6 +8bITWWkA/0R3zADs94dVo+iSNzrtZaDkbHOMb/yjketYmI0XS8UpAP4hUmKN +QcohP6007t0gaQUcgdwum7PKUoM6BeBG8GaTAsddBGGmLJESCisGAQQBl1UB +BQEBB0CibQAv6tvWCWoe6xlkkZGbLpVWvHwgIPzRVdz4e79DdQMBCAcAAP9T +4SntnkgSUnM39dFoTPIoitrsOcHZbvXPCcvclKgZKBJTwngEGBYIAAkFAmGm +LJECGwwAIQkQbyQM0frxshMWIQQmjXlsDM0Vve1oQKtvJAzR+vGyE5ORAQD+ +lfFvJjue+tnuIR+ZubxtpKaJpCOWkAcrkx41NtsLwgD/TAkWh1KDWg0IOcUE +MbVkSnU2Z+vhSmYubDCldNOSVwE= +=bTUQ +-----END PGP PRIVATE KEY BLOCK-----`; + function withCompression(tests) { const compressionTypes = Object.values(openpgp.enums.compression); @@ -1461,6 +1477,81 @@ aOU= Object.assign(openpgp.config, { rejectMessageHashAlgorithms }); } }); + + it('decrypt with `config.constantTimePKCS1Decryption` option should succeed', async function () { + const publicKey = await openpgp.readKey({ armoredKey: pub_key }); + const publicKey2 = await openpgp.readKey({ armoredKey: eccPrivateKey }); + const privateKey = await openpgp.decryptKey({ + privateKey: await openpgp.readKey({ armoredKey: priv_key }), + passphrase + }); + + const encrypted = await openpgp.encrypt({ + message: await openpgp.createMessage({ text: plaintext }), + signingKeys: privateKey, + encryptionKeys: [publicKey, publicKey2] + }); + const { data } = await openpgp.decrypt({ + message: await openpgp.readMessage({ armoredMessage: encrypted }), + decryptionKeys: privateKey, + config: { constantTimePKCS1Decryption: true } + }); + expect(data).to.equal(plaintext); + }); + + it('decrypt with `config.constantTimePKCS1Decryption` option should succeed (with streaming)', async function () { + const publicKey = await openpgp.readKey({ armoredKey: pub_key }); + const publicKey2 = await openpgp.readKey({ armoredKey: eccPrivateKey }); + const privateKey = await openpgp.decryptKey({ + privateKey: await openpgp.readKey({ armoredKey: priv_key }), + passphrase + }); + + const encrypted = await openpgp.encrypt({ + message: await openpgp.createMessage({ text: plaintext }), + signingKeys: privateKey, + encryptionKeys: [publicKey, publicKey2] + }); + const { data: streamedData } = await openpgp.decrypt({ + message: await openpgp.readMessage({ armoredMessage: stream.toStream(encrypted) }), + decryptionKeys: privateKey, + verificationKeys: publicKey, + expectSigned: true, + config: { constantTimePKCS1Decryption: true } + }); + const data = await stream.readToEnd(streamedData); + expect(data).to.equal(plaintext); + }); + + it('decrypt with `config.constantTimePKCS1Decryption` option should fail if session key algo support is disabled', async function () { + const publicKeyRSA = await openpgp.readKey({ armoredKey: pub_key }); + const privateKeyRSA = await openpgp.decryptKey({ + privateKey: await openpgp.readKey({ armoredKey: priv_key }), + passphrase + }); + const privateKeyECC = await openpgp.readPrivateKey({ armoredKey: eccPrivateKey }); + + const encrypted = await openpgp.encrypt({ + message: await openpgp.createMessage({ text: plaintext }), + signingKeys: privateKeyRSA, + encryptionKeys: [publicKeyRSA, privateKeyECC] + }); + + // decryption using RSA key should fail + await expect(openpgp.decrypt({ + message: await openpgp.readMessage({ armoredMessage: encrypted }), + decryptionKeys: privateKeyRSA, + config: { constantTimePKCS1Decryption: true, constantTimePKCS1DecryptionSupportedSymmetricAlgorithms: new Set() } + })).to.be.rejectedWith(/Session key decryption failed/); + // decryption using ECC key should succeed (PKCS1 is not used, so constant time countermeasures are not applied) + const { data } = await openpgp.decrypt({ + message: await openpgp.readMessage({ armoredMessage: encrypted }), + decryptionKeys: privateKeyECC, + config: { constantTimePKCS1Decryption: true } + }); + expect(data).to.equal(plaintext); + }); + }); describe('verify - unit tests', function() { From 1fcbb4864c0ac92e6ee8ff64b5331a46dce426b7 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Wed, 1 Dec 2021 14:26:35 +0100 Subject: [PATCH 3/8] Add constant-time select --- src/crypto/pkcs1.js | 3 +- .../public_key_encrypted_session_key.js | 4 +-- src/util.js | 29 +++++++++++++++++++ test/general/util.js | 27 +++++++++++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/crypto/pkcs1.js b/src/crypto/pkcs1.js index ac6a48c75..863f354f4 100644 --- a/src/crypto/pkcs1.js +++ b/src/crypto/pkcs1.js @@ -26,6 +26,7 @@ import { getRandomBytes } from './random'; import hash from './hash'; +import util from '../util'; /** * ASN1 object identifiers for hashes @@ -116,7 +117,7 @@ export function emeDecode(encoded, randomPayload) { const isValidPadding = encoded[0] === 0 & encoded[1] === 2 & psLen >= 8 & !separatorNotFound; if (randomPayload) { - return isValidPadding ? payload : randomPayload; + return util.selectUint8Array(isValidPadding, payload, randomPayload); } if (isValidPadding) { diff --git a/src/packet/public_key_encrypted_session_key.js b/src/packet/public_key_encrypted_session_key.js index 645b3c947..b56d7ac40 100644 --- a/src/packet/public_key_encrypted_session_key.js +++ b/src/packet/public_key_encrypted_session_key.js @@ -138,8 +138,8 @@ class PublicKeyEncryptedSessionKeyPacket { // We must not leak info about the validity of the decrypted checksum or cipher algo. // The decrypted session key must be of the same algo and size as the random session key, otherwise we discard it and use the random data. const isValidPayload = isValidChecksum & symmetricAlgoByte === randomSessionKey.sessionKeyAlgorithm & sessionKey.length === randomSessionKey.sessionKey.length; - this.sessionKey = isValidPayload ? sessionKey : randomSessionKey.sessionKey; - this.sessionKeyAlgorithm = isValidPayload ? symmetricAlgoByte : randomSessionKey.sessionKeyAlgorithm; + this.sessionKeyAlgorithm = util.selectUint8(isValidPayload, symmetricAlgoByte, randomSessionKey.sessionKeyAlgorithm); + this.sessionKey = util.selectUint8Array(isValidPayload, sessionKey, randomSessionKey.sessionKey); } else { const isValidPayload = isValidChecksum && enums.read(enums.symmetric, symmetricAlgoByte); diff --git a/src/util.js b/src/util.js index a510bc394..c62c09149 100644 --- a/src/util.js +++ b/src/util.js @@ -588,6 +588,35 @@ const util = { })); reject(exception); }); + }, + + /** + * Return either `a` or `b` based on `cond`, in algorithmic constant time. + * @param {Boolean} cond + * @param {Uint8Array} a + * @param {Uint8Array} b + * @returns `a` if `cond` is true, `b` otherwise + */ + selectUint8Array: function(cond, a, b) { + const length = Math.max(a.length, b.length); + const result = new Uint8Array(length); + let end = 0; + for (let i = 0; i < result.length; i++) { + result[i] = (a[i] & (256 - cond)) | (b[i] & (255 + cond)); + end += (cond & i < a.length) | ((1 - cond) & i < b.length); + } + return result.subarray(0, end); + }, + /** + * Return either `a` or `b` based on `cond`, in algorithmic constant time. + * NB: it only supports `a, b` with values between 0-255. + * @param {Boolean} cond + * @param {Uint8} a + * @param {Uint8} b + * @returns `a` if `cond` is true, `b` otherwise + */ + selectUint8: function(cond, a, b) { + return (a & (256 - cond)) | (b & (255 + cond)); } }; diff --git a/test/general/util.js b/test/general/util.js index e93056992..62e856edf 100644 --- a/test/general/util.js +++ b/test/general/util.js @@ -141,6 +141,33 @@ module.exports = () => describe('Util unit tests', function() { }); }); + describe('constant time select', function() { + it('selectUint8Array should work for arrays of equal length', function () { + const size = 10; + const a = new Uint8Array(size).fill(1); + const b = new Uint8Array(size).fill(2); + expect(util.selectUint8Array(true, a, b)).to.deep.equal(a); + expect(util.selectUint8Array(false, a, b)).to.deep.equal(b); + }); + + it('selectUint8Array should work for arrays of different length', function () { + const size = 10; + const a = new Uint8Array(size).fill(1); + const b = new Uint8Array(2 * size).fill(2); + expect(util.selectUint8Array(true, a, b)).to.deep.equal(a); + expect(util.selectUint8Array(false, a, b)).to.deep.equal(b); + expect(util.selectUint8Array(true, b, a)).to.deep.equal(b); + expect(util.selectUint8Array(false, b, a)).to.deep.equal(a); + }); + + it('selectUint8 should return the expected value based on condition', function () { + const a = 1; + const b = 2; + expect(util.selectUint8(true, a, b)).to.equal(a); + expect(util.selectUint8(false, a, b)).to.equal(b); + }); + }); + describe('Misc.', function() { it('util.readNumber should not overflow until full range of uint32', function () { const ints = [2 ** 20, 2 ** 25, 2 ** 30, 2 ** 32 - 1]; From 11a00f204f3e4e9c8dc5b346c6de3024bc70ac1c Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Wed, 1 Dec 2021 14:33:00 +0100 Subject: [PATCH 4/8] Update comments --- src/message.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/message.js b/src/message.js index ed9d60dd0..f575f0168 100644 --- a/src/message.js +++ b/src/message.js @@ -226,10 +226,10 @@ export class Message { if (doConstantTimeDecryption) { // The goal is to not reveal whether PKESK decryption (specifically the PKCS1 decoding step) failed, hence, we always proceed to decrypt the message, - // either with the successfully decrypted session key, or with a randomly generated one of the same type. + // either with the successfully decrypted session key, or with a randomly generated one. // Since the SEIP/AEAD's symmetric algorithm and key size are stored in the encrypted portion of the PKESK, and the execution flow cannot depend on // the decrypted payload, we always assume the message to be encrypted with one of the symmetric algorithms specified in `config.constantTimePKCS1DecryptionSupportedSymmetricAlgorithms`: - // - If the PKESK decryption succeeds, and the session key cipher is in the supported set, then we try to decrypt the data with the decrypted session key as well as the + // - If the PKESK decryption succeeds, and the session key cipher is in the supported set, then we try to decrypt the data with the decrypted session key as well as with the // randomly generated keys of the remaining key types. // - If the PKESK decryptions fails, or if it succeeds but support for the cipher is not enabled, then we discard the session key and try to decrypt the data using only the randomly // generated session keys. From d3ecad0f681dc651f7880bbd52491cce31043739 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Wed, 1 Dec 2021 15:42:58 +0100 Subject: [PATCH 5/8] Fix test --- test/general/openpgp.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/general/openpgp.js b/test/general/openpgp.js index 9c571b774..eb3793332 100644 --- a/test/general/openpgp.js +++ b/test/general/openpgp.js @@ -1537,17 +1537,21 @@ aOU= encryptionKeys: [publicKeyRSA, privateKeyECC] }); + const config = { + constantTimePKCS1Decryption: true, + constantTimePKCS1DecryptionSupportedSymmetricAlgorithms: new Set() + }; // decryption using RSA key should fail await expect(openpgp.decrypt({ message: await openpgp.readMessage({ armoredMessage: encrypted }), decryptionKeys: privateKeyRSA, - config: { constantTimePKCS1Decryption: true, constantTimePKCS1DecryptionSupportedSymmetricAlgorithms: new Set() } + config })).to.be.rejectedWith(/Session key decryption failed/); // decryption using ECC key should succeed (PKCS1 is not used, so constant time countermeasures are not applied) const { data } = await openpgp.decrypt({ message: await openpgp.readMessage({ armoredMessage: encrypted }), decryptionKeys: privateKeyECC, - config: { constantTimePKCS1Decryption: true } + config }); expect(data).to.equal(plaintext); }); From 90311052ab63d7f07f25551b6ddf11838eb67cb2 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Thu, 2 Dec 2021 12:17:18 +0100 Subject: [PATCH 6/8] Check checksum in constant time --- src/packet/public_key_encrypted_session_key.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/packet/public_key_encrypted_session_key.js b/src/packet/public_key_encrypted_session_key.js index b56d7ac40..6de1a3a39 100644 --- a/src/packet/public_key_encrypted_session_key.js +++ b/src/packet/public_key_encrypted_session_key.js @@ -129,10 +129,11 @@ class PublicKeyEncryptedSessionKeyPacket { util.writeChecksum(randomSessionKey.sessionKey) ]) : null; const decoded = await crypto.publicKeyDecrypt(this.publicKeyAlgorithm, key.publicParams, key.privateParams, this.encrypted, key.getFingerprintBytes(), randomPayload); - const checksum = decoded.subarray(decoded.length - 2); - const sessionKey = decoded.subarray(1, decoded.length - 2); const symmetricAlgoByte = decoded[0]; - const isValidChecksum = util.equalsUint8Array(checksum, util.writeChecksum(sessionKey)); + const sessionKey = decoded.subarray(1, decoded.length - 2); + const expectedChecksum = decoded.subarray(decoded.length - 2); + const actualChecksum = util.writeChecksum(sessionKey); + const isValidChecksum = actualChecksum[0] === expectedChecksum[0] & actualChecksum[1] === expectedChecksum[1]; if (randomSessionKey) { // We must not leak info about the validity of the decrypted checksum or cipher algo. From b96a33f4db85659ae4e92beafba54c9cce19af3f Mon Sep 17 00:00:00 2001 From: larabr Date: Thu, 2 Dec 2021 12:18:11 +0100 Subject: [PATCH 7/8] Fix typo in comment [skip ci] Co-authored-by: Daniel Huigens --- src/crypto/pkcs1.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/pkcs1.js b/src/crypto/pkcs1.js index 863f354f4..cfda49d79 100644 --- a/src/crypto/pkcs1.js +++ b/src/crypto/pkcs1.js @@ -113,7 +113,7 @@ export function emeDecode(encoded, randomPayload) { } const psLen = offset - 2; - const payload = encoded.subarray(offset + 1); // discar the 0x00 separator + const payload = encoded.subarray(offset + 1); // discard the 0x00 separator const isValidPadding = encoded[0] === 0 & encoded[1] === 2 & psLen >= 8 & !separatorNotFound; if (randomPayload) { From ecca368c01c29a9ac73cd80d06fd62d0b70a5da1 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Thu, 2 Dec 2021 13:05:59 +0100 Subject: [PATCH 8/8] Rename variables --- src/packet/public_key_encrypted_session_key.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/packet/public_key_encrypted_session_key.js b/src/packet/public_key_encrypted_session_key.js index 6de1a3a39..09552fead 100644 --- a/src/packet/public_key_encrypted_session_key.js +++ b/src/packet/public_key_encrypted_session_key.js @@ -131,9 +131,9 @@ class PublicKeyEncryptedSessionKeyPacket { const decoded = await crypto.publicKeyDecrypt(this.publicKeyAlgorithm, key.publicParams, key.privateParams, this.encrypted, key.getFingerprintBytes(), randomPayload); const symmetricAlgoByte = decoded[0]; const sessionKey = decoded.subarray(1, decoded.length - 2); - const expectedChecksum = decoded.subarray(decoded.length - 2); - const actualChecksum = util.writeChecksum(sessionKey); - const isValidChecksum = actualChecksum[0] === expectedChecksum[0] & actualChecksum[1] === expectedChecksum[1]; + const checksum = decoded.subarray(decoded.length - 2); + const computedChecksum = util.writeChecksum(sessionKey); + const isValidChecksum = computedChecksum[0] === checksum[0] & computedChecksum[1] === checksum[1]; if (randomSessionKey) { // We must not leak info about the validity of the decrypted checksum or cipher algo.