Respond to security review.

This commit is contained in:
Matthew Chen 2018-10-30 16:15:27 -04:00
parent 7d8b20d091
commit 698e48f2d8
7 changed files with 38 additions and 36 deletions

View File

@ -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];

View File

@ -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

View File

@ -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<NSNumber *> *deviceIdSet = [NSMutableSet new];
for (OWSDevice *device in [OWSDevice currentDevicesWithTransaction:transaction]) {
[deviceIdSet addObject:@(device.deviceId)];

View File

@ -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

View File

@ -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)
}
}

View File

@ -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() {

View File

@ -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, ^{