mirrors/signal-desktop
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 2 Wiki Activity Actions

Better merging for changed keys in storage service

Browse source
This commit is contained in:
Fedor Indutny 2022-03-21 15:06:34 -07:00 • committed by GitHub
parent e08d9baaba
commit f536421390
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 78 additions and 169 deletions
Download patch file Download diff file Expand all files Collapse all files

111
ts/SignalProtocolStore.ts
View file

@ -1763,84 +1763,8 @@ export class SignalProtocolStore extends EventsMixin {
return VerifiedStatus.DEFAULT; return VerifiedStatus.DEFAULT;
} }
// Resolves to true if a new identity key was saved // See https://github.com/signalapp/Signal-iOS-Private/blob/e32c2dff0d03f67467b4df621d84b11412d50cdb/SignalServiceKit/src/Messages/OWSIdentityManager.m#L317
processContactSyncVerificationState( // for reference.
uuid: UUID,
verifiedStatus: number,
publicKey: Uint8Array
): Promise<boolean> {
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<boolean> {
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
async processVerifiedMessage( async processVerifiedMessage(
uuid: UUID, uuid: UUID,
verifiedStatus: number, verifiedStatus: number,
@ -1864,55 +1788,34 @@ export class SignalProtocolStore extends EventsMixin {
isEqual = constantTimeEqual(publicKey, identityRecord.publicKey); isEqual = constantTimeEqual(publicKey, identityRecord.publicKey);
} }
if (!identityRecord && verifiedStatus === VerifiedStatus.DEFAULT) { // Just update verified status if the key is the same or not present
log.info('processVerifiedMessage: No existing record for default status'); if (isEqual || !publicKey) {
return false;
}
if (
identityRecord &&
isEqual &&
identityRecord.verified !== VerifiedStatus.DEFAULT &&
verifiedStatus === VerifiedStatus.DEFAULT
) {
await this.setVerified(uuid, verifiedStatus, publicKey); await this.setVerified(uuid, verifiedStatus, publicKey);
return false; return false;
} }
if (
verifiedStatus === VerifiedStatus.VERIFIED &&
(!identityRecord ||
(identityRecord && !isEqual) ||
(identityRecord && identityRecord.verified !== VerifiedStatus.VERIFIED))
) {
await this.saveIdentityWithAttributes(uuid, { await this.saveIdentityWithAttributes(uuid, {
publicKey, publicKey,
verified: verifiedStatus, verified: verifiedStatus,
firstUse: false, firstUse: false,
timestamp: Date.now(), timestamp: Date.now(),
nonblockingApproval: true, nonblockingApproval: verifiedStatus === VerifiedStatus.VERIFIED,
}); });
if (identityRecord && !isEqual) { if (identityRecord) {
try { try {
this.trigger('keychange', uuid); this.trigger('keychange', uuid);
} catch (error) { } catch (error) {
log.error( log.error(
'processVerifiedMessage error triggering keychange:', 'processVerifiedMessage error triggering keychange:',
error && error.stack ? error.stack : error Errors.toLogFormat(error)
); );
} }
await this.archiveAllSessions(uuid);
// true signifies that we overwrote a previous key with a new one // true signifies that we overwrote a previous key with a new one
return true; 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; return false;
} }

23
ts/models/conversations.ts
View file

@ -2555,7 +2555,7 @@ export class ConversationModel extends window.Backbone
); );
} }
setVerifiedDefault(options?: VerificationOptions): Promise<unknown> { setVerifiedDefault(options?: VerificationOptions): Promise<boolean> {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { DEFAULT } = this.verifiedEnum!; const { DEFAULT } = this.verifiedEnum!;
return this.queueJob('setVerifiedDefault', () => return this.queueJob('setVerifiedDefault', () =>
@ -2563,7 +2563,7 @@ export class ConversationModel extends window.Backbone
); );
} }
setVerified(options?: VerificationOptions): Promise<unknown> { setVerified(options?: VerificationOptions): Promise<boolean> {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { VERIFIED } = this.verifiedEnum!; const { VERIFIED } = this.verifiedEnum!;
return this.queueJob('setVerified', () => return this.queueJob('setVerified', () =>
@ -2571,7 +2571,7 @@ export class ConversationModel extends window.Backbone
); );
} }
setUnverified(options: VerificationOptions): Promise<unknown> { setUnverified(options: VerificationOptions): Promise<boolean> {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { UNVERIFIED } = this.verifiedEnum!; const { UNVERIFIED } = this.verifiedEnum!;
return this.queueJob('setUnverified', () => return this.queueJob('setUnverified', () =>
@ -2582,7 +2582,7 @@ export class ConversationModel extends window.Backbone
private async _setVerified( private async _setVerified(
verified: number, verified: number,
providedOptions?: VerificationOptions providedOptions?: VerificationOptions
): Promise<void> { ): Promise<boolean> {
const options = providedOptions || {}; const options = providedOptions || {};
window._.defaults(options, { window._.defaults(options, {
viaStorageServiceSync: false, viaStorageServiceSync: false,
@ -2631,12 +2631,13 @@ export class ConversationModel extends window.Backbone
window.Signal.Data.updateConversation(this.attributes); window.Signal.Data.updateConversation(this.attributes);
} }
if ( if (!options.viaStorageServiceSync) {
!options.viaStorageServiceSync && if (keyChange) {
!keyChange && this.captureChange('keyChange');
beginningVerified !== verified }
) { if (beginningVerified !== verified) {
this.captureChange('verified'); this.captureChange(`verified from=${beginningVerified} to=${verified}`);
}
} }
const didSomethingChange = keyChange || beginningVerified !== verified; const didSomethingChange = keyChange || beginningVerified !== verified;
@ -2663,6 +2664,8 @@ export class ConversationModel extends window.Backbone
if (!options.viaSyncMessage && uuid) { if (!options.viaSyncMessage && uuid) {
await this.sendVerifySyncMessage(this.get('e164'), uuid, verified); await this.sendVerifySyncMessage(this.get('e164'), uuid, verified);
} }
return keyChange;
} }
async sendVerifySyncMessage( async sendVerifySyncMessage(

23
ts/services/storageRecordOps.ts
View file

@ -810,6 +810,7 @@ export async function mergeContactRecord(
); );
} }
let details = new Array<string>();
const remoteName = dropNull(contactRecord.givenName); const remoteName = dropNull(contactRecord.givenName);
const remoteFamilyName = dropNull(contactRecord.familyName); const remoteFamilyName = dropNull(contactRecord.familyName);
const localName = conversation.get('profileName'); const localName = conversation.get('profileName');
@ -821,38 +822,50 @@ export async function mergeContactRecord(
// Local name doesn't match remote name, fetch profile // Local name doesn't match remote name, fetch profile
if (localName) { if (localName) {
conversation.getProfiles(); conversation.getProfiles();
details.push('refreshing profile');
} else { } else {
conversation.set({ conversation.set({
profileName: remoteName, profileName: remoteName,
profileFamilyName: remoteFamilyName, profileFamilyName: remoteFamilyName,
}); });
details.push('updated profile name');
} }
} }
// Update verified status unconditionally to make sure we will take the
// latest identity key from the manifest.
{
const verified = await conversation.safeGetVerified(); const verified = await conversation.safeGetVerified();
const storageServiceVerified = contactRecord.identityState || 0; const storageServiceVerified = contactRecord.identityState || 0;
if (verified !== storageServiceVerified) {
const verifiedOptions = { const verifiedOptions = {
key: contactRecord.identityKey, key: contactRecord.identityKey,
viaStorageServiceSync: true, viaStorageServiceSync: true,
}; };
const STATE_ENUM = Proto.ContactRecord.IdentityState; const STATE_ENUM = Proto.ContactRecord.IdentityState;
if (verified !== storageServiceVerified) {
details.push(`updating verified state to=${verified}`);
}
let keyChange: boolean;
switch (storageServiceVerified) { switch (storageServiceVerified) {
case STATE_ENUM.VERIFIED: case STATE_ENUM.VERIFIED:
await conversation.setVerified(verifiedOptions); keyChange = await conversation.setVerified(verifiedOptions);
break; break;
case STATE_ENUM.UNVERIFIED: case STATE_ENUM.UNVERIFIED:
await conversation.setUnverified(verifiedOptions); keyChange = await conversation.setUnverified(verifiedOptions);
break; break;
default: default:
await conversation.setVerifiedDefault(verifiedOptions); keyChange = await conversation.setVerifiedDefault(verifiedOptions);
}
if (keyChange) {
details.push('key changed');
} }
} }
applyMessageRequestState(contactRecord, conversation); applyMessageRequestState(contactRecord, conversation);
let details = new Array<string>();
addUnknownFields(contactRecord, conversation, details); addUnknownFields(contactRecord, conversation, details);
const oldStorageID = conversation.get('storageID'); const oldStorageID = conversation.get('storageID');

54
ts/test-electron/SignalProtocolStore_test.ts
View file

@ -672,7 +672,7 @@ describe('SignalProtocolStore', () => {
}); });
}); });
}); });
describe('processContactSyncVerificationState', () => { describe('processVerifiedMessage', () => {
const newIdentity = getPublicKey(); const newIdentity = getPublicKey();
let keychangeTriggered: number; let keychangeTriggered: number;
@ -693,8 +693,8 @@ describe('SignalProtocolStore', () => {
await store.hydrateCaches(); await store.hydrateCaches();
}); });
it('does nothing', async () => { it('sets the identity key', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.DEFAULT, store.VerifiedStatus.DEFAULT,
newIdentity newIdentity
@ -703,15 +703,10 @@ describe('SignalProtocolStore', () => {
const identity = await window.Signal.Data.getIdentityKeyById( const identity = await window.Signal.Data.getIdentityKeyById(
theirUuid.toString() theirUuid.toString()
); );
assert.isTrue(
if (identity) { identity?.publicKey &&
// fetchRecord resolved so there is a record. constantTimeEqual(identity.publicKey, newIdentity)
// Bad.
throw new Error(
'processContactSyncVerificationState should not save new records'
); );
}
assert.strictEqual(keychangeTriggered, 0); assert.strictEqual(keychangeTriggered, 0);
}); });
}); });
@ -729,8 +724,8 @@ describe('SignalProtocolStore', () => {
await store.hydrateCaches(); await store.hydrateCaches();
}); });
it('does not save the new identity (because this is a less secure state)', async () => { it('updates the identity', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.DEFAULT, store.VerifiedStatus.DEFAULT,
newIdentity newIdentity
@ -743,14 +738,9 @@ describe('SignalProtocolStore', () => {
throw new Error('Missing identity!'); throw new Error('Missing identity!');
} }
assert.strictEqual( assert.strictEqual(identity.verified, store.VerifiedStatus.DEFAULT);
identity.verified, assert.isTrue(constantTimeEqual(identity.publicKey, newIdentity));
store.VerifiedStatus.VERIFIED assert.strictEqual(keychangeTriggered, 1);
);
assert.isTrue(
constantTimeEqual(identity.publicKey, testKey.pubKey)
);
assert.strictEqual(keychangeTriggered, 0);
}); });
}); });
describe('when the existing key is the same but VERIFIED', () => { describe('when the existing key is the same but VERIFIED', () => {
@ -767,7 +757,7 @@ describe('SignalProtocolStore', () => {
}); });
it('updates the verified status', async () => { it('updates the verified status', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.DEFAULT, store.VerifiedStatus.DEFAULT,
testKey.pubKey testKey.pubKey
@ -801,7 +791,7 @@ describe('SignalProtocolStore', () => {
}); });
it('does not hang', async () => { it('does not hang', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.DEFAULT, store.VerifiedStatus.DEFAULT,
testKey.pubKey testKey.pubKey
@ -819,8 +809,8 @@ describe('SignalProtocolStore', () => {
await store.hydrateCaches(); await store.hydrateCaches();
}); });
it('saves the new identity and marks it verified', async () => { it('saves the new identity and marks it UNVERIFIED', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.UNVERIFIED, store.VerifiedStatus.UNVERIFIED,
newIdentity newIdentity
@ -856,7 +846,7 @@ describe('SignalProtocolStore', () => {
}); });
it('saves the new identity and marks it UNVERIFIED', async () => { it('saves the new identity and marks it UNVERIFIED', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.UNVERIFIED, store.VerifiedStatus.UNVERIFIED,
newIdentity newIdentity
@ -891,7 +881,7 @@ describe('SignalProtocolStore', () => {
}); });
it('updates the verified status', async () => { it('updates the verified status', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.UNVERIFIED, store.VerifiedStatus.UNVERIFIED,
testKey.pubKey testKey.pubKey
@ -927,7 +917,7 @@ describe('SignalProtocolStore', () => {
}); });
it('does not hang', async () => { it('does not hang', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.UNVERIFIED, store.VerifiedStatus.UNVERIFIED,
testKey.pubKey testKey.pubKey
@ -946,7 +936,7 @@ describe('SignalProtocolStore', () => {
}); });
it('saves the new identity and marks it verified', async () => { it('saves the new identity and marks it verified', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.VERIFIED, store.VerifiedStatus.VERIFIED,
newIdentity newIdentity
@ -978,7 +968,7 @@ describe('SignalProtocolStore', () => {
}); });
it('saves the new identity and marks it VERIFIED', async () => { it('saves the new identity and marks it VERIFIED', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.VERIFIED, store.VerifiedStatus.VERIFIED,
newIdentity newIdentity
@ -1013,7 +1003,7 @@ describe('SignalProtocolStore', () => {
}); });
it('saves the identity and marks it verified', async () => { it('saves the identity and marks it verified', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.VERIFIED, store.VerifiedStatus.VERIFIED,
testKey.pubKey testKey.pubKey
@ -1049,7 +1039,7 @@ describe('SignalProtocolStore', () => {
}); });
it('does not hang', async () => { it('does not hang', async () => {
await store.processContactSyncVerificationState( await store.processVerifiedMessage(
theirUuid, theirUuid,
store.VerifiedStatus.VERIFIED, store.VerifiedStatus.VERIFIED,
testKey.pubKey testKey.pubKey