From b5f139b3f7867399fbbf4531e5c14ac58a4faf51 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Tue, 26 Mar 2024 17:11:44 +0100 Subject: [PATCH] Randomise v4 and v5 EdDSA signatures via custom notation, add `config.nonDeterministicEdDSASignaturesViaNotation` to disable feature EdDSA is known to be vulnerable to fault attacks which can lead to secret key extraction if two signatures over the same data can be collected. Randomly occurring bitflips in specific parts of the computation might in principle result in vulnerable faulty signatures being generated. To protect signatures generated using v4 and v5 keys from this possibility, we randomise each signature by adding a custom notation with a random value, functioning as a salt. v6 signatures do not need to rely on this, as they are non-deterministic by design. While this notation solution is interoperable, it will reveal that the signature has been generated using OpenPGP.js, which may not be desirable in some cases. For this reason, the option `config.nonDeterministicEdDSASignaturesViaNotation` (defaulting to true) has been added to turn off the feature. --- src/config/config.js | 7 ++++ src/key/helper.js | 2 +- src/packet/signature.js | 41 ++++++++++++++++++----- test/general/packet.js | 2 +- test/general/signature.js | 62 +++++++++++++++++++++++++++++++++++ test/security/subkey_trust.js | 2 +- 6 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/config/config.js b/src/config/config.js index b006a3f4..b142d342 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -255,6 +255,13 @@ export default { * @property {Array} knownNotations */ knownNotations: [], + /** + * If true, a salt notation is used to randomize EdDSA signatures generated by v4 and v5 keys (v6 signatures are always non-deterministic, by design). + * This protects EdDSA signatures from potentially leaking the secret key in case of faults (i.e. bitflips) which, in principle, could occur + * during the signing computation. + * NOTE: the notation is interoperable, but will reveal that the signature has been generated using OpenPGP.js, which may not be desirable in some cases. + */ + nonDeterministicEdDSASignaturesViaNotation: true, /** * Whether to use the the noble-curves library for curves (other than Curve25519) that are not supported by the available native crypto API. * When false, certain standard curves will not be supported (depending on the platform). diff --git a/src/key/helper.js b/src/key/helper.js index 9ca90bf4..97a68c26 100644 --- a/src/key/helper.js +++ b/src/key/helper.js @@ -219,7 +219,7 @@ export async function createSignaturePacket(dataToSign, privateKey, signingKeyPa signaturePacket.publicKeyAlgorithm = signingKeyPacket.algorithm; signaturePacket.hashAlgorithm = await getPreferredHashAlgo(privateKey, signingKeyPacket, date, userID, config); signaturePacket.rawNotations = notations; - await signaturePacket.sign(signingKeyPacket, dataToSign, date, detached); + await signaturePacket.sign(signingKeyPacket, dataToSign, date, detached, config); return signaturePacket; } diff --git a/src/packet/signature.js b/src/packet/signature.js index b650ca99..674f3ad8 100644 --- a/src/packet/signature.js +++ b/src/packet/signature.js @@ -26,6 +26,12 @@ import defaultConfig from '../config'; // Symbol to store cryptographic validity of the signature, to avoid recomputing multiple times on verification. const verified = Symbol('verified'); +// A salt notation is used to randomize EdDSA signatures, as they are known to be vulnerable to fault attacks +// which can lead to secret key extraction if two signatures over the same data can be collected (see https://github.com/jedisct1/libsodium/issues/170). +// v6 signatures do not need to rely on this notation, as they already include a separate, built-in salt. +const SALT_NOTATION_NAME = 'salt@notations.openpgpjs.org'; +const publicKeyAlgorithmsToAlwaysSalt = new Set([enums.publicKey.eddsaLegacy, enums.publicKey.ed25519, enums.publicKey.ed448]); + // GPG puts the Issuer and Signature subpackets in the unhashed area. // Tampering with those invalidates the signature, so we still trust them and parse them. // All other unhashed subpackets are ignored. @@ -195,7 +201,7 @@ class SignaturePacket { * @throws {Error} if signing failed * @async */ - async sign(key, data, date = new Date(), detached = false) { + async sign(key, data, date = new Date(), detached = false, config) { this.version = key.version; this.created = util.normalizeDate(date); @@ -205,6 +211,31 @@ class SignaturePacket { const arr = [new Uint8Array([this.version, this.signatureType, this.publicKeyAlgorithm, this.hashAlgorithm])]; + // add randomness to the signature + if (this.version === 6) { + const saltLength = saltLengthForHash(this.hashAlgorithm); + if (this.salt === null) { + this.salt = crypto.random.getRandomBytes(saltLength); + } else if (saltLength !== this.salt.length) { + throw new Error('Provided salt does not have the required length'); + } + } else if (config.nonDeterministicEdDSASignaturesViaNotation && publicKeyAlgorithmsToAlwaysSalt.has(this.publicKeyAlgorithm)) { + const saltNotations = this.rawNotations.filter(({ name }) => (name === SALT_NOTATION_NAME)); + // since re-signing the same object is not supported, it's not expected to have multiple salt notations, + // but we guard against it as a sanity check + if (saltNotations.length === 0) { + const saltValue = crypto.random.getRandomBytes(saltLengthForHash(this.hashAlgorithm)); + this.rawNotations.push({ + name: SALT_NOTATION_NAME, + value: saltValue, + humanReadable: false, + critical: false + }); + } else { + throw new Error('Unexpected existing salt notation'); + } + } + // Add hashed subpackets arr.push(this.writeHashedSubPackets()); @@ -215,14 +246,6 @@ class SignaturePacket { this.signatureData = util.concat(arr); - if (this.version === 6) { - const saltLength = saltLengthForHash(this.hashAlgorithm); - if (this.salt === null) { - this.salt = crypto.random.getRandomBytes(saltLength); - } else if (saltLength !== this.salt.length) { - throw new Error('Provided salt does not have the required length'); - } - } const toHash = this.toHash(this.signatureType, data, detached); const hash = await this.hash(this.signatureType, data, toHash, detached); diff --git a/test/general/packet.js b/test/general/packet.js index b40e2ba5..ad34ca31 100644 --- a/test/general/packet.js +++ b/test/general/packet.js @@ -1432,7 +1432,7 @@ V+HOQJQxXJkVRYa3QrFUehiMzTeqqMdgC6ZqJy7+ signature.publicKeyAlgorithm = openpgp.enums.publicKey.rsaSign; signature.signatureType = openpgp.enums.signature.text; - return signature.sign(key, literal).then(async () => { + return signature.sign(key, literal, undefined, undefined, openpgp.config).then(async () => { signed.push(literal); signed.push(signature); diff --git a/test/general/signature.js b/test/general/signature.js index 86b01476..ccc0b64e 100644 --- a/test/general/signature.js +++ b/test/general/signature.js @@ -1420,6 +1420,68 @@ DAAKCRDyMVUMT0fjjlnQAQDFHUs6TIcxrNTtEZFjUFm1M0PJ1Dng/cDW4xN80fsn expect(notations[1].critical).to.be.false; }); + it('EdDSA v4 signatures are randomized via salt notation (`config.nonDeterministicEdDSASignaturesViaNotation`)', async function() { + const v4SigningKey = await openpgp.readKey({ + armoredKey: `-----BEGIN PGP PRIVATE KEY BLOCK----- + +xVgEX8+jfBYJKwYBBAHaRw8BAQdA9GbdDjprR0sWf0R5a5IpulUauc0FsmzJ +mOYCfoowt8EAAP9UwaqC0LWWQ5RlX7mps3728vFa/If1KBVwAjk7Uqhi2BKL +zQ90ZXN0MiA8YkBhLmNvbT7CjAQQFgoAHQUCX8+jfAQLCQcIAxUICgQWAgEA +AhkBAhsDAh4BACEJEG464aV2od77FiEEIcg441MtKnyJnPDRbjrhpXah3vuR +gQD+Il6Gw2oIok4/ANyDDLBYZtKqRrMv4NcfF9DHYuAFcP4BAPhFOffyP3qU +AEZb7QPrWdLfhn8/FeSFZxJvnmupQ9sDx10EX8+jfBIKKwYBBAGXVQEFAQEH +QOSzo9cX1U2esGFClprOt0QWXNJ97228R5tKFxo6/0NoAwEIBwAA/0n4sq2i +N6/jE+6rVO4o/7LW0xahxpV1tTA6qv1Op9TwFIDCeAQYFggACQUCX8+jfAIb +DAAhCRBuOuGldqHe+xYhBCHIOONTLSp8iZzw0W464aV2od773XcA/jlmX8/c +1/zIotEkyMZB4mI+GAg3FQ6bIACFBH1sz0MzAP9Snri0P4FRZ8D5THRCJoUm +GBgpBmrf6IVv484jBswGDA== +=8rBO +-----END PGP PRIVATE KEY BLOCK-----` + }); + + const date = new Date('Tue, 25 Dec 2023 00:00:00 GMT'); + const text = 'test'; + const armoredRandomSignature1 = await openpgp.sign({ + message: await openpgp.createMessage({ text }), + signingKeys: v4SigningKey, + date, + detached: true + }); + const armoredRandomSignature2 = await openpgp.sign({ + message: await openpgp.createMessage({ text }), + signingKeys: v4SigningKey, + date, + detached: true + }); + const randomSignature1 = await openpgp.readSignature({ armoredSignature: armoredRandomSignature1 }); + const randomSignature2 = await openpgp.readSignature({ armoredSignature: armoredRandomSignature2 }); + expect(randomSignature1.packets[0].signedHashValue).to.not.deep.equal(randomSignature2.packets[0].signedHashValue); + + // ensure the signatures are verifiable, as a sanity check + const verification1 = await openpgp.verify({ message: await openpgp.createMessage({ text }), signature: randomSignature1, verificationKeys: v4SigningKey, expectSigned: true }); + expect(verification1.data).to.equal(text); + + const armoredDeterministicSignature1 = await openpgp.sign({ + message: await openpgp.createMessage({ text }), + signingKeys: v4SigningKey, + date, + detached: true, + config: { nonDeterministicEdDSASignaturesViaNotation: false } + }); + const armoredDeterministicSignature2 = await openpgp.sign({ + message: await openpgp.createMessage({ text }), + signingKeys: v4SigningKey, + date, + detached: true, + config: { nonDeterministicEdDSASignaturesViaNotation: false } + }); + const deterministicSignature1 = await openpgp.readSignature({ armoredSignature: armoredDeterministicSignature1 }); + const deterministicSignature2 = await openpgp.readSignature({ armoredSignature: armoredDeterministicSignature2 }); + expect(deterministicSignature1.packets[0].signedHashValue).to.deep.equal(deterministicSignature2.packets[0].signedHashValue); + const verification2 = await openpgp.verify({ message: await openpgp.createMessage({ text }), signature: deterministicSignature1, verificationKeys: v4SigningKey, expectSigned: true }); + expect(verification2.data).to.equal(text); + }); + it('Verify v6 cleartext signed message with openpgp.verify', async function() { // test vector from https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-08.html#name-sample-v6-certificate-trans const cleartextMessage = `-----BEGIN PGP SIGNED MESSAGE----- diff --git a/test/security/subkey_trust.js b/test/security/subkey_trust.js index e8aee45b..5d7f5fbe 100644 --- a/test/security/subkey_trust.js +++ b/test/security/subkey_trust.js @@ -50,7 +50,7 @@ export default () => it('Does not trust subkeys without Primary Key Binding Sign fakeBindingSignature.publicKeyAlgorithm = attackerPrivKey.keyPacket.algorithm; fakeBindingSignature.hashAlgorithm = enums.hash.sha256; fakeBindingSignature.keyFlags = [enums.keyFlags.signData]; - await fakeBindingSignature.sign(attackerPrivKey.keyPacket, dataToSign); + await fakeBindingSignature.sign(attackerPrivKey.keyPacket, dataToSign, undefined, undefined, openpgp.config); const newList = new PacketList(); newList.push( pktPubAttacker[0], // attacker private key