HTTP2: delay any communication until encrypted() can be responded to

Origin: upstream, https://code.qt.io/cgit/qt/qtbase.git/commit/?id=b1e75376cc3adfc7
Last-Update: 2025-03-25

We have the encrypted() signal that lets users do extra checks on the
established connection. It is emitted as BlockingQueued, so the HTTP
thread stalls until it is done emitting. Users can potentially call
abort() on the QNetworkReply at that point, which is passed as a Queued
call back to the HTTP thread. That means that any currently queued
signal emission will be processed before the abort() call is processed.

In the case of HTTP2 it is a little special since it is multiplexed and
the code is built to start requests as they are available. This means
that, while the code worked fine for HTTP1, since one connection only
has one request, it is not working for HTTP2, since we try to send more
requests in-between the encrypted() signal and the abort() call.

This patch changes the code to delay any communication until the
encrypted() signal has been emitted and processed, for HTTP2 only.
It's done by adding a few booleans, both to know that we have to return
early and so we can keep track of what events arose and what we need to
resume once enough time has passed that any abort() call must have been
processed.

Gbp-Pq: Name CVE-2024-39936.diff

diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp
index ddd661cc50369152b64aad6214b43ac60c3fcdc0..bc6c9c9e8811b57e535a2398d07161119dab2c9f 100644 (file)
--- a/src/network/access/qhttp2protocolhandler.cpp
+++ b/src/network/access/qhttp2protocolhandler.cpp
@@ -371,12 +371,12 @@ bool QHttp2ProtocolHandler::sendRequest()
         }
     }
 
-    if (!prefaceSent && !sendClientPreface())
-        return false;
-
     if (!requests.size())
         return true;
 
+    if (!prefaceSent && !sendClientPreface())
+        return false;
+
     m_channel->state = QHttpNetworkConnectionChannel::WritingState;
     // Check what was promised/pushed, maybe we do not have to send a request
     // and have a response already?

diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
index 7620ca1647020bce1d924a4206b3e0d386a185a7..13f9630c65823d6254418f04a7a3324f98dffb8e 100644 (file)
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
@@ -255,6 +255,10 @@ void QHttpNetworkConnectionChannel::abort()
 bool QHttpNetworkConnectionChannel::sendRequest()
 {
     Q_ASSERT(!protocolHandler.isNull());
+    if (waitingForPotentialAbort) {
+        needInvokeSendRequest = true;
+        return false; // this return value is unused
+    }
     return protocolHandler->sendRequest();
 }
 
@@ -267,21 +271,28 @@ bool QHttpNetworkConnectionChannel::sendRequest()
 void QHttpNetworkConnectionChannel::sendRequestDelayed()
 {
     QMetaObject::invokeMethod(this, [this] {
-        Q_ASSERT(!protocolHandler.isNull());
         if (reply)
-            protocolHandler->sendRequest();
+            sendRequest();
     }, Qt::ConnectionType::QueuedConnection);
 }
 
 void QHttpNetworkConnectionChannel::_q_receiveReply()
 {
     Q_ASSERT(!protocolHandler.isNull());
+    if (waitingForPotentialAbort) {
+        needInvokeReceiveReply = true;
+        return;
+    }
     protocolHandler->_q_receiveReply();
 }
 
 void QHttpNetworkConnectionChannel::_q_readyRead()
 {
     Q_ASSERT(!protocolHandler.isNull());
+    if (waitingForPotentialAbort) {
+        needInvokeReadyRead = true;
+        return;
+    }
     protocolHandler->_q_readyRead();
 }
 
@@ -1289,7 +1300,18 @@ void QHttpNetworkConnectionChannel::_q_encrypted()
             // Similar to HTTP/1.1 counterpart below:
             const auto &pairs = spdyRequestsToSend.values(); // (request, reply)
             const auto &pair = pairs.first();
+            waitingForPotentialAbort = true;
             emit pair.second->encrypted();
+
+            // We don't send or handle any received data until any effects from
+            // emitting encrypted() have been processed. This is necessary
+            // because the user may have called abort(). We may also abort the
+            // whole connection if the request has been aborted and there is
+            // no more requests to send.
+            QMetaObject::invokeMethod(this,
+                                      &QHttpNetworkConnectionChannel::checkAndResumeCommunication,
+                                      Qt::QueuedConnection);
+
             // In case our peer has sent us its settings (window size, max concurrent streams etc.)
             // let's give _q_receiveReply a chance to read them first ('invokeMethod', QueuedConnection).
             QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection);
@@ -1307,6 +1329,26 @@ void QHttpNetworkConnectionChannel::_q_encrypted()
     }
 }
 
+void QHttpNetworkConnectionChannel::checkAndResumeCommunication()
+{
+    Q_ASSERT(connection->connectionType() > QHttpNetworkConnection::ConnectionTypeHTTP);
+
+    // Because HTTP/2 requires that we send a SETTINGS frame as the first thing we do, and respond
+    // to a SETTINGS frame with an ACK, we need to delay any handling until we can ensure that any
+    // effects from emitting encrypted() have been processed.
+    // This function is called after encrypted() was emitted, so check for changes.
+
+    if (!reply && spdyRequestsToSend.isEmpty())
+        abort();
+    waitingForPotentialAbort = false;
+    if (needInvokeReadyRead)
+        _q_readyRead();
+    if (needInvokeReceiveReply)
+        _q_receiveReply();
+    if (needInvokeSendRequest)
+        sendRequest();
+}
+
 void QHttpNetworkConnectionChannel::requeueSpdyRequests()
 {
     QList<HttpMessagePair> spdyPairs = spdyRequestsToSend.values();

diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h
index d8ac3979d1929080450a5308917808f9751619ad..eac44464926c0ce68fbb3383ba0800b5fdfc8531 100644 (file)
--- a/src/network/access/qhttpnetworkconnectionchannel_p.h
+++ b/src/network/access/qhttpnetworkconnectionchannel_p.h
@@ -107,6 +107,10 @@ public:
     QAbstractSocket *socket;
     bool ssl;
     bool isInitialized;
+    bool waitingForPotentialAbort = false;
+    bool needInvokeReceiveReply = false;
+    bool needInvokeReadyRead = false;
+    bool needInvokeSendRequest = false;
     ChannelState state;
     QHttpNetworkRequest request; // current request, only used for HTTP
     QHttpNetworkReply *reply; // current reply for this request, only used for HTTP
@@ -187,6 +191,8 @@ public:
     void closeAndResendCurrentRequest();
     void resendCurrentRequest();
 
+    void checkAndResumeCommunication();
+
     bool isSocketBusy() const;
     bool isSocketWriting() const;
     bool isSocketWaiting() const;