From f629ddcb3150a7df73cb1d385c72f1472f2d3608 Mon Sep 17 00:00:00 2001
From: Daniel Huigens <d.huigens@protonmail.com>
Date: Sun, 5 May 2019 00:02:11 +0200
Subject: [PATCH] Fix reading and writing unencrypted V5 secret key packets

---
 src/packet/secret_key.js | 275 +++++++++++++++++++++++----------------
 1 file changed, 161 insertions(+), 114 deletions(-)

diff --git a/src/packet/secret_key.js b/src/packet/secret_key.js
index 160a14f5..ddde4b15 100644
--- a/src/packet/secret_key.js
+++ b/src/packet/secret_key.js
@@ -47,13 +47,33 @@ function SecretKey(date=new Date()) {
    */
   this.tag = enums.packet.secretKey;
   /**
-   * Encrypted secret-key data
+   * Secret-key data
    */
-  this.encrypted = null;
+  this.keyMaterial = null;
   /**
-   * Indicator if secret-key data is encrypted. `this.isEncrypted === false` means data is available in decrypted form.
+   * Indicates whether secret-key data is encrypted. `this.isEncrypted === false` means data is available in decrypted form.
    */
   this.isEncrypted = null;
+  /**
+   * S2K usage
+   * @type {Integer}
+   */
+  this.s2k_usage = 0;
+  /**
+   * S2K object
+   * @type {type/s2k}
+   */
+  this.s2k = null;
+  /**
+   * Symmetric algorithm
+   * @type {String}
+   */
+  this.symmetric = 'aes256';
+  /**
+   * AEAD algorithm
+   * @type {String}
+   */
+  this.aead = 'eax';
 }
 
 SecretKey.prototype = new publicKey();
@@ -99,31 +119,78 @@ function write_cleartext_params(params, algorithm) {
  */
 SecretKey.prototype.read = function (bytes) {
   // - A Public-Key or Public-Subkey packet, as described above.
-  const len = this.readPublicKey(bytes);
-
-  bytes = bytes.subarray(len, bytes.length);
-
+  let i = this.readPublicKey(bytes);
 
   // - One octet indicating string-to-key usage conventions.  Zero
   //   indicates that the secret-key data is not encrypted.  255 or 254
   //   indicates that a string-to-key specifier is being given.  Any
   //   other value is a symmetric-key encryption algorithm identifier.
-  const isEncrypted = bytes[0];
+  this.s2k_usage = bytes[i++];
 
-  if (isEncrypted) {
-    this.encrypted = bytes;
-    this.isEncrypted = true;
-  } else {
-    // - Plain or encrypted multiprecision integers comprising the secret
-    //   key data.  These algorithm-specific fields are as described
-    //   below.
-    const cleartext = bytes.subarray(1, -2);
-    if (!util.equalsUint8Array(util.write_checksum(cleartext), bytes.subarray(-2))) {
+  // - Only for a version 5 packet, a one-octet scalar octet count of
+  //   the next 4 optional fields.
+  if (this.version === 5) {
+    i++;
+  }
+
+  // - [Optional] If string-to-key usage octet was 255, 254, or 253, a
+  //   one-octet symmetric encryption algorithm.
+  if (this.s2k_usage === 255 || this.s2k_usage === 254 || this.s2k_usage === 253) {
+    this.symmetric = bytes[i++];
+    this.symmetric = enums.read(enums.symmetric, this.symmetric);
+
+    // - [Optional] If string-to-key usage octet was 253, a one-octet
+    //   AEAD algorithm.
+    if (this.s2k_usage === 253) {
+      this.aead = bytes[i++];
+      this.aead = enums.read(enums.aead, this.aead);
+    }
+
+    // - [Optional] If string-to-key usage octet was 255, 254, or 253, a
+    //   string-to-key specifier.  The length of the string-to-key
+    //   specifier is implied by its type, as described above.
+    this.s2k = new type_s2k();
+    i += this.s2k.read(bytes.subarray(i, bytes.length));
+
+    if (this.s2k.type === 'gnu-dummy') {
+      return;
+    }
+  } else if (this.s2k_usage) {
+    this.symmetric = this.s2k_usage;
+    this.symmetric = enums.read(enums.symmetric, this.symmetric);
+  }
+
+  // - [Optional] If secret data is encrypted (string-to-key usage octet
+  //   not zero), an Initial Vector (IV) of the same length as the
+  //   cipher's block size.
+  if (this.s2k_usage) {
+    this.iv = bytes.subarray(
+      i,
+      i + crypto.cipher[this.symmetric].blockSize
+    );
+
+    i += this.iv.length;
+  }
+
+  // - Only for a version 5 packet, a four-octet scalar octet count for
+  //   the following key material.
+  if (this.version === 5) {
+    i += 4;
+  }
+
+  // - Plain or encrypted multiprecision integers comprising the secret
+  //   key data.  These algorithm-specific fields are as described
+  //   below.
+  this.keyMaterial = bytes.subarray(i);
+  this.isEncrypted = !!this.s2k_usage;
+
+  if (!this.isEncrypted) {
+    const cleartext = this.keyMaterial.subarray(0, -2);
+    if (!util.equalsUint8Array(util.write_checksum(cleartext), this.keyMaterial.subarray(-2))) {
       throw new Error('Key checksum mismatch');
     }
     const privParams = parse_cleartext_params(cleartext, this.algorithm);
     this.params = this.params.concat(privParams);
-    this.isEncrypted = false;
   }
 };
 
@@ -134,13 +201,51 @@ SecretKey.prototype.read = function (bytes) {
 SecretKey.prototype.write = function () {
   const arr = [this.writePublicKey()];
 
-  if (!this.encrypted) {
-    arr.push(new Uint8Array([0]));
-    const cleartextParams = write_cleartext_params(this.params, this.algorithm);
-    arr.push(cleartextParams);
-    arr.push(util.write_checksum(cleartextParams));
-  } else {
-    arr.push(this.encrypted);
+  arr.push(new Uint8Array([this.s2k_usage]));
+
+  const optionalFieldsArr = [];
+  // - [Optional] If string-to-key usage octet was 255, 254, or 253, a
+  //   one- octet symmetric encryption algorithm.
+  if (this.s2k_usage === 255 || this.s2k_usage === 254 || this.s2k_usage === 253) {
+    optionalFieldsArr.push(enums.write(enums.symmetric, this.symmetric));
+
+    // - [Optional] If string-to-key usage octet was 253, a one-octet
+    //   AEAD algorithm.
+    if (this.s2k_usage === 253) {
+      optionalFieldsArr.push(enums.write(enums.aead, this.aead));
+    }
+
+    // - [Optional] If string-to-key usage octet was 255, 254, or 253, a
+    //   string-to-key specifier.  The length of the string-to-key
+    //   specifier is implied by its type, as described above.
+    optionalFieldsArr.push(...this.s2k.write());
+  }
+
+  // - [Optional] If secret data is encrypted (string-to-key usage octet
+  //   not zero), an Initial Vector (IV) of the same length as the
+  //   cipher's block size.
+  if (this.s2k_usage) {
+    optionalFieldsArr.push(...this.iv);
+  }
+
+  if (this.version === 5) {
+    arr.push(new Uint8Array([optionalFieldsArr.length]));
+  }
+  arr.push(new Uint8Array(optionalFieldsArr));
+
+  if (!this.s2k || this.s2k.type !== 'gnu-dummy') {
+    if (!this.s2k_usage) {
+      const cleartextParams = write_cleartext_params(this.params, this.algorithm);
+      this.keyMaterial = util.concatUint8Array([
+        cleartextParams,
+        util.write_checksum(cleartextParams)
+      ]);
+    }
+
+    if (this.version === 5) {
+      arr.push(util.writeNumber(this.keyMaterial.length, 4));
+    }
+    arr.push(this.keyMaterial);
   }
 
   return util.concatUint8Array(arr);
@@ -164,48 +269,36 @@ SecretKey.prototype.isDecrypted = function() {
  * @async
  */
 SecretKey.prototype.encrypt = async function (passphrase) {
-  if (this.isDecrypted() && this.encrypted) { // gnu-dummy
+  if (this.s2k && this.s2k.type === 'gnu-dummy') {
     return false;
   }
 
   if (this.isDecrypted() && !passphrase) {
-    this.encrypted = null;
+    this.s2k_usage = 0;
     return false;
   } else if (!passphrase) {
     throw new Error('The key must be decrypted before removing passphrase protection.');
   }
 
-  const s2k = new type_s2k();
-  s2k.salt = await crypto.random.getRandomBytes(8);
-  const symmetric = 'aes256';
+  this.s2k = new type_s2k();
+  this.s2k.salt = await crypto.random.getRandomBytes(8);
   const cleartext = write_cleartext_params(this.params, this.algorithm);
-  const key = await produceEncryptionKey(s2k, passphrase, symmetric);
-  const blockLen = crypto.cipher[symmetric].blockSize;
-  const iv = await crypto.random.getRandomBytes(blockLen);
-
-  let arr;
+  const key = await produceEncryptionKey(this.s2k, passphrase, this.symmetric);
+  const blockLen = crypto.cipher[this.symmetric].blockSize;
+  this.iv = await crypto.random.getRandomBytes(blockLen);
 
   if (this.version === 5) {
-    const aead = 'eax';
-    const optionalFields = util.concatUint8Array([new Uint8Array([enums.write(enums.symmetric, symmetric), enums.write(enums.aead, aead)]), s2k.write(), iv]);
-    arr = [new Uint8Array([253, optionalFields.length])];
-    arr.push(optionalFields);
-    const mode = crypto[aead];
-    const modeInstance = await mode(symmetric, key);
-    const encrypted = await modeInstance.encrypt(cleartext, iv.subarray(0, mode.ivLength), new Uint8Array());
-    arr.push(util.writeNumber(encrypted.length, 4));
-    arr.push(encrypted);
+    this.s2k_usage = 253;
+    const mode = crypto[this.aead];
+    const modeInstance = await mode(this.symmetric, key);
+    this.keyMaterial = await modeInstance.encrypt(cleartext, this.iv.subarray(0, mode.ivLength), new Uint8Array());
   } else {
-    arr = [new Uint8Array([254, enums.write(enums.symmetric, symmetric)])];
-    arr.push(s2k.write());
-    arr.push(iv);
-    arr.push(crypto.cfb.encrypt(symmetric, key, util.concatUint8Array([
+    this.s2k_usage = 254;
+    this.keyMaterial = crypto.cfb.encrypt(this.symmetric, key, util.concatUint8Array([
       cleartext,
       await crypto.hash.sha1(cleartext)
-    ]), iv));
+    ]), this.iv);
   }
-
-  this.encrypted = util.concatUint8Array(arr);
   return true;
 };
 
@@ -225,87 +318,40 @@ async function produceEncryptionKey(s2k, passphrase, algorithm) {
  * @async
  */
 SecretKey.prototype.decrypt = async function (passphrase) {
+  if (this.s2k.type === 'gnu-dummy') {
+    this.isEncrypted = false;
+    return false;
+  }
+
   if (this.isDecrypted()) {
     throw new Error('Key packet is already decrypted.');
   }
 
-  let i = 0;
-  let symmetric;
-  let aead;
   let key;
-
-  const s2k_usage = this.encrypted[i++];
-
-  // - Only for a version 5 packet, a one-octet scalar octet count of
-  //   the next 4 optional fields.
-  if (this.version === 5) {
-    i++;
-  }
-
-  // - [Optional] If string-to-key usage octet was 255, 254, or 253, a
-  //   one-octet symmetric encryption algorithm.
-  if (s2k_usage === 255 || s2k_usage === 254 || s2k_usage === 253) {
-    symmetric = this.encrypted[i++];
-    symmetric = enums.read(enums.symmetric, symmetric);
-
-    // - [Optional] If string-to-key usage octet was 253, a one-octet
-    //   AEAD algorithm.
-    if (s2k_usage === 253) {
-      aead = this.encrypted[i++];
-      aead = enums.read(enums.aead, aead);
-    }
-
-    // - [Optional] If string-to-key usage octet was 255, 254, or 253, a
-    //   string-to-key specifier.  The length of the string-to-key
-    //   specifier is implied by its type, as described above.
-    const s2k = new type_s2k();
-    i += s2k.read(this.encrypted.subarray(i, this.encrypted.length));
-
-    if (s2k.type === 'gnu-dummy') {
-      this.isEncrypted = false;
-      return false;
-    }
-    key = await produceEncryptionKey(s2k, passphrase, symmetric);
+  if (this.s2k_usage === 255 || this.s2k_usage === 254 || this.s2k_usage === 253) {
+    key = await produceEncryptionKey(this.s2k, passphrase, this.symmetric);
   } else {
-    symmetric = s2k_usage;
-    symmetric = enums.read(enums.symmetric, symmetric);
     key = await crypto.hash.md5(passphrase);
   }
 
-  // - [Optional] If secret data is encrypted (string-to-key usage octet
-  //   not zero), an Initial Vector (IV) of the same length as the
-  //   cipher's block size.
-  const iv = this.encrypted.subarray(
-    i,
-    i + crypto.cipher[symmetric].blockSize
-  );
-
-  i += iv.length;
-
-  // - Only for a version 5 packet, a four-octet scalar octet count for
-  //   the following key material.
-  if (this.version === 5) {
-    i += 4;
-  }
-
-  const ciphertext = this.encrypted.subarray(i, this.encrypted.length);
   let cleartext;
-  if (aead) {
-    const mode = crypto[aead];
+  if (this.s2k_usage === 253) {
+    const mode = crypto[this.aead];
     try {
-      const modeInstance = await mode(symmetric, key);
-      cleartext = await modeInstance.decrypt(ciphertext, iv.subarray(0, mode.ivLength), new Uint8Array());
+      const modeInstance = await mode(this.symmetric, key);
+      cleartext = await modeInstance.decrypt(this.keyMaterial, this.iv.subarray(0, mode.ivLength), new Uint8Array());
     } catch(err) {
       if (err.message === 'Authentication tag mismatch') {
         throw new Error('Incorrect key passphrase: ' + err.message);
       }
+      throw err;
     }
   } else {
-    const cleartextWithHash = await crypto.cfb.decrypt(symmetric, key, ciphertext, iv);
+    const cleartextWithHash = await crypto.cfb.decrypt(this.symmetric, key, this.keyMaterial, this.iv);
 
     let hash;
     let hashlen;
-    if (s2k_usage === 255) {
+    if (this.s2k_usage === 255) {
       hashlen = 2;
       cleartext = cleartextWithHash.subarray(0, -hashlen);
       hash = util.write_checksum(cleartext);
@@ -323,7 +369,8 @@ SecretKey.prototype.decrypt = async function (passphrase) {
   const privParams = parse_cleartext_params(cleartext, this.algorithm);
   this.params = this.params.concat(privParams);
   this.isEncrypted = false;
-  this.encrypted = null;
+  this.keyMaterial = null;
+  this.s2k_usage = 0;
 
   return true;
 };
@@ -338,7 +385,7 @@ SecretKey.prototype.generate = async function (bits, curve) {
  * Clear private params, return to initial state
  */
 SecretKey.prototype.clearPrivateParams = function () {
-  if (!this.encrypted) {
+  if (!this.keyMaterial) {
     throw new Error('If secret key is not encrypted, clearing private params is irreversible.');
   }
   const algo = enums.write(enums.publicKey, this.algorithm);