From 259695a9fd206fcfd1ce576651db3f1623e1a140 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 26 Oct 2017 17:25:07 -0700 Subject: [PATCH] Attachments require digest or show explanatory error. // FREEBIE --- Signal/src/views/AttachmentPointerView.swift | 6 ++-- .../translations/en.lproj/Localizable.strings | 3 ++ .../Attachments/OWSAttachmentsProcessor.m | 34 +++++++++++++------ .../Attachments/TSAttachmentPointer.h | 1 + SignalServiceKit/src/Util/Cryptography.h | 7 +++- SignalServiceKit/src/Util/Cryptography.m | 17 +++++++++- 6 files changed, 54 insertions(+), 14 deletions(-) diff --git a/Signal/src/views/AttachmentPointerView.swift b/Signal/src/views/AttachmentPointerView.swift index 14366f444..b42387a1d 100644 --- a/Signal/src/views/AttachmentPointerView.swift +++ b/Signal/src/views/AttachmentPointerView.swift @@ -79,11 +79,11 @@ class AttachmentPointerView: UIView { self.addSubview(progressView) progressView.autoPinWidthToSuperview() progressView.autoPinEdge(.top, to: .bottom, of: nameLabel, withOffset: 6) - progressView.autoSetDimension(.height, toSize: 8) self.addSubview(statusLabel) statusLabel.textAlignment = .center statusLabel.adjustsFontSizeToFitWidth = true + statusLabel.numberOfLines = 2 statusLabel.textColor = self.textColor statusLabel.font = UIFont.ows_footnote() @@ -119,14 +119,16 @@ class AttachmentPointerView: UIView { case .downloading: return NSLocalizedString("ATTACHMENT_DOWNLOADING_STATUS_IN_PROGRESS", comment: "Status label when an attachment is currently downloading") case .failed: - return NSLocalizedString("ATTACHMENT_DOWNLOADING_STATUS_FAILED", comment: "Status label when an attachment download has failed.") + return self.attachmentPointer.mostRecentFailureLocalizedText ?? NSLocalizedString("ATTACHMENT_DOWNLOADING_STATUS_FAILED", comment: "Status label when an attachment download has failed.") } }() if attachmentPointer.state == .downloading { progressView.isHidden = false + progressView.autoSetDimension(.height, toSize: 8) } else { progressView.isHidden = true + progressView.autoSetDimension(.height, toSize: 0) } } diff --git a/Signal/translations/en.lproj/Localizable.strings b/Signal/translations/en.lproj/Localizable.strings index fe45f632f..56993bc22 100644 --- a/Signal/translations/en.lproj/Localizable.strings +++ b/Signal/translations/en.lproj/Localizable.strings @@ -547,6 +547,9 @@ /* Error message when attempting to send message */ "ERROR_DESCRIPTION_UNREGISTERED_RECIPIENT" = "Contact is not a Signal user."; +/* Error message when unable to receive an attachment because the sending client is too old. */ +"ERROR_MESSAGE_ATTACHMENT_FROM_OLD_CLIENT" = "Failure: Ask sender to update Signal and resend."; + /* No comment provided by engineer. */ "ERROR_MESSAGE_DUPLICATE_MESSAGE" = "Received a duplicated message."; diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m index 85f3f89a7..b7962e9fc 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -160,7 +160,7 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; void (^markAndHandleFailure)(NSError *) = ^(NSError *error) { // Ensure enclosing transaction is complete. dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - [self setAttachment:attachment didFailInMessage:message]; + [self setAttachment:attachment didFailInMessage:message error:error]; failureHandler(error); }); }; @@ -246,21 +246,32 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; success:(void (^)(TSAttachmentStream *attachmentStream))successHandler failure:(void (^)(NSError *error))failureHandler { - NSData *plaintext = - [Cryptography decryptAttachment:cipherText withKey:attachment.encryptionKey digest:attachment.digest]; + NSError *decryptError; + NSData *plaintext = [Cryptography decryptAttachment:cipherText + withKey:attachment.encryptionKey + digest:attachment.digest + error:&decryptError]; + + if (decryptError) { + DDLogError(@"%@ failed to decrypt with error: %@", self.tag, decryptError); + failureHandler(decryptError); + return; + } if (!plaintext) { NSError *error = OWSErrorWithCodeDescription(OWSErrorCodeFailedToDecryptMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); - return failureHandler(error); + failureHandler(error); + return; } TSAttachmentStream *stream = [[TSAttachmentStream alloc] initWithPointer:attachment]; - NSError *error; - [stream writeData:plaintext error:&error]; - if (error) { - DDLogError(@"%@ Failed writing attachment stream with error: %@", self.tag, error); - return failureHandler(error); + NSError *writeError; + [stream writeData:plaintext error:&writeError]; + if (writeError) { + DDLogError(@"%@ Failed writing attachment stream with error: %@", self.tag, writeError); + failureHandler(writeError); + return; } [stream save]; @@ -405,8 +416,11 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; } } -- (void)setAttachment:(TSAttachmentPointer *)pointer didFailInMessage:(nullable TSMessage *)message +- (void)setAttachment:(TSAttachmentPointer *)pointer + didFailInMessage:(nullable TSMessage *)message + error:(NSError *)error { + pointer.mostRecentFailureLocalizedText = error.localizedDescription; pointer.state = TSAttachmentPointerStateFailed; [pointer save]; if (message) { diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.h b/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.h index d1b13d74e..07dc290fb 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.h +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.h @@ -30,6 +30,7 @@ typedef NS_ENUM(NSUInteger, TSAttachmentPointerState) { @property (nonatomic, readonly) NSString *relay; @property (atomic) TSAttachmentPointerState state; +@property (nullable, atomic) NSString *mostRecentFailureLocalizedText; // Though now required, `digest` may be null for pre-existing records or from // messages received from other clients diff --git a/SignalServiceKit/src/Util/Cryptography.h b/SignalServiceKit/src/Util/Cryptography.h index 92d145db4..9a35081e8 100755 --- a/SignalServiceKit/src/Util/Cryptography.h +++ b/SignalServiceKit/src/Util/Cryptography.h @@ -54,7 +54,12 @@ typedef NS_ENUM(NSInteger, TSMACType) { + (NSData *)decryptAppleMessagePayload:(NSData *)payload withSignalingKey:(NSString *)signalingKeyString; #pragma mark encrypt and decrypt attachment data -+ (NSData *)decryptAttachment:(NSData *)dataToDecrypt withKey:(NSData *)key digest:(nullable NSData *)digest; + +// Though digest can and will be nil for legacy clients, we now reject attachments lacking a digest. ++ (NSData *)decryptAttachment:(NSData *)dataToDecrypt + withKey:(NSData *)key + digest:(nullable NSData *)digest + error:(NSError **)error; + (NSData *)encryptAttachmentData:(NSData *)attachmentData outKey:(NSData *_Nonnull *_Nullable)outKey diff --git a/SignalServiceKit/src/Util/Cryptography.m b/SignalServiceKit/src/Util/Cryptography.m index 76a05863c..0ec59232b 100755 --- a/SignalServiceKit/src/Util/Cryptography.m +++ b/SignalServiceKit/src/Util/Cryptography.m @@ -5,6 +5,7 @@ #import "Cryptography.h" #import "NSData+Base64.h" #import "NSData+OWSConstantTimeCompare.h" +#import "OWSError.h" #import #import #import @@ -293,11 +294,25 @@ const NSUInteger kAES256_KeyByteLength = 32; digest:nil]; } -+ (NSData *)decryptAttachment:(NSData *)dataToDecrypt withKey:(NSData *)key digest:(nullable NSData *)digest ++ (NSData *)decryptAttachment:(NSData *)dataToDecrypt + withKey:(NSData *)key + digest:(nullable NSData *)digest + error:(NSError **)error; { + if (digest.length <= 0) { + // This *could* happen with sufficiently outdated clients. + DDLogError(@"%@ Refusing to decrypt attachment without a digest.", self.tag); + *error = OWSErrorWithCodeDescription(OWSErrorCodeFailedToDecryptMessage, + NSLocalizedString(@"ERROR_MESSAGE_ATTACHMENT_FROM_OLD_CLIENT", + @"Error message when unable to receive an attachment because the sending client is too old.")); + return nil; + } + if (([dataToDecrypt length] < AES_CBC_IV_LENGTH + HMAC256_OUTPUT_LENGTH) || ([key length] < AES_KEY_SIZE + HMAC256_KEY_LENGTH)) { DDLogError(@"%@ Message shorter than crypto overhead!", self.tag); + *error = OWSErrorWithCodeDescription( + OWSErrorCodeFailedToDecryptMessage, NSLocalizedString(@"ERROR_MESSAGE_INVALID_MESSAGE", @"")); return nil; }