From 83241ac17d13aefab9d175f48397445b412a243c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 27 Jan 2020 06:59:04 -0700 Subject: [PATCH 01/13] Update QR code handling for new URL This doesn't have any meaningful change on the process, just makes it more in line with what we do. --- src/crypto/verification/QRCode.js | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index fe58307322b..4890f5c7880 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -1,5 +1,6 @@ /* Copyright 2018 New Vector Ltd +Copyright 2020 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -26,6 +27,7 @@ import { newUserCancelledError, newUserMismatchError, } from './Error'; +import * as qs from "qs"; const MATRIXTO_REGEXP = /^(?:https?:\/\/)?(?:www\.)?matrix\.to\/#\/([#@!+][^?]+)\?(.+)$/; const KEY_REGEXP = /^key_([^:]+:.+)$/; @@ -39,13 +41,25 @@ const newQRCodeError = errorFactory("m.qr_code.invalid", "Invalid QR code"); export class ShowQRCode extends Base { _doVerification() { if (!this._done) { - const url = "https://matrix.to/#/" + this._baseApis.getUserId() - + "?device=" + encodeURIComponent(this._baseApis.deviceId) - + "&action=verify&key_ed25519%3A" - + encodeURIComponent(this._baseApis.deviceId) + "=" - + encodeURIComponent(this._baseApis.getDeviceEd25519Key()); + const crossSigningInfo = this._baseApis.getStoredCrossSigningForUser(this.request.otherUserId); + const myKeyId = this._baseApis.getCrossSigningId(); + const qrCodeKeys = [ + [this._baseApis.getDeviceId(), this._baseApis.getDeviceEd25519Key()], + [myKeyId, myKeyId], + ]; + const query = { + request: this.request.requestEvent.getId(), + action: "verify", + secret: this.request.encodedSharedSecret, + other_user_key: crossSigningInfo.getId("master"), + }; + for (const key of qrCodeKeys) { + query[`key_${key[0]}`] = key[1]; + } + + const uri = `https://matrix.to/#/${this._baseApis.getUserId()}?${qs.stringify(query)}`; this.emit("show_qr_code", { - url: url, + url: uri, }); } } From 419693023f083e16d4b0e893274a226a3ab904e7 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 27 Jan 2020 11:41:52 -0700 Subject: [PATCH 02/13] Add untested reciprocate function --- src/crypto/index.js | 4 +- src/crypto/verification/QRCode.js | 105 +++++++++++++++++++++--------- 2 files changed, 77 insertions(+), 32 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 375b8f08a47..7dd771130ef 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -41,7 +41,7 @@ import { import {SECRET_STORAGE_ALGORITHM_V1, SecretStorage} from './SecretStorage'; import {OutgoingRoomKeyRequestManager} from './OutgoingRoomKeyRequestManager'; import {IndexedDBCryptoStore} from './store/indexeddb-crypto-store'; -import {ScanQRCode, ShowQRCode} from './verification/QRCode'; +import {ReciprocateQRCode, ScanQRCode, ShowQRCode} from './verification/QRCode'; import {SAS} from './verification/SAS'; import {keyFromPassphrase} from './key_passphrase'; import {encodeRecoveryKey} from './recoverykey'; @@ -55,6 +55,7 @@ const DeviceVerification = DeviceInfo.DeviceVerification; const defaultVerificationMethods = { [ScanQRCode.NAME]: ScanQRCode, [ShowQRCode.NAME]: ShowQRCode, + [ReciprocateQRCode.NAME]: ReciprocateQRCode, [SAS.NAME]: SAS, }; @@ -64,6 +65,7 @@ const defaultVerificationMethods = { export const verificationMethods = { QR_CODE_SCAN: ScanQRCode.NAME, QR_CODE_SHOW: ShowQRCode.NAME, + RECIPROCATE_QR_CODE: ReciprocateQRCode.NAME, SAS: SAS.NAME, }; diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index 4890f5c7880..b3536304fb9 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -30,7 +30,6 @@ import { import * as qs from "qs"; const MATRIXTO_REGEXP = /^(?:https?:\/\/)?(?:www\.)?matrix\.to\/#\/([#@!+][^?]+)\?(.+)$/; -const KEY_REGEXP = /^key_([^:]+:.+)$/; const newQRCodeError = errorFactory("m.qr_code.invalid", "Invalid QR code"); @@ -83,38 +82,34 @@ export class ScanQRCode extends Base { cancel: () => reject(newUserCancelledError()), }); }); + const {action, secret, otherUserKey, keys, targetUserId} = ReciprocateQRCode.splitUrl(code); + } +} - const match = code.match(MATRIXTO_REGEXP); - let deviceId; - const keys = {}; - if (!match) { - throw newQRCodeError(); - } - const userId = match[1]; - const params = match[2].split("&").map( - (x) => x.split("=", 2).map(decodeURIComponent), - ); - let action; - for (const [name, value] of params) { - if (name === "device") { - deviceId = value; - } else if (name === "action") { - action = value; - } else { - const keyMatch = name.match(KEY_REGEXP); - if (keyMatch) { - keys[keyMatch[1]] = value; - } - } - } - if (!deviceId || action !== "verify" || Object.keys(keys).length === 0) { - throw newQRCodeError(); - } +ScanQRCode.NAME = "m.qr_code.scan.v1"; + +/** + * @class crypto/verification/QRCode/ReciprocateQRCode + * @extends {module:crypto/verification/Base} + */ +export class ReciprocateQRCode extends Base { + static factory(...args) { + return new ReciprocateQRCode(...args); + } + + async _doVerification() { + const code = await new Promise((resolve, reject) => { + this.emit("scan", { + done: resolve, + cancel: () => reject(newUserCancelledError()), + }); + }); + const {secret, otherUserKey, keys, targetUserId} = ReciprocateQRCode.splitUrl(code); if (!this.userId) { await new Promise((resolve, reject) => { this.emit("confirm_user_id", { - userId: userId, + userId: targetUserId, confirm: resolve, cancel: () => reject(newUserMismatchError()), }); @@ -122,16 +117,64 @@ export class ScanQRCode extends Base { } else if (this.userId !== userId) { throw newUserMismatchError({ expected: this.userId, - actual: userId, + actual: targetUserId, }); } - await this._verifyKeys(userId, keys, (keyId, device, key) => { + const crossSigningInfo = this._baseApis.getStoredCrossSigningInfo(targetUserId); + if (!crossSigningInfo) throw new Error("Missing cross signing info for user"); // this shouldn't happen by now + if (crossSigningInfo.getId("master") !== otherUserKey) { + throw newKeyMismatchError(); + } + + if (secret !== this.request.encodedSharedSecret) { + throw newQRCodeError(); + } + + // Verify our own keys that were sent in this code too + await this._verifyKeys(this._baseApis.getUserId(), keys, (keyId, device, key) => { if (device.keys[keyId] !== key) { throw newKeyMismatchError(); } }); + + await this._verifyKeys(targetUserId, [otherUserKey, otherUserKey], (keyId, device, key) => { + if (device.keys[keyId] !== key) { + throw newKeyMismatchError(); + } + }); + } + + static splitUrl(code) { + const match = code.match(MATRIXTO_REGEXP); + const keys = {}; + if (!match) { + throw newQRCodeError(); + } + const targetUserId = match[1]; + const params = match[2].split("&").map( + (x) => x.split("=", 2).map(decodeURIComponent), + ); + let action; + let otherUserKey; + let secret; + for (const [name, value] of params) { + if (name === "action") { + action = value; + } else if (name.startsWith("key_")) { + keys[name.substring("key_".length)] = value; + } else if (name === "other_user_key") { + otherUserKey = value; + } else if (name === "secret") { + secret = value; + } + } + if (!secret || !otherUserKey || action !== "verify" || Object.keys(keys).length === 0) { + throw newQRCodeError(); + } + + return {action, secret, otherUserKey, keys, targetUserId}; } } -ScanQRCode.NAME = "m.qr_code.scan.v1"; +ReciprocateQRCode.NAME = "m.reciprocate.v1"; From fd563bda6a6668d857e844cfc5e69e530206fdd5 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 09:26:22 +0000 Subject: [PATCH 03/13] Remove irrelevant verification flows for QR codes You can't actually get at these through our verification framework - they scan/show steps are pre-verification framework. --- src/crypto/index.js | 6 +--- src/crypto/verification/QRCode.js | 56 ++----------------------------- 2 files changed, 3 insertions(+), 59 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 99250c1d2c1..fe84e751635 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -41,7 +41,7 @@ import { import {SECRET_STORAGE_ALGORITHM_V1, SecretStorage} from './SecretStorage'; import {OutgoingRoomKeyRequestManager} from './OutgoingRoomKeyRequestManager'; import {IndexedDBCryptoStore} from './store/indexeddb-crypto-store'; -import {ReciprocateQRCode, ScanQRCode, ShowQRCode} from './verification/QRCode'; +import {ReciprocateQRCode} from './verification/QRCode'; import {SAS} from './verification/SAS'; import {keyFromPassphrase} from './key_passphrase'; import {encodeRecoveryKey} from './recoverykey'; @@ -53,8 +53,6 @@ import * as httpApi from "../http-api"; const DeviceVerification = DeviceInfo.DeviceVerification; const defaultVerificationMethods = { - [ScanQRCode.NAME]: ScanQRCode, - [ShowQRCode.NAME]: ShowQRCode, [ReciprocateQRCode.NAME]: ReciprocateQRCode, [SAS.NAME]: SAS, }; @@ -63,8 +61,6 @@ const defaultVerificationMethods = { * verification method names */ export const verificationMethods = { - QR_CODE_SCAN: ScanQRCode.NAME, - QR_CODE_SHOW: ShowQRCode.NAME, RECIPROCATE_QR_CODE: ReciprocateQRCode.NAME, SAS: SAS.NAME, }; diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index b3536304fb9..d304bddc461 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -33,60 +33,8 @@ const MATRIXTO_REGEXP = /^(?:https?:\/\/)?(?:www\.)?matrix\.to\/#\/([#@!+][^?]+) const newQRCodeError = errorFactory("m.qr_code.invalid", "Invalid QR code"); -/** - * @class crypto/verification/QRCode/ShowQRCode - * @extends {module:crypto/verification/Base} - */ -export class ShowQRCode extends Base { - _doVerification() { - if (!this._done) { - const crossSigningInfo = this._baseApis.getStoredCrossSigningForUser(this.request.otherUserId); - const myKeyId = this._baseApis.getCrossSigningId(); - const qrCodeKeys = [ - [this._baseApis.getDeviceId(), this._baseApis.getDeviceEd25519Key()], - [myKeyId, myKeyId], - ]; - const query = { - request: this.request.requestEvent.getId(), - action: "verify", - secret: this.request.encodedSharedSecret, - other_user_key: crossSigningInfo.getId("master"), - }; - for (const key of qrCodeKeys) { - query[`key_${key[0]}`] = key[1]; - } - - const uri = `https://matrix.to/#/${this._baseApis.getUserId()}?${qs.stringify(query)}`; - this.emit("show_qr_code", { - url: uri, - }); - } - } -} - -ShowQRCode.NAME = "m.qr_code.show.v1"; - -/** - * @class crypto/verification/QRCode/ScanQRCode - * @extends {module:crypto/verification/Base} - */ -export class ScanQRCode extends Base { - static factory(...args) { - return new ScanQRCode(...args); - } - - async _doVerification() { - const code = await new Promise((resolve, reject) => { - this.emit("scan", { - done: resolve, - cancel: () => reject(newUserCancelledError()), - }); - }); - const {action, secret, otherUserKey, keys, targetUserId} = ReciprocateQRCode.splitUrl(code); - } -} - -ScanQRCode.NAME = "m.qr_code.scan.v1"; +export const SHOW_QR_CODE_METHOD = "m.qr_code.show.v1"; +export const SCAN_QR_CODE_METHOD = "m.qr_code.scan.v1"; /** * @class crypto/verification/QRCode/ReciprocateQRCode From f689142806c2b0a71455eece54d5f46f47b46f1a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 10:52:26 +0000 Subject: [PATCH 04/13] Define NAME as a property higher up --- src/crypto/verification/QRCode.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index d304bddc461..98d6dfe72ef 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -45,6 +45,10 @@ export class ReciprocateQRCode extends Base { return new ReciprocateQRCode(...args); } + static get NAME() { + return "m.reciprocate.v1"; + } + async _doVerification() { const code = await new Promise((resolve, reject) => { this.emit("scan", { @@ -124,5 +128,3 @@ export class ReciprocateQRCode extends Base { return {action, secret, otherUserKey, keys, targetUserId}; } } - -ReciprocateQRCode.NAME = "m.reciprocate.v1"; From 76402ec8d71fac3a6e2a44c8803d2235d2951612 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 13:45:02 +0000 Subject: [PATCH 05/13] Lie to the verification handling --- src/crypto/index.js | 11 +++++- src/crypto/verification/IllegalMethod.js | 43 ++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 src/crypto/verification/IllegalMethod.js diff --git a/src/crypto/index.js b/src/crypto/index.js index fe84e751635..8a997238c33 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -41,7 +41,7 @@ import { import {SECRET_STORAGE_ALGORITHM_V1, SecretStorage} from './SecretStorage'; import {OutgoingRoomKeyRequestManager} from './OutgoingRoomKeyRequestManager'; import {IndexedDBCryptoStore} from './store/indexeddb-crypto-store'; -import {ReciprocateQRCode} from './verification/QRCode'; +import {ReciprocateQRCode, SCAN_QR_CODE_METHOD, SHOW_QR_CODE_METHOD} from './verification/QRCode'; import {SAS} from './verification/SAS'; import {keyFromPassphrase} from './key_passphrase'; import {encodeRecoveryKey} from './recoverykey'; @@ -49,12 +49,19 @@ import {VerificationRequest} from "./verification/request/VerificationRequest"; import {InRoomChannel, InRoomRequests} from "./verification/request/InRoomChannel"; import {ToDeviceChannel, ToDeviceRequests} from "./verification/request/ToDeviceChannel"; import * as httpApi from "../http-api"; +import {IllegalMethod} from "./verification/IllegalMethod"; const DeviceVerification = DeviceInfo.DeviceVerification; const defaultVerificationMethods = { [ReciprocateQRCode.NAME]: ReciprocateQRCode, [SAS.NAME]: SAS, + + // These two can't be used for actual verification, but we do + // need to be able to define them here for the verification flows + // to start. + [SHOW_QR_CODE_METHOD]: IllegalMethod, + [SCAN_QR_CODE_METHOD]: IllegalMethod, }; /** @@ -130,6 +137,8 @@ export function Crypto(baseApis, sessionStore, userId, deviceId, method.NAME, method, ); + } else { + console.warn(`Excluding unknown verification method ${method}`); } } } diff --git a/src/crypto/verification/IllegalMethod.js b/src/crypto/verification/IllegalMethod.js new file mode 100644 index 00000000000..2f05a28c7eb --- /dev/null +++ b/src/crypto/verification/IllegalMethod.js @@ -0,0 +1,43 @@ +/* +Copyright 2020 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/** + * Verification method that is illegal to have (cannot possibly + * do verification with this method). + * @module crypto/verification/IllegalMethod + */ + +import {VerificationBase as Base} from "./Base"; + +/** + * @class crypto/verification/IllegalMethod/IllegalMethod + * @extends {module:crypto/verification/Base} + */ +export class IllegalMethod extends Base { + static factory(...args) { + return new IllegalMethod(...args); + } + + static get NAME() { + // Typically the name will be something else, but to complete + // the contract we offer a default one here. + return "org.matrix.illegal_method"; + } + + async _doVerification() { + throw new Error("Verification is not possible with this method"); + } +} From be345a523f5cc078ec241bafd44a8cb8580c1d9f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 14:43:37 +0000 Subject: [PATCH 06/13] Fix verification flow --- src/crypto/verification/QRCode.js | 72 ++++--------------- .../request/VerificationRequest.js | 1 + 2 files changed, 14 insertions(+), 59 deletions(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index 98d6dfe72ef..a61fff3426f 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -23,7 +23,7 @@ limitations under the License. import {VerificationBase as Base} from "./Base"; import { errorFactory, - newKeyMismatchError, + newKeyMismatchError, newUnknownTransactionError, newUserCancelledError, newUserMismatchError, } from './Error'; @@ -50,15 +50,15 @@ export class ReciprocateQRCode extends Base { } async _doVerification() { - const code = await new Promise((resolve, reject) => { - this.emit("scan", { - done: resolve, - cancel: () => reject(newUserCancelledError()), - }); - }); - const {secret, otherUserKey, keys, targetUserId} = ReciprocateQRCode.splitUrl(code); + if (!this.startEvent) { + // TODO: Support scanning QR codes + throw new Error("It is not currently possible to start verification" + + "with this method yet."); + } + const targetUserId = this.startEvent.getSender(); if (!this.userId) { + console.log("Asking to confirm user ID"); await new Promise((resolve, reject) => { this.emit("confirm_user_id", { userId: targetUserId, @@ -66,65 +66,19 @@ export class ReciprocateQRCode extends Base { cancel: () => reject(newUserMismatchError()), }); }); - } else if (this.userId !== userId) { + } else if (targetUserId !== this.userId) { throw newUserMismatchError({ expected: this.userId, actual: targetUserId, }); } - const crossSigningInfo = this._baseApis.getStoredCrossSigningInfo(targetUserId); - if (!crossSigningInfo) throw new Error("Missing cross signing info for user"); // this shouldn't happen by now - if (crossSigningInfo.getId("master") !== otherUserKey) { + console.log(this.request); + if (this.startEvent.getContent()['secret'] !== this.request.encodedSharedSecret) { throw newKeyMismatchError(); } - if (secret !== this.request.encodedSharedSecret) { - throw newQRCodeError(); - } - - // Verify our own keys that were sent in this code too - await this._verifyKeys(this._baseApis.getUserId(), keys, (keyId, device, key) => { - if (device.keys[keyId] !== key) { - throw newKeyMismatchError(); - } - }); - - await this._verifyKeys(targetUserId, [otherUserKey, otherUserKey], (keyId, device, key) => { - if (device.keys[keyId] !== key) { - throw newKeyMismatchError(); - } - }); - } - - static splitUrl(code) { - const match = code.match(MATRIXTO_REGEXP); - const keys = {}; - if (!match) { - throw newQRCodeError(); - } - const targetUserId = match[1]; - const params = match[2].split("&").map( - (x) => x.split("=", 2).map(decodeURIComponent), - ); - let action; - let otherUserKey; - let secret; - for (const [name, value] of params) { - if (name === "action") { - action = value; - } else if (name.startsWith("key_")) { - keys[name.substring("key_".length)] = value; - } else if (name === "other_user_key") { - otherUserKey = value; - } else if (name === "secret") { - secret = value; - } - } - if (!secret || !otherUserKey || action !== "verify" || Object.keys(keys).length === 0) { - throw newQRCodeError(); - } - - return {action, secret, otherUserKey, keys, targetUserId}; + const requestEvent = this.request.requestEvent; + if (!requestEvent) throw new Error("Missing request event, somehow"); } } diff --git a/src/crypto/verification/request/VerificationRequest.js b/src/crypto/verification/request/VerificationRequest.js index d9bfeef1716..0bdd22eee97 100644 --- a/src/crypto/verification/request/VerificationRequest.js +++ b/src/crypto/verification/request/VerificationRequest.js @@ -651,6 +651,7 @@ export class VerificationRequest extends EventEmitter { userId, deviceId, startedByMe ? null : startEvent, + this, ); } From 0d7aee2c360206d05a1d0d099e814ea5b13f8619 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 14:52:04 +0000 Subject: [PATCH 07/13] Misc cleanup --- src/crypto/verification/QRCode.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index a61fff3426f..2eacf0041d9 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -59,10 +59,10 @@ export class ReciprocateQRCode extends Base { const targetUserId = this.startEvent.getSender(); if (!this.userId) { console.log("Asking to confirm user ID"); - await new Promise((resolve, reject) => { + this.userId = await new Promise((resolve, reject) => { this.emit("confirm_user_id", { userId: targetUserId, - confirm: resolve, + confirm: resolve, // takes a userId cancel: () => reject(newUserMismatchError()), }); }); @@ -73,12 +73,8 @@ export class ReciprocateQRCode extends Base { }); } - console.log(this.request); if (this.startEvent.getContent()['secret'] !== this.request.encodedSharedSecret) { throw newKeyMismatchError(); } - - const requestEvent = this.request.requestEvent; - if (!requestEvent) throw new Error("Missing request event, somehow"); } } From c45b38cece42580af52f71161fe177d331a81c90 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 14:56:28 +0000 Subject: [PATCH 08/13] Actually do the verification --- src/crypto/verification/QRCode.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index 2eacf0041d9..f74ccc9cf09 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -76,5 +76,12 @@ export class ReciprocateQRCode extends Base { if (this.startEvent.getContent()['secret'] !== this.request.encodedSharedSecret) { throw newKeyMismatchError(); } + + // If we've gotten this far, verify the user's master cross signing key + const xsignInfo = this._baseApis.getStoredCrossSigningInfo(this.userId); + if (!xsignInfo) throw new Error("Missing cross signing info"); + + const masterKey = xsignInfo.getId("master"); + await this._verifyKeys(this.userId, [masterKey, masterKey]); } } From 92f6ec918bf0f4a289e43ce1f656dd03e814cd3b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 15:06:13 +0000 Subject: [PATCH 09/13] Appease the linter --- src/crypto/index.js | 6 +++++- src/crypto/verification/QRCode.js | 9 +-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 8a997238c33..a76c158d9af 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -41,7 +41,11 @@ import { import {SECRET_STORAGE_ALGORITHM_V1, SecretStorage} from './SecretStorage'; import {OutgoingRoomKeyRequestManager} from './OutgoingRoomKeyRequestManager'; import {IndexedDBCryptoStore} from './store/indexeddb-crypto-store'; -import {ReciprocateQRCode, SCAN_QR_CODE_METHOD, SHOW_QR_CODE_METHOD} from './verification/QRCode'; +import { + ReciprocateQRCode, + SCAN_QR_CODE_METHOD, + SHOW_QR_CODE_METHOD, +} from './verification/QRCode'; import {SAS} from './verification/SAS'; import {keyFromPassphrase} from './key_passphrase'; import {encodeRecoveryKey} from './recoverykey'; diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index f74ccc9cf09..a4a37e0f0d9 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -22,16 +22,9 @@ limitations under the License. import {VerificationBase as Base} from "./Base"; import { - errorFactory, - newKeyMismatchError, newUnknownTransactionError, - newUserCancelledError, + newKeyMismatchError, newUserMismatchError, } from './Error'; -import * as qs from "qs"; - -const MATRIXTO_REGEXP = /^(?:https?:\/\/)?(?:www\.)?matrix\.to\/#\/([#@!+][^?]+)\?(.+)$/; - -const newQRCodeError = errorFactory("m.qr_code.invalid", "Invalid QR code"); export const SHOW_QR_CODE_METHOD = "m.qr_code.show.v1"; export const SCAN_QR_CODE_METHOD = "m.qr_code.scan.v1"; From ec4dc582b63adff7d9d9777f01d4539f645c271e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 15:10:35 +0000 Subject: [PATCH 10/13] Remove tests for old QR code stuff --- spec/unit/crypto/verification/qr_code.spec.js | 127 +----------------- 1 file changed, 7 insertions(+), 120 deletions(-) diff --git a/spec/unit/crypto/verification/qr_code.spec.js b/spec/unit/crypto/verification/qr_code.spec.js index 9f9a4c8bb17..e72bc877621 100644 --- a/spec/unit/crypto/verification/qr_code.spec.js +++ b/spec/unit/crypto/verification/qr_code.spec.js @@ -16,8 +16,6 @@ limitations under the License. */ import "../../../olm-loader"; import {logger} from "../../../../src/logger"; -import {DeviceInfo} from "../../../../src/crypto/deviceinfo"; -import {ScanQRCode, ShowQRCode} from "../../../../src/crypto/verification/QRCode"; const Olm = global.Olm; @@ -31,124 +29,13 @@ describe("QR code verification", function() { return Olm.init(); }); - describe("showing", function() { - it("should emit an event to show a QR code", async function() { - const channel = { - send: jest.fn(), - }; - const qrCode = new ShowQRCode(channel, { - getUserId: () => "@alice:example.com", - deviceId: "ABCDEFG", - getDeviceEd25519Key: function() { - return "device+ed25519+key"; - }, - }); - const spy = jest.fn((e) => { - qrCode.done(); - }); - qrCode.on("show_qr_code", spy); - await qrCode.verify(); - expect(spy).toHaveBeenCalledWith({ - url: "https://matrix.to/#/@alice:example.com?device=ABCDEFG" - + "&action=verify&key_ed25519%3AABCDEFG=device%2Bed25519%2Bkey", - }); - }); - }); - - describe("scanning", function() { - const QR_CODE_URL = "https://matrix.to/#/@alice:example.com?device=ABCDEFG" - + "&action=verify&key_ed25519%3AABCDEFG=device%2Bed25519%2Bkey"; - it("should verify when a QR code is sent", async function() { - const device = DeviceInfo.fromStorage( - { - algorithms: [], - keys: { - "curve25519:ABCDEFG": "device+curve25519+key", - "ed25519:ABCDEFG": "device+ed25519+key", - }, - verified: false, - known: false, - unsigned: {}, - }, - "ABCDEFG", - ); - const client = { - getStoredDevice: jest.fn().mockReturnValue(device), - setDeviceVerified: jest.fn(), - }; - const channel = { - send: jest.fn(), - }; - const qrCode = new ScanQRCode(channel, client); - qrCode.on("confirm_user_id", ({userId, confirm}) => { - if (userId === "@alice:example.com") { - confirm(); - } else { - qrCode.cancel(new Error("Incorrect user")); - } - }); - qrCode.on("scan", ({done}) => { - done(QR_CODE_URL); - }); - await qrCode.verify(); - expect(client.getStoredDevice) - .toHaveBeenCalledWith("@alice:example.com", "ABCDEFG"); - expect(client.setDeviceVerified) - .toHaveBeenCalledWith("@alice:example.com", "ABCDEFG"); - }); - - it("should error when the user ID doesn't match", async function() { - const client = { - getStoredDevice: jest.fn(), - setDeviceVerified: jest.fn(), - }; - const channel = { - send: jest.fn(), - }; - const qrCode = new ScanQRCode(channel, client, "@bob:example.com", "ABCDEFG"); - qrCode.on("scan", ({done}) => { - done(QR_CODE_URL); - }); - const spy = jest.fn(); - await qrCode.verify().catch(spy); - expect(spy).toHaveBeenCalled(); - expect(channel.send).toHaveBeenCalled(); - expect(client.getStoredDevice).not.toHaveBeenCalled(); - expect(client.setDeviceVerified).not.toHaveBeenCalled(); - }); - - it("should error if the key doesn't match", async function() { - const device = DeviceInfo.fromStorage( - { - algorithms: [], - keys: { - "curve25519:ABCDEFG": "device+curve25519+key", - "ed25519:ABCDEFG": "a+different+device+ed25519+key", - }, - verified: false, - known: false, - unsigned: {}, - }, - "ABCDEFG", - ); - const client = { - getStoredDevice: jest.fn().mockReturnValue(device), - setDeviceVerified: jest.fn(), - }; - const channel = { - send: jest.fn(), - }; - const qrCode = new ScanQRCode( - channel, client, "@alice:example.com", "ABCDEFG"); - qrCode.on("scan", ({done}) => { - done(QR_CODE_URL); - }); - const spy = jest.fn(); - await qrCode.verify().catch(spy); - expect(spy).toHaveBeenCalled(); - expect(channel.send).toHaveBeenCalled(); - expect(client.getStoredDevice).toHaveBeenCalled(); - expect(client.setDeviceVerified).not.toHaveBeenCalled(); + describe("reciprocate", () => { + it("should verify the secret", () => { + // TODO: Actually write a test for this. + // Tests are hard because we are running before the verification + // process actually begins, and are largely UI-driven rather than + // logic-driven (compared to something like SAS). In the interest + // of time, tests are currently excluded. }); }); }); From 5fbaa9cfa7dc206df7d92323d0024d1e71e6eff0 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 29 Jan 2020 18:06:25 +0000 Subject: [PATCH 11/13] Fix verification of the master key --- src/crypto/verification/QRCode.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index a4a37e0f0d9..663416dde57 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -71,10 +71,16 @@ export class ReciprocateQRCode extends Base { } // If we've gotten this far, verify the user's master cross signing key - const xsignInfo = this._baseApis.getStoredCrossSigningInfo(this.userId); + const xsignInfo = this._baseApis.getStoredCrossSigningForUser(this.userId); if (!xsignInfo) throw new Error("Missing cross signing info"); const masterKey = xsignInfo.getId("master"); - await this._verifyKeys(this.userId, [masterKey, masterKey]); + const masterKeyId = `ed25519:${masterKey}`; + await this._verifyKeys(this.userId, {[masterKeyId]:masterKey}, (keyId, device, keyInfo) => { + console.log({keyId, device, keyInfo}); + if (keyId !== masterKeyId) { + throw newKeyMismatchError(); + } + }); } } From 769bfeb10f0151c1dc7a47c8f10e1bb057f9dbf7 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 30 Jan 2020 11:10:03 +0000 Subject: [PATCH 12/13] Verify all the things --- src/crypto/verification/QRCode.js | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index 663416dde57..3408e79d1a6 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -77,10 +77,30 @@ export class ReciprocateQRCode extends Base { const masterKey = xsignInfo.getId("master"); const masterKeyId = `ed25519:${masterKey}`; await this._verifyKeys(this.userId, {[masterKeyId]:masterKey}, (keyId, device, keyInfo) => { - console.log({keyId, device, keyInfo}); + if (keyInfo !== masterKey) { + console.error("key ID from key info does not match"); + throw newKeyMismatchError(); + } if (keyId !== masterKeyId) { + console.error("key id doesn't match"); + throw newKeyMismatchError(); + } + if (device.deviceId !== masterKey) { + console.error("master key does not match device ID"); throw newKeyMismatchError(); } + for (const deviceKeyId in device.keys) { + if (deviceKeyId !== masterKeyId) { + console.error("device key ID does not match"); + throw newKeyMismatchError(); + } + if (device.keys[deviceKeyId] !== masterKey) { + console.error("master key does not match"); + throw newKeyMismatchError(); + } + } + + // Otherwise it is probably fine }); } } From 3645764f9ad81a47b56d07abb5d030e6b61124a3 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 30 Jan 2020 11:15:25 +0000 Subject: [PATCH 13/13] Appease the linter --- src/crypto/verification/QRCode.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/crypto/verification/QRCode.js b/src/crypto/verification/QRCode.js index 3408e79d1a6..8fc08e6f94d 100644 --- a/src/crypto/verification/QRCode.js +++ b/src/crypto/verification/QRCode.js @@ -76,7 +76,8 @@ export class ReciprocateQRCode extends Base { const masterKey = xsignInfo.getId("master"); const masterKeyId = `ed25519:${masterKey}`; - await this._verifyKeys(this.userId, {[masterKeyId]:masterKey}, (keyId, device, keyInfo) => { + const keys = {[masterKeyId]: masterKey}; + await this._verifyKeys(this.userId, keys, (keyId, device, keyInfo) => { if (keyInfo !== masterKey) { console.error("key ID from key info does not match"); throw newKeyMismatchError();