From 19e3c344fd775a219fa90f890b94691026f57b3e Mon Sep 17 00:00:00 2001 From: Wiktor Kwapisiewicz Date: Mon, 23 Jul 2018 22:01:39 +0200 Subject: [PATCH] Fix verification of User Attributes This change corrects verification of certifications over User Attributes (such as photos). Before this change the code did not differentiate between User IDs and User Attributes as both of them were stored in `data.userid` [0] and incorrectly used the User ID constant (0xB4) for both cases. This change fixes the bug by storing User IDs in `userId` property and User Attributes in `userAttribute` property. The check for property existence has been modified to avoid comparisons with `undefined` as the `User` class sets `null` for not assigned packets instead of `undefined`. Only data structures for signing and verification were modified and not the properties used in the `User` class. [0]: https://github.com/openpgpjs/openpgpjs/blob/11b2d2de3cf39949e5fdfb2b95d027d8329c41fe/src/key.js#L872 --- src/key.js | 33 +++++++++++++++++++++++++-------- src/packet/signature.js | 10 +++++----- test/general/packet.js | 2 +- test/general/signature.js | 33 ++++++++++++++++++++++++++++++++- test/general/x25519.js | 8 ++++---- 5 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/key.js b/src/key.js index b33edd97..4b6fdf69 100644 --- a/src/key.js +++ b/src/key.js @@ -536,7 +536,7 @@ Key.prototype.getPrimaryUser = async function(date=new Date(), userId={}) { }).pop(); const { user, selfCertification: cert } = primaryUser; const primaryKey = this.keyPacket; - const dataToVerify = { userid: user.userId , key: primaryKey }; + const dataToVerify = { userId: user.userId, key: primaryKey }; // skip if certificates is invalid, revoked, or expired // eslint-disable-next-line no-await-in-loop if (!(cert.verified || await cert.verify(primaryKey, dataToVerify))) { @@ -828,8 +828,12 @@ User.prototype.toPacketlist = function() { * @async */ User.prototype.sign = async function(primaryKey, privateKeys) { - const dataToSign = { userid: this.userId || this.userAttribute, key: primaryKey }; - const user = new User(dataToSign.userid); + const dataToSign = { + userId: this.userId, + userAttribute: this.userAttribute, + key: primaryKey + }; + const user = new User(dataToSign.userId || dataToSign.userAttribute); user.otherCertifications = await Promise.all(privateKeys.map(async function(privateKey) { if (privateKey.isPublic()) { throw new Error('Need private key for signing'); @@ -869,7 +873,8 @@ User.prototype.isRevoked = async function(primaryKey, certificate, key, date=new return isDataRevoked( primaryKey, { key: primaryKey, - userid: this.userId || this.userAttribute + userId: this.userId, + userAttribute: this.userAttribute }, this.revocationSignatures, certificate, key, date ); }; @@ -909,7 +914,11 @@ export async function createSignaturePacket(dataToSign, privateKey, signingKeyPa User.prototype.verifyCertificate = async function(primaryKey, certificate, keys, date=new Date()) { const that = this; const keyid = certificate.issuerKeyId; - const dataToVerify = { userid: this.userId || this.userAttribute, key: primaryKey }; + const dataToVerify = { + userId: this.userId, + userAttribute: this.userAttribute, + key: primaryKey + }; const results = await Promise.all(keys.map(async function(key) { if (!key.getKeyIds().some(id => id.equals(keyid))) { return; } const signingKey = await key.getSigningKey(keyid, date); @@ -961,7 +970,11 @@ User.prototype.verify = async function(primaryKey) { return enums.keyStatus.no_self_cert; } const that = this; - const dataToVerify = { userid: this.userId || this.userAttribute, key: primaryKey }; + const dataToVerify = { + userId: this.userId, + userAttribute: this.userAttribute, + key: primaryKey + }; // TODO replace when Promise.some or Promise.any are implemented const results = [enums.keyStatus.invalid].concat( await Promise.all(this.selfCertifications.map(async function(selfCertification) { @@ -987,7 +1000,11 @@ User.prototype.verify = async function(primaryKey) { * module:packet.SecretSubkey} primaryKey primary key used for validation */ User.prototype.update = async function(user, primaryKey) { - const dataToVerify = { userid: this.userId || this.userAttribute, key: primaryKey }; + const dataToVerify = { + userId: this.userId, + userAttribute: this.userAttribute, + key: primaryKey + }; // self signatures await mergeSignatures(user, this, 'selfCertifications', async function(srcSelfSig) { return srcSelfSig.verified || srcSelfSig.verify(primaryKey, dataToVerify); @@ -1391,7 +1408,7 @@ async function wrapKeyObject(secretKeyPacket, secretSubkeyPackets, options) { userIdPacket.format(userId); const dataToSign = {}; - dataToSign.userid = userIdPacket; + dataToSign.userId = userIdPacket; dataToSign.key = secretKeyPacket; const signaturePacket = new packet.Signature(options.date); signaturePacket.signatureType = enums.signature.cert_generic; diff --git a/src/packet/signature.js b/src/packet/signature.js index 51cb70a7..6208f2fc 100644 --- a/src/packet/signature.js +++ b/src/packet/signature.js @@ -594,14 +594,14 @@ Signature.prototype.toSign = function (type, data) { let packet; let tag; - if (data.userid !== undefined) { + if (data.userId) { tag = 0xB4; - packet = data.userid; - } else if (data.userattribute !== undefined) { + packet = data.userId; + } else if (data.userAttribute) { tag = 0xD1; - packet = data.userattribute; + packet = data.userAttribute; } else { - throw new Error('Either a userid or userattribute packet needs to be ' + + throw new Error('Either a userId or userAttribute packet needs to be ' + 'supplied for certification.'); } diff --git a/test/general/packet.js b/test/general/packet.js index c9b55efe..fbb2001f 100644 --- a/test/general/packet.js +++ b/test/general/packet.js @@ -662,7 +662,7 @@ describe("Packet", function() { return Promise.all([ expect(key[2].verify(key[0], { - userid: key[1], + userId: key[1], key: key[0] })).to.eventually.be.true, expect(key[4].verify(key[0], diff --git a/test/general/signature.js b/test/general/signature.js index 039b230e..0a6131d2 100644 --- a/test/general/signature.js +++ b/test/general/signature.js @@ -876,7 +876,7 @@ yYDnCgA= it('Verify V3 certification signature', function(done) { const pubKey = openpgp.key.readArmored(pub_v3).keys[0]; - expect(pubKey.users[0].selfCertifications[0].verify(pubKey.primaryKey, {key: pubKey.primaryKey, userid: pubKey.users[0].userId})).to.eventually.be.true.notify(done); + expect(pubKey.users[0].selfCertifications[0].verify(pubKey.primaryKey, {key: pubKey.primaryKey, userId: pubKey.users[0].userId})).to.eventually.be.true.notify(done); }); it('Write unhashed subpackets', function() { @@ -1015,4 +1015,35 @@ yYDnCgA= }); }); + it('Verify signed UserIDs and User Attributes', async function() { + const armoredKeyWithPhoto = `-----BEGIN PGP PUBLIC KEY BLOCK----- + +mI0EW1CJGAEEAM+BzuFzcYk9HttmDbjGexQ8dfme074Q5PuHas3PBISPm0AwmnDM +tzjlcrrg2VGuLqHvNF600w2ZgOo2gElNYCOas1q/fVFuIgJ4SUduNOEe/JnIW4uP +iEGU9l6zOVVgTc/nGVpZdvHgvOL8nl9BKHtWEnMD3Du7UYAm+Avshu9jABEBAAG0 +AViI1AQTAQoAPhYhBKcH118Rrg0wLBrTk5IyMikCym+4BQJbUIkYAhsDBQkDwmcA +BQsJCAcDBRUKCQgLBRYDAgEAAh4BAheAAAoJEJIyMikCym+4K8oEAJc7YFiNau6V +HTVK4cTvWU5MuYiejejFZai4ELUJy+WF6cZYrLuF/z/kRt8B7hpumXChPCUlT0q7 +FWypQtA3leu83DGMXqhfS80h2S1+VLmDVVWKQXOwgOb44jT9F08bDU5QK08SkjF8 +/EirIy8ANzdwCA4rHytIS2yx6tLlthvX0cBwwG4BEAABAQAAAAAAAAAAAAAAAP/Y +/+AAEEpGSUYAAQEAAAEAAQAA/9sAQwABAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEB +AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEB/9sAQwEBAQEB +AQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEB +AQEBAQEBAQEBAQEB/8AAEQgAAQABAwEiAAIRAQMRAf/EABUAAQEAAAAAAAAAAAAA +AAAAAAAK/8QAFBABAAAAAAAAAAAAAAAAAAAAAP/EABQBAQAAAAAAAAAAAAAAAAAA +AAD/xAAUEQEAAAAAAAAAAAAAAAAAAAAA/9oADAMBAAIRAxEAPwC/gAH/2YjUBBMB +CgA+FiEEpwfXXxGuDTAsGtOTkjIyKQLKb7gFAltQimUCGwMFCQPCZwAFCwkIBwMF +FQoJCAsFFgMCAQACHgECF4AACgkQkjIyKQLKb7gm/wQAiyZF89qr8hf3XQNJ6Ir/ +QtaniPcesjrYCIE47ZfeDYpBTPeiMm295o2dZXVJS4ItllYsplASw5DJiIMnQKlJ +mbXakYFzzclTa/JrKzFYCy/DPT95xK+633omgrIUgJodizoKJE7XeB2U6aRUJJ4O +iTuGu4fEU1UligAXSrZmCdE= +=VK6I +-----END PGP PUBLIC KEY BLOCK-----`; + + const key = openpgp.key.readArmored(armoredKeyWithPhoto).keys[0]; + for (const user of key.users) { + expect(await user.verify(key.primaryKey)).to.equal(openpgp.enums.keyStatus.valid); + } + }); + }); diff --git a/test/general/x25519.js b/test/general/x25519.js index 4d3890cb..adfda585 100644 --- a/test/general/x25519.js +++ b/test/general/x25519.js @@ -240,7 +240,7 @@ describe('X25519 Cryptography', function () { // Self Certificate is valid const user = hi.users[0]; await expect(user.selfCertifications[0].verify( - primaryKey, { userid: user.userId, key: primaryKey } + primaryKey, { userId: user.userId, key: primaryKey } )).to.eventually.be.true; await expect(user.verifyCertificate( primaryKey, user.selfCertifications[0], [hi.toPublic()] @@ -260,7 +260,7 @@ describe('X25519 Cryptography', function () { // Self Certificate is valid const user = bye.users[0]; await expect(user.selfCertifications[0].verify( - bye.primaryKey, { userid: user.userId, key: bye.primaryKey } + bye.primaryKey, { userId: user.userId, key: bye.primaryKey } )).to.eventually.be.true; await expect(user.verifyCertificate( bye.primaryKey, user.selfCertifications[0], [bye.toPublic()] @@ -270,7 +270,7 @@ describe('X25519 Cryptography', function () { // Hi trusts Bye! bye.toPublic().signPrimaryUser([hi]).then(trustedBye => { expect(trustedBye.users[0].otherCertifications[0].verify( - primaryKey, { userid: user.userId, key: bye.toPublic().primaryKey } + primaryKey, { userId: user.userId, key: bye.toPublic().primaryKey } )).to.eventually.be.true; }), // Signing message @@ -539,7 +539,7 @@ describe('X25519 Cryptography', function () { expect(results.user).to.exist; const user = results.user; expect(user.selfCertifications[0].verify( - hi.primaryKey, {userid: user.userId, key: hi.primaryKey} + hi.primaryKey, {userId: user.userId, key: hi.primaryKey} )).to.eventually.be.true; expect(user.verifyCertificate( hi.primaryKey, user.selfCertifications[0], [hi]