Skip to content

Commit

Permalink
Protect against unknown GV2 UUIDs.
Browse files Browse the repository at this point in the history
  • Loading branch information
@alan-signal @greyson-signal
alan-signal authored and greyson-signal committed May 29, 2020
1 parent 981676c commit ec8d5de
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 45 deletions.
53 changes: 36 additions & 17 deletions app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java
View file
Expand Up @@ -30,7 +30,6 @@
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;
Expand All @@ -41,6 +40,7 @@
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 {
Expand Down Expand Up @@ -252,7 +252,7 @@ public Reader getGroups() {
@WorkerThread
public @NonNull List<Recipient> 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<RecipientId> currentMembers = getCurrentMembers(groupId);
Expand Down Expand Up @@ -338,7 +338,7 @@ private void create(@NonNull GroupId groupId,
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");
Expand Down Expand Up @@ -395,7 +395,7 @@ public void update(@NonNull GroupId.V2 groupId, @NonNull DecryptedGroup decrypte
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 + " = ?",
Expand Down Expand Up @@ -517,13 +517,16 @@ public boolean isPendingMember(@NonNull GroupId.Push groupId, @NonNull Recipient
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<RecipientId> 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);
Expand Down Expand Up @@ -779,25 +782,41 @@ public boolean isAdmin(@NonNull Recipient recipient) {
.or(false);
}

public List<Recipient> getMembers(@NonNull Context context, @NonNull MemberSet memberSet) {
boolean includeSelf = memberSet.includeSelf;
DecryptedGroup groupV2 = getDecryptedGroup();
UUID selfUuid = Recipient.self().getUuid().get();
List<Recipient> recipients = new ArrayList<>(groupV2.getMembersCount() + groupV2.getPendingMembersCount());
public List<Recipient> getMemberRecipients(@NonNull MemberSet memberSet) {
return Stream.of(getMemberRecipientIds(memberSet))
.map(Recipient::resolved)
.toList();
}

public List<RecipientId> getMemberRecipientIds(@NonNull MemberSet memberSet) {
boolean includeSelf = memberSet.includeSelf;
DecryptedGroup groupV2 = getDecryptedGroup();
UUID selfUuid = Recipient.self().getUuid().get();
List<RecipientId> 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;
}
}
Expand Down
21 changes: 7 additions & 14 deletions app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.java
View file
Expand Up @@ -1083,9 +1083,7 @@ public long insertMessageOutbox(@NonNull OutgoingMediaMessage message,
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;
Expand Down Expand Up @@ -1129,20 +1127,15 @@ public long insertMessageOutbox(@NonNull OutgoingMediaMessage message,
if (message.getRecipient().isGroup()) {
OutgoingGroupUpdateMessage outgoingGroupUpdateMessage = (message instanceof OutgoingGroupUpdateMessage) ? (OutgoingGroupUpdateMessage) message : null;

GroupReceiptDatabase receiptDatabase = DatabaseFactory.getGroupReceiptDatabase(context);
RecipientDatabase recipientDatabase = DatabaseFactory.getRecipientDatabase(context);
Set<RecipientId> members = new HashSet<>();
GroupReceiptDatabase receiptDatabase = DatabaseFactory.getGroupReceiptDatabase(context);
Set<RecipientId> 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());
Expand Down
8 changes: 7 additions & 1 deletion app/src/main/java/org/thoughtcrime/securesms/groups/v2/ProfileKeySet.java
View file
Expand Up @@ -36,7 +36,13 @@ public void addKeysFromGroupState(@NonNull DecryptedGroup group,
@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());
Expand Down
19 changes: 6 additions & 13 deletions app/src/main/java/org/thoughtcrime/securesms/mms/MessageGroupContext.java
View file
Expand Up @@ -164,21 +164,14 @@ private GroupV2Properties(DecryptedGroupV2Context decryptedGroupV2Context) {
return groupMasterKey;
}

public @NonNull List<UUID> getActiveMembers() {
return DecryptedGroupUtil.membersToUuidList(decryptedGroupV2Context.getGroupState().getMembersList());
}
public @NonNull List<UUID> getAllActivePendingAndRemovedMembers() {
LinkedList<UUID> memberUuids = new LinkedList<>();

public @NonNull List<UUID> 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<UUID> 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
Expand Down
15 changes: 15 additions & 0 deletions libsignal/service/src/main/java/org/whispersystems/signalservice/api/util/UuidUtil.java
View file
Expand Up @@ -81,4 +81,19 @@ public static List<UUID> fromByteStrings(Collection<ByteString> byteStringCollec

return result;
}

/**
* Keep only UUIDs that are not the {@link #UNKNOWN_UUID}.
*/
public static List<UUID> filterKnown(Collection<UUID> uuids) {
ArrayList<UUID> result = new ArrayList<>(uuids.size());

for (UUID uuid : uuids) {
if (!UNKNOWN_UUID.equals(uuid)) {
result.add(uuid);
}
}

return result;
}
}

0 comments on commit ec8d5de

Please sign in to comment.