Add and require primary key binding signatures on signing keys

Also, fix keyFlags of signing subkeys.

Also, store Issuer Key ID and Embedded Signature in unhashed rather
than hashed subpackets.

src/key.js

@@ -301,7 +301,12 @@ Key.prototype.getSigningKey = async function (keyId=null, date=new Date(), userI
         if (await subKeys[i].verify(primaryKey, date) === enums.keyStatus.valid) {
           const dataToVerify = { key: primaryKey, bind: subKeys[i].keyPacket };
           const bindingSignature = await getLatestValidSignature(subKeys[i].bindingSignatures, primaryKey, dataToVerify, date);
-          if (bindingSignature && isValidSigningKeyPacket(subKeys[i].keyPacket, bindingSignature)) {
+          if (
+            bindingSignature &&
+            bindingSignature.embeddedSignature &&
+            isValidSigningKeyPacket(subKeys[i].keyPacket, bindingSignature) &&
+            await getLatestValidSignature([bindingSignature.embeddedSignature], subKeys[i].keyPacket, dataToVerify, date)
+          ) {
             return subKeys[i];
           }
         }
@@ -1515,7 +1520,14 @@ async function wrapKeyObject(secretKeyPacket, secretSubkeyPackets, options) {
     subkeySignaturePacket.signatureType = enums.signature.subkey_binding;
     subkeySignaturePacket.publicKeyAlgorithm = secretKeyPacket.algorithm;
     subkeySignaturePacket.hashAlgorithm = await getPreferredHashAlgo(null, secretSubkeyPacket);
-    subkeySignaturePacket.keyFlags = subkeyOptions.sign ? enums.keyFlags.sign_data : [enums.keyFlags.encrypt_communication | enums.keyFlags.encrypt_storage];
+    if (subkeyOptions.sign) {
+      subkeySignaturePacket.keyFlags = [enums.keyFlags.sign_data];
+      subkeySignaturePacket.embeddedSignature = await createSignaturePacket(dataToSign, null, secretSubkeyPacket, {
+        signatureType: enums.signature.key_binding
+      }, subkeyOptions.date);
+    } else {
+      subkeySignaturePacket.keyFlags = [enums.keyFlags.encrypt_communication | enums.keyFlags.encrypt_storage];
+    }
     if (subkeyOptions.keyExpirationTime > 0) {
       subkeySignaturePacket.keyExpirationTime = subkeyOptions.keyExpirationTime;
       subkeySignaturePacket.keyNeverExpires = false;

src/packet/signature.js

@@ -52,7 +52,7 @@ function Signature(date=new Date()) {
   this.publicKeyAlgorithm = null;
 
   this.signatureData = null;
-  this.unhashedSubpackets = null;
+  this.unhashedSubpackets = [];
   this.signedHashValue = null;
 
   this.created = util.normalizeDate(date);
@@ -123,11 +123,9 @@ Signature.prototype.read = function (bytes) {
   // hash algorithm, the hashed subpacket length, and the hashed
   // subpacket body.
   this.signatureData = bytes.subarray(0, i);
-  const sigDataLength = i;
 
   // unhashed subpackets
   i += this.read_sub_packets(bytes.subarray(i, bytes.length), false);
-  this.unhashedSubpackets = bytes.subarray(sigDataLength, i);
 
   // Two-octet field holding left 16 bits of signed hash value.
   this.signedHashValue = bytes.subarray(i, i + 2);
@@ -139,7 +137,7 @@ Signature.prototype.read = function (bytes) {
 Signature.prototype.write = function () {
   const arr = [];
   arr.push(this.signatureData);
-  arr.push(this.unhashedSubpackets ? this.unhashedSubpackets : util.writeNumber(0, 2));
+  arr.push(this.write_unhashed_sub_packets());
   arr.push(this.signedHashValue);
   arr.push(stream.clone(this.signature));
   return util.concat(arr);
@@ -169,7 +167,7 @@ Signature.prototype.sign = async function (key, data) {
   this.issuerKeyId = key.getKeyId();
 
   // Add hashed subpackets
-  arr.push(this.write_all_sub_packets());
+  arr.push(this.write_hashed_sub_packets());
 
   this.signatureData = util.concat(arr);
 
@@ -192,10 +190,10 @@ Signature.prototype.sign = async function (key, data) {
 };
 
 /**
- * Creates string of bytes with all subpacket data
- * @returns {String} a string-representation of a all subpacket data
+ * Creates Uint8Array of bytes of all subpacket data except Issuer and Embedded Signature subpackets
+ * @returns {Uint8Array} subpacket data
  */
-Signature.prototype.write_all_sub_packets = function () {
+Signature.prototype.write_hashed_sub_packets = function () {
   const sub = enums.signatureSubpacket;
   const arr = [];
   let bytes;
@@ -230,11 +228,6 @@ Signature.prototype.write_all_sub_packets = function () {
     bytes = util.concat([bytes, this.revocationKeyFingerprint]);
     arr.push(write_sub_packet(sub.revocation_key, bytes));
   }
-  if (!this.issuerKeyId.isNull() && this.issuerKeyVersion !== 5) {
-    // If the version of [the] key is greater than 4, this subpacket
-    // MUST NOT be included in the signature.
-    arr.push(write_sub_packet(sub.issuer, this.issuerKeyId.write()));
-  }
   if (this.notation !== null) {
     Object.entries(this.notation).forEach(([name, value]) => {
       bytes = [new Uint8Array([0x80, 0, 0, 0])];
@@ -289,6 +282,30 @@ Signature.prototype.write_all_sub_packets = function () {
     bytes = util.concat(bytes);
     arr.push(write_sub_packet(sub.signature_target, bytes));
   }
+  if (this.preferredAeadAlgorithms !== null) {
+    bytes = util.str_to_Uint8Array(util.Uint8Array_to_str(this.preferredAeadAlgorithms));
+    arr.push(write_sub_packet(sub.preferred_aead_algorithms, bytes));
+  }
+
+  const result = util.concat(arr);
+  const length = util.writeNumber(result.length, 2);
+
+  return util.concat([length, result]);
+};
+
+/**
+ * Creates Uint8Array of bytes of Issuer and Embedded Signature subpackets
+ * @returns {Uint8Array} subpacket data
+ */
+Signature.prototype.write_unhashed_sub_packets = function() {
+  const sub = enums.signatureSubpacket;
+  const arr = [];
+  let bytes;
+  if (!this.issuerKeyId.isNull() && this.issuerKeyVersion !== 5) {
+    // If the version of [the] key is greater than 4, this subpacket
+    // MUST NOT be included in the signature.
+    arr.push(write_sub_packet(sub.issuer, this.issuerKeyId.write()));
+  }
   if (this.embeddedSignature !== null) {
     arr.push(write_sub_packet(sub.embedded_signature, this.embeddedSignature.write()));
   }
@@ -297,10 +314,10 @@ Signature.prototype.write_all_sub_packets = function () {
     bytes = util.concat(bytes);
     arr.push(write_sub_packet(sub.issuer_fingerprint, bytes));
   }
-  if (this.preferredAeadAlgorithms !== null) {
-    bytes = util.str_to_Uint8Array(util.Uint8Array_to_str(this.preferredAeadAlgorithms));
-    arr.push(write_sub_packet(sub.preferred_aead_algorithms, bytes));
-  }
+  this.unhashedSubpackets.forEach(data => {
+    arr.push(packet.writeSimpleLength(data.length));
+    arr.push(data);
+  });
 
   const result = util.concat(arr);
   const length = util.writeNumber(result.length, 2);
@@ -341,18 +358,21 @@ Signature.prototype.read_sub_packet = function (bytes, trusted=true) {
   // The leftmost bit denotes a "critical" packet
   const critical = bytes[mypos] & 0x80;
   const type = bytes[mypos] & 0x7F;
-  mypos++;
 
   // GPG puts the Issuer and Signature subpackets in the unhashed area.
   // Tampering with those invalidates the signature, so we can trust them.
   // Ignore all other unhashed subpackets.
   if (!trusted && ![
     enums.signatureSubpacket.issuer,
+    enums.signatureSubpacket.issuer_fingerprint,
     enums.signatureSubpacket.embedded_signature
   ].includes(type)) {
+    this.unhashedSubpackets.push(bytes.subarray(mypos, bytes.length));
     return;
   }
 
+  mypos++;
+
   // subpacket type
   switch (type) {
     case 2:

test/general/armor.js

@@ -392,9 +392,10 @@ NJCB6+LWtabSoVIjNVgKwyKqyTLaESNwC2ogZwkdE8qPGiDFEHo4Gg9zuRof
 -----END PGP PUBLIC KEY BLOCK-----
 `;
 
-    const {keys: [key]} = await openpgp.key.readArmored(pubKey);
+    const { type, data } = await openpgp.armor.decode(pubKey);
+    const armor = await openpgp.stream.readToEnd(openpgp.armor.encode(type, data));
     expect(
-      key.armor()
+      armor
         .replace(/^(Version|Comment): .*$\r\n/mg, '')
     ).to.equal(
       pubKey

test/security/index.js

@@ -1,4 +1,5 @@
 describe('Security', function () {
   require('./message_signature_bypass');
   require('./unsigned_subpackets');
+  require('./subkey_trust');
 });

test/security/subkey_trust.js

@@ -0,0 +1,76 @@
+const openpgp = typeof window !== 'undefined' && window.openpgp ? window.openpgp : require('../../dist/openpgp');
+
+const { key, cleartext, enums, packet: { List, Signature } } = openpgp;
+
+const chai = require('chai');
+chai.use(require('chai-as-promised'));
+
+const expect = chai.expect;
+
+async function generateTestData() {
+  const victimPrivKey = await key.generate({
+    userIds: ['Victim <victim@example.com>'],
+    numBits: 1024,
+    subkeys: [{
+      sign: true
+    }]
+  });
+  victimPrivKey.revocationSignatures = [];
+
+  const attackerPrivKey = await key.generate({
+    userIds: ['Attacker <attacker@example.com>'],
+    numBits: 1024,
+    subkeys: [],
+    sign: false
+  });
+  attackerPrivKey.revocationSignatures = [];
+  const signed = await openpgp.sign({
+    message: await cleartext.fromText('I am batman'),
+    privateKeys: victimPrivKey,
+    streaming: false,
+    armor: true
+  });
+  return {
+    victimPubKey: victimPrivKey.toPublic(),
+    attackerPrivKey,
+    signed
+  };
+}
+
+async function testSubkeyTrust() {
+  // attacker only has his own private key,
+  // the victim's public key and a signed message
+  const { victimPubKey, attackerPrivKey, signed } = await generateTestData();
+
+  const pktPubVictim = victimPubKey.toPacketlist();
+  const pktPrivAttacker = attackerPrivKey.toPacketlist();
+  const dataToSign = {
+    key: attackerPrivKey.toPublic().keyPacket,
+    bind: pktPubVictim[3] // victim subkey
+  };
+  const fakeBindingSignature = new Signature();
+  fakeBindingSignature.signatureType = enums.signature.subkey_binding;
+  fakeBindingSignature.publicKeyAlgorithm = attackerPrivKey.keyPacket.algorithm;
+  fakeBindingSignature.hashAlgorithm = enums.hash.sha256;
+  fakeBindingSignature.keyFlags = [enums.keyFlags.sign_data];
+  await fakeBindingSignature.sign(attackerPrivKey.keyPacket, dataToSign);
+  const newList = new List();
+  newList.concat([
+    pktPrivAttacker[0], // attacker private key
+    pktPrivAttacker[1], // attacker user
+    pktPrivAttacker[2], // attacker self signature
+    pktPubVictim[3], // victim subkey
+    fakeBindingSignature // faked key binding
+  ]);
+  let fakeKey = new key.Key(newList);
+  fakeKey = (await key.readArmored(await fakeKey.toPublic().armor())).keys[0];
+  const verifyAttackerIsBatman = await openpgp.verify({
+    message: (await cleartext.readArmored(signed.data)),
+    publicKeys: fakeKey,
+    streaming: false
+  });
+  expect(verifyAttackerIsBatman.signatures[0].keyid.equals(victimPubKey.subKeys[0].getKeyId())).to.be.true;
+  expect(verifyAttackerIsBatman.signatures[0].valid).to.be.null;
+}
+
+it('Does not trust subkeys without Primary Key Binding Signature', testSubkeyTrust);

test/security/unsigned_subpackets.js

@@ -78,7 +78,7 @@ async function makeKeyValid() {
   // add key capability
   fake.keyFlags[0] |= enums.keyFlags.encrypt_communication;
   // create modified subpacket data
-  pusersig.unhashedSubpackets = fake.write_all_sub_packets();
+  pusersig.read_sub_packets(fake.write_hashed_sub_packets(), false);
   // reconstruct the modified key
   const newlist = new List();
   newlist.concat([pubkey, puser, pusersig]);