From e5313a9719b70a79ab74b4e37edee50561c850d3 Mon Sep 17 00:00:00 2001 From: Laurent Date: Wed, 13 Apr 2022 12:18:38 +0100 Subject: [PATCH] Desktop: Resolves #6338: Improve E2EE usability when accidentally creating multiple keys (#6399) --- .../EncryptionConfigScreen.tsx | 4 +- .../gui/EncryptionConfigScreen/style.scss | 1 + .../gui/MasterPasswordDialog/Dialog.tsx | 13 +++- packages/app-desktop/main.scss | 1 + packages/lib/Synchronizer.ts | 12 ++- .../lib/services/e2ee/EncryptionService.ts | 1 + packages/lib/services/e2ee/types.ts | 1 + .../synchronizer/Synchronizer.e2ee.test.ts | 28 ++++++- .../synchronizer/syncInfoUtils.test.ts | 33 ++++++++- .../services/synchronizer/syncInfoUtils.ts | 73 ++++++++++++++++++- 10 files changed, 155 insertions(+), 12 deletions(-) diff --git a/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx b/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx index caf9e94e7..a4a936e7a 100644 --- a/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx +++ b/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx @@ -137,7 +137,7 @@ const EncryptionConfigScreen = (props: Props) => { return ( onInputPasswordChange(mk, event.target.value)} />{' '} - @@ -268,7 +268,7 @@ const EncryptionConfigScreen = (props: Props) => { const buttonTitle = CommandService.instance().label('openMasterPasswordDialog'); const needPasswordMessage = !needMasterPassword ? null : ( -

{_('Your master password is needed to decrypt some of your data.')}
{_('Please click on "%s" to proceed', buttonTitle)}

+

{_('Your password is needed to decrypt some of your data.')}
{_('Please click on "%s" to proceed, or set the passwords in the "%s" list below.', buttonTitle, _('Encryption keys'))}

); return ( diff --git a/packages/app-desktop/gui/EncryptionConfigScreen/style.scss b/packages/app-desktop/gui/EncryptionConfigScreen/style.scss index b1a7536f0..75392d089 100644 --- a/packages/app-desktop/gui/EncryptionConfigScreen/style.scss +++ b/packages/app-desktop/gui/EncryptionConfigScreen/style.scss @@ -5,6 +5,7 @@ .manage-password-section > .status { display: flex; flex-direction: row; + align-items: center; } .manage-password-section > .needpassword { diff --git a/packages/app-desktop/gui/MasterPasswordDialog/Dialog.tsx b/packages/app-desktop/gui/MasterPasswordDialog/Dialog.tsx index ee12fe20a..7fd376c2b 100644 --- a/packages/app-desktop/gui/MasterPasswordDialog/Dialog.tsx +++ b/packages/app-desktop/gui/MasterPasswordDialog/Dialog.tsx @@ -34,6 +34,12 @@ export default function(props: Props) { const [updatingPassword, setUpdatingPassword] = useState(false); const [mode, setMode] = useState(Mode.Set); + const showCurrentPassword = useMemo(() => { + if ([MasterPasswordStatus.NotSet, MasterPasswordStatus.Invalid].includes(status)) return false; + if (mode === Mode.Reset) return false; + return true; + }, [status]); + const onClose = useCallback(() => { props.dispatch({ type: 'DIALOG_CLOSE', @@ -63,7 +69,7 @@ export default function(props: Props) { setUpdatingPassword(true); try { if (mode === Mode.Set) { - await updateMasterPassword(currentPassword, password1); + await updateMasterPassword(showCurrentPassword ? currentPassword : null, password1); } else if (mode === Mode.Reset) { await resetMasterPassword(EncryptionService.instance(), KvStore.instance(), ShareService.instance(), password1); } else { @@ -115,7 +121,7 @@ export default function(props: Props) { }, [password1, password2, updatingPassword, needToRepeatPassword]); useEffect(() => { - setShowPasswordForm(status === MasterPasswordStatus.NotSet); + setShowPasswordForm([MasterPasswordStatus.NotSet, MasterPasswordStatus.Invalid].includes(status)); }, [status]); useAsyncEffect(async (event: AsyncEffectEvent) => { @@ -131,8 +137,7 @@ export default function(props: Props) { function renderPasswordForm() { const renderCurrentPassword = () => { - if (status === MasterPasswordStatus.NotSet) return null; - if (mode === Mode.Reset) return null; + if (!showCurrentPassword) return null; // If the master password is in the keychain we preload it into the // field and allow displaying it. That way if the user has forgotten diff --git a/packages/app-desktop/main.scss b/packages/app-desktop/main.scss index 9b3794345..3c199ab58 100644 --- a/packages/app-desktop/main.scss +++ b/packages/app-desktop/main.scss @@ -275,4 +275,5 @@ Component-specific classes .master-password-dialog .fa-times { color: var(--joplin-color-error); + margin-left: 5px; } diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index f04d0e7b7..4d35d466e 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -22,7 +22,7 @@ import TaskQueue from './TaskQueue'; import ItemUploader from './services/synchronizer/ItemUploader'; import { FileApi, RemoteItem } from './file-api'; import JoplinDatabase from './JoplinDatabase'; -import { fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils'; +import { fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, setMasterKeyHasBeenUsed, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils'; import { getMasterPassword, setupAndDisableEncryption, setupAndEnableEncryption } from './services/e2ee/utils'; import { generateKeyPair } from './services/e2ee/ppk'; import syncDebugLog from './services/synchronizer/syncDebugLog'; @@ -439,10 +439,13 @@ export default class Synchronizer { let remoteInfo = await fetchSyncInfo(this.api()); logger.info('Sync target remote info:', remoteInfo); + let syncTargetIsNew = false; + if (!remoteInfo.version) { logger.info('Sync target is new - setting it up...'); await this.migrationHandler().upgrade(Setting.value('syncVersion')); remoteInfo = await fetchSyncInfo(this.api()); + syncTargetIsNew = true; } logger.info('Sync target is already setup - checking it...'); @@ -455,11 +458,16 @@ export default class Synchronizer { localInfo = await this.setPpkIfNotExist(localInfo, remoteInfo); + if (syncTargetIsNew && localInfo.activeMasterKeyId) { + localInfo = setMasterKeyHasBeenUsed(localInfo, localInfo.activeMasterKeyId); + } + // console.info('LOCAL', localInfo); // console.info('REMOTE', remoteInfo); if (!syncInfoEquals(localInfo, remoteInfo)) { - const newInfo = mergeSyncInfos(localInfo, remoteInfo); + let newInfo = mergeSyncInfos(localInfo, remoteInfo); + if (newInfo.activeMasterKeyId) newInfo = setMasterKeyHasBeenUsed(newInfo, newInfo.activeMasterKeyId); const previousE2EE = localInfo.e2ee; logger.info('Sync target info differs between local and remote - merging infos: ', newInfo.toObject()); diff --git a/packages/lib/services/e2ee/EncryptionService.ts b/packages/lib/services/e2ee/EncryptionService.ts index db9a52585..97cff6846 100644 --- a/packages/lib/services/e2ee/EncryptionService.ts +++ b/packages/lib/services/e2ee/EncryptionService.ts @@ -254,6 +254,7 @@ export default class EncryptionService { model.created_time = now; model.updated_time = now; model.source_application = Setting.value('appId'); + model.hasBeenUsed = false; return model; } diff --git a/packages/lib/services/e2ee/types.ts b/packages/lib/services/e2ee/types.ts index 564877acc..7899d1556 100644 --- a/packages/lib/services/e2ee/types.ts +++ b/packages/lib/services/e2ee/types.ts @@ -8,6 +8,7 @@ export interface MasterKeyEntity { content?: string; type_?: number; enabled?: number; + hasBeenUsed?: boolean; } export type RSAKeyPair = any; // Depends on implementation diff --git a/packages/lib/services/synchronizer/Synchronizer.e2ee.test.ts b/packages/lib/services/synchronizer/Synchronizer.e2ee.test.ts index 003fac1f4..4456ede9d 100644 --- a/packages/lib/services/synchronizer/Synchronizer.e2ee.test.ts +++ b/packages/lib/services/synchronizer/Synchronizer.e2ee.test.ts @@ -9,7 +9,7 @@ import ResourceFetcher from '../../services/ResourceFetcher'; import MasterKey from '../../models/MasterKey'; import BaseItem from '../../models/BaseItem'; import Synchronizer from '../../Synchronizer'; -import { getEncryptionEnabled, setEncryptionEnabled } from '../synchronizer/syncInfoUtils'; +import { fetchSyncInfo, getEncryptionEnabled, localSyncInfo, setEncryptionEnabled } from '../synchronizer/syncInfoUtils'; import { loadMasterKeysFromSettings, setupAndDisableEncryption, setupAndEnableEncryption } from '../e2ee/utils'; let insideBeforeEach = false; @@ -73,6 +73,32 @@ describe('Synchronizer.e2ee', function() { expect(!folder1_2.encryption_cipher_text).toBe(true); })); + it('should mark the key has having been used when synchronising the first time', (async () => { + setEncryptionEnabled(true); + await loadEncryptionMasterKey(); + await Folder.save({ title: 'folder1' }); + await synchronizerStart(); + + const localInfo = localSyncInfo(); + const remoteInfo = await fetchSyncInfo(fileApi()); + expect(localInfo.masterKeys[0].hasBeenUsed).toBe(true); + expect(remoteInfo.masterKeys[0].hasBeenUsed).toBe(true); + })); + + it('should mark the key has having been used when synchronising after enabling encryption', (async () => { + await Folder.save({ title: 'folder1' }); + await synchronizerStart(); + + setEncryptionEnabled(true); + await loadEncryptionMasterKey(); + await synchronizerStart(); + + const localInfo = localSyncInfo(); + const remoteInfo = await fetchSyncInfo(fileApi()); + expect(localInfo.masterKeys[0].hasBeenUsed).toBe(true); + expect(remoteInfo.masterKeys[0].hasBeenUsed).toBe(true); + })); + it('should enable encryption automatically when downloading new master key (and none was previously available)',(async () => { // Enable encryption on client 1 and sync an item setEncryptionEnabled(true); diff --git a/packages/lib/services/synchronizer/syncInfoUtils.test.ts b/packages/lib/services/synchronizer/syncInfoUtils.test.ts index bbdb299e0..a0f813333 100644 --- a/packages/lib/services/synchronizer/syncInfoUtils.test.ts +++ b/packages/lib/services/synchronizer/syncInfoUtils.test.ts @@ -1,6 +1,6 @@ -import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService } from '../../testing/test-utils'; +import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService, msleep } from '../../testing/test-utils'; import MasterKey from '../../models/MasterKey'; -import { masterKeyEnabled, setMasterKeyEnabled, SyncInfo, syncInfoEquals } from './syncInfoUtils'; +import { masterKeyEnabled, mergeSyncInfos, setMasterKeyEnabled, SyncInfo, syncInfoEquals } from './syncInfoUtils'; describe('syncInfoUtils', function() { @@ -92,4 +92,33 @@ describe('syncInfoUtils', function() { } }); + it('should merge sync target info and takes into account usage of master key - 1', async () => { + const syncInfo1 = new SyncInfo(); + syncInfo1.masterKeys = [{ + id: '1', + content: 'content1', + hasBeenUsed: true, + }]; + syncInfo1.activeMasterKeyId = '1'; + + await msleep(1); + + const syncInfo2 = new SyncInfo(); + syncInfo2.masterKeys = [{ + id: '2', + content: 'content2', + hasBeenUsed: false, + }]; + syncInfo2.activeMasterKeyId = '2'; + + // If one master key has been used and the other not, it should select + // the one that's been used regardless of timestamps. + expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('1'); + + // If both master keys have been used it should rely on timestamp + // (latest modified is picked). + syncInfo2.masterKeys[0].hasBeenUsed = true; + expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('2'); + }); + }); diff --git a/packages/lib/services/synchronizer/syncInfoUtils.ts b/packages/lib/services/synchronizer/syncInfoUtils.ts index 38a9ccc45..64779de4e 100644 --- a/packages/lib/services/synchronizer/syncInfoUtils.ts +++ b/packages/lib/services/synchronizer/syncInfoUtils.ts @@ -90,14 +90,62 @@ export function localSyncInfoFromState(state: State): SyncInfo { return new SyncInfo(state.settings['syncInfoCache']); } +// When deciding which master key should be active we should take into account +// whether it's been used or not. If it's been used before it should most likely +// remain the active one, regardless of timestamps. This is because the extra +// key was most likely created by mistake by the user, in particular in this +// kind of scenario: +// +// - Client 1 setup sync with sync target +// - Client 1 enable encryption +// - Client 1 sync +// +// Then user 2 does the same: +// +// - Client 2 setup sync with sync target +// - Client 2 enable encryption +// - Client 2 sync +// +// The problem is that enabling encryption was not needed since it was already +// done (and recorded in info.json) on the sync target. As a result an extra key +// has been created and it has been set as the active one, but we shouldn't use +// it. Instead the key created by client 1 should be used and made active again. +// +// And we can do this using the "hasBeenUsed" field which tells us which keys +// 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. +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) { + output.setWithTimestamp(s1, 'activeMasterKeyId'); + } else if (!activeMasterKey1.hasBeenUsed && activeMasterKey2.hasBeenUsed) { + output.setWithTimestamp(s2, 'activeMasterKeyId'); + } else { + doDefaultAction = true; + } + } else { + doDefaultAction = true; + } + + if (doDefaultAction) { + output.setWithTimestamp(s1.keyTimestamp('activeMasterKeyId') > s2.keyTimestamp('activeMasterKeyId') ? s1 : s2, 'activeMasterKeyId'); + } +}; + export function mergeSyncInfos(s1: SyncInfo, s2: SyncInfo): SyncInfo { const output: SyncInfo = new SyncInfo(); output.setWithTimestamp(s1.keyTimestamp('e2ee') > s2.keyTimestamp('e2ee') ? s1 : s2, 'e2ee'); - output.setWithTimestamp(s1.keyTimestamp('activeMasterKeyId') > s2.keyTimestamp('activeMasterKeyId') ? s1 : s2, 'activeMasterKeyId'); output.setWithTimestamp(s1.keyTimestamp('ppk') > s2.keyTimestamp('ppk') ? s1 : s2, 'ppk'); output.version = s1.version > s2.version ? s1.version : s2.version; + mergeActiveMasterKeys(s1, s2, output); + output.masterKeys = s1.masterKeys.slice(); for (const mk of s2.masterKeys) { @@ -154,6 +202,14 @@ export class SyncInfo { this.activeMasterKeyId_ = 'activeMasterKeyId' in s ? s.activeMasterKeyId : { value: '', updatedTime: 0 }; this.masterKeys_ = 'masterKeys' in s ? s.masterKeys : []; this.ppk_ = 'ppk' in s ? s.ppk : { value: null, updatedTime: 0 }; + + // Migration for master keys that didn't have "hasBeenUsed" property - + // in that case we assume they've been used at least once. + for (const mk of this.masterKeys_) { + if (!('hasBeenUsed' in mk) || mk.hasBeenUsed === undefined) { + mk.hasBeenUsed = true; + } + } } public setWithTimestamp(fromSyncInfo: SyncInfo, propName: string) { @@ -275,6 +331,21 @@ export function setMasterKeyEnabled(mkId: string, enabled: boolean = true) { saveLocalSyncInfo(s); } +export const setMasterKeyHasBeenUsed = (s: SyncInfo, mkId: string) => { + const idx = s.masterKeys.findIndex(mk => mk.id === mkId); + if (idx < 0) throw new Error(`No such master key: ${mkId}`); + + s.masterKeys[idx] = { + ...s.masterKeys[idx], + hasBeenUsed: true, + updated_time: Date.now(), + }; + + saveLocalSyncInfo(s); + + return s; +}; + export function masterKeyEnabled(mk: MasterKeyEntity): boolean { if ('enabled' in mk) return !!mk.enabled; return true;