http2: avoid uaf while receiving and sending rst_stream

[Eusgor]

Mark the session as receiving around nghttp2_session_mem_recv() and defer RST_STREAM handling while receive is in progress. This prevents closing a stream while nghttp2 still processes it and avoids heap-use-after-free in nghttp2_session_mem_recv2().

Fixes: #64113

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mertcanaltin]

Can you use "-s" (for Signed-off-by) in first commit command, after run this command
example: git commit --amend --no-edit -s "http2: avoid uaf while receiving and sending rst_stream"

after:
CLANG_FORMAT_START=$(git merge-base HEAD main) make format-cpp for cpp lint errors

[Eusgor]

Is everything correct now?

[RafaelGSS]

Can you add a test case or an script that we could reproduce it?

[Eusgor]

Can you add a test case or an script that we could reproduce it?

The scripts are in the issue #64113
The error is reproduced when using the ASAN sanitizer.

[mcollina]

lgtm

[Eusgor]

If you mean replace the code with this

  if (session_->is_receiving()) {
    if (is_stream_cancel(code) || available_outbound_length_ > 0) {
      session_->AddPendingRstStream(id_);
      return;
    }
    FlushRstStream();
    return;
  }

then it fixes uaf, but I get these failed tests:
test-http2-compat-errors
test-http2-destroy-after-write
test-http2-server-errors

Or if I misunderstood you, please write your version.

[pimterry]

Ah, interesting, yes we'll have to investigate that @Eusgor.

I took a bit more of a look, I think this is part of a bigger more fundamental issue - the nghttp2 docs say:

Do not call nghttp2_session_send(), nghttp2_session_mem_send2(), nghttp2_session_recv() or nghttp2_session_mem_recv2() from the nghttp2 callback functions directly or indirectly. It will lead to the crash.

And in this method, we call SendPendingData while receiving.

The tests are reasonable (e.g. destroy after write - it should flush the written data before the stream is destroyed) but this method we're changing isn't - it should never call SendPendingData with is_receiving() === true.

I had Claude do a quick scan, there's at least three places we ignore this nghttp2 requirement:

• Http2Stream::SubmitRstStream() - fixed by this PR, but only for the case where no writes are pending.
• Http2Session::OnDataChunkReceived - can call SendPendingData if buffer is too large.
• Http2Session::Close() - calls SendPendingData directly

All of those are issues (good find!) and the new is_receiving() flag you've added is the right way to fix all of them, but we should take a more general approach instead of fixing this small part of one case.

Can you take a look? Really I think we need to always defer anything anything that could call one of those methods while receiving - that means something like early-return in SendPendingData() if receiving, deferring resets if there is pending data, and deferred destroy/close to fix the test failures around this logic. Take a look, let me know if you need any pointers and I can come up with a more detailed outline if that'd help.

[codecov]

Codecov Report

❌ Patch coverage is 57.89474% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (5579c49).
⚠️ Report is 352 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http2.cc	56.36%	18 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64166      +/-   ##
==========================================
- Coverage   92.01%   90.24%   -1.77%     
==========================================
  Files         379      739     +360     
  Lines      166972   241720   +74748     
  Branches    25554    45561   +20007     
==========================================
+ Hits       153639   218150   +64511     
- Misses      13041    15112    +2071     
- Partials      292     8458    +8166     

Files with missing lines	Coverage Δ
src/node_http2.h	92.26% <100.00%> (ø)
src/node_http2.cc	81.85% <56.36%> (ø)

... and 554 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[pimterry]

One bit of dead code it'd be nice to clean up, but otherwise this LGTM. Thanks for digging into this further @Eusgor, nice work!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/74826/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/74833/