From 0913b84657deeb154ccf8e6d96302441fdf142ef Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Tue, 26 Nov 2024 12:24:47 -0500 Subject: [PATCH] Verify group ids on peer-to-peer group changes. --- app/src/main/AndroidManifest.xml | 1 + .../securesms/groups/GroupManagerV2.java | 12 +++--- .../groupsv2/DecryptChangeVerificationMode.kt | 39 +++++++++++++++++++ .../api/groupsv2/GroupsV2Api.java | 2 +- .../api/groupsv2/GroupsV2Operations.java | 22 +++++++---- .../src/main/protowire/DecryptedGroups.proto | 1 + .../src/main/protowire/Groups.proto | 1 + .../GroupChangeUtil_changeIsEmpty_Test.java | 2 +- ...roupsV2Operations_decrypt_change_Test.java | 8 ++-- 9 files changed, 69 insertions(+), 19 deletions(-) create mode 100644 libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptChangeVerificationMode.kt diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 1e65ade2ca..b33459568e 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -90,6 +90,7 @@ + diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java index 7ca5faeb4c..26957f44a3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupManagerV2.java @@ -50,8 +50,8 @@ import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.recipients.RecipientId; import org.thoughtcrime.securesms.sms.MessageSender; import org.thoughtcrime.securesms.util.ProfileUtil; +import org.whispersystems.signalservice.api.groupsv2.DecryptChangeVerificationMode; import org.whispersystems.signalservice.api.groupsv2.DecryptedGroupResponse; -import org.whispersystems.signalservice.api.groupsv2.ReceivedGroupSendEndorsements; import org.whispersystems.signalservice.api.groupsv2.DecryptedGroupUtil; import org.whispersystems.signalservice.api.groupsv2.GroupCandidate; import org.whispersystems.signalservice.api.groupsv2.GroupChangeReconstruct; @@ -61,6 +61,7 @@ import org.whispersystems.signalservice.api.groupsv2.GroupsV2Api; import org.whispersystems.signalservice.api.groupsv2.GroupsV2Operations; import org.whispersystems.signalservice.api.groupsv2.InvalidGroupStateException; import org.whispersystems.signalservice.api.groupsv2.NotAbleToApplyGroupV2ChangeException; +import org.whispersystems.signalservice.api.groupsv2.ReceivedGroupSendEndorsements; import org.whispersystems.signalservice.api.push.ServiceId; import org.whispersystems.signalservice.api.push.ServiceId.ACI; import org.whispersystems.signalservice.api.push.ServiceId.PNI; @@ -678,8 +679,7 @@ final class GroupManagerV2 { GroupChangeResponse changeResponse = commitToServer(changeActions); GroupChange signedGroupChange = changeResponse.groupChange; try { - //noinspection OptionalGetWithoutIsPresent - decryptedChange = groupOperations.decryptChange(signedGroupChange, false).get(); + decryptedChange = groupOperations.decryptChange(signedGroupChange, DecryptChangeVerificationMode.alreadyTrusted()).get(); decryptedGroupState = DecryptedGroupUtil.apply(previousGroupState, decryptedChange); } catch (VerificationFailedException | InvalidGroupStateException | NotAbleToApplyGroupV2ChangeException e) { Log.w(TAG, e); @@ -766,7 +766,7 @@ final class GroupManagerV2 { GroupsV2Operations.GroupOperations groupOperations = groupsV2Operations.forGroup(GroupSecretParams.deriveFromMasterKey(groupMasterKey)); try { - return groupOperations.decryptChange(GroupChange.ADAPTER.decode(signedGroupChange), true) + return groupOperations.decryptChange(GroupChange.ADAPTER.decode(signedGroupChange), DecryptChangeVerificationMode.verify(GroupId.getIdentifierForMasterKey(groupMasterKey))) .orElse(null); } catch (VerificationFailedException | InvalidGroupStateException | IOException e) { Log.w(TAG, "Unable to verify supplied group change", e); @@ -989,7 +989,7 @@ final class GroupManagerV2 { { try { //noinspection OptionalGetWithoutIsPresent - return groupOperations.decryptChange(signedGroupChange, false).get(); + return groupOperations.decryptChange(signedGroupChange, DecryptChangeVerificationMode.alreadyTrusted()).get(); } catch (VerificationFailedException | InvalidGroupStateException | IOException e) { Log.w(TAG, e); throw new GroupChangeFailedException(e); @@ -1159,7 +1159,7 @@ final class GroupManagerV2 { try { //noinspection OptionalGetWithoutIsPresent - DecryptedGroupChange decryptedChange = groupOperations.decryptChange(signedGroupChange, false).get(); + DecryptedGroupChange decryptedChange = groupOperations.decryptChange(signedGroupChange, DecryptChangeVerificationMode.alreadyTrusted()).get(); DecryptedGroup newGroup = DecryptedGroupUtil.applyWithoutRevisionCheck(decryptedGroup, decryptedChange); groupDatabase.update(groupId, resetRevision(newGroup, decryptedGroup.revision), null); diff --git a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptChangeVerificationMode.kt b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptChangeVerificationMode.kt new file mode 100644 index 0000000000..2fc1b5840e --- /dev/null +++ b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/DecryptChangeVerificationMode.kt @@ -0,0 +1,39 @@ +/* + * Copyright 2024 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.signalservice.api.groupsv2 + +import org.signal.libsignal.zkgroup.groups.GroupIdentifier + +/** + * Details what verification should take place when decrypting a group change. + * + * @param verify Should perform group change verification during decryption + * @param groupId The id this change should apply to and will be verified is set in change payload + */ +class DecryptChangeVerificationMode private constructor( + @get:JvmName("verify") val verify: Boolean, + @get:JvmName("groupId") val groupId: GroupIdentifier? +) { + companion object { + + /** + * Use when the changes are already trusted. This would be during group creation or when fetching + * group changes directly from the server. + */ + @JvmStatic + @get:JvmName("alreadyTrusted") + val alreadyTrusted by lazy { DecryptChangeVerificationMode(verify = false, groupId = null) } + + /** + * Use when the changes are from an untrusted source like a P2P message of a group change. This + * will verify the signature and group id match. + */ + @JvmStatic + fun verify(groupId: GroupIdentifier): DecryptChangeVerificationMode { + return DecryptChangeVerificationMode(verify = true, groupId = groupId) + } + } +} diff --git a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Api.java b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Api.java index 0ce4797ccb..40f7726311 100644 --- a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Api.java +++ b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Api.java @@ -124,7 +124,7 @@ public class GroupsV2Api { for (GroupChanges.GroupChangeState change : group.getGroupChanges().groupChanges) { DecryptedGroup decryptedGroup = change.groupState != null ? groupOperations.decryptGroup(change.groupState) : null; - DecryptedGroupChange decryptedChange = change.groupChange != null ? groupOperations.decryptChange(change.groupChange, false).orElse(null) : null; + DecryptedGroupChange decryptedChange = change.groupChange != null ? groupOperations.decryptChange(change.groupChange, DecryptChangeVerificationMode.alreadyTrusted()).orElse(null) : null; result.add(new DecryptedGroupChangeLog(decryptedGroup, decryptedChange)); } diff --git a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java index 4019528390..0f71acf647 100644 --- a/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java +++ b/libsignal-service/src/main/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations.java @@ -7,6 +7,7 @@ import org.signal.libsignal.zkgroup.ServerPublicParams; import org.signal.libsignal.zkgroup.VerificationFailedException; import org.signal.libsignal.zkgroup.auth.ClientZkAuthOperations; import org.signal.libsignal.zkgroup.groups.ClientZkGroupCipher; +import org.signal.libsignal.zkgroup.groups.GroupIdentifier; import org.signal.libsignal.zkgroup.groups.GroupSecretParams; import org.signal.libsignal.zkgroup.groups.ProfileKeyCiphertext; import org.signal.libsignal.zkgroup.groups.UuidCiphertext; @@ -45,6 +46,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil; import java.io.IOException; import java.security.SecureRandom; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -486,14 +488,13 @@ public final class GroupsV2Operations { } /** - * @param verifySignature You might want to avoid verification if you already know it's correct, or you - * are not going to pass to other clients. - *

- * Also, if you know it's version 0, do not verify because changes for version 0 - * are not signed, but should be empty. + * @param verification You might want to avoid verification if you already know it's correct, or you are not going to pass to other clients. + *

+ * Also, if you know it's version 0, do not verify because changes for version 0 are not signed, but should be empty. + * * @return {@link Optional#empty()} if the epoch for the change is higher that this code can decrypt. */ - public Optional decryptChange(GroupChange groupChange, boolean verifySignature) + public Optional decryptChange(GroupChange groupChange, @Nonnull DecryptChangeVerificationMode verification) throws IOException, VerificationFailedException, InvalidGroupStateException { if (groupChange.changeEpoch > HIGHEST_KNOWN_EPOCH) { @@ -501,7 +502,14 @@ public final class GroupsV2Operations { return Optional.empty(); } - GroupChange.Actions actions = verifySignature ? getVerifiedActions(groupChange) : getActions(groupChange); + GroupChange.Actions actions = verification.verify() ? getVerifiedActions(groupChange) : getActions(groupChange); + + if (verification.verify()) { + GroupIdentifier groupId = verification.groupId(); + if (groupId == null || !Arrays.equals(groupId.serialize(), actions.groupId.toByteArray())) { + throw new VerificationFailedException("Invalid group id"); + } + } return Optional.of(decryptChange(actions)); } diff --git a/libsignal-service/src/main/protowire/DecryptedGroups.proto b/libsignal-service/src/main/protowire/DecryptedGroups.proto index fb8263ecb3..86b0595438 100644 --- a/libsignal-service/src/main/protowire/DecryptedGroups.proto +++ b/libsignal-service/src/main/protowire/DecryptedGroups.proto @@ -75,6 +75,7 @@ message DecryptedGroup { // Keep field numbers in step message DecryptedGroupChange { bytes editorServiceIdBytes = 1; + reserved 25; // groupId used only during verification + decrypt, only provided by server uint32 revision = 2; repeated DecryptedMember newMembers = 3; repeated bytes deleteMembers = 4; diff --git a/libsignal-service/src/main/protowire/Groups.proto b/libsignal-service/src/main/protowire/Groups.proto index 63970c7741..e202eb2aaf 100644 --- a/libsignal-service/src/main/protowire/Groups.proto +++ b/libsignal-service/src/main/protowire/Groups.proto @@ -183,6 +183,7 @@ message GroupChange { } bytes sourceServiceId = 1; + bytes groupId = 25; // Only set when receiving from server uint32 revision = 2; repeated AddMemberAction addMembers = 3; repeated DeleteMemberAction deleteMembers = 4; diff --git a/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupChangeUtil_changeIsEmpty_Test.java b/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupChangeUtil_changeIsEmpty_Test.java index 24ff9e4f9a..7a58d848ca 100644 --- a/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupChangeUtil_changeIsEmpty_Test.java +++ b/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupChangeUtil_changeIsEmpty_Test.java @@ -22,7 +22,7 @@ public final class GroupChangeUtil_changeIsEmpty_Test { int maxFieldFound = getMaxDeclaredFieldNumber(GroupChange.Actions.class); assertEquals("GroupChangeUtil and its tests need updating to account for new fields on " + GroupChange.Actions.class.getName(), - 24, maxFieldFound); + 25, maxFieldFound); } @Test diff --git a/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java b/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java index 1b963514f9..1ba884d1e7 100644 --- a/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java +++ b/libsignal-service/src/test/java/org/whispersystems/signalservice/api/groupsv2/GroupsV2Operations_decrypt_change_Test.java @@ -82,7 +82,7 @@ public final class GroupsV2Operations_decrypt_change_Test { .changeEpoch(GroupsV2Operations.HIGHEST_KNOWN_EPOCH + 1) .build(); - Optional decryptedGroupChangeOptional = groupOperations.decryptChange(change, false); + Optional decryptedGroupChangeOptional = groupOperations.decryptChange(change, DecryptChangeVerificationMode.alreadyTrusted()); assertFalse(decryptedGroupChangeOptional.isPresent()); } @@ -159,7 +159,7 @@ public final class GroupsV2Operations_decrypt_change_Test { actions.addMembers(List.of(new GroupChange.Actions.AddMemberAction.Builder().added(new Member.Builder().role(Member.Role.DEFAULT) .presentation(ByteString.of(randomPresentation)).build()).build())); - groupOperations.decryptChange(new GroupChange.Builder().actions(actions.build().encodeByteString()).build(), false); + groupOperations.decryptChange(new GroupChange.Builder().actions(actions.build().encodeByteString()).build(), DecryptChangeVerificationMode.alreadyTrusted()); } @Test @@ -180,7 +180,7 @@ public final class GroupsV2Operations_decrypt_change_Test { actions.deleteMembers(List.of(new GroupChange.Actions.DeleteMemberAction.Builder().deletedUserId(ByteString.of(randomPresentation)).build())); - groupOperations.decryptChange(new GroupChange.Builder().actions(actions.build().encodeByteString()).build(), false); + groupOperations.decryptChange(new GroupChange.Builder().actions(actions.build().encodeByteString()).build(), DecryptChangeVerificationMode.alreadyTrusted()); } @Test @@ -518,7 +518,7 @@ public final class GroupsV2Operations_decrypt_change_Test { private DecryptedGroupChange decrypt(GroupChange build) { try { - return groupOperations.decryptChange(build, false).get(); + return groupOperations.decryptChange(build, DecryptChangeVerificationMode.alreadyTrusted()).get(); } catch (IOException | VerificationFailedException | InvalidGroupStateException e) { throw new AssertionError(e); }