From 3c7686163ed4c6ae3e5901b758b7a7d4fd5bb0c0 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Tue, 17 Dec 2024 16:58:03 -0300 Subject: [PATCH] src: fix HTTP2 mem leak on premature close and ERR_PROTO This commit fixes a memory leak when the socket is suddenly closed by the peer (without GOAWAY notification) and when invalid header (by nghttp2) is identified and the connection is terminated by peer. Refs: https://hackerone.com/reports/2841362 PR-URL: https://github.com/nodejs-private/node-private/pull/650 Reviewed-By: James M Snell CVE-ID: CVE-2025-23085 --- lib/internal/http2/core.js | 15 +++- src/node_http2.cc | 36 ++++++-- ...2-connect-method-extended-cant-turn-off.js | 6 ++ .../test-http2-invalid-last-stream-id.js | 77 ++++++++++++++++ ...-http2-options-max-headers-block-length.js | 4 +- ...tp2-options-max-headers-exceeds-nghttp2.js | 4 +- test/parallel/test-http2-premature-close.js | 88 +++++++++++++++++++ 7 files changed, 220 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-http2-invalid-last-stream-id.js create mode 100644 test/parallel/test-http2-premature-close.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index b41e1baee24644..0fdf516baafd74 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -614,11 +614,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 91e9011cd91c6a..29140148eec627 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -858,6 +858,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(stream); size_t size = streams_.size(); @@ -868,6 +869,7 @@ void Http2Session::AddStream(Http2Stream* stream) { BaseObjectPtr Http2Session::RemoveStream(int32_t id) { + Debug(this, "Removing stream: %d", id); BaseObjectPtr stream; if (streams_.empty()) return stream; @@ -1044,6 +1046,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, if (!stream) [[unlikely]] 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 @@ -1113,9 +1116,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); @@ -1178,7 +1193,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) { @@ -1187,7 +1201,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(); @@ -1253,12 +1275,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(user_data); + int32_t id = GetFrameID(frame); + Debug(session, "invalid header received for stream %d", id); // Ignore invalid header fields by default. return 0; } @@ -1654,6 +1679,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 f4d033efe65707..456aa1ce10d627 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-invalid-last-stream-id.js b/test/parallel/test-http2-invalid-last-stream-id.js new file mode 100644 index 00000000000000..c6e4e78dfb82ed --- /dev/null +++ b/test/parallel/test-http2-invalid-last-stream-id.js @@ -0,0 +1,77 @@ +// Flags: --expose-internals +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) common.skip('missing crypto'); + +const h2 = require('http2'); +const net = require('net'); +const assert = require('assert'); +const { ServerHttp2Session } = require('internal/http2/core'); + +async function sendInvalidLastStreamId(server) { + const client = new net.Socket(); + + const address = server.address(); + if (!common.hasIPv6 && address.family === 'IPv6') { + // Necessary to pass CI running inside containers. + client.connect(address.port); + } else { + client.connect(address); + } + + client.on('connect', common.mustCall(function() { + // HTTP/2 preface + client.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'utf8')); + + // Empty SETTINGS frame + client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00])); + + // GOAWAY frame with custom debug message + const goAwayFrame = [ + 0x00, 0x00, 0x21, // Length: 33 bytes + 0x07, // Type: GOAWAY + 0x00, // Flags + 0x00, 0x00, 0x00, 0x00, // Stream ID: 0 + 0x00, 0x00, 0x00, 0x01, // Last Stream ID: 1 + 0x00, 0x00, 0x00, 0x00, // Error Code: 0 (No error) + ]; + + // Add the debug message + const debugMessage = 'client transport shutdown'; + const goAwayBuffer = Buffer.concat([ + Buffer.from(goAwayFrame), + Buffer.from(debugMessage, 'utf8'), + ]); + + client.write(goAwayBuffer); + client.destroy(); + })); +} + +const server = h2.createServer(); + +server.on('error', common.mustNotCall()); + +server.on( + 'sessionError', + common.mustCall((err, session) => { + // When destroying the session, on Windows, we would get ECONNRESET + // errors, make sure we take those into account in our tests. + if (err.code !== 'ECONNRESET') { + assert.strictEqual(err.code, 'ERR_HTTP2_ERROR'); + assert.strictEqual(err.name, 'Error'); + assert.strictEqual(err.message, 'Protocol error'); + assert.strictEqual(session instanceof ServerHttp2Session, true); + } + session.close(); + server.close(); + }), +); + +server.listen( + 0, + common.mustCall(async () => { + await sendInvalidLastStreamId(server); + }), +); 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 af1cc6f9bc4860..15b142ac89b811 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 df3aefff11e7ad..7767dbbc503814 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(); }), ); diff --git a/test/parallel/test-http2-premature-close.js b/test/parallel/test-http2-premature-close.js new file mode 100644 index 00000000000000..a9b08f55d8a3b8 --- /dev/null +++ b/test/parallel/test-http2-premature-close.js @@ -0,0 +1,88 @@ +// Flags: --expose-internals +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) common.skip('missing crypto'); + +const h2 = require('http2'); +const net = require('net'); + +async function requestAndClose(server) { + const client = new net.Socket(); + + const address = server.address(); + if (!common.hasIPv6 && address.family === 'IPv6') { + // Necessary to pass CI running inside containers. + client.connect(address.port); + } else { + client.connect(address); + } + + client.on('connect', common.mustCall(function() { + // Send HTTP/2 Preface + client.write(Buffer.from('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'utf8')); + + // Send a SETTINGS frame (empty payload) + client.write(Buffer.from([0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00])); + + const streamId = 1; + // Send a valid HEADERS frame + const headersFrame = Buffer.concat([ + Buffer.from([ + 0x00, 0x00, 0x0c, // Length: 12 bytes + 0x01, // Type: HEADERS + 0x05, // Flags: END_HEADERS + END_STREAM + (streamId >> 24) & 0xFF, // Stream ID: high byte + (streamId >> 16) & 0xFF, + (streamId >> 8) & 0xFF, + streamId & 0xFF, // Stream ID: low byte + ]), + Buffer.from([ + 0x82, // Indexed Header Field Representation (Predefined ":method: GET") + 0x84, // Indexed Header Field Representation (Predefined ":path: /") + 0x86, // Indexed Header Field Representation (Predefined ":scheme: http") + 0x44, 0x0a, // Custom ":authority: localhost" + 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74, + ]), + ]); + client.write(headersFrame); + + // Send a valid DATA frame + const dataFrame = Buffer.concat([ + Buffer.from([ + 0x00, 0x00, 0x05, // Length: 5 bytes + 0x00, // Type: DATA + 0x00, // Flags: No flags + (streamId >> 24) & 0xFF, // Stream ID: high byte + (streamId >> 16) & 0xFF, + (streamId >> 8) & 0xFF, + streamId & 0xFF, // Stream ID: low byte + ]), + Buffer.from('Hello', 'utf8'), // Data payload + ]); + client.write(dataFrame); + + // Does not wait for server to reply. Shutdown the socket + client.end(); + })); +} + +const server = h2.createServer(); + +server.on('error', common.mustNotCall()); + +server.on( + 'session', + common.mustCall((session) => { + session.on('close', common.mustCall(() => { + server.close(); + })); + }), +); + +server.listen( + 0, + common.mustCall(async () => { + await requestAndClose(server); + }), +);