Skip to content

Commit

Permalink
Address CVE-2025-23085 for nodejs18
Browse files Browse the repository at this point in the history
  • Loading branch information
Kanishk-Bansal committed Feb 11, 2025
1 parent b9fb0f7 commit f958b19
Show file tree
Hide file tree
Showing 2 changed files with 203 additions and 2 deletions.
200 changes: 200 additions & 0 deletions SPECS/nodejs/CVE-2025-23085.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
From 4f5c7b360f31f31ac160b171861c5f52f48e367e Mon Sep 17 00:00:00 2001
From: Kanishk-Bansal <[email protected]>
Date: Tue, 11 Feb 2025 15:53:59 +0000
Subject: [PATCH] Fix CVE-2025-23085

---
lib/internal/http2/core.js | 15 ++++++--
src/node_http2.cc | 36 ++++++++++++++++---
...2-connect-method-extended-cant-turn-off.js | 6 ++++
...-http2-options-max-headers-block-length.js | 4 ++-
...tp2-options-max-headers-exceeds-nghttp2.js | 4 ++-
5 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 92ce193b..38844d30 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -608,11 +608,20 @@ function onFrameError(id, type, code) {
return;
debugSessionObj(session, 'error sending frame type %d on stream %d, code: %d',
type, id, code);
- const emitter = session[kState].streams.get(id) || session;
+
+ const stream = session[kState].streams.get(id);
+ const emitter = stream || session;
emitter[kUpdateTimer]();
emitter.emit('frameError', type, code, id);
- session[kState].streams.get(id).close(code);
- session.close();
+
+ // When a frameError happens is not uncommon that a pending GOAWAY
+ // package from nghttp2 is on flight with a correct error code.
+ // We schedule it using setImmediate to give some time for that
+ // package to arrive.
+ setImmediate(() => {
+ stream?.close(code);
+ session.close();
+ });
}

function onAltSvc(stream, origin, alt) {
diff --git a/src/node_http2.cc b/src/node_http2.cc
index eb3506ff..38d47f0c 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -750,6 +750,7 @@ bool Http2Session::CanAddStream() {
}

void Http2Session::AddStream(Http2Stream* stream) {
+ Debug(this, "Adding stream: %d", stream->id());
CHECK_GE(++statistics_.stream_count, 0);
streams_[stream->id()] = BaseObjectPtr<Http2Stream>(stream);
size_t size = streams_.size();
@@ -760,6 +761,7 @@ void Http2Session::AddStream(Http2Stream* stream) {


BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) {
+ Debug(this, "Removing stream: %d", id);
BaseObjectPtr<Http2Stream> stream;
if (streams_.empty())
return stream;
@@ -936,6 +938,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle,
if (UNLIKELY(!stream))
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;

+ Debug(session, "handling header key/pair for stream %d", id);
// If the stream has already been destroyed, ignore.
if (!stream->is_destroyed() && !stream->AddHeader(name, value, flags)) {
// This will only happen if the connected peer sends us more
@@ -1005,9 +1008,21 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
return 1;
}

- // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
+ // If the error is fatal or if error code is one of the following
+ // we emit and error:
+ //
+ // ERR_STREAM_CLOSED: An invalid frame has been received in a closed stream.
+ //
+ // ERR_PROTO: The RFC 7540 specifies:
+ // "An endpoint that encounters a connection error SHOULD first send a GOAWAY
+ // frame (Section 6.8) with the stream identifier of the last stream that it
+ // successfully received from its peer.
+ // The GOAWAY frame includes an error code that indicates the type of error"
+ // The GOAWAY frame is already sent by nghttp2. We emit the error
+ // to liberate the Http2Session to destroy.
if (nghttp2_is_fatal(lib_error_code) ||
- lib_error_code == NGHTTP2_ERR_STREAM_CLOSED) {
+ lib_error_code == NGHTTP2_ERR_STREAM_CLOSED ||
+ lib_error_code == NGHTTP2_ERR_PROTO) {
Environment* env = session->env();
Isolate* isolate = env->isolate();
HandleScope scope(isolate);
@@ -1070,7 +1085,6 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
Debug(session, "frame type %d was not sent, code: %d",
frame->hd.type, error_code);

- // Do not report if the frame was not sent due to the session closing
if (error_code == NGHTTP2_ERR_SESSION_CLOSING ||
error_code == NGHTTP2_ERR_STREAM_CLOSED ||
error_code == NGHTTP2_ERR_STREAM_CLOSING) {
@@ -1079,7 +1093,15 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
// to destroy the session completely.
// Further information see: https://github.com/nodejs/node/issues/35233
session->DecrefHeaders(frame);
- return 0;
+ // Currently, nghttp2 doesn't not inform us when is the best
+ // time to call session.close(). It relies on a closing connection
+ // from peer. If that doesn't happen, the nghttp2_session will be
+ // closed but the Http2Session will still be up causing a memory leak.
+ // Therefore, if the GOAWAY frame couldn't be send due to
+ // ERR_SESSION_CLOSING we should force close from our side.
+ if (frame->hd.type != 0x03) {
+ return 0;
+ }
}

Isolate* isolate = env->isolate();
@@ -1145,12 +1167,15 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
// ignore these. If this callback was not provided, nghttp2 would handle
// invalid headers strictly and would shut down the stream. We are intentionally
// being more lenient here although we may want to revisit this choice later.
-int Http2Session::OnInvalidHeader(nghttp2_session* session,
+int Http2Session::OnInvalidHeader(nghttp2_session* handle,
const nghttp2_frame* frame,
nghttp2_rcbuf* name,
nghttp2_rcbuf* value,
uint8_t flags,
void* user_data) {
+ Http2Session* session = static_cast<Http2Session*>(user_data);
+ int32_t id = GetFrameID(frame);
+ Debug(session, "invalid header received for stream %d", id);
// Ignore invalid header fields by default.
return 0;
}
@@ -1544,6 +1569,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {

// Called by OnFrameReceived when a complete SETTINGS frame has been received.
void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
+ Debug(this, "handling settings frame");
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
if (!ack) {
js_fields_->bitfield &= ~(1 << kSessionRemoteSettingsIsUpToDate);
diff --git a/test/parallel/test-http2-connect-method-extended-cant-turn-off.js b/test/parallel/test-http2-connect-method-extended-cant-turn-off.js
index f4d033ef..456aa1ce 100644
--- a/test/parallel/test-http2-connect-method-extended-cant-turn-off.js
+++ b/test/parallel/test-http2-connect-method-extended-cant-turn-off.js
@@ -27,4 +27,10 @@ server.listen(0, common.mustCall(() => {
server.close();
}));
}));
+
+ client.on('error', common.expectsError({
+ code: 'ERR_HTTP2_ERROR',
+ name: 'Error',
+ message: 'Protocol error'
+ }));
}));
diff --git a/test/parallel/test-http2-options-max-headers-block-length.js b/test/parallel/test-http2-options-max-headers-block-length.js
index af1cc6f9..15b142ac 100644
--- a/test/parallel/test-http2-options-max-headers-block-length.js
+++ b/test/parallel/test-http2-options-max-headers-block-length.js
@@ -35,9 +35,11 @@ server.listen(0, common.mustCall(() => {
assert.strictEqual(code, h2.constants.NGHTTP2_FRAME_SIZE_ERROR);
}));

+ // NGHTTP2 will automatically send the NGHTTP2_REFUSED_STREAM with
+ // the GOAWAY frame.
req.on('error', common.expectsError({
code: 'ERR_HTTP2_STREAM_ERROR',
name: 'Error',
- message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR'
+ message: 'Stream closed with error code NGHTTP2_REFUSED_STREAM'
}));
}));
diff --git a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js
index df3aefff..7767dbbc 100644
--- a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js
+++ b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js
@@ -59,6 +59,9 @@ server.listen(0, common.mustCall(() => {
'session',
common.mustCall((session) => {
assert.strictEqual(session instanceof ServerHttp2Session, true);
+ session.on('close', common.mustCall(() => {
+ server.close();
+ }));
}),
);
server.on(
@@ -80,7 +83,6 @@ server.listen(0, common.mustCall(() => {
assert.strictEqual(err.name, 'Error');
assert.strictEqual(err.message, 'Session closed with error code 9');
assert.strictEqual(session instanceof ServerHttp2Session, true);
- server.close();
}),
);

--
2.45.2

5 changes: 3 additions & 2 deletions SPECS/nodejs/nodejs18.spec
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Source0: https://nodejs.org/download/release/v%{version}/node-v%{version}
Patch0: CVE-2023-21100.patch
Patch1: CVE-2024-21538.patch
Patch2: CVE-2025-22150.patch
Patch3: CVE-2025-23085.patch
BuildRequires: brotli-devel
BuildRequires: coreutils >= 8.22
BuildRequires: gcc
Expand Down Expand Up @@ -119,8 +120,8 @@ make cctest
%{_datadir}/systemtap/tapset/node.stp

%changelog
* Wed Feb 05 2025 Kanishk Bansal <[email protected]> - 18.20.3-3
- Patch CVE-2025-22150
* Tue Feb 11 2025 Kanishk Bansal <[email protected]> - 18.20.3-3
- Patch CVE-2025-22150, CVE-2025-23085

* Tue Nov 19 2024 Bala <[email protected]> - 18.20.3-2
- Patch CVE-2024-21538
Expand Down

0 comments on commit f958b19

Please sign in to comment.