From fb98d6ef42463c3d63d0553febc294d9d7c45c8e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 Jan 2020 16:24:24 +0100 Subject: [PATCH] QRCode: add other_device_key field and make it optional, along with other_user_key --- .../DefaultVerificationService.kt | 125 ++++++++++++++++-- .../DefaultQrCodeVerificationTransaction.kt | 32 ++++- .../crypto/verification/qrcode/Extensions.kt | 21 ++- .../crypto/verification/qrcode/QrCodeData.kt | 9 +- .../crypto/verification/qrcode/QrCodeTest.kt | 74 +++++++++-- 5 files changed, 230 insertions(+), 31 deletions(-) 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 46355aecb5..cbf2c0cbce 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 @@ -733,7 +733,7 @@ internal class DefaultVerificationService @Inject constructor( // Check if other user is able to scan QR code ?.takeIf { it.contains(VERIFICATION_METHOD_QR_CODE_SCAN) } ?.let { - createQrCodeData(existingRequest.transactionId, existingRequest.otherUserId) + createQrCodeData(existingRequest.transactionId, existingRequest.otherUserId, readyReq.fromDevice) } if (readyReq.methods.orEmpty().contains(VERIFICATION_METHOD_RECIPROCATE)) { @@ -760,13 +760,25 @@ internal class DefaultVerificationService @Inject constructor( )) } - private fun createQrCodeData(transactionId: String?, otherUserId: String): QrCodeData? { - // Build the QR code URL - val requestEventId = transactionId ?: run { + private fun createQrCodeData(requestEventId: String?, otherUserId: String, otherDeviceId: String?): QrCodeData? { + requestEventId ?: run { Timber.w("## Unknown requestEventId") return null } + return when { + userId != otherUserId -> + createQrCodeDataForDistinctUser(requestEventId, otherUserId, otherDeviceId) + crossSigningService.isCrossSigningVerified() -> + // This is a self verification and I am the old device (Osborne2) + createQrCodeDataForVerifiedDevice(requestEventId, otherDeviceId) + else -> + // This is a self verification and I am the new device (Dynabook) + createQrCodeDataForUnVerifiedDevice(requestEventId, otherDeviceId) + } + } + + private fun createQrCodeDataForDistinctUser(requestEventId: String, otherUserId: String, otherDeviceId: String?): QrCodeData? { val myMasterKey = crossSigningService.getMyCrossSigningKeys() ?.masterKey() ?.unpaddedBase64PublicKey @@ -789,9 +801,17 @@ internal class DefaultVerificationService @Inject constructor( return null } - val myDeviceKey = myDeviceInfoHolder.get().myDevice.fingerprint()!! + val myDeviceKey = myDeviceInfoHolder.get().myDevice.fingerprint() + ?: run { + Timber.w("## Unable to get my fingerprint") + return null + } + + val otherDeviceKey = otherDeviceId + ?.let { + cryptoStore.getUserDevice(userId, otherDeviceId)?.fingerprint() + } - val generatedSharedSecret = generateSharedSecret() return QrCodeData( userId = userId, requestEventId = requestEventId, @@ -800,8 +820,95 @@ internal class DefaultVerificationService @Inject constructor( myMasterKey to myMasterKey, myDeviceId to myDeviceKey ), - sharedSecret = generatedSharedSecret, - otherUserKey = otherUserMasterKey + sharedSecret = generateSharedSecret(), + otherUserKey = otherUserMasterKey, + otherDeviceKey = otherDeviceKey + ) + } + + // Create a QR code to display on the old device (Osborne2) + private fun createQrCodeDataForVerifiedDevice(requestEventId: String, otherDeviceId: String?): QrCodeData? { + val myMasterKey = crossSigningService.getMyCrossSigningKeys() + ?.masterKey() + ?.unpaddedBase64PublicKey + ?: run { + Timber.w("## Unable to get my master key") + return null + } + + val otherDeviceKey = otherDeviceId + ?.let { + cryptoStore.getUserDevice(userId, otherDeviceId)?.fingerprint() + } + ?: run { + Timber.w("## Unable to get other device data") + return null + } + + val myDeviceId = deviceId + ?: run { + Timber.w("## Unable to get my deviceId") + return null + } + + val myDeviceKey = myDeviceInfoHolder.get().myDevice.fingerprint() + ?: run { + Timber.w("## Unable to get my fingerprint") + return null + } + + return QrCodeData( + userId = userId, + requestEventId = requestEventId, + action = QrCodeData.ACTION_VERIFY, + keys = hashMapOf( + myMasterKey to myMasterKey, + myDeviceId to myDeviceKey + ), + sharedSecret = generateSharedSecret(), + otherUserKey = null, + otherDeviceKey = otherDeviceKey + ) + } + + // Create a QR code to display on the new device (Dynabook) + private fun createQrCodeDataForUnVerifiedDevice(requestEventId: String, otherDeviceId: String?): QrCodeData? { + val myMasterKey = crossSigningService.getMyCrossSigningKeys() + ?.masterKey() + ?.unpaddedBase64PublicKey + ?: run { + Timber.w("## Unable to get my master key") + return null + } + + val myDeviceId = deviceId + ?: run { + Timber.w("## Unable to get my deviceId") + return null + } + + val myDeviceKey = myDeviceInfoHolder.get().myDevice.fingerprint() + ?: run { + Timber.w("## Unable to get my fingerprint") + return null + } + + val otherDeviceKey = otherDeviceId + ?.let { + cryptoStore.getUserDevice(userId, otherDeviceId)?.fingerprint() + } + + return QrCodeData( + userId = userId, + requestEventId = requestEventId, + action = QrCodeData.ACTION_VERIFY, + keys = hashMapOf( + // Note: no master key here + myDeviceId to myDeviceKey + ), + sharedSecret = generateSharedSecret(), + otherUserKey = myMasterKey, + otherDeviceKey = otherDeviceKey ) } @@ -1128,7 +1235,7 @@ internal class DefaultVerificationService @Inject constructor( if (VERIFICATION_METHOD_QR_CODE_SCAN in otherUserMethods || VERIFICATION_METHOD_QR_CODE_SHOW in otherUserMethods) { // Other user wants to verify using QR code. Cross-signing has to be setup - val qrCodeData = createQrCodeData(transactionId, otherUserId) + val qrCodeData = createQrCodeData(transactionId, otherUserId, otherDeviceId) if (qrCodeData != null) { if (VERIFICATION_METHOD_QR_CODE_SCAN in otherUserMethods && VerificationMethod.QR_CODE_SHOW in methods) { 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 b999059d86..a184e225d2 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 @@ -88,12 +88,29 @@ internal class DefaultQrCodeVerificationTransaction( } // check master key - if (otherQrCodeData.otherUserKey != crossSigningService.getUserCrossSigningKeys(userId)?.masterKey()?.unpaddedBase64PublicKey) { + if (otherQrCodeData.userId != userId + && otherQrCodeData.otherUserKey == null) { + // Verification with other user, other_user_key is mandatory in this case + Timber.d("## Verification QR: Invalid, missing other_user_key") + cancel(CancelCode.QrCodeInvalid) + return + } + + if (otherQrCodeData.otherUserKey != null + && otherQrCodeData.otherUserKey != crossSigningService.getUserCrossSigningKeys(userId)?.masterKey()?.unpaddedBase64PublicKey) { Timber.d("## Verification QR: Invalid other master key ${otherQrCodeData.otherUserKey}") cancel(CancelCode.MismatchedKeys) return } + // Check device key if available + if (otherQrCodeData.otherDeviceKey != null + && otherQrCodeData.otherDeviceKey != cryptoStore.getUserDevice(otherQrCodeData.userId, otherDeviceId ?: "")?.fingerprint()) { + Timber.d("## Verification QR: Invalid other device key") + cancel(CancelCode.MismatchedKeys) + return + } + val toVerifyDeviceIds = mutableListOf() var canTrustOtherUserMasterKey = false @@ -147,8 +164,15 @@ internal class DefaultQrCodeVerificationTransaction( // qrCodeData.sharedSecret will be used to send the start request start(otherQrCodeData.sharedSecret) + val safeOtherDeviceId = otherDeviceId + if (!otherQrCodeData.otherDeviceKey.isNullOrBlank() + && safeOtherDeviceId != null) { + // Locally verify the device + toVerifyDeviceIds.add(safeOtherDeviceId) + } + // Trust the other user - trust(canTrustOtherUserMasterKey, toVerifyDeviceIds) + trust(canTrustOtherUserMasterKey, toVerifyDeviceIds.distinct()) } fun start(remoteSecret: String) { @@ -240,8 +264,8 @@ internal class DefaultQrCodeVerificationTransaction( // TODO what if the otherDevice is not in this list? and should we toVerifyDeviceIds.forEach { - setDeviceVerified(otherUserId, it) - } + setDeviceVerified(otherUserId, it) + } transport.done(transactionId) state = VerificationTxState.Verified } diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/Extensions.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/Extensions.kt index 5f21ecf18e..e3301683c1 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/Extensions.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/Extensions.kt @@ -32,6 +32,7 @@ private const val ENCODING = "utf-8" * &key_=... * &secret= * &other_user_key= + * &other_device_key= * * Example: * https://matrix.to/#/@user:matrix.org? @@ -40,7 +41,8 @@ private const val ENCODING = "utf-8" * &key_VJEDVKUYTQ=DL7LWIw7Qp%2B4AREDACTEDOwy2BjygumSWAGfzaWY * &key_fsh%2FfQ08N3xvh4ySXsINB%2BJ2hREDACTEDVcVOG4qqo=fsh%2FfQ08N3xvh4ySXsINB%2BJ2hREDACTEDVcVOG4qqo * &secret=AjQqw51Fp6UBuPolZ2FAD5WnXc22ZhJG6iGslrVvIdw%3D - * &other_user_key=WqSVLkBCS%2Fi5NqR%2F%2FymC8T7K9RPxBIuqK8Usl6Y3big + * &other_user_key=WqSVLkBCS%2Fi5NqRREDACTEDRPxBIuqK8Usl6Y3big + * &other_device_key=WqSVLkBREDACTEDBsfszdvsdBEvefqsdcsfBvsfcsFb * */ fun QrCodeData.toUrl(): String { @@ -58,8 +60,15 @@ fun QrCodeData.toUrl(): String { append("&secret=") append(URLEncoder.encode(sharedSecret, ENCODING)) - append("&other_user_key=") - append(URLEncoder.encode(otherUserKey, ENCODING)) + + if (!otherUserKey.isNullOrBlank()) { + append("&other_user_key=") + append(URLEncoder.encode(otherUserKey, ENCODING)) + } + if (!otherDeviceKey.isNullOrBlank()) { + append("&other_device_key=") + append(URLEncoder.encode(otherDeviceKey, ENCODING)) + } } } @@ -100,7 +109,8 @@ fun String.toQrCodeData(): QrCodeData? { val requestEventId = keyValues["request"]?.takeIf { MatrixPatterns.isEventId(it) } ?: return null val sharedSecret = keyValues["secret"] ?: return null - val otherUserKey = keyValues["other_user_key"] ?: return null + val otherUserKey = keyValues["other_user_key"] + val otherDeviceKey = keyValues["other_device_key"] val keys = keyValues.keys .filter { it.startsWith("key_") } @@ -115,6 +125,7 @@ fun String.toQrCodeData(): QrCodeData? { action, keys, sharedSecret, - otherUserKey + otherUserKey, + otherDeviceKey ) } diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeData.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeData.kt index 8b400413b0..b66ea68016 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeData.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeData.kt @@ -26,13 +26,18 @@ data class QrCodeData( // The action val action: String, // key_: each key that the user wants verified will have an entry of this form, where the value is the key in unpadded base64. - // The QR code should contain at least the user's master cross-signing key. + // The QR code should contain at least the user's master cross-signing key. In the case where a device does not have a cross-signing key + // (as in the case where a user logs in to a new device, and is verifying against another device), thin the QR code should contain at + // least the device's key. val keys: Map, // random single-use shared secret in unpadded base64. It must be at least 256-bits long (43 characters when base64-encoded). val sharedSecret: String, // the other user's master cross-signing key, in unpadded base64. In other words, if Alice is displaying the QR code, // this would be the copy of Bob's master cross-signing key that Alice has. - val otherUserKey: String + val otherUserKey: String?, + // The other device's key, in unpadded base64 + // This is only needed when a user is verifying their own devices, where the other device has not yet been signed with the cross-signing key. + val otherDeviceKey: String? ) { companion object { const val ACTION_VERIFY = "verify" diff --git a/matrix-sdk-android/src/test/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeTest.kt b/matrix-sdk-android/src/test/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeTest.kt index 9ef28a2aab..d5b0aa0ebb 100644 --- a/matrix-sdk-android/src/test/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeTest.kt +++ b/matrix-sdk-android/src/test/java/im/vector/matrix/android/internal/crypto/verification/qrcode/QrCodeTest.kt @@ -16,6 +16,7 @@ package im.vector.matrix.android.internal.crypto.verification.qrcode +import org.amshove.kluent.shouldBe import org.amshove.kluent.shouldBeEqualTo import org.amshove.kluent.shouldBeNull import org.amshove.kluent.shouldNotBeNull @@ -36,10 +37,18 @@ class QrCodeTest { "2" to "ghijql" ), sharedSecret = "sharedSecret", - otherUserKey = "otherUserKey" + otherUserKey = "otherUserKey", + otherDeviceKey = "otherDeviceKey" ) - private val basicUrl = "https://matrix.to/#/@benoit:matrix.org?request=%24azertyazerty&action=verify&key_1=abcdef&key_2=ghijql&secret=sharedSecret&other_user_key=otherUserKey" + private val basicUrl = "https://matrix.to/#/@benoit:matrix.org" + + "?request=%24azertyazerty" + + "&action=verify" + + "&key_1=abcdef" + + "&key_2=ghijql" + + "&secret=sharedSecret" + + "&other_user_key=otherUserKey" + + "&other_device_key=otherDeviceKey" @Test fun testNominalCase() { @@ -56,7 +65,8 @@ class QrCodeTest { decodedData.keys["1"]?.shouldBeEqualTo("abcdef") decodedData.keys["2"]?.shouldBeEqualTo("ghijql") decodedData.sharedSecret shouldBeEqualTo "sharedSecret" - decodedData.otherUserKey shouldBeEqualTo "otherUserKey" + decodedData.otherUserKey?.shouldBeEqualTo("otherUserKey") + decodedData.otherDeviceKey?.shouldBeEqualTo("otherDeviceKey") } @Test @@ -81,7 +91,56 @@ class QrCodeTest { decodedData.keys["1"]?.shouldBeEqualTo("abcdef") decodedData.keys["2"]?.shouldBeEqualTo("ghijql") decodedData.sharedSecret shouldBeEqualTo "sharedSecret" - decodedData.otherUserKey shouldBeEqualTo "otherUserKey" + decodedData.otherUserKey!! shouldBeEqualTo "otherUserKey" + decodedData.otherDeviceKey!! shouldBeEqualTo "otherDeviceKey" + } + + @Test + fun testNoOtherUserKey() { + val url = basicQrCodeData + .copy( + otherUserKey = null + ) + .toUrl() + + url shouldBeEqualTo basicUrl + .replace("&other_user_key=otherUserKey", "") + + val decodedData = url.toQrCodeData() + + decodedData.shouldNotBeNull() + + decodedData.userId shouldBeEqualTo "@benoit:matrix.org" + decodedData.requestEventId shouldBeEqualTo "\$azertyazerty" + decodedData.keys["1"]?.shouldBeEqualTo("abcdef") + decodedData.keys["2"]?.shouldBeEqualTo("ghijql") + decodedData.sharedSecret shouldBeEqualTo "sharedSecret" + decodedData.otherUserKey shouldBe null + decodedData.otherDeviceKey?.shouldBeEqualTo("otherDeviceKey") + } + + @Test + fun testNoOtherDeviceKey() { + val url = basicQrCodeData + .copy( + otherDeviceKey = null + ) + .toUrl() + + url shouldBeEqualTo basicUrl + .replace("&other_device_key=otherDeviceKey", "") + + val decodedData = url.toQrCodeData() + + decodedData.shouldNotBeNull() + + decodedData.userId shouldBeEqualTo "@benoit:matrix.org" + decodedData.requestEventId shouldBeEqualTo "\$azertyazerty" + decodedData.keys["1"]?.shouldBeEqualTo("abcdef") + decodedData.keys["2"]?.shouldBeEqualTo("ghijql") + decodedData.sharedSecret shouldBeEqualTo "sharedSecret" + decodedData.otherUserKey?.shouldBeEqualTo("otherUserKey") + decodedData.otherDeviceKey shouldBe null } @Test @@ -149,11 +208,4 @@ class QrCodeTest { .toQrCodeData() .shouldBeNull() } - - @Test - fun testMissingOtherUserKey() { - basicUrl.replace("&other_user_key=otherUserKey", "") - .toQrCodeData() - .shouldBeNull() - } }