Handle fragmented responses on SOCKS5 control channel

Server responses may arrive in more than one packet, though this
is rare due to nagle algorithm.
Also fixed IPv6 addresses being discarded from server responses,
which was caught by the new autotest.

Task-number: QTBUG-18564

Change-Id: I32d9e2978037fb3e1fff27b7e618b5da6d222f28
Reviewed-by: Martin Petersson <Martin.Petersson@nokia.com>

diff --git a/src/network/socket/qsocks5socketengine.cpp b/src/network/socket/qsocks5socketengine.cpp
index 76b00bc..48a866c 100644 (file)
--- a/src/network/socket/qsocks5socketengine.cpp
+++ b/src/network/socket/qsocks5socketengine.cpp
@@ -213,11 +213,12 @@ static bool qt_socks5_set_host_name_and_port(const QString &hostname, quint16 po
 
 /*
    retrives the host address in buf at pos and updates pos.
+   return 1 if OK, 0 if need more data, -1 if error
    if the func fails the value of the address and the pos is undefined
 */
-static bool qt_socks5_get_host_address_and_port(const QByteArray &buf, QHostAddress *pAddress, quint16 *pPort, int *pPos)
+static int qt_socks5_get_host_address_and_port(const QByteArray &buf, QHostAddress *pAddress, quint16 *pPort, int *pPos)
 {
-    bool ret = false;
+    int ret = -1;
     int pos = *pPos;
     const unsigned char *pBuf = reinterpret_cast<const unsigned char*>(buf.constData());
     QHostAddress address;
@@ -225,27 +226,28 @@ static bool qt_socks5_get_host_address_and_port(const QByteArray &buf, QHostAddr
 
     if (buf.size() - pos < 1) {
         QSOCKS5_DEBUG << "need more data address/port";
-        return false;
+        return 0;
     }
     if (pBuf[pos] == S5_IP_V4) {
         pos++;
         if (buf.size() - pos < 4) {
             QSOCKS5_DEBUG << "need more data for ip4 address";
-            return false;
+            return 0;
         }
         address.setAddress(qFromBigEndian<quint32>(&pBuf[pos]));
         pos += 4;
-        ret = true;
+        ret = 1;
     } else if (pBuf[pos] == S5_IP_V6) {
         pos++;
         if (buf.size() - pos < 16) {
             QSOCKS5_DEBUG << "need more data for ip6 address";
-            return false;
+            return 0;
         }
         QIPv6Address add;
         for (int i = 0; i < 16; ++i)
             add[i] = buf[pos++];
-        ret = true;
+        address.setAddress(add);
+        ret = 1;
     } else if (pBuf[pos] == S5_DOMAINNAME){
         // just skip it
         pos++;
@@ -253,19 +255,19 @@ static bool qt_socks5_get_host_address_and_port(const QByteArray &buf, QHostAddr
         pos += uchar(pBuf[pos]);
     } else {
         QSOCKS5_DEBUG << "invalid address type" << (int)pBuf[pos];
-        ret = false;
+        ret = -1;
     }
 
-    if (ret) {
+    if (ret == 1) {
         if (buf.size() - pos < 2) {
             QSOCKS5_DEBUG << "need more data for port";
-            return false;
+            return 0;
         }
         port = qFromBigEndian<quint16>(&pBuf[pos]);
         pos += 2;
     }
 
-    if (ret) {
+    if (ret == 1) {
         QSOCKS5_DEBUG << "got [" << address << ':' << port << ']';
         *pAddress = address;
         *pPort = port;
@@ -848,16 +850,20 @@ void QSocks5SocketEnginePrivate::parseRequestMethodReply()
         QSOCKS5_DEBUG << "unSeal failed, needs more data";
         return;
     }
+
+    inBuf.prepend(receivedHeaderFragment);
+    receivedHeaderFragment.clear();
     QSOCKS5_DEBUG << dump(inBuf);
-    if (inBuf.size() < 2) {
+    if (inBuf.size() < 3) {
         QSOCKS5_DEBUG << "need more data for request reply header .. put this data somewhere";
+        receivedHeaderFragment = inBuf;
         return;
     }
 
     QHostAddress address;
     quint16 port = 0;
 
-    if (inBuf.at(0) != S5_VERSION_5 || inBuf.length() < 3 || inBuf.at(2) != 0x00) {
+    if (inBuf.at(0) != S5_VERSION_5 || inBuf.at(2) != 0x00) {
         QSOCKS5_DEBUG << "socks protocol error";
         setErrorState(SocksError);
     } else if (inBuf.at(1) != S5_SUCCESS) {
@@ -873,9 +879,14 @@ void QSocks5SocketEnginePrivate::parseRequestMethodReply()
     } else {
         // connection success, retrieve the remote addresses
         int pos = 3;
-        if (!qt_socks5_get_host_address_and_port(inBuf, &address, &port, &pos)) {
+        int err = qt_socks5_get_host_address_and_port(inBuf, &address, &port, &pos);
+        if (err == -1) {
             QSOCKS5_DEBUG << "error getting address";
             setErrorState(SocksError);
+        } else if (err == 0) {
+            //need more data
+            receivedHeaderFragment = inBuf;
+            return;
         } else {
             inBuf.remove(0, pos);
             for (int i = inBuf.size() - 1; i >= 0 ; --i)
@@ -1310,7 +1321,7 @@ void QSocks5SocketEnginePrivate::_q_udpSocketReadNotification()
             QSOCKS5_D_DEBUG << "don't support fragmentation yet disgarding";
             return;
         }
-        if (!qt_socks5_get_host_address_and_port(inBuf, &datagram.address, &datagram.port, &pos)) {
+        if (qt_socks5_get_host_address_and_port(inBuf, &datagram.address, &datagram.port, &pos) != 1) {
             QSOCKS5_D_DEBUG << "failed to get address from datagram disgarding";
             return;
         }

diff --git a/src/network/socket/qsocks5socketengine_p.h b/src/network/socket/qsocks5socketengine_p.h
index 662ce0d..01b9d1e 100644 (file)
--- a/src/network/socket/qsocks5socketengine_p.h
+++ b/src/network/socket/qsocks5socketengine_p.h
@@ -270,6 +270,7 @@ public:
 #endif
     QSocks5BindData *bindData;
     QString peerName;
+    QByteArray receivedHeaderFragment;
 
     mutable bool readNotificationActivated;
     mutable bool writeNotificationActivated;

diff --git a/tests/auto/network/socket/qsocks5socketengine/tst_qsocks5socketengine.cpp b/tests/auto/network/socket/qsocks5socketengine/tst_qsocks5socketengine.cpp
index 3cc1f32..461ec55 100644 (file)
--- a/tests/auto/network/socket/qsocks5socketengine/tst_qsocks5socketengine.cpp
+++ b/tests/auto/network/socket/qsocks5socketengine/tst_qsocks5socketengine.cpp
@@ -47,6 +47,7 @@
 #include <QtCore/QString>
 #include <QtCore/QCoreApplication>
 #include <QtCore/QMetaType>
+#include <QtCore/QTimer>
 
 #include <private/qsocks5socketengine_p.h>
 #include <qhostinfo.h>
@@ -89,6 +90,10 @@ private slots:
    // void tcpLoopbackPerformance();
     void passwordAuth();
     void passwordAuth2();
+    void fragmentation_data();
+    void fragmentation();
+    void incomplete_data();
+    void incomplete();
 
 protected slots:
     void tcpSocketNonBlocking_hostFound();
@@ -112,40 +117,55 @@ private:
     qint64 bytesAvailable;
 };
 
-class MiniSocks5Server: public QTcpServer
+class MiniSocks5ResponseHandler : public QObject
 {
     Q_OBJECT
 public:
     QQueue<QByteArray> responses;
+    QTcpSocket *client;
 
-    MiniSocks5Server(const QQueue<QByteArray> r)
-        : responses(r)
+    MiniSocks5ResponseHandler(QQueue<QByteArray> r, QTcpSocket *c, int autoResponseTime)
+        : responses(r), client(c)
     {
-        listen();
-        connect(this, SIGNAL(newConnection()), SLOT(handleNewConnection()));
+        client->setParent(this);
+        connect(client, SIGNAL(disconnected()), SLOT(deleteLater()));
+        connect(client, SIGNAL(readyRead()), SLOT(sendNextResponse()));
+        if (autoResponseTime)
+            QTimer::singleShot(autoResponseTime, this, SLOT(sendNextResponse()));
     }
 
 private slots:
-    void handleNewConnection()
-    {
-        QTcpSocket *client = nextPendingConnection();
-        connect(client, SIGNAL(readyRead()), SLOT(handleClientCommand()));
-        client->setProperty("pendingResponses", QVariant::fromValue(responses));
-    }
-
-    void handleClientCommand()
+    void sendNextResponse()
     {
         // WARNING
         // this assumes that the client command is received in its entirety
         // should be ok, since SOCKSv5 commands are rather small
-        QTcpSocket *client = static_cast<QTcpSocket *>(sender());
-        QQueue<QByteArray> pendingResponses =
-            qvariant_cast<QQueue<QByteArray> >(client->property("pendingResponses"));
-        if (pendingResponses.isEmpty())
+        if (responses.isEmpty())
             client->disconnectFromHost();
         else
-            client->write(pendingResponses.dequeue());
-        client->setProperty("pendingResponses", QVariant::fromValue(pendingResponses));
+            client->write(responses.dequeue());
+    }
+};
+
+class MiniSocks5Server: public QTcpServer
+{
+    Q_OBJECT
+public:
+    QQueue<QByteArray> responses;
+    int autoResponseTime;
+
+    MiniSocks5Server(const QQueue<QByteArray> r, int t = 0)
+        : responses(r), autoResponseTime(t)
+    {
+        listen();
+        connect(this, SIGNAL(newConnection()), SLOT(handleNewConnection()));
+    }
+
+private slots:
+    void handleNewConnection()
+    {
+        QTcpSocket *client = nextPendingConnection();
+        new MiniSocks5ResponseHandler(responses, client, autoResponseTime);
     }
 };
 
@@ -966,6 +986,105 @@ void tst_QSocks5SocketEngine::passwordAuth2()
     QVERIFY(socketDevice.state() == QAbstractSocket::UnconnectedState);
 }
 
+void tst_QSocks5SocketEngine::fragmentation_data()
+{
+    QTest::addColumn<QQueue<QByteArray> >("responses");
+
+    QByteArray authMethodNone = QByteArray::fromRawData("\5\0", 2);
+    QByteArray authMethodBasic = QByteArray::fromRawData("\5\2", 2);
+    QByteArray authSuccess = QByteArray::fromRawData("\1\0", 2);
+    QByteArray connectResponseIPv4 = QByteArray::fromRawData("\5\0\0\1\1\2\3\4\5\6", 10);
+    QByteArray connectResponseIPv6 = QByteArray::fromRawData("\5\0\0\4\x01\x23\x45\x67\x89\xab\xcd\xef\x01\x23\x45\x67\x89\xab\xcd\xef\5\6", 22);
+
+    QQueue<QByteArray> responses;
+    responses << authMethodNone.left(1) << authMethodNone.mid(1) << connectResponseIPv4;
+    QTest::newRow("auth-method") << responses;
+
+    responses.clear();
+    responses << authMethodBasic << authSuccess.left(1) << authSuccess.mid(1) << connectResponseIPv4;
+    QTest::newRow("auth-response") << responses;
+
+    for (int i = 1; i < connectResponseIPv4.length() - 1; i++) {
+        responses.clear();
+        responses << authMethodNone << connectResponseIPv4.left(i) << connectResponseIPv4.mid(i);
+        QTest::newRow(qPrintable(QString("connect-response-ipv4-") + QString::number(i))) << responses;
+    }
+
+    for (int i = 1; i < connectResponseIPv6.length() - 1; i++) {
+        responses.clear();
+        responses << authMethodNone << connectResponseIPv6.left(i) << connectResponseIPv6.mid(i);
+        QTest::newRow(qPrintable(QString("connect-response-ipv6-") + QString::number(i))) << responses;
+    }
+}
+
+void tst_QSocks5SocketEngine::fragmentation()
+{
+    QFETCH(QQueue<QByteArray>, responses);
+    MiniSocks5Server server(responses, 500);
+
+    QTcpSocket socket;
+    socket.setProxy(QNetworkProxy(QNetworkProxy::Socks5Proxy, "localhost", server.serverPort(), "user", "password"));
+    socket.connectToHost("0.1.2.3", 12345);
+
+    connect(&socket, SIGNAL(connected()),
+            &QTestEventLoop::instance(), SLOT(exitLoop()));
+    QTestEventLoop::instance().enterLoop(10);
+    QVERIFY(!QTestEventLoop::instance().timeout());
+
+    QVERIFY(socket.localAddress() == QHostAddress("1.2.3.4") || socket.localAddress() == QHostAddress("0123:4567:89ab:cdef:0123:4567:89ab:cdef"));
+    QVERIFY(socket.localPort() == 0x0506);
+}
+
+void tst_QSocks5SocketEngine::incomplete_data()
+{
+    QTest::addColumn<QQueue<QByteArray> >("responses");
+
+    QByteArray authMethodNone = QByteArray::fromRawData("\5\0", 2);
+    QByteArray authMethodBasic = QByteArray::fromRawData("\5\2", 2);
+    QByteArray authSuccess = QByteArray::fromRawData("\1\0", 2);
+    QByteArray connectResponseIPv4 = QByteArray::fromRawData("\5\0\0\1\1\2\3\4\5\6", 10);
+    QByteArray connectResponseIPv6 = QByteArray::fromRawData("\5\0\0\4\x01\x23\x45\x67\x89\xab\xcd\xef\x01\x23\x45\x67\x89\xab\xcd\xef\5\6", 22);
+
+    QQueue<QByteArray> responses;
+    responses << authMethodNone.left(1);
+    QTest::newRow("auth-method") << responses;
+
+    responses.clear();
+    responses << authMethodBasic << authSuccess.left(1);
+    QTest::newRow("auth-response") << responses;
+
+    for (int i = 1; i < connectResponseIPv4.length() - 1; i++) {
+        responses.clear();
+        responses << authMethodNone << connectResponseIPv4.left(i);
+        QTest::newRow(qPrintable(QString("connect-response-ipv4-") + QString::number(i))) << responses;
+    }
+
+    for (int i = 1; i < connectResponseIPv6.length() - 1; i++) {
+        responses.clear();
+        responses << authMethodNone << connectResponseIPv6.left(i);
+        QTest::newRow(qPrintable(QString("connect-response-ipv6-") + QString::number(i))) << responses;
+    }
+}
+
+void tst_QSocks5SocketEngine::incomplete()
+{
+    QFETCH(QQueue<QByteArray>, responses);
+    MiniSocks5Server server(responses, 500);
+
+    QTcpSocket socket;
+    socket.setProxy(QNetworkProxy(QNetworkProxy::Socks5Proxy, "127.0.0.1", server.serverPort(), "user", "password"));
+    socket.connectToHost("0.1.2.3", 12345);
+
+    connect(&socket, SIGNAL(connected()),
+            &QTestEventLoop::instance(), SLOT(exitLoop()));
+    connect(&socket, SIGNAL(error(QAbstractSocket::SocketError)),
+            &QTestEventLoop::instance(), SLOT(exitLoop()));
+    QTestEventLoop::instance().enterLoop(70);
+    QVERIFY(!QTestEventLoop::instance().timeout());
+
+    QCOMPARE(socket.error(), QAbstractSocket::ProxyConnectionClosedError);
+}
+
 //----------------------------------------------------------------------------------
 
 QTEST_MAIN(tst_QSocks5SocketEngine)