From 156dcb4770d7f4747b973ba509f6df1933f64817 Mon Sep 17 00:00:00 2001 From: Mårten Nordheim Date: Mon, 03 Aug 2026 17:00:12 +0200 Subject: [PATCH] QHttp2Connection: guard against null-QPointer plant via RST_STREAM handleRST_STREAM() used QHash::operator[] to look up the stream for an incoming RST_STREAM, which inserts a default-constructed (null) QPointer entry when the stream ID is unknown -- e.g. because the client already disposed of a completed stream and its destructor removed the entry from m_streams. A later PUSH_PROMISE referencing that same stream ID passed the existence check in handlePUSH_PROMISE() (constEnd() only) and dereferenced the null QPointer via (*it)->state(), undefined behavior that segfaults under this build's optimizer. Switch both RST_STREAM lookups to QHash::value(), which never inserts, and add an explicit isNull() check in handlePUSH_PROMISE() so a previously-planted (or otherwise absent) entry takes the existing "invalid associated stream" connection-error path instead. Change-Id: Iefc19a16981b985b76477f653b464a71ae6860e1 Reviewed-by: Edward Welbourne (cherry picked from commit 7773b221b4c893809574ddadc1d2c3dfbed55fcd) (cherry picked from commit 139efc479b6e7adf1afb1888a3fd5daf9c8e2ae3) Reviewed-by: Mate Barany --- diff --git a/src/network/access/qhttp2connection.cpp b/src/network/access/qhttp2connection.cpp index d75a30c..3d8feba 100644 --- a/src/network/access/qhttp2connection.cpp +++ b/src/network/access/qhttp2connection.cpp @@ -1774,7 +1774,7 @@ Q_ASSERT(inboundFrame.payloadSize() == 4); const auto error = qFromBigEndian(inboundFrame.dataBegin()); - if (QPointer stream = m_streams[streamID]) + if (QPointer stream = m_streams.value(streamID)) emit stream->rstFrameReceived(error); // Verify that whatever stream is being RST'd is not in the idle state: @@ -1791,7 +1791,7 @@ Q_ASSERT(inboundFrame.dataSize() == 4); - if (QPointer stream = m_streams[streamID]) + if (QPointer stream = m_streams.value(streamID)) stream->handleRST_STREAM(inboundFrame); } @@ -1879,7 +1879,7 @@ // I.e. If you are the server then the client must have initiated the stream you are sending // the promise on. And since this is about _sending_ we have to invert "Remote" to "Local" // because we are receiving. - if (it == m_streams.constEnd()) + if (it == m_streams.constEnd() || it->isNull()) return connectionError(ENHANCE_YOUR_CALM, u"PUSH_PROMISE with invalid associated stream"_s); if ((m_connectionType == Type::Client && (streamID & 1) == 0) || (m_connectionType == Type::Server && (streamID & 1) == 1)) { diff --git a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp index 0226a37..dec2028 100644 --- a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp +++ b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp @@ -52,6 +52,8 @@ void serverInitiatedGoaways(); void clientInitiatedGoaway(); void uploadByteDeviceDestroyedWhileBlocked(); + void pushPromiseOnStreamRemovedAfterRST_data(); + void pushPromiseOnStreamRemovedAfterRST(); private: enum PeerType { Client, Server }; @@ -1682,6 +1684,73 @@ QVERIFY(!clientConn->m_connectionAborted); } +void tst_QHttp2Connection::pushPromiseOnStreamRemovedAfterRST_data() +{ + QTest::addColumn("pushEnabled"); + QTest::newRow("push-disabled-default") << false; + QTest::newRow("push-enabled") << true; +} + +void tst_QHttp2Connection::pushPromiseOnStreamRemovedAfterRST() +{ + QFETCH(const bool, pushEnabled); + + // handleRST_STREAM() used QHash::operator[] to look up the stream, + // planting a null QPointer for an unknown/disposed stream + // ID. A later PUSH_PROMISE referencing that ID only checked constEnd() + // and dereferenced the null QPointer via (*it)->state(). Reachable with + // push enabled or disabled (see below). + auto [client, server] = makeFakeConnectedSockets(); + QHttp2Configuration config; + config.setServerPushEnabled(pushEnabled); + auto clientConn = makeHttp2Connection(client.get(), config, Client); + + QHttp2Stream *clientStream = clientConn->createStream().unwrap(); + QVERIFY(clientStream); + const quint32 streamId = clientStream->streamID(); + QVERIFY(clientStream->sendHEADERS(getRequiredHeaders(), true)); + + // Simulate normal disposal of a (from the client's point of view) + // finished stream, the same way QNetworkAccessManager's handler + // deleteLater()s streams. The destructor removes the m_streams entry. + delete clientStream; + QVERIFY(!clientConn->getStream(streamId)); + + if (!pushEnabled) { + // With push disabled, PUSH_PROMISE is otherwise rejected outright; + // the client's own initial SETTINGS being still unacknowledged is + // what lets it past the "!pushPromiseEnabled" gate + QVERIFY(clientConn->waitingForSettingsACK); + } + + // The attack makes the *client* raise the connection error (and send its + // own GOAWAY out) - it never receives one, so spy on errorOccurred(). + QSignalSpy clientErrorSpy{ clientConn, &QHttp2Connection::errorOccurred }; + + // RST_STREAM on the now-absent stream. + { + Http2::FrameWriter rst(Http2::FrameType::RST_STREAM, Http2::FrameFlag::EMPTY, streamId); + rst.append(quint32(Http2::CANCEL)); + QVERIFY(rst.write(*server)); + } + QIODevice *clientDevice = client.get(); + QVERIFY(QTest::qWaitFor([clientDevice]() { return clientDevice->bytesAvailable() == 0; })); + + // PUSH_PROMISE associated with that same now-absent stream ID. + { + Http2::FrameWriter pp(Http2::FrameType::PUSH_PROMISE, Http2::FrameFlag::END_HEADERS, + streamId); + pp.append(quint32(streamId + 1)); // promised stream ID + QVERIFY(pp.write(*server)); + } + + // Processing the frame above is what would crash, dereferencing the + // planted null QPointer, before this patch. + QVERIFY(QTest::qWaitFor([&]() { return clientErrorSpy.count() == 1; })); + QCOMPARE(clientErrorSpy.first().first().value(), Http2::ENHANCE_YOUR_CALM); + QVERIFY(clientConn->m_connectionAborted); +} + QTEST_MAIN(tst_QHttp2Connection) #include "tst_qhttp2connection.moc"