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. [backport] - test remove assert.strictEqual(session instanceof ServerHttp2Session, true); that fail on old version without ponyfill - force non specific error in order to avoid an assert 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 origin: backport, https://github.com/nodejs/node/commit/6cc8d58e6f97c37c228f134bd9b98246c8871fb1 bug: https://nodejs.org/en/blog/vulnerability/january-2025-security-releases#goaway-http2-frames-cause-memory-leak-outside-heap-cve-2025-23085---medium --- lib/internal/http2/core.js | 14 +++- src/node_http2.cc | 37 +++++++-- ...-http2-connect-method-extended-cant-turn-off.js | 6 ++ test/parallel/test-http2-invalid-last-stream-id.js | 76 +++++++++++++++++++ .../test-http2-options-max-headers-block-length.js | 2 + test/parallel/test-http2-premature-close.js | 88 ++++++++++++++++++++++ 6 files changed, 216 insertions(+), 7 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 1c6767c..d7caae3 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -573,9 +573,21 @@ 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); + + // 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(() => { + if(stream) + stream.close(NGHTTP2_INTERNAL_ERROR); + session.close(); + }); } function onAltSvc(stream, origin, alt) { diff --git a/src/node_http2.cc b/src/node_http2.cc index c441921..6365734 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -742,6 +742,7 @@ inline bool Http2Session::CanAddStream() { } inline void Http2Session::AddStream(Http2Stream* stream) { + Debug(this, "Adding stream: %d", stream->id()); CHECK_GE(++statistics_.stream_count, 0); streams_[stream->id()] = stream; size_t size = streams_.size(); @@ -750,10 +751,10 @@ inline void Http2Session::AddStream(Http2Stream* stream) { IncrementCurrentSessionMemory(sizeof(*stream)); } - inline void Http2Session::RemoveStream(Http2Stream* stream) { if (streams_.empty() || stream == nullptr) return; // Nothing to remove, item was never added? + Debug(this, "Removing stream: %d", stream->id()); streams_.erase(stream->id()); DecrementCurrentSessionMemory(sizeof(*stream)); } @@ -926,6 +927,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, if (UNLIKELY(stream == nullptr)) 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->IsDestroyed() && !stream->AddHeader(name, value, flags)) { // This will only happen if the connected peer sends us more @@ -995,9 +997,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); @@ -1024,12 +1038,19 @@ 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 || session->js_fields_.frame_error_listener_count == 0) { - 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(); @@ -1095,12 +1116,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; } @@ -1494,6 +1518,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 f4d033e..456aa1c 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 0000000..8e7beae --- /dev/null +++ b/test/parallel/test-http2-invalid-last-stream-id.js @@ -0,0 +1,76 @@ +// 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'); + } + 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 11632c6..a74e1e1 100644 --- a/test/parallel/test-http2-options-max-headers-block-length.js +++ b/test/parallel/test-http2-options-max-headers-block-length.js @@ -35,6 +35,8 @@ server.listen(0, common.mustCall(() => { assert.strictEqual(code, h2.constants.NGHTTP2_ERR_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', diff --git a/test/parallel/test-http2-premature-close.js b/test/parallel/test-http2-premature-close.js new file mode 100644 index 0000000..a9b08f5 --- /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); + }), +);