From 9c829e62e61479aab195c34f0f2446b444183dec Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 28 Jan 2020 15:10:40 +0100 Subject: [PATCH] QrCode: WIP --- .../DefaultCrossSigningService.kt | 3 +- .../DefaultVerificationService.kt | 86 +++++++++++-------- .../SASDefaultVerificationTransaction.kt | 5 +- .../DefaultQrCodeVerificationTransaction.kt | 65 ++++++++++---- .../android/internal/util/StringUtils.kt | 2 + 5 files changed, 108 insertions(+), 53 deletions(-) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/DefaultCrossSigningService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/DefaultCrossSigningService.kt index ca281ac7ef..9df145b0ef 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/DefaultCrossSigningService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/crosssigning/DefaultCrossSigningService.kt @@ -40,6 +40,7 @@ import im.vector.matrix.android.internal.task.TaskExecutor import im.vector.matrix.android.internal.task.configureWith import im.vector.matrix.android.internal.util.JsonCanonicalizer import im.vector.matrix.android.internal.util.MatrixCoroutineDispatchers +import im.vector.matrix.android.internal.util.withoutPrefix import kotlinx.coroutines.CoroutineScope import org.matrix.olm.OlmPkSigning import org.matrix.olm.OlmUtility @@ -384,7 +385,7 @@ internal class DefaultCrossSigningService @Inject constructor( } else { // Maybe it's signed by a locally trusted device? myMasterKey.signatures?.get(userId)?.forEach { (key, value) -> - val potentialDeviceId = if (key.startsWith("ed25519:")) key.substring("ed25519:".length) else key + val potentialDeviceId = key.withoutPrefix("ed25519:") val potentialDevice = cryptoStore.getUserDevice(userId, potentialDeviceId) if (potentialDevice != null && potentialDevice.isVerified) { // Check signature validity? diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt index 14918611ab..2fa32b1b98 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/DefaultVerificationService.kt @@ -24,6 +24,7 @@ import im.vector.matrix.android.api.session.crypto.CryptoService import im.vector.matrix.android.api.session.crypto.crosssigning.CrossSigningService import im.vector.matrix.android.api.session.crypto.sas.CancelCode import im.vector.matrix.android.api.session.crypto.sas.QrCodeVerificationTransaction +import im.vector.matrix.android.api.session.crypto.sas.SasVerificationTransaction import im.vector.matrix.android.api.session.crypto.sas.VerificationMethod import im.vector.matrix.android.api.session.crypto.sas.VerificationService import im.vector.matrix.android.api.session.crypto.sas.VerificationTransaction @@ -372,30 +373,46 @@ internal class DefaultVerificationService @Inject constructor( } } + /** + * Return a CancelCode to make the caller cancel the verification. Else return null + */ private suspend fun handleStart(otherUserId: String?, startReq: VerificationInfoStart, txConfigure: (DefaultVerificationTransaction) -> Unit): CancelCode? { - Timber.d("## SAS onStartRequestReceived ${startReq.transactionID!!}") + Timber.d("## SAS onStartRequestReceived ${startReq.transactionID}") if (checkKeysAreDownloaded(otherUserId!!, startReq.fromDevice ?: "") != null) { - Timber.v("## SAS onStartRequestReceived $startReq") val tid = startReq.transactionID!! val existing = getExistingTransaction(otherUserId, tid) - val existingTxs = getExistingTransactionsForUser(otherUserId) - if (existing != null) { - // should cancel both! - Timber.v("## SAS onStartRequestReceived - Request exist with same if ${startReq.transactionID!!}") - existing.cancel(CancelCode.UnexpectedMessage) - return CancelCode.UnexpectedMessage - // cancelTransaction(tid, otherUserId, startReq.fromDevice!!, CancelCode.UnexpectedMessage) - } else if (existingTxs?.isEmpty() == false) { - Timber.v("## SAS onStartRequestReceived - There is already a transaction with this user ${startReq.transactionID!!}") - // Multiple keyshares between two devices: any two devices may only have at most one key verification in flight at a time. - existingTxs.forEach { - it.cancel(CancelCode.UnexpectedMessage) - } - return CancelCode.UnexpectedMessage - // cancelTransaction(tid, otherUserId, startReq.fromDevice!!, CancelCode.UnexpectedMessage) - } else { - // Ok we can create - if (startReq.method == VERIFICATION_METHOD_SAS) { + + when (startReq.method) { + VERIFICATION_METHOD_SAS -> { + when (existing) { + is SasVerificationTransaction -> { + // should cancel both! + Timber.v("## SAS onStartRequestReceived - Request exist with same id ${startReq.transactionID}") + existing.cancel(CancelCode.UnexpectedMessage) + // Already cancelled, so return null + return null + } + is QrCodeVerificationTransaction -> { + // Nothing to do? + } + null -> { + getExistingTransactionsForUser(otherUserId) + ?.filterIsInstance(SasVerificationTransaction::class.java) + ?.takeIf { it.isNotEmpty() } + ?.also { + // Multiple keyshares between two devices: any two devices may only have at most one key verification in flight at a time. + Timber.v("## SAS onStartRequestReceived - There is already a transaction with this user ${startReq.transactionID}") + } + ?.forEach { + it.cancel(CancelCode.UnexpectedMessage) + } + ?.also { + return CancelCode.UnexpectedMessage + } + } + } + + // Ok we can create a SAS transaction Timber.v("## SAS onStartRequestReceived - request accepted ${startReq.transactionID!!}") // If there is a corresponding request, we can auto accept // as we are the one requesting in first place (or we accepted the request) @@ -414,40 +431,39 @@ internal class DefaultVerificationService @Inject constructor( autoAccept).also { txConfigure(it) } addTransaction(tx) tx.acceptVerificationEvent(otherUserId, startReq) - } else if (startReq.method == VERIFICATION_METHOD_RECIPROCATE) { + return null + } + VERIFICATION_METHOD_RECIPROCATE -> { // Other user has scanned my QR code - val pendingTransaction = getExistingTransaction(otherUserId, startReq.transactionID!!) - - if (pendingTransaction != null && pendingTransaction is DefaultQrCodeVerificationTransaction) { - pendingTransaction.onStartReceived(startReq) + if (existing is DefaultQrCodeVerificationTransaction) { + existing.onStartReceived(startReq) + return null } else { - Timber.w("## SAS onStartRequestReceived - unknown transaction ${startReq.transactionID}") - return CancelCode.UnknownTransaction + Timber.w("## SAS onStartRequestReceived - unexpected message ${startReq.transactionID}") + return CancelCode.UnexpectedMessage } - } else { + } + else ->{ Timber.e("## SAS onStartRequestReceived - unknown method ${startReq.method}") return CancelCode.UnknownMethod - // cancelTransaction(tid, otherUserId, startReq.fromDevice -// ?: event.getSenderKey()!!, CancelCode.UnknownMethod) } } } else { return CancelCode.UnexpectedMessage -// cancelTransaction(startReq.transactionID!!, otherUserId, startReq.fromDevice!!, CancelCode.UnexpectedMessage) } - return null } + // TODO Refacto: It could just return a boolean private suspend fun checkKeysAreDownloaded(otherUserId: String, - fromDevice: String): MXUsersDevicesMap? { + otherDeviceId: String): MXUsersDevicesMap? { return try { var keys = deviceListManager.downloadKeys(listOf(otherUserId), false) - if (keys.getUserDeviceIds(otherUserId)?.contains(fromDevice) == true) { + if (keys.getUserDeviceIds(otherUserId)?.contains(otherDeviceId) == true) { return keys } else { // force download keys = deviceListManager.downloadKeys(listOf(otherUserId), true) - return keys.takeIf { keys.getUserDeviceIds(otherUserId)?.contains(fromDevice) == true } + return keys.takeIf { keys.getUserDeviceIds(otherUserId)?.contains(otherDeviceId) == true } } } catch (e: Exception) { null diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/SASDefaultVerificationTransaction.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/SASDefaultVerificationTransaction.kt index 880fef0662..a879cdb797 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/SASDefaultVerificationTransaction.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/SASDefaultVerificationTransaction.kt @@ -30,6 +30,7 @@ import im.vector.matrix.android.internal.crypto.model.MXKey import im.vector.matrix.android.internal.crypto.model.rest.SignatureUploadResponse import im.vector.matrix.android.internal.crypto.store.IMXCryptoStore import im.vector.matrix.android.internal.extensions.toUnsignedInt +import im.vector.matrix.android.internal.util.withoutPrefix import org.matrix.olm.OlmSAS import org.matrix.olm.OlmUtility import timber.log.Timber @@ -253,7 +254,7 @@ internal abstract class SASDefaultVerificationTransaction( // cannot be empty because it has been validated theirMac!!.mac!!.keys.forEach { - val keyIDNoPrefix = if (it.startsWith("ed25519:")) it.substring("ed25519:".length) else it + val keyIDNoPrefix = it.withoutPrefix("ed25519:") val otherDeviceKey = otherUserKnownDevices?.get(keyIDNoPrefix)?.fingerprint() if (otherDeviceKey == null) { Timber.w("## SAS Verification: Could not find device $keyIDNoPrefix to verify") @@ -276,7 +277,7 @@ internal abstract class SASDefaultVerificationTransaction( if (otherCrossSigningMasterKeyPublic != null) { // Did the user signed his master key theirMac!!.mac!!.keys.forEach { - val keyIDNoPrefix = if (it.startsWith("ed25519:")) it.substring("ed25519:".length) else it + val keyIDNoPrefix = it.withoutPrefix("ed25519:") if (keyIDNoPrefix == otherCrossSigningMasterKeyPublic) { // Check the signature val mac = macUsingAgreedMethod(otherCrossSigningMasterKeyPublic, baseInfo + it) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/DefaultQrCodeVerificationTransaction.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/DefaultQrCodeVerificationTransaction.kt index 5ef2a787a5..6f05571b25 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/DefaultQrCodeVerificationTransaction.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/DefaultQrCodeVerificationTransaction.kt @@ -29,6 +29,7 @@ import im.vector.matrix.android.internal.crypto.store.IMXCryptoStore import im.vector.matrix.android.internal.crypto.verification.DefaultVerificationTransaction import im.vector.matrix.android.internal.crypto.verification.VerificationInfo import im.vector.matrix.android.internal.crypto.verification.VerificationInfoStart +import im.vector.matrix.android.internal.util.withoutPrefix import timber.log.Timber import kotlin.properties.Delegates @@ -89,19 +90,53 @@ internal class DefaultQrCodeVerificationTransaction( return } - val verifiedDeviceIds = mutableListOf() + val toVerifyDeviceIds = mutableListOf() + var canTrustOtherUserMasterKey = false val otherDevices = cryptoStore.getUserDevices(otherUserId) otherQrCodeData.keys.keys.forEach { key -> Timber.w("Checking key $key") - val fingerprint = otherDevices?.get(key)?.fingerprint() - if (fingerprint != null && fingerprint != otherQrCodeData.keys[key]) { - cancel(CancelCode.MismatchedKeys) - return - } else { - // Store the deviceId to verify after - verifiedDeviceIds.add(key) + + when (val keyNoPrefix = key.withoutPrefix("ed25519:")) { + otherQrCodeData.keys[key] -> { + // Maybe master key? + if (otherQrCodeData.keys[key] == crossSigningService.getUserCrossSigningKeys(otherUserId)?.masterKey()?.unpaddedBase64PublicKey) { + canTrustOtherUserMasterKey = true + } else { + cancel(CancelCode.MismatchedKeys) + return + } + } + else -> { + when (val otherDevice = otherDevices?.get(keyNoPrefix)) { + null -> { + // Unknown device, ignore + } + else -> { + when (otherDevice.fingerprint()) { + null -> { + // Ignore + } + otherQrCodeData.keys[key] -> { + // Store the deviceId to verify after + toVerifyDeviceIds.add(key) + } + else -> { + cancel(CancelCode.MismatchedKeys) + return + } + } + } + } + } } + + } + + if (!canTrustOtherUserMasterKey && toVerifyDeviceIds.isEmpty()) { + // Nothing to verify + cancel(CancelCode.MismatchedKeys) + return } // All checks are correct @@ -110,7 +145,7 @@ internal class DefaultQrCodeVerificationTransaction( start(otherQrCodeData.sharedSecret) // Trust the other user - trust(verifiedDeviceIds) + trust(canTrustOtherUserMasterKey, toVerifyDeviceIds) } fun start(remoteSecret: String) { @@ -161,16 +196,16 @@ internal class DefaultQrCodeVerificationTransaction( if (startReq.sharedSecret == qrCodeData.sharedSecret) { // Ok, we can trust the other user // We can only trust the master key in this case - trust(listOf(qrCodeData.otherUserKey)) + trust(true, emptyList()) } else { // Display a warning - cancel(CancelCode.QrCodeInvalid) + cancel(CancelCode.MismatchedKeys) } } - private fun trust(verifiedDeviceIds: List) { + private fun trust(canTrustOtherUserMasterKey: Boolean, toVerifyDeviceIds: List) { // If not me sign his MSK and upload the signature - if (otherUserId != userId) { + if (otherUserId != userId && canTrustOtherUserMasterKey) { // we should trust this master key // And check verification MSK -> SSK? crossSigningService.trustUser(otherUserId, object : MatrixCallback { @@ -191,7 +226,7 @@ internal class DefaultQrCodeVerificationTransaction( } // TODO what if the otherDevice is not in this list? and should we - verifiedDeviceIds.forEach { + toVerifyDeviceIds.forEach { setDeviceVerified(otherUserId, it) } transport.done(transactionId) @@ -200,7 +235,7 @@ internal class DefaultQrCodeVerificationTransaction( private fun setDeviceVerified(userId: String, deviceId: String) { // TODO should not override cross sign status - setDeviceVerificationAction.handle(DeviceTrustLevel(false, true), + setDeviceVerificationAction.handle(DeviceTrustLevel(crossSigningVerified = false, locallyVerified = true), userId, deviceId) } diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/StringUtils.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/StringUtils.kt index f19bebe482..b3f240f23d 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/StringUtils.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/StringUtils.kt @@ -49,3 +49,5 @@ fun convertFromUTF8(s: String): String { s } } + +fun String.withoutPrefix(prefix: String) = if (startsWith(prefix)) substringAfter(prefix) else this