From 72a4b92022a1cebbdbd4e912e0fb799c77551fae Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Wed, 14 Sep 2016 19:16:24 +0100 Subject: [PATCH 1/7] Send a 'm.new_device' when we get a message for an unknown group session This should reduce the risk of a device getting permenantly stuck unable to receive encrypted group messages. --- lib/crypto/OlmDevice.js | 10 +++++---- lib/crypto/algorithms/megolm.js | 9 +++++++-- lib/crypto/algorithms/olm.js | 6 ++++-- lib/crypto/index.js | 36 ++++++++++++++++++++++++++++++++- 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/lib/crypto/OlmDevice.js b/lib/crypto/OlmDevice.js index ca9565fe3f7..5d2fe6620e7 100644 --- a/lib/crypto/OlmDevice.js +++ b/lib/crypto/OlmDevice.js @@ -539,7 +539,9 @@ OlmDevice.prototype._saveInboundGroupSession = function( * @param {string} senderKey * @param {string} sessionId * @param {function} func - * @return {object} result of func + * @return {object} Object with two keys "result": result of func, "exists" + * whether the session exists. if the session doesn't exist then the function + * isn't called and the "result" is undefined. * @private */ OlmDevice.prototype._getInboundGroupSession = function( @@ -550,7 +552,7 @@ OlmDevice.prototype._getInboundGroupSession = function( ); if (r === null) { - throw new Error("Unknown inbound group session id"); + return {sessionExists: false} } r = JSON.parse(r); @@ -567,7 +569,7 @@ OlmDevice.prototype._getInboundGroupSession = function( var session = new Olm.InboundGroupSession(); try { session.unpickle(this._pickleKey, r.session); - return func(session); + return {sessionExists: true, result: func(session)}; } finally { session.free(); } @@ -603,7 +605,7 @@ OlmDevice.prototype.addInboundGroupSession = function( * @param {string} sessionId session identifier * @param {string} body base64-encoded body of the encrypted message * - * @return {string} plaintext + * @return {object} {result: "plaintext"|undefined, sessionExists: Boolean} */ OlmDevice.prototype.decryptGroupMessage = function( roomId, senderKey, sessionId, body diff --git a/lib/crypto/algorithms/megolm.js b/lib/crypto/algorithms/megolm.js index b107945246c..56d7bf2263d 100644 --- a/lib/crypto/algorithms/megolm.js +++ b/lib/crypto/algorithms/megolm.js @@ -356,7 +356,8 @@ utils.inherits(MegolmDecryption, base.DecryptionAlgorithm); * * @param {object} event raw event * - * @return {object} decrypted payload (with properties 'type', 'content') + * @return {object} object with 'result' key with decrypted payload (with + * properties 'type', 'content') and a 'sessionKey' key. * * @throws {module:crypto/algorithms/base.DecryptionError} if there is a * problem decrypting the event @@ -377,7 +378,11 @@ MegolmDecryption.prototype.decryptEvent = function(event) { var res = this._olmDevice.decryptGroupMessage( event.room_id, content.sender_key, content.session_id, content.ciphertext ); - return JSON.parse(res); + if (res.sessionExists) { + return {result: JSON.parse(res.result), sessionExists: true}; + } else { + return {sessionExists: false}; + } } catch (e) { throw new base.DecryptionError(e); } diff --git a/lib/crypto/algorithms/olm.js b/lib/crypto/algorithms/olm.js index d5fd4fd4ecb..e75877cdf8b 100644 --- a/lib/crypto/algorithms/olm.js +++ b/lib/crypto/algorithms/olm.js @@ -142,7 +142,9 @@ utils.inherits(OlmDecryption, base.DecryptionAlgorithm); * * @param {object} event raw event * - * @return {object} decrypted payload (with properties 'type', 'content') + * @return {object} result object with result property with the decrypted + * payload (with properties 'type', 'content'), and a "sessionExists" key + * always set to true. * * @throws {module:crypto/algorithms/base.DecryptionError} if there is a * problem decrypting the event @@ -198,7 +200,7 @@ OlmDecryption.prototype.decryptEvent = function(event) { // TODO: Check the sender user id matches the sender key. // TODO: check the room_id and fingerprint if (payloadString !== null) { - return JSON.parse(payloadString); + return {result: JSON.parse(payloadString), sessionExists: true}; } else { throw new base.DecryptionError("Bad Encrypted Message"); } diff --git a/lib/crypto/index.js b/lib/crypto/index.js index f5184c40bab..00284064d0f 100644 --- a/lib/crypto/index.js +++ b/lib/crypto/index.js @@ -820,7 +820,41 @@ Crypto.prototype.decryptEvent = function(event) { var alg = new AlgClass({ olmDevice: this._olmDevice, }); - return alg.decryptEvent(event); + var r = alg.decryptEvent(event); + if (r.sessionExists) { + return r.result; + } 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 exists so that they might + // tell us about the sender. + if (event.getRoomId !== undefined && event.getSender !== undefined) { + var senderUserId = event.getSender(); + var roomId = event.getRoomId(); + var content = {}; + var senderDeviceId = event.content.device_id; + if (senderDeviceId !== undefined) { + content[senderUserId][senderDeviceId] = { + device_id: this._deviceId, + rooms: [roomId], + }; + } else { + content[senderUserId]["*"] = { + device_id: this._deviceId, + rooms: [roomId], + }; + } + // TODO: Ratelimit the "m.new_device" messages to make sure we don't + // flood the target device with messages if we get lots of encrypted + // messages from them at once. + this._baseApis.sendToDevice( + "m.new_device", // OH HAI! + content + ).done(function() {}); + } + + throw new algorithms.DecryptionError("Unknown inbound session id"); + } }; /** From 5ec8688cf6e8ef831dc6bfec154b50d9331c0420 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Wed, 14 Sep 2016 19:26:44 +0100 Subject: [PATCH 2/7] Semicolon --- lib/crypto/OlmDevice.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/crypto/OlmDevice.js b/lib/crypto/OlmDevice.js index 5d2fe6620e7..01a7105807a 100644 --- a/lib/crypto/OlmDevice.js +++ b/lib/crypto/OlmDevice.js @@ -552,7 +552,7 @@ OlmDevice.prototype._getInboundGroupSession = function( ); if (r === null) { - return {sessionExists: false} + return {sessionExists: false}; } r = JSON.parse(r); From d02c205910d933698eeb92ced7961002ce430b0d Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 15 Sep 2016 11:46:49 +0100 Subject: [PATCH 3/7] Rename the "content" variable to avoid shadowing --- lib/crypto/index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/crypto/index.js b/lib/crypto/index.js index 00284064d0f..533176157a8 100644 --- a/lib/crypto/index.js +++ b/lib/crypto/index.js @@ -831,15 +831,15 @@ Crypto.prototype.decryptEvent = function(event) { if (event.getRoomId !== undefined && event.getSender !== undefined) { var senderUserId = event.getSender(); var roomId = event.getRoomId(); - var content = {}; + var pingContent = {}; var senderDeviceId = event.content.device_id; if (senderDeviceId !== undefined) { - content[senderUserId][senderDeviceId] = { + pingContent[senderUserId][senderDeviceId] = { device_id: this._deviceId, rooms: [roomId], }; } else { - content[senderUserId]["*"] = { + pingContent[senderUserId]["*"] = { device_id: this._deviceId, rooms: [roomId], }; @@ -849,7 +849,7 @@ Crypto.prototype.decryptEvent = function(event) { // messages from them at once. this._baseApis.sendToDevice( "m.new_device", // OH HAI! - content + pingContent ).done(function() {}); } From 6f9bb38232bda585a97003b3c864fc138eb19ea0 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 15 Sep 2016 11:56:56 +0100 Subject: [PATCH 4/7] Include our device key in megolm messages --- lib/crypto/algorithms/megolm.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/crypto/algorithms/megolm.js b/lib/crypto/algorithms/megolm.js index 56d7bf2263d..78bafdffddd 100644 --- a/lib/crypto/algorithms/megolm.js +++ b/lib/crypto/algorithms/megolm.js @@ -272,6 +272,9 @@ MegolmEncryption.prototype.encryptMessage = function(room, eventType, content) { sender_key: self._olmDevice.deviceCurve25519Key, ciphertext: ciphertext, session_id: session_id, + // Include our device ID so that recipients can send us a + // m.new_device message if they don't have our session key. + device_id: self._deviceId, }; return encryptedContent; From 35d99564c195a6a777688b9fa3cbfb505a34413e Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 15 Sep 2016 14:07:40 +0100 Subject: [PATCH 5/7] Rate limit the oh hai pings --- lib/crypto/index.js | 68 ++++++++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/lib/crypto/index.js b/lib/crypto/index.js index 533176157a8..86c724eb825 100644 --- a/lib/crypto/index.js +++ b/lib/crypto/index.js @@ -85,6 +85,8 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId) { ); _registerEventHandlers(this, eventEmitter); + + this._lastNewDeviceMessageTsByUserDeviceRoom = {}; } function _registerEventHandlers(crypto, eventEmitter) { @@ -829,34 +831,56 @@ Crypto.prototype.decryptEvent = function(event) { // Remind the sender that we exists so that they might // tell us about the sender. if (event.getRoomId !== undefined && event.getSender !== undefined) { - var senderUserId = event.getSender(); - var roomId = event.getRoomId(); - var pingContent = {}; - var senderDeviceId = event.content.device_id; - if (senderDeviceId !== undefined) { - pingContent[senderUserId][senderDeviceId] = { - device_id: this._deviceId, - rooms: [roomId], - }; - } else { - pingContent[senderUserId]["*"] = { - device_id: this._deviceId, - rooms: [roomId], - }; - } - // TODO: Ratelimit the "m.new_device" messages to make sure we don't - // flood the target device with messages if we get lots of encrypted - // messages from them at once. - this._baseApis.sendToDevice( - "m.new_device", // OH HAI! - pingContent - ).done(function() {}); + this._sendPingToDevice( + event.getSender(), event.content.device, event.getRoomId + ); } throw new algorithms.DecryptionError("Unknown inbound session id"); } }; +/** + * Send a "m.new_device" message to remind it that we exist and are a member + * of a room. + * + * This is rate limited to send a message at most once an hour per desination. + * + * @param {string} userId The ID of the user to ping. + * @param {string} deviceId The ID of the device to ping. + * @param {string} roomId The ID of the room we want to remind them about. + */ +Crypto.prototype._sendPingToDevice = function(userId, deviceId, roomId) { + if (deviceId === undefined) { + deviceId = "*"; + } + + var lastMessageTsMap = this._lastNewDeviceMessageTsByUserDeviceRoom; + var lastTsByDevice = lastMessageTsMap[userId] || {}; + var lastTsByRoom = lastTsByDevice[deviceId] || {}; + var lastTs = lastTsByRoom[roomId]; + var timeNowMs = Date.now(); + var oneHourMs = 1000 * 60 * 60; + + if (lastTs === undefined || lastTs + oneHourMs < timeNowMs) { + var content = { + userId: { + deviceId: { + device_id: this._deviceId, + rooms: [roomId], + } + } + }; + + lastTsByRoom[roomId] = timeNowMs; + + this._baseApis.sendToDevice( + "m.new_device", // OH HAI! + content + ).done(function() {}); + }; +}; + /** * handle an m.room.encryption event * From 355b728a5782b55e6d2f9ad6fc3d0a9c45570398 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 15 Sep 2016 14:23:30 +0100 Subject: [PATCH 6/7] Remove unnecessary semicolon; --- lib/crypto/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/crypto/index.js b/lib/crypto/index.js index 86c724eb825..70189d7a66f 100644 --- a/lib/crypto/index.js +++ b/lib/crypto/index.js @@ -878,7 +878,7 @@ Crypto.prototype._sendPingToDevice = function(userId, deviceId, roomId) { "m.new_device", // OH HAI! content ).done(function() {}); - }; + } }; /** From 2fbef8638fb2160e75162a77a39aebed6cbaf1fa Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 15 Sep 2016 14:43:23 +0100 Subject: [PATCH 7/7] Fix grammar --- lib/crypto/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/crypto/index.js b/lib/crypto/index.js index 70189d7a66f..b0a519ff68b 100644 --- a/lib/crypto/index.js +++ b/lib/crypto/index.js @@ -828,7 +828,7 @@ Crypto.prototype.decryptEvent = function(event) { } 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 exists so that they might + // Remind the sender that we exist so that they might // tell us about the sender. if (event.getRoomId !== undefined && event.getSender !== undefined) { this._sendPingToDevice(