From 1322f5bc08b1dd4b25ef8126eff35731fa934c2d Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 27 May 2021 16:12:07 -0400 Subject: [PATCH] Be more careful with unknown IDs during storage sync. --- .../database/UnknownStorageIdDatabase.java | 10 +++++++--- .../securesms/jobs/StorageSyncJob.java | 12 ++++-------- .../api/storage/SignalStorageModels.java | 6 ++++++ .../signalservice/api/storage/StorageId.java | 14 +++++++++++++- 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/UnknownStorageIdDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/UnknownStorageIdDatabase.java index 54b7a5ae5..4e1a29ab1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/UnknownStorageIdDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/UnknownStorageIdDatabase.java @@ -15,6 +15,7 @@ import org.thoughtcrime.securesms.util.SqlUtil; import org.whispersystems.libsignal.util.guava.Preconditions; import org.whispersystems.signalservice.api.storage.SignalStorageRecord; import org.whispersystems.signalservice.api.storage.StorageId; +import org.whispersystems.signalservice.internal.storage.protos.ManifestRecord; import org.whispersystems.signalservice.internal.storage.protos.SignalStorage; import java.io.IOException; @@ -46,10 +47,13 @@ public class UnknownStorageIdDatabase extends Database { super(context, databaseHelper); } - public List getAllIds() { - List keys = new ArrayList<>(); + public List getAllUnknownIds() { + List keys = new ArrayList<>(); - try (Cursor cursor = databaseHelper.getReadableDatabase().query(TABLE_NAME, null, null, null, null, null, null)) { + String query = TYPE + " > ?"; + String[] args = SqlUtil.buildArgs(StorageId.largestKnownType()); + + try (Cursor cursor = databaseHelper.getReadableDatabase().query(TABLE_NAME, null, query, args, null, null, null)) { while (cursor != null && cursor.moveToNext()) { String keyEncoded = cursor.getString(cursor.getColumnIndexOrThrow(STORAGE_ID)); int type = cursor.getInt(cursor.getColumnIndexOrThrow(TYPE)); diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java index 44f4bff01..d92d40749 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJob.java @@ -20,7 +20,6 @@ import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraint; import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.migrations.StorageServiceMigrationJob; import org.thoughtcrime.securesms.recipients.Recipient; -import org.thoughtcrime.securesms.recipients.RecipientId; import org.thoughtcrime.securesms.storage.AccountRecordProcessor; import org.thoughtcrime.securesms.storage.ContactRecordProcessor; import org.thoughtcrime.securesms.storage.GroupV1RecordProcessor; @@ -32,8 +31,6 @@ import org.thoughtcrime.securesms.storage.StorageSyncHelper.WriteOperationResult import org.thoughtcrime.securesms.storage.StorageSyncModels; import org.thoughtcrime.securesms.storage.StorageSyncValidations; import org.thoughtcrime.securesms.transport.RetryLaterException; -import org.thoughtcrime.securesms.util.FeatureFlags; -import org.thoughtcrime.securesms.util.SetUtil; import org.thoughtcrime.securesms.util.Stopwatch; import org.thoughtcrime.securesms.util.TextSecurePreferences; import org.thoughtcrime.securesms.util.Util; @@ -51,7 +48,6 @@ import org.whispersystems.signalservice.api.storage.SignalStorageRecord; import org.whispersystems.signalservice.api.storage.StorageId; import org.whispersystems.signalservice.api.storage.StorageKey; import org.whispersystems.signalservice.internal.storage.protos.ManifestRecord; -import org.whispersystems.signalservice.internal.storage.protos.SignalStorage; import java.io.IOException; import java.util.ArrayList; @@ -60,8 +56,6 @@ import java.util.Collection; import java.util.Collections; import java.util.LinkedList; import java.util.List; -import java.util.Locale; -import java.util.Set; import java.util.concurrent.TimeUnit; /** @@ -269,8 +263,10 @@ public class StorageSyncJob extends BaseJob { remoteGv2.add(remote.getGroupV2().get()); } else if (remote.getAccount().isPresent()) { remoteAccount.add(remote.getAccount().get()); - } else { + } else if (remote.getId().isUnknown()) { remoteUnknown.add(remote); + } else { + Log.w(TAG, "Bad record! Type is a known value (" + remote.getId().getType() + "), but doesn't have a matching inner record. Dropping it."); } } @@ -370,7 +366,7 @@ public class StorageSyncJob extends BaseJob { private static @NonNull List getAllLocalStorageIds(@NonNull Context context, @NonNull Recipient self) { return Util.concatenatedList(DatabaseFactory.getRecipientDatabase(context).getContactStorageSyncIds(), Collections.singletonList(StorageId.forAccount(self.getStorageServiceId())), - DatabaseFactory.getUnknownStorageIdDatabase(context).getAllIds()); + DatabaseFactory.getUnknownStorageIdDatabase(context).getAllUnknownIds()); } private static @NonNull List buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull Collection ids) { diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalStorageModels.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalStorageModels.java index d390e808f..a8a419f19 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalStorageModels.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalStorageModels.java @@ -4,6 +4,7 @@ import com.google.protobuf.ByteString; import org.signal.zkgroup.groups.GroupMasterKey; import org.whispersystems.libsignal.InvalidKeyException; +import org.whispersystems.libsignal.logging.Log; import org.whispersystems.signalservice.internal.storage.protos.ManifestRecord; import org.whispersystems.signalservice.internal.storage.protos.StorageItem; import org.whispersystems.signalservice.internal.storage.protos.StorageManifest; @@ -15,6 +16,8 @@ import java.util.List; public final class SignalStorageModels { + private static final String TAG = SignalStorageModels.class.getSimpleName(); + public static SignalStorageManifest remoteToLocalStorageManifest(StorageManifest manifest, StorageKey storageKey) throws IOException, InvalidKeyException { byte[] rawRecord = SignalStorageCipher.decrypt(storageKey.deriveManifestKey(manifest.getVersion()), manifest.getValue().toByteArray()); ManifestRecord manifestRecord = ManifestRecord.parseFrom(rawRecord); @@ -42,6 +45,9 @@ public final class SignalStorageModels { } else if (record.hasAccount() && type == ManifestRecord.Identifier.Type.ACCOUNT_VALUE) { return SignalStorageRecord.forAccount(id, new SignalAccountRecord(id, record.getAccount())); } else { + if (StorageId.isKnownType(type)) { + Log.w(TAG, "StorageId is of known type (" + type + "), but the data is bad! Falling back to unknown."); + } return SignalStorageRecord.forUnknown(StorageId.forType(key, type)); } } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java index 02071fe63..c7f3701d0 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/StorageId.java @@ -53,13 +53,25 @@ public class StorageId { public static boolean isKnownType(int val) { for (ManifestRecord.Identifier.Type type : ManifestRecord.Identifier.Type.values()) { - if (type.getNumber() == val) { + if (type != ManifestRecord.Identifier.Type.UNRECOGNIZED && type.getNumber() == val) { return true; } } return false; } + public static int largestKnownType() { + int max = 0; + + for (ManifestRecord.Identifier.Type type : ManifestRecord.Identifier.Type.values()) { + if (type != ManifestRecord.Identifier.Type.UNRECOGNIZED) { + max = Math.max(type.getNumber(), max); + } + } + + return max; + } + @Override public boolean equals(Object o) { if (this == o) return true;