From f53642139028a9bbb32402dacfa06db042ce97af Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Mon, 21 Mar 2022 15:06:34 -0700 Subject: [PATCH] Better merging for changed keys in storage service --- ts/SignalProtocolStore.ts | 141 +++---------------- ts/models/conversations.ts | 23 +-- ts/services/storageRecordOps.ts | 27 +++- ts/test-electron/SignalProtocolStore_test.ts | 56 +++----- 4 files changed, 78 insertions(+), 169 deletions(-) diff --git a/ts/SignalProtocolStore.ts b/ts/SignalProtocolStore.ts index 45a8284feb38..5e4a36aa31f1 100644 --- a/ts/SignalProtocolStore.ts +++ b/ts/SignalProtocolStore.ts @@ -1763,84 +1763,8 @@ export class SignalProtocolStore extends EventsMixin { return VerifiedStatus.DEFAULT; } - // Resolves to true if a new identity key was saved - processContactSyncVerificationState( - uuid: UUID, - verifiedStatus: number, - publicKey: Uint8Array - ): Promise { - if (verifiedStatus === VerifiedStatus.UNVERIFIED) { - return this.processUnverifiedMessage(uuid, verifiedStatus, publicKey); - } - return this.processVerifiedMessage(uuid, verifiedStatus, publicKey); - } - - // This function encapsulates the non-Java behavior, since the mobile apps don't - // currently receive contact syncs and therefore will see a verify sync with - // UNVERIFIED status - async processUnverifiedMessage( - uuid: UUID, - verifiedStatus: number, - publicKey?: Uint8Array - ): Promise { - if (uuid === null || uuid === undefined) { - throw new Error('processUnverifiedMessage: uuid was undefined/null'); - } - if (publicKey !== undefined && !(publicKey instanceof Uint8Array)) { - throw new Error('processUnverifiedMessage: Invalid public key'); - } - - const identityRecord = await this.getOrMigrateIdentityRecord(uuid); - - let isEqual = false; - - if (identityRecord && publicKey) { - isEqual = constantTimeEqual(publicKey, identityRecord.publicKey); - } - - if ( - identityRecord && - isEqual && - identityRecord.verified !== VerifiedStatus.UNVERIFIED - ) { - await this.setVerified(uuid, verifiedStatus, publicKey); - return false; - } - - if (!identityRecord || !isEqual) { - await this.saveIdentityWithAttributes(uuid, { - publicKey, - verified: verifiedStatus, - firstUse: false, - timestamp: Date.now(), - nonblockingApproval: true, - }); - - if (identityRecord && !isEqual) { - try { - this.trigger('keychange', uuid); - } catch (error) { - log.error( - 'processUnverifiedMessage: error triggering keychange:', - error && error.stack ? error.stack : error - ); - } - - await this.archiveAllSessions(uuid); - - return true; - } - } - - // The situation which could get us here is: - // 1. had a previous key - // 2. new key is the same - // 3. desired new status is same as what we had before - return false; - } - - // This matches the Java method as of - // https://github.com/signalapp/Signal-Android/blob/d0bb68e1378f689e4d10ac6a46014164992ca4e4/src/org/thoughtcrime/securesms/util/IdentityUtil.java#L188 + // See https://github.com/signalapp/Signal-iOS-Private/blob/e32c2dff0d03f67467b4df621d84b11412d50cdb/SignalServiceKit/src/Messages/OWSIdentityManager.m#L317 + // for reference. async processVerifiedMessage( uuid: UUID, verifiedStatus: number, @@ -1864,55 +1788,34 @@ export class SignalProtocolStore extends EventsMixin { isEqual = constantTimeEqual(publicKey, identityRecord.publicKey); } - if (!identityRecord && verifiedStatus === VerifiedStatus.DEFAULT) { - log.info('processVerifiedMessage: No existing record for default status'); - return false; - } - - if ( - identityRecord && - isEqual && - identityRecord.verified !== VerifiedStatus.DEFAULT && - verifiedStatus === VerifiedStatus.DEFAULT - ) { + // Just update verified status if the key is the same or not present + if (isEqual || !publicKey) { await this.setVerified(uuid, verifiedStatus, publicKey); return false; } - if ( - verifiedStatus === VerifiedStatus.VERIFIED && - (!identityRecord || - (identityRecord && !isEqual) || - (identityRecord && identityRecord.verified !== VerifiedStatus.VERIFIED)) - ) { - await this.saveIdentityWithAttributes(uuid, { - publicKey, - verified: verifiedStatus, - firstUse: false, - timestamp: Date.now(), - nonblockingApproval: true, - }); + await this.saveIdentityWithAttributes(uuid, { + publicKey, + verified: verifiedStatus, + firstUse: false, + timestamp: Date.now(), + nonblockingApproval: verifiedStatus === VerifiedStatus.VERIFIED, + }); - if (identityRecord && !isEqual) { - try { - this.trigger('keychange', uuid); - } catch (error) { - log.error( - 'processVerifiedMessage error triggering keychange:', - error && error.stack ? error.stack : error - ); - } - - await this.archiveAllSessions(uuid); - - // true signifies that we overwrote a previous key with a new one - return true; + if (identityRecord) { + try { + this.trigger('keychange', uuid); + } catch (error) { + log.error( + 'processVerifiedMessage error triggering keychange:', + Errors.toLogFormat(error) + ); } + + // true signifies that we overwrote a previous key with a new one + return true; } - // We get here if we got a new key and the status is DEFAULT. If the - // message is out of date, we don't want to lose whatever more-secure - // state we had before. return false; } diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts index b136f9a99e8a..43dd925a3fbc 100644 --- a/ts/models/conversations.ts +++ b/ts/models/conversations.ts @@ -2555,7 +2555,7 @@ export class ConversationModel extends window.Backbone ); } - setVerifiedDefault(options?: VerificationOptions): Promise { + setVerifiedDefault(options?: VerificationOptions): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const { DEFAULT } = this.verifiedEnum!; return this.queueJob('setVerifiedDefault', () => @@ -2563,7 +2563,7 @@ export class ConversationModel extends window.Backbone ); } - setVerified(options?: VerificationOptions): Promise { + setVerified(options?: VerificationOptions): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const { VERIFIED } = this.verifiedEnum!; return this.queueJob('setVerified', () => @@ -2571,7 +2571,7 @@ export class ConversationModel extends window.Backbone ); } - setUnverified(options: VerificationOptions): Promise { + setUnverified(options: VerificationOptions): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const { UNVERIFIED } = this.verifiedEnum!; return this.queueJob('setUnverified', () => @@ -2582,7 +2582,7 @@ export class ConversationModel extends window.Backbone private async _setVerified( verified: number, providedOptions?: VerificationOptions - ): Promise { + ): Promise { const options = providedOptions || {}; window._.defaults(options, { viaStorageServiceSync: false, @@ -2631,12 +2631,13 @@ export class ConversationModel extends window.Backbone window.Signal.Data.updateConversation(this.attributes); } - if ( - !options.viaStorageServiceSync && - !keyChange && - beginningVerified !== verified - ) { - this.captureChange('verified'); + if (!options.viaStorageServiceSync) { + if (keyChange) { + this.captureChange('keyChange'); + } + if (beginningVerified !== verified) { + this.captureChange(`verified from=${beginningVerified} to=${verified}`); + } } const didSomethingChange = keyChange || beginningVerified !== verified; @@ -2663,6 +2664,8 @@ export class ConversationModel extends window.Backbone if (!options.viaSyncMessage && uuid) { await this.sendVerifySyncMessage(this.get('e164'), uuid, verified); } + + return keyChange; } async sendVerifySyncMessage( diff --git a/ts/services/storageRecordOps.ts b/ts/services/storageRecordOps.ts index 8c3d057c879f..3fbdefa3a5f3 100644 --- a/ts/services/storageRecordOps.ts +++ b/ts/services/storageRecordOps.ts @@ -810,6 +810,7 @@ export async function mergeContactRecord( ); } + let details = new Array(); const remoteName = dropNull(contactRecord.givenName); const remoteFamilyName = dropNull(contactRecord.familyName); const localName = conversation.get('profileName'); @@ -821,38 +822,50 @@ export async function mergeContactRecord( // Local name doesn't match remote name, fetch profile if (localName) { conversation.getProfiles(); + details.push('refreshing profile'); } else { conversation.set({ profileName: remoteName, profileFamilyName: remoteFamilyName, }); + details.push('updated profile name'); } } - const verified = await conversation.safeGetVerified(); - const storageServiceVerified = contactRecord.identityState || 0; - if (verified !== storageServiceVerified) { + // Update verified status unconditionally to make sure we will take the + // latest identity key from the manifest. + { + const verified = await conversation.safeGetVerified(); + const storageServiceVerified = contactRecord.identityState || 0; const verifiedOptions = { key: contactRecord.identityKey, viaStorageServiceSync: true, }; const STATE_ENUM = Proto.ContactRecord.IdentityState; + if (verified !== storageServiceVerified) { + details.push(`updating verified state to=${verified}`); + } + + let keyChange: boolean; switch (storageServiceVerified) { case STATE_ENUM.VERIFIED: - await conversation.setVerified(verifiedOptions); + keyChange = await conversation.setVerified(verifiedOptions); break; case STATE_ENUM.UNVERIFIED: - await conversation.setUnverified(verifiedOptions); + keyChange = await conversation.setUnverified(verifiedOptions); break; default: - await conversation.setVerifiedDefault(verifiedOptions); + keyChange = await conversation.setVerifiedDefault(verifiedOptions); + } + + if (keyChange) { + details.push('key changed'); } } applyMessageRequestState(contactRecord, conversation); - let details = new Array(); addUnknownFields(contactRecord, conversation, details); const oldStorageID = conversation.get('storageID'); diff --git a/ts/test-electron/SignalProtocolStore_test.ts b/ts/test-electron/SignalProtocolStore_test.ts index 23242adb71ef..4cb3e362dbe6 100644 --- a/ts/test-electron/SignalProtocolStore_test.ts +++ b/ts/test-electron/SignalProtocolStore_test.ts @@ -672,7 +672,7 @@ describe('SignalProtocolStore', () => { }); }); }); - describe('processContactSyncVerificationState', () => { + describe('processVerifiedMessage', () => { const newIdentity = getPublicKey(); let keychangeTriggered: number; @@ -693,8 +693,8 @@ describe('SignalProtocolStore', () => { await store.hydrateCaches(); }); - it('does nothing', async () => { - await store.processContactSyncVerificationState( + it('sets the identity key', async () => { + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.DEFAULT, newIdentity @@ -703,15 +703,10 @@ describe('SignalProtocolStore', () => { const identity = await window.Signal.Data.getIdentityKeyById( theirUuid.toString() ); - - if (identity) { - // fetchRecord resolved so there is a record. - // Bad. - throw new Error( - 'processContactSyncVerificationState should not save new records' - ); - } - + assert.isTrue( + identity?.publicKey && + constantTimeEqual(identity.publicKey, newIdentity) + ); assert.strictEqual(keychangeTriggered, 0); }); }); @@ -729,8 +724,8 @@ describe('SignalProtocolStore', () => { await store.hydrateCaches(); }); - it('does not save the new identity (because this is a less secure state)', async () => { - await store.processContactSyncVerificationState( + it('updates the identity', async () => { + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.DEFAULT, newIdentity @@ -743,14 +738,9 @@ describe('SignalProtocolStore', () => { throw new Error('Missing identity!'); } - assert.strictEqual( - identity.verified, - store.VerifiedStatus.VERIFIED - ); - assert.isTrue( - constantTimeEqual(identity.publicKey, testKey.pubKey) - ); - assert.strictEqual(keychangeTriggered, 0); + assert.strictEqual(identity.verified, store.VerifiedStatus.DEFAULT); + assert.isTrue(constantTimeEqual(identity.publicKey, newIdentity)); + assert.strictEqual(keychangeTriggered, 1); }); }); describe('when the existing key is the same but VERIFIED', () => { @@ -767,7 +757,7 @@ describe('SignalProtocolStore', () => { }); it('updates the verified status', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.DEFAULT, testKey.pubKey @@ -801,7 +791,7 @@ describe('SignalProtocolStore', () => { }); it('does not hang', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.DEFAULT, testKey.pubKey @@ -819,8 +809,8 @@ describe('SignalProtocolStore', () => { await store.hydrateCaches(); }); - it('saves the new identity and marks it verified', async () => { - await store.processContactSyncVerificationState( + it('saves the new identity and marks it UNVERIFIED', async () => { + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.UNVERIFIED, newIdentity @@ -856,7 +846,7 @@ describe('SignalProtocolStore', () => { }); it('saves the new identity and marks it UNVERIFIED', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.UNVERIFIED, newIdentity @@ -891,7 +881,7 @@ describe('SignalProtocolStore', () => { }); it('updates the verified status', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.UNVERIFIED, testKey.pubKey @@ -927,7 +917,7 @@ describe('SignalProtocolStore', () => { }); it('does not hang', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.UNVERIFIED, testKey.pubKey @@ -946,7 +936,7 @@ describe('SignalProtocolStore', () => { }); it('saves the new identity and marks it verified', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.VERIFIED, newIdentity @@ -978,7 +968,7 @@ describe('SignalProtocolStore', () => { }); it('saves the new identity and marks it VERIFIED', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.VERIFIED, newIdentity @@ -1013,7 +1003,7 @@ describe('SignalProtocolStore', () => { }); it('saves the identity and marks it verified', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.VERIFIED, testKey.pubKey @@ -1049,7 +1039,7 @@ describe('SignalProtocolStore', () => { }); it('does not hang', async () => { - await store.processContactSyncVerificationState( + await store.processVerifiedMessage( theirUuid, store.VerifiedStatus.VERIFIED, testKey.pubKey