From e715bf9ea2f51e2d0c4549d7b4fa68e442e4d026 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 30 Aug 2018 18:59:26 -0600 Subject: [PATCH 1/2] image sizing Validate image sizing --- Pods | 2 +- .../ConversationView/ConversationViewItem.m | 13 ++ .../GifPicker/GifPickerCell.swift | 5 + .../ViewControllers/MediaMessageView.swift | 8 + .../attachments/SignalAttachment.swift | 5 + SignalMessaging/contacts/OWSContactsManager.m | 2 +- .../Messages/Attachments/TSAttachmentStream.h | 3 +- .../Messages/Attachments/TSAttachmentStream.m | 51 ++++--- SignalServiceKit/src/Util/DataSource.h | 2 + SignalServiceKit/src/Util/DataSource.m | 5 + SignalServiceKit/src/Util/NSData+Image.h | 2 + SignalServiceKit/src/Util/NSData+Image.m | 141 +++++++++++++++++- 12 files changed, 213 insertions(+), 26 deletions(-) diff --git a/Pods b/Pods index 4c3935aa7..9a3f6876d 160000 --- a/Pods +++ b/Pods @@ -1 +1 @@ -Subproject commit 4c3935aa74dfe52f047d664197bbf3f9d7b50c86 +Subproject commit 9a3f6876d4a6086d10501383b96fb2d9d47a75b6 diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m index bef6641b5..cb05b04a3 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m @@ -12,6 +12,7 @@ #import #import #import +#import #import #import @@ -482,8 +483,20 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType) if ([self.attachmentStream isAnimated]) { self.messageCellType = OWSMessageCellType_AnimatedImage; } else if ([self.attachmentStream isImage]) { + if (![self.attachmentStream isValidImage]) { + DDLogWarn(@"Treating invalid image as generic attachment."); + self.messageCellType = OWSMessageCellType_GenericAttachment; + return; + } + self.messageCellType = OWSMessageCellType_StillImage; } else if ([self.attachmentStream isVideo]) { + if (![self.attachmentStream isValidVideo]) { + DDLogWarn(@"Treating invalid video as generic attachment."); + self.messageCellType = OWSMessageCellType_GenericAttachment; + return; + } + self.messageCellType = OWSMessageCellType_Video; } else { OWSFail(@"%@ unexpected attachment type.", self.logTag); diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift b/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift index 29632d8e8..0430a1c3a 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerCell.swift @@ -196,6 +196,11 @@ class GifPickerCell: UICollectionViewCell { clearViewState() return } + guard NSData.ows_isValidImage(atPath: asset.filePath, mimeType: OWSMimeTypeImageGif) else { + owsFail("\(logTag) invalid asset.") + clearViewState() + return + } guard let image = YYImage(contentsOfFile: asset.filePath) else { owsFail("\(logTag) could not load asset.") clearViewState() diff --git a/SignalMessaging/ViewControllers/MediaMessageView.swift b/SignalMessaging/ViewControllers/MediaMessageView.swift index 1953ae024..329501f04 100644 --- a/SignalMessaging/ViewControllers/MediaMessageView.swift +++ b/SignalMessaging/ViewControllers/MediaMessageView.swift @@ -207,6 +207,10 @@ public class MediaMessageView: UIView, OWSAudioPlayerDelegate { } private func createImagePreview() { + guard attachment.isValidImage else { + createGenericPreview() + return + } guard let image = attachment.image() else { createGenericPreview() return @@ -225,6 +229,10 @@ public class MediaMessageView: UIView, OWSAudioPlayerDelegate { } private func createVideoPreview() { + guard attachment.isValidVideo else { + createGenericPreview() + return + } guard let image = attachment.videoPreview() else { createGenericPreview() return diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index 1382e5385..402c320c1 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -141,6 +141,11 @@ public class SignalAttachment: NSObject { return dataSource.isValidImage() } + @objc + public var isValidVideo: Bool { + return dataSource.isValidVideo() + } + // This flag should be set for text attachments that can be sent as text messages. @objc public var isConvertibleToTextMessage = false diff --git a/SignalMessaging/contacts/OWSContactsManager.m b/SignalMessaging/contacts/OWSContactsManager.m index 880b0e6e9..7493590ee 100644 --- a/SignalMessaging/contacts/OWSContactsManager.m +++ b/SignalMessaging/contacts/OWSContactsManager.m @@ -198,7 +198,7 @@ NSString *const OWSContactsManagerKeyNextFullIntersectionDate = @"OWSContactsMan avatarImage = [self.cnContactAvatarCache objectForKey:contactId]; if (!avatarImage) { NSData *_Nullable avatarData = [self avatarDataForCNContactId:contactId]; - if (avatarData) { + if (avatarData && [avatarData ows_isValidImage]) { avatarImage = [UIImage imageWithData:avatarData]; } if (avatarImage) { diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h index 87304fbab..34f73f933 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h @@ -77,9 +77,10 @@ NS_ASSUME_NONNULL_BEGIN // Non-nil for attachments which need "lazy backup restore." - (nullable OWSBackupFragment *)lazyRestoreFragment; -#pragma mark - Image Validation +#pragma mark - Validation - (BOOL)isValidImage; +- (BOOL)isValidVideo; #pragma mark - Update With... Methods diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m index b3bb181af..12aa2f582 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m @@ -14,6 +14,8 @@ NS_ASSUME_NONNULL_BEGIN +const CGFloat kMaxVideoStillSize = 1 * 1024; + @interface TSAttachmentStream () // We only want to generate the file path for this attachment once, so that @@ -315,19 +317,18 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Image Validation -- (BOOL)isValidImageWithData:(NSData *)data +- (BOOL)isValidImage { OWSAssert(self.isImage || self.isAnimated); - OWSAssert(data); - return [data ows_isValidImageWithMimeType:self.contentType]; + return [NSData ows_isValidImageAtPath:self.filePath mimeType:self.contentType]; } -- (BOOL)isValidImage +- (BOOL)isValidVideo { - OWSAssert(self.isImage || self.isAnimated); + OWSAssert(self.isVideo); - return [NSData ows_isValidImageAtPath:self.filePath mimeType:self.contentType]; + return [NSData ows_isValidVideoAtURL:self.mediaURL]; } #pragma mark - @@ -341,11 +342,10 @@ NS_ASSUME_NONNULL_BEGIN if (!mediaUrl) { return nil; } - NSData *data = [NSData dataWithContentsOfURL:mediaUrl]; - if (![self isValidImageWithData:data]) { + if (![self isValidImage]) { return nil; } - return [UIImage imageWithData:data]; + return [[UIImage alloc] initWithContentsOfFile:self.filePath]; } else { return nil; } @@ -362,17 +362,12 @@ NS_ASSUME_NONNULL_BEGIN return nil; } - NSURL *_Nullable mediaUrl = [self mediaURL]; - if (!mediaUrl) { + if (![NSData ows_isValidImageAtPath:self.filePath mimeType:self.contentType]) { + OWSFail(@"%@ skipping invalid image", self.logTag); return nil; } - NSData *data = [NSData dataWithContentsOfURL:mediaUrl]; - if (![self isValidImageWithData:data]) { - return nil; - } - - return data; + return [NSData dataWithContentsOfFile:self.filePath]; } + (BOOL)hasThumbnailForMimeType:(NSString *)contentType @@ -462,6 +457,11 @@ NS_ASSUME_NONNULL_BEGIN CGImageRelease(thumbnail); } else if (self.isVideo) { + if (![self isValidVideo]) { + DDLogWarn(@"%@ skipping thumbnail for invalid video at path: %@", self.logTag, self.filePath); + return; + } + result = [self videoStillImageWithMaxSize:CGSizeMake(thumbnailSize, thumbnailSize)]; } else { OWSFail(@"%@ trying to generate thumnail for unexpected attachment: %@ of type: %@", @@ -471,7 +471,7 @@ NS_ASSUME_NONNULL_BEGIN } if (result == nil) { - OWSFail(@"%@ Unable to build thumbnail for attachmentId: %@", self.logTag, self.uniqueId); + DDLogError(@"Unable to build thumbnail for attachmentId: %@", self.uniqueId); return; } @@ -484,12 +484,18 @@ NS_ASSUME_NONNULL_BEGIN - (nullable UIImage *)videoStillImage { + if (![self isValidVideo]) { + return nil; + } // Uses the assets intrinsic size by default - return [self videoStillImageWithMaxSize:CGSizeZero]; + return [self videoStillImageWithMaxSize:CGSizeMake(kMaxVideoStillSize, kMaxVideoStillSize)]; } - (nullable UIImage *)videoStillImageWithMaxSize:(CGSize)maxSize { + maxSize.width = MIN(maxSize.width, kMaxVideoStillSize); + maxSize.height = MIN(maxSize.height, kMaxVideoStillSize); + NSURL *_Nullable mediaUrl = [self mediaURL]; if (!mediaUrl) { return nil; @@ -502,6 +508,10 @@ NS_ASSUME_NONNULL_BEGIN NSError *err = NULL; CMTime time = CMTimeMake(1, 60); CGImageRef imgRef = [generator copyCGImageAtTime:time actualTime:NULL error:&err]; + if (imgRef == NULL) { + DDLogError(@"Could not generate video still: %@", self.filePath.pathExtension); + return nil; + } return [[UIImage alloc] initWithCGImage:imgRef]; } @@ -531,6 +541,9 @@ NS_ASSUME_NONNULL_BEGIN - (CGSize)calculateImageSize { if ([self isVideo]) { + if (![self isValidVideo]) { + return CGSizeZero; + } return [self videoStillImage].size; } else if ([self isImage] || [self isAnimated]) { NSURL *_Nullable mediaUrl = [self mediaURL]; diff --git a/SignalServiceKit/src/Util/DataSource.h b/SignalServiceKit/src/Util/DataSource.h index 74ba0fc25..ad8e2212a 100755 --- a/SignalServiceKit/src/Util/DataSource.h +++ b/SignalServiceKit/src/Util/DataSource.h @@ -31,6 +31,8 @@ NS_ASSUME_NONNULL_BEGIN - (BOOL)isValidImage; +- (BOOL)isValidVideo; + @end #pragma mark - diff --git a/SignalServiceKit/src/Util/DataSource.m b/SignalServiceKit/src/Util/DataSource.m index 322bf624b..784639091 100755 --- a/SignalServiceKit/src/Util/DataSource.m +++ b/SignalServiceKit/src/Util/DataSource.m @@ -73,6 +73,11 @@ NS_ASSUME_NONNULL_BEGIN return [data ows_isValidImage]; } +- (BOOL)isValidVideo +{ + return [NSData ows_isValidVideoAtURL:self.dataUrl]; +} + - (void)setSourceFilename:(nullable NSString *)sourceFilename { _sourceFilename = sourceFilename.filterFilename; diff --git a/SignalServiceKit/src/Util/NSData+Image.h b/SignalServiceKit/src/Util/NSData+Image.h index 5c86b2bcc..8ddea1190 100644 --- a/SignalServiceKit/src/Util/NSData+Image.h +++ b/SignalServiceKit/src/Util/NSData+Image.h @@ -11,4 +11,6 @@ - (BOOL)ows_isValidImage; - (BOOL)ows_isValidImageWithMimeType:(nullable NSString *)mimeType; ++ (BOOL)ows_isValidVideoAtURL:(NSURL *)url; + @end diff --git a/SignalServiceKit/src/Util/NSData+Image.m b/SignalServiceKit/src/Util/NSData+Image.m index 68e055a36..dea34ea64 100644 --- a/SignalServiceKit/src/Util/NSData+Image.m +++ b/SignalServiceKit/src/Util/NSData+Image.m @@ -4,6 +4,7 @@ #import "NSData+Image.h" #import "MIMETypeUtil.h" +#import typedef NS_ENUM(NSInteger, ImageFormat) { ImageFormat_Unknown, @@ -23,18 +24,127 @@ typedef NS_ENUM(NSInteger, ImageFormat) { - (BOOL)ows_isValidImage { - return [self ows_isValidImageWithMimeType:nil]; + if (![self ows_isValidImageWithMimeType:nil]) { + return NO; + } + + if (![self ows_hasValidImageDimensions]) { + return NO; + } + + return YES; } + (BOOL)ows_isValidImageAtPath:(NSString *)filePath mimeType:(nullable NSString *)mimeType { NSError *error = nil; - NSData *data = [NSData dataWithContentsOfFile:filePath options:NSDataReadingMappedIfSafe error:&error]; - if (error) { + NSData *_Nullable data = [NSData dataWithContentsOfFile:filePath options:NSDataReadingMappedIfSafe error:&error]; + if (!data || error) { DDLogError(@"%@ could not read image data: %@", self.logTag, error); + return NO; } - return [data ows_isValidImageWithMimeType:mimeType]; + if (![data ows_isValidImageWithMimeType:mimeType]) { + return NO; + } + + if (![self ows_hasValidImageDimensionsAtPath:filePath]) { + DDLogError(@"%@ image had invalid dimensions.", self.logTag); + return NO; + } + + return YES; +} + +- (BOOL)ows_hasValidImageDimensions +{ + CGImageSourceRef imageSource = CGImageSourceCreateWithData((__bridge CFDataRef)self, NULL); + if (imageSource == NULL) { + return NO; + } + BOOL result = [NSData ows_hasValidImageDimensionWithImageSource:imageSource]; + CFRelease(imageSource); + return result; +} + ++ (BOOL)ows_hasValidImageDimensionsAtPath:(NSString *)path +{ + NSURL *url = [NSURL fileURLWithPath:path]; + if (!url) { + return NO; + } + + CGImageSourceRef imageSource = CGImageSourceCreateWithURL((__bridge CFURLRef)url, NULL); + if (imageSource == NULL) { + return NO; + } + BOOL result = [self ows_hasValidImageDimensionWithImageSource:imageSource]; + CFRelease(imageSource); + return result; +} + ++ (BOOL)ows_hasValidImageDimensionWithImageSource:(CGImageSourceRef)imageSource +{ + OWSAssert(imageSource); + + NSDictionary *imageProperties + = (__bridge_transfer NSDictionary *)CGImageSourceCopyPropertiesAtIndex(imageSource, 0, NULL); + + if (!imageProperties) { + return NO; + } + + NSNumber *widthNumber = imageProperties[(__bridge NSString *)kCGImagePropertyPixelWidth]; + if (!widthNumber) { + DDLogError(@"widthNumber was unexpectedly nil"); + return NO; + } + CGFloat width = widthNumber.floatValue; + + NSNumber *heightNumber = imageProperties[(__bridge NSString *)kCGImagePropertyPixelHeight]; + if (!heightNumber) { + DDLogError(@"heightNumber was unexpectedly nil"); + return NO; + } + CGFloat height = heightNumber.floatValue; + + /* The number of bits in each color sample of each pixel. The value of this + * key is a CFNumberRef. */ + NSNumber *depthNumber = imageProperties[(__bridge NSString *)kCGImagePropertyDepth]; + if (!depthNumber) { + DDLogError(@"depthNumber was unexpectedly nil"); + return NO; + } + NSUInteger depthBits = depthNumber.unsignedIntegerValue; + CGFloat depthBytes = (CGFloat)ceil(depthBits / 8.f); + + /* The color model of the image such as "RGB", "CMYK", "Gray", or "Lab". + * The value of this key is CFStringRef. */ + NSString *colorModel = imageProperties[(__bridge NSString *)kCGImagePropertyColorModel]; + if (!colorModel) { + DDLogError(@"colorModel was unexpectedly nil"); + return NO; + } + if (![colorModel isEqualToString:(__bridge NSString *)kCGImagePropertyColorModelRGB] + && ![colorModel isEqualToString:(__bridge NSString *)kCGImagePropertyColorModelGray]) { + DDLogError(@"Invalid colorModel: %@", colorModel); + return NO; + } + + // We only support (A)RGB and (A)Grayscale, so worst case is 4. + CGFloat kWorseCastComponentsPerPixel = 4; + CGFloat bytesPerPixel = kWorseCastComponentsPerPixel * depthBytes; + + CGFloat kMaxDimension = 2 * 1024; + CGFloat kExpectedBytePerPixel = 4; + CGFloat kMaxBytes = kMaxDimension * kMaxDimension * kExpectedBytePerPixel; + CGFloat actualBytes = width * height * bytesPerPixel; + if (actualBytes > kMaxBytes) { + DDLogWarn(@"invalid dimensions width: %f, height %f, bytesPerPixel: %f", width, height, bytesPerPixel); + return NO; + } + + return YES; } - (BOOL)ows_isValidImageWithMimeType:(nullable NSString *)mimeType @@ -151,4 +261,27 @@ typedef NS_ENUM(NSInteger, ImageFormat) { return (width > 0 && width < kMaxValidSize && height > 0 && height < kMaxValidSize); } ++ (BOOL)ows_isValidVideoAtURL:(NSURL *)url +{ + OWSAssert(url); + AVURLAsset *asset = [[AVURLAsset alloc] initWithURL:url options:nil]; + + CGSize maxSize = CGSizeZero; + for (AVAssetTrack *track in [asset tracksWithMediaType:AVMediaTypeVideo]) { + CGSize trackSize = track.naturalSize; + maxSize.width = MAX(maxSize.width, trackSize.width); + maxSize.height = MAX(maxSize.height, trackSize.height); + } + if (maxSize.width < 1.f || maxSize.height < 1.f) { + DDLogError(@"Invalid video size: %@", NSStringFromCGSize(maxSize)); + return NO; + } + const CGFloat kMaxSize = 3 * 1024.f; + if (maxSize.width > kMaxSize || maxSize.height > kMaxSize) { + DDLogError(@"Invalid video dimensions: %@", NSStringFromCGSize(maxSize)); + return NO; + } + return YES; +} + @end From 6821e4a3a5b88fb886fbc8add1e95b9aa3bb8de7 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 30 Aug 2018 19:16:33 -0600 Subject: [PATCH 2/2] Don't include invalid media in gallery --- .../src/Storage/OWSMediaGalleryFinder.m | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/SignalServiceKit/src/Storage/OWSMediaGalleryFinder.m b/SignalServiceKit/src/Storage/OWSMediaGalleryFinder.m index cb1ec1ef9..587c56c9a 100644 --- a/SignalServiceKit/src/Storage/OWSMediaGalleryFinder.m +++ b/SignalServiceKit/src/Storage/OWSMediaGalleryFinder.m @@ -164,7 +164,7 @@ static NSString *const OWSMediaGalleryFinderExtensionName = @"OWSMediaGalleryFin YapDatabaseViewOptions *options = [YapDatabaseViewOptions new]; options.allowedCollections = [[YapWhitelistBlacklist alloc] initWithWhitelist:[NSSet setWithObject:TSMessage.collection]]; - return [[YapDatabaseAutoView alloc] initWithGrouping:grouping sorting:sorting versionTag:@"1" options:options]; + return [[YapDatabaseAutoView alloc] initWithGrouping:grouping sorting:sorting versionTag:@"2" options:options]; } + (BOOL)attachmentIdShouldAppearInMediaGallery:(NSString *)attachmentId transaction:(YapDatabaseReadTransaction *)transaction @@ -176,8 +176,20 @@ static NSString *const OWSMediaGalleryFinderExtensionName = @"OWSMediaGalleryFin if (![attachment isKindOfClass:[TSAttachmentStream class]]) { return NO; } - - return attachment.isImage || attachment.isVideo || attachment.isAnimated; + + if (attachment.isImage && attachment.isValidImage) { + return YES; + } + + if (attachment.isVideo && attachment.isValidVideo) { + return YES; + } + + if (attachment.isAnimated && attachment.isValidImage) { + return YES; + } + + return NO; } @end