From d642c16fe745e156a93129fbb3750784b361ee39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Thu, 31 Mar 2022 18:48:42 +0200 Subject: QNetworkReply: update decompress error message and handling The error message was quite vague since it would then not require any additional translations. However, in hindsight this was a mistake since now developers just thought their downloads were being corrupted. Pick-to: 6.2 6.3 Fixes: QTBUG-101942 Change-Id: Ie9af42510ca027d15248e5bcf21e836e709898d9 Reviewed-by: Timur Pocheptsov --- src/network/access/qdecompresshelper.cpp | 44 +++++++++++++++++++--- src/network/access/qdecompresshelper_p.h | 4 ++ src/network/access/qnetworkreplyhttpimpl.cpp | 31 +++++++++------ src/network/doc/src/qt6-changes.qdoc | 10 +++++ .../qdecompresshelper/tst_qdecompresshelper.cpp | 7 +++- .../access/qnetworkreply/tst_qnetworkreply.cpp | 35 ++++++++++++++++- 6 files changed, 111 insertions(+), 20 deletions(-) diff --git a/src/network/access/qdecompresshelper.cpp b/src/network/access/qdecompresshelper.cpp index f3227efe8a..ec6cc5b13d 100644 --- a/src/network/access/qdecompresshelper.cpp +++ b/src/network/access/qdecompresshelper.cpp @@ -41,6 +41,7 @@ #include #include +#include #include #include @@ -131,13 +132,16 @@ bool QDecompressHelper::setEncoding(const QByteArray &encoding) Q_ASSERT(contentEncoding == QDecompressHelper::None); if (contentEncoding != QDecompressHelper::None) { qWarning("Encoding is already set."); + // This isn't an error, so it doesn't set errorStr, it's just wrong usage. return false; } ContentEncoding ce = encodingFromByteArray(encoding); if (ce == None) { - qWarning("An unsupported content encoding was selected: %s", encoding.data()); + errorStr = QCoreApplication::translate("QHttp", "Unsupported content encoding: %1") + .arg(QLatin1String(encoding)); return false; } + errorStr = QString(); // clear error return setEncoding(ce); } @@ -179,7 +183,8 @@ bool QDecompressHelper::setEncoding(ContentEncoding ce) break; } if (!decoderPointer) { - qWarning("Failed to initialize the decoder."); + errorStr = QCoreApplication::translate("QHttp", + "Failed to initialize the compression decoder."); contentEncoding = QDecompressHelper::None; return false; } @@ -435,8 +440,13 @@ qsizetype QDecompressHelper::readInternal(char *data, qsizetype maxSize) clear(); totalUncompressedBytes += bytesRead; - if (isPotentialArchiveBomb()) + if (isPotentialArchiveBomb()) { + errorStr = QCoreApplication::translate( + "QHttp", + "The decompressed output exceeds the limits specified by " + "QNetworkRequest::decompressedSafetyCheckThreshold()"); return -1; + } return bytesRead; } @@ -517,11 +527,29 @@ qint64 QDecompressHelper::encodedBytesAvailable() const return compressedDataBuffer.byteAmount(); } +/*! + \internal + Returns whether or not the object is valid. + If it becomes invalid after an operation has been performed + then an error has occurred. + \sa errorString() +*/ bool QDecompressHelper::isValid() const { return contentEncoding != None; } +/*! + \internal + Returns a string describing the error that occurred or an empty + string if no error occurred. + \sa isValid() +*/ +QString QDecompressHelper::errorString() const +{ + return errorStr; +} + void QDecompressHelper::clear() { switch (contentEncoding) { @@ -564,6 +592,8 @@ void QDecompressHelper::clear() totalBytesRead = 0; totalUncompressedBytes = 0; totalCompressedBytes = 0; + + errorStr.clear(); } qsizetype QDecompressHelper::readZLib(char *data, const qsizetype maxSize) @@ -719,8 +749,9 @@ qsizetype QDecompressHelper::readBrotli(char *data, const qsizetype maxSize) switch (result) { case BROTLI_DECODER_RESULT_ERROR: - qWarning("Brotli error: %s", - BrotliDecoderErrorString(BrotliDecoderGetErrorCode(brotliDecoderState))); + errorStr = QLatin1String("Brotli error: %1") + .arg(QString::fromUtf8(BrotliDecoderErrorString( + BrotliDecoderGetErrorCode(brotliDecoderState)))); return -1; case BROTLI_DECODER_RESULT_SUCCESS: BrotliDecoderDestroyInstance(brotliDecoderState); @@ -766,7 +797,8 @@ qsizetype QDecompressHelper::readZstandard(char *data, const qsizetype maxSize) while (outBuf.pos < outBuf.size && (inBuf.pos < inBuf.size || decoderHasData)) { size_t retValue = ZSTD_decompressStream(zstdStream, &outBuf, &inBuf); if (ZSTD_isError(retValue)) { - qWarning("ZStandard error: %s", ZSTD_getErrorName(retValue)); + errorStr = QLatin1String("ZStandard error: %1") + .arg(QString::fromUtf8(ZSTD_getErrorName(retValue))); return -1; } else { decoderHasData = false; diff --git a/src/network/access/qdecompresshelper_p.h b/src/network/access/qdecompresshelper_p.h index b0b60b2119..aa256f61bb 100644 --- a/src/network/access/qdecompresshelper_p.h +++ b/src/network/access/qdecompresshelper_p.h @@ -96,6 +96,8 @@ public: static bool isSupportedEncoding(const QByteArray &encoding); static QByteArrayList acceptedEncoding(); + QString errorString() const; + private: bool isPotentialArchiveBomb() const; bool hasDataInternal() const; @@ -120,6 +122,8 @@ private: bool countDecompressed = false; std::unique_ptr countHelper; + QString errorStr; + // Used for calculating the ratio qint64 archiveBombCheckThreshold = 10 * 1024 * 1024; qint64 totalUncompressedBytes = 0; diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp index 0a47bc9f71..486994a1de 100644 --- a/src/network/access/qnetworkreplyhttpimpl.cpp +++ b/src/network/access/qnetworkreplyhttpimpl.cpp @@ -359,9 +359,10 @@ qint64 QNetworkReplyHttpImpl::readData(char* data, qint64 maxlen) return 0; const qint64 bytesRead = d->decompressHelper.read(data, maxlen); if (!d->decompressHelper.isValid()) { - // error occurred, error copied from QHttpNetworkConnectionPrivate::errorDetail - d->error(QNetworkReplyImpl::NetworkError::ProtocolFailure, - QCoreApplication::translate("QHttp", "Data corrupted")); + d->error(QNetworkReplyImpl::NetworkError::UnknownContentError, + QCoreApplication::translate("QHttp", "Decompression failed: %1") + .arg(d->decompressHelper.errorString())); + return -1; } if (d->cacheSaveDevice) { // Need to write to the cache now that we have the data @@ -1084,9 +1085,9 @@ void QNetworkReplyHttpImplPrivate::replyDownloadData(QByteArray d) decompressHelper.feed(std::move(d)); if (!decompressHelper.isValid()) { - // error occurred, error copied from QHttpNetworkConnectionPrivate::errorDetail - error(QNetworkReplyImpl::NetworkError::ProtocolFailure, - QCoreApplication::translate("QHttp", "Data corrupted")); + error(QNetworkReplyImpl::NetworkError::UnknownContentError, + QCoreApplication::translate("QHttp", "Decompression failed: %1") + .arg(decompressHelper.errorString())); return; } @@ -1103,12 +1104,19 @@ void QNetworkReplyHttpImplPrivate::replyDownloadData(QByteArray d) while (decompressHelper.hasData()) { quint64 nextSize = quint64(d.size()) + quint64(increments); if (nextSize > quint64(std::numeric_limits::max())) { - error(QNetworkReplyImpl::NetworkError::ProtocolFailure, - QCoreApplication::translate("QHttp", "Data corrupted")); + error(QNetworkReplyImpl::NetworkError::UnknownContentError, + QCoreApplication::translate("QHttp", + "Data downloaded is too large to store")); return; } d.resize(nextSize); bytesRead += decompressHelper.read(d.data() + bytesRead, increments); + if (!decompressHelper.isValid()) { + error(QNetworkReplyImpl::NetworkError::UnknownContentError, + QCoreApplication::translate("QHttp", "Decompression failed: %1") + .arg(decompressHelper.errorString())); + return; + } } d.resize(bytesRead); // we're synchronous so we're not calling this function again; reset the decompressHelper @@ -1374,9 +1382,10 @@ void QNetworkReplyHttpImplPrivate::replyDownloadMetaData(const QListsecond)) { - // error occurred, error copied from QHttpNetworkConnectionPrivate::errorDetail - error(QNetworkReplyImpl::NetworkError::ProtocolFailure, - QCoreApplication::translate("QHttp", "Data corrupted")); + error(QNetworkReplyImpl::NetworkError::UnknownContentError, + QCoreApplication::translate("QHttp", "Failed to initialize decompression: %1") + .arg(decompressHelper.errorString())); + return; } decompressHelper.setDecompressedSafetyCheckThreshold( request.decompressedSafetyCheckThreshold()); diff --git a/src/network/doc/src/qt6-changes.qdoc b/src/network/doc/src/qt6-changes.qdoc index f760466f42..2de6c0faa6 100644 --- a/src/network/doc/src/qt6-changes.qdoc +++ b/src/network/doc/src/qt6-changes.qdoc @@ -196,4 +196,14 @@ \code request.setAttribute(QNetworkRequest::Http2AllowedAttribute, false); \endcode + + \section2 QNetworkAccessManager now guards against archive bombs + + Starting with Qt 6.2 QNetworkAccessManager will guard against compressed + files that decompress to files which are much larger than their compressed + form by erroring out the reply if the decompression ratio exceeds a certain + threshold. + This check is only applied to files larger than a certain size, which can be + customized (or disabled by passing -1) by calling + \l{QNetworkRequest::setDecompressedSafetyCheckThreshold()}. */ diff --git a/tests/auto/network/access/qdecompresshelper/tst_qdecompresshelper.cpp b/tests/auto/network/access/qdecompresshelper/tst_qdecompresshelper.cpp index 3177cf02c1..27fb8c8ac5 100644 --- a/tests/auto/network/access/qdecompresshelper/tst_qdecompresshelper.cpp +++ b/tests/auto/network/access/qdecompresshelper/tst_qdecompresshelper.cpp @@ -432,10 +432,13 @@ void tst_QDecompressHelper::archiveBomb() QVERIFY(bytesRead <= output.size()); QVERIFY(helper.isValid()); - if (shouldFail) + if (shouldFail) { QCOMPARE(bytesRead, -1); - else + QVERIFY(!helper.errorString().isEmpty()); + } else { QVERIFY(bytesRead > 0); + QVERIFY(helper.errorString().isEmpty()); + } } void tst_QDecompressHelper::bigZlib() diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp index c8e02f27a9..9c177e6281 100644 --- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp @@ -532,6 +532,8 @@ private Q_SLOTS: void cacheWithContentEncoding(); void downloadProgressWithContentEncoding_data(); void downloadProgressWithContentEncoding(); + void contentEncodingError_data(); + void contentEncodingError(); // NOTE: This test must be last! void parentingRepliesToTheApp(); @@ -7105,7 +7107,7 @@ void tst_QNetworkReply::compressedHttpReplyBrokenGzip() QCOMPARE(waitForFinish(reply), int(Failure)); - QCOMPARE(reply->error(), QNetworkReply::ProtocolFailure); + QCOMPARE(reply->error(), QNetworkReply::UnknownContentError); } // TODO add similar test for FTP @@ -9808,6 +9810,37 @@ void tst_QNetworkReply::downloadProgressWithContentEncoding() QCOMPARE(bytesReceived, expected.size()); } +void tst_QNetworkReply::contentEncodingError_data() +{ + QTest::addColumn("encoding"); + QTest::addColumn("path"); + QTest::addColumn("expectedError"); + + QTest::addRow("archive-bomb") << QByteArray("gzip") << (":/4G.gz") + << QNetworkReply::UnknownContentError; +} + +void tst_QNetworkReply::contentEncodingError() +{ + QFETCH(QString, path); + QFile compressedFile(path); + QVERIFY(compressedFile.open(QIODevice::ReadOnly)); + QByteArray body = compressedFile.readAll(); + + QFETCH(QByteArray, encoding); + QString header("HTTP/1.0 200 OK\r\nContent-Encoding: %1\r\nContent-Length: %2\r\n\r\n"); + header = header.arg(encoding, QString::number(body.size())); + + MiniHttpServer server(header.toLatin1() + body); + + QNetworkRequest request( + QUrl(QLatin1String("http://localhost:%1").arg(QString::number(server.serverPort())))); + QNetworkReplyPtr reply(manager.get(request)); + + QTRY_VERIFY2_WITH_TIMEOUT(reply->isFinished(), qPrintable(reply->errorString()), 15000); + QTEST(reply->error(), "expectedError"); +} + // NOTE: This test must be last testcase in tst_qnetworkreply! void tst_QNetworkReply::parentingRepliesToTheApp() { -- cgit v1.2.3