Validate incoming Group lengths and remote delete entries if wrong.

Ignore incoming messages with bad V1 group lengths.
This commit is contained in:
Alan Evans 2020-09-10 14:39:29 -03:00 committed by GitHub
parent bf4cac0c82
commit 3cffaddc0a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 218 additions and 77 deletions

View file

@ -859,13 +859,14 @@ public class RecipientDatabase extends Database {
for (SignalGroupV2Record insert : groupV2Inserts) {
db.insertOrThrow(TABLE_NAME, null, getValuesForStorageGroupV2(insert));
GroupId.V2 groupId = GroupId.v2(insert.getMasterKey());
GroupMasterKey masterKey = insert.getMasterKeyOrThrow();
GroupId.V2 groupId = GroupId.v2(masterKey);
Recipient recipient = Recipient.externalGroup(context, groupId);
Log.i(TAG, "Creating restore placeholder for " + groupId);
DatabaseFactory.getGroupDatabase(context)
.create(insert.getMasterKey(),
.create(masterKey,
DecryptedGroup.newBuilder()
.setRevision(GroupsV2StateProcessor.RESTORE_PLACEHOLDER_REVISION)
.build());
@ -886,7 +887,8 @@ public class RecipientDatabase extends Database {
throw new AssertionError("Had an update, but it didn't match any rows!");
}
Recipient recipient = Recipient.externalGroup(context, GroupId.v2(update.getOld().getMasterKey()));
GroupMasterKey masterKey = update.getOld().getMasterKeyOrThrow();
Recipient recipient = Recipient.externalGroup(context, GroupId.v2(masterKey));
threadDatabase.setArchived(recipient.getId(), update.getNew().isArchived());
needsRefresh.add(recipient.getId());
@ -1031,7 +1033,7 @@ public class RecipientDatabase extends Database {
private static @NonNull ContentValues getValuesForStorageGroupV2(@NonNull SignalGroupV2Record groupV2) {
ContentValues values = new ContentValues();
values.put(GROUP_ID, GroupId.v2(groupV2.getMasterKey()).toString());
values.put(GROUP_ID, GroupId.v2(groupV2.getMasterKeyOrThrow()).toString());
values.put(GROUP_TYPE, GroupType.SIGNAL_V2.getId());
values.put(PROFILE_SHARING, groupV2.isProfileSharingEnabled() ? "1" : "0");
values.put(BLOCKED, groupV2.isBlocked() ? "1" : "0");

View file

@ -19,6 +19,7 @@ public abstract class GroupId {
private static final String ENCODED_MMS_GROUP_PREFIX = "__signal_mms_group__!";
private static final int MMS_BYTE_LENGTH = 16;
private static final int V1_MMS_BYTE_LENGTH = 16;
private static final int V1_BYTE_LENGTH = 16;
private static final int V2_BYTE_LENGTH = GroupIdentifier.SIZE;
private final String encodedId;
@ -46,6 +47,13 @@ public abstract class GroupId {
return new GroupId.V1(gv1GroupIdBytes);
}
public static @NonNull GroupId.V1 v1Exact(byte[] gv1GroupIdBytes) throws BadGroupIdException {
if (gv1GroupIdBytes.length != V1_BYTE_LENGTH) {
throw new BadGroupIdException();
}
return new GroupId.V1(gv1GroupIdBytes);
}
public static GroupId.V1 createV1(@NonNull SecureRandom secureRandom) {
return v1orThrow(Util.getSecretBytes(secureRandom, V1_MMS_BYTE_LENGTH));
}
@ -54,15 +62,11 @@ public abstract class GroupId {
return mms(Util.getSecretBytes(secureRandom, MMS_BYTE_LENGTH));
}
public static GroupId.V2 v2orThrow(@NonNull byte[] bytes) {
try {
return v2(bytes);
} catch (BadGroupIdException e) {
throw new AssertionError(e);
}
}
public static GroupId.V2 v2(@NonNull byte[] bytes) throws BadGroupIdException {
/**
* Private because it's too easy to pass the {@link GroupMasterKey} bytes directly to this as they
* are the same length as the {@link GroupIdentifier}.
*/
private static GroupId.V2 v2(@NonNull byte[] bytes) throws BadGroupIdException {
if (bytes.length != V2_BYTE_LENGTH) {
throw new BadGroupIdException();
}
@ -70,7 +74,11 @@ public abstract class GroupId {
}
public static GroupId.V2 v2(@NonNull GroupIdentifier groupIdentifier) {
return v2orThrow(groupIdentifier.serialize());
try {
return v2(groupIdentifier.serialize());
} catch (BadGroupIdException e) {
throw new AssertionError(e);
}
}
public static GroupId.V2 v2(@NonNull GroupMasterKey masterKey) {

View file

@ -5,16 +5,16 @@ import androidx.annotation.NonNull;
import com.annimon.stream.Collectors;
import com.annimon.stream.Stream;
import org.thoughtcrime.securesms.groups.BadGroupIdException;
import org.thoughtcrime.securesms.groups.GroupId;
import org.whispersystems.libsignal.util.guava.Optional;
import org.whispersystems.signalservice.api.storage.SignalGroupV1Record;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
class GroupV1ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGroupV1Record> {
final class GroupV1ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGroupV1Record> {
private final Map<GroupId, SignalGroupV1Record> localByGroupId;
@ -29,7 +29,9 @@ class GroupV1ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGr
@Override
public @NonNull Collection<SignalGroupV1Record> getInvalidEntries(@NonNull Collection<SignalGroupV1Record> remoteRecords) {
return Collections.emptySet();
return Stream.of(remoteRecords)
.filterNot(GroupV1ConflictMerger::isValidGroupId)
.toList();
}
@Override
@ -54,4 +56,13 @@ class GroupV1ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGr
.build();
}
}
private static boolean isValidGroupId(@NonNull SignalGroupV1Record record) {
try {
GroupId.v1Exact(record.getGroupId());
return true;
} catch (BadGroupIdException e) {
return false;
}
}
}

View file

@ -4,6 +4,7 @@ import androidx.annotation.NonNull;
import com.annimon.stream.Collectors;
import com.annimon.stream.Stream;
import com.google.protobuf.ByteString;
import org.signal.zkgroup.groups.GroupMasterKey;
import org.whispersystems.libsignal.util.guava.Optional;
@ -11,25 +12,26 @@ import org.whispersystems.signalservice.api.storage.SignalGroupV2Record;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
class GroupV2ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGroupV2Record> {
final class GroupV2ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGroupV2Record> {
private final Map<GroupMasterKey, SignalGroupV2Record> localByGroupId;
private final Map<ByteString, SignalGroupV2Record> localByMasterKeyBytes;
GroupV2ConflictMerger(@NonNull Collection<SignalGroupV2Record> localOnly) {
localByGroupId = Stream.of(localOnly).collect(Collectors.toMap(SignalGroupV2Record::getMasterKey, g -> g));
localByMasterKeyBytes = Stream.of(localOnly).collect(Collectors.toMap((SignalGroupV2Record signalGroupV2Record) -> ByteString.copyFrom(signalGroupV2Record.getMasterKeyBytes()), g -> g));
}
@Override
public @NonNull Optional<SignalGroupV2Record> getMatching(@NonNull SignalGroupV2Record record) {
return Optional.fromNullable(localByGroupId.get(record.getMasterKey()));
return Optional.fromNullable(localByMasterKeyBytes.get(ByteString.copyFrom(record.getMasterKeyBytes())));
}
@Override
public @NonNull Collection<SignalGroupV2Record> getInvalidEntries(@NonNull Collection<SignalGroupV2Record> remoteRecords) {
return Collections.emptySet();
return Stream.of(remoteRecords)
.filterNot(GroupV2ConflictMerger::isValidMasterKey)
.toList();
}
@Override
@ -47,11 +49,15 @@ class GroupV2ConflictMerger implements StorageSyncHelper.ConflictMerger<SignalGr
} else if (matchesLocal) {
return local;
} else {
return new SignalGroupV2Record.Builder(keyGenerator.generate(), remote.getMasterKey())
return new SignalGroupV2Record.Builder(keyGenerator.generate(), remote.getMasterKeyBytes())
.setUnknownFields(unknownFields)
.setBlocked(blocked)
.setProfileSharingEnabled(blocked)
.build();
}
}
private static boolean isValidMasterKey(@NonNull SignalGroupV2Record record) {
return record.getMasterKeyBytes().length == GroupMasterKey.SIZE;
}
}

View file

@ -282,6 +282,7 @@ public final class StorageSyncHelper {
Set<SignalRecord> remoteDeletes = new HashSet<>();
remoteDeletes.addAll(contactMergeResult.remoteDeletes);
remoteDeletes.addAll(groupV1MergeResult.remoteDeletes);
remoteDeletes.addAll(groupV2MergeResult.remoteDeletes);
remoteDeletes.addAll(accountMergeResult.remoteDeletes);
return new MergeResult(contactMergeResult.localInserts,
@ -603,8 +604,8 @@ public final class StorageSyncHelper {
@Override
public @NonNull String toString() {
return String.format(Locale.ENGLISH,
"localContactInserts: %d, localContactUpdates: %d, localGroupV1Inserts: %d, localGroupV1Updates: %d, localGroupV2Inserts: %d, localGroupV2Updates: %d, localUnknownInserts: %d, localUnknownDeletes: %d, localAccountUpdate: %b, remoteInserts: %d, remoteUpdates: %d",
localContactInserts.size(), localContactUpdates.size(), localGroupV1Inserts.size(), localGroupV1Updates.size(), localGroupV2Inserts.size(), localGroupV2Updates.size(), localUnknownInserts.size(), localUnknownDeletes.size(), localAccountUpdate.isPresent(), remoteInserts.size(), remoteUpdates.size());
"localContactInserts: %d, localContactUpdates: %d, localGroupV1Inserts: %d, localGroupV1Updates: %d, localGroupV2Inserts: %d, localGroupV2Updates: %d, localUnknownInserts: %d, localUnknownDeletes: %d, localAccountUpdate: %b, remoteInserts: %d, remoteUpdates: %d, remoteDeletes: %d",
localContactInserts.size(), localContactUpdates.size(), localGroupV1Inserts.size(), localGroupV1Updates.size(), localGroupV2Inserts.size(), localGroupV2Updates.size(), localUnknownInserts.size(), localUnknownDeletes.size(), localAccountUpdate.isPresent(), remoteInserts.size(), remoteUpdates.size(), remoteDeletes.size());
}
}

View file

@ -38,7 +38,7 @@ public final class GroupUtil {
throws BadGroupIdException
{
if (groupContext.getGroupV1().isPresent()) {
return GroupId.v1(groupContext.getGroupV1().get().getGroupId());
return GroupId.v1Exact(groupContext.getGroupV1().get().getGroupId());
} else if (groupContext.getGroupV2().isPresent()) {
return GroupId.v2(groupContext.getGroupV2().get().getMasterKey());
} else {

View file

@ -261,19 +261,19 @@ public final class GroupIdTest {
groupId.requireV2();
}
@Test(expected = BadGroupIdException.class)
public void cannot_create_v1_with_a_v2_length() throws IOException, BadGroupIdException {
GroupId.v1(Hex.fromStringCondensed("9f475f59b2518bff6df22e820803f0e3585bd99e686fa7e7fbfc2f92fd5d953e"));
}
@Test(expected = AssertionError.class)
public void cannot_create_v1_with_a_v2_length() throws IOException {
public void cannot_create_v1_with_a_v2_length_assert() throws IOException {
GroupId.v1orThrow(Hex.fromStringCondensed("9f475f59b2518bff6df22e820803f0e3585bd99e686fa7e7fbfc2f92fd5d953e"));
}
@Test(expected = BadGroupIdException.class)
public void cannot_create_v2_with_a_v1_length() throws IOException, BadGroupIdException {
GroupId.v2(Hex.fromStringCondensed("000102030405060708090a0b0c0d0e0f"));
}
@Test(expected = AssertionError.class)
public void cannot_create_v2_with_a_v1_length_assert() throws IOException {
GroupId.v2orThrow(Hex.fromStringCondensed("000102030405060708090a0b0c0d0e0f"));
public void cannot_create_v1_with_wrong_length() throws IOException, BadGroupIdException {
GroupId.v1Exact(Hex.fromStringCondensed("000102030405060708090a0b0c0d0e"));
}
@Test
@ -292,6 +292,22 @@ public final class GroupIdTest {
assertTrue(v1.isV1());
}
@Test
public void v1_static_factory() throws BadGroupIdException {
GroupId.V1 v1 = GroupId.v1(new byte[]{ 9, 10, 11, 12, 13, 14, 15, 0, 1, 2, 3, 4, 5, 6, 7, 8 });
assertEquals("__textsecure_group__!090a0b0c0d0e0f000102030405060708", v1.toString());
assertTrue(v1.isV1());
}
@Test
public void v1Exact_static_factory() throws BadGroupIdException {
GroupId.V1 v1 = GroupId.v1Exact(new byte[]{ 9, 10, 11, 12, 13, 14, 15, 0, 1, 2, 3, 4, 5, 6, 7, 8 });
assertEquals("__textsecure_group__!090a0b0c0d0e0f000102030405060708", v1.toString());
assertTrue(v1.isV1());
}
@Test
public void parse_bytes_to_v1_via_push() throws BadGroupIdException {
GroupId.V1 v1 = GroupId.push(new byte[]{ 9, 10, 11, 12, 13, 14, 15, 0, 1, 2, 3, 4, 5, 6, 7, 8 }).requireV1();

View file

@ -1,10 +1,11 @@
package org.thoughtcrime.securesms.storage;
import org.junit.Test;
import org.thoughtcrime.securesms.storage.GroupV1ConflictMerger;
import org.thoughtcrime.securesms.storage.StorageSyncHelper.KeyGenerator;
import org.whispersystems.signalservice.api.storage.SignalGroupV1Record;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import static org.junit.Assert.assertArrayEquals;
@ -14,10 +15,11 @@ import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.when;
import static org.thoughtcrime.securesms.testutil.TestHelpers.byteArray;
public class GroupV1ConflictMergerTest {
public final class GroupV1ConflictMergerTest {
private static final byte[] GENERATED_KEY = byteArray(8675309);
private static final KeyGenerator KEY_GENERATOR = mock(KeyGenerator.class);
private static byte[] GENERATED_KEY = byteArray(8675309);
private static KeyGenerator KEY_GENERATOR = mock(KeyGenerator.class);
static {
when(KEY_GENERATOR.generate()).thenReturn(GENERATED_KEY);
}
@ -78,4 +80,31 @@ public class GroupV1ConflictMergerTest {
assertEquals(local, merged);
}
@Test
public void merge_excludeBadGroupId() {
SignalGroupV1Record badRemote = new SignalGroupV1Record.Builder(byteArray(1), badGroupKey(99))
.setBlocked(false)
.setProfileSharingEnabled(true)
.setArchived(true)
.build();
SignalGroupV1Record goodRemote = new SignalGroupV1Record.Builder(byteArray(1), groupKey(99))
.setBlocked(false)
.setProfileSharingEnabled(true)
.setArchived(true)
.build();
Collection<SignalGroupV1Record> invalid = new GroupV1ConflictMerger(Collections.emptyList()).getInvalidEntries(Arrays.asList(badRemote, goodRemote));
assertEquals(Collections.singletonList(badRemote), invalid);
}
private static byte[] groupKey(int value) {
return byteArray(value, 16);
}
private static byte[] badGroupKey(int value) {
return byteArray(value, 32);
}
}

View file

@ -1,11 +1,11 @@
package org.thoughtcrime.securesms.storage;
import org.junit.Test;
import org.signal.zkgroup.InvalidInputException;
import org.signal.zkgroup.groups.GroupMasterKey;
import org.thoughtcrime.securesms.storage.StorageSyncHelper.KeyGenerator;
import org.whispersystems.signalservice.api.storage.SignalGroupV2Record;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import static org.junit.Assert.assertArrayEquals;
@ -15,10 +15,11 @@ import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.when;
import static org.thoughtcrime.securesms.testutil.TestHelpers.byteArray;
public class GroupV2ConflictMergerTest {
public final class GroupV2ConflictMergerTest {
private static final byte[] GENERATED_KEY = byteArray(8675309);
private static final KeyGenerator KEY_GENERATOR = mock(KeyGenerator.class);
private static byte[] GENERATED_KEY = byteArray(8675309);
private static KeyGenerator KEY_GENERATOR = mock(KeyGenerator.class);
static {
when(KEY_GENERATOR.generate()).thenReturn(GENERATED_KEY);
}
@ -39,7 +40,7 @@ public class GroupV2ConflictMergerTest {
SignalGroupV2Record merged = new GroupV2ConflictMerger(Collections.singletonList(local)).merge(remote, local, KEY_GENERATOR);
assertArrayEquals(GENERATED_KEY, merged.getId().getRaw());
assertEquals(groupKey(100), merged.getMasterKey());
assertArrayEquals(groupKey(100), merged.getMasterKeyBytes());
assertFalse(merged.isBlocked());
assertFalse(merged.isArchived());
}
@ -80,11 +81,30 @@ public class GroupV2ConflictMergerTest {
assertEquals(local, merged);
}
private static GroupMasterKey groupKey(int value) {
try {
return new GroupMasterKey(byteArray(value, 32));
} catch (InvalidInputException e) {
throw new AssertionError(e);
@Test
public void merge_excludeBadGroupId() {
SignalGroupV2Record badRemote = new SignalGroupV2Record.Builder(byteArray(1), badGroupKey(99))
.setBlocked(false)
.setProfileSharingEnabled(true)
.setArchived(true)
.build();
SignalGroupV2Record goodRemote = new SignalGroupV2Record.Builder(byteArray(1), groupKey(99))
.setBlocked(false)
.setProfileSharingEnabled(true)
.setArchived(true)
.build();
Collection<SignalGroupV2Record> invalid = new GroupV2ConflictMerger(Collections.emptyList()).getInvalidEntries(Arrays.asList(badRemote, goodRemote));
assertEquals(Collections.singletonList(badRemote), invalid);
}
private static byte[] groupKey(int value) {
return byteArray(value, 32);
}
private static byte[] badGroupKey(int value) {
return byteArray(value, 16);
}
}

View file

@ -129,6 +129,34 @@ public final class StorageSyncHelperTest {
assertEquals(setOf(remote1), result.getRemoteDeletes());
}
@Test
public void resolveConflict_contact_deleteBadGv1() {
SignalGroupV1Record remote1 = badGroupV1(1, 1, true, false);
SignalGroupV1Record local1 = groupV1(2, 1, true, true);
MergeResult result = StorageSyncHelper.resolveConflict(recordSetOf(remote1), recordSetOf(local1));
assertTrue(result.getLocalContactInserts().isEmpty());
assertTrue(result.getLocalContactUpdates().isEmpty());
assertEquals(setOf(record(local1)), result.getRemoteInserts());
assertTrue(result.getRemoteUpdates().isEmpty());
assertEquals(setOf(remote1), result.getRemoteDeletes());
}
@Test
public void resolveConflict_contact_deleteBadGv2() {
SignalGroupV2Record remote1 = badGroupV2(1, 2, true, false);
SignalGroupV2Record local1 = groupV2(2, 2, true, false);
MergeResult result = StorageSyncHelper.resolveConflict(recordSetOf(remote1), recordSetOf(local1));
assertTrue(result.getLocalContactInserts().isEmpty());
assertTrue(result.getLocalContactUpdates().isEmpty());
assertEquals(setOf(record(local1)), result.getRemoteInserts());
assertTrue(result.getRemoteUpdates().isEmpty());
assertEquals(setOf(remote1), result.getRemoteDeletes());
}
@Test
public void resolveConflict_contact_sameAsRemote() {
SignalContactRecord remote1 = contact(1, UUID_A, E164_A, "a");
@ -417,7 +445,15 @@ public final class StorageSyncHelperTest {
boolean blocked,
boolean profileSharing)
{
return new SignalGroupV1Record.Builder(byteArray(key), byteArray(groupId)).setBlocked(blocked).setProfileSharingEnabled(profileSharing).build();
return new SignalGroupV1Record.Builder(byteArray(key), byteArray(groupId, 16)).setBlocked(blocked).setProfileSharingEnabled(profileSharing).build();
}
private static SignalGroupV1Record badGroupV1(int key,
int groupId,
boolean blocked,
boolean profileSharing)
{
return new SignalGroupV1Record.Builder(byteArray(key), byteArray(groupId, 42)).setBlocked(blocked).setProfileSharingEnabled(profileSharing).build();
}
private static SignalGroupV2Record groupV2(int key,
@ -425,11 +461,15 @@ public final class StorageSyncHelperTest {
boolean blocked,
boolean profileSharing)
{
try {
return new SignalGroupV2Record.Builder(byteArray(key), new GroupMasterKey(byteArray(groupId, 32))).setBlocked(blocked).setProfileSharingEnabled(profileSharing).build();
} catch (InvalidInputException e) {
throw new AssertionError(e);
return new SignalGroupV2Record.Builder(byteArray(key), byteArray(groupId, 32)).setBlocked(blocked).setProfileSharingEnabled(profileSharing).build();
}
private static SignalGroupV2Record badGroupV2(int key,
int groupId,
boolean blocked,
boolean profileSharing)
{
return new SignalGroupV2Record.Builder(byteArray(key), byteArray(groupId, 42)).setBlocked(blocked).setProfileSharingEnabled(profileSharing).build();
}
private static <E extends SignalRecord> StorageSyncHelper.RecordUpdate<E> update(E oldRecord, E newRecord) {

View file

@ -13,18 +13,14 @@ public final class SignalGroupV2Record implements SignalRecord {
private final StorageId id;
private final GroupV2Record proto;
private final GroupMasterKey masterKey;
private final byte[] masterKey;
private final boolean hasUnknownFields;
public SignalGroupV2Record(StorageId id, GroupV2Record proto) {
this.id = id;
this.proto = proto;
this.hasUnknownFields = ProtoUtil.hasUnknownFields(proto);
try {
this.masterKey = new GroupMasterKey(proto.getMasterKey().toByteArray());
} catch (InvalidInputException e) {
throw new AssertionError(e);
}
this.masterKey = proto.getMasterKey().toByteArray();
}
@Override
@ -40,10 +36,18 @@ public final class SignalGroupV2Record implements SignalRecord {
return hasUnknownFields ? proto.toByteArray() : null;
}
public GroupMasterKey getMasterKey() {
public byte[] getMasterKeyBytes() {
return masterKey;
}
public GroupMasterKey getMasterKeyOrThrow() {
try {
return new GroupMasterKey(masterKey);
} catch (InvalidInputException e) {
throw new AssertionError(e);
}
}
public boolean isBlocked() {
return proto.getBlocked();
}
@ -81,10 +85,14 @@ public final class SignalGroupV2Record implements SignalRecord {
private byte[] unknownFields;
public Builder(byte[] rawId, GroupMasterKey masterKey) {
this(rawId, masterKey.serialize());
}
public Builder(byte[] rawId, byte[] masterKey) {
this.id = StorageId.forGroupV2(rawId);
this.builder = GroupV2Record.newBuilder();
builder.setMasterKey(ByteString.copyFrom(masterKey.serialize()));
builder.setMasterKey(ByteString.copyFrom(masterKey));
}
public Builder setUnknownFields(byte[] serializedUnknowns) {