diff --git a/src/config/config.js b/src/config/config.js index 3f93e720..e448cbda 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -128,7 +128,12 @@ export default { * process large streams while limiting memory usage by releasing the decrypted chunks as soon as possible * and deferring checking their integrity until the decrypted stream has been read in full. * - * This setting is **insecure** if the partially decrypted message is processed further or displayed to the user. + * This setting is **insecure** if the encrypted data has been corrupted by a malicious entity: + * - if the partially decrypted message is processed further or displayed to the user, it opens up the possibility of attacks such as EFAIL + * (see https://efail.de/). + * - an attacker with access to traces or timing info of internal processing errors could learn some info about the data. + * + * NB: this setting does not apply to AEAD-encrypted data, where the AEAD data chunk is never released until integrity is confirmed. * @memberof module:config * @property {Boolean} allowUnauthenticatedStream */ diff --git a/src/crypto/crypto.js b/src/crypto/crypto.js index a9293e75..2576bd88 100644 --- a/src/crypto/crypto.js +++ b/src/crypto/crypto.js @@ -186,7 +186,7 @@ export function parsePublicKeyParams(algo, bytes) { case enums.publicKey.ed448: case enums.publicKey.x25519: case enums.publicKey.x448: { - const A = bytes.subarray(read, read + getCurvePayloadSize(algo)); read += A.length; + const A = util.readExactSubarray(bytes, read, read + getCurvePayloadSize(algo)); read += A.length; return { read, publicParams: { A } }; } default: @@ -234,13 +234,13 @@ export function parsePrivateKeyParams(algo, bytes, publicParams) { case enums.publicKey.ed25519: case enums.publicKey.ed448: { const payloadSize = getCurvePayloadSize(algo); - const seed = bytes.subarray(read, read + payloadSize); read += seed.length; + const seed = util.readExactSubarray(bytes, read, read + payloadSize); read += seed.length; return { read, privateParams: { seed } }; } case enums.publicKey.x25519: case enums.publicKey.x448: { const payloadSize = getCurvePayloadSize(algo); - const k = bytes.subarray(read, read + payloadSize); read += k.length; + const k = util.readExactSubarray(bytes, read, read + payloadSize); read += k.length; return { read, privateParams: { k } }; } default: @@ -288,7 +288,7 @@ export function parseEncSessionKeyParams(algo, bytes) { case enums.publicKey.x25519: case enums.publicKey.x448: { const pointSize = getCurvePayloadSize(algo); - const ephemeralPublicKey = bytes.subarray(read, read + pointSize); read += ephemeralPublicKey.length; + const ephemeralPublicKey = util.readExactSubarray(bytes, read, read + pointSize); read += ephemeralPublicKey.length; const C = new ECDHXSymmetricKey(); C.read(bytes.subarray(read)); return { ephemeralPublicKey, C }; } diff --git a/src/crypto/signature.js b/src/crypto/signature.js index 5d3db48d..5f0ceeab 100644 --- a/src/crypto/signature.js +++ b/src/crypto/signature.js @@ -27,10 +27,10 @@ export function parseSignatureParams(algo, signature) { case enums.publicKey.rsaEncryptSign: case enums.publicKey.rsaEncrypt: case enums.publicKey.rsaSign: { - const s = util.readMPI(signature.subarray(read)); + const s = util.readMPI(signature.subarray(read)); read += s.length + 2; // The signature needs to be the same length as the public key modulo n. // We pad s on signature verification, where we have access to n. - return { s }; + return { read, signatureParams: { s } }; } // Algorithm-Specific Fields for DSA or ECDSA signatures: // - MPI of DSA or ECDSA value r. @@ -39,8 +39,8 @@ export function parseSignatureParams(algo, signature) { case enums.publicKey.ecdsa: { const r = util.readMPI(signature.subarray(read)); read += r.length + 2; - const s = util.readMPI(signature.subarray(read)); - return { r, s }; + const s = util.readMPI(signature.subarray(read)); read += s.length + 2; + return { read, signatureParams: { r, s } }; } // Algorithm-Specific Fields for legacy EdDSA signatures: // - MPI of an EC point r. @@ -50,9 +50,9 @@ export function parseSignatureParams(algo, signature) { // https://www.ietf.org/archive/id/draft-ietf-openpgp-rfc4880bis-10.html#section-3.2-9 let r = util.readMPI(signature.subarray(read)); read += r.length + 2; r = util.leftPad(r, 32); - let s = util.readMPI(signature.subarray(read)); + let s = util.readMPI(signature.subarray(read)); read += s.length + 2; s = util.leftPad(s, 32); - return { r, s }; + return { read, signatureParams: { r, s } }; } // Algorithm-Specific Fields for Ed25519 signatures: // - 64 octets of the native signature @@ -61,8 +61,8 @@ export function parseSignatureParams(algo, signature) { case enums.publicKey.ed25519: case enums.publicKey.ed448: { const rsSize = 2 * publicKey.elliptic.eddsa.getPayloadSize(algo); - const RS = signature.subarray(read, read + rsSize); read += RS.length; - return { RS }; + const RS = util.readExactSubarray(signature, read, read + rsSize); read += RS.length; + return { read, signatureParams: { RS } }; } default: diff --git a/src/packet/secret_key.js b/src/packet/secret_key.js index dc5b853d..6dbb3b72 100644 --- a/src/packet/secret_key.js +++ b/src/packet/secret_key.js @@ -195,7 +195,10 @@ class SecretKeyPacket extends PublicKeyPacket { } } try { - const { privateParams } = crypto.parsePrivateKeyParams(this.algorithm, cleartext, this.publicParams); + const { read, privateParams } = crypto.parsePrivateKeyParams(this.algorithm, cleartext, this.publicParams); + if (read < cleartext.length) { + throw new Error('Error reading MPIs'); + } this.privateParams = privateParams; } catch (err) { if (err instanceof UnsupportedError) throw err; diff --git a/src/packet/signature.js b/src/packet/signature.js index a66fbd49..b650ca99 100644 --- a/src/packet/signature.js +++ b/src/packet/signature.js @@ -153,7 +153,12 @@ class SignaturePacket { i += saltLength; } - this.params = crypto.signature.parseSignatureParams(this.publicKeyAlgorithm, bytes.subarray(i, bytes.length)); + const signatureMaterial = bytes.subarray(i, bytes.length); + const { read, signatureParams } = crypto.signature.parseSignatureParams(this.publicKeyAlgorithm, signatureMaterial); + if (read < signatureMaterial.length) { + throw new Error('Error reading MPIs'); + } + this.params = signatureParams; } /** diff --git a/src/type/ecdh_x_symkey.js b/src/type/ecdh_x_symkey.js index ecef5715..2f3ee8c9 100644 --- a/src/type/ecdh_x_symkey.js +++ b/src/type/ecdh_x_symkey.js @@ -27,7 +27,7 @@ class ECDHXSymmetricKey { let followLength = bytes[read++]; this.algorithm = followLength % 2 ? bytes[read++] : null; // session key size is always even followLength -= followLength % 2; - this.wrappedKey = bytes.subarray(read, read + followLength); read += followLength; + this.wrappedKey = util.readExactSubarray(bytes, read, read + followLength); read += followLength; } /** diff --git a/src/util.js b/src/util.js index a1d5c3d2..f0ee7f63 100644 --- a/src/util.js +++ b/src/util.js @@ -89,7 +89,26 @@ const util = { readMPI: function (bytes) { const bits = (bytes[0] << 8) | bytes[1]; const bytelen = (bits + 7) >>> 3; - return bytes.subarray(2, 2 + bytelen); + // There is a decryption oracle risk here by enforcing the MPI length using `readExactSubarray` in the context of SEIPDv1 encrypted signatures, + // where unauthenticated streamed decryption is done (via `config.allowUnauthenticatedStream`), since the decrypted signature data being processed + // has not been authenticated (yet). + // However, such config setting is known to be insecure, and there are other packet parsing errors that can cause similar issues. + // Also, AEAD is also not affected. + return util.readExactSubarray(bytes, 2, 2 + bytelen); + }, + + /** + * Read exactly `end - start` bytes from input. + * This is a stricter version of `.subarray`. + * @param {Uint8Array} input - Input data to parse + * @returns {Uint8Array} subarray of size always equal to `end - start` + * @throws if the input array is too short. + */ + readExactSubarray: function (input, start, end) { + if (input.length < (end - start)) { + throw new Error('Input array too short'); + } + return input.subarray(start, end); }, /** diff --git a/test/general/key.js b/test/general/key.js index 0e5dc5ae..adc11528 100644 --- a/test/general/key.js +++ b/test/general/key.js @@ -3257,6 +3257,29 @@ AAAAHh0gf2kdqLoXdFX+aNVORr5VCVgcm2gBw8l68lDJ1ftA71bMllFi6Q5sLPUr expect(encryptionKey.getAlgorithmInfo()).to.deep.equal({ algorithm: 'x448' }); }); + it('Throw when parsing x448 key with unexpected secret param size', async function() { + // x448 subkey with secret seed of 57 bytes instead of 56 + const armoredKey = `-----BEGIN PGP PRIVATE KEY BLOCK----- + +xXsEZRwKeRx0854v253kTyH54UIFSy3dLbMLTSIGh+UeC6IWXvxt2551rUee +wn9y5hJbJwm/f2eA3vkOUqhKbAAAy7hGpOu61AMTr6w9G9VLStDR9Int/vgi +cNSl1LTvF8f5lqBrpMFFPUHwi4igNMqb7I/c7J2Uc+4uInvNAMK3BBAcCgA7 +BYJlHAp5AwsJBwmQ+2DdQup2xx8DFQgKAhYAAhkBApsDAh4BFiEE5NkqBdRy +GYLB7cPm+2DdQup2xx8AAIuONFkN6wRtRJA9EJvwhj7DkzNRjFNw8OE/ENzj +3XcN/WtZYCnLZ+ih9HSar9+CZzI+4mHtvOunq7sAjuvPbGndbbdg46DSy0Ac +wIVxSeIMNpwpktMyUx/ugIZeu7VvcnW4SbQOEB5KPlja/qjapWwg4wIAx3oE +ZRwKeRogjMz3j2jL4X1Zhk+i/EK09BTU/2zuYuB+Pl9Y+RKDaxuOmZ4zzx+S +xa/RYWEVKkcIY9pBAxd4RgDZs0rJP9DRIe69vix1Wd/LxuSctG2SMfcjzyAl +5mmCsb+sgubDduEBotTv3qFnNTYMUUHEFojWC4EfjcKmBBgcCgAqBYJlHAp5 +CZD7YN1C6nbHHwKbDBYhBOTZKgXUchmCwe3D5vtg3ULqdscfAAD/uWh1fZy7 +hMeb7552mWqB0eGXpOJR9K/rLDj8woLkXJMyyhfYU5PTwmRpowsGwbm7TMku +gXxMryvfgBDKTN8tkgJ4BJUsDDwU7aJE1fzOZ5TP4iNHpPOY1qqpmaAtTh6Q +PzIEeL7UH3trraFmi+Gq8u4kAA== +-----END PGP PRIVATE KEY BLOCK-----`; + + await expect(openpgp.readKey({ armoredKey })).to.be.rejectedWith(/Error reading MPIs/); + }); + it('Testing key ID and fingerprint for V4 keys', async function() { const pubKeysV4 = await openpgp.readKeys({ armoredKeys: twoKeys }); expect(pubKeysV4).to.exist;