Skip to content

Commit

Permalink
src: fix HTTP2 mem leak on premature close and ERR_PROTO
Browse files Browse the repository at this point in the history
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: nodejs-private/node-private#650
Reviewed-By: James M Snell <[email protected]>
CVE-ID: CVE-2025-23085
  • Loading branch information
RafaelGSS committed Jan 20, 2025
1 parent 0afc6f9 commit 3c76861
Show file tree
Hide file tree
Showing 7 changed files with 220 additions and 10 deletions.
15 changes: 12 additions & 3 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
36 changes: 31 additions & 5 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Http2Stream>(stream);
size_t size = streams_.size();
Expand All @@ -868,6 +869,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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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();
Expand Down Expand Up @@ -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<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;
}
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}));
}));
77 changes: 77 additions & 0 deletions test/parallel/test-http2-invalid-last-stream-id.js
Original file line number Diff line number Diff line change
@@ -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);
}),
);
4 changes: 3 additions & 1 deletion test/parallel/test-http2-options-max-headers-block-length.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}));
}));
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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();
}),
);

Expand Down
88 changes: 88 additions & 0 deletions test/parallel/test-http2-premature-close.js
Original file line number Diff line number Diff line change
@@ -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);
}),
);

0 comments on commit 3c76861

Please sign in to comment.