From 86a1888e39765dd57762c4422e6ede0c4a408935 Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Wed, 1 Jul 2020 21:09:34 +0200 Subject: QXmppMessageReceiptManager: Ignore all error messages Not only receipt requests, but also receipts from error messages should not be used. --- src/client/QXmppMessageReceiptManager.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/client') diff --git a/src/client/QXmppMessageReceiptManager.cpp b/src/client/QXmppMessageReceiptManager.cpp index 27d96d11..ea4bef96 100644 --- a/src/client/QXmppMessageReceiptManager.cpp +++ b/src/client/QXmppMessageReceiptManager.cpp @@ -53,6 +53,9 @@ bool QXmppMessageReceiptManager::handleStanza(const QDomElement &stanza) QXmppMessage message; message.parse(stanza); + if (message.type() == QXmppMessage::Error) + return false; + // Handle receipts and cancel any further processing. if (!message.receiptId().isEmpty()) { // Buggy clients also mark carbon messages as received; to avoid this @@ -64,7 +67,7 @@ bool QXmppMessageReceiptManager::handleStanza(const QDomElement &stanza) } // If requested, send a receipt. - if (message.isReceiptRequested() && !message.from().isEmpty() && !message.id().isEmpty() && message.type() != QXmppMessage::Error) { + if (message.isReceiptRequested() && !message.from().isEmpty() && !message.id().isEmpty()) { QXmppMessage receipt; receipt.setTo(message.from()); receipt.setReceiptId(message.id()); -- cgit v1.2.3 From e73120d4ab5b1a93c6ad051ff6807af02cd8a039 Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Sun, 19 Jul 2020 13:50:13 +0200 Subject: QXmppCarbonManager: Fix vulnerability: Add sender check The XEP requires that only carbon messages from the client's bare JID are accepted. This prevents that other entities can inject messages into the client. --- src/client/QXmppCarbonManager.cpp | 6 ++ .../qxmppcarbonmanager/tst_qxmppcarbonmanager.cpp | 69 ++++++++++++++++++++-- 2 files changed, 71 insertions(+), 4 deletions(-) (limited to 'src/client') diff --git a/src/client/QXmppCarbonManager.cpp b/src/client/QXmppCarbonManager.cpp index d403e6de..e936ddf2 100644 --- a/src/client/QXmppCarbonManager.cpp +++ b/src/client/QXmppCarbonManager.cpp @@ -93,6 +93,12 @@ bool QXmppCarbonManager::handleStanza(const QDomElement &element) if (carbon.isNull() || carbon.namespaceURI() != ns_carbons) return false; // Neither sent nor received -> no carbon message + // carbon copies must always come from our bare JID + if (element.attribute("from") != client()->configuration().jidBare()) { + info("Received carbon copy from possible attacker trying to use CVE-2017-5603."); + return false; + } + QDomElement forwarded = carbon.firstChildElement("forwarded"); if (forwarded.isNull()) return false; diff --git a/tests/qxmppcarbonmanager/tst_qxmppcarbonmanager.cpp b/tests/qxmppcarbonmanager/tst_qxmppcarbonmanager.cpp index 6166c6f1..684c7470 100644 --- a/tests/qxmppcarbonmanager/tst_qxmppcarbonmanager.cpp +++ b/tests/qxmppcarbonmanager/tst_qxmppcarbonmanager.cpp @@ -23,6 +23,7 @@ */ #include "QXmppCarbonManager.h" +#include "QXmppClient.h" #include "QXmppMessage.h" #include "util.h" @@ -56,16 +57,24 @@ private slots: private: QXmppCarbonTestHelper m_helper; - QXmppCarbonManager m_manager; + QXmppCarbonManager *m_manager; + QXmppClient client; }; void tst_QXmppCarbonManager::initTestCase() { - connect(&m_manager, &QXmppCarbonManager::messageSent, + m_manager = new QXmppCarbonManager(); + + connect(m_manager, &QXmppCarbonManager::messageSent, &m_helper, &QXmppCarbonTestHelper::messageSent); - connect(&m_manager, &QXmppCarbonManager::messageReceived, + connect(m_manager, &QXmppCarbonManager::messageReceived, &m_helper, &QXmppCarbonTestHelper::messageReceived); + + client.connectToServer("romeo@montague.example", "a"); + client.disconnectFromServer(); + + client.addExtension(m_manager); } void tst_QXmppCarbonManager::testHandleStanza_data() @@ -127,6 +136,58 @@ void tst_QXmppCarbonManager::testHandleStanza_data() "0e3141cd80894871a68e6fe6b1ec56fa" ""); + QTest::newRow("received-wrong-from") + << QByteArray("" + "" + "" + "" + "What man art thou that, thus bescreen'd in night, so stumblest on my counsel?" + "0e3141cd80894871a68e6fe6b1ec56fa" + "" + "" + "" + "") + << false << false + << QByteArray("" + "What man art thou that, thus bescreen'd in night, so stumblest on my counsel?" + "0e3141cd80894871a68e6fe6b1ec56fa" + ""); + + QTest::newRow("sent-wrong-from") + << QByteArray("" + "" + "" + "" + "Neither, fair saint, if either thee dislike." + "0e3141cd80894871a68e6fe6b1ec56fa" + "" + "" + "" + "") + << false << true + << QByteArray("" + "Neither, fair saint, if either thee dislike." + "0e3141cd80894871a68e6fe6b1ec56fa" + ""); + QTest::newRow("forwarded_normal") << QByteArray("" "A most courteous exposition!" @@ -167,7 +228,7 @@ void tst_QXmppCarbonManager::testHandleStanza() QCOMPARE(doc.setContent(xml, true), true); QDomElement element = doc.documentElement(); - bool accepted = m_manager.handleStanza(element); + bool accepted = m_manager->handleStanza(element); QCOMPARE(accepted, accept); QCOMPARE(m_helper.m_signalTriggered, accept); -- cgit v1.2.3 From 0710dee7e93ad0cfa0f0d12fdcefa7dc2d7c9865 Mon Sep 17 00:00:00 2001 From: Linus Jahn Date: Mon, 20 Jul 2020 16:22:47 +0200 Subject: Clean up QXmppCarbonManager --- src/client/QXmppCarbonManager.cpp | 40 ++++++++++++++++++++++++++++----------- src/client/QXmppCarbonManager.h | 7 ------- 2 files changed, 29 insertions(+), 18 deletions(-) (limited to 'src/client') diff --git a/src/client/QXmppCarbonManager.cpp b/src/client/QXmppCarbonManager.cpp index e936ddf2..45dd96da 100644 --- a/src/client/QXmppCarbonManager.cpp +++ b/src/client/QXmppCarbonManager.cpp @@ -40,21 +40,24 @@ QXmppCarbonManager::~QXmppCarbonManager() { } +/// /// Returns whether message carbons are currently enabled - +/// bool QXmppCarbonManager::carbonsEnabled() const { return m_carbonsEnabled; } -/// Enable or disable message carbons. +/// +/// Enables or disables message carbons for this connection. +/// /// This function does not check whether the server supports /// message carbons, but just sends the corresponding stanza /// to the server, so one must check in advance by using the /// discovery manager. /// /// By default, carbon copies are disabled. - +/// void QXmppCarbonManager::setCarbonsEnabled(bool enabled) { if (m_carbonsEnabled == enabled) @@ -73,6 +76,7 @@ void QXmppCarbonManager::setCarbonsEnabled(bool enabled) } } +/// \cond QStringList QXmppCarbonManager::discoveryFeatures() const { return QStringList() << ns_carbons; @@ -91,7 +95,7 @@ bool QXmppCarbonManager::handleStanza(const QDomElement &element) } if (carbon.isNull() || carbon.namespaceURI() != ns_carbons) - return false; // Neither sent nor received -> no carbon message + return false; // carbon copies must always come from our bare JID if (element.attribute("from") != client()->configuration().jidBare()) { @@ -99,16 +103,13 @@ bool QXmppCarbonManager::handleStanza(const QDomElement &element) return false; } - QDomElement forwarded = carbon.firstChildElement("forwarded"); - if (forwarded.isNull()) - return false; - - QDomElement messageelement = forwarded.firstChildElement("message"); - if (messageelement.isNull()) + auto forwarded = carbon.firstChildElement("forwarded"); + auto messageElement = forwarded.firstChildElement("message"); + if (messageElement.isNull()) return false; QXmppMessage message; - message.parse(messageelement); + message.parse(messageElement); if (sent) emit messageSent(message); @@ -117,3 +118,20 @@ bool QXmppCarbonManager::handleStanza(const QDomElement &element) return true; } +/// \endcond + +/// +/// \fn QXmppCarbonManager::messageReceived() +/// +/// Emitted when a message was received from someone else and directed to +/// another resource. +/// +/// If you connect this signal to the QXmppClient::messageReceived signal, they +/// will appear as normal messages. +/// + +/// +/// \fn QXmppCarbonManager::messageSent() +/// +/// Emitted when another resource sent a message to someone else. +/// diff --git a/src/client/QXmppCarbonManager.h b/src/client/QXmppCarbonManager.h index d1e113be..33a39627 100644 --- a/src/client/QXmppCarbonManager.h +++ b/src/client/QXmppCarbonManager.h @@ -56,14 +56,7 @@ public: /// \endcond Q_SIGNALS: - /// \brief Emitted when a message was received from someone else - /// and directed to another resource. - /// If you connect this signal to the \s QXmppClient::messageReceived - /// signal, they will appear as normal messages. void messageReceived(const QXmppMessage &); - - /// \brief Emitted when another resource sent a message to - /// someone else void messageSent(const QXmppMessage &); private: -- cgit v1.2.3