From 180fea8ace8a50471f43ef8ccae17a7a1ac8c805 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 18 Nov 2019 18:30:43 +0100 Subject: [PATCH 1/5] only send decrypted events to Verifier in e2ee rooms --- src/crypto/index.js | 45 +++++++++++++++++++++++++++++---- src/crypto/verification/Base.js | 19 ++++++++++++-- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 11b6ff07a..36a8c86d5 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -65,6 +65,32 @@ export function isCryptoAvailable() { return Boolean(global.Olm); } +/* subscribes to timeline events for SAS verification */ +function listenForEvents(client, roomId, listener) { + let isEncrypted = false; + if (roomId) { + isEncrypted = client.isRoomEncrypted(roomId); + } + + if (isEncrypted) { + client.on("Event.decrypted", listener); + } else { + client.on("event", listener); + } + let subscribed = true; + return function() { + if (subscribed) { + if (isEncrypted) { + client.off("Event.decrypted", listener); + } else { + client.off("event", listener); + } + subscribed = false; + } + return null; + }; +} + const MIN_FORCE_SESSION_INTERVAL_MS = 60 * 60 * 1000; const KEY_BACKUP_KEYS_PER_REQUEST = 200; @@ -826,7 +852,9 @@ Crypto.prototype.requestVerificationDM = async function(userId, roomId, methods) ); // this handler gets removed when the verification finishes // (see the verify method of crypto/verification/Base.js) - this._baseApis.on("event", verifier.handler); + const subscription = + listenForEvents(this._baseApis, roomId, verifier.handler); + verifier.setEventsSubscription(subscription); resolve(verifier); break; } @@ -836,14 +864,19 @@ Crypto.prototype.requestVerificationDM = async function(userId, roomId, methods) } } }; - this._baseApis.on("event", listener); + let initialResponseSubscription = + listenForEvents(this._baseApis, roomId, listener); const resolve = (...args) => { - this._baseApis.off("event", listener); + if (initialResponseSubscription) { + initialResponseSubscription = initialResponseSubscription(); + } _resolve(...args); }; const reject = (...args) => { - this._baseApis.off("event", listener); + if (initialResponseSubscription) { + initialResponseSubscription = initialResponseSubscription(); + } _reject(...args); }; }); @@ -878,7 +911,9 @@ Crypto.prototype.acceptVerificationDM = function(event, Method) { verifier.handler = verificationEventHandler( verifier, event.getSender(), event.getRoomId(), event.getId(), ); - this._baseApis.on("event", verifier.handler); + const subscription = listenForEvents( + this._baseApis, event.getRoomId(), verifier.handler); + verifier.setEventsSubscription(subscription); return verifier; }; diff --git a/src/crypto/verification/Base.js b/src/crypto/verification/Base.js index fa67c72a0..d9404ba63 100644 --- a/src/crypto/verification/Base.js +++ b/src/crypto/verification/Base.js @@ -74,6 +74,7 @@ export default class VerificationBase extends EventEmitter { this._done = false; this._promise = null; this._transactionTimeoutTimer = null; + this._eventsSubscription = null; // At this point, the verification request was received so start the timeout timer. this._resetTimer(); @@ -222,6 +223,10 @@ export default class VerificationBase extends EventEmitter { // but no reject function. If cancel is called again, we'd error. if (this._reject) this._reject(e); } else { + // unsubscribe from events, this happens in _reject usually but we don't have one here + if (this._eventsSubscription) { + this._eventsSubscription = this._eventsSubscription(); + } // FIXME: this causes an "Uncaught promise" console message // if nothing ends up chaining this promise. this._promise = Promise.reject(e); @@ -246,7 +251,10 @@ export default class VerificationBase extends EventEmitter { this._done = true; this._endTimer(); if (this.handler) { - this._baseApis.off("event", this.handler); + // these listeners are attached in Crypto.acceptVerificationDM + if (this._eventsSubscription) { + this._eventsSubscription = this._eventsSubscription(); + } } resolve(...args); }; @@ -254,7 +262,10 @@ export default class VerificationBase extends EventEmitter { this._done = true; this._endTimer(); if (this.handler) { - this._baseApis.off("event", this.handler); + // these listeners are attached in Crypto.acceptVerificationDM + if (this._eventsSubscription) { + this._eventsSubscription = this._eventsSubscription(); + } } reject(...args); }; @@ -295,4 +306,8 @@ export default class VerificationBase extends EventEmitter { await this._baseApis.setDeviceVerified(userId, deviceId); } } + + setEventsSubscription(subscription) { + this._eventsSubscription = subscription; + } } From cd735ef459ac4c6d175daef03bad44e4a4f3540e Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 18 Nov 2019 18:31:39 +0100 Subject: [PATCH 2/5] use getRelation as getContent()[m.relates_to] doesn't work in e2ee rooms --- src/crypto/index.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index 36a8c86d5..d598ea12f 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -789,13 +789,9 @@ function verificationEventHandler(target, userId, roomId, eventId) { || event.getSender() !== userId) { return; } - const content = event.getContent(); - if (!content["m.relates_to"]) { - return; - } - const relatesTo - = content["m.relationship"] || content["m.relates_to"]; - if (!relatesTo.rel_type + const relatesTo = event.getRelation(); + if (!relatesTo + || !relatesTo.rel_type || relatesTo.rel_type !== "m.reference" || !relatesTo.event_id || relatesTo.event_id !== eventId) { From 24ae7877364ae8e61f799743d4c179e25fd64241 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 18 Nov 2019 18:33:11 +0100 Subject: [PATCH 3/5] make it explicit that the transaction id is added for the start event as it should included in the commitment hash --- src/crypto/verification/Base.js | 31 ++++++++++++++++++++++--------- src/crypto/verification/SAS.js | 10 +++++----- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/crypto/verification/Base.js b/src/crypto/verification/Base.js index d9404ba63..3a9f76039 100644 --- a/src/crypto/verification/Base.js +++ b/src/crypto/verification/Base.js @@ -80,9 +80,9 @@ export default class VerificationBase extends EventEmitter { this._resetTimer(); if (this.roomId) { - this._send = this._sendMessage; + this._sendWithTxnId = this._sendMessage; } else { - this._send = this._sendToDevice; + this._sendWithTxnId = this._sendToDevice; } } @@ -106,13 +106,32 @@ export default class VerificationBase extends EventEmitter { } } + + /* creates a content object with the transaction id added to it */ + _contentWithTxnId(content) { + const copy = Object.assign({}, content); + if (this.roomId) { // verification as timeline event + copy["m.relates_to"] = { + rel_type: "m.reference", + event_id: this.transactionId, + }; + } else { // verification as to_device event + copy.transaction_id = this.transactionId; + } + return copy; + } + + _send(type, contentWithoutTxnId) { + const content = this._contentWithTxnId(contentWithoutTxnId); + return this._sendWithTxnId(type, content); + } + /* send a message to the other participant, using to-device messages */ _sendToDevice(type, content) { if (this._done) { return Promise.reject(new Error("Verification is already done")); } - content.transaction_id = this.transactionId; return this._baseApis.sendToDevice(type, { [this.userId]: { [this.deviceId]: content }, }); @@ -124,12 +143,6 @@ export default class VerificationBase extends EventEmitter { if (this._done) { return Promise.reject(new Error("Verification is already done")); } - // FIXME: if MSC1849 decides to use m.relationship instead of - // m.relates_to, we should follow suit here - content["m.relates_to"] = { - rel_type: "m.reference", - event_id: this.transactionId, - }; return this._baseApis.sendEvent(this.roomId, type, content); } diff --git a/src/crypto/verification/SAS.js b/src/crypto/verification/SAS.js index a2af399cf..9d3e7286c 100644 --- a/src/crypto/verification/SAS.js +++ b/src/crypto/verification/SAS.js @@ -205,7 +205,7 @@ export default class SAS extends Base { } async _doSendVerification() { - const initialMessage = { + const initialMessage = this._contentWithTxnId({ method: SAS.NAME, from_device: this._baseApis.deviceId, key_agreement_protocols: KEY_AGREEMENT_LIST, @@ -213,10 +213,10 @@ export default class SAS extends Base { message_authentication_codes: MAC_LIST, // FIXME: allow app to specify what SAS methods can be used short_authentication_string: SAS_LIST, - }; - // NOTE: this._send will modify initialMessage to include the - // transaction_id field, or the m.relationship/m.relates_to field - this._send("m.key.verification.start", initialMessage); + }); + // add the transaction id to the message beforehand because + // it needs to be included in the commitment hash later on + this._sendWithTxnId("m.key.verification.start", initialMessage); let e = await this._waitForEvent("m.key.verification.accept"); From 3b02b62ba53d693bff59562c4bf73149f77528df Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 18 Nov 2019 18:34:05 +0100 Subject: [PATCH 4/5] add m.relates_to back to the content on the requesting side for e2e room as it needs to be added to the commitment hash as before, getContent() in an e2ee room doesn't return the relation --- src/crypto/verification/Base.js | 11 +++++++++++ src/crypto/verification/SAS.js | 5 ++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/crypto/verification/Base.js b/src/crypto/verification/Base.js index 3a9f76039..553de34af 100644 --- a/src/crypto/verification/Base.js +++ b/src/crypto/verification/Base.js @@ -106,6 +106,17 @@ export default class VerificationBase extends EventEmitter { } } + _contentFromEventWithTxnId(event) { + if (this.roomId) { // verification as timeline event + // ensure m.related_to is included in e2ee rooms + // as the field is excluded from encryption + const content = Object.assign({}, event.getContent()); + content["m.relates_to"] = event.getRelation(); + return content; + } else { // verification as to_device event + return event.getContent(); + } + } /* creates a content object with the transaction id added to it */ _contentWithTxnId(content) { diff --git a/src/crypto/verification/SAS.js b/src/crypto/verification/SAS.js index 9d3e7286c..720e5e80c 100644 --- a/src/crypto/verification/SAS.js +++ b/src/crypto/verification/SAS.js @@ -281,7 +281,10 @@ export default class SAS extends Base { } async _doRespondVerification() { - let content = this.startEvent.getContent(); + // as m.related_to is not included in the encrypted content in e2e rooms, + // we need to make sure it is added + let content = this._contentFromEventWithTxnId(this.startEvent); + // Note: we intersect using our pre-made lists, rather than the sets, // so that the result will be in our order of preference. Then // fetching the first element from the array will give our preferred From 90512bdd5f4cb5f05d9d3c0c733a7c091bb027d9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 19 Nov 2019 15:23:05 +0100 Subject: [PATCH 5/5] also listen for non-encrypted events when verifying over DM --- src/crypto/index.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index d598ea12f..92c4686f1 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -65,7 +65,7 @@ export function isCryptoAvailable() { return Boolean(global.Olm); } -/* subscribes to timeline events for SAS verification */ +/* subscribes to timeline events / to_device events for SAS verification */ function listenForEvents(client, roomId, listener) { let isEncrypted = false; if (roomId) { @@ -74,17 +74,15 @@ function listenForEvents(client, roomId, listener) { if (isEncrypted) { client.on("Event.decrypted", listener); - } else { - client.on("event", listener); } + client.on("event", listener); let subscribed = true; return function() { if (subscribed) { if (isEncrypted) { client.off("Event.decrypted", listener); - } else { - client.off("event", listener); } + client.off("event", listener); subscribed = false; } return null; @@ -789,6 +787,12 @@ function verificationEventHandler(target, userId, roomId, eventId) { || event.getSender() !== userId) { return; } + // ignore events that haven't been decrypted yet. + // we also listen for undecrypted events, just in case + // the other side would be sending unencrypted events in an e2ee room + if (event.getType() === "m.room.encrypted") { + return; + } const relatesTo = event.getRelation(); if (!relatesTo || !relatesTo.rel_type