From 4a40c10d4cce4af31ae0f9be7e1c1a76e398be0d Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 21 Nov 2019 17:13:43 +0100 Subject: [PATCH 1/5] add helper method on event for current age according to local clock --- src/models/event.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/models/event.js b/src/models/event.js index 6aa39c943..83ca20d2e 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -297,6 +297,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { return this.getUnsigned().age || this.event.age; // v2 / v1 }, + /** + * Get the age of the event when this function was called. + * Relies on the local clock being in sync with the clock of the original homeserver. + * @return {Number} The age of this event in milliseconds. + */ + getLocalAge: function() { + return Date.now() - this.getTs(); + }, + /** * Get the event state_key if it has one. This will return undefined * for message events. From ac1173c628a2c70cdfaaac9efc95cfb558c773be Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Nov 2019 16:11:49 +0100 Subject: [PATCH 2/5] also emit `crypto.verification.request` for verification over DM --- src/crypto/index.js | 60 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/crypto/index.js b/src/crypto/index.js index 11b450a8e..cc3dc1f96 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -65,6 +65,9 @@ export const verificationMethods = { SAS: SAS.NAME, }; +const VERIFICATION_REQUEST_TIMEOUT = 5 * 60 * 1000; //5m +const VERIFICATION_REQUEST_MARGIN = 3 * 1000; //3s + export function isCryptoAvailable() { return Boolean(global.Olm); } @@ -919,6 +922,17 @@ Crypto.prototype.registerEventHandlers = function(eventEmitter) { eventEmitter.on("toDeviceEvent", function(event) { crypto._onToDeviceEvent(event); }); + + // only sync timeline events, not events when backpaginating, loading, ... + eventEmitter.on("Room.timeline", function(event) { + if (event.getRoomId()) { + crypto._onTimelineEvent(event); + } + }); + + eventEmitter.on("Event.decrypted", function(event) { + crypto._onTimelineEvent(event); + }); }; @@ -2584,6 +2598,52 @@ Crypto.prototype._onKeyVerificationMessage = function(event) { } }; +/** + * Handle key verification requests sent as timeline events + * + * @private + * @param {module:models/event.MatrixEvent} event the timeline event + */ +Crypto.prototype._onTimelineEvent = function(event) { + if (event.getType() !== "m.room.message") { + return; + } + const content = event.getContent(); + if (content.msgtype !== "m.key.verification.request") { + return; + } + // ignore event if malformed + if (!("from_device" in content) || typeof content.from_device !== "string" + || !("methods" in content) || !Array.isArray(content.methods) + || !("to" in content) || typeof content.to !== "string") { + logger.warn("received invalid verification request over DM from " + + event.getSender()); + return; + } + // check the request was directed to the syncing user + if (content.to !== this._baseApis.getUserId()) { + return; + } + + const timeout = VERIFICATION_REQUEST_TIMEOUT - VERIFICATION_REQUEST_MARGIN; + if (event.getLocalAge() >= timeout) { + return; + } + const request = { + event, + methods: content.methods, + beginKeyVerification: (method) => { + const verifier = this.acceptVerificationDM(event, method); + return verifier; + }, + cancel: () => { + const verifier = this.acceptVerificationDM(event, content.methods[0]); + verifier.cancel("User declined"); + }, + }; + this._baseApis.emit("crypto.verification.request", request); +}; + /** * Handle a toDevice event that couldn't be decrypted * From ca89b6e7a81b097f1938c092d15a7bd57ccf689e Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Nov 2019 16:12:19 +0100 Subject: [PATCH 3/5] use adapter for to_device requests to have same api as for verif over DM Riot doesn't fully implement to_device verifications, e.g. it doesn't send a `request` but immediately sends a `start` event. Because of this, `crypto.verification.request` doesn't get fired, as that code path doesn't get triggered. This is why MatrixChat in the react-sdk was listening for `crypto.verification.start`. Verification over DM *does* send a `request` event first, so to have the same API for both methods, we fake the request and wrap the verifier in it. --- src/client.js | 2 ++ src/crypto/index.js | 31 +++++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/client.js b/src/client.js index 187944420..f2e74fc99 100644 --- a/src/client.js +++ b/src/client.js @@ -5201,6 +5201,8 @@ module.exports.CRYPTO_ENABLED = CRYPTO_ENABLED; * @param {object} data * @param {MatrixEvent} data.event the original verification request message * @param {Array} data.methods the verification methods that can be used + * @param {Number} data.timeout the amount of milliseconds that should be waited + * before cancelling the request automatically. * @param {Function} data.beginKeyVerification a function to call if a key * verification should be performed. The function takes one argument: the * name of the key verification method (taken from data.methods) to use. diff --git a/src/crypto/index.js b/src/crypto/index.js index cc3dc1f96..496aca155 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -2357,8 +2357,8 @@ Crypto.prototype._onKeyVerificationRequest = function(event) { const content = event.getContent(); if (!("from_device" in content) || typeof content.from_device !== "string" - || !("transaction_id" in content) || typeof content.from_device !== "string" - || !("methods" in content) || !(content.methods instanceof Array) + || !("transaction_id" in content) + || !("methods" in content) || !Array.isArray(content.methods) || !("timestamp" in content) || typeof content.timestamp !== "number") { logger.warn("received invalid verification request from " + event.getSender()); // ignore event if malformed @@ -2434,6 +2434,7 @@ Crypto.prototype._onKeyVerificationRequest = function(event) { // notify the application of the verification request, so it can // decide what to do with it const request = { + timeout: VERIFICATION_REQUEST_TIMEOUT, event: event, methods: methods, beginKeyVerification: (method) => { @@ -2566,6 +2567,31 @@ Crypto.prototype._onKeyVerificationStart = function(event) { } } this._baseApis.emit("crypto.verification.start", verifier); + + // Riot does not implement `m.key.verification.request` while + // verifying over to_device messages, but the protocol is made to + // work when starting with a `m.key.verification.start` event straight + // away as well. Verification over DM *does* start with a request event first, + // and to expose a uniform api to monitor verification requests, we mock + // the request api here for to_device messages. + // "crypto.verification.start" is kept for backwards compatibility. + + // ahh, this will create 2 request notifications for clients that do support the request event + // so maybe we should remove emitting the request when actually receiving it *sigh* + const requestAdapter = { + event, + timeout: VERIFICATION_REQUEST_TIMEOUT, + methods: [content.method], + beginKeyVerification: (method) => { + if (content.method === method) { + return verifier; + } + }, + cancel: () => { + return verifier.cancel("User cancelled"); + }, + }; + this._baseApis.emit("crypto.verification.request", requestAdapter); } }; @@ -2631,6 +2657,7 @@ Crypto.prototype._onTimelineEvent = function(event) { } const request = { event, + timeout: VERIFICATION_REQUEST_TIMEOUT, methods: content.methods, beginKeyVerification: (method) => { const verifier = this.acceptVerificationDM(event, method); From 51898cffe8e84714b988d494aea6d4e117e49cf1 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Nov 2019 17:31:48 +0100 Subject: [PATCH 4/5] add comments for timeout constants --- src/crypto/index.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/crypto/index.js b/src/crypto/index.js index 496aca155..78cfdf576 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -65,7 +65,14 @@ export const verificationMethods = { SAS: SAS.NAME, }; +// the recommended amount of time before a verification request +// should be (automatically) cancelled without user interaction +// and ignored. const VERIFICATION_REQUEST_TIMEOUT = 5 * 60 * 1000; //5m +// to avoid almost expired verification notifications +// from showing a notification and almost immediately +// disappearing, also ignore verification requests that +// are this amount of time away from expiring. const VERIFICATION_REQUEST_MARGIN = 3 * 1000; //3s export function isCryptoAvailable() { From 6952db67625f8e1af7d5055505f75a0c8e960a75 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 22 Nov 2019 17:32:37 +0100 Subject: [PATCH 5/5] no need to filter here anymore when listening for timeline, also remove obsolete docs --- src/crypto/index.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 78cfdf576..c987d03cc 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -930,11 +930,8 @@ Crypto.prototype.registerEventHandlers = function(eventEmitter) { crypto._onToDeviceEvent(event); }); - // only sync timeline events, not events when backpaginating, loading, ... eventEmitter.on("Room.timeline", function(event) { - if (event.getRoomId()) { - crypto._onTimelineEvent(event); - } + crypto._onTimelineEvent(event); }); eventEmitter.on("Event.decrypted", function(event) {