From 832559926fde9cab33e260886f87afcf4ae84aeb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 20 Sep 2016 20:27:49 +0100 Subject: [PATCH] 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 | 22 +++++++----- lib/crypto/OlmDevice.js | 42 ++++++++++++----------- lib/crypto/algorithms/base.js | 16 +-------- lib/crypto/algorithms/megolm.js | 14 +++++--- lib/crypto/algorithms/olm.js | 4 +-- lib/crypto/index.js | 61 ++++++++++++++++++++++++--------- lib/models/event.js | 39 ++++++++++++--------- 7 files changed, 114 insertions(+), 84 deletions(-) diff --git a/lib/client.js b/lib/client.js index e4cb2cbe0..d9e6f16cc 100644 --- a/lib/client.js +++ b/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 matrixEvent; + return new MatrixEvent(plainOldJsObject); } return mapper; } diff --git a/lib/crypto/OlmDevice.js b/lib/crypto/OlmDevice.js index 9661a00cb..f38016d88 100644 --- a/lib/crypto/OlmDevice.js +++ b/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): 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, keysClaimed: - * Object}} result + * @return {T} result of func * * @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 { - result: func(session), - keysProved: keysProved, - keysClaimed: keysClaimed, - }; + return func(session, r.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 {number} chainIndex index at which sessionKey applies + * @param {string} sessionKey base64-encoded secret key + * @param {Object} 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( diff --git a/lib/crypto/algorithms/base.js b/lib/crypto/algorithms/base.js index 3c076a35b..2e406b459 100644 --- a/lib/crypto/algorithms/base.js +++ b/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} keysClaimed keys that the sender of the - * event claims ownership of: map from key type to base64-encoded key - * - * @property {Object} 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 diff --git a/lib/crypto/algorithms/megolm.js b/lib/crypto/algorithms/megolm.js index 1e1be1e27..cf9c09ab1 100644 --- a/lib/crypto/algorithms/megolm.js +++ b/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) { - res.result = JSON.parse(res.result); + if (res === null) { + return null; } - return res; + return { + payload: JSON.parse(res.result), + keysClaimed: res.keysClaimed, + keysProved: res.keysProved, + }; } catch (e) { throw new base.DecryptionError(e); } diff --git a/lib/crypto/algorithms/olm.js b/lib/crypto/algorithms/olm.js index 672308d24..55443f860 100644 --- a/lib/crypto/algorithms/olm.js +++ b/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 || {} diff --git a/lib/crypto/index.js b/lib/crypto/index.js index 15a500be1..68f240acb 100644 --- a/lib/crypto/index.js +++ b/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 - // purports to have been sent from. It's assumed that, by the time we get here, - // we have already checked that the event was, in fact, sent by that device. - // - // 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. + // sender_key is the Curve25519 identity key of the device which the event + // was sent from. In the case of Megolm, it's actually the Curve25519 + // identity key of the device which set up the Megolm session. - 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} keysClaimed keys that the sender of the + * event claims ownership of: map from key type to base64-encoded key + * + * @property {Object} 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; - payload.keysClaimed = r.keysClaimed; - payload.keysProved = r.keysProved; - return payload; + return r; } 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 diff --git a/lib/models/event.js b/lib/models/event.js index 71eb3dd1f..31da3d72d 100644 --- a/lib/models/event.js +++ b/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 type and content 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. Do not access * this property 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. This property is experimental and may change. */ -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. + *

* 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} + * + * @return {Object} */ getKeysProved: function() { - // The keysProved property usually isn't actually part of the decrypted - // plaintext. Instead it is added after decryption by the crypto - // algorithm in lib/crypto/algorithms. - return this._clearEvent.keysProved || {}; + return this._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. + *

* 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} */ getKeysClaimed: function() { - // The keysClaimed property usually isn't actually part of the - // decrypted plaintext. Instead it is added after decryption by the - // crypto algorithm in lib/crypto/algorithms. - return this._clearEvent.keysClaimed || {}; + return this._keysClaimed; }, getUnsigned: function() {