From 2aa3674c4bb70572c2a83cd891fc868e93c8e96b Mon Sep 17 00:00:00 2001 From: "Alexander \"PapaTutuWawa" Date: Sun, 2 Oct 2022 14:56:20 +0200 Subject: [PATCH] fix: Fix receiving an old key exchange breaking decryption This was mostly caused by Dart not copying values but referencing them. AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA. We know make some assumptions about received key exchanges, so this needs some field testing. --- lib/src/double_ratchet/double_ratchet.dart | 43 ++++++-- lib/src/errors.dart | 10 ++ lib/src/omemo/sessionmanager.dart | 46 ++++++-- test/double_ratchet_test.dart | 1 + test/omemo_test.dart | 121 +++++++-------------- test/serialisation_test.dart | 1 + 6 files changed, 125 insertions(+), 97 deletions(-) diff --git a/lib/src/double_ratchet/double_ratchet.dart b/lib/src/double_ratchet/double_ratchet.dart index ca20266..fc9202d 100644 --- a/lib/src/double_ratchet/double_ratchet.dart +++ b/lib/src/double_ratchet/double_ratchet.dart @@ -69,7 +69,7 @@ class OmemoDoubleRatchet { this.mkSkipped, // MKSKIPPED this.acknowledged, ); - + factory OmemoDoubleRatchet.fromJson(Map data) { /* { @@ -167,7 +167,7 @@ class OmemoDoubleRatchet { /// Create an OMEMO session using the Signed Pre Key [spk], the shared secret [sk] that /// was obtained using a X3DH and the associated data [ad] that was also obtained through /// a X3DH. [ik] refers to Bob's (the receiver's) IK public key. - static Future initiateNewSession(OmemoPublicKey spk, OmemoPublicKey ik, List sk, List ad) async { + static Future initiateNewSession(OmemoPublicKey spk, OmemoPublicKey ik, List sk, List ad, int pn) async { final dhs = await OmemoKeyPair.generateNewPair(KeyPairType.x25519); final dhr = spk; final rk = await kdfRk(sk, await omemoDH(dhs, dhr, 0)); @@ -181,7 +181,7 @@ class OmemoDoubleRatchet { null, 0, 0, - 0, + pn, ik, ad, {}, @@ -330,18 +330,45 @@ class OmemoDoubleRatchet { return decrypt(mk, ciphertext, concat([sessionAd, header.writeToBuffer()]), sessionAd); } + OmemoDoubleRatchet clone() { + return OmemoDoubleRatchet( + dhs, + dhr, + rk, + cks != null ? + List.from(cks!) : + null, + ckr != null ? + List.from(ckr!) : + null, + ns, + nr, + pn, + ik, + sessionAd, + Map>.from(mkSkipped), + acknowledged, + ); + } + @visibleForTesting Future equals(OmemoDoubleRatchet other) async { - // ignore: invalid_use_of_visible_for_testing_member - final dhrMatch = dhr == null ? other.dhr == null : await dhr!.equals(other.dhr!); - final ckrMatch = ckr == null ? other.ckr == null : listsEqual(ckr!, other.ckr!); - final cksMatch = cks == null ? other.cks == null : listsEqual(cks!, other.cks!); + final dhrMatch = dhr == null ? + other.dhr == null : + // ignore: invalid_use_of_visible_for_testing_member + other.dhr != null && await dhr!.equals(other.dhr!); + final ckrMatch = ckr == null ? + other.ckr == null : + other.ckr != null && listsEqual(ckr!, other.ckr!); + final cksMatch = cks == null ? + other.cks == null : + other.cks != null && listsEqual(cks!, other.cks!); // ignore: invalid_use_of_visible_for_testing_member final dhsMatch = await dhs.equals(other.dhs); // ignore: invalid_use_of_visible_for_testing_member final ikMatch = await ik.equals(other.ik); - + return dhsMatch && ikMatch && dhrMatch && diff --git a/lib/src/errors.dart b/lib/src/errors.dart index 67cf159..3a2cf70 100644 --- a/lib/src/errors.dart +++ b/lib/src/errors.dart @@ -30,3 +30,13 @@ class NoDecryptionKeyException implements Exception { class UnknownSignedPrekeyException implements Exception { String errMsg() => 'Unknown Signed Prekey used.'; } + +/// Triggered by the Session Manager when the received Key Exchange message does not +/// meet our expectations. This happens when the PN attribute of the message is not equal +/// to our receive number. +class InvalidKeyExchangeException implements Exception { + const InvalidKeyExchangeException(this.expectedPn, this.actualPn); + final int expectedPn; + final int actualPn; + String errMsg() => 'The pn attribute of the key exchange is invalid. Expected $expectedPn, got $actualPn'; +} diff --git a/lib/src/omemo/sessionmanager.dart b/lib/src/omemo/sessionmanager.dart index 1298ee5..3fbd160 100644 --- a/lib/src/omemo/sessionmanager.dart +++ b/lib/src/omemo/sessionmanager.dart @@ -143,7 +143,7 @@ class OmemoSessionManager { /// Create a ratchet session initiated by Alice to the user with Jid [jid] and the device /// [deviceId] from the bundle [bundle]. @visibleForTesting - Future addSessionFromBundle(String jid, int deviceId, OmemoBundle bundle) async { + Future addSessionFromBundle(String jid, int deviceId, OmemoBundle bundle, int pn) async { final device = await getDevice(); final kexResult = await x3dhFromBundle( bundle, @@ -154,6 +154,7 @@ class OmemoSessionManager { bundle.ik, kexResult.sk, kexResult.ad, + pn, ); await _trustManager.onNewSession(jid, deviceId); @@ -240,7 +241,22 @@ class OmemoSessionManager { final kex = {}; if (newSessions != null) { for (final newSession in newSessions) { - kex[newSession.id] = await addSessionFromBundle(newSession.jid, newSession.id, newSession); + final session = await _getRatchet( + RatchetMapKey( + newSession.jid, + newSession.id, + ), + ); + + final pn = session != null ? + session.ns : + 0; + kex[newSession.id] = await addSessionFromBundle( + newSession.jid, + newSession.id, + newSession, + pn, + ); } } @@ -300,7 +316,7 @@ class OmemoSessionManager { /// [mapKey] with [oldRatchet]. Future _restoreRatchet(RatchetMapKey mapKey, OmemoDoubleRatchet oldRatchet) async { await _lock.synchronized(() { - _log.finest('Restoring ratchet ${mapKey.jid}:${mapKey.deviceId}'); + _log.finest('Restoring ratchet ${mapKey.jid}:${mapKey.deviceId} to ${oldRatchet.nr}'); _ratchetMap[mapKey] = oldRatchet; // Commit the ratchet @@ -335,22 +351,32 @@ class OmemoSessionManager { final decodedRawKey = base64.decode(rawKey.value); OmemoAuthenticatedMessage authMessage; OmemoDoubleRatchet? oldRatchet; + OmemoMessage? message; if (rawKey.kex) { // If the ratchet already existed, we store it. If it didn't, oldRatchet will stay // null. - oldRatchet = await _getRatchet(ratchetKey); - - // TODO(PapaTutuWawa): Only do this when we should + final oldRatchet = (await _getRatchet(ratchetKey))?.clone(); final kex = OmemoKeyExchange.fromBuffer(decodedRawKey); + authMessage = kex.message!; + message = OmemoMessage.fromBuffer(authMessage.message!); + + // Guard against old key exchanges + if (oldRatchet != null) { + _log.finest('KEX for existent ratchet. ${oldRatchet.pn}'); + if (message.pn != oldRatchet.nr) { + throw InvalidKeyExchangeException(oldRatchet.nr, message.pn!); + } + } + + // TODO(PapaTutuWawa): Only do this when we should await _addSessionFromKeyExchange( senderJid, senderDeviceId, kex, ); - authMessage = kex.message!; - // Replace the OPK + // TODO(PapaTutuWawa): Replace the OPK when we know that the KEX worked await _deviceLock.synchronized(() async { device = await device.replaceOnetimePrekey(kex.pkId!); @@ -359,6 +385,7 @@ class OmemoSessionManager { }); } else { authMessage = OmemoAuthenticatedMessage.fromBuffer(decodedRawKey); + message = OmemoMessage.fromBuffer(authMessage.message!); } final devices = _deviceMap[senderJid]; @@ -369,11 +396,10 @@ class OmemoSessionManager { throw NoDecryptionKeyException(); } - final message = OmemoMessage.fromBuffer(authMessage.message!); List? keyAndHmac; // We can guarantee that the ratchet exists at this point in time final ratchet = (await _getRatchet(ratchetKey))!; - oldRatchet ??= ratchet ; + oldRatchet ??= ratchet.clone(); try { if (rawKey.kex) { diff --git a/test/double_ratchet_test.dart b/test/double_ratchet_test.dart index 58f6131..4644bc7 100644 --- a/test/double_ratchet_test.dart +++ b/test/double_ratchet_test.dart @@ -84,6 +84,7 @@ void main() { ikBob.pk, resultAlice.sk, resultAlice.ad, + 0, ); final bobsRatchet = await OmemoDoubleRatchet.acceptNewSession( spkBob, diff --git a/test/omemo_test.dart b/test/omemo_test.dart index 35d2432..cdaf4fc 100644 --- a/test/omemo_test.dart +++ b/test/omemo_test.dart @@ -653,7 +653,7 @@ void main() { expect(await bobRatchet1.equals(bobRatchet2), false); }); - test('Test receiving an old message that contains a KEX', () async { + test('Test receiving old messages including a KEX', () async { const aliceJid = 'alice@server.example'; const bobJid = 'bob@other.server.example'; // Alice and Bob generate their sessions @@ -668,6 +668,8 @@ void main() { opkAmount: 2, ); + final bobsReceivedMessages = List.empty(growable: true); + // Alice sends Bob a message final msg1 = await aliceSession.encryptToJid( bobJid, @@ -676,7 +678,7 @@ void main() { await bobSession.getDeviceBundle(), ], ); - + bobsReceivedMessages.add(msg1); await bobSession.decryptMessage( msg1.ciphertext, aliceJid, @@ -696,90 +698,51 @@ void main() { msg2.encryptedKeys, ); - // Due to some issue with the transport protocol, the first message Bob received is - // received again - try { - await bobSession.decryptMessage( - msg1.ciphertext, + // Send some messages between the two + for (var i = 0; i < 100; i++) { + final msg = await aliceSession.encryptToJid( + bobJid, + 'Hello $i', + ); + bobsReceivedMessages.add(msg); + final result = await bobSession.decryptMessage( + msg.ciphertext, aliceJid, await aliceSession.getDeviceId(), - msg1.encryptedKeys, + msg.encryptedKeys, ); - expect(true, false); - } on InvalidMessageHMACException { - // NOOP + + expect(result, 'Hello $i'); } - final msg3 = await aliceSession.encryptToJid( - bobJid, - 'Are you okay?', - ); - final result = await bobSession.decryptMessage( - msg3.ciphertext, - aliceJid, - await aliceSession.getDeviceId(), - msg3.encryptedKeys, - ); - - expect(result, 'Are you okay?'); - }); - - test('Test receiving an old message that does not contain a KEX', () async { - const aliceJid = 'alice@server.example'; - const bobJid = 'bob@other.server.example'; - // Alice and Bob generate their sessions - final aliceSession = await OmemoSessionManager.generateNewIdentity( - aliceJid, - AlwaysTrustingTrustManager(), - opkAmount: 1, - ); - final bobSession = await OmemoSessionManager.generateNewIdentity( - bobJid, - AlwaysTrustingTrustManager(), - opkAmount: 2, - ); - - // Alice sends Bob a message - final msg1 = await aliceSession.encryptToJid( - bobJid, - 'Hallo Welt', - newSessions: [ - await bobSession.getDeviceBundle(), - ], - ); - await bobSession.decryptMessage( - msg1.ciphertext, - aliceJid, - await aliceSession.getDeviceId(), - msg1.encryptedKeys, - ); - - // Bob responds - final msg2 = await bobSession.encryptToJid( - aliceJid, - 'Hello!', - ); - await aliceSession.decryptMessage( - msg2.ciphertext, - bobJid, - await bobSession.getDeviceId(), - msg2.encryptedKeys, - ); - - // Due to some issue with the transport protocol, the first message Alice received is - // received again. - try { - await aliceSession.decryptMessage( - msg2.ciphertext, - bobJid, - await bobSession.getDeviceId(), - msg2.encryptedKeys, - ); - expect(true, false); - } catch (_) { - // NOOP + // Due to some issue with the transport protocol, the messages to Bob are received + // again. + final ratchetPreError = bobSession + .getRatchet(aliceJid, await aliceSession.getDeviceId()) + .clone(); + var errorCounter = 0; + for (final msg in bobsReceivedMessages) { + try { + await bobSession.decryptMessage( + msg.ciphertext, + aliceJid, + await aliceSession.getDeviceId(), + msg.encryptedKeys, + ); + expect(true, false); + } catch (_) { + errorCounter++; + } } + final ratchetPostError = bobSession + .getRatchet(aliceJid, await aliceSession.getDeviceId()) + .clone(); + // The 100 messages including the initial KEX message + expect(errorCounter, 101); + expect(await ratchetPreError.equals(ratchetPostError), true); + + final msg3 = await aliceSession.encryptToJid( bobJid, 'Are you okay?', diff --git a/test/serialisation_test.dart b/test/serialisation_test.dart index 889f186..27ba65c 100644 --- a/test/serialisation_test.dart +++ b/test/serialisation_test.dart @@ -86,6 +86,7 @@ void main() { 'bob@localhost', await bobSession.getDeviceId(), await bobSession.getDeviceBundle(), + 0, ); // Serialise and deserialise