From 8fa3aadea235aba8e0b8bb36f9602c6d6e12e6d8 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Tue, 30 Oct 2018 10:34:11 +0100 Subject: [PATCH] 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 | 16 +++++- src/packet/signature.js | 56 +++++++++++++------- test/general/armor.js | 5 +- test/security/index.js | 1 + test/security/subkey_trust.js | 76 ++++++++++++++++++++++++++++ test/security/unsigned_subpackets.js | 2 +- 6 files changed, 133 insertions(+), 23 deletions(-) create mode 100644 test/security/subkey_trust.js diff --git a/src/key.js b/src/key.js index a66d92f6..ae23d9a6 100644 --- a/src/key.js +++ b/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; diff --git a/src/packet/signature.js b/src/packet/signature.js index 51085634..292ba128 100644 --- a/src/packet/signature.js +++ b/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: diff --git a/test/general/armor.js b/test/general/armor.js index cafc8287..d91768be 100644 --- a/test/general/armor.js +++ b/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 diff --git a/test/security/index.js b/test/security/index.js index 8c4dd7d3..5ca983e1 100644 --- a/test/security/index.js +++ b/test/security/index.js @@ -1,4 +1,5 @@ describe('Security', function () { require('./message_signature_bypass'); require('./unsigned_subpackets'); + require('./subkey_trust'); }); diff --git a/test/security/subkey_trust.js b/test/security/subkey_trust.js new file mode 100644 index 00000000..f0e8db52 --- /dev/null +++ b/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 '], + numBits: 1024, + subkeys: [{ + sign: true + }] + }); + victimPrivKey.revocationSignatures = []; + + const attackerPrivKey = await key.generate({ + userIds: ['Attacker '], + 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); diff --git a/test/security/unsigned_subpackets.js b/test/security/unsigned_subpackets.js index c92721eb..9ea55056 100644 --- a/test/security/unsigned_subpackets.js +++ b/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]);