Skip to content

Commit 092a3c2

Browse files
lundibundiaddaleax
authored andcommitted
http2: allow to configure maximum tolerated invalid frames
PR-URL: #30534 Fixes: #30505 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent e92afd9 commit 092a3c2

7 files changed

+181
-8
lines changed

doc/api/http2.md

+12
Original file line numberDiff line numberDiff line change
@@ -1941,6 +1941,9 @@ error will be thrown.
19411941
<!-- YAML
19421942
added: v8.4.0
19431943
changes:
1944+
- version: REPLACEME
1945+
pr-url: https://github.com/nodejs/node/pull/30534
1946+
description: Added `maxSessionInvalidFrames` option with a default of 1000.
19441947
- version: v13.0.0
19451948
pr-url: https://github.com/nodejs/node/pull/29144
19461949
description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to
@@ -2001,6 +2004,9 @@ changes:
20012004
streams for the remote peer as if a `SETTINGS` frame had been received. Will
20022005
be overridden if the remote peer sets its own value for
20032006
`maxConcurrentStreams`. **Default:** `100`.
2007+
* `maxSessionInvalidFrames` {integer} Sets the maximum number of invalid
2008+
frames that will be tolerated before the session is closed.
2009+
**Default:** `1000`.
20042010
* `settings` {HTTP/2 Settings Object} The initial settings to send to the
20052011
remote peer upon connection.
20062012
* `Http1IncomingMessage` {http.IncomingMessage} Specifies the
@@ -2053,6 +2059,9 @@ server.listen(80);
20532059
<!-- YAML
20542060
added: v8.4.0
20552061
changes:
2062+
- version: REPLACEME
2063+
pr-url: https://github.com/nodejs/node/pull/30534
2064+
description: Added `maxSessionInvalidFrames` option with a default of 1000.
20562065
- version: v13.0.0
20572066
pr-url: https://github.com/nodejs/node/pull/29144
20582067
description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to
@@ -2113,6 +2122,9 @@ changes:
21132122
streams for the remote peer as if a `SETTINGS` frame had been received. Will
21142123
be overridden if the remote peer sets its own value for
21152124
`maxConcurrentStreams`. **Default:** `100`.
2125+
* `maxSessionInvalidFrames` {integer} Sets the maximum number of invalid
2126+
frames that will be tolerated before the session is closed.
2127+
**Default:** `1000`.
21162128
* `settings` {HTTP/2 Settings Object} The initial settings to send to the
21172129
remote peer upon connection.
21182130
* ...: Any [`tls.createServer()`][] options can be provided. For

lib/internal/http2/core.js

+15-1
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,11 @@ const {
8181
},
8282
hideStackFrames
8383
} = require('internal/errors');
84-
const { validateNumber, validateString } = require('internal/validators');
84+
const { validateNumber,
85+
validateString,
86+
validateUint32,
87+
isUint32,
88+
} = require('internal/validators');
8589
const fsPromisesInternal = require('internal/fs/promises');
8690
const { utcDate } = require('internal/http');
8791
const { onServerStream,
@@ -199,6 +203,7 @@ const {
199203
kBitfield,
200204
kSessionPriorityListenerCount,
201205
kSessionFrameErrorListenerCount,
206+
kSessionMaxInvalidFrames,
202207
kSessionUint8FieldCount,
203208
kSessionHasRemoteSettingsListeners,
204209
kSessionRemoteSettingsIsUpToDate,
@@ -937,6 +942,12 @@ function setupHandle(socket, type, options) {
937942
this[kEncrypted] = false;
938943
}
939944

945+
if (isUint32(options.maxSessionInvalidFrames)) {
946+
const uint32 = new Uint32Array(
947+
this[kNativeFields].buffer, kSessionMaxInvalidFrames, 1);
948+
uint32[0] = options.maxSessionInvalidFrames;
949+
}
950+
940951
const settings = typeof options.settings === 'object' ?
941952
options.settings : {};
942953

@@ -2768,6 +2779,9 @@ function initializeOptions(options) {
27682779
assertIsObject(options.settings, 'options.settings');
27692780
options.settings = { ...options.settings };
27702781

2782+
if (options.maxSessionInvalidFrames !== undefined)
2783+
validateUint32(options.maxSessionInvalidFrames, 'maxSessionInvalidFrames');
2784+
27712785
// Used only with allowHTTP1
27722786
options.Http1IncomingMessage = options.Http1IncomingMessage ||
27732787
http.IncomingMessage;

src/node_http2.cc

+7-2
Original file line numberDiff line numberDiff line change
@@ -1011,8 +1011,12 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
10111011
void* user_data) {
10121012
Http2Session* session = static_cast<Http2Session*>(user_data);
10131013

1014-
Debug(session, "invalid frame received, code: %d", lib_error_code);
1015-
if (session->invalid_frame_count_++ > 1000)
1014+
Debug(session,
1015+
"invalid frame received (%u/%u), code: %d",
1016+
session->invalid_frame_count_,
1017+
session->js_fields_.max_invalid_frames,
1018+
lib_error_code);
1019+
if (session->invalid_frame_count_++ > session->js_fields_.max_invalid_frames)
10161020
return 1;
10171021

10181022
// If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
@@ -3057,6 +3061,7 @@ void Initialize(Local<Object> target,
30573061
NODE_DEFINE_CONSTANT(target, kBitfield);
30583062
NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount);
30593063
NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount);
3064+
NODE_DEFINE_CONSTANT(target, kSessionMaxInvalidFrames);
30603065
NODE_DEFINE_CONSTANT(target, kSessionUint8FieldCount);
30613066

30623067
NODE_DEFINE_CONSTANT(target, kSessionHasRemoteSettingsListeners);

src/node_http2.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,7 @@ typedef struct {
677677
uint8_t bitfield;
678678
uint8_t priority_listener_count;
679679
uint8_t frame_error_listener_count;
680+
uint32_t max_invalid_frames = 1000;
680681
} SessionJSFields;
681682

682683
// Indices for js_fields_, which serves as a way to communicate data with JS
@@ -689,6 +690,7 @@ enum SessionUint8Fields {
689690
offsetof(SessionJSFields, priority_listener_count),
690691
kSessionFrameErrorListenerCount =
691692
offsetof(SessionJSFields, frame_error_listener_count),
693+
kSessionMaxInvalidFrames = offsetof(SessionJSFields, max_invalid_frames),
692694
kSessionUint8FieldCount = sizeof(SessionJSFields)
693695
};
694696

@@ -1024,7 +1026,7 @@ class Http2Session : public AsyncWrap, public StreamListener {
10241026
// accepted again.
10251027
int32_t rejected_stream_count_ = 0;
10261028
// Also use the invalid frame count as a measure for rejecting input frames.
1027-
int32_t invalid_frame_count_ = 0;
1029+
uint32_t invalid_frame_count_ = 0;
10281030

10291031
void PushOutgoingBuffer(nghttp2_stream_write&& write);
10301032
void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length);

test/parallel/test-http2-createsecureserver-options.js

+29-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ if (!common.hasCrypto)
77
const assert = require('assert');
88
const http2 = require('http2');
99

10-
// Error if invalid options are passed to createSecureServer
10+
// Error if invalid options are passed to createSecureServer.
1111
const invalidOptions = [() => {}, 1, 'test', null, Symbol('test')];
1212
invalidOptions.forEach((invalidOption) => {
1313
assert.throws(
@@ -21,7 +21,7 @@ invalidOptions.forEach((invalidOption) => {
2121
);
2222
});
2323

24-
// Error if invalid options.settings are passed to createSecureServer
24+
// Error if invalid options.settings are passed to createSecureServer.
2525
invalidOptions.forEach((invalidSettingsOption) => {
2626
assert.throws(
2727
() => http2.createSecureServer({ settings: invalidSettingsOption }),
@@ -33,3 +33,30 @@ invalidOptions.forEach((invalidSettingsOption) => {
3333
}
3434
);
3535
});
36+
37+
// Test that http2.createSecureServer validates input options.
38+
Object.entries({
39+
maxSessionInvalidFrames: [
40+
{
41+
val: -1,
42+
err: {
43+
name: 'RangeError',
44+
code: 'ERR_OUT_OF_RANGE',
45+
},
46+
},
47+
{
48+
val: Number.NEGATIVE_INFINITY,
49+
err: {
50+
name: 'RangeError',
51+
code: 'ERR_OUT_OF_RANGE',
52+
},
53+
},
54+
],
55+
}).forEach(([opt, tests]) => {
56+
tests.forEach(({ val, err }) => {
57+
assert.throws(
58+
() => http2.createSecureServer({ [opt]: val }),
59+
err
60+
);
61+
});
62+
});

test/parallel/test-http2-createserver-options.js

+29-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ if (!common.hasCrypto)
77
const assert = require('assert');
88
const http2 = require('http2');
99

10-
// Error if invalid options are passed to createServer
10+
// Error if invalid options are passed to createServer.
1111
const invalidOptions = [1, true, 'test', null, Symbol('test')];
1212
invalidOptions.forEach((invalidOption) => {
1313
assert.throws(
@@ -21,7 +21,7 @@ invalidOptions.forEach((invalidOption) => {
2121
);
2222
});
2323

24-
// Error if invalid options.settings are passed to createServer
24+
// Error if invalid options.settings are passed to createServer.
2525
invalidOptions.forEach((invalidSettingsOption) => {
2626
assert.throws(
2727
() => http2.createServer({ settings: invalidSettingsOption }),
@@ -33,3 +33,30 @@ invalidOptions.forEach((invalidSettingsOption) => {
3333
}
3434
);
3535
});
36+
37+
// Test that http2.createServer validates input options.
38+
Object.entries({
39+
maxSessionInvalidFrames: [
40+
{
41+
val: -1,
42+
err: {
43+
name: 'RangeError',
44+
code: 'ERR_OUT_OF_RANGE',
45+
},
46+
},
47+
{
48+
val: Number.NEGATIVE_INFINITY,
49+
err: {
50+
name: 'RangeError',
51+
code: 'ERR_OUT_OF_RANGE',
52+
},
53+
},
54+
],
55+
}).forEach(([opt, tests]) => {
56+
tests.forEach(({ val, err }) => {
57+
assert.throws(
58+
() => http2.createServer({ [opt]: val }),
59+
err
60+
);
61+
});
62+
});
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
6+
const assert = require('assert');
7+
const http2 = require('http2');
8+
const net = require('net');
9+
10+
// Verify that creating a number of invalid HTTP/2 streams will
11+
// result in the peer closing the session within maxSessionInvalidFrames
12+
// frames.
13+
14+
const maxSessionInvalidFrames = 100;
15+
const server = http2.createServer({ maxSessionInvalidFrames });
16+
server.on('stream', (stream) => {
17+
stream.respond({
18+
'content-type': 'text/plain',
19+
':status': 200
20+
});
21+
stream.end('Hello, world!\n');
22+
});
23+
24+
server.listen(0, () => {
25+
const h2header = Buffer.alloc(9);
26+
const conn = net.connect(server.address().port);
27+
28+
conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n');
29+
30+
h2header[3] = 4; // Send a settings frame.
31+
conn.write(Buffer.from(h2header));
32+
33+
let inbuf = Buffer.alloc(0);
34+
let state = 'settingsHeader';
35+
let settingsFrameLength;
36+
conn.on('data', (chunk) => {
37+
inbuf = Buffer.concat([inbuf, chunk]);
38+
switch (state) {
39+
case 'settingsHeader':
40+
if (inbuf.length < 9) return;
41+
settingsFrameLength = inbuf.readIntBE(0, 3);
42+
inbuf = inbuf.slice(9);
43+
state = 'readingSettings';
44+
// Fallthrough
45+
case 'readingSettings':
46+
if (inbuf.length < settingsFrameLength) return;
47+
inbuf = inbuf.slice(settingsFrameLength);
48+
h2header[3] = 4; // Send a settings ACK.
49+
h2header[4] = 1;
50+
conn.write(Buffer.from(h2header));
51+
state = 'ignoreInput';
52+
writeRequests();
53+
}
54+
});
55+
56+
let gotError = false;
57+
let streamId = 1;
58+
let reqCount = 0;
59+
60+
function writeRequests() {
61+
for (let i = 1; i < 10 && !gotError; i++) {
62+
h2header[3] = 1; // HEADERS
63+
h2header[4] = 0x5; // END_HEADERS|END_STREAM
64+
h2header.writeIntBE(1, 0, 3); // Length: 1
65+
h2header.writeIntBE(streamId, 5, 4); // Stream ID
66+
streamId += 2;
67+
// 0x88 = :status: 200
68+
if (!conn.write(Buffer.concat([h2header, Buffer.from([0x88])]))) {
69+
break;
70+
}
71+
reqCount++;
72+
}
73+
// Timeout requests to slow down the rate so we get more accurate reqCount.
74+
if (!gotError)
75+
setTimeout(writeRequests, 10);
76+
}
77+
78+
conn.once('error', common.mustCall(() => {
79+
gotError = true;
80+
assert.ok(Math.abs(reqCount - maxSessionInvalidFrames) < 100,
81+
`Request count (${reqCount}) must be around (±100)` +
82+
` maxSessionInvalidFrames option (${maxSessionInvalidFrames})`);
83+
conn.destroy();
84+
server.close();
85+
}));
86+
});

0 commit comments

Comments
 (0)