From 6facc6485af1fca4559a1827b1efc27655dbf728 Mon Sep 17 00:00:00 2001
From: Alexey Rusakov <Kitsune-Ral@users.sf.net>
Date: Wed, 20 Nov 2024 08:53:13 +0100
Subject: [PATCH] KeyVerificationSession: be more vigilant with the received
 commitment

The commitment is now checked to actually be valid base64 as we receive
it. Also, it is stored as QByteArray now - thanks to QString and
QByteArray having the same layout (3 pointers + alignment) we can get
away with this without breaking ABI compat.
---
 Quotient/keyverificationsession.cpp | 13 ++++++++-----
 Quotient/keyverificationsession.h   |  2 +-
 autotests/testkeyverification.cpp   |  8 +++++---
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/Quotient/keyverificationsession.cpp b/Quotient/keyverificationsession.cpp
index 7755cc9b4..aebd41073 100644
--- a/Quotient/keyverificationsession.cpp
+++ b/Quotient/keyverificationsession.cpp
@@ -164,7 +164,12 @@ void KeyVerificationSession::handleEvent(const KeyVerificationEvent& baseEvent)
                     cancelVerification(UNKNOWN_METHOD);
                     return false;
                 }
-                m_commitment = event.commitment();
+                m_commitment = event.commitment().toLatin1();
+                if (!QByteArray::fromBase64Encoding(m_commitment,
+                                                   QByteArray::AbortOnBase64DecodingErrors)) {
+                    cancelVerification(INVALID_MESSAGE);
+                    return false;
+                }
                 sendKey();
                 setState(WAITINGFORKEY);
                 return true;
@@ -228,12 +233,10 @@ void KeyVerificationSession::handleKey(const KeyVerificationKeyEvent& event)
     olm_sas_set_their_key(olmData, eventKey.data(), unsignedSize(eventKey));
 
     if (startSentByUs) {
-        const auto paddedCommitment =
+        const auto unpaddedCommitment =
             QCryptographicHash::hash((event.key() % m_startEvent).toLatin1(),
                                      QCryptographicHash::Sha256)
-                .toBase64();
-        const QLatin1String unpaddedCommitment(paddedCommitment.constData(),
-                                               QString::fromLatin1(paddedCommitment).indexOf(u'='));
+                .toBase64(QByteArray::OmitTrailingEquals);
         if (unpaddedCommitment != m_commitment) {
             qCWarning(E2EE) << "Commitment mismatch; aborting verification";
             cancelVerification(MISMATCHED_COMMITMENT);
diff --git a/Quotient/keyverificationsession.h b/Quotient/keyverificationsession.h
index a28db2ecb..3c568ef4f 100644
--- a/Quotient/keyverificationsession.h
+++ b/Quotient/keyverificationsession.h
@@ -158,7 +158,7 @@ public Q_SLOTS:
     State m_state = INCOMING;
     Error m_error = NONE;
     QString m_startEvent{};
-    QString m_commitment{};
+    QByteArray m_commitment{};
     bool macReceived = false;
     bool m_verified = false;
     QString m_pendingEdKeyId{};
diff --git a/autotests/testkeyverification.cpp b/autotests/testkeyverification.cpp
index 9e8fabb74..604c63864 100644
--- a/autotests/testkeyverification.cpp
+++ b/autotests/testkeyverification.cpp
@@ -9,6 +9,7 @@
 
 #include <Quotient/connection.h>
 #include <Quotient/e2ee/qolmaccount.h>
+#include <Quotient/e2ee/cryptoutils.h>
 
 using namespace Quotient;
 
@@ -26,7 +27,8 @@ private Q_SLOTS:
         QVERIFY(session->state() == KeyVerificationSession::WAITINGFORREADY);
         session->handleEvent(KeyVerificationReadyEvent(transactionId, "ABCDEF"_L1, {SasV1Method}));
         QVERIFY(session->state() == KeyVerificationSession::WAITINGFORACCEPT);
-        session->handleEvent(KeyVerificationAcceptEvent(transactionId, "commitment_TODO"_L1));
+        session->handleEvent(KeyVerificationAcceptEvent(
+            transactionId, QString::fromLatin1("commitment_TODO"_ba.toBase64())));
         QVERIFY(session->state() == KeyVerificationSession::WAITINGFORKEY);
         // Since we can't get the events sent by the session, we're limited by what we can test. This means that continuing here would force us to test
         // the exact same path as the other test, which is useless.
@@ -51,8 +53,8 @@ private Q_SLOTS:
         auto sas = olm_sas(new std::byte[olm_sas_size()]);
         const auto randomLength = olm_create_sas_random_length(sas);
         olm_create_sas(sas, getRandom(randomLength).data(), randomLength);
-        QByteArray keyBytes(olm_sas_pubkey_length(sas), '\0');
-        olm_sas_get_pubkey(sas, keyBytes.data(), keyBytes.size());
+        auto keyBytes = byteArrayForOlm(olm_sas_pubkey_length(sas));
+        olm_sas_get_pubkey(sas, keyBytes.data(), unsignedSize(keyBytes));
         session->handleEvent(KeyVerificationKeyEvent(transactionId, QString::fromLatin1(keyBytes)));
         QVERIFY(session->state() == KeyVerificationSession::WAITINGFORVERIFICATION);
         session->sendMac();