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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR

[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 367 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]

Landed in 46de80d

[Eusgor]

Is it planned to add the patch to the LTS branches?

[mcollina]

To all who arrive here: this cannot be reproduced without an ASAN build, making it non-exploitable in our tests. Therefore, it does not classify as a vulnerability per our threat model.

[Eusgor]

The error is also reproduced in version 26.x. The patch should be added to the v26.x branch as well.

[aduh95]

test-http2-many-writes-and-destroy is timing out on v26.x-staging with this change, we would need a manual backport PR