From 5e253f1c266dccb3df631d28c4c29481d3da0579 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 5 Oct 2018 15:56:21 -0400 Subject: [PATCH 1/7] Always include "local user" in contacts sync messages. --- .../DeviceSyncing/OWSSyncContactsMessage.m | 41 ++++++++++++++++--- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m index f660ac0a1..4c398c2b3 100644 --- a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m +++ b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m @@ -10,12 +10,15 @@ #import "ProfileManagerProtocol.h" #import "SSKEnvironment.h" #import "SignalAccount.h" +#import "TSAccountManager.h" #import "TSAttachment.h" #import "TSAttachmentStream.h" #import "TSContactThread.h" #import #import +@import Contacts; + NS_ASSUME_NONNULL_BEGIN @interface OWSSyncContactsMessage () @@ -49,6 +52,18 @@ NS_ASSUME_NONNULL_BEGIN return [super initWithCoder:coder]; } +#pragma mark - Dependencies + +- (id)contactsManager { + return SSKEnvironment.shared.contactsManager; +} + +- (TSAccountManager *)tsAccountManager { + return TSAccountManager.sharedInstance; +} + +#pragma mark - + - (nullable SSKProtoSyncMessageBuilder *)syncMessageBuilder { if (self.attachmentIds.count != 1) { @@ -79,7 +94,22 @@ NS_ASSUME_NONNULL_BEGIN - (nullable NSData *)buildPlainTextAttachmentDataWithTransaction:(YapDatabaseReadTransaction *)transaction { - id contactsManager = SSKEnvironment.shared.contactsManager; + NSMutableArray *signalAccounts = [self.signalAccounts mutableCopy]; + + NSString *_Nullable localNumber = self.tsAccountManager.localNumber; + OWSAssertDebug(localNumber); + if (localNumber) { + BOOL hasLocalNumber = NO; + for (SignalAccount *signalAccount in signalAccounts) { + hasLocalNumber |= [signalAccount.recipientId isEqualToString:localNumber]; + } + if (!hasLocalNumber) { + SignalAccount *signalAccount = [[SignalAccount alloc] initWithRecipientId:localNumber]; + // OWSContactsOutputStream requires all signalAccount to have a contact. + signalAccount.contact = [[Contact alloc] initWithSystemContact:[CNContact new]]; + [signalAccounts addObject:signalAccount]; + } + } // TODO use temp file stream to avoid loading everything into memory at once // First though, we need to re-engineer our attachment process to accept streams (encrypting with stream, @@ -87,14 +117,13 @@ NS_ASSUME_NONNULL_BEGIN NSOutputStream *dataOutputStream = [NSOutputStream outputStreamToMemory]; [dataOutputStream open]; OWSContactsOutputStream *contactsOutputStream = - [[OWSContactsOutputStream alloc] initWithOutputStream:dataOutputStream]; + [[OWSContactsOutputStream alloc] initWithOutputStream:dataOutputStream]; - for (SignalAccount *signalAccount in self.signalAccounts) { + for (SignalAccount *signalAccount in signalAccounts) { OWSRecipientIdentity *_Nullable recipientIdentity = [self.identityManager recipientIdentityForRecipientId:signalAccount.recipientId]; NSData *_Nullable profileKeyData = [self.profileManager profileKeyDataForRecipientId:signalAccount.recipientId]; - OWSDisappearingMessagesConfiguration *_Nullable disappearingMessagesConfiguration; NSString *conversationColorName; @@ -109,11 +138,11 @@ NS_ASSUME_NONNULL_BEGIN [contactsOutputStream writeSignalAccount:signalAccount recipientIdentity:recipientIdentity profileKeyData:profileKeyData - contactsManager:contactsManager + contactsManager:self.contactsManager conversationColorName:conversationColorName disappearingMessagesConfiguration:disappearingMessagesConfiguration]; } - + [dataOutputStream close]; if (contactsOutputStream.hasError) { From d9c8a218bcc20f44214d769816d12b8a2bdffc48 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 5 Oct 2018 15:56:50 -0400 Subject: [PATCH 2/7] Use local profile data for the local phone number. --- SignalMessaging/profiles/OWSProfileManager.m | 40 +++++++++++++------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index 3079528b4..64470bc6b 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -111,6 +111,12 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; object:nil]; } +#pragma mark - Dependencies + +- (TSAccountManager *)tsAccountManager { + return TSAccountManager.sharedInstance; +} + - (AFHTTPSessionManager *)avatarHTTPManager { return [OWSSignalService sharedInstance].CDNSessionManager; @@ -491,7 +497,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; { OWSAssertIsOnMainThread(); - NSString *_Nullable localNumber = [TSAccountManager sharedInstance].localNumber; + NSString *_Nullable localNumber = self.tsAccountManager.localNumber; if (!localNumber) { return; } @@ -676,7 +682,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; - (void)logUserProfiles { [self.dbConnection asyncReadWithBlock:^(YapDatabaseReadTransaction *transaction) { - OWSLogError(@"logUserProfiles: %zd", [transaction numberOfKeysInCollection:OWSUserProfile.collection]); + OWSLogError(@"logUserProfiles: %ld", (unsigned long) [transaction numberOfKeysInCollection:OWSUserProfile.collection]); [transaction enumerateKeysAndObjectsInCollection:OWSUserProfile.collection usingBlock:^(NSString *_Nonnull key, id _Nonnull object, BOOL *_Nonnull stop) { @@ -730,9 +736,11 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; - (nullable OWSAES256Key *)profileKeyForRecipientId:(NSString *)recipientId { OWSAssertDebug(recipientId.length > 0); - - OWSUserProfile *userProfile = - [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]; + + // For "local reads", use the local user profile. + OWSUserProfile *userProfile = ([self.tsAccountManager.localNumber isEqualToString:recipientId] + ? self.localUserProfile + : [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]); OWSAssertDebug(userProfile); return userProfile.profileKey; @@ -742,8 +750,10 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; { OWSAssertDebug(recipientId.length > 0); - OWSUserProfile *userProfile = - [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]; + // For "local reads", use the local user profile. + OWSUserProfile *userProfile = ([self.tsAccountManager.localNumber isEqualToString:recipientId] + ? self.localUserProfile + : [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]); return userProfile.profileName; } @@ -752,8 +762,10 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; { OWSAssertDebug(recipientId.length > 0); - OWSUserProfile *userProfile = - [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]; + // For "local reads", use the local user profile. + OWSUserProfile *userProfile = ([self.tsAccountManager.localNumber isEqualToString:recipientId] + ? self.localUserProfile + : [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]); if (userProfile.avatarFileName.length > 0) { return [self loadProfileAvatarWithFilename:userProfile.avatarFileName]; @@ -770,8 +782,10 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; { OWSAssertDebug(recipientId.length > 0); - OWSUserProfile *userProfile = - [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]; + // For "local reads", use the local user profile. + OWSUserProfile *userProfile = ([self.tsAccountManager.localNumber isEqualToString:recipientId] + ? self.localUserProfile + : [OWSUserProfile getOrBuildUserProfileForRecipientId:recipientId dbConnection:self.dbConnection]); if (userProfile.avatarFileName.length > 0) { return [self loadProfileDataWithFilename:userProfile.avatarFileName]; @@ -862,7 +876,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; // If we're updating the profile that corresponds to our local number, // update the local profile as well. - NSString *_Nullable localNumber = [TSAccountManager sharedInstance].localNumber; + NSString *_Nullable localNumber = self.tsAccountManager.localNumber; if (localNumber && [localNumber isEqualToString:userProfile.recipientId]) { OWSUserProfile *localUserProfile = self.localUserProfile; OWSAssertDebug(localUserProfile); @@ -919,7 +933,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; // If we're updating the profile that corresponds to our local number, // update the local profile as well. - NSString *_Nullable localNumber = [TSAccountManager sharedInstance].localNumber; + NSString *_Nullable localNumber = self.tsAccountManager.localNumber; if (localNumber && [localNumber isEqualToString:recipientId]) { OWSUserProfile *localUserProfile = self.localUserProfile; OWSAssertDebug(localUserProfile); From 7ef39bf258b4a1c053967ed48c41a5f184ff00a7 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 5 Oct 2018 16:17:32 -0400 Subject: [PATCH 3/7] Clean up proto utils. --- SignalServiceKit/src/Protocols/ProtoUtils.m | 31 +++++++++++---------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/SignalServiceKit/src/Protocols/ProtoUtils.m b/SignalServiceKit/src/Protocols/ProtoUtils.m index 19ef17a71..853e1ef1c 100644 --- a/SignalServiceKit/src/Protocols/ProtoUtils.m +++ b/SignalServiceKit/src/Protocols/ProtoUtils.m @@ -13,32 +13,37 @@ NS_ASSUME_NONNULL_BEGIN @implementation ProtoUtils +#pragma mark - Dependencies + ++ (id)profileManager { + return SSKEnvironment.shared.profileManager; +} + ++ (OWSAES256Key *)localProfileKey +{ + return self.profileManager.localProfileKey; +} + +#pragma mark - + + (BOOL)shouldMessageHaveLocalProfileKey:(TSThread *)thread recipientId:(NSString *_Nullable)recipientId { OWSAssertDebug(thread); - id profileManager = SSKEnvironment.shared.profileManager; - // For 1:1 threads, we want to include the profile key IFF the // contact is in the whitelist. // // For Group threads, we want to include the profile key IFF the // recipient OR the group is in the whitelist. - if (recipientId.length > 0 && [profileManager isUserInProfileWhitelist:recipientId]) { + if (recipientId.length > 0 && [self.profileManager isUserInProfileWhitelist:recipientId]) { return YES; - } else if ([profileManager isThreadInProfileWhitelist:thread]) { + } else if ([self.profileManager isThreadInProfileWhitelist:thread]) { return YES; } return NO; } -+ (OWSAES256Key *)localProfileKey -{ - id profileManager = SSKEnvironment.shared.profileManager; - return profileManager.localProfileKey; -} - + (void)addLocalProfileKeyIfNecessary:(TSThread *)thread recipientId:(NSString *_Nullable)recipientId dataMessageBuilder:(SSKProtoDataMessageBuilder *)dataMessageBuilder @@ -52,10 +57,9 @@ NS_ASSUME_NONNULL_BEGIN if (recipientId.length > 0) { // Once we've shared our profile key with a user (perhaps due to being // a member of a whitelisted group), make sure they're whitelisted. - id profileManager = SSKEnvironment.shared.profileManager; // FIXME PERF avoid this dispatch. It's going to happen for *each* recipient in a group message. dispatch_async(dispatch_get_main_queue(), ^{ - [profileManager addUserToProfileWhitelist:recipientId]; + [self.profileManager addUserToProfileWhitelist:recipientId]; }); } } @@ -81,10 +85,9 @@ NS_ASSUME_NONNULL_BEGIN // Once we've shared our profile key with a user (perhaps due to being // a member of a whitelisted group), make sure they're whitelisted. - id profileManager = SSKEnvironment.shared.profileManager; // FIXME PERF avoid this dispatch. It's going to happen for *each* recipient in a group message. dispatch_async(dispatch_get_main_queue(), ^{ - [profileManager addUserToProfileWhitelist:recipientId]; + [self.profileManager addUserToProfileWhitelist:recipientId]; }); } } From 283cb182887d84a71e9d44ececc863fbd77ee155 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 9 Oct 2018 09:09:30 -0400 Subject: [PATCH 4/7] Re-run UD attributes migration. --- .../environment/migrations/OWS111UDAttributesMigration.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift b/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift index bf0c43cc5..708d0cd08 100644 --- a/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift +++ b/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift @@ -19,7 +19,7 @@ public class OWS111UDAttributesMigration: OWSDatabaseMigration { // increment a similar constant for each migration. @objc class func migrationId() -> String { - return "111" + return "111.1" } override public func runUp(completion: @escaping OWSDatabaseMigrationCompletion) { From e47b69e0aaec1a964b16db6f97e282e5dd60c7da Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 9 Oct 2018 09:11:06 -0400 Subject: [PATCH 5/7] Send sync messages to self via UD (only); discard self-sent sync messages. --- .../src/Contacts/SignalRecipient.m | 11 ++++--- .../src/Messages/OWSMessageDecrypter.h | 1 + .../src/Messages/OWSMessageDecrypter.m | 30 ++++++++++++++----- .../src/Messages/OWSMessageSender.m | 19 ++++++++++-- .../src/Messages/UD/OWSUDManager.swift | 7 +++++ 5 files changed, 53 insertions(+), 15 deletions(-) diff --git a/SignalServiceKit/src/Contacts/SignalRecipient.m b/SignalServiceKit/src/Contacts/SignalRecipient.m index 991f93352..451c84a32 100644 --- a/SignalServiceKit/src/Contacts/SignalRecipient.m +++ b/SignalServiceKit/src/Contacts/SignalRecipient.m @@ -3,6 +3,7 @@ // #import "SignalRecipient.h" +#import "OWSDevice.h" #import "TSAccountManager.h" #import @@ -52,7 +53,7 @@ NS_ASSUME_NONNULL_BEGIN // // OWSMessageSender will correct this if it is wrong the next time // we send a message to this recipient. - _devices = [NSOrderedSet orderedSetWithObject:@(1)]; + _devices = [NSOrderedSet orderedSetWithObject:@(OWSDevicePrimaryDeviceId)]; } return self; @@ -69,7 +70,8 @@ NS_ASSUME_NONNULL_BEGIN _devices = [NSOrderedSet new]; } - if ([self.uniqueId isEqual:[TSAccountManager localNumber]] && [self.devices containsObject:@(1)]) { + if ([self.uniqueId isEqual:[TSAccountManager localNumber]] && + [self.devices containsObject:@(OWSDevicePrimaryDeviceId)]) { OWSFailDebug(@"self as recipient device"); } @@ -89,8 +91,9 @@ NS_ASSUME_NONNULL_BEGIN - (void)addDevices:(NSSet *)devices { OWSAssertDebug(devices.count > 0); - - if ([self.uniqueId isEqual:[TSAccountManager localNumber]] && [devices containsObject:@(1)]) { + + if ([self.uniqueId isEqual:[TSAccountManager localNumber]] && + [devices containsObject:@(OWSDevicePrimaryDeviceId)]) { OWSFailDebug(@"adding self as recipient device"); return; } diff --git a/SignalServiceKit/src/Messages/OWSMessageDecrypter.h b/SignalServiceKit/src/Messages/OWSMessageDecrypter.h index 0fcd81f9e..dda173286 100644 --- a/SignalServiceKit/src/Messages/OWSMessageDecrypter.h +++ b/SignalServiceKit/src/Messages/OWSMessageDecrypter.h @@ -16,6 +16,7 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, readonly, nullable) NSData *plaintextData; @property (nonatomic, readonly) NSString *source; @property (nonatomic, readonly) UInt32 sourceDevice; +@property (nonatomic, readonly) BOOL isUDMessage; @end diff --git a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m index 59a9b94c6..44b14d46a 100644 --- a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m +++ b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m @@ -8,6 +8,7 @@ #import "NotificationsProtocol.h" #import "OWSAnalytics.h" #import "OWSBlockingManager.h" +#import "OWSDevice.h" #import "OWSError.h" #import "OWSIdentityManager.h" #import "OWSPrimaryStorage+PreKeyStore.h" @@ -46,6 +47,7 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes @property (nonatomic, nullable) NSData *plaintextData; @property (nonatomic) NSString *source; @property (nonatomic) UInt32 sourceDevice; +@property (nonatomic) BOOL isUDMessage; @end @@ -57,6 +59,7 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes plaintextData:(nullable NSData *)plaintextData source:(NSString *)source sourceDevice:(UInt32)sourceDevice + isUDMessage:(BOOL)isUDMessage { OWSAssertDebug(envelopeData); OWSAssertDebug(source.length > 0); @@ -67,6 +70,7 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes result.plaintextData = plaintextData; result.source = source; result.sourceDevice = sourceDevice; + result.isUDMessage = isUDMessage; return result; } @@ -161,7 +165,15 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes OWSMessageDecryptResult *result, YapDatabaseReadWriteTransaction *transaction) { // Ensure all blocked messages are discarded. if ([self isEnvelopeSenderBlocked:envelope]) { - OWSLogInfo(@"ignoring blocked envelope: %@", envelope.source); + OWSLogInfo(@"Ignoring blocked envelope: %@", envelope.source); + return failureBlock(); + } + + if ([result.source isEqualToString:TSAccountManager.sharedInstance.localNumber] + && result.sourceDevice == OWSDevicePrimaryDeviceId) { + OWSAssertDebug(result.isUDMessage); + + OWSLogInfo(@"Ignoring self-sent sync message."); return failureBlock(); } @@ -237,7 +249,8 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes [OWSMessageDecryptResult resultWithEnvelopeData:envelopeData plaintextData:nil source:envelope.source - sourceDevice:envelope.sourceDevice]; + sourceDevice:envelope.sourceDevice + isUDMessage:NO]; successBlock(result, transaction); }]; // Return to avoid double-acknowledging. @@ -361,11 +374,11 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes // plaintextData may be nil for some envelope types. NSData *_Nullable plaintextData = [[cipher decrypt:cipherMessage protocolContext:transaction] removePadding]; - OWSMessageDecryptResult *result = - [OWSMessageDecryptResult resultWithEnvelopeData:envelopeData - plaintextData:plaintextData - source:envelope.source - sourceDevice:envelope.sourceDevice]; + OWSMessageDecryptResult *result = [OWSMessageDecryptResult resultWithEnvelopeData:envelopeData + plaintextData:plaintextData + source:envelope.source + sourceDevice:envelope.sourceDevice + isUDMessage:NO]; successBlock(result, transaction); } @catch (NSException *exception) { dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ @@ -471,7 +484,8 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes OWSMessageDecryptResult *result = [OWSMessageDecryptResult resultWithEnvelopeData:newEnvelopeData plaintextData:plaintextData source:source - sourceDevice:(uint32_t)sourceDeviceId]; + sourceDevice:(uint32_t)sourceDeviceId + isUDMessage:YES]; successBlock(result, transaction); } @catch (NSException *exception) { dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index f4d815cf7..cf2d6d1cd 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1324,7 +1324,20 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSLogDebug( @"built message: %@ plainTextData.length: %lu", [messageSend.message class], (unsigned long)plainText.length); - for (NSNumber *deviceNumber in messageSend.recipient.devices) { + OWSLogDebug(@"recipient.devices: %@", recipient.devices); + [DDLog flushLog]; + + NSMutableArray *deviceIds = [recipient.devices mutableCopy]; + OWSAssertDebug(deviceIds); + + if (messageSend.isUDSend && messageSend.isLocalNumber) { + const NSUInteger kLocalDeviceId = 1; + OWSAssertDebug(![deviceIds containsObject:@(OWSDevicePrimaryDeviceId)]); + + [deviceIds addObject:@(OWSDevicePrimaryDeviceId)]; + } + + for (NSNumber *deviceId in deviceIds) { @try { __block NSDictionary *messageDict; __block NSException *encryptionException; @@ -1332,7 +1345,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { @try { messageDict = [self encryptedMessageForMessageSend:messageSend - deviceId:deviceNumber + deviceId:deviceId plainText:plainText transaction:transaction]; } @catch (NSException *exception) { @@ -1353,7 +1366,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } @catch (NSException *exception) { if ([exception.name isEqualToString:OWSMessageSenderInvalidDeviceException]) { [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [recipient removeDevicesFromRecipient:[NSSet setWithObject:deviceNumber] transaction:transaction]; + [recipient removeDevicesFromRecipient:[NSSet setWithObject:deviceId] transaction:transaction]; }]; } else { @throw exception; diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 83841d72d..0c4022eed 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -94,10 +94,17 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { return SSKEnvironment.shared.profileManager } + private var tsAccountManager: TSAccountManager { + return TSAccountManager.sharedInstance() + } + // MARK: - Recipient state @objc public func supportsUnidentifiedDelivery(recipientId: String) -> Bool { + if tsAccountManager.localNumber() == recipientId { + return true + } return dbConnection.bool(forKey: recipientId, inCollection: kUDRecipientModeCollection, defaultValue: false) } From 75e59bbc6f2b711a7c69c2d625b613b601b3dced Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 9 Oct 2018 10:54:54 -0400 Subject: [PATCH 6/7] Discard self-sent messages during the decryption process. --- Podfile | 2 +- Podfile.lock | 9 +++--- .../src/Messages/OWSMessageDecrypter.m | 32 +++++++++++++++---- .../src/Messages/OWSMessageSender.m | 1 - 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/Podfile b/Podfile index 228be5bb6..e8031bb64 100644 --- a/Podfile +++ b/Podfile @@ -19,7 +19,7 @@ def shared_pods pod 'Curve25519Kit', git: 'https://github.com/signalapp/Curve25519Kit', testspecs: ["Tests"] # pod 'Curve25519Kit', path: '../Curve25519Kit', testspecs: ["Tests"] # TODO: Use public repo. - pod 'SignalMetadataKit', git: 'https://github.com/signalapp/SignalMetadataKit', testspecs: ["Tests"] + pod 'SignalMetadataKit', git: 'https://github.com/signalapp/SignalMetadataKit', testspecs: ["Tests"], branch: 'charlesmchen/discardSelfSentMessages' # pod 'SignalMetadataKit', path: '../SignalMetadataKit', testspecs: ["Tests"] pod 'SignalServiceKit', path: '.', testspecs: ["Tests"] pod 'GRKOpenSSLFramework', git: 'https://github.com/signalapp/GRKOpenSSLFramework' diff --git a/Podfile.lock b/Podfile.lock index d4cf61fc0..aaf2e22cf 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -196,8 +196,8 @@ DEPENDENCIES: - Reachability - SignalCoreKit (from `https://github.com/signalapp/SignalCoreKit.git`) - SignalCoreKit/Tests (from `https://github.com/signalapp/SignalCoreKit.git`) - - SignalMetadataKit (from `https://github.com/signalapp/SignalMetadataKit`) - - SignalMetadataKit/Tests (from `https://github.com/signalapp/SignalMetadataKit`) + - SignalMetadataKit (from `https://github.com/signalapp/SignalMetadataKit`, branch `charlesmchen/discardSelfSentMessages`) + - SignalMetadataKit/Tests (from `https://github.com/signalapp/SignalMetadataKit`, branch `charlesmchen/discardSelfSentMessages`) - SignalServiceKit (from `.`) - SignalServiceKit/Tests (from `.`) - SocketRocket (from `https://github.com/signalapp/SocketRocket.git`, branch `mkirk/handle-sec-err`) @@ -233,6 +233,7 @@ EXTERNAL SOURCES: SignalCoreKit: :git: https://github.com/signalapp/SignalCoreKit.git SignalMetadataKit: + :branch: charlesmchen/discardSelfSentMessages :git: https://github.com/signalapp/SignalMetadataKit SignalServiceKit: :path: "." @@ -263,7 +264,7 @@ CHECKOUT OPTIONS: :commit: ff0b95770520133b83a4bd7b26bc2c90b51abc4d :git: https://github.com/signalapp/SignalCoreKit.git SignalMetadataKit: - :commit: b0e664410dd3d709355bfdb9d464ae02644aeb74 + :commit: 0ff4673181315f5bd7b883a87b783b5772f7b412 :git: https://github.com/signalapp/SignalMetadataKit SocketRocket: :commit: 9f9563a83cd8960503074aa8de72206f83fb7a69 @@ -298,6 +299,6 @@ SPEC CHECKSUMS: YapDatabase: b418a4baa6906e8028748938f9159807fd039af4 YYImage: 1e1b62a9997399593e4b9c4ecfbbabbf1d3f3b54 -PODFILE CHECKSUM: 820287bc7925d7c20e02a02923976c60b1f5386b +PODFILE CHECKSUM: b4815f8e6306c08266b24710736a8c956b666aa1 COCOAPODS: 1.5.3 diff --git a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m index 44b14d46a..35817bcba 100644 --- a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m +++ b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m @@ -138,6 +138,11 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes return [self.blockingManager.blockedPhoneNumbers containsObject:envelope.source]; } +- (TSAccountManager *)tsAccountManager +{ + return TSAccountManager.sharedInstance; +} + #pragma mark - Decryption - (void)decryptEnvelope:(SSKProtoEnvelope *)envelope @@ -161,6 +166,8 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes }); }; + NSString *localRecipientId = self.tsAccountManager.localNumber; + uint32_t localDeviceId = OWSDevicePrimaryDeviceId; DecryptSuccessBlock successBlock = ^( OWSMessageDecryptResult *result, YapDatabaseReadWriteTransaction *transaction) { // Ensure all blocked messages are discarded. @@ -169,11 +176,9 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes return failureBlock(); } - if ([result.source isEqualToString:TSAccountManager.sharedInstance.localNumber] - && result.sourceDevice == OWSDevicePrimaryDeviceId) { - OWSAssertDebug(result.isUDMessage); - - OWSLogInfo(@"Ignoring self-sent sync message."); + if ([result.source isEqualToString:localRecipientId] && result.sourceDevice == localDeviceId) { + // Self-sent messages should be discarded during the decryption process. + OWSFailDebug(@"Unexpected self-sent sync message."); return failureBlock(); } @@ -427,6 +432,9 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes id certificateValidator = [[SMKCertificateDefaultValidator alloc] initWithTrustRoot:self.udManager.trustRoot]; + NSString *localRecipientId = self.tsAccountManager.localNumber; + uint32_t localDeviceId = OWSDevicePrimaryDeviceId; + [self.dbConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { @try { NSError *error; @@ -446,9 +454,16 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes [cipher decryptMessageWithCertificateValidator:certificateValidator cipherTextData:encryptedData timestamp:serverTimestamp + localRecipientId:localRecipientId + localDeviceId:localDeviceId protocolContext:transaction error:&error]; if (error || !decryptResult) { + if ([error.domain isEqualToString:@"SignalMetadataKit.SMKSelfSentMessageError"]) { + // Self-sent messages can be safely discarded. + return failureBlock(error); + } + OWSFailDebug(@"Could not decrypt UD message: %@", error); error = EnsureDecryptError(error, @"Could not decrypt UD message"); return failureBlock(error); @@ -531,7 +546,12 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes return; } else { OWSProdErrorWEnvelope([OWSAnalyticsEvents messageManagerErrorCorruptMessage], envelope); - errorMessage = [TSErrorMessage corruptedMessageWithEnvelope:envelope withTransaction:transaction]; + if (envelope.source.length > 0) { + errorMessage = [TSErrorMessage corruptedMessageWithEnvelope:envelope withTransaction:transaction]; + } else { + // TODO: Find another way to surface undecryptable UD messages to the user. + return; + } } OWSAssertDebug(errorMessage); diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index cf2d6d1cd..d091dc94d 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1331,7 +1331,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSAssertDebug(deviceIds); if (messageSend.isUDSend && messageSend.isLocalNumber) { - const NSUInteger kLocalDeviceId = 1; OWSAssertDebug(![deviceIds containsObject:@(OWSDevicePrimaryDeviceId)]); [deviceIds addObject:@(OWSDevicePrimaryDeviceId)]; From fab79e26737f2faa7ad411c3148da0d1bddf3e1c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 10 Oct 2018 09:59:10 -0400 Subject: [PATCH 7/7] Respond to CR. --- Podfile | 2 +- Podfile.lock | 9 ++++----- .../migrations/OWS111UDAttributesMigration.swift | 2 ++ .../src/Messages/DeviceSyncing/OWSSyncContactsMessage.m | 2 +- SignalServiceKit/src/Messages/OWSMessageDecrypter.m | 3 ++- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Podfile b/Podfile index e8031bb64..228be5bb6 100644 --- a/Podfile +++ b/Podfile @@ -19,7 +19,7 @@ def shared_pods pod 'Curve25519Kit', git: 'https://github.com/signalapp/Curve25519Kit', testspecs: ["Tests"] # pod 'Curve25519Kit', path: '../Curve25519Kit', testspecs: ["Tests"] # TODO: Use public repo. - pod 'SignalMetadataKit', git: 'https://github.com/signalapp/SignalMetadataKit', testspecs: ["Tests"], branch: 'charlesmchen/discardSelfSentMessages' + pod 'SignalMetadataKit', git: 'https://github.com/signalapp/SignalMetadataKit', testspecs: ["Tests"] # pod 'SignalMetadataKit', path: '../SignalMetadataKit', testspecs: ["Tests"] pod 'SignalServiceKit', path: '.', testspecs: ["Tests"] pod 'GRKOpenSSLFramework', git: 'https://github.com/signalapp/GRKOpenSSLFramework' diff --git a/Podfile.lock b/Podfile.lock index aaf2e22cf..1b830b351 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -196,8 +196,8 @@ DEPENDENCIES: - Reachability - SignalCoreKit (from `https://github.com/signalapp/SignalCoreKit.git`) - SignalCoreKit/Tests (from `https://github.com/signalapp/SignalCoreKit.git`) - - SignalMetadataKit (from `https://github.com/signalapp/SignalMetadataKit`, branch `charlesmchen/discardSelfSentMessages`) - - SignalMetadataKit/Tests (from `https://github.com/signalapp/SignalMetadataKit`, branch `charlesmchen/discardSelfSentMessages`) + - SignalMetadataKit (from `https://github.com/signalapp/SignalMetadataKit`) + - SignalMetadataKit/Tests (from `https://github.com/signalapp/SignalMetadataKit`) - SignalServiceKit (from `.`) - SignalServiceKit/Tests (from `.`) - SocketRocket (from `https://github.com/signalapp/SocketRocket.git`, branch `mkirk/handle-sec-err`) @@ -233,7 +233,6 @@ EXTERNAL SOURCES: SignalCoreKit: :git: https://github.com/signalapp/SignalCoreKit.git SignalMetadataKit: - :branch: charlesmchen/discardSelfSentMessages :git: https://github.com/signalapp/SignalMetadataKit SignalServiceKit: :path: "." @@ -264,7 +263,7 @@ CHECKOUT OPTIONS: :commit: ff0b95770520133b83a4bd7b26bc2c90b51abc4d :git: https://github.com/signalapp/SignalCoreKit.git SignalMetadataKit: - :commit: 0ff4673181315f5bd7b883a87b783b5772f7b412 + :commit: beb10a358db0202228b8d67bcb466d877fefb405 :git: https://github.com/signalapp/SignalMetadataKit SocketRocket: :commit: 9f9563a83cd8960503074aa8de72206f83fb7a69 @@ -299,6 +298,6 @@ SPEC CHECKSUMS: YapDatabase: b418a4baa6906e8028748938f9159807fd039af4 YYImage: 1e1b62a9997399593e4b9c4ecfbbabbf1d3f3b54 -PODFILE CHECKSUM: b4815f8e6306c08266b24710736a8c956b666aa1 +PODFILE CHECKSUM: 820287bc7925d7c20e02a02923976c60b1f5386b COCOAPODS: 1.5.3 diff --git a/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift b/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift index 708d0cd08..98f35617a 100644 --- a/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift +++ b/SignalMessaging/environment/migrations/OWS111UDAttributesMigration.swift @@ -19,6 +19,8 @@ public class OWS111UDAttributesMigration: OWSDatabaseMigration { // increment a similar constant for each migration. @objc class func migrationId() -> String { + // NOTE: Changes were made to the service after this migration was initially + // merged, so we need to re-migrate any developer devices. return "111.1" } diff --git a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m index 4c398c2b3..97264b96f 100644 --- a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m +++ b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncContactsMessage.m @@ -117,7 +117,7 @@ NS_ASSUME_NONNULL_BEGIN NSOutputStream *dataOutputStream = [NSOutputStream outputStreamToMemory]; [dataOutputStream open]; OWSContactsOutputStream *contactsOutputStream = - [[OWSContactsOutputStream alloc] initWithOutputStream:dataOutputStream]; + [[OWSContactsOutputStream alloc] initWithOutputStream:dataOutputStream]; for (SignalAccount *signalAccount in signalAccounts) { OWSRecipientIdentity *_Nullable recipientIdentity = diff --git a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m index 35817bcba..8e7b51445 100644 --- a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m +++ b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m @@ -459,7 +459,8 @@ NSError *EnsureDecryptError(NSError *_Nullable error, NSString *fallbackErrorDes protocolContext:transaction error:&error]; if (error || !decryptResult) { - if ([error.domain isEqualToString:@"SignalMetadataKit.SMKSelfSentMessageError"]) { + if ([error.domain isEqualToString:@"SignalMetadataKit.SMKSecretSessionCipherError"] + && error.code == SMKSecretSessionCipherErrorSelfSentMessage) { // Self-sent messages can be safely discarded. return failureBlock(error); }