Merge pull request #335 from matrix-org/matthew/warn-unknown-devices

Support for warning users when unknown devices show up

docs/warning-on-unverified-devices.txt

@@ -0,0 +1,31 @@
+Random notes from Matthew on the two possible approaches for warning users about unexpected
+unverified devices popping up in their rooms....
+
+Original idea...
+================
+
+Warn when an existing user adds an unknown device to a room.
+
+Warn when a user joins the room with unverified or unknown devices.
+
+Warn when you initial sync if the room has any unverified devices in it.
+ ^ this is good enough if we're doing local storage.
+ OR, better:
+Warn when you initial sync if the room has any new undefined devices since you were last there.
+ => This means persisting the rooms that devices are in, across initial syncs.
+
+
+Updated idea...
+===============
+
+Warn when the user tries to send a message:
+  - If the room has unverified devices which the user has not yet been told about in the context of this room
+    ...or in the context of this user?  currently all verification is per-user, not per-room.
+    ...this should be good enough.
+
+  - so track whether we have warned the user or not about unverified devices - blocked, unverified, verified, unverified_warned.
+    throw an error when trying to encrypt if there are pure unverified devices there
+    app will have to search for the devices which are pure unverified to warn about them - have to do this from MembersList anyway?
+      - or megolm could warn which devices are causing the problems.
+
+Why do we wait to establish outbound sessions?  It just makes a horrible pause when we first try to send a message... but could otherwise unnecessarily consume resources?

spec/integ/megolm.spec.js

@@ -97,7 +97,10 @@ TestClient.prototype.start = function(existingDevices) {
         }};
     });
 
-    this.client.startClient();
+    this.client.startClient({
+        // set this so that we can get hold of failed events
+        pendingEventOrdering: 'detached',
+    });
 
     return this.httpBackend.flush();
 };
@@ -533,11 +536,24 @@ describe("megolm", function() {
             aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);
             return aliceTestClient.httpBackend.flush('/sync', 1);
         }).then(function() {
-            let inboundGroupSession;
+            // start out with the device unknown - the send should be rejected.
             aliceTestClient.httpBackend.when('POST', '/keys/query').respond(
                 200, getTestKeysQueryResponse('@bob:xyz'),
             );
 
+            return q.all([
+                aliceTestClient.client.sendTextMessage(ROOM_ID, 'test').then(() => {
+                    throw new Error("sendTextMessage failed on an unknown device");
+                }, (e) => {
+                    expect(e.name).toEqual("UnknownDeviceError");
+                }),
+                aliceTestClient.httpBackend.flush(),
+            ]);
+        }).then(function() {
+            // mark the device as known, and resend.
+            aliceTestClient.client.setDeviceKnown('@bob:xyz', 'DEVICE_ID');
+
+            let inboundGroupSession;
             aliceTestClient.httpBackend.when(
                 'PUT', '/sendToDevice/m.room.encrypted/',
             ).respond(200, function(path, content) {
@@ -568,8 +584,11 @@ describe("megolm", function() {
                 };
             });
 
+            const room = aliceTestClient.client.getRoom(ROOM_ID);
+            const pendingMsg = room.getPendingEvents()[0];
+
             return q.all([
-                aliceTestClient.client.sendTextMessage(ROOM_ID, 'test'),
+                aliceTestClient.client.resendEvent(pendingMsg, room),
                 aliceTestClient.httpBackend.flush(),
             ]);
         }).nodeify(done);
@@ -678,12 +697,21 @@ describe("megolm", function() {
 
             return aliceTestClient.httpBackend.flush('/sync', 1);
         }).then(function() {
-            console.log('Telling alice to send a megolm message');
+            console.log("Fetching bob's devices and marking known");
 
             aliceTestClient.httpBackend.when('POST', '/keys/query').respond(
                 200, getTestKeysQueryResponse('@bob:xyz'),
             );
 
+            return q.all([
+                aliceTestClient.client.downloadKeys(['@bob:xyz']),
+                aliceTestClient.httpBackend.flush(),
+            ]).then((keys) => {
+                aliceTestClient.client.setDeviceKnown('@bob:xyz', 'DEVICE_ID');
+            });
+        }).then(function() {
+            console.log('Telling alice to send a megolm message');
+
             aliceTestClient.httpBackend.when(
                 'PUT', '/sendToDevice/m.room.encrypted/',
             ).respond(200, function(path, content) {
@@ -737,7 +765,7 @@ describe("megolm", function() {
     // https://github.com/vector-im/riot-web/issues/2676
     it("Alice should send to her other devices", function(done) {
         // for this test, we make the testOlmAccount be another of Alice's devices.
-        // it ought to get include in messages Alice sends.
+        // it ought to get included in messages Alice sends.
 
         let p2pSession;
         let inboundGroupSession;
@@ -774,6 +802,7 @@ describe("megolm", function() {
 
             return aliceTestClient.httpBackend.flush();
         }).then(function() {
+            aliceTestClient.client.setDeviceKnown(aliceTestClient.userId, 'DEVICE_ID');
             aliceTestClient.httpBackend.when('POST', '/keys/claim').respond(
                 200, function(path, content) {
                 expect(content.one_time_keys[aliceTestClient.userId].DEVICE_ID)
@@ -882,10 +911,15 @@ describe("megolm", function() {
 
             // this will block
             downloadPromise = aliceTestClient.client.downloadKeys(['@bob:xyz']);
-        }).then(function() {
+
             // so will this.
-            sendPromise = aliceTestClient.client.sendTextMessage(ROOM_ID, 'test');
-        }).then(function() {
+            sendPromise = aliceTestClient.client.sendTextMessage(ROOM_ID, 'test')
+            .then(() => {
+                throw new Error("sendTextMessage failed on an unknown device");
+            }, (e) => {
+                expect(e.name).toEqual("UnknownDeviceError");
+            });
+
             aliceTestClient.httpBackend.when('POST', '/keys/query').respond(
                 200, getTestKeysQueryResponse('@bob:xyz'),
             );

src/client.js

@@ -395,11 +395,29 @@ MatrixClient.prototype.setDeviceBlocked = function(userId, deviceId, blocked) {
     _setDeviceVerification(this, userId, deviceId, null, blocked);
 };
 
-function _setDeviceVerification(client, userId, deviceId, verified, blocked) {
+/**
+ * Mark the given device as known/unknown
+ *
+ * @param {string} userId owner of the device
+ * @param {string} deviceId unique identifier for the device
+ *
+ * @param {boolean=} known whether to mark the device as known. defaults
+ *   to 'true'.
+ *
+ * @fires module:client~event:MatrixClient"deviceVerificationChanged"
+ */
+MatrixClient.prototype.setDeviceKnown = function(userId, deviceId, known) {
+    if (known === undefined) {
+        known = true;
+    }
+    _setDeviceVerification(this, userId, deviceId, null, null, known);
+};
+
+function _setDeviceVerification(client, userId, deviceId, verified, blocked, known) {
     if (!client._crypto) {
         throw new Error("End-to-End encryption disabled");
     }
-    client._crypto.setDeviceVerification(userId, deviceId, verified, blocked);
+    client._crypto.setDeviceVerification(userId, deviceId, verified, blocked, known);
     client.emit("deviceVerificationChanged", userId, deviceId);
 }
 

src/crypto/algorithms/base.js

@@ -157,6 +157,23 @@ module.exports.DecryptionError = function(msg) {
 };
 utils.inherits(module.exports.DecryptionError, Error);
 
+/**
+ * Exception thrown specifically when we want to warn the user to consider
+ * the security of their conversation before continuing
+ *
+ * @constructor
+ * @param {string} msg message describing the problem
+ * @param {Object} devices userId -> {deviceId -> object}
+ *      set of unknown devices per user we're warning about
+ * @extends Error
+ */
+module.exports.UnknownDeviceError = function(msg, devices) {
+    this.name = "UnknownDeviceError";
+    this.message = msg;
+    this.devices = devices;
+};
+utils.inherits(module.exports.UnknownDeviceError, Error);
+
 /**
  * Registers an encryption/decryption class for a particular algorithm
  *

src/crypto/algorithms/megolm.js

@@ -388,6 +388,10 @@ MegolmEncryption.prototype._shareKeyWithDevices = function(session, devicesByUse
 MegolmEncryption.prototype.encryptMessage = function(room, eventType, content) {
     const self = this;
     return this._getDevicesInRoom(room).then(function(devicesInRoom) {
+        // check if any of these devices are not yet known to the user.
+        // if so, warn the user so they can verify or ignore.
+        self._checkForUnknownDevices(devicesInRoom);
+
         return self._ensureOutboundSession(devicesInRoom);
     }).then(function(session) {
         const payloadJson = {
@@ -415,6 +419,37 @@ MegolmEncryption.prototype.encryptMessage = function(room, eventType, content) {
     });
 };
 
+/**
+ * Checks the devices we're about to send to and see if any are entirely
+ * unknown to the user.  If so, warn the user, and mark them as known to
+ * give the user a chance to go verify them before re-sending this message.
+ *
+ * @param {Object} devicesInRoom userId -> {deviceId -> object}
+ *   devices we should shared the session with.
+ */
+MegolmEncryption.prototype._checkForUnknownDevices = function(devicesInRoom) {
+    const unknownDevices = {};
+
+    Object.keys(devicesInRoom).forEach((userId)=>{
+        Object.keys(devicesInRoom[userId]).forEach((deviceId)=>{
+            const device = devicesInRoom[userId][deviceId];
+            if (device.isUnverified() && !device.isKnown()) {
+                if (!unknownDevices[userId]) {
+                    unknownDevices[userId] = {};
+                }
+                unknownDevices[userId][deviceId] = device;
+            }
+        });
+    });
+
+    if (Object.keys(unknownDevices).length) {
+        // it'd be kind to pass unknownDevices up to the user in this error
+        throw new base.UnknownDeviceError(
+            "This room contains unknown devices which have not been verified. " +
+            "We strongly recommend you verify them before continuing.", unknownDevices);
+    }
+};
+
 /**
  * Get the list of unblocked devices for all users in the room
  *
@@ -433,6 +468,11 @@ MegolmEncryption.prototype._getDevicesInRoom = function(room) {
     // have a list of the user's devices, then we already share an e2e room
     // with them, which means that they will have announced any new devices via
     // an m.new_device.
+    //
+    // XXX: what if the cache is stale, and the user left the room we had in
+    // common and then added new devices before joining this one? --Matthew
+    //
+    // yup, see https://github.com/vector-im/riot-web/issues/2305 --richvdh
     return this._crypto.downloadKeys(roomMembers, false).then(function(devices) {
         // remove any blocked devices
         for (const userId in devices) {

src/crypto/deviceinfo.js

@@ -34,7 +34,11 @@ limitations under the License.
   *      <key type>:<id> -> <base64-encoded key>>
   *
   * @property {module:crypto/deviceinfo.DeviceVerification} verified
-  *     whether the device has been verified by the user
+  *     whether the device has been verified/blocked by the user
+  *
+  * @property {boolean} known
+  *     whether the user knows of this device's existence (useful when warning
+  *     the user that a user has added new devices)
   *
   * @property {Object} unsigned  additional data from the homeserver
   *
@@ -50,6 +54,7 @@ function DeviceInfo(deviceId) {
     this.algorithms = [];
     this.keys = {};
     this.verified = DeviceVerification.UNVERIFIED;
+    this.known = false;
     this.unsigned = {};
 }
 
@@ -81,6 +86,7 @@ DeviceInfo.prototype.toStorage = function() {
         algorithms: this.algorithms,
         keys: this.keys,
         verified: this.verified,
+        known: this.known,
         unsigned: this.unsigned,
     };
 };
@@ -130,6 +136,24 @@ DeviceInfo.prototype.isVerified = function() {
     return this.verified == DeviceVerification.VERIFIED;
 };
 
+/**
+ * Returns true if this device is unverified
+ *
+ * @return {Boolean} true if unverified
+ */
+DeviceInfo.prototype.isUnverified = function() {
+    return this.verified == DeviceVerification.UNVERIFIED;
+};
+
+/**
+ * Returns true if the user knows about this device's existence
+ *
+ * @return {Boolean} true if known
+ */
+DeviceInfo.prototype.isKnown = function() {
+    return this.known == true;
+};
+
 /**
  * @enum
  */

src/crypto/index.js

@@ -96,6 +96,7 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId) {
             keys: this._deviceKeys,
             algorithms: this._supportedAlgorithms,
             verified: DeviceVerification.VERIFIED,
+            known: true,
         };
 
         myDevices[this._deviceId] = deviceInfo;
@@ -684,8 +685,12 @@ Crypto.prototype.getDeviceByIdentityKey = function(userId, algorithm, sender_key
  *
  * @param {?boolean} blocked whether to mark the device as blocked. Null to
  *      leave unchanged.
+ *
+ * @param {?boolean} known whether to mark that the user has been made aware of
+ *      the existence of this device. Null to leave unchanged
  */
-Crypto.prototype.setDeviceVerification = function(userId, deviceId, verified, blocked) {
+Crypto.prototype.setDeviceVerification = function(userId, deviceId, verified,
+                                                  blocked, known) {
     const devices = this._sessionStore.getEndToEndDevicesForUser(userId);
     if (!devices || !devices[deviceId]) {
         throw new Error("Unknown device " + userId + ":" + deviceId);
@@ -706,10 +711,16 @@ Crypto.prototype.setDeviceVerification = function(userId, deviceId, verified, bl
         verificationStatus = DeviceVerification.UNVERIFIED;
     }
 
-    if (dev.verified === verificationStatus) {
+    let knownStatus = dev.known;
+    if (known !== null && known !== undefined) {
+        knownStatus = known;
+    }
+
+    if (dev.verified === verificationStatus && dev.known === knownStatus) {
         return;
     }
     dev.verified = verificationStatus;
+    dev.known = knownStatus;
     this._sessionStore.storeEndToEndDevicesForUser(userId, devices);
 };