From 462d4fd09f36dbaece5cab196e50e974a265b7fc Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sat, 21 Sep 2019 01:21:54 -0300 Subject: [PATCH 1/6] Refine image validation. --- SignalServiceKit/src/Util/NSData+Image.m | 181 +++++++++++++++-------- 1 file changed, 116 insertions(+), 65 deletions(-) diff --git a/SignalServiceKit/src/Util/NSData+Image.m b/SignalServiceKit/src/Util/NSData+Image.m index 4120800e5e..6407950855 100644 --- a/SignalServiceKit/src/Util/NSData+Image.m +++ b/SignalServiceKit/src/Util/NSData+Image.m @@ -23,6 +23,29 @@ typedef NS_ENUM(NSInteger, ImageFormat) { ImageFormat_Webp, }; +NSString *NSStringForImageFormat(ImageFormat value) +{ + switch (value) { + case ImageFormat_Unknown: + return @"ImageFormat_Unknown"; + case ImageFormat_Png: + return @"ImageFormat_Png"; + case ImageFormat_Gif: + return @"ImageFormat_Gif"; + case ImageFormat_Tiff: + return @"ImageFormat_Tiff"; + case ImageFormat_Jpeg: + return @"ImageFormat_Jpeg"; + case ImageFormat_Bmp: + return @"ImageFormat_Bmp"; + case ImageFormat_Webp: + return @"ImageFormat_Webp"; + default: + OWSCFailDebug(@"Unknown ImageFormat."); + return @"Unknown"; + } +} + @implementation NSData (Image) + (BOOL)ows_isValidImageAtUrl:(NSURL *)fileUrl mimeType:(nullable NSString *)mimeType @@ -35,11 +58,57 @@ typedef NS_ENUM(NSInteger, ImageFormat) { return [self ows_isValidImageAtPath:filePath mimeType:nil]; } ++ (BOOL)ows_isValidImageAtPath:(NSString *)filePath mimeType:(nullable NSString *)declaredMimeType +{ + NSError *error = nil; + NSData *_Nullable data = [NSData dataWithContentsOfFile:filePath options:NSDataReadingMappedIfSafe error:&error]; + if (!data || error) { + OWSLogError(@"Could not read image data: %@", error); + return NO; + } + return [data ows_isValidImageWithPath:filePath mimeType:declaredMimeType]; +} + - (BOOL)ows_isValidImage +{ + return [self ows_isValidImageWithPath:nil mimeType:nil]; +} + +- (BOOL)ows_isValidImageWithMimeType:(nullable NSString *)declaredMimeType +{ + return [self ows_isValidImageWithPath:nil mimeType:declaredMimeType]; +} + +- (BOOL)ows_isValidImageWithPath:(nullable NSString *)filePath mimeType:(nullable NSString *)declaredMimeType { ImageFormat imageFormat = [self ows_guessImageFormat]; - BOOL isAnimated = imageFormat == ImageFormat_Gif; + if (![self ows_hasValidImageFormat:imageFormat]) { + return NO; + } + + NSString *_Nullable mimeType = [self mimeTypeForImageFormat:imageFormat]; + if (mimeType.length < 1) { + return NO; + } + + if (declaredMimeType.length > 0 && ![self ows_isValidMimeType:declaredMimeType imageFormat:imageFormat]) { + OWSFailDebug(@"Mimetypes do not match: %@, %@", mimeType, declaredMimeType); + // Do not fail in production. + } + + if (filePath.length > 0) { + NSString *fileExtension = [filePath pathExtension].lowercaseString; + NSString *_Nullable mimeTypeForFileExtension = [MIMETypeUtil mimeTypeForFileExtension:fileExtension]; + if (mimeTypeForFileExtension.length > 0 && + [mimeType caseInsensitiveCompare:mimeTypeForFileExtension] != NSOrderedSame) { + OWSFailDebug( + @"fileExtension does not match: %@, %@, %@", fileExtension, mimeType, mimeTypeForFileExtension); + // Do not fail in production. + } + } + + BOOL isAnimated = [self isAnimatedWithImageFormat:imageFormat]; const NSUInteger kMaxFileSize = (isAnimated ? OWSMediaUtils.kMaxFileSizeAnimatedImage : OWSMediaUtils.kMaxFileSizeImage); @@ -49,10 +118,6 @@ typedef NS_ENUM(NSInteger, ImageFormat) { return NO; } - if (![self ows_isValidImageWithMimeType:nil imageFormat:imageFormat]) { - return NO; - } - if (![self ows_hasValidImageDimensionsWithIsAnimated:isAnimated imageFormat:imageFormat]) { return NO; } @@ -60,57 +125,6 @@ typedef NS_ENUM(NSInteger, ImageFormat) { return YES; } -+ (BOOL)ows_isValidImageAtPath:(NSString *)filePath mimeType:(nullable NSString *)mimeType -{ - if (mimeType.length < 1) { - NSString *fileExtension = [filePath pathExtension].lowercaseString; - mimeType = [MIMETypeUtil mimeTypeForFileExtension:fileExtension]; - } - if (mimeType.length < 1) { - OWSLogError(@"Image has unknown MIME type."); - return NO; - } - NSNumber *_Nullable fileSize = [OWSFileSystem fileSizeOfPath:filePath]; - if (!fileSize) { - OWSLogError(@"Could not determine file size."); - return NO; - } - - BOOL isAnimated = [MIMETypeUtil isSupportedAnimatedMIMEType:mimeType]; - if (isAnimated) { - if (fileSize.unsignedIntegerValue > OWSMediaUtils.kMaxFileSizeAnimatedImage) { - OWSLogWarn(@"Oversize animated image."); - return NO; - } - } else if ([MIMETypeUtil isSupportedImageMIMEType:mimeType]) { - if (fileSize.unsignedIntegerValue > OWSMediaUtils.kMaxFileSizeImage) { - OWSLogWarn(@"Oversize still image."); - return NO; - } - } else { - OWSLogError(@"Image has unsupported MIME type."); - return NO; - } - - NSError *error = nil; - NSData *_Nullable data = [NSData dataWithContentsOfFile:filePath options:NSDataReadingMappedIfSafe error:&error]; - if (!data || error) { - OWSLogError(@"Could not read image data: %@", error); - return NO; - } - - if (![data ows_isValidImageWithMimeType:mimeType]) { - return NO; - } - - if (![self ows_hasValidImageDimensionsAtPath:filePath isAnimated:isAnimated]) { - OWSLogError(@"%@ image had invalid dimensions.", self.logTag); - return NO; - } - - return YES; -} - - (BOOL)ows_hasValidImageDimensionsWithIsAnimated:(BOOL)isAnimated imageFormat:(ImageFormat)imageFormat { if (imageFormat == ImageFormat_Webp) { @@ -127,9 +141,11 @@ typedef NS_ENUM(NSInteger, ImageFormat) { return result; } -+ (BOOL)ows_hasValidImageDimensionsAtPath:(NSString *)path isAnimated:(BOOL)isAnimated ++ (BOOL)ows_hasValidImageDimensionsAtPath:(NSString *)path + imageFormat:(ImageFormat)imageFormat + isAnimated:(BOOL)isAnimated { - if ([self isWebpFilePath:path]) { + if (imageFormat == ImageFormat_Webp) { CGSize imageSize = [self sizeForWebpFilePath:path]; return [self ows_isValidImageDimension:imageSize depthBytes:1 isAnimated:YES]; } @@ -227,13 +243,33 @@ typedef NS_ENUM(NSInteger, ImageFormat) { return YES; } -- (BOOL)ows_isValidImageWithMimeType:(nullable NSString *)mimeType +- (nullable NSString *)mimeTypeForImageFormat:(ImageFormat)imageFormat { - ImageFormat imageFormat = [self ows_guessImageFormat]; - return [self ows_isValidImageWithMimeType:mimeType imageFormat:imageFormat]; + switch (imageFormat) { + case ImageFormat_Unknown: + return nil; + case ImageFormat_Png: + return OWSMimeTypeImagePng; + case ImageFormat_Gif: + return OWSMimeTypeImageGif; + case ImageFormat_Tiff: + return OWSMimeTypeImageTiff1; + case ImageFormat_Jpeg: + return OWSMimeTypeImageJpeg; + case ImageFormat_Bmp: + return OWSMimeTypeImageBmp1; + case ImageFormat_Webp: + return OWSMimeTypeImageWebp; + } } -- (BOOL)ows_isValidImageWithMimeType:(nullable NSString *)mimeType imageFormat:(ImageFormat)imageFormat +- (BOOL)isAnimatedWithImageFormat:(ImageFormat)imageFormat +{ + // TODO: ImageFormat_Webp + return imageFormat == ImageFormat_Gif; +} + +- (BOOL)ows_hasValidImageFormat:(ImageFormat)imageFormat { // Don't trust the file extension; iOS (e.g. UIKit, Core Graphics) will happily // load a .gif with a .png file extension. @@ -242,15 +278,30 @@ typedef NS_ENUM(NSInteger, ImageFormat) { // // If the image has a declared MIME type, ensure that agrees with the // deduced image format. + switch (imageFormat) { + case ImageFormat_Unknown: + return NO; + case ImageFormat_Gif: + return [self ows_hasValidGifSize]; + case ImageFormat_Png: + case ImageFormat_Tiff: + case ImageFormat_Jpeg: + case ImageFormat_Bmp: + case ImageFormat_Webp: + return YES; + } +} + +- (BOOL)ows_isValidMimeType:(NSString *)mimeType imageFormat:(ImageFormat)imageFormat +{ + OWSAssertDebug(mimeType.length > 0); + switch (imageFormat) { case ImageFormat_Unknown: return NO; case ImageFormat_Png: return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImagePng]); case ImageFormat_Gif: - if (![self ows_hasValidGifSize]) { - return NO; - } return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageGif]); case ImageFormat_Tiff: return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageTiff1] || From 3e68b55de1b071e921a4aaf158dbd009bd89b668 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sat, 21 Sep 2019 01:31:18 -0300 Subject: [PATCH 2/6] Refine image validation. --- SignalServiceKit/src/Util/NSData+Image.m | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/SignalServiceKit/src/Util/NSData+Image.m b/SignalServiceKit/src/Util/NSData+Image.m index 6407950855..c7f080cc61 100644 --- a/SignalServiceKit/src/Util/NSData+Image.m +++ b/SignalServiceKit/src/Util/NSData+Image.m @@ -79,6 +79,12 @@ NSString *NSStringForImageFormat(ImageFormat value) return [self ows_isValidImageWithPath:nil mimeType:declaredMimeType]; } +// If filePath and/or declaredMimeType is supplied, we warn +// if they do not match the actual file contents. But they are +// both optional, we consider the actual image format (deduced +// using magic numbers) to be authoritative. The file extension +// and declared MIME type could be wrong, but we can proceed in +// that case. - (BOOL)ows_isValidImageWithPath:(nullable NSString *)filePath mimeType:(nullable NSString *)declaredMimeType { ImageFormat imageFormat = [self ows_guessImageFormat]; From df3e6971a25cebf525a5f10c2bddb16a7c4ee192 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sat, 21 Sep 2019 01:50:55 -0300 Subject: [PATCH 3/6] Refine image validation. --- SignalServiceKit/src/Util/NSData+Image.m | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/SignalServiceKit/src/Util/NSData+Image.m b/SignalServiceKit/src/Util/NSData+Image.m index c7f080cc61..d35fa6e24f 100644 --- a/SignalServiceKit/src/Util/NSData+Image.m +++ b/SignalServiceKit/src/Util/NSData+Image.m @@ -298,7 +298,7 @@ NSString *NSStringForImageFormat(ImageFormat value) } } -- (BOOL)ows_isValidMimeType:(NSString *)mimeType imageFormat:(ImageFormat)imageFormat +- (BOOL)ows_isValidMimeType:(nullable NSString *)mimeType imageFormat:(ImageFormat)imageFormat { OWSAssertDebug(mimeType.length > 0); @@ -306,19 +306,19 @@ NSString *NSStringForImageFormat(ImageFormat value) case ImageFormat_Unknown: return NO; case ImageFormat_Png: - return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImagePng]); + return (mimeType == nil || [mimeType caseInsensitiveCompare:OWSMimeTypeImagePng] == NSOrderedSame); case ImageFormat_Gif: - return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageGif]); + return (mimeType == nil || [mimeType caseInsensitiveCompare:OWSMimeTypeImageGif] == NSOrderedSame); case ImageFormat_Tiff: - return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageTiff1] || - [mimeType isEqualToString:OWSMimeTypeImageTiff2]); + return (mimeType == nil || [mimeType caseInsensitiveCompare:OWSMimeTypeImageTiff1] == NSOrderedSame || + [mimeType caseInsensitiveCompare:OWSMimeTypeImageTiff2] == NSOrderedSame); case ImageFormat_Jpeg: - return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageJpeg]); + return (mimeType == nil || [mimeType caseInsensitiveCompare:OWSMimeTypeImageJpeg] == NSOrderedSame); case ImageFormat_Bmp: - return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageBmp1] || - [mimeType isEqualToString:OWSMimeTypeImageBmp2]); + return (mimeType == nil || [mimeType caseInsensitiveCompare:OWSMimeTypeImageBmp1] == NSOrderedSame || + [mimeType caseInsensitiveCompare:OWSMimeTypeImageBmp2] == NSOrderedSame); case ImageFormat_Webp: - return (mimeType == nil || [mimeType isEqualToString:OWSMimeTypeImageWebp]); + return (mimeType == nil || [mimeType caseInsensitiveCompare:OWSMimeTypeImageWebp] == NSOrderedSame); } } From ae8a38dda7fab626896605048c8ecc7dc0d4641c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sat, 21 Sep 2019 01:56:49 -0300 Subject: [PATCH 4/6] Elaborate logging around missing images. --- SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m index 3eadc9c481..3cc5da9255 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m @@ -519,7 +519,8 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); } UIImage *_Nullable image = [[UIImage alloc] initWithContentsOfFile:self.originalFilePath]; if (image == nil) { - OWSFailDebug(@"Couldn't load original image."); + OWSFailDebug( + @"Couldn't load original image: %d.", [OWSFileSystem fileOrFolderExistsAtPath:self.originalFilePath]); } return image; } else { From 8c924fd0a9982606eef11ff48b113ee7383a1407 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sat, 21 Sep 2019 09:18:01 -0300 Subject: [PATCH 5/6] Relax still image restrictions slightly. --- SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift b/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift index 42534fde91..583c71eae9 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift +++ b/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift @@ -136,7 +136,7 @@ public enum OWSMediaError: Error { @objc public static let kMaxFileSizeAnimatedImage = UInt(25 * 1024 * 1024) @objc - public static let kMaxFileSizeImage = UInt(6 * 1024 * 1024) + public static let kMaxFileSizeImage = UInt(8 * 1024 * 1024) @objc public static let kMaxFileSizeVideo = UInt(100 * 1024 * 1024) @objc @@ -149,5 +149,5 @@ public enum OWSMediaError: Error { @objc public static let kMaxAnimatedImageDimensions: UInt = 1 * 1024 @objc - public static let kMaxStillImageDimensions: UInt = 8 * 1024 + public static let kMaxStillImageDimensions: UInt = 12 * 1024 } From d7c577f9e0d344e97290b3acf8e490ed9ba4269d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sat, 21 Sep 2019 09:22:21 -0300 Subject: [PATCH 6/6] Don't abort media transitions if original image can't be loaded. --- .../Transitions/MediaDismissAnimationController.swift | 3 ++- .../Transitions/MediaZoomAnimationController.swift | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Signal/src/ViewControllers/MediaGallery/Transitions/MediaDismissAnimationController.swift b/Signal/src/ViewControllers/MediaGallery/Transitions/MediaDismissAnimationController.swift index 8923b94b52..17bf0b575f 100644 --- a/Signal/src/ViewControllers/MediaGallery/Transitions/MediaDismissAnimationController.swift +++ b/Signal/src/ViewControllers/MediaGallery/Transitions/MediaDismissAnimationController.swift @@ -95,7 +95,8 @@ extension MediaDismissAnimationController: UIViewControllerAnimatedTransitioning guard let presentationImage = galleryItem.attachmentStream.originalImage else { owsFailDebug("presentationImage was unexpectedly nil") - transitionContext.completeTransition(false) + // Complete transition immediately. + transitionContext.completeTransition(true) return } diff --git a/Signal/src/ViewControllers/MediaGallery/Transitions/MediaZoomAnimationController.swift b/Signal/src/ViewControllers/MediaGallery/Transitions/MediaZoomAnimationController.swift index 743567bb2f..0b90184f3a 100644 --- a/Signal/src/ViewControllers/MediaGallery/Transitions/MediaZoomAnimationController.swift +++ b/Signal/src/ViewControllers/MediaGallery/Transitions/MediaZoomAnimationController.swift @@ -76,7 +76,8 @@ extension MediaZoomAnimationController: UIViewControllerAnimatedTransitioning { guard let presentationImage = galleryItem.attachmentStream.originalImage else { owsFailDebug("presentationImage was unexpectedly nil") - transitionContext.completeTransition(false) + // Complete transition immediately. + transitionContext.completeTransition(true) return }