From b1925424279fda478e35f83dfe0ac936c80549f3 Mon Sep 17 00:00:00 2001 From: nielsandriesse Date: Mon, 19 Oct 2020 09:27:34 +1100 Subject: [PATCH 1/2] Fix various SSK race conditions --- .../database/helpers/SQLCipherOpenHelper.java | 12 +++-- .../loki/database/SharedSenderKeysDatabase.kt | 47 ++++++++++++++----- .../loki/protocol/ClosedGroupsProtocol.kt | 15 ++++++ 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java b/src/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java index 21ddb9f1a0..82d8bf0a06 100644 --- a/src/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java +++ b/src/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java @@ -91,8 +91,9 @@ public class SQLCipherOpenHelper extends SQLiteOpenHelper { private static final int lokiV12 = 33; private static final int lokiV13 = 34; private static final int lokiV14_BACKUP_FILES = 35; + private static final int lokiV15 = 36; - private static final int DATABASE_VERSION = lokiV14_BACKUP_FILES; // Loki - onUpgrade(...) must be updated to use Loki version numbers if Signal makes any database changes + private static final int DATABASE_VERSION = lokiV15; private static final String DATABASE_NAME = "signal.db"; private final Context context; @@ -164,7 +165,8 @@ public class SQLCipherOpenHelper extends SQLiteOpenHelper { db.execSQL(LokiUserDatabase.getCreateDisplayNameTableCommand()); db.execSQL(LokiUserDatabase.getCreateServerDisplayNameTableCommand()); db.execSQL(LokiBackupFilesDatabase.getCreateTableCommand()); - db.execSQL(SharedSenderKeysDatabase.getCreateClosedGroupRatchetTableCommand()); + db.execSQL(SharedSenderKeysDatabase.getCreateOldClosedGroupRatchetTableCommand()); + db.execSQL(SharedSenderKeysDatabase.getCreateCurrentClosedGroupRatchetTableCommand()); db.execSQL(SharedSenderKeysDatabase.getCreateClosedGroupPrivateKeyTableCommand()); executeStatements(db, SmsDatabase.CREATE_INDEXS); @@ -614,7 +616,7 @@ public class SQLCipherOpenHelper extends SQLiteOpenHelper { if (oldVersion < lokiV12) { db.execSQL(LokiAPIDatabase.getCreateLastMessageHashValueTable2Command()); - db.execSQL(SharedSenderKeysDatabase.getCreateClosedGroupRatchetTableCommand()); + db.execSQL(SharedSenderKeysDatabase.getCreateCurrentClosedGroupRatchetTableCommand()); db.execSQL(SharedSenderKeysDatabase.getCreateClosedGroupPrivateKeyTableCommand()); } @@ -626,6 +628,10 @@ public class SQLCipherOpenHelper extends SQLiteOpenHelper { db.execSQL(LokiBackupFilesDatabase.getCreateTableCommand()); } + if (oldVersion < lokiV15) { + db.execSQL(SharedSenderKeysDatabase.getCreateOldClosedGroupRatchetTableCommand()); + } + db.setTransactionSuccessful(); } finally { db.endTransaction(); diff --git a/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt b/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt index 548e717011..fd6fc8f170 100644 --- a/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt +++ b/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt @@ -7,6 +7,7 @@ import org.thoughtcrime.securesms.database.helpers.SQLCipherOpenHelper import org.thoughtcrime.securesms.loki.utilities.* import org.thoughtcrime.securesms.util.Hex import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupRatchet +import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupRatchetCollectionType import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupSenderKey import org.whispersystems.signalservice.loki.protocol.closedgroups.SharedSenderKeysDatabaseProtocol import org.whispersystems.signalservice.loki.utilities.PublicKeyValidation @@ -17,13 +18,18 @@ class SharedSenderKeysDatabase(context: Context, helper: SQLCipherOpenHelper) : // Shared private val closedGroupPublicKey = "closed_group_public_key" // Ratchets - private val closedGroupRatchetTable = "closed_group_ratchet_table" + private val oldClosedGroupRatchetTable = "old_closed_group_ratchet_table" + private val currentClosedGroupRatchetTable = "closed_group_ratchet_table" private val senderPublicKey = "sender_public_key" private val chainKey = "chain_key" private val keyIndex = "key_index" private val messageKeys = "message_keys" - @JvmStatic val createClosedGroupRatchetTableCommand - = "CREATE TABLE $closedGroupRatchetTable ($closedGroupPublicKey STRING, $senderPublicKey STRING, $chainKey STRING, " + + @JvmStatic val createOldClosedGroupRatchetTableCommand + = "CREATE TABLE $currentClosedGroupRatchetTable ($closedGroupPublicKey STRING, $senderPublicKey STRING, $chainKey STRING, " + + "$keyIndex INTEGER DEFAULT 0, $messageKeys TEXT, PRIMARY KEY ($closedGroupPublicKey, $senderPublicKey));" + // Private keys + @JvmStatic val createCurrentClosedGroupRatchetTableCommand + = "CREATE TABLE $currentClosedGroupRatchetTable ($closedGroupPublicKey STRING, $senderPublicKey STRING, $chainKey STRING, " + "$keyIndex INTEGER DEFAULT 0, $messageKeys TEXT, PRIMARY KEY ($closedGroupPublicKey, $senderPublicKey));" // Private keys private val closedGroupPrivateKeyTable = "closed_group_private_key_table" @@ -32,11 +38,18 @@ class SharedSenderKeysDatabase(context: Context, helper: SQLCipherOpenHelper) : = "CREATE TABLE $closedGroupPrivateKeyTable ($closedGroupPublicKey STRING PRIMARY KEY, $closedGroupPrivateKey STRING);" } + private fun getTable(collection: ClosedGroupRatchetCollectionType): String { + return when (collection) { + ClosedGroupRatchetCollectionType.Old -> oldClosedGroupRatchetTable + ClosedGroupRatchetCollectionType.Current -> currentClosedGroupRatchetTable + } + } + // region Ratchets & Sender Keys - override fun getClosedGroupRatchet(groupPublicKey: String, senderPublicKey: String): ClosedGroupRatchet? { + override fun getClosedGroupRatchet(groupPublicKey: String, senderPublicKey: String, collection: ClosedGroupRatchetCollectionType): ClosedGroupRatchet? { val database = databaseHelper.readableDatabase val query = "${Companion.closedGroupPublicKey} = ? AND ${Companion.senderPublicKey} = ?" - return database.get(closedGroupRatchetTable, query, arrayOf( groupPublicKey, senderPublicKey )) { cursor -> + return database.get(getTable(collection), query, arrayOf( groupPublicKey, senderPublicKey )) { cursor -> val chainKey = cursor.getString(Companion.chainKey) val keyIndex = cursor.getInt(Companion.keyIndex) val messageKeys = cursor.getString(Companion.messageKeys).split("-") @@ -44,7 +57,7 @@ class SharedSenderKeysDatabase(context: Context, helper: SQLCipherOpenHelper) : } } - override fun setClosedGroupRatchet(groupPublicKey: String, senderPublicKey: String, ratchet: ClosedGroupRatchet) { + override fun setClosedGroupRatchet(groupPublicKey: String, senderPublicKey: String, ratchet: ClosedGroupRatchet, collection: ClosedGroupRatchetCollectionType) { val database = databaseHelper.writableDatabase val values = ContentValues() values.put(Companion.closedGroupPublicKey, groupPublicKey) @@ -53,23 +66,33 @@ class SharedSenderKeysDatabase(context: Context, helper: SQLCipherOpenHelper) : values.put(Companion.keyIndex, ratchet.keyIndex) values.put(Companion.messageKeys, ratchet.messageKeys.joinToString("-")) val query = "${Companion.closedGroupPublicKey} = ? AND ${Companion.senderPublicKey} = ?" - database.insertOrUpdate(closedGroupRatchetTable, values, query, arrayOf( groupPublicKey, senderPublicKey )) + database.insertOrUpdate(getTable(collection), values, query, arrayOf( groupPublicKey, senderPublicKey )) } - override fun removeAllClosedGroupRatchets(groupPublicKey: String) { + override fun removeAllClosedGroupRatchets(groupPublicKey: String, collection: ClosedGroupRatchetCollectionType) { val database = databaseHelper.writableDatabase val query = "${Companion.closedGroupPublicKey} = ?" - database.delete(closedGroupRatchetTable, query, arrayOf( groupPublicKey )) + database.delete(getTable(collection), query, arrayOf( groupPublicKey )) } - override fun getAllClosedGroupSenderKeys(groupPublicKey: String): Set { + override fun getAllClosedGroupRatchets(groupPublicKey: String, collection: ClosedGroupRatchetCollectionType): Set> { val database = databaseHelper.readableDatabase val query = "${Companion.closedGroupPublicKey} = ?" - return database.getAll(closedGroupRatchetTable, query, arrayOf( groupPublicKey )) { cursor -> + return database.getAll(getTable(collection), query, arrayOf( groupPublicKey )) { cursor -> val chainKey = cursor.getString(Companion.chainKey) val keyIndex = cursor.getInt(Companion.keyIndex) + val messageKeys = cursor.getString(Companion.messageKeys).split("-") val senderPublicKey = cursor.getString(Companion.senderPublicKey) - ClosedGroupSenderKey(Hex.fromStringCondensed(chainKey), keyIndex, Hex.fromStringCondensed(senderPublicKey)) + val ratchet = ClosedGroupRatchet(chainKey, keyIndex, messageKeys) + Pair(senderPublicKey, ratchet) + }.toSet() + } + + override fun getAllClosedGroupSenderKeys(groupPublicKey: String, collection: ClosedGroupRatchetCollectionType): Set { + return getAllClosedGroupRatchets(groupPublicKey, collection).map { pair -> + val senderPublicKey = pair.first + val ratchet = pair.second + ClosedGroupSenderKey(Hex.fromStringCondensed(ratchet.chainKey), ratchet.keyIndex, Hex.fromStringCondensed(senderPublicKey)) }.toSet() } // endregion diff --git a/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt b/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt index 9050e39a80..5bd50ed233 100644 --- a/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt +++ b/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt @@ -27,6 +27,7 @@ import org.whispersystems.signalservice.internal.push.SignalServiceProtos import org.whispersystems.signalservice.internal.push.SignalServiceProtos.GroupContext import org.whispersystems.signalservice.loki.api.SnodeAPI import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupRatchet +import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupRatchetCollectionType import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupSenderKey import org.whispersystems.signalservice.loki.protocol.closedgroups.SharedSenderKeysImplementation import org.whispersystems.signalservice.loki.utilities.hexEncodedPrivateKey @@ -151,6 +152,13 @@ object ClosedGroupsProtocol { job.setContext(context) job.onRun() // Run the job immediately } + val allOldRatchets = sskDatabase.getAllClosedGroupRatchets(groupPublicKey, ClosedGroupRatchetCollectionType.Current) + for (pair in allOldRatchets) { + val senderPublicKey = pair.first + val ratchet = pair.second + val collection = ClosedGroupRatchetCollectionType.Old + sskDatabase.setClosedGroupRatchet(groupPublicKey, senderPublicKey, ratchet, collection) + } // Delete all ratchets (it's important that this happens * after * sending out the update) sskDatabase.removeAllClosedGroupRatchets(groupPublicKey) // Remove the group from the user's set of public keys to poll for if the user is leaving. Otherwise generate a new ratchet and @@ -366,6 +374,13 @@ object ClosedGroupsProtocol { val wasAnyUserRemoved = members.toSet().intersect(oldMembers) != oldMembers.toSet() val wasSenderRemoved = !members.contains(senderPublicKey) if (wasAnyUserRemoved) { + val allOldRatchets = sskDatabase.getAllClosedGroupRatchets(groupPublicKey, ClosedGroupRatchetCollectionType.Current) + for (pair in allOldRatchets) { + @Suppress("NAME_SHADOWING") val senderPublicKey = pair.first + val ratchet = pair.second + val collection = ClosedGroupRatchetCollectionType.Old + sskDatabase.setClosedGroupRatchet(groupPublicKey, senderPublicKey, ratchet, collection) + } sskDatabase.removeAllClosedGroupRatchets(groupPublicKey) if (wasCurrentUserRemoved) { sskDatabase.removeClosedGroupPrivateKey(groupPublicKey) From 9fd7b5802f809b584d1d5590320a1b54d4e135c9 Mon Sep 17 00:00:00 2001 From: nielsandriesse Date: Mon, 19 Oct 2020 09:35:25 +1100 Subject: [PATCH 2/2] Debug --- .../loki/database/SharedSenderKeysDatabase.kt | 2 +- .../loki/protocol/ClosedGroupsProtocol.kt | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt b/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt index fd6fc8f170..d87f212d5e 100644 --- a/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt +++ b/src/org/thoughtcrime/securesms/loki/database/SharedSenderKeysDatabase.kt @@ -25,7 +25,7 @@ class SharedSenderKeysDatabase(context: Context, helper: SQLCipherOpenHelper) : private val keyIndex = "key_index" private val messageKeys = "message_keys" @JvmStatic val createOldClosedGroupRatchetTableCommand - = "CREATE TABLE $currentClosedGroupRatchetTable ($closedGroupPublicKey STRING, $senderPublicKey STRING, $chainKey STRING, " + + = "CREATE TABLE $oldClosedGroupRatchetTable ($closedGroupPublicKey STRING, $senderPublicKey STRING, $chainKey STRING, " + "$keyIndex INTEGER DEFAULT 0, $messageKeys TEXT, PRIMARY KEY ($closedGroupPublicKey, $senderPublicKey));" // Private keys @JvmStatic val createCurrentClosedGroupRatchetTableCommand diff --git a/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt b/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt index 5bd50ed233..bed24db122 100644 --- a/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt +++ b/src/org/thoughtcrime/securesms/loki/protocol/ClosedGroupsProtocol.kt @@ -160,7 +160,7 @@ object ClosedGroupsProtocol { sskDatabase.setClosedGroupRatchet(groupPublicKey, senderPublicKey, ratchet, collection) } // Delete all ratchets (it's important that this happens * after * sending out the update) - sskDatabase.removeAllClosedGroupRatchets(groupPublicKey) + sskDatabase.removeAllClosedGroupRatchets(groupPublicKey, ClosedGroupRatchetCollectionType.Current) // Remove the group from the user's set of public keys to poll for if the user is leaving. Otherwise generate a new ratchet and // send it out to all members (minus the removed ones) using established channels. if (isUserLeaving) { @@ -205,7 +205,7 @@ object ClosedGroupsProtocol { // Establish sessions if needed establishSessionsWithMembersIfNeeded(context, newMembers) // Send closed group update messages to the new members using established channels - var allSenderKeys = sskDatabase.getAllClosedGroupSenderKeys(groupPublicKey); + var allSenderKeys = sskDatabase.getAllClosedGroupSenderKeys(groupPublicKey, ClosedGroupRatchetCollectionType.Current) allSenderKeys = allSenderKeys.union(newSenderKeys) for (member in newMembers) { @Suppress("NAME_SHADOWING") @@ -216,7 +216,7 @@ object ClosedGroupsProtocol { ApplicationContext.getInstance(context).jobManager.add(job) } } else { - val allSenderKeys = sskDatabase.getAllClosedGroupSenderKeys(groupPublicKey); + val allSenderKeys = sskDatabase.getAllClosedGroupSenderKeys(groupPublicKey, ClosedGroupRatchetCollectionType.Current) val closedGroupUpdateKind = ClosedGroupUpdateMessageSendJob.Kind.Info(Hex.fromStringCondensed(groupPublicKey), name, allSenderKeys, membersAsData, adminsAsData) val job = ClosedGroupUpdateMessageSendJob(groupPublicKey, closedGroupUpdateKind) @@ -295,7 +295,7 @@ object ClosedGroupsProtocol { senderKeys.forEach { senderKey -> if (!members.contains(senderKey.publicKey.toHexString())) { return@forEach } val ratchet = ClosedGroupRatchet(senderKey.chainKey.toHexString(), senderKey.keyIndex, listOf()) - sskDatabase.setClosedGroupRatchet(groupPublicKey, senderKey.publicKey.toHexString(), ratchet) + sskDatabase.setClosedGroupRatchet(groupPublicKey, senderKey.publicKey.toHexString(), ratchet, ClosedGroupRatchetCollectionType.Current) } // Sort out any discrepancies between the provided sender keys and what's required val missingSenderKeys = members.toSet().subtract(senderKeys.map { Hex.toStringCondensed(it.publicKey) }) @@ -365,7 +365,7 @@ object ClosedGroupsProtocol { // Store the ratchets for any new members (it's important that this happens before the code below) senderKeys.forEach { senderKey -> val ratchet = ClosedGroupRatchet(senderKey.chainKey.toHexString(), senderKey.keyIndex, listOf()) - sskDatabase.setClosedGroupRatchet(groupPublicKey, senderKey.publicKey.toHexString(), ratchet) + sskDatabase.setClosedGroupRatchet(groupPublicKey, senderKey.publicKey.toHexString(), ratchet, ClosedGroupRatchetCollectionType.Current) } // Delete all ratchets and either: // • Send out the user's new ratchet using established channels if other members of the group left or were removed @@ -381,7 +381,7 @@ object ClosedGroupsProtocol { val collection = ClosedGroupRatchetCollectionType.Old sskDatabase.setClosedGroupRatchet(groupPublicKey, senderPublicKey, ratchet, collection) } - sskDatabase.removeAllClosedGroupRatchets(groupPublicKey) + sskDatabase.removeAllClosedGroupRatchets(groupPublicKey, ClosedGroupRatchetCollectionType.Current) if (wasCurrentUserRemoved) { sskDatabase.removeClosedGroupPrivateKey(groupPublicKey) groupDB.setActive(groupID, false) @@ -431,7 +431,7 @@ object ClosedGroupsProtocol { // Respond to the request Log.d("Loki", "Responding to sender key request from: $senderPublicKey.") ApplicationContext.getInstance(context).sendSessionRequestIfNeeded(senderPublicKey) - val userRatchet = DatabaseFactory.getSSKDatabase(context).getClosedGroupRatchet(groupPublicKey, userPublicKey) + val userRatchet = DatabaseFactory.getSSKDatabase(context).getClosedGroupRatchet(groupPublicKey, userPublicKey, ClosedGroupRatchetCollectionType.Current) ?: SharedSenderKeysImplementation.shared.generateRatchet(groupPublicKey, userPublicKey) val userSenderKey = ClosedGroupSenderKey(Hex.fromStringCondensed(userRatchet.chainKey), userRatchet.keyIndex, Hex.fromStringCondensed(userPublicKey)) val closedGroupUpdateKind = ClosedGroupUpdateMessageSendJob.Kind.SenderKey(Hex.fromStringCondensed(groupPublicKey), userSenderKey) @@ -456,7 +456,7 @@ object ClosedGroupsProtocol { // Store the sender key Log.d("Loki", "Received a sender key from: $senderPublicKey.") val ratchet = ClosedGroupRatchet(senderKey.chainKey.toHexString(), senderKey.keyIndex, listOf()) - sskDatabase.setClosedGroupRatchet(groupPublicKey, senderPublicKey, ratchet) + sskDatabase.setClosedGroupRatchet(groupPublicKey, senderPublicKey, ratchet, ClosedGroupRatchetCollectionType.Current) } @JvmStatic