From ec8d5defd4a89290a3304f460bba3d3fe4a27571 Mon Sep 17 00:00:00 2001 From: Alan Evans Date: Tue, 19 May 2020 15:41:48 -0300 Subject: [PATCH] Protect against unknown GV2 UUIDs. --- .../securesms/database/GroupDatabase.java | 53 +++++++++++++------ .../securesms/database/MmsDatabase.java | 21 +++----- .../securesms/groups/v2/ProfileKeySet.java | 8 ++- .../securesms/mms/MessageGroupContext.java | 19 +++---- .../signalservice/api/util/UuidUtil.java | 15 ++++++ 5 files changed, 71 insertions(+), 45 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java index 5d8a313d07..4a3e30320f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java @@ -30,7 +30,6 @@ import org.thoughtcrime.securesms.recipients.RecipientId; import org.whispersystems.libsignal.util.guava.Optional; import org.whispersystems.signalservice.api.groupsv2.DecryptedGroupUtil; import org.whispersystems.signalservice.api.messages.SignalServiceAttachmentPointer; -import org.whispersystems.signalservice.api.push.SignalServiceAddress; import org.whispersystems.signalservice.api.util.UuidUtil; import java.io.Closeable; @@ -41,6 +40,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Locale; import java.util.UUID; public final class GroupDatabase extends Database { @@ -252,7 +252,7 @@ public final class GroupDatabase extends Database { @WorkerThread public @NonNull List getGroupMembers(@NonNull GroupId groupId, @NonNull MemberSet memberSet) { if (groupId.isV2()) { - return getGroup(groupId).transform(g -> g.requireV2GroupProperties().getMembers(context, memberSet)) + return getGroup(groupId).transform(g -> g.requireV2GroupProperties().getMemberRecipients(memberSet)) .or(Collections.emptyList()); } else { List currentMembers = getCurrentMembers(groupId); @@ -338,7 +338,7 @@ public final class GroupDatabase extends Database { contentValues.put(V2_MASTER_KEY, groupMasterKey.serialize()); contentValues.put(V2_REVISION, groupState.getVersion()); contentValues.put(V2_DECRYPTED_GROUP, groupState.toByteArray()); - contentValues.put(MEMBERS, serializeV2GroupMembers(context, groupState)); + contentValues.put(MEMBERS, serializeV2GroupMembers(groupState)); } else { if (groupId.isV2()) { throw new AssertionError("V2 group id but no master key"); @@ -395,7 +395,7 @@ public final class GroupDatabase extends Database { contentValues.put(TITLE, title); contentValues.put(V2_REVISION, decryptedGroup.getVersion()); contentValues.put(V2_DECRYPTED_GROUP, decryptedGroup.toByteArray()); - contentValues.put(MEMBERS, serializeV2GroupMembers(context, decryptedGroup)); + contentValues.put(MEMBERS, serializeV2GroupMembers(decryptedGroup)); databaseHelper.getWritableDatabase().update(TABLE_NAME, contentValues, GROUP_ID + " = ?", @@ -517,13 +517,16 @@ public final class GroupDatabase extends Database { return getGroup(groupId).transform(g -> g.isPendingMember(recipient)).or(false); } - private static String serializeV2GroupMembers(@NonNull Context context, @NonNull DecryptedGroup decryptedGroup) { + private static String serializeV2GroupMembers(@NonNull DecryptedGroup decryptedGroup) { List groupMembers = new ArrayList<>(decryptedGroup.getMembersCount()); for (DecryptedMember member : decryptedGroup.getMembersList()) { - Recipient recipient = Recipient.externalPush(context, new SignalServiceAddress(UuidUtil.fromByteString(member.getUuid()), null)); - - groupMembers.add(recipient.getId()); + UUID uuid = UuidUtil.fromByteString(member.getUuid()); + if (UuidUtil.UNKNOWN_UUID.equals(uuid)) { + Log.w(TAG, "Seen unknown UUID in members list"); + } else { + groupMembers.add(RecipientId.from(uuid, null)); + } } Collections.sort(groupMembers); @@ -779,25 +782,41 @@ public final class GroupDatabase extends Database { .or(false); } - public List getMembers(@NonNull Context context, @NonNull MemberSet memberSet) { - boolean includeSelf = memberSet.includeSelf; - DecryptedGroup groupV2 = getDecryptedGroup(); - UUID selfUuid = Recipient.self().getUuid().get(); - List recipients = new ArrayList<>(groupV2.getMembersCount() + groupV2.getPendingMembersCount()); + public List getMemberRecipients(@NonNull MemberSet memberSet) { + return Stream.of(getMemberRecipientIds(memberSet)) + .map(Recipient::resolved) + .toList(); + } + + public List getMemberRecipientIds(@NonNull MemberSet memberSet) { + boolean includeSelf = memberSet.includeSelf; + DecryptedGroup groupV2 = getDecryptedGroup(); + UUID selfUuid = Recipient.self().getUuid().get(); + List recipients = new ArrayList<>(groupV2.getMembersCount() + groupV2.getPendingMembersCount()); + int unknownMembers = 0; + int unknownPending = 0; for (UUID uuid : DecryptedGroupUtil.toUuidList(groupV2.getMembersList())) { - if (includeSelf || !selfUuid.equals(uuid)) { - recipients.add(Recipient.externalPush(context, uuid, null)); + if (UuidUtil.UNKNOWN_UUID.equals(uuid)) { + unknownMembers++; + } else if (includeSelf || !selfUuid.equals(uuid)) { + recipients.add(RecipientId.from(uuid, null)); } } if (memberSet.includePending) { for (UUID uuid : DecryptedGroupUtil.pendingToUuidList(groupV2.getPendingMembersList())) { - if (includeSelf || !selfUuid.equals(uuid)) { - recipients.add(Recipient.externalPush(context, uuid, null)); + if (UuidUtil.UNKNOWN_UUID.equals(uuid)) { + unknownPending++; + } else if (includeSelf || !selfUuid.equals(uuid)) { + recipients.add(RecipientId.from(uuid, null)); } } } + if ((unknownMembers + unknownPending) > 0) { + Log.w(TAG, String.format(Locale.US, "Group contains %d + %d unknown pending and full members", unknownPending, unknownMembers)); + } + return recipients; } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java index e1d5792a4d..6e73c6d458 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java @@ -1083,9 +1083,7 @@ public class MmsDatabase extends MessagingDatabase { if (message.isGroup()) { OutgoingGroupUpdateMessage outgoingGroupUpdateMessage = (OutgoingGroupUpdateMessage) message; if (outgoingGroupUpdateMessage.isV2Group()) { - MessageGroupContext.GroupV2Properties groupV2Properties = outgoingGroupUpdateMessage.requireGroupV2Properties(); - type |= Types.GROUP_V2_BIT; - if (groupV2Properties.isUpdate()) type |= Types.GROUP_UPDATE_BIT; + type |= Types.GROUP_V2_BIT | Types.GROUP_UPDATE_BIT; } else { MessageGroupContext.GroupV1Properties properties = outgoingGroupUpdateMessage.requireGroupV1Properties(); if (properties.isUpdate()) type |= Types.GROUP_UPDATE_BIT; @@ -1129,20 +1127,15 @@ public class MmsDatabase extends MessagingDatabase { if (message.getRecipient().isGroup()) { OutgoingGroupUpdateMessage outgoingGroupUpdateMessage = (message instanceof OutgoingGroupUpdateMessage) ? (OutgoingGroupUpdateMessage) message : null; - GroupReceiptDatabase receiptDatabase = DatabaseFactory.getGroupReceiptDatabase(context); - RecipientDatabase recipientDatabase = DatabaseFactory.getRecipientDatabase(context); - Set members = new HashSet<>(); + GroupReceiptDatabase receiptDatabase = DatabaseFactory.getGroupReceiptDatabase(context); + Set members = new HashSet<>(); if (outgoingGroupUpdateMessage != null && outgoingGroupUpdateMessage.isV2Group()) { MessageGroupContext.GroupV2Properties groupV2Properties = outgoingGroupUpdateMessage.requireGroupV2Properties(); - members.addAll(Stream.of(groupV2Properties.getActiveMembers()).map(recipientDatabase::getOrInsertFromUuid).toList()); - if (groupV2Properties.isUpdate()) { - members.addAll(Stream.concat(Stream.of(groupV2Properties.getPendingMembers()), - Stream.of(groupV2Properties.getRemovedMembers())) - .distinct() - .map(recipientDatabase::getOrInsertFromUuid) - .toList()); - } + members.addAll(Stream.of(groupV2Properties.getAllActivePendingAndRemovedMembers()) + .distinct() + .map(uuid -> RecipientId.from(uuid, null)) + .toList()); members.remove(Recipient.self().getId()); } else { members.addAll(Stream.of(DatabaseFactory.getGroupDatabase(context).getGroupMembers(message.getRecipient().requireGroupId(), GroupDatabase.MemberSet.FULL_MEMBERS_EXCLUDING_SELF)).map(Recipient::getId).toList()); diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/v2/ProfileKeySet.java b/app/src/main/java/org/thoughtcrime/securesms/groups/v2/ProfileKeySet.java index 7e36971aad..e5536484a8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/v2/ProfileKeySet.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/v2/ProfileKeySet.java @@ -36,7 +36,13 @@ public final class ProfileKeySet { @Nullable UUID changeSource) { for (DecryptedMember member : group.getMembersList()) { - UUID memberUuid = UuidUtil.fromByteString(member.getUuid()); + UUID memberUuid = UuidUtil.fromByteString(member.getUuid()); + + if (UuidUtil.UNKNOWN_UUID.equals(memberUuid)) { + Log.w(TAG, "Seen unknown member UUID"); + continue; + } + ProfileKey profileKey; try { profileKey = new ProfileKey(member.getProfileKey().toByteArray()); diff --git a/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java b/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java index 34307809e8..ff93e66321 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java +++ b/app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java @@ -164,21 +164,14 @@ public final class MessageGroupContext { return groupMasterKey; } - public @NonNull List getActiveMembers() { - return DecryptedGroupUtil.membersToUuidList(decryptedGroupV2Context.getGroupState().getMembersList()); - } + public @NonNull List getAllActivePendingAndRemovedMembers() { + LinkedList memberUuids = new LinkedList<>(); - public @NonNull List getPendingMembers() { - return DecryptedGroupUtil.pendingToUuidList(decryptedGroupV2Context.getGroupState().getPendingMembersList()); - } + memberUuids.addAll(DecryptedGroupUtil.membersToUuidList(decryptedGroupV2Context.getGroupState().getMembersList())); + memberUuids.addAll(DecryptedGroupUtil.pendingToUuidList(decryptedGroupV2Context.getGroupState().getPendingMembersList())); + memberUuids.addAll(DecryptedGroupUtil.removedMembersUuidList(decryptedGroupV2Context.getChange())); - public @NonNull List getRemovedMembers() { - return DecryptedGroupUtil.removedMembersUuidList(decryptedGroupV2Context.getChange()); - } - - public boolean isUpdate() { - // The group context is only stored on update messages. - return true; + return UuidUtil.filterKnown(memberUuids); } @Override diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/util/UuidUtil.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/util/UuidUtil.java index 83967f3951..1467e35c69 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/util/UuidUtil.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/util/UuidUtil.java @@ -81,4 +81,19 @@ public final class UuidUtil { return result; } + + /** + * Keep only UUIDs that are not the {@link #UNKNOWN_UUID}. + */ + public static List filterKnown(Collection uuids) { + ArrayList result = new ArrayList<>(uuids.size()); + + for (UUID uuid : uuids) { + if (!UNKNOWN_UUID.equals(uuid)) { + result.add(uuid); + } + } + + return result; + } }