Merge pull request #1077 from matrix-org/bwindels/dm-verif-in-e2ee-rooms

Fix SAS verification in encrypted DMs

src/crypto/index.js

@@ -69,6 +69,30 @@ export function isCryptoAvailable() {
     return Boolean(global.Olm);
 }
 
+/* subscribes to timeline events / to_device 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);
+    }
+    client.on("event", listener);
+    let subscribed = true;
+    return function() {
+        if (subscribed) {
+            if (isEncrypted) {
+                client.off("Event.decrypted", listener);
+            }
+            client.off("event", listener);
+            subscribed = false;
+        }
+        return null;
+    };
+}
+
 const MIN_FORCE_SESSION_INTERVAL_MS = 60 * 60 * 1000;
 const KEY_BACKUP_KEYS_PER_REQUEST = 200;
 
@@ -1278,13 +1302,15 @@ function verificationEventHandler(target, userId, roomId, eventId) {
             || event.getSender() !== userId) {
             return;
         }
-        const content = event.getContent();
-        if (!content["m.relates_to"]) {
+        // 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
-              = 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) {
@@ -1341,7 +1367,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;
             }
@@ -1351,14 +1379,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);
         };
     });
@@ -1393,7 +1426,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;
 };
 

src/crypto/verification/Base.js

@@ -75,14 +75,15 @@ 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();
 
         if (this.roomId) {
-            this._send = this._sendMessage;
+            this._sendWithTxnId = this._sendMessage;
         } else {
-            this._send = this._sendToDevice;
+            this._sendWithTxnId = this._sendToDevice;
         }
     }
 
@@ -106,13 +107,43 @@ 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) {
+        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 +155,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);
     }
 
@@ -223,6 +248,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);
@@ -247,7 +276,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);
             };
@@ -255,7 +287,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);
             };
@@ -309,4 +344,8 @@ export default class VerificationBase extends EventEmitter {
             await this._baseApis.setDeviceVerified(userId, deviceId);
         }
     }
+
+    setEventsSubscription(subscription) {
+        this._eventsSubscription = subscription;
+    }
 }

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");
@@ -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