From 80f4f95f81663a75f29cd43a0d26d4993f476925 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 31 Jan 2020 10:24:59 +0100 Subject: [PATCH] QRCode: requestId is not supposed to be an eventId --- .../DefaultVerificationService.kt | 24 ++++----- .../DefaultQrCodeVerificationTransaction.kt | 4 +- .../crypto/verification/qrcode/Extensions.kt | 8 +-- .../crypto/verification/qrcode/QrCodeData.kt | 4 +- .../crypto/verification/qrcode/QrCodeTest.kt | 51 ++++++++++++++++--- 5 files changed, 63 insertions(+), 28 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 a9ff3807cf..aa1a64cb6a 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 @@ -786,25 +786,25 @@ internal class DefaultVerificationService @Inject constructor( )) } - private fun createQrCodeData(requestEventId: String?, otherUserId: String, otherDeviceId: String?): QrCodeData? { - requestEventId ?: run { - Timber.w("## Unknown requestEventId") + private fun createQrCodeData(requestId: String?, otherUserId: String, otherDeviceId: String?): QrCodeData? { + requestId ?: run { + Timber.w("## Unknown requestId") return null } return when { userId != otherUserId -> - createQrCodeDataForDistinctUser(requestEventId, otherUserId, otherDeviceId) + createQrCodeDataForDistinctUser(requestId, otherUserId, otherDeviceId) crossSigningService.isCrossSigningVerified() -> // This is a self verification and I am the old device (Osborne2) - createQrCodeDataForVerifiedDevice(requestEventId, otherDeviceId) + createQrCodeDataForVerifiedDevice(requestId, otherDeviceId) else -> // This is a self verification and I am the new device (Dynabook) - createQrCodeDataForUnVerifiedDevice(requestEventId, otherDeviceId) + createQrCodeDataForUnVerifiedDevice(requestId, otherDeviceId) } } - private fun createQrCodeDataForDistinctUser(requestEventId: String, otherUserId: String, otherDeviceId: String?): QrCodeData? { + private fun createQrCodeDataForDistinctUser(requestId: String, otherUserId: String, otherDeviceId: String?): QrCodeData? { val myMasterKey = crossSigningService.getMyCrossSigningKeys() ?.masterKey() ?.unpaddedBase64PublicKey @@ -840,7 +840,7 @@ internal class DefaultVerificationService @Inject constructor( return QrCodeData( userId = userId, - requestEventId = requestEventId, + requestId = requestId, action = QrCodeData.ACTION_VERIFY, keys = hashMapOf( myMasterKey to myMasterKey, @@ -853,7 +853,7 @@ internal class DefaultVerificationService @Inject constructor( } // Create a QR code to display on the old device (Osborne2) - private fun createQrCodeDataForVerifiedDevice(requestEventId: String, otherDeviceId: String?): QrCodeData? { + private fun createQrCodeDataForVerifiedDevice(requestId: String, otherDeviceId: String?): QrCodeData? { val myMasterKey = crossSigningService.getMyCrossSigningKeys() ?.masterKey() ?.unpaddedBase64PublicKey @@ -885,7 +885,7 @@ internal class DefaultVerificationService @Inject constructor( return QrCodeData( userId = userId, - requestEventId = requestEventId, + requestId = requestId, action = QrCodeData.ACTION_VERIFY, keys = hashMapOf( myMasterKey to myMasterKey, @@ -898,7 +898,7 @@ internal class DefaultVerificationService @Inject constructor( } // Create a QR code to display on the new device (Dynabook) - private fun createQrCodeDataForUnVerifiedDevice(requestEventId: String, otherDeviceId: String?): QrCodeData? { + private fun createQrCodeDataForUnVerifiedDevice(requestId: String, otherDeviceId: String?): QrCodeData? { val myMasterKey = crossSigningService.getMyCrossSigningKeys() ?.masterKey() ?.unpaddedBase64PublicKey @@ -926,7 +926,7 @@ internal class DefaultVerificationService @Inject constructor( return QrCodeData( userId = userId, - requestEventId = requestEventId, + requestId = requestId, action = QrCodeData.ACTION_VERIFY, keys = hashMapOf( // Note: no master key here 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 a184e225d2..6833390393 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 @@ -81,8 +81,8 @@ internal class DefaultQrCodeVerificationTransaction( return } - if (otherQrCodeData.requestEventId != transactionId) { - Timber.d("## Verification QR: Invalid transaction actual ${otherQrCodeData.requestEventId} expected:$transactionId") + if (otherQrCodeData.requestId != transactionId) { + Timber.d("## Verification QR: Invalid transaction actual ${otherQrCodeData.requestId} expected:$transactionId") cancel(CancelCode.QrCodeInvalid) return } 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 e3301683c1..d539152135 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 @@ -49,7 +49,7 @@ fun QrCodeData.toUrl(): String { return buildString { append(PermalinkFactory.createPermalink(userId)) append("?request=") - append(URLEncoder.encode(requestEventId, ENCODING)) + append(URLEncoder.encode(requestId, ENCODING)) append("&action=") append(URLEncoder.encode(action, ENCODING)) @@ -105,10 +105,10 @@ fun String.toQrCodeData(): QrCodeData? { (it.substringBefore("=") to it.substringAfter("=").let { value -> URLDecoder.decode(value, ENCODING) }) }.toMap() - val action = keyValues["action"] ?: return null + val action = keyValues["action"]?.takeIf { it.isNotBlank() } ?: return null - val requestEventId = keyValues["request"]?.takeIf { MatrixPatterns.isEventId(it) } ?: return null - val sharedSecret = keyValues["secret"] ?: return null + val requestEventId = keyValues["request"]?.takeIf { it.isNotBlank() } ?: return null + val sharedSecret = keyValues["secret"]?.takeIf { it.isNotBlank() } ?: return null val otherUserKey = keyValues["other_user_key"] val otherDeviceKey = keyValues["other_device_key"] 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 b66ea68016..0f9a31ab32 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 @@ -21,8 +21,8 @@ package im.vector.matrix.android.internal.crypto.verification.qrcode */ data class QrCodeData( val userId: String, - // the event ID of the associated verification request event. - val requestEventId: String, + // Request Id. Can be an arbitrary value. In DM, it will be the event ID of the associated verification request event. + val requestId: String, // 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. 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 d5b0aa0ebb..2dd0649be1 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 @@ -30,7 +30,7 @@ class QrCodeTest { private val basicQrCodeData = QrCodeData( userId = "@benoit:matrix.org", - requestEventId = "\$azertyazerty", + requestId = "\$azertyazerty", action = QrCodeData.ACTION_VERIFY, keys = mapOf( "1" to "abcdef", @@ -61,7 +61,7 @@ class QrCodeTest { decodedData.shouldNotBeNull() decodedData.userId shouldBeEqualTo "@benoit:matrix.org" - decodedData.requestEventId shouldBeEqualTo "\$azertyazerty" + decodedData.requestId shouldBeEqualTo "\$azertyazerty" decodedData.keys["1"]?.shouldBeEqualTo("abcdef") decodedData.keys["2"]?.shouldBeEqualTo("ghijql") decodedData.sharedSecret shouldBeEqualTo "sharedSecret" @@ -74,7 +74,7 @@ class QrCodeTest { val url = basicQrCodeData .copy( userId = "@benoit/foo:matrix.org", - requestEventId = "\$azertyazerty/bar" + requestId = "\$azertyazerty/bar" ) .toUrl() @@ -87,7 +87,7 @@ class QrCodeTest { decodedData.shouldNotBeNull() decodedData.userId shouldBeEqualTo "@benoit/foo:matrix.org" - decodedData.requestEventId shouldBeEqualTo "\$azertyazerty/bar" + decodedData.requestId shouldBeEqualTo "\$azertyazerty/bar" decodedData.keys["1"]?.shouldBeEqualTo("abcdef") decodedData.keys["2"]?.shouldBeEqualTo("ghijql") decodedData.sharedSecret shouldBeEqualTo "sharedSecret" @@ -111,7 +111,7 @@ class QrCodeTest { decodedData.shouldNotBeNull() decodedData.userId shouldBeEqualTo "@benoit:matrix.org" - decodedData.requestEventId shouldBeEqualTo "\$azertyazerty" + decodedData.requestId shouldBeEqualTo "\$azertyazerty" decodedData.keys["1"]?.shouldBeEqualTo("abcdef") decodedData.keys["2"]?.shouldBeEqualTo("ghijql") decodedData.sharedSecret shouldBeEqualTo "sharedSecret" @@ -135,7 +135,7 @@ class QrCodeTest { decodedData.shouldNotBeNull() decodedData.userId shouldBeEqualTo "@benoit:matrix.org" - decodedData.requestEventId shouldBeEqualTo "\$azertyazerty" + decodedData.requestId shouldBeEqualTo "\$azertyazerty" decodedData.keys["1"]?.shouldBeEqualTo("abcdef") decodedData.keys["2"]?.shouldBeEqualTo("ghijql") decodedData.sharedSecret shouldBeEqualTo "sharedSecret" @@ -173,6 +173,13 @@ class QrCodeTest { .shouldBeNull() } + @Test + fun testEmptyActionCase() { + basicUrl.replace("&action=verify", "&action=") + .toQrCodeData() + .shouldBeNull() + } + @Test fun testOtherActionCase() { basicUrl.replace("&action=verify", "&action=confirm") @@ -182,8 +189,15 @@ class QrCodeTest { } @Test - fun testBadRequestEventId() { - basicUrl.replace("%24azertyazerty", "%32azertyazerty") + fun testMissingRequestId() { + basicUrl.replace("request=%24azertyazerty", "") + .toQrCodeData() + .shouldBeNull() + } + + @Test + fun testEmptyRequestId() { + basicUrl.replace("request=%24azertyazerty", "request=") .toQrCodeData() .shouldBeNull() } @@ -208,4 +222,25 @@ class QrCodeTest { .toQrCodeData() .shouldBeNull() } + + @Test + fun testEmptySecret() { + basicUrl.replace("&secret=sharedSecret", "&secret=") + .toQrCodeData() + .shouldBeNull() + } + + @Test + fun testSelfSigning() { + // request is not an eventId in this case + val url = "https://matrix.to/#/@benoit0815:matrix.org" + + "?request=local.4dff40e1-7bf1-4e80-81ed-c6090d43bf20" + + "&action=verify" + + "&key_utbSRFcFjFDYf0KcNv3FoBHFSbvUPXtCYutuOg6WQ%2Bs=utbSRFcFjFDYf0KcNv3FoBHFSbvUPXtCYutuOg6WQ%2Bs" + + "&key_YSOXZVBXIZ=F0XWqgUePgwm5HMYG3yhBNneHmscrAxxlooLHjy8YQc" + + "&secret=LYVcEQmfdorbJ3vbQnq7nbNZc%2BGmDxUen1rByV9hRM4" + + "&other_device_key=eGoUqZqAroCYpjp7FLGIkTEzYHBFED4uUAfJ267gqQQ" + + url.toQrCodeData()!!.requestId shouldBeEqualTo "local.4dff40e1-7bf1-4e80-81ed-c6090d43bf20" + } }