Fix the ed25519 key checking

Finish plumbing in the Ed25519 key checks. Make sure we store the claimed key
correctly in the megolm sessions, and keep them as a separate field in
MatrixEvent rather than stuffing them into _clearEvent

lib/client.js

@@ -465,32 +465,38 @@ MatrixClient.prototype.isRoomEncrypted = function(roomId) {
  * @param {MatrixClient} client
  * @param {object} raw event
  *
- * @return {object} decrypted payload (with properties 'type', 'content')
+ * @return {MatrixEvent}
  */
-function _decryptMessage(client, event) {
+function _decryptEvent(client, event) {
     if (!client._crypto) {
         return _badEncryptedMessage(event, "**Encryption not enabled**");
     }
 
+    var decryptionResult;
     try {
-        return client._crypto.decryptEvent(event);
+        decryptionResult = client._crypto.decryptEvent(event);
     } catch (e) {
         if (!(e instanceof Crypto.DecryptionError)) {
             throw e;
         }
         return _badEncryptedMessage(event, "**" + e.message + "**");
     }
+    return new MatrixEvent(
+        event, decryptionResult.payload,
+        decryptionResult.keysProved,
+        decryptionResult.keysClaimed
+    );
 }
 
 function _badEncryptedMessage(event, reason) {
-    return {
+    return new MatrixEvent(event, {
         type: "m.room.message",
         content: {
             msgtype: "m.bad.encrypted",
             body: reason,
             content: event.content,
         },
-    };
+    });
 }
 
 // Room ops
@@ -2782,12 +2788,10 @@ function _resolve(callback, defer, res) {
 
 function _PojoToMatrixEventMapper(client) {
     function mapper(plainOldJsObject) {
-        var clearData;
         if (plainOldJsObject.type === "m.room.encrypted") {
-            clearData = _decryptMessage(client, plainOldJsObject);
+            return _decryptEvent(client, plainOldJsObject);
         }
-        var matrixEvent = new MatrixEvent(plainOldJsObject, clearData);
+        return new MatrixEvent(plainOldJsObject);
-        return matrixEvent;
     }
     return mapper;
 }

lib/crypto/OlmDevice.js

@@ -557,12 +557,12 @@ OlmDevice.prototype._saveInboundGroupSession = function(
  * @param {string} roomId
  * @param {string} senderKey
  * @param {string} sessionId
- * @param {function(Olm.InboundGroupSession): T} func
+ * @param {function(Olm.InboundGroupSession, Object<string, string>): T} func
+ *   function to call. Second argument is the map of keys claimed by the session.
  *
  * @return {null} the sessionId is unknown
  *
- * @return {{result: T, keysProved: Object<string, string>, keysClaimed:
+ * @return {T} result of func
- *    Object<string, string>}} result
  *
  * @private
  * @template {T}
@@ -589,19 +589,10 @@ OlmDevice.prototype._getInboundGroupSession = function(
         );
     }
 
-    // the sender must have had the senderKey to persuade us to save the
-    // session.
-    var keysProved = {curve25519: senderKey};
-    var keysClaimed = r.keysClaimed || {};
-
     var session = new Olm.InboundGroupSession();
     try {
         session.unpickle(this._pickleKey, r.session);
-        return {
+        return func(session, r.keysClaimed || {});
-            result: func(session),
-            keysProved: keysProved,
-            keysClaimed: keysClaimed,
-        };
     } finally {
         session.free();
     }
@@ -613,11 +604,11 @@ OlmDevice.prototype._getInboundGroupSession = function(
  * @param {string} roomId     room in which this session will be used
  * @param {string} senderKey  base64-encoded curve25519 key of the sender
  * @param {string} sessionId  session identifier
- * @param {string} sessionKey base64-encoded secret key at index chainIndex
+ * @param {string} sessionKey base64-encoded secret key
- * @param {number} chainIndex index at which sessionKey applies
+ * @param {Object<string, string>} keysClaimed Other keys the sender claims.
  */
 OlmDevice.prototype.addInboundGroupSession = function(
-    roomId, senderKey, sessionId, sessionKey, chainIndex
+    roomId, senderKey, sessionId, sessionKey, keysClaimed
 ) {
     var self = this;
     var session = new Olm.InboundGroupSession();
@@ -628,7 +619,9 @@ OlmDevice.prototype.addInboundGroupSession = function(
                 "Mismatched group session ID from senderKey: " + senderKey
             );
         }
-        self._saveInboundGroupSession(roomId, senderKey, sessionId, session);
+        self._saveInboundGroupSession(
+            roomId, senderKey, sessionId, session, keysClaimed
+        );
     } finally {
         session.free();
     }
@@ -652,12 +645,21 @@ OlmDevice.prototype.decryptGroupMessage = function(
 ) {
     var self = this;
 
-    function decrypt(session) {
+    function decrypt(session, keysClaimed) {
         var res = session.decrypt(body);
+
+        // the sender must have had the senderKey to persuade us to save the
+        // session.
+        var keysProved = {curve25519: senderKey};
+
         self._saveInboundGroupSession(
-            roomId, senderKey, sessionId, session
+            roomId, senderKey, sessionId, session, keysClaimed
         );
-        return res;
+        return {
+            result: res,
+            keysClaimed: keysClaimed,
+            keysProved: keysProved,
+        };
     }
 
     return this._getInboundGroupSession(

lib/crypto/algorithms/base.js

@@ -109,20 +109,6 @@ var DecryptionAlgorithm = function(params) {
 /** */
 module.exports.DecryptionAlgorithm = DecryptionAlgorithm;
 
-/**
- * @typedef {Object} module:crypto/algorithms/base.DecryptionResult
- *
- * @property {Object} result decrypted payload (with properties 'type',
- *     'content').
- *
- * @property {Object<string, string>} keysClaimed keys that the sender of the
- *     event claims ownership of: map from key type to base64-encoded key
- *
- * @property {Object<string, string>} keysProved keys that the sender of the
- *     event is known to have ownership of: map from key type to base64-encoded
- *     key
- */
-
 /**
  * Decrypt an event
  *
@@ -132,7 +118,7 @@ module.exports.DecryptionAlgorithm = DecryptionAlgorithm;
  * @param {object} event raw event
  *
  * @return {null} if the event referred to an unknown megolm session
- * @return {module:crypto/algorithms/base.DecryptionResult} decryption result
+ * @return {module:crypto.DecryptionResult} decryption result
  *
  * @throws {module:crypto/algorithms/base.DecryptionError} if there is a
  *   problem decrypting the event

lib/crypto/algorithms/megolm.js

@@ -124,7 +124,7 @@ MegolmEncryption.prototype._prepareNewSession = function(room) {
 
     this._olmDevice.addInboundGroupSession(
         this._roomId, this._olmDevice.deviceCurve25519Key, session_id,
-        key.key
+        key.key, {ed25519: this._olmDevice.deviceEd25519Key}
     );
 
     // we're going to share the key with all current members of the room,
@@ -399,7 +399,7 @@ utils.inherits(MegolmDecryption, base.DecryptionAlgorithm);
  * @param {object} event raw event
  *
  * @return {null} The event referred to an unknown megolm session
- * @return {module:crypto/algorithms/base.DecryptionResult} decryption result
+ * @return {module:crypto.DecryptionResult} decryption result
  *
  * @throws {module:crypto/algorithms/base.DecryptionError} if there is a
  *   problem decrypting the event
@@ -417,10 +417,14 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
         var res = this._olmDevice.decryptGroupMessage(
             event.room_id, content.sender_key, content.session_id, content.ciphertext
         );
-        if (res !== null) {
+        if (res === null) {
-            res.result = JSON.parse(res.result);
+            return null;
         }
-        return res;
+        return {
+            payload: JSON.parse(res.result),
+            keysClaimed: res.keysClaimed,
+            keysProved: res.keysProved,
+        };
     } catch (e) {
         throw new base.DecryptionError(e);
     }

lib/crypto/algorithms/olm.js

@@ -142,7 +142,7 @@ utils.inherits(OlmDecryption, base.DecryptionAlgorithm);
  *
  * @param {object} event raw event
  *
- * @return {module:crypto/algorithms/base.DecryptionResult} decryption result
+ * @return {module:crypto.DecryptionResult} decryption result
  *
  * @throws {module:crypto/algorithms/base.DecryptionError} if there is a
  *   problem decrypting the event
@@ -178,7 +178,7 @@ OlmDecryption.prototype.decryptEvent = function(event) {
     // TODO: check the room_id and fingerprint
     var payload = JSON.parse(payloadString);
     return {
-        result: payload,
+        payload: payload,
         sessionExists: true,
         keysProved: {curve25519: deviceKey},
         keysClaimed: payload.keys || {}

lib/crypto/index.js

@@ -625,21 +625,37 @@ Crypto.prototype.getEventSenderDeviceInfo = function(event) {
         return null;
     }
 
-    // sender_key is the curve25519 public key of the device, that the event
+    // sender_key is the Curve25519 identity key of the device which the event
-    // purports to have been sent from. It's assumed that, by the time we get here,
+    // was sent from. In the case of Megolm, it's actually the Curve25519
-    // we have already checked that the event was, in fact, sent by that device.
+    // identity key of the device which set up the Megolm session.
-    //
-    // In the case of both olm and megolm, that is achieved primarily by the
-    // fact that sessions are indexed by the curve25519 key of the device that
-    // created the session, and we assume that only that device has the keys
-    // necessary to create valid messages in that session.
-    //
-    // So, all we need to do here is look up the device by sender and
-    // curve25519 key and determine the state of the verification flag.
 
-    return this.getDeviceByIdentityKey(
+    var device = this.getDeviceByIdentityKey(
         event.getSender(), algorithm, sender_key
     );
+
+    // so far so good, but now we need to check that the sender of this event
+    // hadn't advertised someone else's Curve25519 key as their own. We do that
+    // by checking the Ed25519 claimed by the event (or, in the case of megolm,
+    // the event which set up the megolm session), to check that it matches the
+    // fingerprint of the purported sending device.
+    //
+    // (see https://github.com/vector-im/vector-web/issues/2215)
+
+    var claimedKey = event.getKeysClaimed().ed25519;
+    if (!claimedKey) {
+        console.warn("Event " + event.getId() + " claims no ed25519 key: " +
+                     "cannot verify sending device");
+        return null;
+    }
+
+    if (claimedKey !== device.getFingerprint()) {
+        console.warn(
+            "Event " + event.getId() + " claims ed25519 key " + claimedKey +
+                "but sender device has key " + device.getFingerprint());
+        return null;
+    }
+
+    return device;
 };
 
 
@@ -850,12 +866,26 @@ Crypto.prototype.encryptEventIfNeeded = function(event, room) {
     });
 };
 
+/**
+ * @typedef {Object} module:crypto.DecryptionResult
+ *
+ * @property {Object} payload decrypted payload (with properties 'type',
+ *     'content').
+ *
+ * @property {Object<string, string>} keysClaimed keys that the sender of the
+ *     event claims ownership of: map from key type to base64-encoded key
+ *
+ * @property {Object<string, string>} keysProved keys that the sender of the
+ *     event is known to have ownership of: map from key type to base64-encoded
+ *     key
+ */
+
 /**
  * Decrypt a received event
  *
  * @param {object} event raw event
  *
- * @return {object} decrypted payload (with properties 'type', 'content')
+ * @return {module:crypto.DecryptionResult} decryption result
  *
  * @raises {algorithms.DecryptionError} if there is a problem decrypting the event
  */
@@ -871,10 +901,7 @@ Crypto.prototype.decryptEvent = function(event) {
     var r = alg.decryptEvent(event);
 
     if (r !== null) {
-        var payload = r.result;
+        return r;
-        payload.keysClaimed = r.keysClaimed;
-        payload.keysProved = r.keysProved;
-        return payload;
     } else {
         // We've got a message for a session we don't have.  Maybe the sender
         // forgot to tell us about the session.  Remind the sender that we

lib/models/event.js

@@ -54,6 +54,12 @@ module.exports.EventStatus = {
  * @param {Object=} clearEvent For encrypted events, the plaintext payload for
  * the event (typically containing <tt>type</tt> and <tt>content</tt> fields).
  *
+ * @param {Object=} keysProved Keys owned by the sender of this event.
+ *    See {@link module:models/event.MatrixEvent#getKeysProved}.
+ *
+ * @param {Object=} keysClaimed Keys the sender of this event claims.
+ *    See {@link module:models/event.MatrixEvent#getKeysClaimed}.
+ *
  * @prop {Object} event The raw (possibly encrypted) event. <b>Do not access
  * this property</b> directly unless you absolutely have to. Prefer the getter
  * methods defined on this class. Using the getter methods shields your app
@@ -68,7 +74,9 @@ module.exports.EventStatus = {
  * that getDirectionalContent() will return event.content and not event.prev_content.
  * Default: true. <strong>This property is experimental and may change.</strong>
  */
-module.exports.MatrixEvent = function MatrixEvent(event, clearEvent) {
+module.exports.MatrixEvent = function MatrixEvent(
+    event, clearEvent, keysProved, keysClaimed
+) {
     this.event = event || {};
     this.sender = null;
     this.target = null;
@@ -77,6 +85,9 @@ module.exports.MatrixEvent = function MatrixEvent(event, clearEvent) {
 
     this._clearEvent = clearEvent || {};
     this._pushActions = null;
+
+    this._keysProved = keysProved || {};
+    this._keysClaimed = keysClaimed || {};
 };
 
 module.exports.MatrixEvent.prototype = {
@@ -221,11 +232,11 @@ module.exports.MatrixEvent.prototype = {
         this._clearEvent = {
             type: this.event.type,
             content: this.event.content,
-            keysProved: keys,
-            keysClaimed: keys,
         };
         this.event.type = crypto_type;
         this.event.content = crypto_content;
+        this._keysProved = keys;
+        this._keysClaimed = keys;
     },
 
     /**
@@ -246,32 +257,28 @@ module.exports.MatrixEvent.prototype = {
 
     /**
      * The keys that must have been owned by the sender of this encrypted event.
+     * <p>
      * These don't necessarily have to come from this event itself, but may be
      * implied by the cryptographic session.
-     * For example megolm messages don't prove keys directly, but instead
+     *
-     * inherit a proof from the olm message that established the session.
+     * @return {Object<string, string>}
-     * @return {object}
      */
     getKeysProved: function() {
-        // The keysProved property usually isn't actually part of the decrypted
+        return this._keysProved;
-        // plaintext. Instead it is added after decryption by the crypto
-        // algorithm in lib/crypto/algorithms.
-        return this._clearEvent.keysProved || {};
     },
 
     /**
-     * The additional keys the sender of this encrypted event claims to possess
+     * The additional keys the sender of this encrypted event claims to possess.
+     * <p>
      * These don't necessarily have to come from this event itself, but may be
      * implied by the cryptographic session.
      * For example megolm messages don't claim keys directly, but instead
      * inherit a claim from the olm message that established the session.
-     * @return {object}
+     *
+     * @return {Object<string, string>}
      */
     getKeysClaimed: function() {
-        // The keysClaimed property usually isn't actually part of the
+        return this._keysClaimed;
-        // decrypted plaintext. Instead it is added after decryption by the
-        // crypto algorithm in lib/crypto/algorithms.
-        return this._clearEvent.keysClaimed || {};
     },
 
     getUnsigned: function() {