From 54b184bc97e3dfa6841b59d1c41393c5ec90bd32 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 11 Oct 2018 13:44:17 -0600 Subject: [PATCH 1/5] Whitelist cds domain --- Signal/Signal-Info.plist | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Signal/Signal-Info.plist b/Signal/Signal-Info.plist index 4c2802d1e..00700784f 100644 --- a/Signal/Signal-Info.plist +++ b/Signal/Signal-Info.plist @@ -60,6 +60,13 @@ NSThirdPartyExceptionRequiresForwardSecrecy + api.directory.signal.org + + NSThirdPartyExceptionRequiresForwardSecrecy + + NSExceptionAllowsInsecureHTTPLoads + + signal.org NSExceptionAllowsInsecureHTTPLoads From e7170dc6e875adbde1d1a9d9a0eb73873b3f5e17 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 11 Oct 2018 17:06:12 -0600 Subject: [PATCH 2/5] conventional error structure for connectivity error --- .../OWS106EnsureProfileComplete.swift | 2 +- .../src/Account/TSAccountManager.m | 2 +- .../src/Network/API/TSNetworkManager.h | 10 ++++++++- .../src/Network/API/TSNetworkManager.m | 21 +++++++++++-------- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift b/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift index 0029c1e50..cc27c5bdd 100644 --- a/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift +++ b/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift @@ -64,7 +64,7 @@ public class OWS106EnsureProfileComplete: OWSDatabaseMigration { self.completionHandler(true) }.catch { error in let nserror = error as NSError - if nserror.domain == TSNetworkManagerDomain { + if nserror.domain == TSNetworkManagerErrorDomain { // Don't retry if we had an unrecoverable error. // In particular, 401 (invalid auth) is unrecoverable. let isUnrecoverableError = nserror.code == 401 diff --git a/SignalServiceKit/src/Account/TSAccountManager.m b/SignalServiceKit/src/Account/TSAccountManager.m index d5dbca936..087445efd 100644 --- a/SignalServiceKit/src/Account/TSAccountManager.m +++ b/SignalServiceKit/src/Account/TSAccountManager.m @@ -386,7 +386,7 @@ NSString *const TSAccountManager_ServerSignalingKey = @"TSStorageServerSignaling if (!IsNSErrorNetworkFailure(error)) { OWSProdError([OWSAnalyticsEvents accountsErrorVerifyAccountRequestFailed]); } - OWSAssertDebug([error.domain isEqualToString:TSNetworkManagerDomain]); + OWSAssertDebug([error.domain isEqualToString:TSNetworkManagerErrorDomain]); OWSLogWarn(@"Error verifying code: %@", error.debugDescription); diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.h b/SignalServiceKit/src/Network/API/TSNetworkManager.h index e91bacae5..0dda2495a 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.h +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.h @@ -6,7 +6,15 @@ NS_ASSUME_NONNULL_BEGIN -extern NSString *const TSNetworkManagerDomain; +extern NSErrorDomain const TSNetworkManagerErrorDomain; +typedef NS_ERROR_ENUM(TSNetworkManagerErrorDomain, TSNetworkManagerError){ + // It's a shame to use 0 as an enum value for anything other than something like default or unknown, because it's + // indistinguishable from "not set" in Objc. + // However this value was existing behavior for connectivity errors, and since we might be using this in other + // places I didn't want to change it out of hand + TSNetworkManagerErrorFailedConnection = 0, + // Other TSNetworkManagerError's use HTTP status codes (e.g. 404, etc) +}; BOOL IsNSErrorNetworkFailure(NSError *_Nullable error); diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.m b/SignalServiceKit/src/Network/API/TSNetworkManager.m index 2c57838fd..af3bec64a 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.m +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.m @@ -7,6 +7,7 @@ #import "NSData+OWS.h" #import "NSError+messageSending.h" #import "NSURLSessionDataTask+StatusCode.h" +#import "OWSError.h" #import "OWSSignalService.h" #import "SSKEnvironment.h" #import "TSAccountManager.h" @@ -14,11 +15,12 @@ #import #import -NSString *const TSNetworkManagerDomain = @"org.whispersystems.signal.networkManager"; +NSErrorDomain const TSNetworkManagerErrorDomain = @"SignalServiceKit.TSNetworkManager"; BOOL IsNSErrorNetworkFailure(NSError *_Nullable error) { - return ([error.domain isEqualToString:TSNetworkManagerDomain] && error.code == 0); + return ([error.domain isEqualToString:TSNetworkManagerErrorDomain] + && error.code == TSNetworkManagerErrorFailedConnection); } @interface TSNetworkManager () @@ -218,16 +220,17 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); switch (statusCode) { case 0: { - error.isRetryable = YES; - - OWSLogWarn(@"The network request failed because of a connectivity error: %@", request); - failureBlock(task, - [self errorWithHTTPCode:statusCode + NSError *connectivityError = + [self errorWithHTTPCode:TSNetworkManagerErrorFailedConnection description:NSLocalizedString(@"ERROR_DESCRIPTION_NO_INTERNET", @"Generic error used whenever Signal can't contact the server") failureReason:networkError.localizedFailureReason recoverySuggestion:NSLocalizedString(@"NETWORK_ERROR_RECOVERY", nil) - fallbackError:networkError]); + fallbackError:networkError]; + connectivityError.isRetryable = YES; + + OWSLogWarn(@"The network request failed because of a connectivity error: %@", request); + failureBlock(task, connectivityError); break; } case 400: { @@ -356,7 +359,7 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); [dict setObject:failureData forKey:AFNetworkingOperationFailingURLResponseDataErrorKey]; } - return [NSError errorWithDomain:TSNetworkManagerDomain code:code userInfo:dict]; + return [NSError errorWithDomain:TSNetworkManagerErrorDomain code:code userInfo:dict]; } @end From e22ad8ba66ca77af325dd7c169ba0b08bca80ee2 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 11 Oct 2018 17:11:25 -0600 Subject: [PATCH 3/5] include underlying error in wrapped TSNetworkErrors --- SignalServiceKit/src/Network/API/TSNetworkManager.m | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.m b/SignalServiceKit/src/Network/API/TSNetworkManager.m index af3bec64a..1733b244e 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.m +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.m @@ -330,7 +330,10 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); description:(NSString *)description failureReason:(NSString *)failureReason recoverySuggestion:(NSString *)recoverySuggestion - fallbackError:(NSError *_Nonnull)fallbackError { + fallbackError:(NSError *)fallbackError +{ + OWSAssertDebug(fallbackError); + if (!description) { description = fallbackError.localizedDescription; } @@ -359,6 +362,8 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); [dict setObject:failureData forKey:AFNetworkingOperationFailingURLResponseDataErrorKey]; } + dict[NSUnderlyingErrorKey] = fallbackError; + return [NSError errorWithDomain:TSNetworkManagerErrorDomain code:code userInfo:dict]; } From c4550ebc903f9325856ad6060a742cd5b760033f Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 11 Oct 2018 17:15:35 -0600 Subject: [PATCH 4/5] don't submit feedback for connectivity errors --- .../src/Contacts/OWSContactDiscoveryOperation.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index df4ac48dc..c60900788 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -456,8 +456,11 @@ class CDSFeedbackOperation: OWSOperation { if let error = cdsOperation.failingError { switch error { + case TSNetworkManagerError.failedConnection: + // Don't submit feedback for connectivity errors + self.reportSuccess() case ContactDiscoveryError.serverError, ContactDiscoveryError.clientError: - // Server already has this information, no need to report. + // Server already has this information, no need submit feedback self.reportSuccess() case ContactDiscoveryError.attestationError: self.makeRequest(result: .attestationError) From 5edf2e426e05cfa9367632744adce813067234f4 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 11 Oct 2018 17:53:58 -0600 Subject: [PATCH 5/5] Only report attestation failure if we *received* the attestion. per Jeff --- .../src/Contacts/ContactDiscoveryService.h | 5 +++++ .../src/Contacts/ContactDiscoveryService.m | 10 +++++++--- .../src/Contacts/OWSContactDiscoveryOperation.swift | 11 ++--------- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/SignalServiceKit/src/Contacts/ContactDiscoveryService.h b/SignalServiceKit/src/Contacts/ContactDiscoveryService.h index 58952bb27..645b33691 100644 --- a/SignalServiceKit/src/Contacts/ContactDiscoveryService.h +++ b/SignalServiceKit/src/Contacts/ContactDiscoveryService.h @@ -4,6 +4,11 @@ NS_ASSUME_NONNULL_BEGIN +extern NSErrorDomain const ContactDiscoveryServiceErrorDomain; +typedef NS_ERROR_ENUM(ContactDiscoveryServiceErrorDomain, ContactDiscoveryServiceError){ + ContactDiscoveryServiceErrorAttestationFailed = 100, +}; + @class ECKeyPair; @class OWSAES256Key; diff --git a/SignalServiceKit/src/Contacts/ContactDiscoveryService.m b/SignalServiceKit/src/Contacts/ContactDiscoveryService.m index 8f6fee440..935c87164 100644 --- a/SignalServiceKit/src/Contacts/ContactDiscoveryService.m +++ b/SignalServiceKit/src/Contacts/ContactDiscoveryService.m @@ -8,6 +8,7 @@ #import "Cryptography.h" #import "NSData+OWS.h" #import "NSDate+OWS.h" +#import "NSError+MessageSending.h" #import "OWSError.h" #import "OWSRequestFactory.h" #import "TSNetworkManager.h" @@ -16,6 +17,8 @@ NS_ASSUME_NONNULL_BEGIN +NSErrorDomain const ContactDiscoveryServiceErrorDomain = @"SignalServiceKit.ContactDiscoveryService"; + @interface RemoteAttestationAuth () @property (nonatomic) NSString *username; @@ -357,7 +360,10 @@ NS_ASSUME_NONNULL_BEGIN auth:auth]; if (!attestation) { - NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); + NSError *error = [NSError errorWithDomain:ContactDiscoveryServiceErrorDomain + code:ContactDiscoveryServiceErrorAttestationFailed + userInfo:nil]; + error.isRetryable = NO; failureHandler(error); return; } @@ -366,8 +372,6 @@ NS_ASSUME_NONNULL_BEGIN }); } failure:^(NSURLSessionDataTask *task, NSError *error) { - NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response; - OWSLogVerbose(@"remote attestation failure: %lu", (unsigned long)response.statusCode); failureHandler(error); }]; } diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index c60900788..b55b7eaab 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -139,7 +139,6 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { enum ContactDiscoveryError: Error { case parseError(description: String) case assertionError(description: String) - case attestationError(underlyingError: Error) case clientError(underlyingError: Error) case serverError(underlyingError: Error) } @@ -234,13 +233,7 @@ class CDSBatchOperation: OWSOperation { contactDiscoveryService.performRemoteAttestation(success: { (remoteAttestation: RemoteAttestation) in self.makeContactDiscoveryRequest(remoteAttestation: remoteAttestation) }, - failure: self.attestationFailure) - } - - private func attestationFailure(error: Error) { - let attestationError: NSError = ContactDiscoveryError.attestationError(underlyingError: error) as NSError - attestationError.isRetryable = false - self.reportError(attestationError) + failure: self.reportError) } private func makeContactDiscoveryRequest(remoteAttestation: RemoteAttestation) { @@ -462,7 +455,7 @@ class CDSFeedbackOperation: OWSOperation { case ContactDiscoveryError.serverError, ContactDiscoveryError.clientError: // Server already has this information, no need submit feedback self.reportSuccess() - case ContactDiscoveryError.attestationError: + case ContactDiscoveryServiceError.attestationFailed: self.makeRequest(result: .attestationError) default: self.makeRequest(result: .unexpectedError)