Merge pull request #239 from matrix-org/rav/fix_unknown_key

Check recipient and sender in Olm messages

lib/crypto/OlmDevice.js

@@ -374,6 +374,10 @@ OlmDevice.prototype.encryptMessage = function(
 ) {
     var self = this;
 
+    if (payloadString === undefined) {
+        throw new Error("payloadString undefined");
+    }
+
     return this._getSession(theirDeviceIdentityKey, sessionId, function(session) {
         var res = session.encrypt(payloadString);
         self._saveSession(theirDeviceIdentityKey, session);

lib/crypto/algorithms/base.js

@@ -45,13 +45,15 @@ module.exports.DECRYPTION_CLASSES = {};
  * @alias module:crypto/algorithms/base.EncryptionAlgorithm
  *
  * @param {object} params parameters
+ * @param {string} params.userId  The UserID for the local user
  * @param {string} params.deviceId The identifier for this device.
  * @param {module:crypto} params.crypto crypto core
  * @param {module:crypto/OlmDevice} params.olmDevice olm.js wrapper
  * @param {module:base-apis~MatrixBaseApis} baseApis base matrix api interface
  * @param {string} params.roomId  The ID of the room we will be sending to
  */
 var EncryptionAlgorithm = function(params) {
+    this._userId = params.userId;
     this._deviceId = params.deviceId;
     this._crypto = params.crypto;
     this._olmDevice = params.olmDevice;
@@ -101,9 +103,11 @@ EncryptionAlgorithm.prototype.onNewDevice = function(userId, deviceId) {};
  * @alias module:crypto/algorithms/base.DecryptionAlgorithm
  *
  * @param {object} params parameters
+ * @param {string} params.userId  The UserID for the local user
  * @param {module:crypto/OlmDevice} params.olmDevice olm.js wrapper
  */
 var DecryptionAlgorithm = function(params) {
+    this._userId = params.userId;
     this._olmDevice = params.olmDevice;
 };
 /** */

lib/crypto/algorithms/megolm.js

@@ -244,17 +244,27 @@ MegolmEncryption.prototype._shareKeyWithDevices = function(session_id, shareMap)
 
                 var deviceInfo = sessionResult.device;
 
+                var encryptedContent = {
+                    algorithm: olmlib.OLM_ALGORITHM,
+                    sender_key: self._olmDevice.deviceCurve25519Key,
+                    ciphertext: {},
+                };
+
+                olmlib.encryptMessageForDevice(
+                    encryptedContent.ciphertext,
+                    self._userId,
+                    self._deviceId,
+                    self._olmDevice,
+                    userId,
+                    deviceInfo,
+                    payload
+                );
+
                 if (!contentMap[userId]) {
                     contentMap[userId] = {};
                 }
 
-                contentMap[userId][deviceId] =
-                    olmlib.encryptMessageForDevices(
-                        self._deviceId,
-                        self._olmDevice,
-                        [deviceInfo.getIdentityKey()],
-                        payload
-                    );
+                contentMap[userId][deviceId] = encryptedContent;
                 haveTargets = true;
             }
         }
@@ -413,21 +423,35 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
         throw new base.DecryptionError("Missing fields in input");
     }
 
+    var res;
     try {
-        var res = this._olmDevice.decryptGroupMessage(
+        res = this._olmDevice.decryptGroupMessage(
             event.room_id, content.sender_key, content.session_id, content.ciphertext
         );
-        if (res === null) {
-            return null;
-        }
-        return {
-            payload: JSON.parse(res.result),
-            keysClaimed: res.keysClaimed,
-            keysProved: res.keysProved,
-        };
     } catch (e) {
         throw new base.DecryptionError(e);
     }
+
+    if (res === null) {
+        return null;
+    }
+
+    var payload = JSON.parse(res.result);
+
+    // belt-and-braces check that the room id matches that indicated by the HS
+    // (this is somewhat redundant, since the megolm session is scoped to the
+    // room, so neither the sender nor a MITM can lie about the room_id).
+    if (payload.room_id !== event.room_id) {
+        throw new base.DecryptionError(
+            "Message intended for room " + payload.room_id
+        );
+    }
+
+    return {
+        payload: payload,
+        keysClaimed: res.keysClaimed,
+        keysProved: res.keysProved,
+    };
 };
 
 /**

lib/crypto/algorithms/olm.js

@@ -95,32 +95,43 @@ OlmEncryption.prototype.encryptMessage = function(room, eventType, content) {
 
     var self = this;
     return this._ensureSession(users).then(function() {
-        var participantKeys = [];
+        var payloadFields = {
+            room_id: room.roomId,
+            type: eventType,
+            content: content,
+        };
+
+        var encryptedContent = {
+            algorithm: olmlib.OLM_ALGORITHM,
+            sender_key: self._olmDevice.deviceCurve25519Key,
+            ciphertext: {},
+        };
+
         for (var i = 0; i < users.length; ++i) {
             var userId = users[i];
             var devices = self._crypto.getStoredDevicesForUser(userId);
+
             for (var j = 0; j < devices.length; ++j) {
                 var deviceInfo = devices[j];
                 var key = deviceInfo.getIdentityKey();
                 if (key == self._olmDevice.deviceCurve25519Key) {
-                    // don't bother setting up session to ourself
+                    // don't bother sending to ourself
                     continue;
                 }
                 if (deviceInfo.verified == DeviceVerification.BLOCKED) {
                     // don't bother setting up sessions with blocked users
                     continue;
                 }
-                participantKeys.push(key);
+
+                olmlib.encryptMessageForDevice(
+                    encryptedContent.ciphertext,
+                    self._userId, self._deviceId, self._olmDevice,
+                    userId, deviceInfo, payloadFields
+                );
             }
         }
 
-        return olmlib.encryptMessageForDevices(
-            self._deviceId, self._olmDevice, participantKeys, {
-                room_id: room.roomId,
-                type: eventType,
-                content: content,
-            }
-        );
+        return encryptedContent;
     });
 };
 
@@ -173,10 +184,70 @@ OlmDecryption.prototype.decryptEvent = function(event) {
         throw new base.DecryptionError("Bad Encrypted Message");
     }
 
-
-    // TODO: Check the sender user id matches the sender key.
-    // TODO: check the room_id and fingerprint
     var payload = JSON.parse(payloadString);
+
+    // check that we were the intended recipient, to avoid unknown-key attack
+    // https://github.com/vector-im/vector-web/issues/2483
+    if (payload.recipient === undefined) {
+        // older versions of riot did not set this field, so we cannot make
+        // this check. TODO: kill this off once our users have updated
+        console.warn(
+            "Olm event (id=" + event.event_id + ") contains no 'recipient' " +
+             "property; cannot prevent unknown-key attack");
+    } else if (payload.recipient != this._userId) {
+        console.warn(
+            "Event " + event.event_id + ": Intended recipient " +
+            payload.recipient + " does not match our id " + this._userId
+        );
+        throw new base.DecryptionError(
+            "Message was intented for " + payload.recipient
+        );
+    }
+
+    if (payload.recipient_keys === undefined) {
+        // ditto
+        console.warn(
+            "Olm event (id=" + event.event_id + ") contains no " +
+            "'recipient_keys' property; cannot prevent unknown-key attack");
+    } else if (payload.recipient_keys.ed25519 !=
+               this._olmDevice.deviceEd25519Key) {
+        console.warn(
+            "Event " + event.event_id + ": Intended recipient ed25519 key " +
+            payload.recipient_keys.ed25519 + " did not match ours"
+        );
+        throw new base.DecryptionError("Message not intended for this device");
+    }
+
+    // check that the original sender matches what the homeserver told us, to
+    // avoid people masquerading as others.
+    // (this check is also provided via the sender's embedded ed25519 key,
+    // which is checked elsewhere).
+    if (payload.sender === undefined) {
+        // ditto
+        console.warn(
+            "Olm event (id=" + event.event_id + ") contains no " +
+            "'sender' property; cannot prevent unknown-key attack");
+    } else if (payload.sender != event.sender) {
+        console.warn(
+            "Event " + event.event_id + ": original sender " + payload.sender +
+            " does not match reported sender " + event.sender
+        );
+        throw new base.DecryptionError(
+            "Message forwarded from " + payload.sender
+        );
+    }
+
+    // Olm events intended for a room have a room_id.
+    if (payload.room_id !== event.room_id) {
+        console.warn(
+            "Event " + event.event_id + ": original room " + payload.room_id +
+            " does not match reported room " + event.room_id
+        );
+        throw new base.DecryptionError(
+            "Message intended for room " + payload.room_id
+        );
+    }
+
     return {
         payload: payload,
         sessionExists: true,

lib/crypto/index.js

@@ -710,6 +710,7 @@ Crypto.prototype.setRoomEncryption = function(roomId, config) {
     this._sessionStore.storeEndToEndRoom(roomId, config);
 
     var alg = new AlgClass({
+        userId: this._userId,
         deviceId: this._deviceId,
         crypto: this,
         olmDevice: this._olmDevice,
@@ -917,6 +918,7 @@ Crypto.prototype.decryptEvent = function(event) {
         throw new algorithms.DecryptionError("Unable to decrypt " + content.algorithm);
     }
     var alg = new AlgClass({
+        userId: this._userId,
         olmDevice: this._olmDevice,
     });
     var r = alg.decryptEvent(event);
@@ -1100,6 +1102,7 @@ Crypto.prototype._onRoomKeyEvent = function(event) {
         );
     }
     var alg = new AlgClass({
+        userId: this._userId,
         olmDevice: this._olmDevice,
     });
     alg.onRoomKeyEvent(event);

lib/crypto/olmlib.js

@@ -34,22 +34,38 @@ module.exports.MEGOLM_ALGORITHM = "m.megolm.v1.aes-sha2";
 
 
 /**
- * Encrypt an event payload for a list of devices
+ * Encrypt an event payload for an Olm device
  *
+ * @param {Object<string, string>} resultsObject  The `ciphertext` property
+ *   of the m.room.encrypted event to which to add our result
+ *
+ * @param {string} ourUserId
  * @param {string} ourDeviceId
  * @param {module:crypto/OlmDevice} olmDevice olm.js wrapper
- * @param {string[]} participantKeys list of curve25519 keys to encrypt for
+ * @param {string} recipientUserId
+ * @param {module:crypto/deviceinfo} recipientDevice
  * @param {object} payloadFields fields to include in the encrypted payload
- *
- * @return {object} content for an m.room.encrypted event
  */
-module.exports.encryptMessageForDevices = function(
-    ourDeviceId, olmDevice, participantKeys, payloadFields
+module.exports.encryptMessageForDevice = function(
+    resultsObject,
+    ourUserId, ourDeviceId, olmDevice, recipientUserId, recipientDevice,
+    payloadFields
 ) {
-    participantKeys.sort();
-    var participantHash = ""; // Olm.sha256(participantKeys.join());
-    var payloadJson = {
-        fingerprint: participantHash,
+    var deviceKey = recipientDevice.getIdentityKey();
+    var sessionId = olmDevice.getSessionIdForDevice(deviceKey);
+    if (sessionId === null) {
+        // If we don't have a session for a device then
+        // we can't encrypt a message for it.
+        return;
+    }
+
+    console.log(
+        "Using sessionid " + sessionId + " for device " +
+            recipientUserId + ":" + recipientDevice.deviceId
+    );
+
+    var payload = {
+        sender: ourUserId,
         sender_device: ourDeviceId,
 
         // Include the Ed25519 key so that the recipient knows what
@@ -63,28 +79,24 @@ module.exports.encryptMessageForDevices = function(
         keys: {
             "ed25519": olmDevice.deviceEd25519Key,
         },
+
+        // include the recipient device details in the payload,
+        // to avoid unknown key attacks, per
+        // https://github.com/vector-im/vector-web/issues/2483
+        recipient: recipientUserId,
+        recipient_keys: {
+            "ed25519": recipientDevice.getFingerprint(),
+        },
     };
-    utils.extend(payloadJson, payloadFields);
-
-    var ciphertext = {};
-    var payloadString = JSON.stringify(payloadJson);
-    for (var i = 0; i < participantKeys.length; ++i) {
-        var deviceKey = participantKeys[i];
-        var sessionId = olmDevice.getSessionIdForDevice(deviceKey);
-        if (sessionId === null) {
-            // If we don't have a session for a device then
-            // we can't encrypt a message for it.
-            continue;
-        }
-        console.log("Using sessionid " + sessionId + " for device " + deviceKey);
-        ciphertext[deviceKey] = olmDevice.encryptMessage(
-            deviceKey, sessionId, payloadString
-        );
-    }
-    var encryptedContent = {
-        algorithm: module.exports.OLM_ALGORITHM,
-        sender_key: olmDevice.deviceCurve25519Key,
-        ciphertext: ciphertext
-    };
-    return encryptedContent;
+
+    // TODO: technically, a bunch of that stuff only needs to be included for
+    // pre-key messages: after that, both sides know exactly which devices are
+    // involved in the session. If we're looking to reduce data transfer in the
+    // future, we could elide them for subsequent messages.
+
+    utils.extend(payload, payloadFields);
+
+    resultsObject[deviceKey] = olmDevice.encryptMessage(
+        deviceKey, sessionId, JSON.stringify(payload)
+    );
 };