From e20eaed5c1968e32eca97cf449fa588cfab35a5d Mon Sep 17 00:00:00 2001
From: Sami Rosendahl <ext-sami.1.rosendahl@nokia.com>
Date: Wed, 30 Nov 2011 20:36:56 +0100
Subject: Fix stack overwrite in QDBusDemarshaller

QDBusArgument extraction operators and QDBusDemarshaller that implements
the extraction do not check the type of the extracted value.
Helper function template qIterGet in qdbusdemarshaller.cpp that is used
for extracting basic data types only reserves space from the stack for
the expected type as specified by client.
If the actual type in the DBus parameter is larger stack will be
overwritten in the helper function by at most 7 bytes (expected one byte,
received dbus_uint_64_t of size 8 bytes).

The fix always reserves space for the largest basic type dbus_uint64_t
readable by dbus_message_iter_get_basic API.

See also http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga41c23a05e552d0574d0444d4693d18ab

PMO 280456
Task-number: QTBUG-22735
Merge-request: 1469
Reviewed-by: thiago
---
 src/dbus/qdbusdemarshaller.cpp                 | 24 +++++++-
 tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp | 82 ++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/src/dbus/qdbusdemarshaller.cpp b/src/dbus/qdbusdemarshaller.cpp
index d9bb5b5..4103552 100644
--- a/src/dbus/qdbusdemarshaller.cpp
+++ b/src/dbus/qdbusdemarshaller.cpp
@@ -48,10 +48,28 @@ QT_BEGIN_NAMESPACE
 template <typename T>
 static inline T qIterGet(DBusMessageIter *it)
 {
-    T t;
-    q_dbus_message_iter_get_basic(it, &t);
+    // Use a union of expected and largest type q_dbus_message_iter_get_basic
+    // will return to ensure reading the wrong basic type does not result in
+    // stack overwrite
+    union {
+        // The value to be extracted
+        T t;
+        // Largest type that q_dbus_message_iter_get_basic will return
+        // according to dbus_message_iter_get_basic API documentation
+        dbus_uint64_t maxValue;
+        // A pointer to ensure no stack overwrite in case there is a platform
+        // where sizeof(void*) > sizeof(dbus_uint64_t)
+        void* ptr;
+    } value;
+
+    // Initialize the value in case a narrower type is extracted to it.
+    // Note that the result of extracting a narrower type in place of a wider
+    // one and vice-versa will be platform-dependent.
+    value.t = T();
+
+    q_dbus_message_iter_get_basic(it, &value);
     q_dbus_message_iter_next(it);
-    return t;
+    return value.t;
 }
 
 QDBusDemarshaller::~QDBusDemarshaller()
diff --git a/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp b/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp
index cca212e..9754a84 100644
--- a/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp
+++ b/tests/auto/qdbusmarshall/tst_qdbusmarshall.cpp
@@ -93,6 +93,9 @@ private slots:
     void receiveUnknownType_data();
     void receiveUnknownType();
 
+    void demarshallPrimitives_data();
+    void demarshallPrimitives();
+
 private:
     int fileDescriptorForTest();
 
@@ -1168,5 +1171,84 @@ void tst_QDBusMarshall::receiveUnknownType()
 #endif
 }
 
+void tst_QDBusMarshall::demarshallPrimitives_data()
+{
+    sendBasic_data();
+}
+
+template<class T>
+QVariant demarshallPrimitiveAs(const QDBusArgument& dbusArg)
+{
+    T val;
+    dbusArg >> val;
+    return qVariantFromValue(val);
+}
+
+QVariant demarshallPrimitiveAs(int typeIndex, const QDBusArgument& dbusArg)
+{
+    switch (typeIndex) {
+    case 0:
+        return demarshallPrimitiveAs<uchar>(dbusArg);
+    case 1:
+        return demarshallPrimitiveAs<bool>(dbusArg);
+    case 2:
+        return demarshallPrimitiveAs<short>(dbusArg);
+    case 3:
+        return demarshallPrimitiveAs<ushort>(dbusArg);
+    case 4:
+        return demarshallPrimitiveAs<int>(dbusArg);
+    case 5:
+        return demarshallPrimitiveAs<uint>(dbusArg);
+    case 6:
+        return demarshallPrimitiveAs<qlonglong>(dbusArg);
+    case 7:
+        return demarshallPrimitiveAs<qulonglong>(dbusArg);
+    case 8:
+        return demarshallPrimitiveAs<double>(dbusArg);
+    default:
+        return QVariant();
+    }
+}
+
+void tst_QDBusMarshall::demarshallPrimitives()
+{
+    QFETCH(QVariant, value);
+    QFETCH(QString, sig);
+
+    QDBusConnection con = QDBusConnection::sessionBus();
+
+    QVERIFY(con.isConnected());
+
+    // Demarshall each test data value to all primitive types to test
+    // demarshalling to the wrong type does not cause a crash
+    for (int typeIndex = 0; true; ++typeIndex) {
+        QDBusMessage msg = QDBusMessage::createMethodCall(serviceName, objectPath,
+                                                          interfaceName, "ping");
+        QDBusArgument sendArg;
+        sendArg.beginStructure();
+        sendArg.appendVariant(value);
+        sendArg.endStructure();
+        msg.setArguments(QVariantList() << qVariantFromValue(sendArg));
+        QDBusMessage reply = con.call(msg);
+
+        const QDBusArgument receiveArg = qvariant_cast<QDBusArgument>(reply.arguments().at(0));
+        receiveArg.beginStructure();
+        QCOMPARE(receiveArg.currentSignature(), sig);
+
+        const QVariant receiveValue = demarshallPrimitiveAs(typeIndex, receiveArg);
+        if (receiveValue.type() == value.type()) {
+            // Value type is the same, compare the values
+            QCOMPARE(receiveValue, value);
+            QVERIFY(receiveArg.atEnd());
+        }
+
+        receiveArg.endStructure();
+        QVERIFY(receiveArg.atEnd());
+
+        if (!receiveValue.isValid())
+            break;
+    }
+}
+
 QTEST_MAIN(tst_QDBusMarshall)
 #include "tst_qdbusmarshall.moc"
-- 
cgit v0.12