From 32f1ce947303e876ed35b57a6cda4254863999d6 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Sep 2018 10:38:21 -0400 Subject: [PATCH 1/3] Clean up attachment downloads. --- .../Attachments/OWSAttachmentsProcessor.m | 142 ++++++++++++------ 1 file changed, 96 insertions(+), 46 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m index aa483099b..ad1587ff0 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -10,6 +10,7 @@ #import "OWSBackgroundTask.h" #import "OWSDispatch.h" #import "OWSError.h" +#import "OWSFileSystem.h" #import "OWSPrimaryStorage.h" #import "OWSRequestFactory.h" #import "TSAttachmentPointer.h" @@ -174,13 +175,27 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; dispatch_async([OWSDispatch attachmentsQueue], ^{ [self downloadFromLocation:location pointer:attachment - success:^(NSData *encryptedData) { - [self decryptAttachmentData:encryptedData - pointer:attachment - success:markAndHandleSuccess - failure:markAndHandleFailure]; + success:^(NSString *encryptedDataFilePath) { + // Use attachmentDecryptSerialQueue to ensure that we only load into memory + // & decrypt a single attachment at a time. + dispatch_async(self.attachmentDecryptSerialQueue, ^{ + @autoreleasepool { + NSData *_Nullable encryptedData = [NSData dataWithContentsOfFile:encryptedDataFilePath]; + if (!encryptedData) { + OWSLogError(@"Could not load encrypted data."); + NSError *error = OWSErrorWithCodeDescription(OWSErrorCodeInvalidMessage, + NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return markAndHandleFailure(error); + } + + [self decryptAttachmentData:encryptedData + pointer:attachment + success:markAndHandleSuccess + failure:markAndHandleFailure]; + } + }); } - failure:^(NSURLSessionDataTask *_Nullable task, NSError *error) { + failure:^(NSURLSessionTask *_Nullable task, NSError *error) { if (attachment.serverId < 100) { // This looks like the symptom of the "frequent 404 // downloading attachments with low server ids". @@ -191,9 +206,7 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; (unsigned long long)attachment.serverId, error); } - if (markAndHandleFailure) { - markAndHandleFailure(error); - } + markAndHandleFailure(error); }]; }); } @@ -254,27 +267,58 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; successHandler(stream); } +- (dispatch_queue_t)attachmentDecryptSerialQueue +{ + static dispatch_queue_t _serialQueue; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + _serialQueue = dispatch_queue_create("org.whispersystems.attachment.decrypt", DISPATCH_QUEUE_SERIAL); + }); + + return _serialQueue; +} + - (void)downloadFromLocation:(NSString *)location pointer:(TSAttachmentPointer *)pointer - success:(void (^)(NSData *encryptedData))successHandler - failure:(void (^)(NSURLSessionDataTask *_Nullable task, NSError *error))failureHandler + success:(void (^)(NSString *encryptedDataPath))successHandler + failure:(void (^)(NSURLSessionTask *_Nullable task, NSError *error))failureHandlerParam { + dispatch_queue_t completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); + AFHTTPSessionManager *manager = [AFHTTPSessionManager manager]; manager.requestSerializer = [AFHTTPRequestSerializer serializer]; [manager.requestSerializer setValue:OWSMimeTypeApplicationOctetStream forHTTPHeaderField:@"Content-Type"]; manager.responseSerializer = [AFHTTPResponseSerializer serializer]; - manager.completionQueue = dispatch_get_main_queue(); + manager.completionQueue = completionQueue; // We want to avoid large downloads from a compromised or buggy service. const long kMaxDownloadSize = 150 * 1024 * 1024; - // TODO stream this download rather than storing the entire blob. - __block NSURLSessionDataTask *task = nil; __block BOOL hasCheckedContentLength = NO; - task = [manager GET:location - parameters:nil + + __block NSURLSessionDownloadTask *task; + void (^failureHandler)(NSError *) = ^(NSError *error) { + OWSLogError(@"Failed to download attachment with error: %@", error.description); + failureHandlerParam(task, error); + }; + + NSString *method = @"GET"; + NSError *serializationError = nil; + NSMutableURLRequest *request = [manager.requestSerializer requestWithMethod:method + URLString:location + parameters:nil + error:&serializationError]; + if (serializationError) { + failureHandler(serializationError); + return; + } + + NSString *tempFilePath = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; + NSURL *tempFileURL = [NSURL fileURLWithPath:tempFilePath]; + + task = [manager downloadTaskWithRequest:request progress:^(NSProgress *progress) { OWSAssertDebug(progress != nil); - + // Don't do anything until we've received at least one byte of data. if (progress.completedUnitCount < 1) { return; @@ -305,7 +349,7 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; if (hasCheckedContentLength) { return; } - + // Once we've received some bytes of the download, check the content length // header for the download. // @@ -318,57 +362,63 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; abortDownload(); return; } - + NSDictionary *headers = [httpResponse allHeaderFields]; if (![headers isKindOfClass:[NSDictionary class]]) { OWSLogError(@"Attachment download invalid headers."); abortDownload(); return; } - - + + NSString *contentLength = headers[@"Content-Length"]; if (![contentLength isKindOfClass:[NSString class]]) { OWSLogError(@"Attachment download missing or invalid content length."); abortDownload(); return; } - - + + if (contentLength.longLongValue > kMaxDownloadSize) { OWSLogError(@"Attachment download content length exceeds max download size."); abortDownload(); return; } - + // This response has a valid content length that is less // than our max download size. Proceed with the download. hasCheckedContentLength = YES; } - success:^(NSURLSessionDataTask *task, id _Nullable responseObject) { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - if (![responseObject isKindOfClass:[NSData class]]) { - OWSLogError(@"Failed retrieval of attachment. Response had unexpected format."); - NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(task, error); - } - NSData *responseData = (NSData *)responseObject; - if (responseData.length > kMaxDownloadSize) { - OWSLogError(@"Attachment download content length exceeds max download size."); - NSError *error = OWSErrorWithCodeDescription( - OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); - failureHandler(task, error); - } else { - successHandler(responseData); - } - }); + destination:^(NSURL *targetPath, NSURLResponse *response) { + return tempFileURL; } - failure:^(NSURLSessionDataTask *_Nullable task, NSError *error) { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - OWSLogError(@"Failed to retrieve attachment with error: %@", error.description); - return failureHandler(task, error); - }); + completionHandler:^(NSURLResponse *response, NSURL *_Nullable filePath, NSError *_Nullable error) { + if (error) { + failureHandler(error); + return; + } + if (![tempFileURL isEqual:filePath]) { + OWSLogError(@"Unexpected temp file path."); + NSError *error = OWSErrorWithCodeDescription( + OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return failureHandler(error); + } + NSNumber *_Nullable fileSize = [OWSFileSystem fileSizeOfPath:filePath.path]; + if (!fileSize) { + OWSLogError(@"Could not determine attachment file size."); + NSError *error = OWSErrorWithCodeDescription( + OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return failureHandler(error); + } + if (fileSize.unsignedIntegerValue > kMaxDownloadSize) { + OWSLogError(@"Attachment download length exceeds max size."); + NSError *error = OWSErrorWithCodeDescription( + OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return failureHandler(error); + } + successHandler(filePath.path); }]; + [task resume]; } - (void)fireProgressNotification:(CGFloat)progress attachmentId:(NSString *)attachmentId From 2ea751bbac0b7a5beaec5bf465ecdc5ee1d771d2 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Sep 2018 13:04:23 -0400 Subject: [PATCH 2/3] Clean up attachment downloads. --- .../Attachments/OWSAttachmentsProcessor.m | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m index ad1587ff0..5282dc1c7 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -192,6 +192,10 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; pointer:attachment success:markAndHandleSuccess failure:markAndHandleFailure]; + + if (![OWSFileSystem deleteFile:encryptedDataFilePath]) { + OWSLogError(@"Could not delete temporary file."); + } } }); } @@ -295,9 +299,17 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; const long kMaxDownloadSize = 150 * 1024 * 1024; __block BOOL hasCheckedContentLength = NO; + NSString *tempFilePath = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; + NSURL *tempFileURL = [NSURL fileURLWithPath:tempFilePath]; + __block NSURLSessionDownloadTask *task; void (^failureHandler)(NSError *) = ^(NSError *error) { OWSLogError(@"Failed to download attachment with error: %@", error.description); + + if (![OWSFileSystem deleteFileIfExists:tempFilePath]) { + OWSLogError(@"Could not delete temporary file."); + } + failureHandlerParam(task, error); }; @@ -308,13 +320,9 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; parameters:nil error:&serializationError]; if (serializationError) { - failureHandler(serializationError); - return; + return failureHandler(serializationError); } - NSString *tempFilePath = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; - NSURL *tempFileURL = [NSURL fileURLWithPath:tempFilePath]; - task = [manager downloadTaskWithRequest:request progress:^(NSProgress *progress) { OWSAssertDebug(progress != nil); @@ -403,6 +411,15 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); return failureHandler(error); } + + // Protect the temporary file. + if (![OWSFileSystem ensureFileExists:filePath.path]) { + OWSLogError(@"Could not protect temporary file."); + NSError *error = OWSErrorWithCodeDescription( + OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return failureHandler(error); + } + NSNumber *_Nullable fileSize = [OWSFileSystem fileSizeOfPath:filePath.path]; if (!fileSize) { OWSLogError(@"Could not determine attachment file size."); From 22afe39cd0d0fc88b48c17f2159b0e29e165878f Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 10 Sep 2018 18:33:52 -0500 Subject: [PATCH 3/3] Respond to CR. --- .../Attachments/OWSAttachmentsProcessor.m | 104 +++++++++++------- 1 file changed, 65 insertions(+), 39 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m index 5282dc1c7..12bb76c77 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -176,28 +176,10 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; [self downloadFromLocation:location pointer:attachment success:^(NSString *encryptedDataFilePath) { - // Use attachmentDecryptSerialQueue to ensure that we only load into memory - // & decrypt a single attachment at a time. - dispatch_async(self.attachmentDecryptSerialQueue, ^{ - @autoreleasepool { - NSData *_Nullable encryptedData = [NSData dataWithContentsOfFile:encryptedDataFilePath]; - if (!encryptedData) { - OWSLogError(@"Could not load encrypted data."); - NSError *error = OWSErrorWithCodeDescription(OWSErrorCodeInvalidMessage, - NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); - return markAndHandleFailure(error); - } - - [self decryptAttachmentData:encryptedData - pointer:attachment - success:markAndHandleSuccess - failure:markAndHandleFailure]; - - if (![OWSFileSystem deleteFile:encryptedDataFilePath]) { - OWSLogError(@"Could not delete temporary file."); - } - } - }); + [self decryptAttachmentPath:encryptedDataFilePath + pointer:attachment + success:markAndHandleSuccess + failure:markAndHandleFailure]; } failure:^(NSURLSessionTask *_Nullable task, NSError *error) { if (attachment.serverId < 100) { @@ -233,6 +215,35 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; }]; } +- (void)decryptAttachmentPath:(NSString *)encryptedDataFilePath + pointer:(TSAttachmentPointer *)attachment + success:(void (^)(TSAttachmentStream *attachmentStream))success + failure:(void (^)(NSError *error))failure +{ + OWSAssertDebug(encryptedDataFilePath.length > 0); + OWSAssertDebug(attachment); + + // Use attachmentDecryptSerialQueue to ensure that we only load into memory + // & decrypt a single attachment at a time. + dispatch_async(self.attachmentDecryptSerialQueue, ^{ + @autoreleasepool { + NSData *_Nullable encryptedData = [NSData dataWithContentsOfFile:encryptedDataFilePath]; + if (!encryptedData) { + OWSLogError(@"Could not load encrypted data."); + NSError *error = OWSErrorWithCodeDescription( + OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return failure(error); + } + + [self decryptAttachmentData:encryptedData pointer:attachment success:success failure:failure]; + + if (![OWSFileSystem deleteFile:encryptedDataFilePath]) { + OWSLogError(@"Could not delete temporary file."); + } + } + }); +} + - (void)decryptAttachmentData:(NSData *)cipherText pointer:(TSAttachmentPointer *)attachment success:(void (^)(TSAttachmentStream *attachmentStream))successHandler @@ -287,32 +298,47 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; success:(void (^)(NSString *encryptedDataPath))successHandler failure:(void (^)(NSURLSessionTask *_Nullable task, NSError *error))failureHandlerParam { - dispatch_queue_t completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); - AFHTTPSessionManager *manager = [AFHTTPSessionManager manager]; manager.requestSerializer = [AFHTTPRequestSerializer serializer]; [manager.requestSerializer setValue:OWSMimeTypeApplicationOctetStream forHTTPHeaderField:@"Content-Type"]; manager.responseSerializer = [AFHTTPResponseSerializer serializer]; - manager.completionQueue = completionQueue; + manager.completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); // We want to avoid large downloads from a compromised or buggy service. const long kMaxDownloadSize = 150 * 1024 * 1024; __block BOOL hasCheckedContentLength = NO; - NSString *tempFilePath = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; - NSURL *tempFileURL = [NSURL fileURLWithPath:tempFilePath]; + NSString *tempSubdirPath = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; + NSString *tempFilePath1 = [tempSubdirPath stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; + NSString *tempFilePath2 = [tempSubdirPath stringByAppendingPathComponent:[NSUUID UUID].UUIDString]; + NSURL *tempFileURL1 = [NSURL fileURLWithPath:tempFilePath1]; __block NSURLSessionDownloadTask *task; void (^failureHandler)(NSError *) = ^(NSError *error) { OWSLogError(@"Failed to download attachment with error: %@", error.description); - if (![OWSFileSystem deleteFileIfExists:tempFilePath]) { - OWSLogError(@"Could not delete temporary file."); + if (![OWSFileSystem deleteFileIfExists:tempFilePath1]) { + OWSLogError(@"Could not delete temporary file #1."); + } + if (![OWSFileSystem deleteFileIfExists:tempFilePath2]) { + OWSLogError(@"Could not delete temporary file #2."); } failureHandlerParam(task, error); }; + // downloadTaskWithRequest's destination callback needs to + // return a path to a non-existent file path, and we can't apply + // file protection to a non-existent file path. + // By creating the temporary file inside a temporary subdirectory, + // we can apply file protection to that subdirectory. + if (![OWSFileSystem ensureDirectoryExists:tempSubdirPath]) { + OWSLogError(@"Could not create temporary subdirectory for attachment download."); + NSError *error = OWSErrorWithCodeDescription( + OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); + return failureHandler(error); + } + NSString *method = @"GET"; NSError *serializationError = nil; NSMutableURLRequest *request = [manager.requestSerializer requestWithMethod:method @@ -398,29 +424,29 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; hasCheckedContentLength = YES; } destination:^(NSURL *targetPath, NSURLResponse *response) { - return tempFileURL; + return tempFileURL1; } completionHandler:^(NSURLResponse *response, NSURL *_Nullable filePath, NSError *_Nullable error) { if (error) { failureHandler(error); return; } - if (![tempFileURL isEqual:filePath]) { + if (![tempFileURL1 isEqual:filePath]) { OWSLogError(@"Unexpected temp file path."); NSError *error = OWSErrorWithCodeDescription( OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); return failureHandler(error); } - // Protect the temporary file. - if (![OWSFileSystem ensureFileExists:filePath.path]) { - OWSLogError(@"Could not protect temporary file."); - NSError *error = OWSErrorWithCodeDescription( - OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); - return failureHandler(error); + // Move the temporary file to a second temporary location + // to ensure that it isn't deleted before we're done with it. + NSError *moveError; + if (![NSFileManager.defaultManager moveItemAtPath:tempFilePath1 toPath:tempFilePath2 error:&moveError]) { + OWSLogError(@"Could not move temporary file."); + return failureHandler(moveError); } - NSNumber *_Nullable fileSize = [OWSFileSystem fileSizeOfPath:filePath.path]; + NSNumber *_Nullable fileSize = [OWSFileSystem fileSizeOfPath:tempFilePath2]; if (!fileSize) { OWSLogError(@"Could not determine attachment file size."); NSError *error = OWSErrorWithCodeDescription( @@ -433,7 +459,7 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; OWSErrorCodeInvalidMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); return failureHandler(error); } - successHandler(filePath.path); + successHandler(tempFilePath2); }]; [task resume]; }