From d2e9a03ee3474e5bfce06095895bb2ed53ac4f9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Tue, 20 Aug 2024 18:28:03 +0200 Subject: [PATCH] Http2: flip some checks for PUSH_PROMISE around It was correctly following the RFC, but we overlooked that the RFC is talking about restrictions on _sending_, while we are verifying on the _receiving_ end. So, some things has to be flipped. And the dataSize offset was incorrect and could read out-of-bounds. Amends d17d260948e16549d82f1fdd4dec98d246b0622e Pick-to: 6.7 Change-Id: Idb7e00637398dff190fc4e5b9776a8649a04f391 Reviewed-by: Edward Welbourne (cherry picked from commit 02d1d638545fd8600fac9707a8f9af03f99197ec) Reviewed-by: Qt Cherry-pick Bot --- src/network/access/qhttp2connection.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/network/access/qhttp2connection.cpp b/src/network/access/qhttp2connection.cpp index 8e0b6cd6332..e425e5faea2 100644 --- a/src/network/access/qhttp2connection.cpp +++ b/src/network/access/qhttp2connection.cpp @@ -1564,14 +1564,18 @@ void QHttp2Connection::handlePUSH_PROMISE() #endif // RFC 9113, 6.6: PUSH_PROMISE frames MUST only be sent on a peer-initiated stream that // is in either the "open" or "half-closed (remote)" state. + + // 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()) return connectionError(ENHANCE_YOUR_CALM, "PUSH_PROMISE with invalid associated stream"); - if ((m_connectionType == Type::Client && (streamID & 1) == 1) || - (m_connectionType == Type::Server && (streamID & 1) == 0)) { + if ((m_connectionType == Type::Client && (streamID & 1) == 0) || + (m_connectionType == Type::Server && (streamID & 1) == 1)) { return connectionError(ENHANCE_YOUR_CALM, "PUSH_PROMISE with invalid associated stream"); } if ((*it)->state() != QHttp2Stream::State::Open && - (*it)->state() != QHttp2Stream::State::HalfClosedRemote) { + (*it)->state() != QHttp2Stream::State::HalfClosedLocal) { return connectionError(ENHANCE_YOUR_CALM, "PUSH_PROMISE with invalid associated stream"); } @@ -1600,7 +1604,7 @@ void QHttp2Connection::handlePUSH_PROMISE() // Length field. If the length of the padding is the length of the frame payload or greater, // the recipient MUST treat this as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. // checked in Frame::validateHeader() - Q_ASSERT(inboundFrame.dataSize() > qFromBigEndian(inboundFrame.dataBegin() + 32)); + Q_ASSERT(inboundFrame.dataSize() > inboundFrame.padding()); const bool endHeaders = inboundFrame.flags().testFlag(FrameFlag::END_HEADERS); continuedFrames.clear(); continuedFrames.push_back(std::move(inboundFrame));