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 ++- .../handshakerequest/tst_handshakerequest.cpp | 76 ++++++++++++++++++++-- .../handshakeresponse/tst_handshakeresponse.cpp | 2 +- 5 files changed, 130 insertions(+), 14 deletions(-) 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()); diff --git a/tests/auto/websockets/handshakerequest/tst_handshakerequest.cpp b/tests/auto/websockets/handshakerequest/tst_handshakerequest.cpp index 9c579bd..2921e57 100644 --- a/tests/auto/websockets/handshakerequest/tst_handshakerequest.cpp +++ b/tests/auto/websockets/handshakerequest/tst_handshakerequest.cpp @@ -45,6 +45,9 @@ QT_USE_NAMESPACE Q_DECLARE_METATYPE(QWebSocketProtocol::CloseCode) Q_DECLARE_METATYPE(QWebSocketProtocol::OpCode) +const int MAX_HEADERLINE_LENGTH = 8 * 1024; +const int MAX_HEADERS = 100; + class tst_HandshakeRequest : public QObject { Q_OBJECT @@ -67,6 +70,8 @@ private Q_SLOTS: void tst_multipleVersions(); void tst_qtbug_39355(); + void tst_qtbug_48123_data(); + void tst_qtbug_48123(); }; tst_HandshakeRequest::tst_HandshakeRequest() @@ -203,7 +208,7 @@ void tst_HandshakeRequest::tst_invalidStream() textStream << dataStream; textStream.seek(0); - request.readHandshake(textStream); + request.readHandshake(textStream, MAX_HEADERLINE_LENGTH, MAX_HEADERS); QVERIFY(!request.isValid()); QCOMPARE(request.port(), 80); @@ -239,7 +244,7 @@ void tst_HandshakeRequest::tst_multipleValuesInConnectionHeader() textStream << header; textStream.seek(0); - request.readHandshake(textStream); + request.readHandshake(textStream, MAX_HEADERLINE_LENGTH, MAX_HEADERS); QVERIFY(request.isValid()); QCOMPARE(request.port(), 80); @@ -269,7 +274,7 @@ void tst_HandshakeRequest::tst_multipleVersions() textStream << header; textStream.seek(0); - request.readHandshake(textStream); + request.readHandshake(textStream, MAX_HEADERLINE_LENGTH, MAX_HEADERS); QVERIFY(request.isValid()); QCOMPARE(request.port(), 80); @@ -305,13 +310,76 @@ void tst_HandshakeRequest::tst_qtbug_39355() textStream << header; textStream.seek(0); - request.readHandshake(textStream); + request.readHandshake(textStream, MAX_HEADERLINE_LENGTH, MAX_HEADERS); QVERIFY(request.isValid()); QCOMPARE(request.port(), 1234); QCOMPARE(request.host(), QStringLiteral("localhost")); } +void tst_HandshakeRequest::tst_qtbug_48123_data() +{ + QTest::addColumn("header"); + QTest::addColumn("shouldBeValid"); + const QString header = QStringLiteral("GET /ABC/DEF/ HTTP/1.1\r\nHost: localhost:1234\r\n") + + QStringLiteral("Sec-WebSocket-Version: 13\r\n") + + QStringLiteral("Sec-WebSocket-Key: 2Wg20829/4ziWlmsUAD8Dg==\r\n") + + QStringLiteral("Upgrade: websocket\r\n") + + QStringLiteral("Connection: Upgrade\r\n"); + const int numHeaderLines = header.count(QStringLiteral("\r\n")) - 1; //-1: exclude requestline + + //a headerline should not be larger than MAX_HEADERLINE_LENGTH characters (excluding CRLF) + QString illegalHeader = header; + illegalHeader.append(QString(MAX_HEADERLINE_LENGTH + 1, QChar::fromAscii('c'))); + illegalHeader.append(QStringLiteral("\r\n\r\n")); + + QTest::newRow("headerline too long") << illegalHeader << false; + + QString legalHeader = header; + const QString headerKey = QStringLiteral("X-CUSTOM-KEY: "); + legalHeader.append(headerKey); + legalHeader.append(QString(MAX_HEADERLINE_LENGTH - headerKey.length(), QChar::fromAscii('c'))); + legalHeader.append(QStringLiteral("\r\n\r\n")); + + QTest::newRow("headerline with maximum length") << legalHeader << true; + + //a header should not contain more than MAX_HEADERS header lines (excluding the request line) + //test with MAX_HEADERS + 1 + illegalHeader = header; + const QString headerLine(QStringLiteral("Host: localhost:1234\r\n")); + for (int i = 0; i < (MAX_HEADERS - numHeaderLines + 1); ++i) { + illegalHeader.append(headerLine); + } + illegalHeader.append(QStringLiteral("\r\n")); + + QTest::newRow("too many headerlines") << illegalHeader << false; + + //test with MAX_HEADERS header lines (excluding the request line) + legalHeader = header; + for (int i = 0; i < (MAX_HEADERS - numHeaderLines); ++i) { + legalHeader.append(headerLine); + } + legalHeader.append(QStringLiteral("\r\n")); + + QTest::newRow("just enough headerlines") << legalHeader << true; +} + +void tst_HandshakeRequest::tst_qtbug_48123() +{ + QFETCH(QString, header); + QFETCH(bool, shouldBeValid); + + QByteArray data; + QTextStream textStream(&data); + QWebSocketHandshakeRequest request(8080, false); + + textStream << header; + textStream.seek(0); + request.readHandshake(textStream, MAX_HEADERLINE_LENGTH, MAX_HEADERS); + + QCOMPARE(request.isValid(), shouldBeValid); +} + QTEST_MAIN(tst_HandshakeRequest) #include "tst_handshakerequest.moc" diff --git a/tests/auto/websockets/handshakeresponse/tst_handshakeresponse.cpp b/tests/auto/websockets/handshakeresponse/tst_handshakeresponse.cpp index b5f103b..7d35cd0 100644 --- a/tests/auto/websockets/handshakeresponse/tst_handshakeresponse.cpp +++ b/tests/auto/websockets/handshakeresponse/tst_handshakeresponse.cpp @@ -91,7 +91,7 @@ void tst_HandshakeResponse::tst_date_response() QStringLiteral("Sec-WebSocket-Key: AVDFBDDFF\r\n") + QStringLiteral("Upgrade: websocket\r\n") + QStringLiteral("Connection: Upgrade\r\n\r\n"); - request.readHandshake(input); + request.readHandshake(input, 8 * 1024, 100); QWebSocketHandshakeResponse response(request, "example.com", true, QList() << QWebSocketProtocol::Version13, -- cgit v1.2.1