Skip to content

Commit

Permalink
http2: allow to configure maximum tolerated invalid frames
Browse files Browse the repository at this point in the history
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
lundibundi authored and targos committed Jan 13, 2020
1 parent 46cb0da commit 3bed1fa
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 8 deletions.
12 changes: 12 additions & 0 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -1939,6 +1939,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: v12.4.0
pr-url: https://github.com/nodejs/node/pull/27782
description: The `options` parameter now supports `net.createServer()`
Expand Down Expand Up @@ -1999,6 +2002,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`.
* `selectPadding` {Function} When `options.paddingStrategy` is equal to
`http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function
used to determine the padding. See [Using `options.selectPadding()`][].
Expand Down Expand Up @@ -2054,6 +2060,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: v10.12.0
pr-url: https://github.com/nodejs/node/pull/22956
description: Added the `origins` option to automatically send an `ORIGIN`
Expand Down Expand Up @@ -2114,6 +2123,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`.
* `selectPadding` {Function} When `options.paddingStrategy` is equal to
`http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function
used to determine the padding. See [Using `options.selectPadding()`][].
Expand Down
16 changes: 15 additions & 1 deletion lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,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,
Expand Down Expand Up @@ -207,6 +211,7 @@ const {
kBitfield,
kSessionPriorityListenerCount,
kSessionFrameErrorListenerCount,
kSessionMaxInvalidFrames,
kSessionUint8FieldCount,
kSessionHasRemoteSettingsListeners,
kSessionRemoteSettingsIsUpToDate,
Expand Down Expand Up @@ -959,6 +964,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 : {};

Expand Down Expand Up @@ -2790,6 +2801,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;
Expand Down
10 changes: 8 additions & 2 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1045,8 +1045,13 @@ 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 &&
!IsReverted(SECURITY_REVERT_CVE_2019_9514)) {
return 1;
}
Expand Down Expand Up @@ -3105,6 +3110,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);
Expand Down
4 changes: 3 additions & 1 deletion src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,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
Expand All @@ -691,6 +692,7 @@ enum SessionUint8Fields {
offsetof(SessionJSFields, priority_listener_count),
kSessionFrameErrorListenerCount =
offsetof(SessionJSFields, frame_error_listener_count),
kSessionMaxInvalidFrames = offsetof(SessionJSFields, max_invalid_frames),
kSessionUint8FieldCount = sizeof(SessionJSFields)
};

Expand Down Expand Up @@ -1028,7 +1030,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);
Expand Down
31 changes: 29 additions & 2 deletions test/parallel/test-http2-createsecureserver-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 }),
Expand All @@ -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
);
});
});
31 changes: 29 additions & 2 deletions test/parallel/test-http2-createserver-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 }),
Expand All @@ -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
);
});
});
86 changes: 86 additions & 0 deletions test/parallel/test-http2-max-invalid-frames.js
Original file line number Diff line number Diff line change
@@ -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();
}));
});

0 comments on commit 3bed1fa

Please sign in to comment.