From 2c61943537c5c8de986e44e7f2ea0616cdaaa46a Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 20 Mar 2017 18:16:37 -0400 Subject: [PATCH 1/3] Abort attachment downloads of excessive size. // FREEBIE --- .../Attachments/OWSAttachmentsProcessor.m | 72 +++++++++++++++---- 1 file changed, 57 insertions(+), 15 deletions(-) diff --git a/src/Messages/Attachments/OWSAttachmentsProcessor.m b/src/Messages/Attachments/OWSAttachmentsProcessor.m index 2d59730df..917ae8c28 100644 --- a/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -233,22 +233,64 @@ NS_ASSUME_NONNULL_BEGIN manager.responseSerializer = [AFHTTPResponseSerializer serializer]; manager.completionQueue = dispatch_get_main_queue(); + // 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. - [manager GET:location - parameters:nil - progress:nil // TODO show some progress! - success:^(NSURLSessionDataTask *_Nonnull task, id _Nullable responseObject) { - if (![responseObject isKindOfClass:[NSData class]]) { - DDLogError(@"%@ Failed retrieval of attachment. Response had unexpected format.", self.tag); - NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(task, error); - } - successHandler((NSData *)responseObject); - } - failure:^(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull error) { - DDLogError(@"Failed to retrieve attachment with error: %@", error.description); - return failureHandler(task, error); - }]; + __block NSURLSessionDataTask *task = nil; + // We only need to check the content length header once. + __block BOOL hasCheckedContentLength = NO; + task = [manager GET:location + parameters:nil + progress:^(NSProgress *_Nonnull progress) { + if (!hasCheckedContentLength) { + NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)task.response; + if ([httpResponse isKindOfClass:[NSHTTPURLResponse class]]) { + NSDictionary *headers = [httpResponse allHeaderFields]; + if ([headers isKindOfClass:[NSDictionary class]]) { + NSString *contentLength = headers[@"Content-Length"]; + if ([contentLength isKindOfClass:[NSString class]]) { + if (contentLength.longLongValue > 0) { + if (contentLength.longLongValue <= kMaxDownloadSize) { + // This response has a valid content length that is less + // than our max download size. Proceed with the download. + hasCheckedContentLength = YES; + return; + } + } + } + } + } + // If the task doesn't exist, or doesn't have a response, or is missing + // the expected headers, or has an invalid or oversize content length, etc., + // abort the download. + OWSAssert(0); + [task cancel]; + } else { + OWSAssert(progress != nil); + + if (progress.totalUnitCount > kMaxDownloadSize || progress.completedUnitCount > kMaxDownloadSize) { + // A malicious service might send an incorrect content length header, + // so.... + // + // If the current downloaded bytes or the expected total byes + // exceed the max download size, abort the download. + OWSAssert(0); + [task cancel]; + } + } + } + success:^(NSURLSessionDataTask *_Nonnull task, id _Nullable responseObject) { + if (![responseObject isKindOfClass:[NSData class]]) { + DDLogError(@"%@ Failed retrieval of attachment. Response had unexpected format.", self.tag); + NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); + return failureHandler(task, error); + } + successHandler((NSData *)responseObject); + } + failure:^(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull error) { + DDLogError(@"Failed to retrieve attachment with error: %@", error.description); + return failureHandler(task, error); + }]; } - (void)setAttachment:(TSAttachmentPointer *)pointer isDownloadingInMessage:(nullable TSMessage *)message From 8231f79977606d3249cbeb124a92e7347fe6f64c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 21 Mar 2017 09:49:51 -0400 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20check=20content=20length=20?= =?UTF-8?q?header=20until=20we=E2=80=99ve=20received=20at=20least=20one=20?= =?UTF-8?q?byte=20of=20the=20attachment=20download.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit // FREEBIE --- src/Messages/Attachments/OWSAttachmentsProcessor.m | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Messages/Attachments/OWSAttachmentsProcessor.m b/src/Messages/Attachments/OWSAttachmentsProcessor.m index 917ae8c28..577a82741 100644 --- a/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -242,7 +242,9 @@ NS_ASSUME_NONNULL_BEGIN task = [manager GET:location parameters:nil progress:^(NSProgress *_Nonnull progress) { - if (!hasCheckedContentLength) { + // Once we've received some bytes of the download, check the content length + // header for the download. + if (progress.completedUnitCount > 0 && !hasCheckedContentLength) { NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)task.response; if ([httpResponse isKindOfClass:[NSHTTPURLResponse class]]) { NSDictionary *headers = [httpResponse allHeaderFields]; @@ -269,7 +271,7 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(progress != nil); if (progress.totalUnitCount > kMaxDownloadSize || progress.completedUnitCount > kMaxDownloadSize) { - // A malicious service might send an incorrect content length header, + // A malicious service might send a misleading content length header, // so.... // // If the current downloaded bytes or the expected total byes From 04a3e4323c23296b08f23906462c7bdce2334541 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 21 Mar 2017 13:00:48 -0400 Subject: [PATCH 3/3] Respond to CR. // FREEBIE --- .../Attachments/OWSAttachmentsProcessor.m | 106 ++++++++++++------ 1 file changed, 70 insertions(+), 36 deletions(-) diff --git a/src/Messages/Attachments/OWSAttachmentsProcessor.m b/src/Messages/Attachments/OWSAttachmentsProcessor.m index 577a82741..3ca0748c6 100644 --- a/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -237,49 +237,83 @@ NS_ASSUME_NONNULL_BEGIN const long kMaxDownloadSize = 150 * 1024 * 1024; // TODO stream this download rather than storing the entire blob. __block NSURLSessionDataTask *task = nil; - // We only need to check the content length header once. __block BOOL hasCheckedContentLength = NO; task = [manager GET:location parameters:nil progress:^(NSProgress *_Nonnull progress) { - // Once we've received some bytes of the download, check the content length - // header for the download. - if (progress.completedUnitCount > 0 && !hasCheckedContentLength) { - NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)task.response; - if ([httpResponse isKindOfClass:[NSHTTPURLResponse class]]) { - NSDictionary *headers = [httpResponse allHeaderFields]; - if ([headers isKindOfClass:[NSDictionary class]]) { - NSString *contentLength = headers[@"Content-Length"]; - if ([contentLength isKindOfClass:[NSString class]]) { - if (contentLength.longLongValue > 0) { - if (contentLength.longLongValue <= kMaxDownloadSize) { - // This response has a valid content length that is less - // than our max download size. Proceed with the download. - hasCheckedContentLength = YES; - return; - } - } - } - } - } - // If the task doesn't exist, or doesn't have a response, or is missing - // the expected headers, or has an invalid or oversize content length, etc., - // abort the download. + OWSAssert(progress != nil); + + // Don't do anything until we've received at least one byte of data. + if (progress.completedUnitCount < 1) { + return; + } + + void (^abortDownload)() = ^{ OWSAssert(0); [task cancel]; - } else { - OWSAssert(progress != nil); - - if (progress.totalUnitCount > kMaxDownloadSize || progress.completedUnitCount > kMaxDownloadSize) { - // A malicious service might send a misleading content length header, - // so.... - // - // If the current downloaded bytes or the expected total byes - // exceed the max download size, abort the download. - OWSAssert(0); - [task cancel]; - } + }; + + if (progress.totalUnitCount > kMaxDownloadSize || progress.completedUnitCount > kMaxDownloadSize) { + // A malicious service might send a misleading content length header, + // so.... + // + // If the current downloaded bytes or the expected total byes + // exceed the max download size, abort the download. + DDLogError(@"%@ Attachment download exceed expected content length: %lld, %lld.", + self.tag, + (long long) progress.totalUnitCount, + (long long) progress.completedUnitCount); + abortDownload(); + return; + } + + // We only need to check the content length header once. + if (hasCheckedContentLength) { + return; + } + + // Once we've received some bytes of the download, check the content length + // header for the download. + // + // If the task doesn't exist, or doesn't have a response, or is missing + // the expected headers, or has an invalid or oversize content length, etc., + // abort the download. + NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)task.response; + if (![httpResponse isKindOfClass:[NSHTTPURLResponse class]]) { + DDLogError(@"%@ Attachment download has missing or invalid response.", + self.tag); + abortDownload(); + return; + } + + NSDictionary *headers = [httpResponse allHeaderFields]; + if (![headers isKindOfClass:[NSDictionary class]]) { + DDLogError(@"%@ Attachment download invalid headers.", + self.tag); + abortDownload(); + return; + } + + + NSString *contentLength = headers[@"Content-Length"]; + if (![contentLength isKindOfClass:[NSString class]]) { + DDLogError(@"%@ Attachment download missing or invalid content length.", + self.tag); + abortDownload(); + return; + } + + + if (contentLength.longLongValue > kMaxDownloadSize) { + DDLogError(@"%@ Attachment download content length exceeds max download size.", + self.tag); + 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 *_Nonnull task, id _Nullable responseObject) { if (![responseObject isKindOfClass:[NSData class]]) {