FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: fix integration issues with the latest nghttp2 · nodejs/node@a8a0d12 · GitHub

/ node Public

Commit a8a0d12

Browse files
pimterry
authored and
aduh95
committed
deps: fix integration issues with the latest nghttp2
This is a set of src & tests fixes for nghttp2 due to changes in v1.67.0+ which require a selection of changes to how we handle low-level protocol errors when using the latest versions of nghttp2, changing both some src error handling and updating some tests to match. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #62891 Backport-PR-URL: #63164 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Refs: https://hackerone.com/reports/3658225 CVE-ID: CVE-2026-48937
1 parent 66e6203 commit a8a0d12

6 files changed

Lines changed: 172 additions & 22 deletions

‎src/node_http2.cc‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,6 +1265,24 @@ int Http2Session::OnFrameSent(nghttp2_session* handle,
12651265
void* user_data) {
12661266
Http2Session* session = static_cast<Http2Session*>(user_data);
12671267
session->statistics_.frame_sent += 1;
1268+
1269+
// If nghttp2 has internally terminated the session (e.g. due to a protocol
1270+
// error like oversized frames, padding errors, or HPACK compression
1271+
// failures), it calls nghttp2_session_terminate_session() directly which
1272+
// queues a GOAWAY but does not invoke any application-level callback.
1273+
// Detect that case here: a GOAWAY was sent but we never initiated it
1274+
// (no Close(), no session.close(), no session.goaway()).
1275+
//
1276+
// We set a flag here, and then throw the error at the end of
1277+
// SendPendingData, to wait until the GOAWAY is written before the session
1278+
// is torn down.
1279+
if (frame->hd.type == NGHTTP2_GOAWAY && !session->is_closing() &&
1280+
!session->is_destroyed() && !session->IsGracefulCloseInitiated() &&
1281+
!session->goaway_initiated_) {
1282+
Debug(session, "nghttp2 session terminated internally");
1283+
session->internal_goaway_sent_ = true;
1284+
}
1285+
12681286
return 0;
12691287
}
12701288

@@ -1999,6 +2017,18 @@ uint8_t Http2Session::SendPendingData() {
19992017

20002018
MaybeStopReading();
20012019

2020+
// If nghttp2 has internally torn down the session (detected in OnFrameSent)
2021+
// during the nghttp2_session_mem_send loop above, at this point we error:
2022+
if (internal_goaway_sent_) {
2023+
internal_goaway_sent_ = false;
2024+
if (!is_closing() && !is_destroyed()) {
2025+
Isolate* isolate = env()->isolate();
2026+
HandleScope scope(isolate);
2027+
Local<Value> arg = Integer::New(isolate, NGHTTP2_ERR_PROTO);
2028+
MakeCallback(env()->http2session_on_error_function(), 1, &arg);
2029+
}
2030+
}
2031+
20022032
return 0;
20032033
}
20042034

@@ -2940,6 +2970,7 @@ void Http2Session::Goaway(uint32_t code,
29402970
if (is_destroyed())
29412971
return;
29422972

2973+
goaway_initiated_ = true;
29432974
Http2Scope h2scope(this);
29442975
// the last proc stream id is the most recently created Http2Stream.
29452976
if (lastStreamID <= 0)

‎src/node_http2.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -971,6 +971,8 @@ class Http2Session : public AsyncWrap,
971971

972972
// Flag to indicate that JavaScript has initiated a graceful closure
973973
bool graceful_close_initiated_ = false;
974+
bool goaway_initiated_ = false;
975+
bool internal_goaway_sent_ = false;
974976
};
975977

976978
struct Http2SessionPerformanceEntryTraits {

‎test/parallel/test-http2-misbehaving-flow-control-paused.js‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,23 +47,29 @@ const data = Buffer.from([
4747
// Bad client! Bad!
4848
//
4949
// Fortunately, nghttp2 handles this situation for us by keeping track
50-
// of the flow control window and responding with a FLOW_CONTROL_ERROR
51-
// causing the stream to get shut down...
52-
//
53-
// At least, that's what is supposed to happen.
50+
// of the flow control window and sending GOAWAY to end the session.
5451

5552
let client;
5653

5754
const server = h2.createServer({ settings: { initialWindowSize: 36 } });
55+
56+
server.on('session', common.mustCall((session) => {
57+
session.on('error', common.expectsError({
58+
code: 'ERR_HTTP2_ERROR',
59+
name: 'Error',
60+
message: 'Protocol error'
61+
}));
62+
}));
63+
5864
server.on('stream', common.mustCall((stream) => {
5965
// Set the high water mark to zero, since otherwise we still accept
6066
// reads from the source stream (if we can consume them).
6167
stream._readableState.highWaterMark = 0;
6268
stream.pause();
6369
stream.on('error', common.expectsError({
64-
code: 'ERR_HTTP2_STREAM_ERROR',
70+
code: 'ERR_HTTP2_ERROR',
6571
name: 'Error',
66-
message: 'Stream closed with error code NGHTTP2_FLOW_CONTROL_ERROR'
72+
message: 'Protocol error'
6773
}));
6874
stream.on('close', common.mustCall(() => {
6975
server.close();
@@ -72,11 +78,14 @@ server.on('stream', common.mustCall((stream) => {
7278
stream.on('end', common.mustNotCall());
7379
}));
7480

75-
server.listen(0, () => {
76-
client = net.connect(server.address().port, () => {
81+
server.listen(0, common.mustCall(() => {
82+
client = net.connect(server.address().port, common.mustCall(() => {
7783
client.write(preamble);
7884
client.write(data);
7985
client.write(data);
8086
client.write(data);
81-
});
82-
});
87+
88+
// TCP connection is closed by the server
89+
client.on('close', common.mustCall());
90+
}));
91+
}));

‎test/parallel/test-http2-misbehaving-flow-control.js‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,24 @@ const data = Buffer.from([
5858
// Bad client! Bad!
5959
//
6060
// Fortunately, nghttp2 handles this situation for us by keeping track
61-
// of the flow control window and responding with a FLOW_CONTROL_ERROR
62-
// causing the stream to get shut down...
63-
//
64-
// At least, that's what is supposed to happen.
61+
// of the flow control window and sending GOAWAY to end the session.
6562

6663
let client;
6764
const server = h2.createServer({ settings: { initialWindowSize: 18 } });
65+
66+
server.on('session', common.mustCall((session) => {
67+
session.on('error', common.expectsError({
68+
code: 'ERR_HTTP2_ERROR',
69+
name: 'Error',
70+
message: 'Protocol error'
71+
}));
72+
}));
73+
6874
server.on('stream', common.mustCall((stream) => {
6975
stream.on('error', common.expectsError({
70-
code: 'ERR_HTTP2_STREAM_ERROR',
76+
code: 'ERR_HTTP2_ERROR',
7177
name: 'Error',
72-
message: 'Stream closed with error code NGHTTP2_FLOW_CONTROL_ERROR'
78+
message: 'Protocol error'
7379
}));
7480
stream.on('close', common.mustCall(() => {
7581
server.close(common.mustCall());
@@ -82,10 +88,13 @@ server.on('stream', common.mustCall((stream) => {
8288

8389
server.on('close', common.mustCall());
8490

85-
server.listen(0, () => {
86-
client = net.connect(server.address().port, () => {
91+
server.listen(0, common.mustCall(() => {
92+
client = net.connect(server.address().port, common.mustCall(() => {
8793
client.write(preamble);
8894
client.write(data);
8995
client.write(data);
90-
});
91-
});
96+
97+
// TCP connection is closed by the server
98+
client.on('close', common.mustCall());
99+
}));
100+
}));

‎test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,19 @@ server.listen(0, common.mustCall(() => {
9090
0,
9191
common.mustCall(() => {
9292
const client = h2.connect(`http://localhost:${server.address().port}`);
93-
client.on('error', common.mustNotCall());
93+
// The server sends oversized headers that cause a compression error on
94+
// the client side, so nghttp2 internally terminates the client session.
95+
client.on('error', common.expectsError({
96+
code: 'ERR_HTTP2_ERROR',
97+
name: 'Error',
98+
}));
9499

95100
const req = client.request();
96101
req.on('response', common.mustNotCall());
97-
req.on('error', common.mustNotCall());
102+
req.on('error', common.expectsError({
103+
code: 'ERR_HTTP2_ERROR',
104+
name: 'Error',
105+
}));
98106
req.end();
99107
}),
100108
);

‎test/parallel/test-http2-session-cleanup-on-nghttp2-goaway.js‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const http2 = require('http2');
9+
const net = require('net');
10+
11+
// When nghttp2 internally sends a GOAWAY frame due to a protocol error, it
12+
// may call nghttp2_session_terminate_session() directly, bypassing the
13+
// on_invalid_frame_recv_callback entirely. This test ensures that even
14+
// in that scenario, we still correctly clean up the session & connection.
15+
//
16+
// This test reproduces this with a client who sends a frame header with
17+
// a length exceeding the default max_frame_size (16384). nghttp2 responds
18+
// with GOAWAY(FRAME_SIZE_ERROR) without notifying Node through any callback.
19+
20+
const server = http2.createServer();
21+
22+
server.on('session', common.mustCall((session) => {
23+
session.on('error', common.expectsError({
24+
code: 'ERR_HTTP2_ERROR',
25+
name: 'Error',
26+
message: 'Protocol error'
27+
}));
28+
29+
session.on('close', common.mustCall(() => server.close()));
30+
}));
31+
32+
server.listen(0, common.mustCall(() => {
33+
const conn = net.connect({
34+
port: server.address().port,
35+
allowHalfOpen: true,
36+
});
37+
38+
// HTTP/2 client connection preface.
39+
conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n');
40+
41+
// Empty SETTINGS frame.
42+
const settingsFrame = Buffer.alloc(9);
43+
settingsFrame[3] = 0x04; // type: SETTINGS
44+
conn.write(settingsFrame);
45+
46+
let inbuf = Buffer.alloc(0);
47+
let state = 'settingsHeader';
48+
let settingsFrameLength;
49+
50+
conn.on('data', (chunk) => {
51+
inbuf = Buffer.concat([inbuf, chunk]);
52+
53+
switch (state) {
54+
case 'settingsHeader':
55+
if (inbuf.length < 9) return;
56+
settingsFrameLength = inbuf.readUIntBE(0, 3);
57+
inbuf = inbuf.slice(9);
58+
state = 'readingSettings';
59+
// Fallthrough
60+
case 'readingSettings': {
61+
if (inbuf.length < settingsFrameLength) return;
62+
inbuf = inbuf.slice(settingsFrameLength);
63+
state = 'done';
64+
65+
// ACK the server SETTINGS.
66+
const ack = Buffer.alloc(9);
67+
ack[3] = 0x04; // type: SETTINGS
68+
ack[4] = 0x01; // flag: ACK
69+
conn.write(ack);
70+
71+
// Send a HEADERS frame header claiming length 16385, which exceeds
72+
// the default max_frame_size of 16384. nghttp2 checks the length
73+
// before reading any payload, so no body is needed. This triggers
74+
// nghttp2_session_terminate_session(FRAME_SIZE_ERROR) directly in
75+
// nghttp2_session_mem_recv2 — bypassing on_invalid_frame_recv_callback.
76+
const oversized = Buffer.alloc(9);
77+
oversized.writeUIntBE(16385, 0, 3); // length: 16385 (one over max)
78+
oversized[3] = 0x01; // type: HEADERS
79+
oversized[4] = 0x04; // flags: END_HEADERS
80+
oversized.writeUInt32BE(1, 5); // stream id: 1
81+
conn.write(oversized);
82+
83+
// No need to write the data - the header alone triggers the check.
84+
}
85+
}
86+
});
87+
88+
// The server must close the connection after sending GOAWAY:
89+
conn.on('end', common.mustCall(() => conn.end()));
90+
conn.on('close', common.mustCall());
91+
}));

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL