[v24.x] http2: avoid UAF while receiving and sending RST_STREAM

src/node_http2.cc

@@ -1074,6 +1074,11 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle,
   int32_t id = GetFrameID(frame);
   Debug(session, "beginning headers for stream %d", id);
 
+  // Close() can be called by JavaScript from an earlier receive callback.
+  // Do not create streams that can no longer be exposed to JavaScript.
+  if (session->is_close_pending())
+    return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
+
   BaseObjectPtr<Http2Stream> stream = session->FindStream(id);
   // The common case is that we're creating a new stream. The less likely
   // case is that we're receiving a set of trailers
@@ -1139,6 +1144,12 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle,
   session->statistics_.frame_count++;
   Debug(session, "complete frame received: type: %d",
         frame->hd.type);
+
+  // JavaScript may have closed the session from an earlier receive callback.
+  // FinishClose() runs after nghttp2_session_mem_recv() returns.
+  if (session->is_close_pending())
+    return 0;
+
   switch (frame->hd.type) {
     case NGHTTP2_DATA:
       return session->HandleDataFrame(frame);
@@ -1405,6 +1416,12 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
   if (len == 0)
     return 0;
 
+  // Close() can be called by JavaScript from an earlier receive callback.
+  // Ignore the rest of the buffered DATA because its stream may never have
+  // been exposed to JavaScript and therefore has no onread callback.
+  if (session->is_close_pending())
+    return 0;
+
   // Notify nghttp2 that we've consumed a chunk of data on the connection
   // so that it can send a WINDOW_UPDATE frame. This is a critical part of
   // the flow control process in http2
@@ -2004,6 +2021,11 @@ uint8_t Http2Session::SendPendingData() {
   if (is_sending())
     return 1;
 
+  // Do not call `nghttp2_session_mem_send()` while nghttp2 is processing
+  // incoming data. Sending may close the stream and free nghttp2 state
+  // that is still in use by `nghttp2_session_mem_recv()`.
+  if (is_receiving()) return 1;
+
   // This is cleared by ClearOutgoing().
   set_sending();
 
@@ -2603,12 +2625,17 @@ void Http2Stream::SubmitRstStream(const uint32_t code) {
   // Do not call `nghttp2_session_mem_send()` while nghttp2 is processing
   // incoming data. Sending may close the stream and free nghttp2 state
   // that is still in use by `nghttp2_session_mem_recv()`.
-  if (session_->is_receiving() && available_outbound_length_ == 0) {
-    if (is_stream_cancel(code)) {
+  if (session_->is_receiving()) {
+    // These resets must be submitted before the current callback returns.
+    // In particular, nghttp2 otherwise replaces ENHANCE_YOUR_CALM with
+    // INTERNAL_ERROR when OnHeaderCallback returns a temporal failure.
+    if (code == NGHTTP2_ENHANCE_YOUR_CALM ||
+        code == NGHTTP2_REFUSED_STREAM) {
+      FlushRstStream();
+    } else {
+      // Let queued DATA, including END_STREAM, be serialized before the reset.
       session_->AddPendingRstStream(id_);
-      return;
     }
-    FlushRstStream();
     return;
   }
 
@@ -2864,7 +2891,9 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
   if (stream->available_outbound_length_ == 0 && !stream->is_writable()) {
     Debug(session, "no more data for stream %d", id);
     *flags |= NGHTTP2_DATA_FLAG_EOF;
-    if (stream->has_trailers()) {
+    // A deferred Destroy() cannot call back into JavaScript for trailers.
+    // Let the DATA frame end the stream instead.
+    if (stream->has_trailers() && !stream->is_destroyed()) {
       *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
       stream->OnTrailers();
     }

test/parallel/test-http2-session-destroy-during-receive.js

@@ -0,0 +1,41 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const fixtures = require('../common/fixtures');
+const http2 = require('http2');
+
+// Regression test for closing a session while nghttp2 is processing several
+// streams from the same input buffer. No stream created after the close can be
+// exposed to JavaScript, so delivering its DATA would call a missing onread.
+const server = http2.createSecureServer({
+  key: fixtures.readKey('agent2-key.pem'),
+  cert: fixtures.readKey('agent2-cert.pem')
+});
+
+server.on('stream', common.mustCallAtLeast((stream) => {
+  stream.on('error', () => {});
+  stream.session.destroy();
+}, 1));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`https://localhost:${server.address().port}`, {
+    rejectUnauthorized: false
+  });
+  client.on('error', () => {});
+  client.on('close', common.mustCall(() => server.close()));
+
+  client.on('remoteSettings', common.mustCall(() => {
+    for (let i = 0; i < 8; i++) {
+      const stream = client.request({
+        ':method': 'POST',
+        ':path': `/${i}`
+      });
+      stream.on('error', () => {});
+      stream.resume();
+      stream.end(Buffer.alloc(512));
+    }
+  }));
+}));