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.
This commit is contained in:
PapaTutuWawa 2022-10-02 14:56:20 +02:00
parent 7c3a9a75df
commit 2aa3674c4b
6 changed files with 125 additions and 97 deletions

View File

@ -167,7 +167,7 @@ class OmemoDoubleRatchet {
/// Create an OMEMO session using the Signed Pre Key [spk], the shared secret [sk] that /// 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 /// 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. /// a X3DH. [ik] refers to Bob's (the receiver's) IK public key.
static Future<OmemoDoubleRatchet> initiateNewSession(OmemoPublicKey spk, OmemoPublicKey ik, List<int> sk, List<int> ad) async { static Future<OmemoDoubleRatchet> initiateNewSession(OmemoPublicKey spk, OmemoPublicKey ik, List<int> sk, List<int> ad, int pn) async {
final dhs = await OmemoKeyPair.generateNewPair(KeyPairType.x25519); final dhs = await OmemoKeyPair.generateNewPair(KeyPairType.x25519);
final dhr = spk; final dhr = spk;
final rk = await kdfRk(sk, await omemoDH(dhs, dhr, 0)); final rk = await kdfRk(sk, await omemoDH(dhs, dhr, 0));
@ -181,7 +181,7 @@ class OmemoDoubleRatchet {
null, null,
0, 0,
0, 0,
0, pn,
ik, ik,
ad, ad,
{}, {},
@ -330,12 +330,39 @@ class OmemoDoubleRatchet {
return decrypt(mk, ciphertext, concat([sessionAd, header.writeToBuffer()]), sessionAd); return decrypt(mk, ciphertext, concat([sessionAd, header.writeToBuffer()]), sessionAd);
} }
OmemoDoubleRatchet clone() {
return OmemoDoubleRatchet(
dhs,
dhr,
rk,
cks != null ?
List<int>.from(cks!) :
null,
ckr != null ?
List<int>.from(ckr!) :
null,
ns,
nr,
pn,
ik,
sessionAd,
Map<SkippedKey, List<int>>.from(mkSkipped),
acknowledged,
);
}
@visibleForTesting @visibleForTesting
Future<bool> equals(OmemoDoubleRatchet other) async { Future<bool> equals(OmemoDoubleRatchet other) async {
// ignore: invalid_use_of_visible_for_testing_member final dhrMatch = dhr == null ?
final dhrMatch = dhr == null ? other.dhr == null : await dhr!.equals(other.dhr!); other.dhr == null :
final ckrMatch = ckr == null ? other.ckr == null : listsEqual(ckr!, other.ckr!); // ignore: invalid_use_of_visible_for_testing_member
final cksMatch = cks == null ? other.cks == null : listsEqual(cks!, other.cks!); 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 // ignore: invalid_use_of_visible_for_testing_member
final dhsMatch = await dhs.equals(other.dhs); final dhsMatch = await dhs.equals(other.dhs);

View File

@ -30,3 +30,13 @@ class NoDecryptionKeyException implements Exception {
class UnknownSignedPrekeyException implements Exception { class UnknownSignedPrekeyException implements Exception {
String errMsg() => 'Unknown Signed Prekey used.'; 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';
}

View File

@ -143,7 +143,7 @@ class OmemoSessionManager {
/// Create a ratchet session initiated by Alice to the user with Jid [jid] and the device /// Create a ratchet session initiated by Alice to the user with Jid [jid] and the device
/// [deviceId] from the bundle [bundle]. /// [deviceId] from the bundle [bundle].
@visibleForTesting @visibleForTesting
Future<OmemoKeyExchange> addSessionFromBundle(String jid, int deviceId, OmemoBundle bundle) async { Future<OmemoKeyExchange> addSessionFromBundle(String jid, int deviceId, OmemoBundle bundle, int pn) async {
final device = await getDevice(); final device = await getDevice();
final kexResult = await x3dhFromBundle( final kexResult = await x3dhFromBundle(
bundle, bundle,
@ -154,6 +154,7 @@ class OmemoSessionManager {
bundle.ik, bundle.ik,
kexResult.sk, kexResult.sk,
kexResult.ad, kexResult.ad,
pn,
); );
await _trustManager.onNewSession(jid, deviceId); await _trustManager.onNewSession(jid, deviceId);
@ -240,7 +241,22 @@ class OmemoSessionManager {
final kex = <int, OmemoKeyExchange>{}; final kex = <int, OmemoKeyExchange>{};
if (newSessions != null) { if (newSessions != null) {
for (final newSession in newSessions) { 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]. /// [mapKey] with [oldRatchet].
Future<void> _restoreRatchet(RatchetMapKey mapKey, OmemoDoubleRatchet oldRatchet) async { Future<void> _restoreRatchet(RatchetMapKey mapKey, OmemoDoubleRatchet oldRatchet) async {
await _lock.synchronized(() { 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; _ratchetMap[mapKey] = oldRatchet;
// Commit the ratchet // Commit the ratchet
@ -335,22 +351,32 @@ class OmemoSessionManager {
final decodedRawKey = base64.decode(rawKey.value); final decodedRawKey = base64.decode(rawKey.value);
OmemoAuthenticatedMessage authMessage; OmemoAuthenticatedMessage authMessage;
OmemoDoubleRatchet? oldRatchet; OmemoDoubleRatchet? oldRatchet;
OmemoMessage? message;
if (rawKey.kex) { if (rawKey.kex) {
// If the ratchet already existed, we store it. If it didn't, oldRatchet will stay // If the ratchet already existed, we store it. If it didn't, oldRatchet will stay
// null. // null.
oldRatchet = await _getRatchet(ratchetKey); 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 // TODO(PapaTutuWawa): Only do this when we should
final kex = OmemoKeyExchange.fromBuffer(decodedRawKey);
await _addSessionFromKeyExchange( await _addSessionFromKeyExchange(
senderJid, senderJid,
senderDeviceId, senderDeviceId,
kex, kex,
); );
authMessage = kex.message!;
// Replace the OPK // Replace the OPK
// TODO(PapaTutuWawa): Replace the OPK when we know that the KEX worked
await _deviceLock.synchronized(() async { await _deviceLock.synchronized(() async {
device = await device.replaceOnetimePrekey(kex.pkId!); device = await device.replaceOnetimePrekey(kex.pkId!);
@ -359,6 +385,7 @@ class OmemoSessionManager {
}); });
} else { } else {
authMessage = OmemoAuthenticatedMessage.fromBuffer(decodedRawKey); authMessage = OmemoAuthenticatedMessage.fromBuffer(decodedRawKey);
message = OmemoMessage.fromBuffer(authMessage.message!);
} }
final devices = _deviceMap[senderJid]; final devices = _deviceMap[senderJid];
@ -369,11 +396,10 @@ class OmemoSessionManager {
throw NoDecryptionKeyException(); throw NoDecryptionKeyException();
} }
final message = OmemoMessage.fromBuffer(authMessage.message!);
List<int>? keyAndHmac; List<int>? keyAndHmac;
// We can guarantee that the ratchet exists at this point in time // We can guarantee that the ratchet exists at this point in time
final ratchet = (await _getRatchet(ratchetKey))!; final ratchet = (await _getRatchet(ratchetKey))!;
oldRatchet ??= ratchet ; oldRatchet ??= ratchet.clone();
try { try {
if (rawKey.kex) { if (rawKey.kex) {

View File

@ -84,6 +84,7 @@ void main() {
ikBob.pk, ikBob.pk,
resultAlice.sk, resultAlice.sk,
resultAlice.ad, resultAlice.ad,
0,
); );
final bobsRatchet = await OmemoDoubleRatchet.acceptNewSession( final bobsRatchet = await OmemoDoubleRatchet.acceptNewSession(
spkBob, spkBob,

View File

@ -653,7 +653,7 @@ void main() {
expect(await bobRatchet1.equals(bobRatchet2), false); 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 aliceJid = 'alice@server.example';
const bobJid = 'bob@other.server.example'; const bobJid = 'bob@other.server.example';
// Alice and Bob generate their sessions // Alice and Bob generate their sessions
@ -668,6 +668,8 @@ void main() {
opkAmount: 2, opkAmount: 2,
); );
final bobsReceivedMessages = List<EncryptionResult>.empty(growable: true);
// Alice sends Bob a message // Alice sends Bob a message
final msg1 = await aliceSession.encryptToJid( final msg1 = await aliceSession.encryptToJid(
bobJid, bobJid,
@ -676,7 +678,7 @@ void main() {
await bobSession.getDeviceBundle(), await bobSession.getDeviceBundle(),
], ],
); );
bobsReceivedMessages.add(msg1);
await bobSession.decryptMessage( await bobSession.decryptMessage(
msg1.ciphertext, msg1.ciphertext,
aliceJid, aliceJid,
@ -696,89 +698,50 @@ void main() {
msg2.encryptedKeys, msg2.encryptedKeys,
); );
// Due to some issue with the transport protocol, the first message Bob received is // Send some messages between the two
// received again for (var i = 0; i < 100; i++) {
try { final msg = await aliceSession.encryptToJid(
await bobSession.decryptMessage( bobJid,
msg1.ciphertext, 'Hello $i',
);
bobsReceivedMessages.add(msg);
final result = await bobSession.decryptMessage(
msg.ciphertext,
aliceJid, aliceJid,
await aliceSession.getDeviceId(), await aliceSession.getDeviceId(),
msg1.encryptedKeys, msg.encryptedKeys,
); );
expect(true, false);
} on InvalidMessageHMACException { expect(result, 'Hello $i');
// NOOP
} }
final msg3 = await aliceSession.encryptToJid( // Due to some issue with the transport protocol, the messages to Bob are received
bobJid, // again.
'Are you okay?', final ratchetPreError = bobSession
); .getRatchet(aliceJid, await aliceSession.getDeviceId())
final result = await bobSession.decryptMessage( .clone();
msg3.ciphertext, var errorCounter = 0;
aliceJid, for (final msg in bobsReceivedMessages) {
await aliceSession.getDeviceId(), try {
msg3.encryptedKeys, await bobSession.decryptMessage(
); msg.ciphertext,
aliceJid,
expect(result, 'Are you okay?'); await aliceSession.getDeviceId(),
}); msg.encryptedKeys,
);
test('Test receiving an old message that does not contain a KEX', () async { expect(true, false);
const aliceJid = 'alice@server.example'; } catch (_) {
const bobJid = 'bob@other.server.example'; errorCounter++;
// 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
} }
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( final msg3 = await aliceSession.encryptToJid(
bobJid, bobJid,

View File

@ -86,6 +86,7 @@ void main() {
'bob@localhost', 'bob@localhost',
await bobSession.getDeviceId(), await bobSession.getDeviceId(),
await bobSession.getDeviceBundle(), await bobSession.getDeviceBundle(),
0,
); );
// Serialise and deserialise // Serialise and deserialise