From 698e48f2d85ef2498b99b28593dfb1caf9c8ecf0 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 30 Oct 2018 16:15:27 -0400 Subject: [PATCH] Respond to security review. --- SignalMessaging/profiles/OWSProfileManager.m | 2 +- .../profiles/ProfileFetcherJob.swift | 4 ++-- .../src/Messages/OWSMessageManager.m | 4 ++++ .../src/Messages/OWSMessageSender.m | 21 +++++++++++++++--- .../src/Messages/UD/OWSRequestMaker.swift | 16 ++++++++------ .../src/Messages/UD/OWSUDManager.swift | 22 ++++--------------- .../src/Network/WebSockets/OWSWebSocket.m | 5 ----- 7 files changed, 38 insertions(+), 36 deletions(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index c31f64414..e846fff17 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -1013,7 +1013,7 @@ typedef void (^ProfileManagerFailureBlock)(NSError *error); [userProfile clearWithProfileKey:profileKey dbConnection:self.dbConnection completion:^{ - dispatch_async(dispatch_get_main_queue(), ^(void) { + dispatch_async(dispatch_get_main_queue(), ^{ [self.udManager setUnidentifiedAccessMode:UnidentifiedAccessModeUnknown recipientId:recipientId]; [self fetchProfileForRecipientId:recipientId]; diff --git a/SignalMessaging/profiles/ProfileFetcherJob.swift b/SignalMessaging/profiles/ProfileFetcherJob.swift index c28236745..1261b3b0e 100644 --- a/SignalMessaging/profiles/ProfileFetcherJob.swift +++ b/SignalMessaging/profiles/ProfileFetcherJob.swift @@ -195,13 +195,13 @@ public class ProfileFetcherJob: NSObject { } let dataToVerify = Data(count: 32) - guard let expectedVerfier = Cryptography.computeSHA256HMAC(dataToVerify, withHMACKey: udAccessKey.keyData) else { + guard let expectedVerifier = Cryptography.computeSHA256HMAC(dataToVerify, withHMACKey: udAccessKey.keyData) else { owsFailDebug("could not compute verification") udManager.setUnidentifiedAccessMode(.disabled, recipientId: recipientId) return } - guard expectedVerfier == verifier else { + guard expectedVerifier.ows_constantTimeIsEqual(to: verifier) else { Logger.verbose("verifier mismatch, new profile key?") udManager.setUnidentifiedAccessMode(.disabled, recipientId: recipientId) return diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index ce403e995..1247a4a20 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -1453,6 +1453,8 @@ NS_ASSUME_NONNULL_BEGIN return; } + // Consult the device list cache we use for message sending + // whether or not we know about this linked device. SignalRecipient *_Nullable recipient = [SignalRecipient registeredRecipientForRecipientId:localNumber transaction:transaction]; if (!recipient) { @@ -1469,6 +1471,8 @@ NS_ASSUME_NONNULL_BEGIN } } + // Consult the device list cache we use for the "linked device" UI + // whether or not we know about this linked device. NSMutableSet *deviceIdSet = [NSMutableSet new]; for (OWSDevice *device in [OWSDevice currentDevicesWithTransaction:transaction]) { [deviceIdSet addObject:@(device.deviceId)]; diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 6ea0ce7ce..c65d29bc0 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1076,8 +1076,15 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSLogWarn(@"Sending a message with no device messages."); } - // NOTE: canFailoverUDAuth is NO because UD-auth and Non-UD-auth requests - // use different device lists. + // NOTE: canFailoverUDAuth depends on whether or not we're sending a + // sync message because sync messages use different device lists + // for UD-auth and Non-UD-auth requests. + // + // Therefore, for sync messages, we can't use OWSRequestMaker's + // retry/failover logic; we need to use the message sender's retry + // logic that will build a new set of device messages. + BOOL isSyncMessageSend = messageSend.isLocalNumber; + BOOL canFailoverUDAuth = !isSyncMessageSend; OWSRequestMaker *requestMaker = [[OWSRequestMaker alloc] initWithLabel:@"Message Send" requestFactoryBlock:^(SMKUDAccessKey *_Nullable udAccessKey) { return [OWSRequestFactory submitMessageRequestWithRecipient:recipient.recipientId @@ -1086,14 +1093,18 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; udAccessKey:udAccessKey]; } udAuthFailureBlock:^{ + // Note the UD auth failure so subsequent retries + // to this recipient also use basic auth. [messageSend setHasUDAuthFailed]; } websocketFailureBlock:^{ + // Note the websocket failure so subsequent retries + // to this recipient also use REST. messageSend.hasWebsocketSendFailed = YES; } recipientId:recipient.recipientId udAccess:messageSend.udAccess - canFailoverUDAuth:NO]; + canFailoverUDAuth:canFailoverUDAuth]; [[requestMaker makeRequestObjc] .then(^(OWSRequestMakerResult *result) { dispatch_async([OWSDispatch sendingQueue], ^{ @@ -1578,9 +1589,13 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; udAccessKey:udAccessKey]; } udAuthFailureBlock:^{ + // Note the UD auth failure so subsequent retries + // to this recipient also use basic auth. [messageSend setHasUDAuthFailed]; } websocketFailureBlock:^{ + // Note the websocket failure so subsequent retries + // to this recipient also use REST. messageSend.hasWebsocketSendFailed = YES; } recipientId:recipientId diff --git a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift index d3193bcec..6a9b7b88b 100644 --- a/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift +++ b/SignalServiceKit/src/Messages/UD/OWSRequestMaker.swift @@ -112,8 +112,8 @@ public class RequestMaker: NSObject { if !skipUD { udAccessForRequest = udAccess } - let isUDRequest = udAccessForRequest != nil - let request = requestFactoryBlock(udAccessForRequest?.udAccessKey) + let isUDRequest: Bool = udAccessForRequest != nil + let request: TSRequest = requestFactoryBlock(udAccessForRequest?.udAccessKey) let webSocketType: OWSWebSocketType = (isUDRequest ? .UD : .default) let canMakeWebsocketRequests = (socketManager.canMakeRequests(of: webSocketType) && !skipWebsocket) @@ -142,8 +142,8 @@ public class RequestMaker: NSObject { // failure), mark recipient as _not_ in UD mode, then retry. self.udManager.setUnidentifiedAccessMode(.disabled, recipientId: self.recipientId) self.profileManager.fetchProfile(forRecipientId: self.recipientId) - self.udAuthFailureBlock() + if self.canFailoverUDAuth { Logger.info("UD websocket request '\(self.label)' auth failed; failing over to non-UD websocket request.") return self.makeRequestInternal(skipUD: true, skipWebsocket: skipWebsocket) @@ -185,8 +185,8 @@ public class RequestMaker: NSObject { // failure), mark recipient as _not_ in UD mode, then retry. self.udManager.setUnidentifiedAccessMode(.disabled, recipientId: self.recipientId) self.profileManager.fetchProfile(forRecipientId: self.recipientId) - self.udAuthFailureBlock() + if self.canFailoverUDAuth { Logger.info("UD REST request '\(self.label)' auth failed; failing over to non-UD REST request.") return self.makeRequestInternal(skipUD: true, skipWebsocket: skipWebsocket) @@ -207,9 +207,11 @@ public class RequestMaker: NSObject { } private func requestSucceeded(udAccess: OWSUDAccess?) { + // If this was a UD request... guard let udAccess = udAccess else { return } + // ...made for a user in "unknown" UD access mode... guard udAccess.udAccessMode == .unknown else { return } @@ -217,12 +219,12 @@ public class RequestMaker: NSObject { if udAccess.isRandomKey { // If a UD request succeeds for an unknown user with a random key, // mark recipient as .unrestricted. - self.udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: self.recipientId) + udManager.setUnidentifiedAccessMode(.unrestricted, recipientId: recipientId) } else { // If a UD request succeeds for an unknown user with a non-random key, // mark recipient as .enabled. - self.udManager.setUnidentifiedAccessMode(.enabled, recipientId: self.recipientId) + udManager.setUnidentifiedAccessMode(.enabled, recipientId: recipientId) } - self.profileManager.fetchProfile(forRecipientId: self.recipientId) + profileManager.fetchProfile(forRecipientId: recipientId) } } diff --git a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift index 95e5a8142..012c03eed 100644 --- a/SignalServiceKit/src/Messages/UD/OWSUDManager.swift +++ b/SignalServiceKit/src/Messages/UD/OWSUDManager.swift @@ -60,8 +60,6 @@ public class OWSUDAccess: NSObject { @objc func trustRoot() -> ECPublicKey - @objc func isUDEnabled() -> Bool - @objc func isUDVerboseLoggingEnabled() -> Bool // MARK: - Recipient State @@ -69,9 +67,6 @@ public class OWSUDAccess: NSObject { @objc func setUnidentifiedAccessMode(_ mode: UnidentifiedAccessMode, recipientId: String) - @objc - func randomUDAccessKey() -> SMKUDAccessKey - @objc func unidentifiedAccessMode(forRecipientId recipientId: RecipientIdentifier) -> UnidentifiedAccessMode @@ -151,11 +146,6 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { // MARK: - - @objc - public func isUDEnabled() -> Bool { - return true - } - @objc public func isUDVerboseLoggingEnabled() -> Bool { return true @@ -186,6 +176,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { return defaultValue } guard let existingValue = UnidentifiedAccessMode(rawValue: existingRawValue) else { + owsFailDebug("Couldn't parse mode value.") return defaultValue } return existingValue @@ -247,18 +238,12 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { @objc public func udAccess(forRecipientId recipientId: RecipientIdentifier, requireSyncAccess: Bool) -> OWSUDAccess? { - guard isUDEnabled() else { - if isUDVerboseLoggingEnabled() { - Logger.info("UD disabled for \(recipientId), UD disabled.") - } - return nil - } - if requireSyncAccess { guard let localNumber = tsAccountManager.localNumber() else { if isUDVerboseLoggingEnabled() { Logger.info("UD disabled for \(recipientId), no local number.") } + owsFailDebug("Missing local number.") return nil } if localNumber != recipientId { @@ -286,7 +271,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { // and otherwise use a random key. if let udAccessKey = udAccessKey(forRecipientId: recipientId) { if isUDVerboseLoggingEnabled() { - Logger.info("UD unknown for \(recipientId); trying random key.") + Logger.info("UD unknown for \(recipientId); trying derived key.") } return OWSUDAccess(udAccessKey: udAccessKey, udAccessMode: accessMode, isRandomKey: false) } else { @@ -301,6 +286,7 @@ public class OWSUDManagerImpl: NSObject, OWSUDManager { if isUDVerboseLoggingEnabled() { Logger.info("UD disabled for \(recipientId), no profile key for this recipient.") } + owsFailDebug("Couldn't find profile key for UD-enabled user.") return nil } if isUDVerboseLoggingEnabled() { diff --git a/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m b/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m index ad7757151..b2f514781 100644 --- a/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m +++ b/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.m @@ -1045,11 +1045,6 @@ NSString *NSStringFromOWSWebSocketType(OWSWebSocketType type) } #endif - if (!self.udManager.isUDEnabled && self.webSocketType == OWSWebSocketTypeUD) { - OWSLogWarn(@"Suppressing UD socket in prod."); - return; - } - if (!AppReadiness.isAppReady) { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{