From c68bd960cea7557907891578f7a1b44cdc5f1147 Mon Sep 17 00:00:00 2001 From: larabr Date: Tue, 2 Apr 2024 17:37:57 +0200 Subject: [PATCH] Randomise v4 and v5 signatures via custom notation, add `config.nonDeterministicSignaturesViaNotation` to disable feature (#1737) 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. For simplicity, we add the salt to all algos, not just EdDSA, as it may also serve as protection in case of weaknesses in the hash algo, potentially hindering e.g. some chosen-prefix attacks. 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.nonDeterministicSignaturesViaNotation` (defaulting to true) has been added to turn off the feature. --- src/config/config.js | 8 +++++ src/key/helper.js | 4 +-- src/packet/signature.js | 43 +++++++++++++++++----- test/general/packet.js | 2 +- test/general/signature.js | 67 ++++++++++++++++++++++++++++++++++- test/security/subkey_trust.js | 2 +- 6 files changed, 112 insertions(+), 14 deletions(-) diff --git a/src/config/config.js b/src/config/config.js index d5c9c14c..866a0c34 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -250,6 +250,14 @@ export default { * @property {Array} knownNotations */ knownNotations: [], + /** + * If true, a salt notation is used to randomize 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. It is added to signatures of any algo for simplicity, and as it may also serve as protection in case of + * weaknesses in the hash algo, potentially hindering e.g. some chosen-prefix attacks. + * 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. + */ + nonDeterministicSignaturesViaNotation: 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 d479b081..9425ebf8 100644 --- a/src/key/helper.js +++ b/src/key/helper.js @@ -218,8 +218,8 @@ export async function createSignaturePacket(dataToSign, privateKey, signingKeyPa Object.assign(signaturePacket, signatureProperties); signaturePacket.publicKeyAlgorithm = signingKeyPacket.algorithm; signaturePacket.hashAlgorithm = await getPreferredHashAlgo(privateKey, signingKeyPacket, date, userID, config); - signaturePacket.rawNotations = notations; - await signaturePacket.sign(signingKeyPacket, dataToSign, date, detached); + signaturePacket.rawNotations = [...notations]; + await signaturePacket.sign(signingKeyPacket, dataToSign, date, detached, config); return signaturePacket; } diff --git a/src/packet/signature.js b/src/packet/signature.js index b650ca99..43fc2002 100644 --- a/src/packet/signature.js +++ b/src/packet/signature.js @@ -26,6 +26,14 @@ 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 signatures. +// This is to protect EdDSA signatures in particular, which are known to be vulnerable to fault attacks +// leading to secret key extraction if two signatures over the same data can be collected (see https://github.com/jedisct1/libsodium/issues/170). +// For simplicity, we add the salt to all algos, as it may also serve as protection in case of weaknesses in the hash algo, potentially hindering e.g. +// some chosen-prefix attacks. +// 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'; + // 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 +203,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 +213,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.nonDeterministicSignaturesViaNotation) { + 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 +248,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..20bd0c30 100644 --- a/test/general/signature.js +++ b/test/general/signature.js @@ -1409,7 +1409,7 @@ DAAKCRDyMVUMT0fjjlnQAQDFHUs6TIcxrNTtEZFjUFm1M0PJ1Dng/cDW4xN80fsn }); expect(await sig.verified).to.be.true; const { packets: [{ rawNotations: notations }] } = await sig.signature; - expect(notations).to.have.length(2); + expect(notations).to.have.length(3); expect(notations[0].name).to.equal('test@example.com'); expect(notations[0].value).to.deep.equal(new Uint8Array([116, 101, 115, 116])); expect(notations[0].humanReadable).to.be.true; @@ -1418,6 +1418,71 @@ DAAKCRDyMVUMT0fjjlnQAQDFHUs6TIcxrNTtEZFjUFm1M0PJ1Dng/cDW4xN80fsn expect(notations[1].value).to.deep.equal(new Uint8Array([0, 1, 2, 3])); expect(notations[1].humanReadable).to.be.false; expect(notations[1].critical).to.be.false; + expect(notations[2].name).to.equal('salt@notations.openpgpjs.org'); + expect(notations[2].humanReadable).to.be.false; + expect(notations[2].critical).to.be.false; + }); + + it('v4 signatures are randomized via salt notation (`config.nonDeterministicSignaturesViaNotation`)', 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: { nonDeterministicSignaturesViaNotation: false } + }); + const armoredDeterministicSignature2 = await openpgp.sign({ + message: await openpgp.createMessage({ text }), + signingKeys: v4SigningKey, + date, + detached: true, + config: { nonDeterministicSignaturesViaNotation: 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() { 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