Check on newline characters in origin and urls

New line characters (\r\n) in the resource part of a url and in the origin
string can be used to forge the http header and can lead to insertion of
unwanted header entries. This can be an indication of an attack,
so QWebSocket immediately refuses a connection.

Change-Id: I9cdb309bfbe7025ad675925e6ea3e038476a1fd6
Reviewed-by: Frederik Gladhorn <frederik.gladhorn@digia.com>

diff --git a/src/websockets/qwebsocket.cpp b/src/websockets/qwebsocket.cpp
index f2a2b6a..0a0c420 100644 (file)
--- a/src/websockets/qwebsocket.cpp
+++ b/src/websockets/qwebsocket.cpp
@@ -255,6 +255,8 @@ QT_BEGIN_NAMESPACE
  * The \a origin of the client is as specified \l {http://tools.ietf.org/html/rfc6454}{RFC 6454}.
  * (The \a origin is not required for non-web browser clients
  * (see \l {http://tools.ietf.org/html/rfc6455}{RFC 6455})).
+ * The \a origin may not contain new line characters, otherwise the connection will be
+ * aborted immediately during the handshake phase.
  * \note Currently only V13 (\l {http://tools.ietf.org/html/rfc6455} {RFC 6455}) is supported
  */
 QWebSocket::QWebSocket(const QString &origin,
@@ -373,6 +375,9 @@ void QWebSocket::close(QWebSocketProtocol::CloseCode closeCode, const QString &r
 
 /*!
     \brief Opens a websocket connection using the given \a url.
+
+    If the url contains newline characters (\\r\\n), then the error signal will be emitted
+    with QAbstractSocket::ConnectionRefusedError as error type.
  */
 void QWebSocket::open(const QUrl &url)
 {

diff --git a/src/websockets/qwebsocket_p.cpp b/src/websockets/qwebsocket_p.cpp
index 1c4baca..d54337f 100644 (file)
--- a/src/websockets/qwebsocket_p.cpp
+++ b/src/websockets/qwebsocket_p.cpp
@@ -330,8 +330,14 @@ void QWebSocketPrivate::close(QWebSocketProtocol::CloseCode closeCode, QString r
 void QWebSocketPrivate::open(const QUrl &url, bool mask)
 {
     //just delete the old socket for the moment;
-    //later, we can add more 'intelligent' handling by looking at the url
+    //later, we can add more 'intelligent' handling by looking at the URL
     //m_pSocket.reset();
+    Q_Q(QWebSocket);
+    if (!url.isValid() || url.toString().contains(QStringLiteral("\r\n"))) {
+        setErrorString(QWebSocket::tr("Invalid URL."));
+        Q_EMIT q->error(QAbstractSocket::ConnectionRefusedError);
+        return;
+    }
     QTcpSocket *pTcpSocket = m_pSocket.take();
     if (pTcpSocket) {
         releaseConnections(pTcpSocket);
@@ -339,14 +345,18 @@ void QWebSocketPrivate::open(const QUrl &url, bool mask)
     }
     //if (m_url != url)
     if (Q_LIKELY(!m_pSocket)) {
-        Q_Q(QWebSocket);
-
         m_dataProcessor.clear();
         m_isClosingHandshakeReceived = false;
         m_isClosingHandshakeSent = false;
 
         setRequestUrl(url);
         QString resourceName = url.path();
+        if (resourceName.contains(QStringLiteral("\r\n"))) {
+            setRequestUrl(QUrl());  //clear requestUrl
+            setErrorString(QWebSocket::tr("Invalid resource name."));
+            Q_EMIT q->error(QAbstractSocket::ConnectionRefusedError);
+            return;
+        }
         if (!url.query().isEmpty()) {
             if (!resourceName.endsWith(QChar::fromLatin1('?'))) {
                 resourceName.append(QChar::fromLatin1('?'));
@@ -973,6 +983,11 @@ void QWebSocketPrivate::processStateChanged(QAbstractSocket::SocketState socketS
                                            QString(),
                                            QString(),
                                            m_key);
+            if (handshake.isEmpty()) {
+                m_pSocket->abort();
+                Q_EMIT q->error(QAbstractSocket::ConnectionRefusedError);
+                return;
+            }
             m_pSocket->write(handshake.toLatin1());
         }
         break;
@@ -1062,6 +1077,31 @@ QString QWebSocketPrivate::createHandShakeRequest(QString resourceName,
                                                   QByteArray key)
 {
     QStringList handshakeRequest;
+    if (resourceName.contains(QStringLiteral("\r\n"))) {
+        setErrorString(QWebSocket::tr("The resource name contains newlines. " \
+                                      "Possible attack detected."));
+        return QString();
+    }
+    if (host.contains(QStringLiteral("\r\n"))) {
+        setErrorString(QWebSocket::tr("The hostname contains newlines. " \
+                                      "Possible attack detected."));
+        return QString();
+    }
+    if (origin.contains(QStringLiteral("\r\n"))) {
+        setErrorString(QWebSocket::tr("The origin contains newlines. " \
+                                      "Possible attack detected."));
+        return QString();
+    }
+    if (extensions.contains(QStringLiteral("\r\n"))) {
+        setErrorString(QWebSocket::tr("The extensions attribute contains newlines. " \
+                                      "Possible attack detected."));
+        return QString();
+    }
+    if (protocols.contains(QStringLiteral("\r\n"))) {
+        setErrorString(QWebSocket::tr("The protocols attribute contains newlines. " \
+                                      "Possible attack detected."));
+        return QString();
+    }
 
     handshakeRequest << QStringLiteral("GET ") % resourceName % QStringLiteral(" HTTP/1.1") <<
                         QStringLiteral("Host: ") % host <<

diff --git a/src/websockets/qwebsockethandshakeresponse.cpp b/src/websockets/qwebsockethandshakeresponse.cpp
index 37c0636..fd2ccd5 100644 (file)
--- a/src/websockets/qwebsockethandshakeresponse.cpp
+++ b/src/websockets/qwebsockethandshakeresponse.cpp
@@ -177,19 +177,27 @@ QString QWebSocketHandshakeResponse::getHandshakeResponse(
                     response << QStringLiteral("Sec-WebSocket-Extensions: ") % m_acceptedExtension;
                 }
                 QString origin = request.origin().trimmed();
-                if (origin.isEmpty())
-                    origin = QStringLiteral("*");
-                response << QStringLiteral("Server: ") % serverName                      <<
-                            QStringLiteral("Access-Control-Allow-Credentials: false")    <<
-                            QStringLiteral("Access-Control-Allow-Methods: GET")          <<
-                            QStringLiteral("Access-Control-Allow-Headers: content-type") <<
-                            QStringLiteral("Access-Control-Allow-Origin: ") % origin     <<
-                            QStringLiteral("Date: ") %
-                                QDateTime::currentDateTimeUtc()
-                                    .toString(QStringLiteral("ddd, dd MMM yyyy hh:mm:ss 'GMT'"));
+                if (origin.contains(QStringLiteral("\r\n")) ||
+                        serverName.contains(QStringLiteral("\r\n"))) {
+                    m_error = QWebSocketProtocol::CloseCodeAbnormalDisconnection;
+                    m_errorString = tr("One of the headers contains a newline. " \
+                                       "Possible attack detected.");
+                    m_canUpgrade = false;
+                } else {
+                    if (origin.isEmpty())
+                        origin = QStringLiteral("*");
+                    response << QStringLiteral("Server: ") % serverName                      <<
+                                QStringLiteral("Access-Control-Allow-Credentials: false")    <<
+                                QStringLiteral("Access-Control-Allow-Methods: GET")          <<
+                                QStringLiteral("Access-Control-Allow-Headers: content-type") <<
+                                QStringLiteral("Access-Control-Allow-Origin: ") % origin     <<
+                                QStringLiteral("Date: ") %
+                                    QDateTime::currentDateTimeUtc()
+                                        .toString(QStringLiteral("ddd, dd MMM yyyy hh:mm:ss 'GMT'"));
 
-                m_acceptedVersion = QWebSocketProtocol::currentVersion();
-                m_canUpgrade = true;
+                    m_acceptedVersion = QWebSocketProtocol::currentVersion();
+                    m_canUpgrade = true;
+                }
             }
         } else {
             m_error = QWebSocketProtocol::CloseCodeProtocolError;

diff --git a/tests/auto/qwebsocket/tst_qwebsocket.cpp b/tests/auto/qwebsocket/tst_qwebsocket.cpp
index 2273d9e..51cfc08 100644 (file)
--- a/tests/auto/qwebsocket/tst_qwebsocket.cpp
+++ b/tests/auto/qwebsocket/tst_qwebsocket.cpp
@@ -61,7 +61,9 @@ private Q_SLOTS:
     void tst_initialisation_data();
     void tst_initialisation();
     void tst_settersAndGetters();
+    void tst_invalidOpen_data();
     void tst_invalidOpen();
+    void tst_invalidOrigin();
 };
 
 tst_QWebSocket::tst_QWebSocket()
@@ -155,8 +157,44 @@ void tst_QWebSocket::tst_settersAndGetters()
     QCOMPARE(socket.readBufferSize(), -1);
 }
 
+void tst_QWebSocket::tst_invalidOpen_data()
+{
+    QTest::addColumn<QString>("url");
+    QTest::addColumn<QString>("expectedUrl");
+    QTest::addColumn<QString>("expectedPeerName");
+    QTest::addColumn<QString>("expectedResourceName");
+    QTest::addColumn<QAbstractSocket::SocketState>("stateAfterOpenCall");
+    QTest::addColumn<int>("disconnectedCount");
+    QTest::addColumn<int>("stateChangedCount");
+
+    QTest::newRow("Illegal local address")
+            << QStringLiteral("ws://127.0.0.1:1/") << QStringLiteral("ws://127.0.0.1:1/")
+            << QStringLiteral("127.0.0.1")
+            << QStringLiteral("/") << QAbstractSocket::ConnectingState
+            << 1
+            << 2;  //going from connecting to disconnected
+    QTest::newRow("URL containing new line in the hostname")
+            << QStringLiteral("ws://myhacky\r\nserver/") << QString()
+            << QString()
+            << QString() << QAbstractSocket::UnconnectedState
+            << 0 << 0;
+    QTest::newRow("URL containing new line in the resource name")
+            << QStringLiteral("ws://127.0.0.1:1/tricky\r\npath") << QString()
+            << QString()
+            << QString()
+            << QAbstractSocket::UnconnectedState
+            << 0 << 0;
+}
+
 void tst_QWebSocket::tst_invalidOpen()
 {
+    QFETCH(QString, url);
+    QFETCH(QString, expectedUrl);
+    QFETCH(QString, expectedPeerName);
+    QFETCH(QString, expectedResourceName);
+    QFETCH(QAbstractSocket::SocketState, stateAfterOpenCall);
+    QFETCH(int, disconnectedCount);
+    QFETCH(int, stateChangedCount);
     QWebSocket socket;
     QSignalSpy errorSpy(&socket, SIGNAL(error(QAbstractSocket::SocketError)));
     QSignalSpy aboutToCloseSpy(&socket, SIGNAL(aboutToClose()));
@@ -171,7 +209,7 @@ void tst_QWebSocket::tst_invalidOpen()
     QSignalSpy pongSpy(&socket, SIGNAL(pong(quint64,QByteArray)));
     QSignalSpy bytesWrittenSpy(&socket, SIGNAL(bytesWritten(qint64)));
 
-    socket.open(QUrl(QStringLiteral("ws://127.0.0.1:1/")));
+    socket.open(QUrl(url));
 
     QVERIFY(socket.origin().isEmpty());
     QCOMPARE(socket.version(), QWebSocketProtocol::VersionLatest);
@@ -185,18 +223,82 @@ void tst_QWebSocket::tst_invalidOpen()
     QCOMPARE(socket.pauseMode(), QAbstractSocket::PauseNever);
     QVERIFY(socket.peerAddress().isNull());
     QCOMPARE(socket.peerPort(), quint16(0));
+    QCOMPARE(socket.peerName(), expectedPeerName);
+    QCOMPARE(socket.state(), stateAfterOpenCall);
+    QCOMPARE(socket.readBufferSize(), 0);
+    QCOMPARE(socket.resourceName(), expectedResourceName);
+    QCOMPARE(socket.requestUrl().toString(), expectedUrl);
+    QCOMPARE(socket.closeCode(), QWebSocketProtocol::CloseCodeNormal);
+    QVERIFY(socket.closeReason().isEmpty());
+    QCOMPARE(socket.sendTextMessage(QStringLiteral("A text message")), 0);
+    QCOMPARE(socket.sendBinaryMessage(QByteArrayLiteral("A text message")), 0);
+
+    if (errorSpy.count() == 0)
+        QVERIFY(errorSpy.wait());
+    QCOMPARE(errorSpy.count(), 1);
+    QList<QVariant> arguments = errorSpy.takeFirst();
+    QAbstractSocket::SocketError socketError =
+            qvariant_cast<QAbstractSocket::SocketError>(arguments.at(0));
+    QCOMPARE(socketError, QAbstractSocket::ConnectionRefusedError);
+    QCOMPARE(aboutToCloseSpy.count(), 0);
+    QCOMPARE(connectedSpy.count(), 0);
+    QCOMPARE(disconnectedSpy.count(), disconnectedCount);
+    QCOMPARE(stateChangedSpy.count(), stateChangedCount);
+    if (stateChangedCount == 2) {
+        arguments = stateChangedSpy.takeFirst();
+        QAbstractSocket::SocketState socketState =
+                qvariant_cast<QAbstractSocket::SocketState>(arguments.at(0));
+        arguments = stateChangedSpy.takeFirst();
+        socketState = qvariant_cast<QAbstractSocket::SocketState>(arguments.at(0));
+        QCOMPARE(socketState, QAbstractSocket::UnconnectedState);
+    }
+    QCOMPARE(readChannelFinishedSpy.count(), 0);
+    QCOMPARE(textFrameReceivedSpy.count(), 0);
+    QCOMPARE(binaryFrameReceivedSpy.count(), 0);
+    QCOMPARE(textMessageReceivedSpy.count(), 0);
+    QCOMPARE(binaryMessageReceivedSpy.count(), 0);
+    QCOMPARE(pongSpy.count(), 0);
+    QCOMPARE(bytesWrittenSpy.count(), 0);
+}
+
+void tst_QWebSocket::tst_invalidOrigin()
+{
+    QWebSocket socket(QStringLiteral("My server\r\nin the wild."));
+
+    QSignalSpy errorSpy(&socket, SIGNAL(error(QAbstractSocket::SocketError)));
+    QSignalSpy aboutToCloseSpy(&socket, SIGNAL(aboutToClose()));
+    QSignalSpy connectedSpy(&socket, SIGNAL(connected()));
+    QSignalSpy disconnectedSpy(&socket, SIGNAL(disconnected()));
+    QSignalSpy stateChangedSpy(&socket, SIGNAL(stateChanged(QAbstractSocket::SocketState)));
+    QSignalSpy readChannelFinishedSpy(&socket, SIGNAL(readChannelFinished()));
+    QSignalSpy textFrameReceivedSpy(&socket, SIGNAL(textFrameReceived(QString,bool)));
+    QSignalSpy binaryFrameReceivedSpy(&socket, SIGNAL(binaryFrameReceived(QByteArray,bool)));
+    QSignalSpy textMessageReceivedSpy(&socket, SIGNAL(textMessageReceived(QString)));
+    QSignalSpy binaryMessageReceivedSpy(&socket, SIGNAL(binaryMessageReceived(QByteArray)));
+    QSignalSpy pongSpy(&socket, SIGNAL(pong(quint64,QByteArray)));
+    QSignalSpy bytesWrittenSpy(&socket, SIGNAL(bytesWritten(qint64)));
+
+    socket.open(QUrl(QStringLiteral("ws://127.0.0.1:1/")));
+
+    //at this point the socket is in a connecting state
+    //so, there should no error at this point
+    QCOMPARE(socket.error(), QAbstractSocket::UnknownSocketError);
+    QVERIFY(!socket.errorString().isEmpty());
+    QVERIFY(!socket.isValid());
+    QVERIFY(socket.localAddress().isNull());
+    QCOMPARE(socket.localPort(), quint16(0));
+    QCOMPARE(socket.pauseMode(), QAbstractSocket::PauseNever);
+    QVERIFY(socket.peerAddress().isNull());
+    QCOMPARE(socket.peerPort(), quint16(0));
     QCOMPARE(socket.peerName(), QStringLiteral("127.0.0.1"));
     QCOMPARE(socket.state(), QAbstractSocket::ConnectingState);
     QCOMPARE(socket.readBufferSize(), 0);
     QCOMPARE(socket.resourceName(), QStringLiteral("/"));
     QCOMPARE(socket.requestUrl(), QUrl(QStringLiteral("ws://127.0.0.1:1/")));
     QCOMPARE(socket.closeCode(), QWebSocketProtocol::CloseCodeNormal);
-    QVERIFY(socket.closeReason().isEmpty());
-    QVERIFY(!socket.flush());   //flush should fail if socket is in connecting state
-    QCOMPARE(socket.sendTextMessage(QStringLiteral("A text message")), 0);
-    QCOMPARE(socket.sendBinaryMessage(QByteArrayLiteral("A text message")), 0);
 
     QVERIFY(errorSpy.wait());
+
     QCOMPARE(errorSpy.count(), 1);
     QList<QVariant> arguments = errorSpy.takeFirst();
     QAbstractSocket::SocketError socketError =