From 4713282bb124a9f30b325f2ebc1b2638606d622e Mon Sep 17 00:00:00 2001 From: larabr Date: Tue, 22 Mar 2022 15:11:51 +0100 Subject: [PATCH] Throw on empty passphrase in `encryptKey` and `SecretKeyPacket.encrypt` (#1508) Breaking change: `openpgp.encryptKey` now throws if an empty string is given as passphrase. The operation used to succeed, but the resulting key was left in an inconsistent state, and e.g. serialization would not be possible. Non-breaking changes: - `options.passphrase` in `generateKey` and `reformatKey` now defaults to `undefined` instead of empty string. Passing an empty string does not throw for now, but this might change in the future to align with `encryptKey`'s behaviour. - In TS, add `GenerateKeyOptions` as alias of `KeyOptions`, to clarify its scope. --- openpgp.d.ts | 9 +++++---- src/openpgp.js | 8 ++++---- src/packet/secret_key.js | 7 ++----- test/general/openpgp.js | 8 ++++++++ 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/openpgp.d.ts b/openpgp.d.ts index a30cbdc6e..11a448e23 100644 --- a/openpgp.d.ts +++ b/openpgp.d.ts @@ -20,9 +20,9 @@ export function readPrivateKey(options: { armoredKey: string, config?: PartialCo export function readPrivateKey(options: { binaryKey: Uint8Array, config?: PartialConfig }): Promise; export function readPrivateKeys(options: { armoredKeys: string, config?: PartialConfig }): Promise; export function readPrivateKeys(options: { binaryKeys: Uint8Array, config?: PartialConfig }): Promise; -export function generateKey(options: KeyOptions & { format?: 'armored' }): Promise & { revocationCertificate: string }>; -export function generateKey(options: KeyOptions & { format: 'binary' }): Promise & { revocationCertificate: string }>; -export function generateKey(options: KeyOptions & { format: 'object' }): Promise; +export function generateKey(options: GenerateKeyOptions & { format?: 'armored' }): Promise & { revocationCertificate: string }>; +export function generateKey(options: GenerateKeyOptions & { format: 'binary' }): Promise & { revocationCertificate: string }>; +export function generateKey(options: GenerateKeyOptions & { format: 'object' }): Promise; export function decryptKey(options: { privateKey: PrivateKey; passphrase?: MaybeArray; config?: PartialConfig }): Promise; export function encryptKey(options: { privateKey: PrivateKey; passphrase?: MaybeArray; config?: PartialConfig }): Promise; export function reformatKey(options: { privateKey: PrivateKey; userIDs?: MaybeArray; passphrase?: string; keyExpirationTime?: number; date?: Date, format?: 'armored', config?: PartialConfig }): Promise & { revocationCertificate: string }>; @@ -662,7 +662,7 @@ interface KeyPair { export type EllipticCurveName = 'ed25519' | 'curve25519' | 'p256' | 'p384' | 'p521' | 'secp256k1' | 'brainpoolP256r1' | 'brainpoolP384r1' | 'brainpoolP512r1'; -interface KeyOptions { +interface GenerateKeyOptions { userIDs: MaybeArray; passphrase?: string; type?: 'ecc' | 'rsa'; @@ -674,6 +674,7 @@ interface KeyOptions { format?: 'armored' | 'object' | 'binary'; config?: PartialConfig; } +export type KeyOptions = GenerateKeyOptions; interface SubkeyOptions { type?: 'ecc' | 'rsa'; diff --git a/src/openpgp.js b/src/openpgp.js index 56a01c1f0..968c01ecc 100644 --- a/src/openpgp.js +++ b/src/openpgp.js @@ -37,7 +37,7 @@ import { checkKeyRequirements } from './key/helper'; * @param {Object} options * @param {Object|Array} options.userIDs - User IDs as objects: `{ name: 'Jo Doe', email: 'info@jo.com' }` * @param {'ecc'|'rsa'} [options.type='ecc'] - The primary key algorithm type: ECC (default) or RSA - * @param {String} [options.passphrase=(not protected)] - The passphrase used to encrypt the generated private key. If omitted, the key won't be encrypted. + * @param {String} [options.passphrase=(not protected)] - The passphrase used to encrypt the generated private key. If omitted or empty, the key won't be encrypted. * @param {Number} [options.rsaBits=4096] - Number of bits for RSA keys * @param {String} [options.curve='curve25519'] - Elliptic curve for ECC keys: * curve25519 (default), p256, p384, p521, secp256k1, @@ -53,7 +53,7 @@ import { checkKeyRequirements } from './key/helper'; * @async * @static */ -export async function generateKey({ userIDs = [], passphrase = '', type = 'ecc', rsaBits = 4096, curve = 'curve25519', keyExpirationTime = 0, date = new Date(), subkeys = [{}], format = 'armored', config, ...rest }) { +export async function generateKey({ userIDs = [], passphrase, type = 'ecc', rsaBits = 4096, curve = 'curve25519', keyExpirationTime = 0, date = new Date(), subkeys = [{}], format = 'armored', config, ...rest }) { config = { ...defaultConfig, ...config }; checkConfig(config); userIDs = toArray(userIDs); const unknownOptions = Object.keys(rest); if (unknownOptions.length > 0) throw new Error(`Unknown option: ${unknownOptions.join(', ')}`); @@ -86,7 +86,7 @@ export async function generateKey({ userIDs = [], passphrase = '', type = 'ecc', * @param {Object} options * @param {PrivateKey} options.privateKey - Private key to reformat * @param {Object|Array} options.userIDs - User IDs as objects: `{ name: 'Jo Doe', email: 'info@jo.com' }` - * @param {String} [options.passphrase=(not protected)] - The passphrase used to encrypt the reformatted private key. If omitted, the key won't be encrypted. + * @param {String} [options.passphrase=(not protected)] - The passphrase used to encrypt the reformatted private key. If omitted or empty, the key won't be encrypted. * @param {Number} [options.keyExpirationTime=0 (never expires)] - Number of seconds from the key creation time after which the key expires * @param {Date} [options.date] - Override the creation date of the key signatures. If the key was previously used to sign messages, it is recommended * to set the same date as the key creation time to ensure that old message signatures will still be verifiable using the reformatted key. @@ -97,7 +97,7 @@ export async function generateKey({ userIDs = [], passphrase = '', type = 'ecc', * @async * @static */ -export async function reformatKey({ privateKey, userIDs = [], passphrase = '', keyExpirationTime = 0, date, format = 'armored', config, ...rest }) { +export async function reformatKey({ privateKey, userIDs = [], passphrase, keyExpirationTime = 0, date, format = 'armored', config, ...rest }) { config = { ...defaultConfig, ...config }; checkConfig(config); userIDs = toArray(userIDs); const unknownOptions = Object.keys(rest); if (unknownOptions.length > 0) throw new Error(`Unknown option: ${unknownOptions.join(', ')}`); diff --git a/src/packet/secret_key.js b/src/packet/secret_key.js index c1ef8c3e5..016fbd4f4 100644 --- a/src/packet/secret_key.js +++ b/src/packet/secret_key.js @@ -275,11 +275,8 @@ class SecretKeyPacket extends PublicKeyPacket { throw new Error('Key packet is already encrypted'); } - if (this.isDecrypted() && !passphrase) { - this.s2kUsage = 0; - return; - } else if (!passphrase) { - throw new Error('The key must be decrypted before removing passphrase protection.'); + if (!passphrase) { + throw new Error('A non-empty passphrase is required for key encryption.'); } this.s2k = new S2K(config); diff --git a/test/general/openpgp.js b/test/general/openpgp.js index eb3793332..3f5aadd8b 100644 --- a/test/general/openpgp.js +++ b/test/general/openpgp.js @@ -1192,6 +1192,14 @@ module.exports = () => describe('OpenPGP.js public api tests', function() { expect(unlocked.isDecrypted()).to.be.true; }); + it('should throw on empty passphrase', async function() { + const { privateKey } = await openpgp.generateKey({ userIDs: [{ name: 'test', email: 'test@test.com' }], format: 'object' }); + await expect(openpgp.encryptKey({ + privateKey, + passphrase: '' + })).to.be.rejectedWith(/passphrase is required for key encryption/); + }); + it('should support multiple passphrases', async function() { const { privateKey } = await openpgp.generateKey({ userIDs: [{ name: 'test', email: 'test@test.com' }], format: 'object' }); const passphrases = ['123', '456'];