From cd5131daa5954c32497fcca3100e3793de4e7b7d Mon Sep 17 00:00:00 2001 From: Sorunome Date: Fri, 26 Feb 2021 12:39:32 +0100 Subject: [PATCH] fix: Add locking to sending encrypted to_device messages to prevent potential race conditions Due to chunked lazy sending of megolm sessions it was in theory that we encrypted two olm messages to the same device in different futures out-of-order. Introducing locking here should fix this (increadibly rare, so far only theoretical?) race-condition --- lib/src/client.dart | 31 +++++++++++---- lib/src/utils/device_keys_list.dart | 5 +++ lib/src/utils/multilock.dart | 51 ++++++++++++++++++++++++ test/multilock_test.dart | 62 +++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 lib/src/utils/multilock.dart create mode 100644 test/multilock_test.dart diff --git a/lib/src/client.dart b/lib/src/client.dart index 577b67ec..70479dab 100644 --- a/lib/src/client.dart +++ b/lib/src/client.dart @@ -38,6 +38,7 @@ import 'utils/matrix_file.dart'; import 'utils/room_update.dart'; import 'utils/to_device_event.dart'; import 'utils/uia_request.dart'; +import 'utils/multilock.dart'; typedef RoomSorter = int Function(Room a, Room b); @@ -1857,6 +1858,8 @@ sort order of ${prevState.sortOrder}. This should never happen...'''); return; } + final MultiLock _sendToDeviceEncryptedLock = MultiLock(); + /// Sends an encrypted [message] of this [eventType] to these [deviceKeys]. Future sendToDeviceEncrypted( List deviceKeys, @@ -1876,13 +1879,27 @@ sort order of ${prevState.sortOrder}. This should never happen...'''); if (deviceKeys.isEmpty) return; } - // Send with send-to-device messaging - var data = >>{}; - data = - await encryption.encryptToDeviceMessage(deviceKeys, eventType, message); - eventType = EventTypes.Encrypted; - await sendToDevice( - eventType, messageId ?? generateUniqueTransactionId(), data); + // So that we can guarantee order of encrypted to_device messages to be preserved we + // must ensure that we don't attempt to encrypt multiple concurrent to_device messages + // to the same device at the same time. + // A failure to do so can result in edge-cases where encryption and sending order of + // said to_device messages does not match up, resulting in an olm session corruption. + // As we send to multiple devices at the same time, we may only proceed here if the lock for + // *all* of them is freed and lock *all* of them while sending. + + try { + await _sendToDeviceEncryptedLock.lock(deviceKeys); + + // Send with send-to-device messaging + var data = >>{}; + data = await encryption.encryptToDeviceMessage( + deviceKeys, eventType, message); + eventType = EventTypes.Encrypted; + await sendToDevice( + eventType, messageId ?? generateUniqueTransactionId(), data); + } finally { + _sendToDeviceEncryptedLock.unlock(deviceKeys); + } } /// Sends an encrypted [message] of this [eventType] to these [deviceKeys]. diff --git a/lib/src/utils/device_keys_list.dart b/lib/src/utils/device_keys_list.dart index 4e6d6a5f..bc8088f1 100644 --- a/lib/src/utils/device_keys_list.dart +++ b/lib/src/utils/device_keys_list.dart @@ -333,6 +333,11 @@ abstract class SignableKey extends MatrixSignableKey { @override String toString() => json.encode(toJson()); + + @override + bool operator ==(dynamic other) => (other is SignableKey && + other.userId == userId && + other.identifier == identifier); } class CrossSigningKey extends SignableKey { diff --git a/lib/src/utils/multilock.dart b/lib/src/utils/multilock.dart new file mode 100644 index 00000000..30eb5890 --- /dev/null +++ b/lib/src/utils/multilock.dart @@ -0,0 +1,51 @@ +import 'dart:async'; + +/// Lock management class. It allows to lock and unlock multiple keys at once. The keys have +/// the type [T] +class MultiLock { + final Map> _completers = {}; + + /// Set a number of [keys] locks, awaiting them to be released previously. + Future lock(Iterable keys) async { + // An iterable might have duplicate entries. A set is guaranteed not to, and we need + // unique entries, as else a lot of things might go bad. + final uniqueKeys = keys.toSet(); + // we want to make sure that there are no existing completers for any of the locks + // we are trying to set. So, we await all the completers until they are all gone. + // We can't just assume they are all gone after one go, due to rare race conditions + // which could then result in a deadlock. + while (_completers.keys.any((k) => uniqueKeys.contains(k))) { + // Here we try to build all the futures to wait for single completers and then await + // them at the same time, in parallel + final futures = >[]; + for (final key in uniqueKeys) { + if (_completers[key] != null) { + futures.add(() async { + while (_completers[key] != null) { + await _completers[key].future; + } + }()); + } + } + await Future.wait(futures); + } + // And finally set all the completers + for (final key in uniqueKeys) { + _completers[key] = Completer(); + } + } + + /// Unlock all [keys] locks. Typically these should be the same keys as called + /// in `.lock(keys)`` + void unlock(Iterable keys) { + final uniqueKeys = keys.toSet(); + // we just have to simply unlock all the completers + for (final key in uniqueKeys) { + if (_completers[key] != null) { + final completer = _completers[key]; + _completers.remove(key); + completer.complete(); + } + } + } +} diff --git a/test/multilock_test.dart b/test/multilock_test.dart new file mode 100644 index 00000000..292b1e4e --- /dev/null +++ b/test/multilock_test.dart @@ -0,0 +1,62 @@ +/* + * Famedly Matrix SDK + * Copyright (C) 2021 Famedly GmbH + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import 'package:matrix/src/utils/multilock.dart'; +import 'package:test/test.dart'; + +void main() { + group('lock', () { + final lock = MultiLock(); + test('lock and unlock', () async { + // lock and unlock + await lock.lock(['fox']); + lock.unlock(['fox']); + expect(true, true); // we were able to reach this line of code! + }); + test('lock the same lock', () async { + var counter = 0; + await lock.lock(['fox']); + final future = lock.lock(['fox']).then((_) { + counter++; + lock.unlock(['fox']); + }); + await Future.delayed(Duration(milliseconds: 50)); + expect(counter, 0); + lock.unlock(['fox']); + await future; + expect(counter, 1); + }); + test('multilock', () async { + var counter = 0; + await lock.lock(['fox']); + final future1 = lock.lock(['fox', 'raccoon']).then((_) { + counter++; + lock.unlock(['fox', 'raccoon']); + }); + await Future.delayed(Duration(milliseconds: 50)); + expect(counter, 0); + await lock.lock(['raccoon']); + lock.unlock(['fox']); + await Future.delayed(Duration(milliseconds: 50)); + expect(counter, 0); + lock.unlock(['raccoon']); + await future1; + expect(counter, 1); + }); + }); +}