Improve parsing of GET argument to avoid misinterpreting queries

Fixes issue #247

docs/CHANGES_2_1.txt

@@ -11,6 +11,7 @@ Client-side:
 Server-side:
 ============
 * Disable HTTP/2 support (which Qt 6 enables by default), it causes trouble with some SOAP servers (issue #246).
+* Improve parsing of GET argument to avoid misinterpreting queries (possible security issue #247).
 
 WSDL parser / code generator changes, applying to both client and server side:
 ================================================================

src/KDSoapServer/KDSoapServerSocket.cpp

@@ -32,6 +32,8 @@
 #include <QFileInfo>
 #include <QVarLengthArray>
 
+static const char s_forbidden[] = "HTTP/1.1 403 Forbidden\r\nContent-Length: 0\r\n\r\n";
+
 KDSoapServerSocket::KDSoapServerSocket(KDSoapSocketList *owner, QObject *serverObject)
 #ifndef QT_NO_SSL
     : QSslSocket()
@@ -74,10 +76,20 @@ static HeadersMap parseHeaders(const QByteArray &headerData)
         return headersMap;
     }
     const QByteArray &requestType = firstLine.at(0);
-    const QByteArray path = QDir::cleanPath(QString::fromLatin1(firstLine.at(1).constData())).toLatin1();
-    const QByteArray &httpVersion = firstLine.at(2);
     headersMap.insert("_requestType", requestType);
-    headersMap.insert("_path", path);
+
+    // Grammar from https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.1
+    //  origin-form    = absolute-path [ "?" query ]
+    // and https://datatracker.ietf.org/doc/html/rfc3986#section-3.3
+    // says the path ends at the first '?' or '#' character
+    const QByteArray arg1 = firstLine.at(1);
+    const int queryPos = arg1.indexOf('?');
+    const QByteArray path = queryPos >= 0 ? arg1.left(queryPos) : arg1;
+    // Unfortunately QDir::cleanPath works with QString
+    const QByteArray cleanedPath = QDir::cleanPath(QString::fromUtf8(path)).toUtf8();
+    headersMap.insert("_path", cleanedPath);
+
+    const QByteArray &httpVersion = firstLine.at(2);
     headersMap.insert("_httpVersion", httpVersion);
 
     while (!sourceBuffer.atEnd()) {
@@ -275,6 +287,12 @@ void KDSoapServerSocket::handleRequest(const QMap<QByteArray, QByteArray> &httpH
     const QByteArray requestType = httpHeaders.value("_requestType");
     const QString path = QString::fromLatin1(httpHeaders.value("_path").constData());
 
+    if (!path.startsWith(QLatin1String("/"))) {
+        // denied for security reasons (ex: path starting with "..")
+        write(s_forbidden);
+        return;
+    }
+
     KDSoapServerAuthInterface *serverAuthInterface = qobject_cast<KDSoapServerAuthInterface *>(m_serverObject);
     if (serverAuthInterface) {
         const QByteArray authValue = httpHeaders.value("authorization");
@@ -404,8 +422,7 @@ bool KDSoapServerSocket::handleFileDownload(KDSoapServerObjectInterface *serverO
         return true;
     }
     if (!device->open(QIODevice::ReadOnly)) {
-        const QByteArray forbidden = "HTTP/1.1 403 Forbidden\r\nContent-Length: 0\r\n\r\n";
-        write(forbidden);
+        write(s_forbidden);
         delete device;
         return true; // handled!
     }

unittests/serverlib/test_serverlib.cpp

@@ -107,6 +107,7 @@ class CountryServerObject : public QObject,
 
     virtual QIODevice *processFileRequest(const QString &path, QByteArray &contentType) override
     {
+        Q_ASSERT(!path.startsWith(".."));
         if (path == QLatin1String("/path/to/file_download.txt")) {
             QFile *file = new QFile(QLatin1String("file_download.txt")); // local file, created by the unittest
             contentType = "text/plain";
@@ -917,26 +918,33 @@ private Q_SLOTS:
     {
         QTest::addColumn<QString>("fileToDownload"); // client
         QTest::addColumn<QFile::Permissions>("permissions"); // server
-        QTest::addColumn<QNetworkReply::NetworkError>("expectedReplyCode");
-
-        QFile::Permissions readable = QFile::ReadOwner | QFile::ReadUser;
-        QFile::Permissions writable = QFile::WriteOwner | QFile::WriteUser;
-
-        QTest::newRow("readable") << "file_download.txt" << readable << QNetworkReply::NoError;
-        QTest::newRow("nonexistent") << "nonexistent.txt" << readable << QNetworkReply::ContentNotFoundError;
-        QTest::newRow("unreadable") << "file_download.txt" << writable
-#if QT_VERSION >= QT_VERSION_CHECK(5, 9, 0) // this changed in qtbase 079aa711ec
-                                    << QNetworkReply::ContentAccessDenied;
-#else
-                                    << QNetworkReply::ContentOperationNotPermittedError;
-#endif
+        QTest::addColumn<QByteArray>("expectedHttpReply");
+
+        const QFile::Permissions readable = QFile::ReadOwner | QFile::ReadUser;
+        const QFile::Permissions writable = QFile::WriteOwner | QFile::WriteUser;
+
+        const QByteArray httpOK = "200 OK";
+        const QByteArray httpForbidden = "403 Forbidden";
+        const QByteArray httpNotFound = "404 Not Found";
+
+        QTest::newRow("readable") << "/path/to/file_download.txt" << readable << httpOK;
+        QTest::newRow("nonexistent") << "/nonexistent.txt" << readable << httpNotFound;
+        QTest::newRow("unreadable") << "/path/to/file_download.txt" << writable << httpForbidden;
+        QTest::newRow("dot_dot_in_middle") << "/subdir/../other/../path/to/file_download.txt" << readable << httpOK;
+        QTest::newRow("double_slash") << "/subdir/../other//../path//to/file_download.txt" << readable << httpOK;
+        QTest::newRow("dot_dot_at_start") << "../../path/to/file_download.txt" << readable << httpForbidden;
+        QTest::newRow("with_query") << "/?query=../../path/to/file_download.txt" << readable << httpNotFound; // "GET /"
+        QTest::newRow("another_query") << "?query=/../path/to/file_download.txt" << readable << httpForbidden;
+        QTest::newRow("query_is_ignored") << "/path/to/file_download.txt?a=b&c=d" << readable << httpOK;
+        QTest::newRow("with_ref") << "#/../../../path/to/file_download.txt" << readable << httpForbidden;
+        QTest::newRow("invalid") << "#/path/to/file_download.txt" << readable << httpForbidden;
     }
 
     void testFileDownload()
     {
         QFETCH(QString, fileToDownload);
         QFETCH(QFile::Permissions, permissions);
-        QFETCH(QNetworkReply::NetworkError, expectedReplyCode);
+        QFETCH(QByteArray, expectedHttpReply);
 
         QTimer download_timeout;
         download_timeout.setInterval(5000); // 5 seconds
@@ -949,38 +957,41 @@ private Q_SLOTS:
 
         const QString fileName = QString::fromLatin1("file_download.txt");
         QFile file(fileName);
-        QVERIFY(file.open(QIODevice::WriteOnly));
+        QVERIFY2(file.open(QIODevice::WriteOnly), qPrintable(file.errorString()));
         file.write("Hello world");
         file.flush();
         file.setPermissions(permissions);
-        QString pathInUrl = QString::fromLatin1("/path/to/") + fileToDownload;
-        QString url = server->endPoint();
-        url.chop(1) /*trailing slash*/;
-        url += pathInUrl;
 
-        QNetworkAccessManager manager;
-        QNetworkRequest request(QUrl { url });
-        QNetworkReply *reply = manager.get(request);
-        QEventLoop loop;
-        connect(&download_timeout, &QTimer::timeout, &loop, &QEventLoop::quit);
-        connect(reply, &QNetworkReply::finished, &loop, &QEventLoop::quit);
-        download_timeout.start();
-        loop.exec();
+        ClientSocket socket(server);
+        QVERIFY(socket.waitForConnected());
+        const QByteArray request = "GET " + fileToDownload.toLatin1() + " HTTP/1.1\r\n"
+                                   "Content-Type: text/xml;charset=utf-8\r\n"
+                                   "Content-Length: 0\r\n"
+                                   "Host: 127.0.0.1:12345\r\n" // ignored
+                                   "\r\n";
+        socket.write(request);
+        QVERIFY(socket.waitForBytesWritten(3000));
+        QVERIFY(socket.bytesAvailable() || socket.waitForReadyRead(3000));
+        const QByteArray reply = socket.readAll();
 
         file.setPermissions(QFile::ReadOwner | QFile::ReadUser | QFile::WriteOwner | QFile::WriteUser);
         QFile::remove(fileName);
 
         QCOMPARE(timeout_spy.count(), 0);
 #if defined(Q_OS_WIN)
-        if (permissions & QFile::WriteOwner) {
+        if (!(permissions & QFile::ReadOwner)) {
             // on Windows, setting permissions to writeonly using QFile::setPermissions does not work
             // this has been confirmed in tst_qfile.cpp in the Qt unittests
             QEXPECT_FAIL("unreadable", "Windows does not currently support non-readable files.", Abort);
         }
 #endif
-        QCOMPARE(( int )reply->error(), ( int )expectedReplyCode);
-        if (expectedReplyCode == QNetworkReply::NoError) {
-            QCOMPARE(reply->readAll(), QByteArray("Hello world"));
+
+        const QByteArray firstLine = reply.left(reply.indexOf('\r'));
+        QCOMPARE(firstLine, "HTTP/1.1 " + expectedHttpReply);
+
+        if (expectedHttpReply.endsWith("OK")) {
+            const QByteArray lastLine = reply.mid(reply.lastIndexOf("\r\n") + 2);
+            QCOMPARE(lastLine, QByteArray("Hello world"));
         }
     }