From 3216fd371469727833bc34e8a626db72c8b096f0 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Sun, 22 Jan 2017 18:09:38 -0500 Subject: [PATCH] Prevent session corruption by using same queue for encrypt vs. decrypt // FREEBIE --- src/Messages/OWSMessageSender.m | 29 ++++++++++++++---------- src/Network/WebSockets/TSSocketManager.m | 23 +++++++++++-------- src/Util/OWSDispatch.h | 15 ++++++++++++ src/Util/OWSDispatch.m | 15 ++++++++++-- 4 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/Messages/OWSMessageSender.m b/src/Messages/OWSMessageSender.m index 8fae6acb0..9ab0934fa 100644 --- a/src/Messages/OWSMessageSender.m +++ b/src/Messages/OWSMessageSender.m @@ -1,5 +1,6 @@ -// Created by Michael Kirk on 10/7/16. -// Copyright © 2016 Open Whisper Systems. All rights reserved. +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// #import "OWSMessageSender.h" #import "ContactsUpdater.h" @@ -628,11 +629,17 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // DEPRECATED - Remove after all clients have been upgraded. BOOL isLegacyMessage = ![message isKindOfClass:[OWSOutgoingSyncMessage class]]; - NSDictionary *messageDict = [self encryptedMessageWithPlaintext:plainText - toRecipient:recipient.uniqueId - deviceId:deviceNumber - keyingStorage:[TSStorageManager sharedManager] - legacy:isLegacyMessage]; + __block NSDictionary *messageDict; + // Mutating session state is not thread safe, so we operate on a serial queue, shared with decryption + // operations. + dispatch_sync([OWSDispatch sessionCipher], ^{ + messageDict = [self encryptedMessageWithPlaintext:plainText + toRecipient:recipient.uniqueId + deviceId:deviceNumber + keyingStorage:[TSStorageManager sharedManager] + legacy:isLegacyMessage]; + }); + if (messageDict) { [messagesArray addObject:messageDict]; } else { @@ -724,11 +731,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; recipientId:identifier deviceId:[deviceNumber intValue]]; - // Mutating session state is not thread safe. - id encryptedMessage; - @synchronized (self) { - encryptedMessage = [cipher encryptMessage:[plainText paddedMessageBody]]; - } + id encryptedMessage = [cipher encryptMessage:[plainText paddedMessageBody]]; + + NSData *serializedMessage = encryptedMessage.serialized; TSWhisperMessageType messageType = [self messageTypeForCipherMessage:encryptedMessage]; diff --git a/src/Network/WebSockets/TSSocketManager.m b/src/Network/WebSockets/TSSocketManager.m index 8f5f139cd..2a55dbf8e 100644 --- a/src/Network/WebSockets/TSSocketManager.m +++ b/src/Network/WebSockets/TSSocketManager.m @@ -5,6 +5,7 @@ #import "SubProtocol.pb.h" #import "Cryptography.h" +#import "OWSDispatch.h" #import "OWSSignalService.h" #import "OWSWebsocketSecurityPolicy.h" #import "TSAccountManager.h" @@ -218,17 +219,21 @@ NSString *const SocketConnectingNotification = @"SocketConnectingNotification"; [self keepAliveBackground]; if ([message.path isEqualToString:@"/api/v1/message"] && [message.verb isEqualToString:@"PUT"]) { - NSData *decryptedPayload = - [Cryptography decryptAppleMessagePayload:message.body withSignalingKey:TSStorageManager.signalingKey]; - - if (!decryptedPayload) { - DDLogWarn(@"Failed to decrypt incoming payload or bad HMAC"); - return; - } + // SessionCipher state is mutated in a couple places, during decryption, in handleReceivedEnvelope. + // To ensure consistent state, that decryption mutation must occur on the same queue as encryption. + dispatch_async([OWSDispatch sessionCipher], ^{ + NSData *decryptedPayload = + [Cryptography decryptAppleMessagePayload:message.body withSignalingKey:TSStorageManager.signalingKey]; + + if (!decryptedPayload) { + DDLogWarn(@"Failed to decrypt incoming payload or bad HMAC"); + return; + } - OWSSignalServiceProtosEnvelope *envelope = [OWSSignalServiceProtosEnvelope parseFromData:decryptedPayload]; + OWSSignalServiceProtosEnvelope *envelope = [OWSSignalServiceProtosEnvelope parseFromData:decryptedPayload]; - [[TSMessagesManager sharedManager] handleReceivedEnvelope:envelope]; + [[TSMessagesManager sharedManager] handleReceivedEnvelope:envelope]; + }); } else { DDLogWarn(@"Unsupported WebSocket Request"); } diff --git a/src/Util/OWSDispatch.h b/src/Util/OWSDispatch.h index 87017bdd9..068379a6c 100644 --- a/src/Util/OWSDispatch.h +++ b/src/Util/OWSDispatch.h @@ -1,10 +1,25 @@ +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// NS_ASSUME_NONNULL_BEGIN @interface OWSDispatch : NSObject +/** + * Attachment downloading + */ + (dispatch_queue_t)attachmentsQueue; +/** + * Signal protocol session state must be coordinated on a serial queue. This is sometimes used synchronously, + * so it's recommended that you avoid dispatching sync *from* this queue to avoid deadlock. + */ ++ (dispatch_queue_t)sessionCipher; + +/** + * Serial message sending queue + */ + (dispatch_queue_t)sendingQueue; @end diff --git a/src/Util/OWSDispatch.m b/src/Util/OWSDispatch.m index 8af8ad720..6d3353100 100644 --- a/src/Util/OWSDispatch.m +++ b/src/Util/OWSDispatch.m @@ -1,5 +1,6 @@ -// Created by Michael Kirk on 10/18/16. -// Copyright © 2016 Open Whisper Systems. All rights reserved. +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// #import "OWSDispatch.h" @@ -17,6 +18,16 @@ NS_ASSUME_NONNULL_BEGIN return queue; } ++ (dispatch_queue_t)sessionCipher +{ + static dispatch_once_t onceToken; + static dispatch_queue_t queue; + dispatch_once(&onceToken, ^{ + queue = dispatch_queue_create("org.whispersystems.signal.sessionCipher", NULL); + }); + return queue; +} + + (dispatch_queue_t)sendingQueue { static dispatch_once_t onceToken;