From 8f252992e44be2a5f9060030a89c779dd310f11b Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 11 Oct 2017 22:49:44 -0400 Subject: [PATCH 1/2] keep track of event ID and timestamp of decrypted messages This is to avoid false positives when detecting replay attacks. fixes: vector-im/riot-web#3712 Signed-off-by: Hubert Chathi --- spec/unit/crypto/algorithms/megolm.spec.js | 83 ++++++++++++++++++++++ src/crypto/OlmDevice.js | 32 +++++++-- src/crypto/algorithms/megolm.js | 1 + 3 files changed, 109 insertions(+), 7 deletions(-) diff --git a/spec/unit/crypto/algorithms/megolm.spec.js b/spec/unit/crypto/algorithms/megolm.spec.js index 22c3bc4f6ec..b81e866898e 100644 --- a/spec/unit/crypto/algorithms/megolm.spec.js +++ b/spec/unit/crypto/algorithms/megolm.spec.js @@ -181,5 +181,88 @@ describe("MegolmDecryption", function() { expect(payload.content.session_key).toExist(); }); }); + + it("can detect replay attacks", function() { + // trying to decrypt two different messages (marked by different + // event IDs or timestamps) using the same (sender key, session id, + // message index) triple should result in an exception being thrown + // as it should be detected as a replay attack. + const sessionId = groupSession.session_id(); + const cipherText = groupSession.encrypt(JSON.stringify({ + room_id: ROOM_ID, + content: 'testytest', + })); + const event1 = new MatrixEvent({ + type: 'm.room.encrypted', + room_id: ROOM_ID, + content: { + algorithm: 'm.megolm.v1.aes-sha2', + sender_key: "SENDER_CURVE25519", + session_id: sessionId, + ciphertext: cipherText, + }, + event_id: "$event1", + origin_server_ts: 1507753886000, + }); + + const successHandler = expect.createSpy(); + const failureHandler = expect.createSpy() + .andCall((err) => { + expect(err.toString()).toMatch( + /Duplicate message index, possible replay attack/, + ); + }); + + return megolmDecryption.decryptEvent(event1).then((res) => { + const event2 = new MatrixEvent({ + type: 'm.room.encrypted', + room_id: ROOM_ID, + content: { + algorithm: 'm.megolm.v1.aes-sha2', + sender_key: "SENDER_CURVE25519", + session_id: sessionId, + ciphertext: cipherText, + }, + event_id: "$event2", + origin_server_ts: 1507754149000, + }); + + return megolmDecryption.decryptEvent(event2); + }).then( + successHandler, + failureHandler, + ).then(() => { + expect(successHandler).toNotHaveBeenCalled(); + expect(failureHandler).toHaveBeenCalled(); + }); + }); + + it("allows re-decryption of the same event", function() { + // in contrast with the previous test, if the event ID and + // timestamp are the same, then it should not be considered a + // replay attack + const sessionId = groupSession.session_id(); + const cipherText = groupSession.encrypt(JSON.stringify({ + room_id: ROOM_ID, + content: 'testytest', + })); + const event = new MatrixEvent({ + type: 'm.room.encrypted', + room_id: ROOM_ID, + content: { + algorithm: 'm.megolm.v1.aes-sha2', + sender_key: "SENDER_CURVE25519", + session_id: sessionId, + ciphertext: cipherText, + }, + event_id: "$event1", + origin_server_ts: 1507753886000, + }); + + return megolmDecryption.decryptEvent(event).then((res) => { + return megolmDecryption.decryptEvent(event); + // test is successful if no exception is thrown + }); + }); }); }); diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 3c3336e8522..4c8c8d312b8 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -96,12 +96,18 @@ function OlmDevice(sessionStore) { // This partially mitigates a replay attack where a MITM resends a group // message into the room. // - // TODO: If we ever remove an event from memory we will also need to remove - // it from this map. Otherwise if we download the event from the server we - // will think that it is a duplicate. + // When we decrypt a message and the message index matches a previously + // decrypted message, one possible cause of that is that we are decrypting + // the same event, and may not indicate an actual replay attack. For + // example, this could happen if we receive events, forget about them, and + // then re-fetch them when we backfill. So we store the event ID and + // timestamp corresponding to each message index when we first decrypt it, + // and compare these against the event ID and timestamp every time we use + // that same index. If they match, then we're probably decrypting the same + // event and we don't consider it a replay attack. // // Keys are strings of form "||" - // Values are true. + // Values are objects containing the event ID and timestamp. this._inboundGroupSessionMessageIndexes = {}; } @@ -794,6 +800,8 @@ OlmDevice.prototype.importInboundGroupSession = async function(data) { * @param {string} senderKey base64-encoded curve25519 key of the sender * @param {string} sessionId session identifier * @param {string} body base64-encoded body of the encrypted message + * @param {string} eventId ID of the event being decrypted + * @param {Number} timestamp timestamp of the event being decrypted * * @return {null} the sessionId is unknown * @@ -802,9 +810,10 @@ OlmDevice.prototype.importInboundGroupSession = async function(data) { * keysClaimed: Object}>} */ OlmDevice.prototype.decryptGroupMessage = async function( - roomId, senderKey, sessionId, body, + roomId, senderKey, sessionId, body, eventId, timestamp, ) { const self = this; + const argumentsLength = arguments.length; function decrypt(session, sessionData) { const res = session.decrypt(body); @@ -815,14 +824,23 @@ OlmDevice.prototype.decryptGroupMessage = async function( plaintext = res; } else { // Check if we have seen this message index before to detect replay attacks. + // If the event ID and timestamp are specified, and the match the event ID + // and timestamp from the last time we used this message index, then we + // don't consider it a replay attack. const messageIndexKey = senderKey + "|" + sessionId + "|" + res.message_index; - if (messageIndexKey in self._inboundGroupSessionMessageIndexes) { + if (messageIndexKey in self._inboundGroupSessionMessageIndexes + && (argumentsLength <= 4 // Compatibility for older old versions. + || self._inboundGroupSessionMessageIndexes[messageIndexKey].id !== eventId + || self._inboundGroupSessionMessageIndexes[messageIndexKey].timestamp !== timestamp)) { throw new Error( "Duplicate message index, possible replay attack: " + messageIndexKey, ); } - self._inboundGroupSessionMessageIndexes[messageIndexKey] = true; + self._inboundGroupSessionMessageIndexes[messageIndexKey] = { + id: eventId, + timestamp: timestamp, + }; } sessionData.session = session.pickle(self._pickleKey); diff --git a/src/crypto/algorithms/megolm.js b/src/crypto/algorithms/megolm.js index 7d2fd7e1752..55665f9ee54 100644 --- a/src/crypto/algorithms/megolm.js +++ b/src/crypto/algorithms/megolm.js @@ -628,6 +628,7 @@ MegolmDecryption.prototype.decryptEvent = async function(event) { try { res = await this._olmDevice.decryptGroupMessage( event.getRoomId(), content.sender_key, content.session_id, content.ciphertext, + event.getId(), event.getTs(), ); } catch (e) { if (e.message === 'OLM.UNKNOWN_MESSAGE_INDEX') { From dcab4eb70b8973c25ec45a1f5230a61fce6ccb79 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 12 Oct 2017 09:04:12 -0400 Subject: [PATCH 2/2] fix lint error, and incorporate suggestions from richvdh and krombel Signed-off-by: Hubert Chathi --- src/crypto/OlmDevice.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/crypto/OlmDevice.js b/src/crypto/OlmDevice.js index 4c8c8d312b8..3b4d1f2ae27 100644 --- a/src/crypto/OlmDevice.js +++ b/src/crypto/OlmDevice.js @@ -107,7 +107,7 @@ function OlmDevice(sessionStore) { // event and we don't consider it a replay attack. // // Keys are strings of form "||" - // Values are objects containing the event ID and timestamp. + // Values are objects of the form "{id: , timestamp: }" this._inboundGroupSessionMessageIndexes = {}; } @@ -813,7 +813,6 @@ OlmDevice.prototype.decryptGroupMessage = async function( roomId, senderKey, sessionId, body, eventId, timestamp, ) { const self = this; - const argumentsLength = arguments.length; function decrypt(session, sessionData) { const res = session.decrypt(body); @@ -828,14 +827,14 @@ OlmDevice.prototype.decryptGroupMessage = async function( // and timestamp from the last time we used this message index, then we // don't consider it a replay attack. const messageIndexKey = senderKey + "|" + sessionId + "|" + res.message_index; - if (messageIndexKey in self._inboundGroupSessionMessageIndexes - && (argumentsLength <= 4 // Compatibility for older old versions. - || self._inboundGroupSessionMessageIndexes[messageIndexKey].id !== eventId - || self._inboundGroupSessionMessageIndexes[messageIndexKey].timestamp !== timestamp)) { - throw new Error( - "Duplicate message index, possible replay attack: " + - messageIndexKey, - ); + if (messageIndexKey in self._inboundGroupSessionMessageIndexes) { + const msgInfo = self._inboundGroupSessionMessageIndexes[messageIndexKey]; + if (msgInfo.id !== eventId || msgInfo.timestamp !== timestamp) { + throw new Error( + "Duplicate message index, possible replay attack: " + + messageIndexKey, + ); + } } self._inboundGroupSessionMessageIndexes[messageIndexKey] = { id: eventId,