From 577aa519e0c01ccba4598dfe7c0205490e42d235 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Wed, 31 May 2023 20:16:17 +0100 Subject: [PATCH] All: Fixes #8254: Improve selection of active E2EE key --- packages/lib/services/e2ee/utils.test.ts | 36 +++++++++++++++++-- packages/lib/services/e2ee/utils.ts | 33 ++++++++++++++++- .../synchronizer/syncInfoUtils.test.ts | 34 ++++++++++++++++++ .../services/synchronizer/syncInfoUtils.ts | 9 ++++- 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/packages/lib/services/e2ee/utils.test.ts b/packages/lib/services/e2ee/utils.test.ts index f0fb7a9db..0972371ca 100644 --- a/packages/lib/services/e2ee/utils.test.ts +++ b/packages/lib/services/e2ee/utils.test.ts @@ -1,6 +1,6 @@ -import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService, expectNotThrow, expectThrow, kvStore } from '../../testing/test-utils'; +import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService, expectNotThrow, expectThrow, kvStore, msleep } from '../../testing/test-utils'; import MasterKey from '../../models/MasterKey'; -import { migrateMasterPassword, resetMasterPassword, showMissingMasterKeyMessage, updateMasterPassword } from './utils'; +import { activeMasterKeySanityCheck, migrateMasterPassword, resetMasterPassword, showMissingMasterKeyMessage, updateMasterPassword } from './utils'; import { localSyncInfo, masterKeyById, masterKeyEnabled, setActiveMasterKeyId, setMasterKeyEnabled, setPpk } from '../synchronizer/syncInfoUtils'; import Setting from '../../models/Setting'; import { generateKeyPair, ppkPasswordIsValid } from './ppk'; @@ -147,4 +147,36 @@ describe('e2ee/utils', () => { expect(localSyncInfo().ppk.publicKey).not.toBe(previousPpk.publicKey); }); + it('should fix active key selection issues - 1', async () => { + const masterPassword1 = '111111'; + Setting.setValue('encryption.masterPassword', masterPassword1); + const mk1 = await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1)); + await msleep(1); + await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1)); + await msleep(1); + const mk3 = await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1)); + setActiveMasterKeyId(mk1.id); + setMasterKeyEnabled(mk1.id, false); + + activeMasterKeySanityCheck(); + + const syncInfo = localSyncInfo(); + expect(syncInfo.activeMasterKeyId).toBe(mk3.id); + }); + + it('should fix active key selection issues - 2', async () => { + // Should not do anything if the active key is already an enabled one. + const masterPassword1 = '111111'; + Setting.setValue('encryption.masterPassword', masterPassword1); + const mk1 = await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1)); + await msleep(1); + await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1)); + setActiveMasterKeyId(mk1.id); + + activeMasterKeySanityCheck(); + + const syncInfo = localSyncInfo(); + expect(syncInfo.activeMasterKeyId).toBe(mk1.id); + }); + }); diff --git a/packages/lib/services/e2ee/utils.ts b/packages/lib/services/e2ee/utils.ts index 28883f1c8..52d513dc8 100644 --- a/packages/lib/services/e2ee/utils.ts +++ b/packages/lib/services/e2ee/utils.ts @@ -4,7 +4,7 @@ import MasterKey from '../../models/MasterKey'; import Setting from '../../models/Setting'; import { MasterKeyEntity } from './types'; import EncryptionService from './EncryptionService'; -import { getActiveMasterKey, getActiveMasterKeyId, localSyncInfo, masterKeyEnabled, saveLocalSyncInfo, setEncryptionEnabled, SyncInfo } from '../synchronizer/syncInfoUtils'; +import { getActiveMasterKey, getActiveMasterKeyId, localSyncInfo, masterKeyEnabled, saveLocalSyncInfo, setActiveMasterKeyId, setEncryptionEnabled, SyncInfo } from '../synchronizer/syncInfoUtils'; import JoplinError from '../../JoplinError'; import { generateKeyPair, pkReencryptPrivateKey, ppkPasswordIsValid } from './ppk'; import KvStore from '../KvStore'; @@ -131,6 +131,8 @@ export async function findMasterKeyPassword(service: EncryptionService, masterKe } export async function loadMasterKeysFromSettings(service: EncryptionService) { + activeMasterKeySanityCheck(); + const masterKeys = await MasterKey.all(); const activeMasterKeyId = getActiveMasterKeyId(); @@ -153,6 +155,35 @@ export async function loadMasterKeysFromSettings(service: EncryptionService) { logger.info(`Loaded master keys: ${service.loadedMasterKeysCount()}`); } +// In some rare cases (normally should no longer be possible), a disabled master +// key end up being the active one (the one used to encrypt data). This sanity +// check resolves this by making an enabled key the active one. +export const activeMasterKeySanityCheck = () => { + const syncInfo = localSyncInfo(); + const activeMasterKeyId = syncInfo.activeMasterKeyId; + const enabledMasterKeys = syncInfo.masterKeys.filter(mk => masterKeyEnabled(mk)); + if (!enabledMasterKeys.length) return; + + if (enabledMasterKeys.find(mk => mk.id === activeMasterKeyId)) { + logger.info('activeMasterKeySanityCheck: Active key is an enabled key - nothing to do'); + return; + } + + logger.info('activeMasterKeySanityCheck: Active key is **not** an enabled key - selecting a different key as the active key...'); + + const latestMasterKey = enabledMasterKeys.reduce((acc: MasterKeyEntity, current: MasterKeyEntity) => { + if (current.created_time > acc.created_time) { + return current; + } else { + return acc; + } + }); + + logger.info('activeMasterKeySanityCheck: Selected new active key:', latestMasterKey); + + setActiveMasterKeyId(latestMasterKey.id); +}; + export function showMissingMasterKeyMessage(syncInfo: SyncInfo, notLoadedMasterKeys: string[]) { if (!syncInfo.masterKeys.length) return false; diff --git a/packages/lib/services/synchronizer/syncInfoUtils.test.ts b/packages/lib/services/synchronizer/syncInfoUtils.test.ts index ff2a82dc1..559684d48 100644 --- a/packages/lib/services/synchronizer/syncInfoUtils.test.ts +++ b/packages/lib/services/synchronizer/syncInfoUtils.test.ts @@ -120,4 +120,38 @@ describe('syncInfoUtils', () => { expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('2'); }); + it('should merge sync target info, but should not make a disabled key the active one', async () => { + const syncInfo1 = new SyncInfo(); + syncInfo1.masterKeys = [{ + id: '1', + content: 'content1', + hasBeenUsed: true, + enabled: 0, + }]; + syncInfo1.activeMasterKeyId = '1'; + + await msleep(1); + + const syncInfo2 = new SyncInfo(); + syncInfo2.masterKeys = [{ + id: '2', + content: 'content2', + enabled: 1, + hasBeenUsed: false, + }]; + syncInfo2.activeMasterKeyId = '2'; + + // Normally, if one master key has been used (1) and the other not (2), + // it should select the one that's been used regardless of timestamps. + // **However**, if the key 1 has been disabled by user, it should + // **not** be picked as the active one. Instead it should use key 2, + // because it's still enabled. + expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('2'); + + // If both key are disabled, we go back to the original logic, where we + // select the key that's been used. + syncInfo2.masterKeys[0].enabled = 0; + expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('1'); + }); + }); diff --git a/packages/lib/services/synchronizer/syncInfoUtils.ts b/packages/lib/services/synchronizer/syncInfoUtils.ts index 64779de4e..fb2b23815 100644 --- a/packages/lib/services/synchronizer/syncInfoUtils.ts +++ b/packages/lib/services/synchronizer/syncInfoUtils.ts @@ -115,13 +115,20 @@ export function localSyncInfoFromState(state: State): SyncInfo { // has already been used to encrypt data. In this case, at the moment we compare // local and remote sync info (before synchronising the data), key1.hasBeenUsed // is true, but key2.hasBeenUsed is false. +// +// 2023-05-30: Additionally, if one key is enabled and the other is not, we +// always pick the enabled one regardless of usage. const mergeActiveMasterKeys = (s1: SyncInfo, s2: SyncInfo, output: SyncInfo) => { const activeMasterKey1 = getActiveMasterKey(s1); const activeMasterKey2 = getActiveMasterKey(s2); let doDefaultAction = false; if (activeMasterKey1 && activeMasterKey2) { - if (activeMasterKey1.hasBeenUsed && !activeMasterKey2.hasBeenUsed) { + if (masterKeyEnabled(activeMasterKey1) && !masterKeyEnabled(activeMasterKey2)) { + output.setWithTimestamp(s1, 'activeMasterKeyId'); + } else if (!masterKeyEnabled(activeMasterKey1) && masterKeyEnabled(activeMasterKey2)) { + output.setWithTimestamp(s2, 'activeMasterKeyId'); + } else if (activeMasterKey1.hasBeenUsed && !activeMasterKey2.hasBeenUsed) { output.setWithTimestamp(s1, 'activeMasterKeyId'); } else if (!activeMasterKey1.hasBeenUsed && activeMasterKey2.hasBeenUsed) { output.setWithTimestamp(s2, 'activeMasterKeyId');