From 0dbab7ede0388abeb7d1e3f11848f74451aa0ff0 Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Wed, 8 Jan 2025 16:59:44 -0500 Subject: [PATCH] Mitigate PNI editor server bug for group member add updates. --- .../helpers/SignalDatabaseMigrations.kt | 6 +- .../migration/V264_FixGroupAddMemberUpdate.kt | 78 +++++++++++++++++++ .../model/GroupsV2UpdateMessageConverter.kt | 7 +- .../migrations/ApplicationMigrations.java | 7 +- .../api/groupsv2/GroupsV2Operations.java | 24 +++++- 5 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V264_FixGroupAddMemberUpdate.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt index 0389a700f7..4863bc9746 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt @@ -119,6 +119,7 @@ import org.thoughtcrime.securesms.database.helpers.migration.V259_AdjustNotifica import org.thoughtcrime.securesms.database.helpers.migration.V260_RemapQuoteAuthors import org.thoughtcrime.securesms.database.helpers.migration.V261_RemapCallRingers import org.thoughtcrime.securesms.database.helpers.migration.V263_InAppPaymentsSubscriberTableRebuild +import org.thoughtcrime.securesms.database.helpers.migration.V264_FixGroupAddMemberUpdate /** * Contains all of the database migrations for [SignalDatabase]. Broken into a separate file for cleanliness. @@ -241,10 +242,11 @@ object SignalDatabaseMigrations { 260 to V260_RemapQuoteAuthors, 261 to V261_RemapCallRingers, // V263 was originally V262, but a typo in the version mapping caused it not to be run. - 263 to V263_InAppPaymentsSubscriberTableRebuild + 263 to V263_InAppPaymentsSubscriberTableRebuild, + 264 to V264_FixGroupAddMemberUpdate ) - const val DATABASE_VERSION = 263 + const val DATABASE_VERSION = 264 @JvmStatic fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V264_FixGroupAddMemberUpdate.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V264_FixGroupAddMemberUpdate.kt new file mode 100644 index 0000000000..14d61d01c8 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V264_FixGroupAddMemberUpdate.kt @@ -0,0 +1,78 @@ +/* + * Copyright 2024 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.database.helpers.migration + +import android.app.Application +import androidx.core.content.contentValuesOf +import net.zetetic.database.sqlcipher.SQLiteDatabase +import okio.IOException +import org.signal.core.util.forEach +import org.signal.core.util.logging.Log +import org.signal.core.util.requireBlob +import org.signal.core.util.requireLong +import org.thoughtcrime.securesms.backup.v2.proto.GroupMemberAddedUpdate +import org.thoughtcrime.securesms.database.model.databaseprotos.MessageExtras +import org.whispersystems.signalservice.api.push.ServiceId + +/** + * Ensure we store ACIs only in the ACI fields for [GroupMemberAddedUpdate.updaterAci] in [GroupMemberAddedUpdate]. + */ +@Suppress("ClassName") +object V264_FixGroupAddMemberUpdate : SignalDatabaseMigration { + + private val TAG = Log.tag(V264_FixGroupAddMemberUpdate::class) + + override fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + val messageExtrasFixes = mutableListOf>() + + db.query("message", arrayOf("_id", "message_extras"), "message_extras IS NOT NULL AND type & 0x10000 != 0", null, null, null, null) + .forEach { cursor -> + val blob = cursor.requireBlob("message_extras")!! + + val messageExtras: MessageExtras? = try { + MessageExtras.ADAPTER.decode(blob) + } catch (e: IOException) { + Log.w(TAG, "Unable to decode message extras", e) + null + } + + if (messageExtras?.gv2UpdateDescription?.groupChangeUpdate?.updates?.any { it.groupMemberAddedUpdate != null } != true) { + return@forEach + } + + val groupUpdateDescription = messageExtras.gv2UpdateDescription + val groupUpdate = groupUpdateDescription.groupChangeUpdate!! + val updates = groupUpdate.updates.toMutableList() + var dataMigrated = false + + updates + .replaceAll { change -> + if (change.groupMemberAddedUpdate != null && ServiceId.parseOrNull(change.groupMemberAddedUpdate.updaterAci) is ServiceId.PNI) { + dataMigrated = true + change.copy(groupMemberAddedUpdate = change.groupMemberAddedUpdate.copy(updaterAci = null)) + } else { + change + } + } + + if (dataMigrated) { + val updatedMessageExtras = messageExtras.copy( + gv2UpdateDescription = groupUpdateDescription.copy( + groupChangeUpdate = groupUpdate.copy( + updates = updates + ) + ) + ) + + messageExtrasFixes += cursor.requireLong("_id") to updatedMessageExtras.encode() + } + } + + messageExtrasFixes.forEach { (id, extras) -> + db.update("message", contentValuesOf("message_extras" to extras), "_id = $id", null) + } + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageConverter.kt b/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageConverter.kt index ae52e61b86..7a043c5940 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageConverter.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageConverter.kt @@ -6,7 +6,6 @@ package org.thoughtcrime.securesms.database.model import okio.ByteString -import okio.ByteString.Companion.toByteString import org.signal.core.util.StringUtil import org.signal.core.util.isNullOrEmpty import org.signal.storageservice.protos.groups.AccessControl @@ -136,7 +135,7 @@ object GroupsV2UpdateMessageConverter { if (editorServiceId == null || editorServiceId.isUnknown) { editorUnknown = true } - translateMemberAdditions(change, editorUnknown, updates) + translateMemberAdditions(change, editorUnknown, editorServiceId, updates) translateModifyMemberRoles(change, editorUnknown, updates) translateInvitations(selfIds, change, editorUnknown, updates) translateRevokedInvitations(selfIds, change, editorUnknown, updates) @@ -161,7 +160,7 @@ object GroupsV2UpdateMessageConverter { } @JvmStatic - fun translateMemberAdditions(change: DecryptedGroupChange, editorUnknown: Boolean, updates: MutableList) { + fun translateMemberAdditions(change: DecryptedGroupChange, editorUnknown: Boolean, editorServiceId: ServiceId?, updates: MutableList) { for (member in change.newMembers) { if (!editorUnknown && member.aciBytes == change.editorServiceIdBytes) { updates.add( @@ -173,7 +172,7 @@ object GroupsV2UpdateMessageConverter { updates.add( GroupChangeChatUpdate.Update( groupMemberAddedUpdate = GroupMemberAddedUpdate( - updaterAci = if (editorUnknown) null else change.editorServiceIdBytes, + updaterAci = if (editorUnknown || editorServiceId is ServiceId.PNI) null else change.editorServiceIdBytes, newMemberAci = member.aciBytes, hadOpenInvitation = false ) diff --git a/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java b/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java index 225a105b4b..49f848f65f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java +++ b/app/src/main/java/org/thoughtcrime/securesms/migrations/ApplicationMigrations.java @@ -166,9 +166,10 @@ public class ApplicationMigrations { static final int QUOTE_AUTHOR_FIX = 122; static final int BAD_E164_FIX = 123; static final int GPB_TOKEN_MIGRATION = 124; + static final int GROUP_ADD_MIGRATION = 125; } - public static final int CURRENT_VERSION = 124; + public static final int CURRENT_VERSION = 125; /** * This *must* be called after the {@link JobManager} has been instantiated, but *before* the call @@ -763,6 +764,10 @@ public class ApplicationMigrations { jobs.put(Version.GPB_TOKEN_MIGRATION, new GooglePlayBillingPurchaseTokenMigrationJob()); } + if (lastSeenVersion < Version.GROUP_ADD_MIGRATION) { + jobs.put(Version.GROUP_ADD_MIGRATION, new DatabaseMigrationJob()); + } + return jobs; } 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 0f71acf647..3d44b1f354 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 @@ -524,13 +524,15 @@ public final class GroupsV2Operations { throws VerificationFailedException, InvalidGroupStateException { DecryptedGroupChange.Builder builder = new DecryptedGroupChange.Builder(); + ServiceId editorServiceId; // Field 1 if (source != null) { - builder.editorServiceIdBytes(source.toByteString()); + editorServiceId = source; } else { - builder.editorServiceIdBytes(decryptServiceIdToBinary(actions.sourceServiceId)); + editorServiceId = decryptServiceId(actions.sourceServiceId); } + builder.editorServiceIdBytes(editorServiceId.toByteString()); // Field 2 builder.revision(actions.revision); @@ -752,6 +754,24 @@ public final class GroupsV2Operations { } builder.promotePendingPniAciMembers(promotePendingPniAciMembers); + if (editorServiceId instanceof ServiceId.PNI) { + if (actions.addMembers.size() == 1 && builder.newMembers.size() == 1) { + GroupChange.Actions.AddMemberAction addMemberAction = actions.addMembers.get(0); + DecryptedMember decryptedMember = builder.newMembers.get(0); + + if (addMemberAction.joinFromInviteLink) { + Log.d(TAG, "Replacing PNI editor with ACI for buggy join from invite link"); + builder.editorServiceIdBytes(decryptedMember.aciBytes); + } else { + Log.w(TAG, "Unable to replace PNI editor with ACI for add member update"); + builder.editorServiceIdBytes(ByteString.EMPTY); + } + } else if (actions.deletePendingMembers.isEmpty() && actions.promotePendingPniAciMembers.isEmpty()) { + Log.w(TAG, "Received group change with PNI editor for a non-PNI editor eligible update, clearing editor"); + builder.editorServiceIdBytes(ByteString.EMPTY); + } + } + return builder.build(); }