From 46c1da13857ec5d7deae917ba23d0c19c7723f11 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 14 Aug 2019 11:32:06 +0200 Subject: [PATCH 01/13] deps: update nghttp2 to 1.39.2 This includes mitigations for CVE-2019-9512/CVE-2019-9515. --- deps/nghttp2/lib/includes/nghttp2/nghttp2.h | 11 +++++++++++ deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 ++-- deps/nghttp2/lib/nghttp2_option.c | 5 +++++ deps/nghttp2/lib/nghttp2_option.h | 5 +++++ deps/nghttp2/lib/nghttp2_session.c | 9 +++++++-- deps/nghttp2/lib/nghttp2_session.h | 8 ++++++-- 6 files changed, 36 insertions(+), 6 deletions(-) diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h index 925a4cbcaf6d6a..313fb23daa7449 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h @@ -2648,6 +2648,17 @@ nghttp2_option_set_max_deflate_dynamic_table_size(nghttp2_option *option, NGHTTP2_EXTERN void nghttp2_option_set_no_closed_streams(nghttp2_option *option, int val); +/** + * @function + * + * This function sets the maximum number of outgoing SETTINGS ACK and + * PING ACK frames retained in :type:`nghttp2_session` object. If + * more than those frames are retained, the peer is considered to be + * misbehaving and session will be closed. The default value is 1000. + */ +NGHTTP2_EXTERN void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, + size_t val); + /** * @function * diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 210cfaa16ae816..1f1d4808ca27c0 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.39.1" +#define NGHTTP2_VERSION "1.33.0" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012701 +#define NGHTTP2_VERSION_NUM 0x012100 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_option.c b/deps/nghttp2/lib/nghttp2_option.c index 8946d7dd38cfb8..e53f22d367f84a 100644 --- a/deps/nghttp2/lib/nghttp2_option.c +++ b/deps/nghttp2/lib/nghttp2_option.c @@ -116,3 +116,8 @@ void nghttp2_option_set_no_closed_streams(nghttp2_option *option, int val) { option->opt_set_mask |= NGHTTP2_OPT_NO_CLOSED_STREAMS; option->no_closed_streams = val; } + +void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, size_t val) { + option->opt_set_mask |= NGHTTP2_OPT_MAX_OUTBOUND_ACK; + option->max_outbound_ack = val; +} diff --git a/deps/nghttp2/lib/nghttp2_option.h b/deps/nghttp2/lib/nghttp2_option.h index 29e72aa321007a..1f740aaa6e364e 100644 --- a/deps/nghttp2/lib/nghttp2_option.h +++ b/deps/nghttp2/lib/nghttp2_option.h @@ -66,6 +66,7 @@ typedef enum { NGHTTP2_OPT_MAX_SEND_HEADER_BLOCK_LENGTH = 1 << 8, NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE = 1 << 9, NGHTTP2_OPT_NO_CLOSED_STREAMS = 1 << 10, + NGHTTP2_OPT_MAX_OUTBOUND_ACK = 1 << 11, } nghttp2_option_flag; /** @@ -80,6 +81,10 @@ struct nghttp2_option { * NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE */ size_t max_deflate_dynamic_table_size; + /** + * NGHTTP2_OPT_MAX_OUTBOUND_ACK + */ + size_t max_outbound_ack; /** * Bitwise OR of nghttp2_option_flag to determine that which fields * are specified. diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index 33d987667edaf5..3420cfa2f1c653 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -457,6 +457,7 @@ static int session_new(nghttp2_session **session_ptr, (*session_ptr)->remote_settings.max_concurrent_streams = 100; (*session_ptr)->max_send_header_block_length = NGHTTP2_MAX_HEADERSLEN; + (*session_ptr)->max_outbound_ack = NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM; if (option) { if ((option->opt_set_mask & NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE) && @@ -516,6 +517,10 @@ static int session_new(nghttp2_session **session_ptr, option->no_closed_streams) { (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_NO_CLOSED_STREAMS; } + + if (option->opt_set_mask & NGHTTP2_OPT_MAX_OUTBOUND_ACK) { + (*session_ptr)->max_outbound_ack = option->max_outbound_ack; + } } rv = nghttp2_hd_deflate_init2(&(*session_ptr)->hd_deflater, @@ -6857,7 +6862,7 @@ int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags, mem = &session->mem; if ((flags & NGHTTP2_FLAG_ACK) && - session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) { + session->obq_flood_counter_ >= session->max_outbound_ack) { return NGHTTP2_ERR_FLOODED; } @@ -7002,7 +7007,7 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, return NGHTTP2_ERR_INVALID_ARGUMENT; } - if (session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) { + if (session->obq_flood_counter_ >= session->max_outbound_ack) { return NGHTTP2_ERR_FLOODED; } } diff --git a/deps/nghttp2/lib/nghttp2_session.h b/deps/nghttp2/lib/nghttp2_session.h index bd7a5b35817599..90ead9c0395b4f 100644 --- a/deps/nghttp2/lib/nghttp2_session.h +++ b/deps/nghttp2/lib/nghttp2_session.h @@ -97,7 +97,7 @@ typedef struct { response frames are stacked up, which leads to memory exhaustion. The value selected here is arbitrary, but safe value and if we have these frames in this number, it is considered suspicious. */ -#define NGHTTP2_MAX_OBQ_FLOOD_ITEM 10000 +#define NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM 1000 /* The default value of maximum number of concurrent streams. */ #define NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS 0xffffffffu @@ -258,8 +258,12 @@ struct nghttp2_session { size_t num_idle_streams; /* The number of bytes allocated for nvbuf */ size_t nvbuflen; - /* Counter for detecting flooding in outbound queue */ + /* Counter for detecting flooding in outbound queue. If it exceeds + max_outbound_ack, session will be closed. */ size_t obq_flood_counter_; + /* The maximum number of outgoing SETTINGS ACK and PING ACK in + outbound queue. */ + size_t max_outbound_ack; /* The maximum length of header block to send. Calculated by the same way as nghttp2_hd_deflate_bound() does. */ size_t max_send_header_block_length; From cc4b58978de8a05176a4fd7ba6be9ba8dc7cb02b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 5 Aug 2019 17:22:30 +0200 Subject: [PATCH 02/13] http2: improve JS-side debug logging DRY up the `debug()` calls, and in particular, avoid building template strings before we know whether we need to. --- lib/internal/http2/core.js | 112 +++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 54 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index fca37ae2b0812a..ab0a64e73e1858 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -138,6 +138,26 @@ const { StreamPipe } = internalBinding('stream_pipe'); const { _connectionListener: httpConnectionListener } = http; const debug = require('internal/util/debuglog').debuglog('http2'); +// TODO(addaleax): See if this can be made more efficient by figuring out +// whether debugging is enabled before we perform any further steps. Currently, +// this seems pretty fast, though. +function debugStream(id, sessionType, message, ...args) { + debug('Http2Stream %s [Http2Session %s]: ' + message, + id, sessionName(sessionType), ...args); +} + +function debugStreamObj(stream, message, ...args) { + debugStream(stream[kID], stream[kSession][kType], ...args); +} + +function debugSession(sessionType, message, ...args) { + debug('Http2Session %s: ' + message, sessionName(sessionType), ...args); +} + +function debugSessionObj(session, message, ...args) { + debugSession(session[kType], message, ...args); +} + const kMaxFrameSize = (2 ** 24) - 1; const kMaxInt = (2 ** 32) - 1; const kMaxStreams = (2 ** 31) - 1; @@ -256,8 +276,7 @@ function onSessionHeaders(handle, id, cat, flags, headers) { const type = session[kType]; session[kUpdateTimer](); - debug(`Http2Stream ${id} [Http2Session ` + - `${sessionName(type)}]: headers received`); + debugStream(id, type, 'headers received'); const streams = session[kState].streams; const endOfStream = !!(flags & NGHTTP2_FLAG_END_STREAM); @@ -317,8 +336,7 @@ function onSessionHeaders(handle, id, cat, flags, headers) { const originSet = session[kState].originSet = initOriginSet(session); originSet.delete(stream[kOrigin]); } - debug(`Http2Stream ${id} [Http2Session ` + - `${sessionName(type)}]: emitting stream '${event}' event`); + debugStream(id, type, "emitting stream '%s' event", event); process.nextTick(emit, stream, event, obj, flags, headers); } if (endOfStream) { @@ -359,7 +377,7 @@ function onPing(payload) { if (session.destroyed) return; session[kUpdateTimer](); - debug(`Http2Session ${sessionName(session[kType])}: new ping received`); + debugSessionObj(session, 'new ping received'); session.emit('ping', payload); } @@ -374,8 +392,7 @@ function onStreamClose(code) { if (!stream || stream.destroyed) return false; - debug(`Http2Stream ${stream[kID]} [Http2Session ` + - `${sessionName(stream[kSession][kType])}]: closed with code ${code}`); + debugStreamObj(stream, 'closed with code %d', code); if (!stream.closed) closeStream(stream, code, kNoRstStream); @@ -412,7 +429,7 @@ function onSettings() { if (session.destroyed) return; session[kUpdateTimer](); - debug(`Http2Session ${sessionName(session[kType])}: new settings received`); + debugSessionObj(session, 'new settings received'); session[kRemoteSettings] = undefined; session.emit('remoteSettings', session.remoteSettings); } @@ -424,9 +441,9 @@ function onPriority(id, parent, weight, exclusive) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Stream ${id} [Http2Session ` + - `${sessionName(session[kType])}]: priority [parent: ${parent}, ` + - `weight: ${weight}, exclusive: ${exclusive}]`); + debugStream(id, session[kType], + 'priority [parent: %d, weight: %d, exclusive: %s]', + parent, weight, exclusive); const emitter = session[kState].streams.get(id) || session; if (!emitter.destroyed) { emitter[kUpdateTimer](); @@ -440,8 +457,8 @@ function onFrameError(id, type, code) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Session ${sessionName(session[kType])}: error sending frame ` + - `type ${type} on stream ${id}, code: ${code}`); + debugSessionObj(session, 'error sending frame type %d on stream %d, code: %d', + type, id, code); const emitter = session[kState].streams.get(id) || session; emitter[kUpdateTimer](); emitter.emit('frameError', type, code, id); @@ -451,8 +468,8 @@ function onAltSvc(stream, origin, alt) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Session ${sessionName(session[kType])}: altsvc received: ` + - `stream: ${stream}, origin: ${origin}, alt: ${alt}`); + debugSessionObj(session, 'altsvc received: stream: %d, origin: %s, alt: %s', + stream, origin, alt); session[kUpdateTimer](); session.emit('altsvc', alt, origin, stream); } @@ -479,8 +496,7 @@ function onOrigin(origins) { const session = this[kOwner]; if (session.destroyed) return; - debug('Http2Session %s: origin received: %j', - sessionName(session[kType]), origins); + debugSessionObj(session, 'origin received: %j', origins); session[kUpdateTimer](); if (!session.encrypted || session.destroyed) return undefined; @@ -500,8 +516,8 @@ function onGoawayData(code, lastStreamID, buf) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Session ${sessionName(session[kType])}: goaway ${code} ` + - `received [last stream id: ${lastStreamID}]`); + debugSessionObj(session, 'goaway %d received [last stream id: %d]', + code, lastStreamID); const state = session[kState]; state.goawayCode = code; @@ -556,8 +572,7 @@ function requestOnConnect(headers, options) { return; } - debug(`Http2Session ${sessionName(session[kType])}: connected, ` + - 'initializing request'); + debugSessionObj(session, 'connected, initializing request'); let streamOptions = 0; if (options.endStream) @@ -646,13 +661,13 @@ function settingsCallback(cb, ack, duration) { this[kState].pendingAck--; this[kLocalSettings] = undefined; if (ack) { - debug(`Http2Session ${sessionName(this[kType])}: settings received`); + debugSessionObj(this, 'settings received'); const settings = this.localSettings; if (typeof cb === 'function') cb(null, settings, duration); this.emit('localSettings', settings); } else { - debug(`Http2Session ${sessionName(this[kType])}: settings canceled`); + debugSessionObj(this, 'settings canceled'); if (typeof cb === 'function') cb(new ERR_HTTP2_SETTINGS_CANCEL()); } @@ -662,7 +677,7 @@ function settingsCallback(cb, ack, duration) { function submitSettings(settings, callback) { if (this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: submitting settings`); + debugSessionObj(this, 'submitting settings'); this[kUpdateTimer](); updateSettingsBuffer(settings); if (!this[kHandle].settings(settingsCallback.bind(this, callback))) { @@ -696,7 +711,7 @@ function submitPriority(options) { function submitGoaway(code, lastStreamID, opaqueData) { if (this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: submitting goaway`); + debugSessionObj(this, 'submitting goaway'); this[kUpdateTimer](); this[kHandle].goaway(code, lastStreamID, opaqueData); } @@ -827,7 +842,7 @@ function setupHandle(socket, type, options) { 'Internal HTTP/2 Failure. The socket is not connected. Please ' + 'report this as a bug in Node.js'); - debug(`Http2Session ${sessionName(type)}: setting up session handle`); + debugSession(type, 'setting up session handle'); this[kState].flags |= SESSION_FLAGS_READY; updateOptionsBuffer(options); @@ -983,7 +998,7 @@ class Http2Session extends EventEmitter { setupFn(); } - debug(`Http2Session ${sessionName(type)}: created`); + debugSession(type, 'created'); } // Returns undefined if the socket is not yet connected, true if the @@ -1159,7 +1174,7 @@ class Http2Session extends EventEmitter { if (callback && typeof callback !== 'function') throw new ERR_INVALID_CALLBACK(callback); - debug(`Http2Session ${sessionName(this[kType])}: sending settings`); + debugSessionObj(this, 'sending settings'); this[kState].pendingAck++; @@ -1200,7 +1215,7 @@ class Http2Session extends EventEmitter { destroy(error = NGHTTP2_NO_ERROR, code) { if (this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: destroying`); + debugSessionObj(this, 'destroying'); if (typeof error === 'number') { code = error; @@ -1257,7 +1272,7 @@ class Http2Session extends EventEmitter { close(callback) { if (this.closed || this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: marking session closed`); + debugSessionObj(this, 'marking session closed'); this[kState].flags |= SESSION_FLAGS_CLOSED; if (typeof callback === 'function') this.once('close', callback); @@ -1428,7 +1443,7 @@ class ClientHttp2Session extends Http2Session { // Submits a new HTTP2 request to the connected peer. Returns the // associated Http2Stream instance. request(headers, options) { - debug(`Http2Session ${sessionName(this[kType])}: initiating request`); + debugSessionObj(this, 'initiating request'); if (this.destroyed) throw new ERR_HTTP2_INVALID_SESSION(); @@ -1828,8 +1843,7 @@ class Http2Stream extends Duplex { if (this.pending) { this.once('ready', () => this._final(cb)); } else if (handle !== undefined) { - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(this[kSession][kType])}]: _final shutting down`); + debugStreamObj(this, '_final shutting down'); const req = new ShutdownWrap(); req.oncomplete = afterShutdown; req.callback = cb; @@ -1888,9 +1902,7 @@ class Http2Stream extends Duplex { assertIsObject(headers, 'headers'); headers = Object.assign(Object.create(null), headers); - const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: sending trailers`); + debugStreamObj(this, 'sending trailers'); this[kUpdateTimer](); @@ -1943,8 +1955,7 @@ class Http2Stream extends Duplex { const handle = this[kHandle]; const id = this[kID]; - debug(`Http2Stream ${this[kID] || ''} [Http2Session ` + - `${sessionName(session[kType])}]: destroying stream`); + debugStream(this[kID] || 'pending', session[kType], 'destroying stream'); const state = this[kState]; const sessionCode = session[kState].goawayCode || @@ -2270,8 +2281,7 @@ class ServerHttp2Stream extends Http2Stream { const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating push stream`); + debugStreamObj(this, 'initiating push stream'); this[kUpdateTimer](); @@ -2351,9 +2361,7 @@ class ServerHttp2Stream extends Http2Stream { assertIsObject(options, 'options'); options = { ...options }; - const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating response`); + debugStreamObj(this, 'initiating response'); this[kUpdateTimer](); options.endStream = !!options.endStream; @@ -2426,8 +2434,7 @@ class ServerHttp2Stream extends Http2Stream { validateNumber(fd, 'fd'); - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating response from fd`); + debugStreamObj(this, 'initiating response from fd'); this[kUpdateTimer](); this.ownsFd = false; @@ -2488,8 +2495,7 @@ class ServerHttp2Stream extends Http2Stream { } const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating response from file`); + debugStreamObj(this, 'initiating response from file'); this[kUpdateTimer](); this.ownsFd = true; @@ -2523,9 +2529,7 @@ class ServerHttp2Stream extends Http2Stream { assertIsObject(headers, 'headers'); headers = Object.assign(Object.create(null), headers); - const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: sending additional headers`); + debugStreamObj(this, 'sending additional headers'); if (headers[HTTP2_HEADER_STATUS] != null) { const statusCode = headers[HTTP2_HEADER_STATUS] |= 0; @@ -2586,8 +2590,7 @@ function socketOnError(error) { // we can do and the other side is fully within its rights to do so. if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null) return session.destroy(); - debug(`Http2Session ${sessionName(session[kType])}: socket error [` + - `${error.message}]`); + debugSessionObj(this, 'socket error [%s]', error.message); session.destroy(error); } } @@ -2632,7 +2635,8 @@ function connectionListener(socket) { return httpConnectionListener.call(this, socket); } // Let event handler deal with the socket - debug(`Unknown protocol from ${socket.remoteAddress}:${socket.remotePort}`); + debug('Unknown protocol from %s:%s', + socket.remoteAddress, socket.remotePort); if (!this.emit('unknownProtocol', socket)) { // We don't know what to do, so let's just tell the other side what's // going on in a format that they *might* understand. @@ -2757,7 +2761,7 @@ function setupCompat(ev) { function socketOnClose() { const session = this[kSession]; if (session !== undefined) { - debug(`Http2Session ${sessionName(session[kType])}: socket closed`); + debugSessionObj(session, 'socket closed'); const err = session.connecting ? new ERR_SOCKET_CLOSED() : null; const state = session[kState]; state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL)); From 85512ecfed5372efce660fa7f2f479241ff29260 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 00:51:51 +0200 Subject: [PATCH 03/13] http2: only call into JS when necessary for session events For some JS events, it only makes sense to call into JS when there are listeners for the event in question. The overhead is noticeable if a lot of these events are emitted during the lifetime of a session. To reduce this overhead, keep track of whether any/how many JS listeners are present, and if there are none, skip calls into JS altogether. This is part of performance improvements to mitigate CVE-2019-9513. --- lib/internal/http2/core.js | 119 ++++++++++++++++++++++++++++++++++--- src/env.h | 1 + src/node_http2.cc | 29 ++++++++- src/node_http2.h | 20 +++++++ 4 files changed, 161 insertions(+), 8 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index ab0a64e73e1858..69666bb6ba5853 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -177,6 +177,7 @@ const kID = Symbol('id'); const kInit = Symbol('init'); const kInfoHeaders = Symbol('sent-info-headers'); const kLocalSettings = Symbol('local-settings'); +const kNativeFields = Symbol('kNativeFields'); const kOptions = Symbol('options'); const kOwner = owner_symbol; const kOrigin = Symbol('origin'); @@ -196,7 +197,15 @@ const { paddingBuffer, PADDING_BUF_FRAME_LENGTH, PADDING_BUF_MAX_PAYLOAD_LENGTH, - PADDING_BUF_RETURN_VALUE + PADDING_BUF_RETURN_VALUE, + kBitfield, + kSessionPriorityListenerCount, + kSessionFrameErrorListenerCount, + kSessionUint8FieldCount, + kSessionHasRemoteSettingsListeners, + kSessionRemoteSettingsIsUpToDate, + kSessionHasPingListeners, + kSessionHasAltsvcListeners, } = binding; const { @@ -372,6 +381,76 @@ function submitRstStream(code) { } } +// Keep track of the number/presence of JS event listeners. Knowing that there +// are no listeners allows the C++ code to skip calling into JS for an event. +function sessionListenerAdded(name) { + switch (name) { + case 'ping': + this[kNativeFields][kBitfield] |= 1 << kSessionHasPingListeners; + break; + case 'altsvc': + this[kNativeFields][kBitfield] |= 1 << kSessionHasAltsvcListeners; + break; + case 'remoteSettings': + this[kNativeFields][kBitfield] |= 1 << kSessionHasRemoteSettingsListeners; + break; + case 'priority': + this[kNativeFields][kSessionPriorityListenerCount]++; + break; + case 'frameError': + this[kNativeFields][kSessionFrameErrorListenerCount]++; + break; + } +} + +function sessionListenerRemoved(name) { + switch (name) { + case 'ping': + if (this.listenerCount(name) > 0) return; + this[kNativeFields][kBitfield] &= ~(1 << kSessionHasPingListeners); + break; + case 'altsvc': + if (this.listenerCount(name) > 0) return; + this[kNativeFields][kBitfield] &= ~(1 << kSessionHasAltsvcListeners); + break; + case 'remoteSettings': + if (this.listenerCount(name) > 0) return; + this[kNativeFields][kBitfield] &= + ~(1 << kSessionHasRemoteSettingsListeners); + break; + case 'priority': + this[kNativeFields][kSessionPriorityListenerCount]--; + break; + case 'frameError': + this[kNativeFields][kSessionFrameErrorListenerCount]--; + break; + } +} + +// Also keep track of listeners for the Http2Stream instances, as some events +// are emitted on those objects. +function streamListenerAdded(name) { + switch (name) { + case 'priority': + this[kSession][kNativeFields][kSessionPriorityListenerCount]++; + break; + case 'frameError': + this[kSession][kNativeFields][kSessionFrameErrorListenerCount]++; + break; + } +} + +function streamListenerRemoved(name) { + switch (name) { + case 'priority': + this[kSession][kNativeFields][kSessionPriorityListenerCount]--; + break; + case 'frameError': + this[kSession][kNativeFields][kSessionFrameErrorListenerCount]--; + break; + } +} + function onPing(payload) { const session = this[kOwner]; if (session.destroyed) @@ -430,7 +509,6 @@ function onSettings() { return; session[kUpdateTimer](); debugSessionObj(session, 'new settings received'); - session[kRemoteSettings] = undefined; session.emit('remoteSettings', session.remoteSettings); } @@ -854,6 +932,10 @@ function setupHandle(socket, type, options) { handle.consume(socket._handle); this[kHandle] = handle; + if (this[kNativeFields]) + handle.fields.set(this[kNativeFields]); + else + this[kNativeFields] = handle.fields; if (socket.encrypted) { this[kAlpnProtocol] = socket.alpnProtocol; @@ -895,6 +977,7 @@ function finishSessionDestroy(session, error) { session[kProxySocket] = undefined; session[kSocket] = undefined; session[kHandle] = undefined; + session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount); socket[kSession] = undefined; socket[kServer] = undefined; @@ -974,6 +1057,7 @@ class Http2Session extends EventEmitter { this[kProxySocket] = null; this[kSocket] = socket; this[kTimeout] = null; + this[kHandle] = undefined; // Do not use nagle's algorithm if (typeof socket.setNoDelay === 'function') @@ -998,6 +1082,11 @@ class Http2Session extends EventEmitter { setupFn(); } + if (!this[kNativeFields]) + this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount); + this.on('newListener', sessionListenerAdded); + this.on('removeListener', sessionListenerRemoved); + debugSession(type, 'created'); } @@ -1155,13 +1244,18 @@ class Http2Session extends EventEmitter { // The settings currently in effect for the remote peer. get remoteSettings() { - const settings = this[kRemoteSettings]; - if (settings !== undefined) - return settings; + if (this[kNativeFields][kBitfield] & + (1 << kSessionRemoteSettingsIsUpToDate)) { + const settings = this[kRemoteSettings]; + if (settings !== undefined) { + return settings; + } + } if (this.destroyed || this.connecting) return {}; + this[kNativeFields][kBitfield] |= (1 << kSessionRemoteSettingsIsUpToDate); return this[kRemoteSettings] = getSettings(this[kHandle], true); // Remote } @@ -1340,6 +1434,12 @@ class ServerHttp2Session extends Http2Session { constructor(options, socket, server) { super(NGHTTP2_SESSION_SERVER, options, socket); this[kServer] = server; + // This is a bit inaccurate because it does not reflect changes to + // number of listeners made after the session was created. This should + // not be an issue in practice. Additionally, the 'priority' event on + // server instances (or any other object) is fully undocumented. + this[kNativeFields][kSessionPriorityListenerCount] = + server.listenerCount('priority'); } get server() { @@ -1652,6 +1752,9 @@ class Http2Stream extends Duplex { this[kProxySocket] = null; this.on('pause', streamOnPause); + + this.on('newListener', streamListenerAdded); + this.on('removeListener', streamListenerRemoved); } [kUpdateTimer]() { @@ -2608,7 +2711,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) { } function sessionOnError(error) { - if (this[kServer]) + if (this[kServer] !== undefined) this[kServer].emit('sessionError', error, this); } @@ -2657,8 +2760,10 @@ function connectionListener(socket) { const session = new ServerHttp2Session(options, socket, this); session.on('stream', sessionOnStream); - session.on('priority', sessionOnPriority); session.on('error', sessionOnError); + // Don't count our own internal listener. + session.on('priority', sessionOnPriority); + session[kNativeFields][kSessionPriorityListenerCount]--; if (this.timeout) session.setTimeout(this.timeout, sessionOnTimeout); diff --git a/src/env.h b/src/env.h index ada3d4f9d8d860..9a0f98e121b380 100644 --- a/src/env.h +++ b/src/env.h @@ -230,6 +230,7 @@ constexpr size_t kFsStatsBufferLength = V(family_string, "family") \ V(fatal_exception_string, "_fatalException") \ V(fd_string, "fd") \ + V(fields_string, "fields") \ V(file_string, "file") \ V(fingerprint256_string, "fingerprint256") \ V(fingerprint_string, "fingerprint") \ diff --git a/src/node_http2.cc b/src/node_http2.cc index 9d6c37373ff487..265d1ce3575c6d 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -25,6 +25,7 @@ using v8::ObjectTemplate; using v8::String; using v8::Uint32; using v8::Uint32Array; +using v8::Uint8Array; using v8::Undefined; using node::performance::PerformanceEntry; @@ -639,6 +640,15 @@ Http2Session::Http2Session(Environment* env, outgoing_storage_.reserve(4096); outgoing_buffers_.reserve(32); + + { + // Make the js_fields_ property accessible to JS land. + Local ab = + ArrayBuffer::New(env->isolate(), js_fields_, kSessionUint8FieldCount); + Local uint8_arr = + Uint8Array::New(ab, 0, kSessionUint8FieldCount); + USE(wrap->Set(env->context(), env->fields_string(), uint8_arr)); + } } Http2Session::~Http2Session() { @@ -1033,7 +1043,8 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle, // 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) { + error_code == NGHTTP2_ERR_STREAM_CLOSING || + session->js_fields_[kSessionFrameErrorListenerCount] == 0) { return 0; } @@ -1316,6 +1327,7 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // are considered advisory only, so this has no real effect other than to // simply let user code know that the priority has changed. void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { + if (js_fields_[kSessionPriorityListenerCount] == 0) return; Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1380,6 +1392,7 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete ALTSVC frame has been received. void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { + if (!(js_fields_[kBitfield] & (1 << kSessionHasAltsvcListeners))) return; Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1458,6 +1471,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { return; } + if (!(js_fields_[kBitfield] & (1 << kSessionHasPingListeners))) return; // Notify the session that a ping occurred arg = Buffer::Copy(env(), reinterpret_cast(frame->ping.opaque_data), @@ -1469,6 +1483,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (!ack) { + js_fields_[kBitfield] &= ~(1 << kSessionRemoteSettingsIsUpToDate); + if (!(js_fields_[kBitfield] & (1 << kSessionHasRemoteSettingsListeners))) + return; // This is not a SETTINGS acknowledgement, notify and return MakeCallback(env()->http2session_on_settings_function(), 0, nullptr); return; @@ -2981,6 +2998,16 @@ void Initialize(Local target, NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH); NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE); + NODE_DEFINE_CONSTANT(target, kBitfield); + NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount); + NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount); + NODE_DEFINE_CONSTANT(target, kSessionUint8FieldCount); + + NODE_DEFINE_CONSTANT(target, kSessionHasRemoteSettingsListeners); + NODE_DEFINE_CONSTANT(target, kSessionRemoteSettingsIsUpToDate); + NODE_DEFINE_CONSTANT(target, kSessionHasPingListeners); + NODE_DEFINE_CONSTANT(target, kSessionHasAltsvcListeners); + // Method to fetch the nghttp2 string description of an nghttp2 error code env->SetMethod(target, "nghttp2ErrorString", HttpErrorString); diff --git a/src/node_http2.h b/src/node_http2.h index d9636628c25939..9a156c8bfa883c 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -672,6 +672,23 @@ class Http2Stream::Provider::Stream : public Http2Stream::Provider { void* user_data); }; +// Indices for js_fields_, which serves as a way to communicate data with JS +// land fast. In particular, we store information about the number/presence +// of certain event listeners in JS, and skip calls from C++ into JS if they +// are missing. +enum SessionUint8Fields { + kBitfield, // See below + kSessionPriorityListenerCount, + kSessionFrameErrorListenerCount, + kSessionUint8FieldCount +}; + +enum SessionBitfieldFlags { + kSessionHasRemoteSettingsListeners, + kSessionRemoteSettingsIsUpToDate, + kSessionHasPingListeners, + kSessionHasAltsvcListeners +}; class Http2Session : public AsyncWrap, public StreamListener { public: @@ -949,6 +966,9 @@ class Http2Session : public AsyncWrap, public StreamListener { // The underlying nghttp2_session handle nghttp2_session* session_; + // JS-accessible numeric fields, as indexed by SessionUint8Fields. + uint8_t js_fields_[kSessionUint8FieldCount] = {}; + // The session type: client or server nghttp2_session_type session_type_; From d642202a68cc677608dd693d49ff04d1c8ead85d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 01:30:35 +0200 Subject: [PATCH 04/13] http2: do not create ArrayBuffers when no DATA received MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lazily allocate `ArrayBuffer`s for the contents of DATA frames. Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8. This is part of performance improvements to mitigate CVE-2019-9513. Together with the previous commit, these changes improve throughput in the adversarial case by about 100 %, and there is little more that we can do besides artificially limiting the rate of incoming metadata frames (i.e. after this patch, CPU usage is virtually exclusively in libnghttp2). --- src/node_http2.cc | 21 +++++++++++++++------ src/node_http2.h | 3 ++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 265d1ce3575c6d..3f24c3a25ee363 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1259,7 +1259,13 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { return; } - CHECK(!session->stream_buf_ab_.IsEmpty()); + Local ab; + if (session->stream_buf_ab_.IsEmpty()) { + ab = session->stream_buf_allocation_.ToArrayBuffer(); + session->stream_buf_ab_.Reset(env->isolate(), ab); + } else { + ab = PersistentToLocal::Strong(session->stream_buf_ab_); + } // There is a single large array buffer for the entire data read from the // network; create a slice of that array buffer and emit it as the @@ -1270,7 +1276,7 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { CHECK_LE(offset, session->stream_buf_.len); CHECK_LE(offset + buf.len, session->stream_buf_.len); - stream->CallJSOnreadMethod(nread, session->stream_buf_ab_, offset); + stream->CallJSOnreadMethod(nread, ab, offset); } @@ -1789,6 +1795,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { Http2Scope h2scope(this); CHECK_NOT_NULL(stream_); Debug(this, "receiving %d bytes", nread); + CHECK_EQ(stream_buf_allocation_.size(), 0); CHECK(stream_buf_ab_.IsEmpty()); AllocatedBuffer buf(env(), buf_); @@ -1810,7 +1817,8 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // We use `nread` instead of `buf.size()` here, because the buffer is // cleared as part of the `.ToArrayBuffer()` call below. DecrementCurrentSessionMemory(nread); - stream_buf_ab_ = Local(); + stream_buf_ab_.Reset(); + stream_buf_allocation_.clear(); stream_buf_ = uv_buf_init(nullptr, 0); }); @@ -1824,9 +1832,10 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { Isolate* isolate = env()->isolate(); - // Create an array buffer for the read data. DATA frames will be emitted - // as slices of this array buffer to avoid having to copy memory. - stream_buf_ab_ = buf.ToArrayBuffer(); + // Store this so we can create an ArrayBuffer for read data from it. + // DATA frames will be emitted as slices of that ArrayBuffer to avoid having + // to copy memory. + stream_buf_allocation_ = std::move(buf); statistics_.data_received += nread; ssize_t ret = Write(&stream_buf_, 1); diff --git a/src/node_http2.h b/src/node_http2.h index 9a156c8bfa883c..47d25f2b076ed0 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -993,7 +993,8 @@ class Http2Session : public AsyncWrap, public StreamListener { uint32_t chunks_sent_since_last_write_ = 0; uv_buf_t stream_buf_ = uv_buf_init(nullptr, 0); - v8::Local stream_buf_ab_; + v8::Global stream_buf_ab_; + AllocatedBuffer stream_buf_allocation_; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; std::queue> outstanding_pings_; From f33d342f3a81a1b67705e9f76f9b4858c500b8a8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 22:27:48 +0200 Subject: [PATCH 05/13] http2: limit number of rejected stream openings Limit the number of streams that are rejected upon creation. Since each such rejection is associated with an `NGHTTP2_ENHANCE_YOUR_CALM` error that should tell the peer to not open any more streams, continuing to open streams should be read as a sign of a misbehaving peer. The limit is currently set to 100 but could be changed or made configurable. This is intended to mitigate CVE-2019-9514. --- src/node_http2.cc | 7 +++++++ src/node_http2.h | 5 +++++ src/node_revert.h | 5 ++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 3f24c3a25ee363..63617cfd9f93e8 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -6,6 +6,7 @@ #include "node_http2.h" #include "node_http2_state.h" #include "node_perf.h" +#include "node_revert.h" #include "util-inl.h" #include @@ -921,11 +922,17 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, if (UNLIKELY(!session->CanAddStream() || Http2Stream::New(session, id, frame->headers.cat) == nullptr)) { + if (session->rejected_stream_count_++ > 100 && + !IsReverted(SECURITY_REVERT_CVE_2019_9514)) { + return NGHTTP2_ERR_CALLBACK_FAILURE; + } // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } + + session->rejected_stream_count_ = 0; } else if (!stream->IsDestroyed()) { stream->StartHeaders(frame->headers.cat); } diff --git a/src/node_http2.h b/src/node_http2.h index 47d25f2b076ed0..1739a29b256bc9 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1005,6 +1005,11 @@ class Http2Session : public AsyncWrap, public StreamListener { std::vector outgoing_buffers_; std::vector outgoing_storage_; std::vector pending_rst_streams_; + // Count streams that have been rejected while being opened. Exceeding a fixed + // limit will result in the session being destroyed, as an indication of a + // misbehaving peer. This counter is reset once new streams are being + // accepted again. + int32_t rejected_stream_count_ = 0; void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); diff --git a/src/node_revert.h b/src/node_revert.h index 38e2ba71053691..b0853ee75f1628 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -15,8 +15,11 @@ **/ namespace node { -#define SECURITY_REVERSIONS(XX) +#define SECURITY_REVERSIONS(XX) \ + XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") + // TODO(addaleax): Remove all of the above before Node.js 13 as the comment + // at the start of the file indicates. enum reversion { #define V(code, ...) SECURITY_REVERT_##code, From c1ff7e711f357c75f27f5d78e94be5afe7210787 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Aug 2019 22:55:16 +0200 Subject: [PATCH 06/13] http2: limit number of invalid incoming frames Limit the number of invalid input frames, as they may be pointing towards a misbehaving peer. The limit is currently set to 1000 but could be changed or made configurable. This is intended to mitigate CVE-2019-9514. --- src/node_http2.cc | 4 ++ src/node_http2.h | 2 + test/parallel/test-http2-reset-flood.js | 79 +++++++++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 test/parallel/test-http2-reset-flood.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 63617cfd9f93e8..058ae1f1909a02 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1017,6 +1017,10 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, Http2Session* session = static_cast(user_data); Debug(session, "invalid frame received, code: %d", lib_error_code); + if (session->invalid_frame_count_++ > 1000 && + !IsReverted(SECURITY_REVERT_CVE_2019_9514)) { + return 1; + } // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error if (nghttp2_is_fatal(lib_error_code) || diff --git a/src/node_http2.h b/src/node_http2.h index 1739a29b256bc9..fe0c3ffa7aa9ca 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1010,6 +1010,8 @@ class Http2Session : public AsyncWrap, public StreamListener { // misbehaving peer. This counter is reset once new streams are being // 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; void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); diff --git a/test/parallel/test-http2-reset-flood.js b/test/parallel/test-http2-reset-flood.js new file mode 100644 index 00000000000000..a6553401fbb6e7 --- /dev/null +++ b/test/parallel/test-http2-reset-flood.js @@ -0,0 +1,79 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); +const { Worker, parentPort } = require('worker_threads'); + +// Verify that creating a number of invalid HTTP/2 streams will eventually +// result in the peer closing the session. +// This test uses separate threads for client and server to avoid +// the two event loops intermixing, as we are writing in a busy loop here. + +if (process.env.HAS_STARTED_WORKER) { + const server = http2.createServer(); + server.on('stream', (stream) => { + stream.respond({ + 'content-type': 'text/plain', + ':status': 200 + }); + stream.end('Hello, world!\n'); + }); + server.listen(0, () => parentPort.postMessage(server.address().port)); + return; +} + +process.env.HAS_STARTED_WORKER = 1; +const worker = new Worker(__filename).on('message', common.mustCall((port) => { + const h2header = Buffer.alloc(9); + const conn = net.connect(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; + + function writeRequests() { + for (let i = 1; !gotError; i += 2) { + h2header[3] = 1; // HEADERS + h2header[4] = 0x5; // END_HEADERS|END_STREAM + h2header.writeIntBE(1, 0, 3); // Length: 1 + h2header.writeIntBE(i, 5, 4); // Stream ID + // 0x88 = :status: 200 + conn.write(Buffer.concat([h2header, Buffer.from([0x88])])); + } + } + + conn.once('error', common.mustCall(() => { + gotError = true; + worker.terminate(); + conn.destroy(); + })); +})); From dc51a633dc430d0184642f717b1020deb3df19c8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 23:10:54 +0200 Subject: [PATCH 07/13] http2: handle 0-length headers better Ignore headers with 0-length names and track memory for headers the way we track it for other HTTP/2 session memory too. This is intended to mitigate CVE-2019-9516. --- src/node_http2.cc | 13 ++++++++-- src/node_revert.h | 1 + .../parallel/test-http2-zero-length-header.js | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-zero-length-header.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 058ae1f1909a02..e184a3ce82c462 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1309,6 +1309,8 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { return; std::vector headers(stream->move_headers()); + DecrementCurrentSessionMemory(stream->current_headers_length_); + stream->current_headers_length_ = 0; // The headers are passed in above as a queue of nghttp2_header structs. // The following converts that into a JS array with the structure: @@ -1942,6 +1944,7 @@ Http2Stream::~Http2Stream() { if (session_ == nullptr) return; Debug(this, "tearing down stream"); + session_->DecrementCurrentSessionMemory(current_headers_length_); session_->RemoveStream(this); session_ = nullptr; } @@ -1956,6 +1959,7 @@ std::string Http2Stream::diagnostic_name() const { void Http2Stream::StartHeaders(nghttp2_headers_category category) { Debug(this, "starting headers, category: %d", id_, category); CHECK(!this->IsDestroyed()); + session_->DecrementCurrentSessionMemory(current_headers_length_); current_headers_length_ = 0; current_headers_.clear(); current_headers_category_ = category; @@ -2225,8 +2229,12 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name, CHECK(!this->IsDestroyed()); if (this->statistics_.first_header == 0) this->statistics_.first_header = uv_hrtime(); - size_t length = nghttp2_rcbuf_get_buf(name).len + - nghttp2_rcbuf_get_buf(value).len + 32; + size_t name_len = nghttp2_rcbuf_get_buf(name).len; + if (name_len == 0 && !IsReverted(SECURITY_REVERT_CVE_2019_9516)) { + return true; // Ignore headers with empty names. + } + size_t value_len = nghttp2_rcbuf_get_buf(value).len; + size_t length = name_len + value_len + 32; // A header can only be added if we have not exceeded the maximum number // of headers and the session has memory available for it. if (!session_->IsAvailableSessionMemory(length) || @@ -2242,6 +2250,7 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name, nghttp2_rcbuf_incref(name); nghttp2_rcbuf_incref(value); current_headers_length_ += length; + session_->IncrementCurrentSessionMemory(length); return true; } diff --git a/src/node_revert.h b/src/node_revert.h index b0853ee75f1628..dfce73b95db540 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -17,6 +17,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ + XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") // TODO(addaleax): Remove all of the above before Node.js 13 as the comment // at the start of the file indicates. diff --git a/test/parallel/test-http2-zero-length-header.js b/test/parallel/test-http2-zero-length-header.js new file mode 100644 index 00000000000000..7b142d75f003b6 --- /dev/null +++ b/test/parallel/test-http2-zero-length-header.js @@ -0,0 +1,25 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const http2 = require('http2'); + +const server = http2.createServer(); +server.on('stream', (stream, headers) => { + assert.deepStrictEqual(headers, { + ':scheme': 'http', + ':authority': `localhost:${server.address().port}`, + ':method': 'GET', + ':path': '/', + 'bar': '', + '__proto__': null + }); + stream.session.destroy(); + server.close(); +}); +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}/`); + client.request({ ':path': '/', '': 'foo', 'bar': '' }).end(); +})); From bfcae459684fc7648ba5b54b115c994e1a7073d6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 23:18:13 +0200 Subject: [PATCH 08/13] http2: shrink default `vector::reserve()` allocations Allocating memory upfront comes with overhead, and in particular, `std::vector` implementations do not necessarily return memory to the system when one might expect that (e.g. after shrinking the vector). --- src/node_http2.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index e184a3ce82c462..d4b672a603fb80 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -639,7 +639,7 @@ Http2Session::Http2Session(Environment* env, // fails. CHECK_EQ(fn(&session_, callbacks, this, *opts, *allocator_info), 0); - outgoing_storage_.reserve(4096); + outgoing_storage_.reserve(1024); outgoing_buffers_.reserve(32); { @@ -1914,7 +1914,7 @@ Http2Stream::Http2Stream(Http2Session* session, if (max_header_pairs_ == 0) { max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; } - current_headers_.reserve(max_header_pairs_); + current_headers_.reserve(std::min(max_header_pairs_, 12u)); // Limit the number of header octets max_header_length_ = From cc17632ed320cdbd9dfcd031588f0a8e58f7144c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 23:37:58 +0200 Subject: [PATCH 09/13] http2: consider 0-length non-end DATA frames an error This is intended to mitigate CVE-2019-9518. --- src/node_http2.cc | 12 ++++++++---- src/node_http2.h | 2 +- src/node_revert.h | 1 + 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index d4b672a603fb80..a0b87dd94a9495 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -979,8 +979,7 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle, frame->hd.type); switch (frame->hd.type) { case NGHTTP2_DATA: - session->HandleDataFrame(frame); - break; + return session->HandleDataFrame(frame); case NGHTTP2_PUSH_PROMISE: // Intentional fall-through, handled just like headers frames case NGHTTP2_HEADERS: @@ -1372,13 +1371,18 @@ void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete DATA frame has been received. // If we know that this was the last DATA frame (because the END_STREAM flag // is set), then we'll terminate the readable side of the StreamBase. -void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { +int Http2Session::HandleDataFrame(const nghttp2_frame* frame) { int32_t id = GetFrameID(frame); Debug(this, "handling data frame for stream %d", id); Http2Stream* stream = FindStream(id); - if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) + if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { stream->EmitRead(UV_EOF); + } else if (frame->hd.length == 0 && + !IsReverted(SECURITY_REVERT_CVE_2019_9518)) { + return 1; // Consider 0-length frame without END_STREAM an error. + } + return 0; } diff --git a/src/node_http2.h b/src/node_http2.h index fe0c3ffa7aa9ca..e0862614f291d2 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -877,7 +877,7 @@ class Http2Session : public AsyncWrap, public StreamListener { size_t maxPayloadLen); // Frame Handler - void HandleDataFrame(const nghttp2_frame* frame); + int HandleDataFrame(const nghttp2_frame* frame); void HandleGoawayFrame(const nghttp2_frame* frame); void HandleHeadersFrame(const nghttp2_frame* frame); void HandlePriorityFrame(const nghttp2_frame* frame); diff --git a/src/node_revert.h b/src/node_revert.h index dfce73b95db540..33b30a0dfe7f00 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -18,6 +18,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak") \ + XX(CVE_2019_9518, "CVE-2019-9518", "HTTP/2 Empty DATA Frame Flooding") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") // TODO(addaleax): Remove all of the above before Node.js 13 as the comment // at the start of the file indicates. From c13022c2dc8a3821bdb432e01659c8e3bd65ad85 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 11 Aug 2019 01:31:20 +0200 Subject: [PATCH 10/13] http2: stop reading from socket if writes are in progress If a write to the underlying socket finishes asynchronously, that means that we cannot write any more data at that point without waiting for it to finish. If this happens, we should also not be producing any more input. This is part of mitigating CVE-2019-9511/CVE-2019-9517. --- src/node_http2.cc | 17 ++++++++++++++++- src/node_http2.h | 2 ++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index a0b87dd94a9495..0fec129373e701 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1542,9 +1542,18 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { Debug(this, "write finished with status %d", status); + CHECK_NE(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); + flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; + // Inform all pending writes about their completion. ClearOutgoing(status); + if ((flags_ & SESSION_STATE_READING_STOPPED) && + nghttp2_session_want_read(session_)) { + flags_ &= ~SESSION_STATE_READING_STOPPED; + stream_->ReadStart(); + } + if (!(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); @@ -1582,10 +1591,13 @@ void Http2Session::MaybeScheduleWrite() { } void Http2Session::MaybeStopReading() { + if (flags_ & SESSION_STATE_READING_STOPPED) return; int want_read = nghttp2_session_want_read(session_); Debug(this, "wants read? %d", want_read); - if (want_read == 0) + if (want_read == 0 || (flags_ & SESSION_STATE_WRITE_IN_PROGRESS)) { + flags_ |= SESSION_STATE_READING_STOPPED; stream_->ReadStop(); + } } // Unset the sending state, finish up all current writes, and reset @@ -1716,8 +1728,11 @@ uint8_t Http2Session::SendPendingData() { chunks_sent_since_last_write_++; + CHECK_EQ(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); + flags_ |= SESSION_STATE_WRITE_IN_PROGRESS; StreamWriteResult res = underlying_stream()->Write(*bufs, count); if (!res.async) { + flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; ClearOutgoing(res.err); } diff --git a/src/node_http2.h b/src/node_http2.h index e0862614f291d2..21e05157c18b82 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -335,6 +335,8 @@ enum session_state_flags { SESSION_STATE_CLOSED = 0x4, SESSION_STATE_CLOSING = 0x8, SESSION_STATE_SENDING = 0x10, + SESSION_STATE_WRITE_IN_PROGRESS = 0x20, + SESSION_STATE_READING_STOPPED = 0x40, }; typedef uint32_t(*get_setting)(nghttp2_session* session, From 164ac5b241b96089e6bad5bb83ea416966b3245f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 11 Aug 2019 02:22:00 +0200 Subject: [PATCH 11/13] http2: pause input processing if sending output If we are waiting for the ability to send more output, we should not process more input. This commit a) makes us send output earlier, during processing of input, if we accumulate a lot and b) allows interrupting the call into nghttp2 that processes input data and resuming it at a later time, if we do find ourselves in a position where we are waiting to be able to send more output. This is part of mitigating CVE-2019-9511/CVE-2019-9517. --- src/node_http2.cc | 128 ++++++++++++------ src/node_http2.h | 12 +- ...est-http2-large-write-multiple-requests.js | 39 ++++++ 3 files changed, 136 insertions(+), 43 deletions(-) create mode 100644 test/parallel/test-http2-large-write-multiple-requests.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 0fec129373e701..7e32f7f14bc9cd 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -868,31 +868,51 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen, // various callback functions. Each of these will typically result in a call // out to JavaScript so this particular function is rather hot and can be // quite expensive. This is a potential performance optimization target later. -ssize_t Http2Session::Write(const uv_buf_t* bufs, size_t nbufs) { - size_t total = 0; - // Note that nghttp2_session_mem_recv is a synchronous operation that - // will trigger a number of other callbacks. Those will, in turn have +ssize_t Http2Session::ConsumeHTTP2Data() { + CHECK_NOT_NULL(stream_buf_.base); + CHECK_LT(stream_buf_offset_, stream_buf_.len); + size_t read_len = stream_buf_.len - stream_buf_offset_; + // multiple side effects. - for (size_t n = 0; n < nbufs; n++) { - Debug(this, "receiving %d bytes [wants data? %d]", - bufs[n].len, - nghttp2_session_want_read(session_)); - ssize_t ret = - nghttp2_session_mem_recv(session_, - reinterpret_cast(bufs[n].base), - bufs[n].len); - CHECK_NE(ret, NGHTTP2_ERR_NOMEM); - - if (ret < 0) - return ret; + Debug(this, "receiving %d bytes [wants data? %d]", + read_len, + nghttp2_session_want_read(session_)); + flags_ &= ~SESSION_STATE_NGHTTP2_RECV_PAUSED; + ssize_t ret = + nghttp2_session_mem_recv(session_, + reinterpret_cast(stream_buf_.base) + + stream_buf_offset_, + read_len); + CHECK_NE(ret, NGHTTP2_ERR_NOMEM); - total += ret; + if (flags_ & SESSION_STATE_NGHTTP2_RECV_PAUSED) { + CHECK_NE(flags_ & SESSION_STATE_READING_STOPPED, 0); + + CHECK_GT(ret, 0); + CHECK_LE(static_cast(ret), read_len); + + if (static_cast(ret) < read_len) { + // Mark the remainder of the data as available for later consumption. + stream_buf_offset_ += ret; + return ret; + } } + + // We are done processing the current input chunk. + DecrementCurrentSessionMemory(stream_buf_.len); + stream_buf_offset_ = 0; + stream_buf_ab_.Reset(); + stream_buf_allocation_.clear(); + stream_buf_ = uv_buf_init(nullptr, 0); + + if (ret < 0) + return ret; + // Send any data that was queued up while processing the received data. if (!IsDestroyed()) { SendPendingData(); } - return total; + return ret; } @@ -1194,8 +1214,18 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, nghttp2_session_consume_stream(handle, id, avail); else stream->inbound_consumed_data_while_paused_ += avail; + + // If we have a gathered a lot of data for output, try sending it now. + if (session->outgoing_length_ > 4096) session->SendPendingData(); } while (len != 0); + // If we are currently waiting for a write operation to finish, we should + // tell nghttp2 that we want to wait before we process more input data. + if (session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) { + session->flags_ |= SESSION_STATE_NGHTTP2_RECV_PAUSED; + return NGHTTP2_ERR_PAUSE; + } + return 0; } @@ -1283,6 +1313,7 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { size_t offset = buf.base - session->stream_buf_.base; // Verify that the data offset is inside the current read buffer. + CHECK_GE(offset, session->stream_buf_offset_); CHECK_LE(offset, session->stream_buf_.len); CHECK_LE(offset + buf.len, session->stream_buf_.len); @@ -1554,6 +1585,11 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { stream_->ReadStart(); } + // If there is more incoming data queued up, consume it. + if (stream_buf_offset_ > 0) { + ConsumeHTTP2Data(); + } + if (!(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); @@ -1609,6 +1645,7 @@ void Http2Session::ClearOutgoing(int status) { if (outgoing_buffers_.size() > 0) { outgoing_storage_.clear(); + outgoing_length_ = 0; std::vector current_outgoing_buffers_; current_outgoing_buffers_.swap(outgoing_buffers_); @@ -1639,6 +1676,11 @@ void Http2Session::ClearOutgoing(int status) { } } +void Http2Session::PushOutgoingBuffer(nghttp2_stream_write&& write) { + outgoing_length_ += write.buf.len; + outgoing_buffers_.emplace_back(std::move(write)); +} + // Queue a given block of data for sending. This always creates a copy, // so it is used for the cases in which nghttp2 requests sending of a // small chunk of data. @@ -1651,7 +1693,7 @@ void Http2Session::CopyDataIntoOutgoing(const uint8_t* src, size_t src_length) { // of the outgoing_buffers_ vector may invalidate the pointer. // The correct base pointers will be set later, before writing to the // underlying socket. - outgoing_buffers_.emplace_back(nghttp2_stream_write { + PushOutgoingBuffer(nghttp2_stream_write { uv_buf_init(nullptr, src_length) }); } @@ -1774,13 +1816,13 @@ int Http2Session::OnSendData( if (write.buf.len <= length) { // This write does not suffice by itself, so we can consume it completely. length -= write.buf.len; - session->outgoing_buffers_.emplace_back(std::move(write)); + session->PushOutgoingBuffer(std::move(write)); stream->queue_.pop(); continue; } // Slice off `length` bytes of the first write in the queue. - session->outgoing_buffers_.emplace_back(nghttp2_stream_write { + session->PushOutgoingBuffer(nghttp2_stream_write { uv_buf_init(write.buf.base, length) }); write.buf.base += length; @@ -1790,7 +1832,7 @@ int Http2Session::OnSendData( if (frame->data.padlen > 0) { // Send padding if that was requested. - session->outgoing_buffers_.emplace_back(nghttp2_stream_write { + session->PushOutgoingBuffer(nghttp2_stream_write { uv_buf_init(const_cast(zero_bytes_256), frame->data.padlen - 1) }); } @@ -1827,8 +1869,6 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { Http2Scope h2scope(this); CHECK_NOT_NULL(stream_); Debug(this, "receiving %d bytes", nread); - CHECK_EQ(stream_buf_allocation_.size(), 0); - CHECK(stream_buf_ab_.IsEmpty()); AllocatedBuffer buf(env(), buf_); // Only pass data on if nread > 0 @@ -1839,24 +1879,31 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { return; } - // Shrink to the actual amount of used data. - buf.Resize(nread); + statistics_.data_received += nread; - IncrementCurrentSessionMemory(nread); - OnScopeLeave on_scope_leave([&]() { - // Once finished handling this write, reset the stream buffer. - // The memory has either been free()d or was handed over to V8. - // We use `nread` instead of `buf.size()` here, because the buffer is - // cleared as part of the `.ToArrayBuffer()` call below. - DecrementCurrentSessionMemory(nread); + if (UNLIKELY(stream_buf_offset_ > 0)) { + // This is a very unlikely case, and should only happen if the ReadStart() + // call in OnStreamAfterWrite() immediately provides data. If that does + // happen, we concatenate the data we received with the already-stored + // pending input data, slicing off the already processed part. + AllocatedBuffer new_buf = env()->AllocateManaged( + stream_buf_.len - stream_buf_offset_ + nread); + memcpy(new_buf.data(), + stream_buf_.base + stream_buf_offset_, + stream_buf_.len - stream_buf_offset_); + memcpy(new_buf.data() + stream_buf_.len - stream_buf_offset_, + buf.data(), + nread); + buf = std::move(new_buf); + nread = buf.size(); + stream_buf_offset_ = 0; stream_buf_ab_.Reset(); - stream_buf_allocation_.clear(); - stream_buf_ = uv_buf_init(nullptr, 0); - }); + DecrementCurrentSessionMemory(stream_buf_offset_); + } - // Make sure that there was no read previously active. - CHECK_NULL(stream_buf_.base); - CHECK_EQ(stream_buf_.len, 0); + // Shrink to the actual amount of used data. + buf.Resize(nread); + IncrementCurrentSessionMemory(nread); // Remember the current buffer, so that OnDataChunkReceived knows the // offset of a DATA frame's data into the socket read buffer. @@ -1869,8 +1916,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // to copy memory. stream_buf_allocation_ = std::move(buf); - statistics_.data_received += nread; - ssize_t ret = Write(&stream_buf_, 1); + ssize_t ret = ConsumeHTTP2Data(); if (UNLIKELY(ret < 0)) { Debug(this, "fatal error receiving data: %d", ret); diff --git a/src/node_http2.h b/src/node_http2.h index 21e05157c18b82..9c032f02d17ac7 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -337,6 +337,7 @@ enum session_state_flags { SESSION_STATE_SENDING = 0x10, SESSION_STATE_WRITE_IN_PROGRESS = 0x20, SESSION_STATE_READING_STOPPED = 0x40, + SESSION_STATE_NGHTTP2_RECV_PAUSED = 0x80 }; typedef uint32_t(*get_setting)(nghttp2_session* session, @@ -767,14 +768,15 @@ class Http2Session : public AsyncWrap, public StreamListener { // Indicates whether there currently exist outgoing buffers for this stream. bool HasWritesOnSocketForStream(Http2Stream* stream); - // Write data to the session - ssize_t Write(const uv_buf_t* bufs, size_t nbufs); + // Write data from stream_buf_ to the session + ssize_t ConsumeHTTP2Data(); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("streams", streams_); tracker->TrackField("outstanding_pings", outstanding_pings_); tracker->TrackField("outstanding_settings", outstanding_settings_); tracker->TrackField("outgoing_buffers", outgoing_buffers_); + tracker->TrackFieldWithSize("stream_buf", stream_buf_.len); tracker->TrackFieldWithSize("outgoing_storage", outgoing_storage_.size()); tracker->TrackFieldWithSize("pending_rst_streams", pending_rst_streams_.size() * sizeof(int32_t)); @@ -833,6 +835,7 @@ class Http2Session : public AsyncWrap, public StreamListener { } void DecrementCurrentSessionMemory(uint64_t amount) { + DCHECK_LE(amount, current_session_memory_); current_session_memory_ -= amount; } @@ -995,8 +998,11 @@ class Http2Session : public AsyncWrap, public StreamListener { uint32_t chunks_sent_since_last_write_ = 0; uv_buf_t stream_buf_ = uv_buf_init(nullptr, 0); + // When processing input data, either stream_buf_ab_ or stream_buf_allocation_ + // will be set. stream_buf_ab_ is lazily created from stream_buf_allocation_. v8::Global stream_buf_ab_; AllocatedBuffer stream_buf_allocation_; + size_t stream_buf_offset_ = 0; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; std::queue> outstanding_pings_; @@ -1006,6 +1012,7 @@ class Http2Session : public AsyncWrap, public StreamListener { std::vector outgoing_buffers_; std::vector outgoing_storage_; + size_t outgoing_length_ = 0; std::vector pending_rst_streams_; // Count streams that have been rejected while being opened. Exceeding a fixed // limit will result in the session being destroyed, as an indication of a @@ -1015,6 +1022,7 @@ class Http2Session : public AsyncWrap, public StreamListener { // Also use the invalid frame count as a measure for rejecting input frames. int32_t invalid_frame_count_ = 0; + void PushOutgoingBuffer(nghttp2_stream_write&& write); void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); diff --git a/test/parallel/test-http2-large-write-multiple-requests.js b/test/parallel/test-http2-large-write-multiple-requests.js new file mode 100644 index 00000000000000..0d65c3479b409d --- /dev/null +++ b/test/parallel/test-http2-large-write-multiple-requests.js @@ -0,0 +1,39 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const http2 = require('http2'); + +const content = fixtures.readSync('person-large.jpg'); + +const server = http2.createServer({ + maxSessionMemory: 1000 +}); +server.on('stream', (stream, headers) => { + stream.respond({ + 'content-type': 'image/jpeg', + ':status': 200 + }); + stream.end(content); +}); +server.unref(); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}/`); + + let finished = 0; + for (let i = 0; i < 100; i++) { + const req = client.request({ ':path': '/' }).end(); + const chunks = []; + req.on('data', (chunk) => { + chunks.push(chunk); + }); + req.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), content); + if (++finished === 100) client.close(); + })); + } +})); From 81f52dc9eb74cc1d3583ec27d99fc9c5b354a1a0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Aug 2019 23:36:00 +0200 Subject: [PATCH 12/13] http2: allow security revert for Ping/Settings Flood nghttp2 has updated its limit for outstanding Ping/Settings ACKs to 1000. This commit allows reverting to the old default of 10000. The associated CVEs are CVE-2019-9512/CVE-2019-9515. --- src/node_http2.cc | 3 +++ src/node_revert.h | 1 + 2 files changed, 4 insertions(+) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7e32f7f14bc9cd..8cb1158b559dec 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -151,6 +151,9 @@ Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { buffer[IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS]); } + if (IsReverted(SECURITY_REVERT_CVE_2019_9512)) + nghttp2_option_set_max_outbound_ack(options_, 10000); + // The padding strategy sets the mechanism by which we determine how much // additional frame padding to apply to DATA and HEADERS frames. Currently // this is set on a per-session basis, but eventually we may switch to diff --git a/src/node_revert.h b/src/node_revert.h index 33b30a0dfe7f00..66161c9c9b2048 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -16,6 +16,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ + XX(CVE_2019_9512, "CVE-2019-9512", "HTTP/2 Ping/Settings Flood") \ XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak") \ XX(CVE_2019_9518, "CVE-2019-9518", "HTTP/2 Empty DATA Frame Flooding") \ From 8b1cd59f61260528765d49762a10e05fa65fbf27 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 14 Aug 2019 20:42:03 +0200 Subject: [PATCH 13/13] fixup! deps: update nghttp2 to 1.39.2 --- deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 1f1d4808ca27c0..45bb0c9102cb05 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.33.0" +#define NGHTTP2_VERSION "1.39.2" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012100 +#define NGHTTP2_VERSION_NUM 0x012702 #endif /* NGHTTP2VER_H */