From 6a18fdc9bf36abedb851066715ad0785089c56f3 Mon Sep 17 00:00:00 2001 From: Tobias Fella Date: Thu, 17 Oct 2024 18:38:10 +0200 Subject: [PATCH] Use real pickling key --- CMakeLists.txt | 4 +- Quotient/connection.cpp | 80 +++++++++++++- Quotient/connection_p.h | 4 + Quotient/e2ee/e2ee_common.cpp | 171 ++++++++++++++++++++++++++++++ matrix-rust-sdk-crypto/src/lib.rs | 8 +- 5 files changed, 256 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2d1afe88b..f4d3e7f12 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -334,9 +334,11 @@ target_include_directories(${QUOTIENT_LIB_NAME} PUBLIC $>:${CMAKE_CURRENT_SOURCE_DIR}/Quotient>> ) +find_package(OpenSSL REQUIRED) + target_link_libraries(${QUOTIENT_LIB_NAME} PUBLIC ${Qt}::Core ${Qt}::Network ${Qt}::Gui qt${${Qt}Core_VERSION_MAJOR}keychain ${Qt}::Sql - PRIVATE ${Qt}::CorePrivate matrix_rust_sdk_crypto_cpp) + PRIVATE ${Qt}::CorePrivate matrix_rust_sdk_crypto_cpp OpenSSL::Crypto) if(NOT WIN32) target_link_libraries(${QUOTIENT_LIB_NAME} PRIVATE PkgConfig::SQLITE) diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index 12796a38e..315e22952 100644 --- a/Quotient/connection.cpp +++ b/Quotient/connection.cpp @@ -319,6 +319,73 @@ void Connection::Private::loginToServer(LoginArgTs&&... loginArgs) }); } +inline QFuture runKeychainJob(QKeychain::Job* j, const QString& keychainId) +{ + j->setAutoDelete(true); + j->setKey(keychainId); + auto ft = QtFuture::connect(j, &QKeychain::Job::finished); + j->start(); + return ft; +} + +QFuture Connection::Private::setupPicklingKey() +{ + using namespace QKeychain; + const auto keychainId = q->userId() + "-Pickle"_L1; + qCInfo(MAIN) << "Keychain request: app" << qAppName() << "id" << keychainId; + + return runKeychainJob(new ReadPasswordJob(qAppName()), keychainId) + .then([keychainId, this](const Job* j) -> QFuture { + // The future will hold nullptr if the existing pickling key was found and no write is + // pending; a pointer to the write job if if a new key was made and is being written; + // be cancelled in case of an error. + switch (const auto readJob = static_cast(j); readJob->error()) { + case Error::NoError: { + auto&& data = readJob->binaryData(); + qDebug(E2EE) << "Successfully loaded pickling key from keychain"; + + setupCryptoMachine(data); + return QtFuture::makeReadyFuture(nullptr); + qCritical(E2EE) + << "The pickling key loaded from" << keychainId << "has length" + << data.size() << "but the library expected" << PicklingKey::extent; + return {}; + } + case Error::EntryNotFound: { + auto&& picklingKey = PicklingKey::generate(); + auto writeJob = new WritePasswordJob(qAppName()); + setupCryptoMachine(picklingKey.viewAsByteArray().toBase64()); + writeJob->setBinaryData(picklingKey.viewAsByteArray().toBase64()); + qDebug(E2EE) << "Saving a new pickling key to the keychain"; + return runKeychainJob(writeJob, keychainId); + } + default: + qWarning(E2EE) << "Error loading pickling key - please fix your keychain:" + << readJob->errorString(); + //TODO: We probably want to fail entirely here. + } + return {}; + }) + .unwrap() + .then([](QFuture writeFuture) { + if (const Job* const writeJob = writeFuture.result(); + writeJob && writeJob->error() != Error::NoError) // + { + qCritical(E2EE) << "Could not save pickling key to keychain: " + << writeJob->errorString(); + writeFuture.cancel(); + } + }); +} + +void Connection::Private::setupCryptoMachine(const QByteArray& picklingKey) +{ + auto mxIdForDb = q->userId(); + mxIdForDb.replace(u':', u'_'); + const QString databasePath{ QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) % u'/' % mxIdForDb % u'/' % q->deviceId() }; + cryptoMachine = crypto::init(stringToRust(q->userId()), stringToRust(q->deviceId()), stringToRust(databasePath), bytesToRust(picklingKey)); +} + void Connection::Private::completeSetup(const QString& mxId, bool newLogin, const std::optional& deviceId, const std::optional& accessToken) @@ -329,10 +396,7 @@ void Connection::Private::completeSetup(const QString& mxId, bool newLogin, << "by user" << data->userId() << "from device" << data->deviceId(); connect(qApp, &QCoreApplication::aboutToQuit, q, &Connection::saveState); - auto mxIdForDb = mxId; - mxIdForDb.replace(u':', u'_'); - const QString databasePath{ QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) % u'/' % mxIdForDb % u'/' % *deviceId }; - cryptoMachine = crypto::init(stringToRust(mxId), stringToRust(*deviceId), stringToRust(databasePath)); + setupPicklingKey(); if (accessToken.has_value()) { q->loadVersions(); @@ -528,6 +592,10 @@ void Connection::onSyncSuccess(SyncData&& data, bool fromCache) void Connection::Private::processOutgoingRequests() { + if (!cryptoMachine) { + qWarning() << "Crypto machine not loaded yet"; + return; + } auto requests = (*cryptoMachine)->outgoing_requests(); for (const auto &request : requests) { auto id = stringFromRust(request.id()); @@ -622,7 +690,9 @@ void Connection::Private::consumeRoomData(SyncDataList&& roomDataList, for (const auto &id : ids) { userIds.push_back(stringToRust(id)); } - (*cryptoMachine)->update_tracked_users(userIds); + if (cryptoMachine) { + (*cryptoMachine)->update_tracked_users(userIds); + } } }, Qt::QueuedConnection); diff --git a/Quotient/connection_p.h b/Quotient/connection_p.h index cf3ee42ac..8c885ef52 100644 --- a/Quotient/connection_p.h +++ b/Quotient/connection_p.h @@ -9,6 +9,7 @@ #include "connectiondata.h" #include "settings.h" #include "syncdata.h" +#include "e2ee/e2ee_common.h" #include "csapi/account-data.h" #include "csapi/capabilities.h" @@ -160,5 +161,8 @@ class Q_DECL_HIDDEN Quotient::Connection::Private { void requestDeviceVerification(KeyVerificationSession* session); void requestUserVerification(KeyVerificationSession* session); + + QFuture setupPicklingKey(); + void setupCryptoMachine(const QByteArray& picklingKey); }; } // namespace Quotient diff --git a/Quotient/e2ee/e2ee_common.cpp b/Quotient/e2ee/e2ee_common.cpp index 5cf15cac3..d3c04da86 100644 --- a/Quotient/e2ee/e2ee_common.cpp +++ b/Quotient/e2ee/e2ee_common.cpp @@ -8,5 +8,176 @@ #include +#include + using namespace Quotient; +QByteArray Quotient::byteArrayForOlm(size_t bufferSize) +{ + if (std::in_range(bufferSize)) [[likely]] + return { static_cast(bufferSize), '\0' }; + + qCritical(E2EE) << "Buffer size out of QByteArray range:" << bufferSize; + // Zero-length QByteArray is an almost guaranteed way to cause + // an internal error in QOlm* classes, unless checked + return {}; +} + +void Quotient::_impl::checkForSpanShortfall(QByteArray::size_type inputSize, int neededSize) +{ + if (inputSize < neededSize) { + qCCritical(E2EE) << "Not enough bytes to create a valid span: " + << inputSize << '<' << neededSize + << "- undefined behaviour imminent"; + Q_ASSERT(false); + // Can't help it in Release builds; a span of the given size has + // to be returned regardless, so UB + } +} + +void Quotient::fillFromSecureRng(std::span bytes) +{ + // Discussion of QRandomGenerator::system() vs. OpenSSL's RAND_bytes + // + // It is a rather close call between the two; to be honest, TL;DR from the + // below is "it doesn't really matter". Going through the source code of + // both libraries, along with reading others' investigations like + // https://github.com/golang/go/issues/33542, left me (@kitsune) with + // the following: + // 1. Both ultimately use more or less the same stuff internally - which is + // is not surprising and is reassuring in TL;DR. Contrary to occasional + // claims on interwebs though, QRandomGenerator does not rely on OpenSSL, + // calling system primitives directly instead. + // 2. Qt's code is massively simpler - again, effectively it's a rather + // thin wrapper around what I know for a reasonably good practice across + // the platforms supported by Quotient. + // 3. OpenSSL is more flexible in that you can specify which RNG engine + // you actually want: you can, e.g., explicitly use the hardware-based + // (aka "true") RNG on modern Intel CPUs. QRandomGenerator "simply" + // offers the nice Qt interface to whatever best the operating system + // provides (and at least Linux kernel happens to also use true RNGs + // where it can, so ¯\(ツ)/¯) + // 3. QRandomGenerator produces stuff in uint32_t (or uint64_t in case of + // QRandomGenerator64) chunks; OpenSSL generates bytes, which is sorta + // more convenient. Quotient never needs quantities of randomness that + // are not multiples of 16 _bytes_; the Q_UNLIKELY branch below is merely + // to cover an edge case that should never happen in the first place. + // So, ¯\(ツ)/¯ again. + // + // Based on this, QRandomGenerator::system() wins by a tiny non-functional + // margin; my somewhat educated opinion for now is that both APIs are + // equally good from the functionality and security point of view. + + // QRandomGenerator::fillRange works in terms of 32-bit words, + // and FixedBuffer happens to deal with sizes that are multiples + // of those (16, 32, etc.) + const qsizetype wordsCount = bytes.size() / 4; + QRandomGenerator::system()->fillRange(std::bit_cast(bytes.data()), wordsCount); + if (const int remainder = bytes.size() % 4; Q_UNLIKELY(remainder != 0)) { + // Not normal; but if it happens, apply best effort + QRandomGenerator::system()->generate(bytes.end() - remainder, + bytes.end()); + } +} + +namespace { +auto initializeSecureHeap() +{ +#if !defined(LIBRESSL_VERSION_NUMBER) + const auto result = + CRYPTO_secure_malloc_init(FixedBufferBase::TotalSecureHeapSize, 16); + if (result > 0) { + qDebug(E2EE) << FixedBufferBase::TotalSecureHeapSize + << "bytes of secure heap initialised"; + if (std::atexit([] { + CRYPTO_secure_malloc_done(); + qDebug(E2EE) << "Dismantled secure heap"; + }) + != 0) + qWarning(E2EE) + << "Could not register a cleanup function for secure heap!"; + } else + qCritical(E2EE) << "Secure heap could not be initialised, sensitive " + "data will remain in common dynamic memory"; +#else + const auto result = 0; + qCritical(E2EE) << "Secure heap is not available in LibreSSL"; +#endif + return result; +} + +FixedBufferBase::value_type* allocate(size_t bytes, bool initWithZeros = false) +{ +#if !defined(LIBRESSL_VERSION_NUMBER) + static auto secureHeapInitialised [[maybe_unused]] = initializeSecureHeap(); + + const auto p = static_cast( + initWithZeros ? OPENSSL_secure_zalloc(bytes) : OPENSSL_secure_malloc(bytes)); + Q_ASSERT(CRYPTO_secure_allocated(p)); + qDebug(E2EE) << "Allocated" << CRYPTO_secure_actual_size(p) + << "bytes of secure heap (requested" << bytes << "bytes)," + << CRYPTO_secure_used() + << "/ 65536 bytes of secure heap used in total"; +#else + const auto p = + static_cast(initWithZeros ? calloc(bytes, 1) : malloc(bytes)); +#endif + return p; +} + +} // namespace + +FixedBufferBase::FixedBufferBase(size_type bufferSize, InitOptions options) + : size_(bufferSize) +{ + if (bufferSize >= TotalSecureHeapSize) { + qCritical(E2EE) << "Too large buffer size:" << bufferSize; + return; + } + if (options == Uninitialized) + return; + + data_ = allocate(bufferSize, options == FillWithZeros); + if (options == FillWithRandom) + fillFromSecureRng({ data_, bufferSize }); +} + +void FixedBufferBase::fillFrom(QByteArray&& source) +{ + if (unsignedSize(source) != size_) { + qCritical(E2EE) << "Can't load a fixed buffer of length" << size_ + << "from a string with length" << source.size(); + Q_ASSERT(unsignedSize(source) == size_); // Always false + return; + } + if (data_ != nullptr) { + qWarning(E2EE) << "Overwriting the fixed buffer with another string"; + clear(); + } + + data_ = allocate(size_); + std::copy(source.cbegin(), source.cend(), std::bit_cast(data_)); + if (source.isDetached()) + source.clear(); + else + qWarning(E2EE) + << "The fixed buffer source is shared; assuming that the caller is " + "responsible for securely clearing other copies"; +} + +void FixedBufferBase::clear() +{ + if (empty()) + return; + +#if !defined(LIBRESSL_VERSION_NUMBER) + Q_ASSERT(CRYPTO_secure_allocated(data_)); + const auto actualSize = OPENSSL_secure_actual_size(data_); + OPENSSL_secure_clear_free(data_, size_); + qDebug(E2EE) << "Deallocated" << actualSize << "bytes," + << CRYPTO_secure_used() << "/ 65536 bytes of secure heap used"; +#else + free(data_); +#endif + data_ = nullptr; +} diff --git a/matrix-rust-sdk-crypto/src/lib.rs b/matrix-rust-sdk-crypto/src/lib.rs index 37e706c4c..ac662a46a 100644 --- a/matrix-rust-sdk-crypto/src/lib.rs +++ b/matrix-rust-sdk-crypto/src/lib.rs @@ -34,7 +34,7 @@ impl Drop for CryptoMachine { } } -fn init(user_id: String, device_id: String, path: String) -> Box { +fn init(user_id: String, device_id: String, path: String, pickle_key: String) -> Box { let rt = tokio::runtime::Runtime::new().unwrap(); let _ = rt.enter(); @@ -42,9 +42,7 @@ fn init(user_id: String, device_id: String, path: String) -> Box let user_id = UserId::parse(user_id).unwrap(); let device_id: Box = device_id.into(); - //TODO: use real pickle key - println!("path: {path}"); - let store = SqliteCryptoStore::open(Path::new(&path), "123123123".into()).await.unwrap(); + let store = SqliteCryptoStore::open(Path::new(&path), Some(&pickle_key)).await.unwrap(); OlmMachine::with_store(&user_id, &device_id, store) .await @@ -845,7 +843,7 @@ mod ffi { type EncryptionInfo; // General CryptoMachine functions - fn init(user_id: String, device_id: String, path: String) -> Box; + fn init(user_id: String, device_id: String, path: String, pickle_key: String) -> Box; fn outgoing_requests(self: &CryptoMachine) -> Vec; fn receive_sync_changes(self: &mut CryptoMachine, sync_json: String) -> Vec; fn receive_verification_event(self: &mut CryptoMachine, full_json: String);