From aff4be51d29c6212b1bba239471089c549911b02 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 26 Aug 2021 22:15:46 -0700 Subject: [PATCH 1/2] Some small sender key changes - Annotate groupIds fetched from the UD messgae content as untrusted - Double check the sender of a failed decryption is a member of the group before inserting a placeholder/error message - Double check group membership hasn't changed before deciding not to expire a sender key. --- .../src/Messages/Interactions/TSErrorMessage.h | 2 +- .../src/Messages/Interactions/TSErrorMessage.m | 14 ++++++++++---- .../src/Messages/OWSMessageDecrypter.swift | 16 ++++++++-------- .../OWSRecoverableDecryptionPlaceholder.h | 2 +- .../OWSRecoverableDecryptionPlaceholder.m | 11 ++++++++--- .../Storage/AxolotlStore/SenderKeyStore.swift | 6 +++++- 6 files changed, 33 insertions(+), 18 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.h b/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.h index b561007fde..db8863aabd 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.h @@ -126,7 +126,7 @@ NS_DESIGNATED_INITIALIZER NS_SWIFT_NAME(init(grdbId:uniqueId:receivedAtTimestamp wasIdentityVerified:(BOOL)wasIdentityVerified; + (instancetype)failedDecryptionForEnvelope:(SSKProtoEnvelope *)envelope - groupId:(nullable NSData *)groupId + untrustedGroupId:(nullable NSData *)untrustedGroupId withTransaction:(SDSAnyWriteTransaction *)transaction; + (instancetype)failedDecryptionForSender:(nullable SignalServiceAddress *)sender diff --git a/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m b/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m index 1bf73eee71..dd41e64289 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m @@ -314,7 +314,7 @@ NSUInteger TSErrorMessageSchemaVersion = 2; } + (instancetype)failedDecryptionForEnvelope:(SSKProtoEnvelope *)envelope - groupId:(nullable NSData *)groupId + untrustedGroupId:(nullable NSData *)untrustedGroupId withTransaction:(SDSAnyWriteTransaction *)transaction { SignalServiceAddress *sender = [[SignalServiceAddress alloc] initWithUuidString:envelope.sourceUuid]; @@ -323,9 +323,15 @@ NSUInteger TSErrorMessageSchemaVersion = 2; return nil; } - TSThread *thread; - if (groupId.length > 0) { - thread = [TSGroupThread fetchWithGroupId:groupId transaction:transaction]; + TSThread *_Nullable thread = nil; + if (untrustedGroupId.length > 0) { + TSGroupThread *_Nullable groupThread = [TSGroupThread fetchWithGroupId:untrustedGroupId + transaction:transaction]; + // If we aren't sure that the sender is a member of the reported grouupId, we should fall back + // to inserting the placeholder in the contact thread. + if ([groupThread.groupMembership isFullMember:sender]) { + thread = groupThread; + } OWSAssertDebug(thread); } if (!thread) { diff --git a/SignalServiceKit/src/Messages/OWSMessageDecrypter.swift b/SignalServiceKit/src/Messages/OWSMessageDecrypter.swift index e3df401f2b..8104270e7b 100644 --- a/SignalServiceKit/src/Messages/OWSMessageDecrypter.swift +++ b/SignalServiceKit/src/Messages/OWSMessageDecrypter.swift @@ -285,7 +285,7 @@ public class OWSMessageDecrypter: OWSMessageHandler { private func processError( _ error: Error, envelope: SSKProtoEnvelope, - groupId: Data?, + untrustedGroupId: Data?, cipherType: CiphertextMessage.MessageType, contentHint: SealedSenderContentHint, transaction: SDSAnyWriteTransaction @@ -346,13 +346,13 @@ public class OWSMessageDecrypter: OWSMessageHandler { // If default, insert an error message right away errorMessage = TSErrorMessage.failedDecryption( for: envelope, - groupId: groupId, + untrustedGroupId: untrustedGroupId, with: transaction) case .resendable: // If resendable, insert a placeholder errorMessage = OWSRecoverableDecryptionPlaceholder( failedEnvelope: envelope, - groupId: groupId, + untrustedGroupId: untrustedGroupId, transaction: transaction) case .implicit: errorMessage = nil @@ -587,7 +587,7 @@ public class OWSMessageDecrypter: OWSMessageHandler { let wrappedError = processError( error, envelope: envelope, - groupId: nil, + untrustedGroupId: nil, cipherType: cipherType, contentHint: .default, transaction: transaction @@ -691,7 +691,7 @@ public class OWSMessageDecrypter: OWSMessageHandler { return .failure(handleUnidentifiedSenderDecryptionError( error: outerError.underlyingError, envelope: envelope.buildIdentifiedCopy(using: outerError), - groupId: outerError.groupId, + untrustedGroupId: outerError.groupId, cipherType: outerError.cipherType, contentHint: SealedSenderContentHint(outerError.contentHint), transaction: transaction) @@ -700,7 +700,7 @@ public class OWSMessageDecrypter: OWSMessageHandler { return .failure(handleUnidentifiedSenderDecryptionError( error: error, envelope: envelope, - groupId: nil, + untrustedGroupId: nil, cipherType: .plaintext, contentHint: .default, transaction: transaction) @@ -760,7 +760,7 @@ public class OWSMessageDecrypter: OWSMessageHandler { func handleUnidentifiedSenderDecryptionError( error: Error, envelope: SSKProtoEnvelope, - groupId: Data?, + untrustedGroupId: Data?, cipherType: CiphertextMessage.MessageType, contentHint: SealedSenderContentHint, transaction: SDSAnyWriteTransaction @@ -771,7 +771,7 @@ public class OWSMessageDecrypter: OWSMessageHandler { } else if isSignalClientError(error) { return processError(error, envelope: envelope, - groupId: groupId, + untrustedGroupId: untrustedGroupId, cipherType: cipherType, contentHint: contentHint, transaction: transaction) diff --git a/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.h b/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.h index d08b1614c1..aaea6374a6 100644 --- a/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.h +++ b/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.h @@ -10,7 +10,7 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initErrorMessageWithBuilder:(TSErrorMessageBuilder *)errorMessageBuilder NS_UNAVAILABLE; - (nullable instancetype)initWithFailedEnvelope:(SSKProtoEnvelope *)envelope - groupId:(nullable NSData *)groupId + untrustedGroupId:(nullable NSData *)untrustedGroupId transaction:(SDSAnyWriteTransaction *)writeTx NS_DESIGNATED_INITIALIZER; - (nullable instancetype)initWithCoder:(NSCoder *)coder NS_DESIGNATED_INITIALIZER; diff --git a/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m b/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m index 4995f7860f..2f9d595ac3 100644 --- a/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m +++ b/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m @@ -11,7 +11,7 @@ NS_ASSUME_NONNULL_BEGIN @implementation OWSRecoverableDecryptionPlaceholder - (nullable instancetype)initWithFailedEnvelope:(SSKProtoEnvelope *)envelope - groupId:(nullable NSData *)groupId + untrustedGroupId:(nullable NSData *)untrustedGroupId transaction:(SDSAnyWriteTransaction *)writeTx { SignalServiceAddress *sender = [[SignalServiceAddress alloc] initWithUuidString:envelope.sourceUuid]; @@ -21,8 +21,13 @@ NS_ASSUME_NONNULL_BEGIN } TSThread *thread; - if (groupId.length > 0) { - thread = [TSGroupThread fetchWithGroupId:groupId transaction:writeTx]; + if (untrustedGroupId.length > 0) { + TSGroupThread *_Nullable groupThread = [TSGroupThread fetchWithGroupId:untrustedGroupId transaction:writeTx]; + // If we aren't sure that the sender is a member of the reported grouupId, we should fall back + // to inserting the placeholder in the contact thread. + if ([groupThread.groupMembership isFullMember:sender]) { + thread = groupThread; + } OWSAssertDebug(thread); } if (!thread) { diff --git a/SignalServiceKit/src/Storage/AxolotlStore/SenderKeyStore.swift b/SignalServiceKit/src/Storage/AxolotlStore/SenderKeyStore.swift index 80c9052500..c8aff520d8 100644 --- a/SignalServiceKit/src/Storage/AxolotlStore/SenderKeyStore.swift +++ b/SignalServiceKit/src/Storage/AxolotlStore/SenderKeyStore.swift @@ -103,7 +103,11 @@ public class SenderKeyStore: NSObject { let distributionId = distributionIdForSendingToThreadId(thread.uniqueId, writeTx: writeTx) guard let keyMetadata = getKeyMetadata(for: distributionId, readTx: writeTx) else { return } - if !keyMetadata.isValid { + let currentKeyRecipients = Set(keyMetadata.keyRecipients.keys) + let currentGroupMembership = thread.groupMembership.fullMembers + let didSomeoneLeaveTheGroup = currentKeyRecipients.subtracting(currentGroupMembership).count > 0 + + if !keyMetadata.isValid || didSomeoneLeaveTheGroup { setMetadata(nil, for: distributionId, writeTx: writeTx) } } From d3ff346af95cb6f053203db3eb5891291235389a Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Fri, 27 Aug 2021 16:07:52 -0700 Subject: [PATCH 2/2] PR Feedback --- SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m | 3 ++- .../src/Messages/OWSRecoverableDecryptionPlaceholder.m | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m b/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m index dd41e64289..162990675b 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSErrorMessage.m @@ -325,9 +325,10 @@ NSUInteger TSErrorMessageSchemaVersion = 2; TSThread *_Nullable thread = nil; if (untrustedGroupId.length > 0) { + [TSGroupThread ensureGroupIdMappingForGroupId:untrustedGroupId transaction:writeTx]; TSGroupThread *_Nullable groupThread = [TSGroupThread fetchWithGroupId:untrustedGroupId transaction:transaction]; - // If we aren't sure that the sender is a member of the reported grouupId, we should fall back + // If we aren't sure that the sender is a member of the reported groupId, we should fall back // to inserting the placeholder in the contact thread. if ([groupThread.groupMembership isFullMember:sender]) { thread = groupThread; diff --git a/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m b/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m index 2f9d595ac3..0e4f52023a 100644 --- a/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m +++ b/SignalServiceKit/src/Messages/OWSRecoverableDecryptionPlaceholder.m @@ -22,8 +22,9 @@ NS_ASSUME_NONNULL_BEGIN TSThread *thread; if (untrustedGroupId.length > 0) { + [TSGroupThread ensureGroupIdMappingForGroupId:untrustedGroupId transaction:writeTx]; TSGroupThread *_Nullable groupThread = [TSGroupThread fetchWithGroupId:untrustedGroupId transaction:writeTx]; - // If we aren't sure that the sender is a member of the reported grouupId, we should fall back + // If we aren't sure that the sender is a member of the reported groupId, we should fall back // to inserting the placeholder in the contact thread. if ([groupThread.groupMembership isFullMember:sender]) { thread = groupThread;