From bf04c9114ec1a7176fe98c09d939974846895d5a Mon Sep 17 00:00:00 2001 From: Ken Powers Date: Fri, 12 Jun 2020 18:36:32 -0400 Subject: [PATCH] Harden UUID-handling code paths --- js/background.js | 133 ++++++++++++----------- js/conversation_controller.js | 62 ++++++++++- js/models/conversations.js | 38 ++++--- js/models/messages.js | 20 ++-- js/modules/refresh_sender_certificate.js | 11 +- js/signal_protocol_store.js | 14 +-- ts/textsecure/AccountManager.ts | 17 +-- ts/textsecure/MessageReceiver.ts | 5 + ts/util/lint/exceptions.json | 12 +- ts/window.d.ts | 5 + 10 files changed, 193 insertions(+), 124 deletions(-) diff --git a/js/background.js b/js/background.js index f300f9d4213..2a1b421c3b1 100644 --- a/js/background.js +++ b/js/background.js @@ -244,9 +244,7 @@ regionCode: window.storage.get('regionCode'), ourNumber, ourUuid, - ourConversationId: ConversationController.getConversationId( - ourNumber || ourUuid - ), + ourConversationId: ConversationController.getOurConversationId(), }; Whisper.events.trigger('userChanged', user); @@ -616,9 +614,7 @@ ); const ourNumber = textsecure.storage.user.getNumber(); const ourUuid = textsecure.storage.user.getUuid(); - const ourConversationId = ConversationController.getConversationId( - ourNumber || ourUuid - ); + const ourConversationId = ConversationController.getOurConversationId(); const initialState = { conversations: { conversationLookup: Signal.Util.makeLookup(conversations, 'id'), @@ -1803,10 +1799,9 @@ Whisper.events.trigger('contactsync'); }); - const ourUuid = textsecure.storage.user.getUuid(); - const ourNumber = textsecure.storage.user.getNumber(); + const ourId = ConversationController.getOurConversationId(); const { wrap, sendOptions } = ConversationController.prepareForSend( - ourNumber || ourUuid, + ourId, { syncMessage: true, } @@ -1953,18 +1948,16 @@ return; } - const conversation = ConversationController.get( - groupId || sender || senderUuid - ); - const ourUuid = textsecure.storage.user.getUuid(); - const ourNumber = textsecure.storage.user.getNumber(); + const senderId = ConversationController.ensureContactIds({ + e164: sender, + uuid: senderUuid, + }); + const conversation = ConversationController.get(groupId || senderId); + const ourId = ConversationController.getOurConversationId(); if (conversation) { // We drop typing notifications in groups we're not a part of - if ( - !conversation.isPrivate() && - !conversation.hasMember(ourNumber || ourUuid) - ) { + if (!conversation.isPrivate() && !conversation.hasMember(ourId)) { window.log.warn( `Received typing indicator for group ${conversation.idForLogging()}, which we're not a part of. Dropping.` ); @@ -1973,8 +1966,10 @@ conversation.notifyTyping({ isTyping: started, + isMe: ourId === senderId, sender, senderUuid, + senderId, senderDevice, }); } @@ -2046,10 +2041,11 @@ } try { - const conversation = await ConversationController.getOrCreateAndWait( - details.number || details.uuid, - 'private' - ); + const detailsId = ConversationController.ensureContactIds({ + e164: details.number, + uuid: details.uuid, + }); + const conversation = ConversationController.get(detailsId); let activeAt = conversation.get('active_at'); // The idea is to make any new contact show up in the left pane. If @@ -2116,15 +2112,16 @@ const { expireTimer } = details; const isValidExpireTimer = typeof expireTimer === 'number'; if (isValidExpireTimer) { - const sourceE164 = textsecure.storage.user.getNumber(); - const sourceUuid = textsecure.storage.user.getUuid(); + const ourId = ConversationController.getOurConversationId(); const receivedAt = Date.now(); await conversation.updateExpirationTimer( expireTimer, - sourceE164 || sourceUuid, + ourId, receivedAt, - { fromSync: true } + { + fromSync: true, + } ); } @@ -2265,7 +2262,13 @@ const getDescriptorForReceived = ({ message, source, sourceUuid }) => message.group ? getGroupDescriptor(message.group) - : { type: Message.PRIVATE, id: source || sourceUuid }; + : { + type: Message.PRIVATE, + id: ConversationController.ensureContactIds({ + e164: source, + uuid: sourceUuid, + }), + }; // Received: async function handleMessageReceivedProfileUpdate({ @@ -2304,22 +2307,7 @@ }); } - const message = initIncomingMessage(data); - - const result = ConversationController.getOrCreate( - messageDescriptor.id, - messageDescriptor.type, - messageDescriptor.type === 'group' - ? { addedBy: message.getContact().get('id') } - : undefined - ); - - if (messageDescriptor.type === 'private') { - result.updateE164(data.source); - if (data.sourceUuid) { - result.updateUuid(data.sourceUuid); - } - } + const message = initIncomingMessage(data, messageDescriptor); if (data.message.reaction) { const { reaction } = data.message; @@ -2331,7 +2319,10 @@ targetAuthorUuid: reaction.targetAuthorUuid, targetTimestamp: reaction.targetTimestamp.toNumber(), timestamp: Date.now(), - fromId: data.source || data.sourceUuid, + fromId: ConversationController.ensureContactIds({ + e164: data.source, + uuid: data.sourceUuid, + }), }); // Note: We do not wait for completion here Whisper.Reactions.onReaction(reactionModel); @@ -2345,7 +2336,10 @@ const deleteModel = Whisper.Deletes.add({ targetSentTimestamp: del.targetSentTimestamp, serverTimestamp: data.serverTimestamp, - fromId: data.source || data.sourceUuid, + fromId: ConversationController.ensureContactIds({ + e164: data.source, + uuid: data.sourceUuid, + }), }); // Note: We do not wait for completion here Whisper.Deletes.onDelete(deleteModel); @@ -2410,13 +2404,9 @@ window.Signal.Data.updateConversation(conversation.attributes); // Then we update our own profileKey if it's different from what we have - const ourNumber = textsecure.storage.user.getNumber(); - const ourUuid = textsecure.storage.user.getUuid(); + const ourId = ConversationController.getOurConversationId(); + const me = ConversationController.get(ourId); const profileKey = data.message.profileKey.toString('base64'); - const me = await ConversationController.getOrCreate( - ourNumber || ourUuid, - 'private' - ); // Will do the save for us if needed await me.setProfileKey(profileKey); @@ -2477,9 +2467,6 @@ const message = createSentMessage(data); - const ourNumber = textsecure.storage.user.getNumber(); - const ourUuid = textsecure.storage.user.getUuid(); - if (data.message.reaction) { const { reaction } = data.message; const reactionModel = Whisper.Reactions.add({ @@ -2489,7 +2476,7 @@ targetAuthorUuid: reaction.targetAuthorUuid, targetTimestamp: reaction.targetTimestamp.toNumber(), timestamp: Date.now(), - fromId: ourNumber || ourUuid, + fromId: ConversationController.getOurConversationId(), fromSync: true, }); // Note: We do not wait for completion here @@ -2504,7 +2491,7 @@ const deleteModel = Whisper.Deletes.add({ targetSentTimestamp: del.targetSentTimestamp, serverTimestamp: del.serverTimestamp, - fromId: ourNumber || ourUuid, + fromId: ConversationController.getOurConversationId(), }); // Note: We do not wait for completion here Whisper.Deletes.onDelete(deleteModel); @@ -2525,10 +2512,22 @@ return Promise.resolve(); } - function initIncomingMessage(data) { - const targetId = data.source || data.sourceUuid; - const conversation = ConversationController.get(targetId); - const conversationId = conversation ? conversation.id : targetId; + function initIncomingMessage(data, descriptor) { + // Ensure that we have an accurate record for who this message is from + const fromContactId = ConversationController.ensureContactIds({ + e164: data.source, + uuid: data.sourceUuid, + }); + + // Determine if this message is in a group + const isGroup = descriptor.type === Message.GROUP; + + // Determine the conversationId this message belongs to + const conversationId = isGroup + ? ConversationController.ensureGroup(descriptor.id, { + addedBy: fromContactId, + }) + : fromContactId; return new Whisper.Message({ source: data.source, @@ -2640,7 +2639,13 @@ return Promise.resolve(); } const envelope = ev.proto; - const message = initIncomingMessage(envelope); + const message = initIncomingMessage(envelope, { + type: Message.PRIVATE, + id: ConversationController.ensureContactIds({ + e164: envelope.source, + uuid: envelope.sourceUuid, + }), + }); const conversationId = message.get('conversationId'); const conversation = ConversationController.getOrCreate( @@ -2832,10 +2837,8 @@ ev.viaContactSync ? 'via contact sync' : '' ); - const contact = await ConversationController.getOrCreateAndWait( - e164 || uuid, - 'private' - ); + const verifiedId = ConversationController.ensureContactIds({ e164, uuid }); + const contact = await ConversationController.get(verifiedId, 'private'); const options = { viaSyncMessage: true, viaContactSync: ev.viaContactSync, diff --git a/js/conversation_controller.js b/js/conversation_controller.js index 490ea3de5d1..ad447ffb98e 100644 --- a/js/conversation_controller.js +++ b/js/conversation_controller.js @@ -67,7 +67,7 @@ dangerouslyCreateAndAdd(attributes) { return conversations.add(attributes); }, - getOrCreate(identifier, type) { + getOrCreate(identifier, type, additionalInitialProps = {}) { if (typeof identifier !== 'string') { throw new TypeError("'id' must be a string"); } @@ -99,6 +99,7 @@ groupId: identifier, type, version: 2, + ...additionalInitialProps, }); } else if (window.isValidGuid(identifier)) { conversation = conversations.add({ @@ -108,6 +109,7 @@ groupId: null, type, version: 2, + ...additionalInitialProps, }); } else { conversation = conversations.add({ @@ -117,6 +119,7 @@ groupId: null, type, version: 2, + ...additionalInitialProps, }); } @@ -154,9 +157,9 @@ return conversation; }, - getOrCreateAndWait(id, type) { + getOrCreateAndWait(id, type, additionalInitialProps = {}) { return this._initialPromise.then(() => { - const conversation = this.getOrCreate(id, type); + const conversation = this.getOrCreate(id, type, additionalInitialProps); if (conversation) { return conversation.initialPromise.then(() => conversation); @@ -184,7 +187,58 @@ getOurConversationId() { const e164 = textsecure.storage.user.getNumber(); const uuid = textsecure.storage.user.getUuid(); - return this.getConversationId(e164 || uuid); + return this.ensureContactIds({ e164, uuid }); + }, + /** + * Given a UUID and/or an E164, resolves to a string representing the local + * database of the given contact. If a conversation is found it is updated + * to have the given UUID and E164. If a conversation is not found, this + * function creates a conversation with the given UUID and E164. If the + * conversation * is found in the local database it is updated. + * + * This function also additionally checks for mismatched e164/uuid pairs out + * of abundance of caution. + */ + ensureContactIds({ e164, uuid }) { + // Check for at least one parameter being provided. This is necessary + // because this path can be called on startup to resolve our own ID before + // our phone number or UUID are known. The existing behavior in these + // cases can handle a returned `undefined` id, so we do that. + if (!e164 && !uuid) { + return undefined; + } + + const lowerUuid = uuid ? uuid.toLowerCase() : undefined; + + const convoE164 = this.get(e164); + const convoUuid = this.get(lowerUuid); + + // Check for mismatched UUID and E164 + if ( + convoE164 && + convoUuid && + convoE164.get('id') !== convoUuid.get('id') + ) { + window.log.warn('Received a message with a mismatched UUID and E164.'); + } + + const convo = convoUuid || convoE164; + + const idOrIdentifier = convo ? convo.get('id') : e164 || lowerUuid; + + const finalConversation = this.getOrCreate(idOrIdentifier, 'private'); + finalConversation.updateE164(e164); + finalConversation.updateUuid(lowerUuid); + + return finalConversation.get('id'); + }, + /** + * Given a groupId and optional additional initialization properties, + * ensures the existence of a group conversation and returns a string + * representing the local database ID of the group conversation. + */ + ensureGroup(groupId, additionalInitProps = {}) { + return this.getOrCreate(groupId, 'group', additionalInitProps).get('id'); }, /** * Given certain metadata about a message (an identifier of who wrote the diff --git a/js/models/conversations.js b/js/models/conversations.js index aa46fd35eb5..2f321f2bdf8 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -435,7 +435,7 @@ const typingValues = _.values(this.contactTypingTimers || {}); const typingMostRecent = _.first(_.sortBy(typingValues, 'timestamp')); const typingContact = typingMostRecent - ? ConversationController.getOrCreate(typingMostRecent.sender, 'private') + ? ConversationController.get(typingMostRecent.senderId) : null; const timestamp = this.get('timestamp'); @@ -491,7 +491,7 @@ updateE164(e164) { const oldValue = this.get('e164'); - if (e164 !== oldValue) { + if (e164 && e164 !== oldValue) { this.set('e164', e164); window.Signal.Data.updateConversation(this.attributes); this.trigger('idUpdated', this, 'e164', oldValue); @@ -499,15 +499,15 @@ }, updateUuid(uuid) { const oldValue = this.get('uuid'); - if (uuid !== oldValue) { - this.set('uuid', uuid); + if (uuid && uuid !== oldValue) { + this.set('uuid', uuid.toLowerCase()); window.Signal.Data.updateConversation(this.attributes); this.trigger('idUpdated', this, 'uuid', oldValue); } }, updateGroupId(groupId) { const oldValue = this.get('groupId'); - if (groupId !== oldValue) { + if (groupId && groupId !== oldValue) { this.set('groupId', groupId); window.Signal.Data.updateConversation(this.attributes); this.trigger('idUpdated', this, 'groupId', oldValue); @@ -1716,10 +1716,11 @@ // START: this code has an Expiration date of ~2018/11/21 // We don't want to enable unidentified delivery for send unless it is // also enabled for our own account. - const me = ConversationController.getOrCreate( - this.ourNumber || this.ourUuid, - 'private' - ); + const myId = ConversationController.ensureContactIds({ + e164: this.ourNumber, + uuid: this.ourUuid, + }); + const me = ConversationController.get(myId); if ( !disableMeCheck && me.get('sealedSender') === SEALED_SENDER.DISABLED @@ -2262,7 +2263,7 @@ ); } - const c = await ConversationController.getOrCreateAndWait(id, 'private'); + const c = ConversationController.getOrCreate(id, 'private'); const { generateProfileKeyCredentialRequest, getClientZkProfileOperations, @@ -2613,8 +2614,8 @@ return Promise.resolve(); } const members = this.get('members') || []; - const promises = members.map(number => - ConversationController.getOrCreateAndWait(number, 'private') + const promises = members.map(identifier => + ConversationController.getOrCreateAndWait(identifier, 'private') ); return Promise.all(promises).then(contacts => { @@ -2807,10 +2808,17 @@ }, notifyTyping(options = {}) { - const { isTyping, sender, senderUuid, senderDevice } = options; + const { + isTyping, + sender, + senderUuid, + senderId, + isMe, + senderDevice, + } = options; // We don't do anything with typing messages from our other devices - if (sender === this.ourNumber || senderUuid === this.ourUuid) { + if (isMe) { return; } @@ -2829,6 +2837,8 @@ ] || { timestamp: Date.now(), sender, + senderId, + senderUuid, senderDevice, }; diff --git a/js/models/messages.js b/js/models/messages.js index fa3044643e9..9fe04af502e 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -1228,10 +1228,11 @@ return null; } - return ConversationController.getOrCreate( - source || sourceUuid, - 'private' - ); + const contactId = ConversationController.ensureContactIds({ + e164: source, + uuid: sourceUuid, + }); + return ConversationController.get(contactId, 'private'); }, isOutgoing() { return this.get('type') === 'outgoing'; @@ -2562,12 +2563,13 @@ } else if (conversation.isPrivate()) { conversation.setProfileKey(profileKey); } else { - ConversationController.getOrCreateAndWait( - source || sourceUuid, - 'private' - ).then(sender => { - sender.setProfileKey(profileKey); + const localId = ConversationController.ensureContactIds({ + e164: source, + uuid: sourceUuid, }); + ConversationController.get(localId, 'private').setProfileKey( + profileKey + ); } } diff --git a/js/modules/refresh_sender_certificate.js b/js/modules/refresh_sender_certificate.js index abf714c5eb0..8f1433278bb 100644 --- a/js/modules/refresh_sender_certificate.js +++ b/js/modules/refresh_sender_certificate.js @@ -17,12 +17,11 @@ function refreshOurProfile() { window.log.info('refreshOurProfile'); const ourNumber = textsecure.storage.user.getNumber(); const ourUuid = textsecure.storage.user.getUuid(); - const conversation = ConversationController.getOrCreate( - // This is explicitly ourNumber first in order to avoid creating new - // conversations when an old one exists - ourNumber || ourUuid, - 'private' - ); + const ourId = ConversationController.ensureContactIds({ + e164: ourNumber, + uuid: ourUuid, + }); + const conversation = ConversationController.get(ourId, 'private'); conversation.updateUuid(ourUuid); conversation.updateE164(ourNumber); conversation.getProfiles(); diff --git a/js/signal_protocol_store.js b/js/signal_protocol_store.js index f2a84a3960f..d080a375bdd 100644 --- a/js/signal_protocol_store.js +++ b/js/signal_protocol_store.js @@ -152,10 +152,7 @@ encodedAddress ); try { - const conv = await ConversationController.getOrCreateAndWait( - identifier, - 'private' - ); + const conv = ConversationController.getOrCreate(identifier, 'private'); return `${conv.get('id')}.${deviceId}`; } catch (e) { window.log.error( @@ -585,7 +582,9 @@ const identifier = textsecure.utils.unencodeNumber(encodedAddress)[0]; const identityRecord = this.getIdentityRecord(identifier); - const id = ConversationController.getConversationId(identifier); + const id = ConversationController.getOrCreate(identifier, 'private').get( + 'id' + ); if (!identityRecord || !identityRecord.publicKey) { // Lookup failed, or the current key was removed, so save this one. @@ -661,10 +660,7 @@ const identifier = textsecure.utils.unencodeNumber(encodedAddress)[0]; const identityRecord = this.getIdentityRecord(identifier); - const conv = await ConversationController.getOrCreateAndWait( - identifier, - 'private' - ); + const conv = ConversationController.getOrCreate(identifier, 'private'); const id = conv.get('id'); const updates = { diff --git a/ts/textsecure/AccountManager.ts b/ts/textsecure/AccountManager.ts index 86e99379a74..a62f4eeb431 100644 --- a/ts/textsecure/AccountManager.ts +++ b/ts/textsecure/AccountManager.ts @@ -693,19 +693,14 @@ export default class AccountManager extends EventTarget { async registrationDone({ uuid, number }: { uuid?: string; number?: string }) { window.log.info('registration done'); - const identifier = number || uuid; - if (!identifier) { - throw new Error('registrationDone: no identifier!'); + const conversationId = window.ConversationController.ensureContactIds({ + e164: number, + uuid, + }); + if (!conversationId) { + throw new Error('registrationDone: no conversationId!'); } - // Ensure that we always have a conversation for ourself - const conversation = await window.ConversationController.getOrCreateAndWait( - identifier, - 'private' - ); - conversation.updateE164(number); - conversation.updateUuid(uuid); - window.log.info('dispatching registration event'); this.dispatchEvent(new Event('registration')); diff --git a/ts/textsecure/MessageReceiver.ts b/ts/textsecure/MessageReceiver.ts index 68a6ce784f7..88d2cb97db4 100644 --- a/ts/textsecure/MessageReceiver.ts +++ b/ts/textsecure/MessageReceiver.ts @@ -1535,6 +1535,11 @@ class MessageReceiverInner extends EventTarget { while (contactDetails !== undefined) { const contactEvent = new Event('contact'); contactEvent.contactDetails = contactDetails; + window.normalizeUuids( + contactEvent, + ['contactDetails.verified.destinationUuid'], + 'message_receiver::handleContacts::handleAttachment' + ); results.push(this.dispatchAndWait(contactEvent)); contactDetails = contactBuffer.next(); diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index 4572f36724e..7ed82529741 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -164,7 +164,7 @@ "rule": "jQuery-load(", "path": "js/conversation_controller.js", "line": " async load() {", - "lineNumber": 252, + "lineNumber": 306, "reasonCategory": "falseMatch", "updated": "2020-06-19T18:29:40.067Z" }, @@ -172,7 +172,7 @@ "rule": "jQuery-load(", "path": "js/conversation_controller.js", "line": " this._initialPromise = load();", - "lineNumber": 294, + "lineNumber": 348, "reasonCategory": "falseMatch", "updated": "2020-06-19T18:29:40.067Z" }, @@ -225,7 +225,7 @@ "line": " await wrap(", "lineNumber": 641, "reasonCategory": "falseMatch", - "updated": "2020-05-27T21:15:43.044Z" + "updated": "2020-06-09T20:26:46.515Z" }, { "rule": "jQuery-append(", @@ -325,9 +325,9 @@ "rule": "jQuery-load(", "path": "js/signal_protocol_store.js", "line": " await ConversationController.load();", - "lineNumber": 985, + "lineNumber": 981, "reasonCategory": "falseMatch", - "updated": "2020-04-27T20:02:20.424Z" + "updated": "2020-06-12T14:20:09.936Z" }, { "rule": "DOM-innerHTML", @@ -11878,4 +11878,4 @@ "reasonCategory": "falseMatch", "updated": "2020-04-05T23:45:16.746Z" } -] +] \ No newline at end of file diff --git a/ts/window.d.ts b/ts/window.d.ts index 5776fef3f22..bddf0b99887 100644 --- a/ts/window.d.ts +++ b/ts/window.d.ts @@ -96,7 +96,12 @@ export type ConversationControllerType = { identifier: string, type: 'private' | 'group' ) => Promise; + getOrCreate: ( + identifier: string, + type: 'private' | 'group' + ) => ConversationType; getConversationId: (identifier: string) => string | null; + ensureContactIds: (o: { e164?: string; uuid?: string }) => string; prepareForSend: ( id: string, options: Object