diff options
| -rw-r--r-- | doc/api/http2.md | 12 | ||||
| -rw-r--r-- | lib/internal/http2/core.js | 16 | ||||
| -rw-r--r-- | src/node_http2.cc | 9 | ||||
| -rw-r--r-- | src/node_http2.h | 4 | ||||
| -rw-r--r-- | test/parallel/test-http2-createsecureserver-options.js | 31 | ||||
| -rw-r--r-- | test/parallel/test-http2-createserver-options.js | 31 | ||||
| -rw-r--r-- | test/parallel/test-http2-max-invalid-frames.js | 86 |
7 files changed, 181 insertions, 8 deletions
diff --git a/doc/api/http2.md b/doc/api/http2.md index 53461ac58d..7ac4f6ffa1 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1941,6 +1941,9 @@ error will be thrown. <!-- YAML added: v8.4.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/30534 + description: Added `maxSessionInvalidFrames` option with a default of 1000. - version: v13.0.0 pr-url: https://github.com/nodejs/node/pull/29144 description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to @@ -2001,6 +2004,9 @@ changes: streams for the remote peer as if a `SETTINGS` frame had been received. Will be overridden if the remote peer sets its own value for `maxConcurrentStreams`. **Default:** `100`. + * `maxSessionInvalidFrames` {integer} Sets the maximum number of invalid + frames that will be tolerated before the session is closed. + **Default:** `1000`. * `settings` {HTTP/2 Settings Object} The initial settings to send to the remote peer upon connection. * `Http1IncomingMessage` {http.IncomingMessage} Specifies the @@ -2053,6 +2059,9 @@ server.listen(80); <!-- YAML added: v8.4.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/30534 + description: Added `maxSessionInvalidFrames` option with a default of 1000. - version: v13.0.0 pr-url: https://github.com/nodejs/node/pull/29144 description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to @@ -2113,6 +2122,9 @@ changes: streams for the remote peer as if a `SETTINGS` frame had been received. Will be overridden if the remote peer sets its own value for `maxConcurrentStreams`. **Default:** `100`. + * `maxSessionInvalidFrames` {integer} Sets the maximum number of invalid + frames that will be tolerated before the session is closed. + **Default:** `1000`. * `settings` {HTTP/2 Settings Object} The initial settings to send to the remote peer upon connection. * ...: Any [`tls.createServer()`][] options can be provided. For diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 4060aff75e..7eaf1f4e92 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -90,7 +90,11 @@ const { }, hideStackFrames } = require('internal/errors'); -const { validateNumber, validateString } = require('internal/validators'); +const { validateNumber, + validateString, + validateUint32, + isUint32, +} = require('internal/validators'); const fsPromisesInternal = require('internal/fs/promises'); const { utcDate } = require('internal/http'); const { onServerStream, @@ -206,6 +210,7 @@ const { kBitfield, kSessionPriorityListenerCount, kSessionFrameErrorListenerCount, + kSessionMaxInvalidFrames, kSessionUint8FieldCount, kSessionHasRemoteSettingsListeners, kSessionRemoteSettingsIsUpToDate, @@ -944,6 +949,12 @@ function setupHandle(socket, type, options) { this[kEncrypted] = false; } + if (isUint32(options.maxSessionInvalidFrames)) { + const uint32 = new Uint32Array( + this[kNativeFields].buffer, kSessionMaxInvalidFrames, 1); + uint32[0] = options.maxSessionInvalidFrames; + } + const settings = typeof options.settings === 'object' ? options.settings : {}; @@ -2774,6 +2785,9 @@ function initializeOptions(options) { assertIsObject(options.settings, 'options.settings'); options.settings = { ...options.settings }; + if (options.maxSessionInvalidFrames !== undefined) + validateUint32(options.maxSessionInvalidFrames, 'maxSessionInvalidFrames'); + // Used only with allowHTTP1 options.Http1IncomingMessage = options.Http1IncomingMessage || http.IncomingMessage; diff --git a/src/node_http2.cc b/src/node_http2.cc index 4c1e5dd583..459cc3b0e6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1011,8 +1011,12 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, void* user_data) { Http2Session* session = static_cast<Http2Session*>(user_data); - Debug(session, "invalid frame received, code: %d", lib_error_code); - if (session->invalid_frame_count_++ > 1000) + Debug(session, + "invalid frame received (%u/%u), code: %d", + session->invalid_frame_count_, + session->js_fields_.max_invalid_frames, + lib_error_code); + if (session->invalid_frame_count_++ > session->js_fields_.max_invalid_frames) return 1; // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error @@ -3057,6 +3061,7 @@ void Initialize(Local<Object> target, NODE_DEFINE_CONSTANT(target, kBitfield); NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount); NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount); + NODE_DEFINE_CONSTANT(target, kSessionMaxInvalidFrames); NODE_DEFINE_CONSTANT(target, kSessionUint8FieldCount); NODE_DEFINE_CONSTANT(target, kSessionHasRemoteSettingsListeners); diff --git a/src/node_http2.h b/src/node_http2.h index 61092b60c0..79d648276f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -677,6 +677,7 @@ typedef struct { uint8_t bitfield; uint8_t priority_listener_count; uint8_t frame_error_listener_count; + uint32_t max_invalid_frames = 1000; } SessionJSFields; // Indices for js_fields_, which serves as a way to communicate data with JS @@ -689,6 +690,7 @@ enum SessionUint8Fields { offsetof(SessionJSFields, priority_listener_count), kSessionFrameErrorListenerCount = offsetof(SessionJSFields, frame_error_listener_count), + kSessionMaxInvalidFrames = offsetof(SessionJSFields, max_invalid_frames), kSessionUint8FieldCount = sizeof(SessionJSFields) }; @@ -1024,7 +1026,7 @@ class Http2Session : public AsyncWrap, public StreamListener { // accepted again. int32_t rejected_stream_count_ = 0; // Also use the invalid frame count as a measure for rejecting input frames. - int32_t invalid_frame_count_ = 0; + uint32_t invalid_frame_count_ = 0; void PushOutgoingBuffer(nghttp2_stream_write&& write); void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); diff --git a/test/parallel/test-http2-createsecureserver-options.js b/test/parallel/test-http2-createsecureserver-options.js index 4ef85a45b5..b97d6e5d6c 100644 --- a/test/parallel/test-http2-createsecureserver-options.js +++ b/test/parallel/test-http2-createsecureserver-options.js @@ -7,7 +7,7 @@ if (!common.hasCrypto) const assert = require('assert'); const http2 = require('http2'); -// Error if invalid options are passed to createSecureServer +// Error if invalid options are passed to createSecureServer. const invalidOptions = [() => {}, 1, 'test', null, Symbol('test')]; invalidOptions.forEach((invalidOption) => { assert.throws( @@ -21,7 +21,7 @@ invalidOptions.forEach((invalidOption) => { ); }); -// Error if invalid options.settings are passed to createSecureServer +// Error if invalid options.settings are passed to createSecureServer. invalidOptions.forEach((invalidSettingsOption) => { assert.throws( () => http2.createSecureServer({ settings: invalidSettingsOption }), @@ -33,3 +33,30 @@ invalidOptions.forEach((invalidSettingsOption) => { } ); }); + +// Test that http2.createSecureServer validates input options. +Object.entries({ + maxSessionInvalidFrames: [ + { + val: -1, + err: { + name: 'RangeError', + code: 'ERR_OUT_OF_RANGE', + }, + }, + { + val: Number.NEGATIVE_INFINITY, + err: { + name: 'RangeError', + code: 'ERR_OUT_OF_RANGE', + }, + }, + ], +}).forEach(([opt, tests]) => { + tests.forEach(({ val, err }) => { + assert.throws( + () => http2.createSecureServer({ [opt]: val }), + err + ); + }); +}); diff --git a/test/parallel/test-http2-createserver-options.js b/test/parallel/test-http2-createserver-options.js index d322506f55..47504a8a77 100644 --- a/test/parallel/test-http2-createserver-options.js +++ b/test/parallel/test-http2-createserver-options.js @@ -7,7 +7,7 @@ if (!common.hasCrypto) const assert = require('assert'); const http2 = require('http2'); -// Error if invalid options are passed to createServer +// Error if invalid options are passed to createServer. const invalidOptions = [1, true, 'test', null, Symbol('test')]; invalidOptions.forEach((invalidOption) => { assert.throws( @@ -21,7 +21,7 @@ invalidOptions.forEach((invalidOption) => { ); }); -// Error if invalid options.settings are passed to createServer +// Error if invalid options.settings are passed to createServer. invalidOptions.forEach((invalidSettingsOption) => { assert.throws( () => http2.createServer({ settings: invalidSettingsOption }), @@ -33,3 +33,30 @@ invalidOptions.forEach((invalidSettingsOption) => { } ); }); + +// Test that http2.createServer validates input options. +Object.entries({ + maxSessionInvalidFrames: [ + { + val: -1, + err: { + name: 'RangeError', + code: 'ERR_OUT_OF_RANGE', + }, + }, + { + val: Number.NEGATIVE_INFINITY, + err: { + name: 'RangeError', + code: 'ERR_OUT_OF_RANGE', + }, + }, + ], +}).forEach(([opt, tests]) => { + tests.forEach(({ val, err }) => { + assert.throws( + () => http2.createServer({ [opt]: val }), + err + ); + }); +}); diff --git a/test/parallel/test-http2-max-invalid-frames.js b/test/parallel/test-http2-max-invalid-frames.js new file mode 100644 index 0000000000..597bd8e811 --- /dev/null +++ b/test/parallel/test-http2-max-invalid-frames.js @@ -0,0 +1,86 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const http2 = require('http2'); +const net = require('net'); + +// Verify that creating a number of invalid HTTP/2 streams will +// result in the peer closing the session within maxSessionInvalidFrames +// frames. + +const maxSessionInvalidFrames = 100; +const server = http2.createServer({ maxSessionInvalidFrames }); +server.on('stream', (stream) => { + stream.respond({ + 'content-type': 'text/plain', + ':status': 200 + }); + stream.end('Hello, world!\n'); +}); + +server.listen(0, () => { + const h2header = Buffer.alloc(9); + const conn = net.connect(server.address().port); + + conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'); + + h2header[3] = 4; // Send a settings frame. + conn.write(Buffer.from(h2header)); + + let inbuf = Buffer.alloc(0); + let state = 'settingsHeader'; + let settingsFrameLength; + conn.on('data', (chunk) => { + inbuf = Buffer.concat([inbuf, chunk]); + switch (state) { + case 'settingsHeader': + if (inbuf.length < 9) return; + settingsFrameLength = inbuf.readIntBE(0, 3); + inbuf = inbuf.slice(9); + state = 'readingSettings'; + // Fallthrough + case 'readingSettings': + if (inbuf.length < settingsFrameLength) return; + inbuf = inbuf.slice(settingsFrameLength); + h2header[3] = 4; // Send a settings ACK. + h2header[4] = 1; + conn.write(Buffer.from(h2header)); + state = 'ignoreInput'; + writeRequests(); + } + }); + + let gotError = false; + let streamId = 1; + let reqCount = 0; + + function writeRequests() { + for (let i = 1; i < 10 && !gotError; i++) { + h2header[3] = 1; // HEADERS + h2header[4] = 0x5; // END_HEADERS|END_STREAM + h2header.writeIntBE(1, 0, 3); // Length: 1 + h2header.writeIntBE(streamId, 5, 4); // Stream ID + streamId += 2; + // 0x88 = :status: 200 + if (!conn.write(Buffer.concat([h2header, Buffer.from([0x88])]))) { + break; + } + reqCount++; + } + // Timeout requests to slow down the rate so we get more accurate reqCount. + if (!gotError) + setTimeout(writeRequests, 10); + } + + conn.once('error', common.mustCall(() => { + gotError = true; + assert.ok(Math.abs(reqCount - maxSessionInvalidFrames) < 100, + `Request count (${reqCount}) must be around (±100)` + + ` maxSessionInvalidFrames option (${maxSessionInvalidFrames})`); + conn.destroy(); + server.close(); + })); +}); |