Throw on unexpected param sizes in secret keys, session keys and signatures

Detect extra bytes in secret key material, as well as missing bytes in other parameters.

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
    */

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 };
     }

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:

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;

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;
   }
 
   /**

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;
   }
 
   /**

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);
   },
 
   /**

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;