From e8335d48aa8c6323a6b89d29c76f2b340afd1be4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20K=C3=BCmmel?= Date: Mon, 24 Aug 2015 18:34:40 +0200 Subject: Set parent of internal socket objects After moving the websocket into another thread current code doesn't work because then the QTcpSocket/QSslSocket objects reside in a different thread, for instance: "QSocketNotifier: Socket notifiers cannot be enabled or disabled from another thread" QObject::moveToThread(QThread*) also moves QObjects's children, therefore their internal socket objects need to be children of QWebSocket. QWebSocket has ownership of the internal socket, and the smart pointer is not needed any more. Change of cleanup code to prevent crashes with clang/msvc builds: QWebSocketPrivate is a scoped member of QObject (not QWebSocket) and is destroyed after QObject destructor body was executed, and so m_pSocket&co had already been destroyed (being children) when the destructor of QWebSocketPrivate is called via the scoped pointer. Analogous to 64927e04f202d33b9a9a1f94141ef692c0b513ac Change-Id: I1ade6cda3fa793c30332cc5e103025e2dda3c78c Reviewed-by: Luca Niccoli Reviewed-by: Alex Blasche --- src/websockets/qwebsocket.cpp | 2 ++ src/websockets/qwebsocket_p.cpp | 43 ++++++++++++++++++++++++----------------- src/websockets/qwebsocket_p.h | 3 ++- 3 files changed, 29 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/websockets/qwebsocket.cpp b/src/websockets/qwebsocket.cpp index a77f23c..ee9b2d2 100644 --- a/src/websockets/qwebsocket.cpp +++ b/src/websockets/qwebsocket.cpp @@ -281,6 +281,8 @@ QWebSocket::QWebSocket(const QString &origin, */ QWebSocket::~QWebSocket() { + Q_D(QWebSocket); + d->closeGoingAway(); } /*! diff --git a/src/websockets/qwebsocket_p.cpp b/src/websockets/qwebsocket_p.cpp index 174214c..2a27abb 100644 --- a/src/websockets/qwebsocket_p.cpp +++ b/src/websockets/qwebsocket_p.cpp @@ -84,7 +84,7 @@ QWebSocketPrivate::QWebSocketPrivate(const QString &origin, QWebSocketProtocol:: QWebSocket *pWebSocket) : QObjectPrivate(), q_ptr(pWebSocket), - m_pSocket(), + m_pSocket(Q_NULLPTR), m_errorString(), m_version(version), m_resourceName(), @@ -154,7 +154,7 @@ void QWebSocketPrivate::init() m_pMaskGenerator->seed(); if (m_pSocket) { - makeConnections(m_pSocket.data()); + makeConnections(m_pSocket); } } @@ -162,12 +162,19 @@ void QWebSocketPrivate::init() \internal */ QWebSocketPrivate::~QWebSocketPrivate() +{ +} + +/*! + \internal +*/ +void QWebSocketPrivate::closeGoingAway() { if (!m_pSocket) return; if (state() == QAbstractSocket::ConnectedState) close(QWebSocketProtocol::CloseCodeGoingAway, QWebSocket::tr("Connection closed")); - releaseConnections(m_pSocket.data()); + releaseConnections(m_pSocket); } /*! @@ -262,7 +269,7 @@ void QWebSocketPrivate::ignoreSslErrors() { m_configuration.m_ignoreSslErrors = true; if (Q_LIKELY(m_pSocket)) { - QSslSocket *pSslSocket = qobject_cast(m_pSocket.data()); + QSslSocket *pSslSocket = qobject_cast(m_pSocket); if (Q_LIKELY(pSslSocket)) pSslSocket->ignoreSslErrors(); } @@ -334,17 +341,17 @@ 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 - //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); - pTcpSocket->deleteLater(); + if (m_pSocket) { + releaseConnections(m_pSocket); + m_pSocket->deleteLater(); + m_pSocket = Q_NULLPTR; } //if (m_url != url) if (Q_LIKELY(!m_pSocket)) { @@ -380,15 +387,15 @@ void QWebSocketPrivate::open(const QUrl &url, bool mask) setErrorString(message); Q_EMIT q->error(QAbstractSocket::UnsupportedSocketOperationError); } else { - QSslSocket *sslSocket = new QSslSocket; - m_pSocket.reset(sslSocket); + QSslSocket *sslSocket = new QSslSocket(q_ptr); + m_pSocket = sslSocket; if (Q_LIKELY(m_pSocket)) { m_pSocket->setSocketOption(QAbstractSocket::LowDelayOption, 1); m_pSocket->setSocketOption(QAbstractSocket::KeepAliveOption, 1); m_pSocket->setReadBufferSize(m_readBufferSize); m_pSocket->setPauseMode(m_pauseMode); - makeConnections(m_pSocket.data()); + makeConnections(m_pSocket); setSocketState(QAbstractSocket::ConnectingState); sslSocket->setSslConfiguration(m_configuration.m_sslConfiguration); @@ -409,14 +416,14 @@ void QWebSocketPrivate::open(const QUrl &url, bool mask) } else #endif if (url.scheme() == QStringLiteral("ws")) { - m_pSocket.reset(new QTcpSocket); + m_pSocket = new QTcpSocket(q_ptr); if (Q_LIKELY(m_pSocket)) { m_pSocket->setSocketOption(QAbstractSocket::LowDelayOption, 1); m_pSocket->setSocketOption(QAbstractSocket::KeepAliveOption, 1); m_pSocket->setReadBufferSize(m_readBufferSize); m_pSocket->setPauseMode(m_pauseMode); - makeConnections(m_pSocket.data()); + makeConnections(m_pSocket); setSocketState(QAbstractSocket::ConnectingState); #ifndef QT_NO_NETWORKPROXY m_pSocket->setProxy(m_configuration.m_proxy); @@ -1091,8 +1098,8 @@ void QWebSocketPrivate::processStateChanged(QAbstractSocket::SocketState socketS void QWebSocketPrivate::socketDestroyed(QObject *socket) { Q_ASSERT(m_pSocket); - if (m_pSocket.data() == socket) - m_pSocket.take(); + if (m_pSocket == socket) + m_pSocket = Q_NULLPTR; } /*! @@ -1103,9 +1110,9 @@ void QWebSocketPrivate::processData() Q_ASSERT(m_pSocket); while (m_pSocket->bytesAvailable()) { if (state() == QAbstractSocket::ConnectingState) - processHandshake(m_pSocket.data()); + processHandshake(m_pSocket); else - m_dataProcessor.process(m_pSocket.data()); + m_dataProcessor.process(m_pSocket); } } diff --git a/src/websockets/qwebsocket_p.h b/src/websockets/qwebsocket_p.h index 7379c51..99acad2 100644 --- a/src/websockets/qwebsocket_p.h +++ b/src/websockets/qwebsocket_p.h @@ -143,6 +143,7 @@ public: QSslConfiguration sslConfiguration() const; #endif + void closeGoingAway(); void close(QWebSocketProtocol::CloseCode closeCode, QString reason); void open(const QUrl &url, bool mask); void ping(const QByteArray &payload); @@ -196,7 +197,7 @@ private: qint64 writeFrames(const QList &frames) Q_REQUIRED_RESULT; qint64 writeFrame(const QByteArray &frame) Q_REQUIRED_RESULT; - QScopedPointer m_pSocket; + QTcpSocket *m_pSocket; QString m_errorString; QWebSocketProtocol::Version m_version; QUrl m_resource; -- cgit v1.2.1 From 12e424f241b29ef26ad2a3a70740d8b320e9e85a Mon Sep 17 00:00:00 2001 From: Kurt Pattyn Date: Sun, 6 Sep 2015 15:30:03 +0200 Subject: Fix DoS vulnerability Add checks on maximum header line length and on the maximum number of header lines. Task-number: QTBUG-48123 Change-Id: I65dbeb53af7aa0dfa137ce31fc2549940559314e Reviewed-by: Richard J. Moore --- src/websockets/qwebsockethandshakerequest.cpp | 56 +++++++++++++++++++++++---- src/websockets/qwebsockethandshakerequest_p.h | 2 +- src/websockets/qwebsocketserver_p.cpp | 8 +++- 3 files changed, 57 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/websockets/qwebsockethandshakerequest.cpp b/src/websockets/qwebsockethandshakerequest.cpp index 528cb32..e2cbaef 100644 --- a/src/websockets/qwebsockethandshakerequest.cpp +++ b/src/websockets/qwebsockethandshakerequest.cpp @@ -183,18 +183,49 @@ QUrl QWebSocketHandshakeRequest::requestUrl() const } /*! + Reads a line of text from the given textstream (terminated by CR/LF). + If an empty line was detected, an empty string is returned. + When an error occurs, a null string is returned. \internal */ -void QWebSocketHandshakeRequest::readHandshake(QTextStream &textStream) +static QString readLine(QTextStream &stream, int maxHeaderLineLength) +{ + QString line; + char c; + while (!stream.atEnd()) { + stream >> c; + if (stream.status() != QTextStream::Ok) + return QString(); + if (c == char('\r')) { + //eat the \n character + stream >> c; + line.append(QStringLiteral("")); + break; + } else { + line.append(QChar::fromLatin1(c)); + if (line.length() > maxHeaderLineLength) + return QString(); + } + } + return line; +} + +/*! + \internal + */ +void QWebSocketHandshakeRequest::readHandshake(QTextStream &textStream, int maxHeaderLineLength, + int maxHeaders) { - m_isValid = false; clear(); if (Q_UNLIKELY(textStream.status() != QTextStream::Ok)) return; - const QString requestLine = textStream.readLine(); + const QString requestLine = readLine(textStream, maxHeaderLineLength); + if (requestLine.isNull()) { + clear(); + return; + } const QStringList tokens = requestLine.split(' ', QString::SkipEmptyParts); if (Q_UNLIKELY(tokens.length() < 3)) { - m_isValid = false; clear(); return; } @@ -206,10 +237,13 @@ void QWebSocketHandshakeRequest::readHandshake(QTextStream &textStream) if (Q_UNLIKELY(!conversionOk)) { clear(); - m_isValid = false; return; } - QString headerLine = textStream.readLine(); + QString headerLine = readLine(textStream, maxHeaderLineLength); + if (headerLine.isNull()) { + clear(); + return; + } m_headers.clear(); while (!headerLine.isEmpty()) { const QStringList headerField = headerLine.split(QStringLiteral(": "), @@ -219,7 +253,15 @@ void QWebSocketHandshakeRequest::readHandshake(QTextStream &textStream) return; } m_headers.insertMulti(headerField.at(0).toLower(), headerField.at(1)); - headerLine = textStream.readLine(); + if (m_headers.size() > maxHeaders) { + clear(); + return; + } + headerLine = readLine(textStream, maxHeaderLineLength); + if (headerLine.isNull()) { + clear(); + return; + } } m_requestUrl = QUrl::fromEncoded(resourceName.toLatin1()); diff --git a/src/websockets/qwebsockethandshakerequest_p.h b/src/websockets/qwebsockethandshakerequest_p.h index ad2a249..6ad5381 100644 --- a/src/websockets/qwebsockethandshakerequest_p.h +++ b/src/websockets/qwebsockethandshakerequest_p.h @@ -78,7 +78,7 @@ public: QString resourceName() const; QString host() const; - void readHandshake(QTextStream &textStream); + void readHandshake(QTextStream &textStream, int maxHeaderLineLength, int maxHeaders); private: diff --git a/src/websockets/qwebsocketserver_p.cpp b/src/websockets/qwebsocketserver_p.cpp index bc23674..d1750ce 100644 --- a/src/websockets/qwebsocketserver_p.cpp +++ b/src/websockets/qwebsocketserver_p.cpp @@ -49,6 +49,12 @@ QT_BEGIN_NAMESPACE +//both constants are taken from the default settings of Apache +//see: http://httpd.apache.org/docs/2.2/mod/core.html#limitrequestfieldsize and +//http://httpd.apache.org/docs/2.2/mod/core.html#limitrequestfields +const int MAX_HEADERLINE_LENGTH = 8 * 1024; //maximum length of a http request header line +const int MAX_HEADERLINES = 100; //maximum number of http request header lines + /*! \internal */ @@ -431,7 +437,7 @@ void QWebSocketServerPrivate::handshakeReceived() QWebSocketHandshakeRequest request(pTcpSocket->peerPort(), isSecure); QTextStream textStream(pTcpSocket); - request.readHandshake(textStream); + request.readHandshake(textStream, MAX_HEADERLINE_LENGTH, MAX_HEADERLINES); if (request.isValid()) { QWebSocketCorsAuthenticator corsAuthenticator(request.origin()); -- cgit v1.2.1