From e4ed2b06e449f11f4e8f7587946b64e0befba60b Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 19 Oct 2017 21:32:20 -0400 Subject: [PATCH] http2: do not allow socket manipulation Because of the specific serialization and processing requirements of HTTP/2, sockets should not be directly manipulated. This forbids any interactions with destroy, emit, end, pause, read, resume and write methods of the socket. It also redirects setTimeout to session instead of socket. PR-URL: https://github.com/nodejs/node/pull/16330 Fixes: https://github.com/nodejs/node/issues/16252 Refs: https://github.com/nodejs/node/pull/16211 Reviewed-By: James M Snell --- doc/api/errors.md | 4 +- doc/api/http2.md | 28 +++--- lib/internal/errors.js | 3 +- lib/internal/http2/compat.js | 13 +-- lib/internal/http2/core.js | 82 ++++++++++------- lib/internal/http2/util.js | 3 + test/parallel/test-http2-client-destroy.js | 11 ++- .../test-http2-client-socket-destroy.js | 6 +- test/parallel/test-http2-compat-socket-set.js | 4 +- test/parallel/test-http2-compat-socket.js | 4 +- ...test-http2-create-client-secure-session.js | 7 +- .../test-http2-server-socket-destroy.js | 7 +- .../parallel/test-http2-server-socketerror.js | 11 ++- test/parallel/test-http2-socket-proxy.js | 88 +++++++++++++++++++ 14 files changed, 200 insertions(+), 71 deletions(-) create mode 100644 test/parallel/test-http2-socket-proxy.js diff --git a/doc/api/errors.md b/doc/api/errors.md index b78c6b36d6..6de2e6f3af 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -810,8 +810,8 @@ reached. ### ERR_HTTP2_NO_SOCKET_MANIPULATION -Used when attempting to read, write, pause, and/or resume a socket attached to -an `Http2Session`. +Used when attempting to directly manipulate (e.g read, write, pause, resume, +etc.) a socket attached to an `Http2Session`. ### ERR_HTTP2_OUT_OF_STREAMS diff --git a/doc/api/http2.md b/doc/api/http2.md index e6261ad62e..abf0ac5b52 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -463,12 +463,16 @@ added: v8.4.0 * Value: {net.Socket|tls.TLSSocket} -A reference to the [`net.Socket`][] or [`tls.TLSSocket`][] to which this -`Http2Session` instance is bound. +Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but +limits available methods to ones safe to use with HTTP/2. -*Note*: It is not recommended for user code to interact directly with a -`Socket` bound to an `Http2Session`. See [Http2Session and Sockets][] for -details. +`destroy`, `emit`, `end`, `pause`, `read`, `resume`, and `write` will throw +an error with code `ERR_HTTP2_NO_SOCKET_MANIPULATION`. See +[Http2Session and Sockets][] for more information. + +`setTimeout` method will be called on this `Http2Session`. + +All other interactions will be routed directly to the socket. #### http2session.state -* {net.Socket} +* {net.Socket|tls.TLSSocket} -Returns a Proxy object that acts as a `net.Socket` but applies getters, -setters and methods based on HTTP/2 logic. +Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but +applies getters, setters and methods based on HTTP/2 logic. `destroyed`, `readable`, and `writable` properties will be retrieved from and set on `request.stream`. @@ -2293,7 +2297,7 @@ will result in a [`TypeError`][] being thrown. added: v8.4.0 --> -* {net.Socket} +* {net.Socket|tls.TLSSocket} See [`response.socket`][]. @@ -2510,10 +2514,10 @@ Returns `response`. added: v8.4.0 --> -* {net.Socket} +* {net.Socket|tls.TLSSocket} -Returns a Proxy object that acts as a `net.Socket` but applies getters, -setters and methods based on HTTP/2 logic. +Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but +applies getters, setters and methods based on HTTP/2 logic. `destroyed`, `readable`, and `writable` properties will be retrieved from and set on `response.stream`. diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 408a0e9507..b8bda9d2b2 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -208,8 +208,7 @@ E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed'); E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK', (max) => `Maximum number of pending settings acknowledgements (${max})`); E('ERR_HTTP2_NO_SOCKET_MANIPULATION', - 'HTTP/2 sockets should not be directly read from, written to, ' + - 'paused and/or resumed.'); + 'HTTP/2 sockets should not be directly manipulated (e.g. read and written)'); E('ERR_HTTP2_OUT_OF_STREAMS', 'No stream ID is available because maximum stream ID has been reached'); E('ERR_HTTP2_PAYLOAD_FORBIDDEN', diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 84f15b2ed8..c96fc93196 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -5,6 +5,7 @@ const Readable = Stream.Readable; const binding = process.binding('http2'); const constants = binding.constants; const errors = require('internal/errors'); +const { kSocket } = require('internal/http2/util'); const kFinish = Symbol('finish'); const kBeginSend = Symbol('begin-send'); @@ -176,15 +177,15 @@ const proxySocketHandler = { throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const ref = stream.session !== undefined ? - stream.session.socket : stream; + stream.session[kSocket] : stream; const value = ref[prop]; return typeof value === 'function' ? value.bind(ref) : value; } }, getPrototypeOf(stream) { if (stream.session !== undefined) - return stream.session.socket.constructor.prototype; - return stream.prototype; + return Reflect.getPrototypeOf(stream.session[kSocket]); + return Reflect.getPrototypeOf(stream); }, set(stream, prop, value) { switch (prop) { @@ -201,9 +202,9 @@ const proxySocketHandler = { case 'setTimeout': const session = stream.session; if (session !== undefined) - session[prop] = value; + session.setTimeout = value; else - stream[prop] = value; + stream.setTimeout = value; return true; case 'write': case 'read': @@ -212,7 +213,7 @@ const proxySocketHandler = { throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const ref = stream.session !== undefined ? - stream.session.socket : stream; + stream.session[kSocket] : stream; ref[prop] = value; return true; } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8d8f200262..727ca51798 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -38,6 +38,7 @@ const { getSettings, getStreamState, isPayloadMeaningless, + kSocket, mapToHeaders, NghttpError, sessionName, @@ -70,10 +71,10 @@ const kOptions = Symbol('options'); const kOwner = Symbol('owner'); const kProceed = Symbol('proceed'); const kProtocol = Symbol('protocol'); +const kProxySocket = Symbol('proxy-socket'); const kRemoteSettings = Symbol('remote-settings'); const kServer = Symbol('server'); const kSession = Symbol('session'); -const kSocket = Symbol('socket'); const kState = Symbol('state'); const kType = Symbol('type'); @@ -672,6 +673,48 @@ function finishSessionDestroy(self, socket) { debug(`[${sessionName(self[kType])}] nghttp2session destroyed`); } +const proxySocketHandler = { + get(session, prop) { + switch (prop) { + case 'setTimeout': + return session.setTimeout.bind(session); + case 'destroy': + case 'emit': + case 'end': + case 'pause': + case 'read': + case 'resume': + case 'write': + throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); + default: + const socket = session[kSocket]; + const value = socket[prop]; + return typeof value === 'function' ? value.bind(socket) : value; + } + }, + getPrototypeOf(session) { + return Reflect.getPrototypeOf(session[kSocket]); + }, + set(session, prop, value) { + switch (prop) { + case 'setTimeout': + session.setTimeout = value; + return true; + case 'destroy': + case 'emit': + case 'end': + case 'pause': + case 'read': + case 'resume': + case 'write': + throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); + default: + session[kSocket][prop] = value; + return true; + } + } +}; + // Upon creation, the Http2Session takes ownership of the socket. The session // may not be ready to use immediately if the socket is not yet fully connected. class Http2Session extends EventEmitter { @@ -707,6 +750,7 @@ class Http2Session extends EventEmitter { }; this[kType] = type; + this[kProxySocket] = null; this[kSocket] = socket; // Do not use nagle's algorithm @@ -756,7 +800,10 @@ class Http2Session extends EventEmitter { // The socket owned by this session get socket() { - return this[kSocket]; + const proxySocket = this[kProxySocket]; + if (proxySocket === null) + return this[kProxySocket] = new Proxy(this, proxySocketHandler); + return proxySocket; } // The session type @@ -957,6 +1004,7 @@ class Http2Session extends EventEmitter { // Disassociate from the socket and server const socket = this[kSocket]; // socket.pause(); + delete this[kProxySocket]; delete this[kSocket]; delete this[kServer]; @@ -2155,30 +2203,6 @@ function socketDestroy(error) { this.destroy(error); } -function socketOnResume() { - if (this._paused) - return this.pause(); - if (this._handle && !this._handle.reading) { - this._handle.reading = true; - this._handle.readStart(); - } -} - -function socketOnPause() { - if (this._handle && this._handle.reading) { - this._handle.reading = false; - this._handle.readStop(); - } -} - -function socketOnDrain() { - const needPause = 0 > this._writableState.highWaterMark; - if (this._paused && !needPause) { - this._paused = false; - this.resume(); - } -} - // When an Http2Session emits an error, first try to forward it to the // server as a sessionError; failing that, forward it to the socket as // a sessionError; failing that, destroy, remove the error listener, and @@ -2267,9 +2291,6 @@ function connectionListener(socket) { } socket.on('error', socketOnError); - socket.on('resume', socketOnResume); - socket.on('pause', socketOnPause); - socket.on('drain', socketOnDrain); socket.on('close', socketOnClose); // Set up the Session @@ -2426,9 +2447,6 @@ function connect(authority, options, listener) { } socket.on('error', socketOnError); - socket.on('resume', socketOnResume); - socket.on('pause', socketOnPause); - socket.on('drain', socketOnDrain); socket.on('close', socketOnClose); const session = new ClientHttp2Session(options, socket); diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 4610be7e4d..2426a409cf 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -3,6 +3,8 @@ const binding = process.binding('http2'); const errors = require('internal/errors'); +const kSocket = Symbol('socket'); + const { NGHTTP2_SESSION_CLIENT, NGHTTP2_SESSION_SERVER, @@ -551,6 +553,7 @@ module.exports = { getSettings, getStreamState, isPayloadMeaningless, + kSocket, mapToHeaders, NghttpError, sessionName, diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index f10bf60ce3..8b91f2d210 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -1,3 +1,5 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -5,6 +7,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); { const server = h2.createServer(); @@ -13,7 +16,7 @@ const h2 = require('http2'); common.mustCall(() => { const destroyCallbacks = [ (client) => client.destroy(), - (client) => client.socket.destroy() + (client) => client[kSocket].destroy() ]; let remaining = destroyCallbacks.length; @@ -23,9 +26,9 @@ const h2 = require('http2'); client.on( 'connect', common.mustCall(() => { - const socket = client.socket; + const socket = client[kSocket]; - assert(client.socket, 'client session has associated socket'); + assert(socket, 'client session has associated socket'); assert( !client.destroyed, 'client has not been destroyed before destroy is called' @@ -41,7 +44,7 @@ const h2 = require('http2'); destroyCallback(client); assert( - !client.socket, + !client[kSocket], 'client.socket undefined after destroy is called' ); diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js index f56a45cc3b..faf4643b03 100644 --- a/test/parallel/test-http2-client-socket-destroy.js +++ b/test/parallel/test-http2-client-socket-destroy.js @@ -1,9 +1,13 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); + const body = '

this is some data

'; @@ -32,7 +36,7 @@ server.on('listening', common.mustCall(function() { req.on('response', common.mustCall(() => { // send a premature socket close - client.socket.destroy(); + client[kSocket].destroy(); })); req.on('data', common.mustNotCall()); diff --git a/test/parallel/test-http2-compat-socket-set.js b/test/parallel/test-http2-compat-socket-set.js index 7411fef602..87b7a2b444 100644 --- a/test/parallel/test-http2-compat-socket-set.js +++ b/test/parallel/test-http2-compat-socket-set.js @@ -12,8 +12,8 @@ const h2 = require('http2'); const errMsg = { code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION', type: Error, - message: 'HTTP/2 sockets should not be directly read from, written to, ' + - 'paused and/or resumed.' + message: 'HTTP/2 sockets should not be directly manipulated ' + + '(e.g. read and written)' }; const server = h2.createServer(); diff --git a/test/parallel/test-http2-compat-socket.js b/test/parallel/test-http2-compat-socket.js index 8292232f74..aed74149f7 100644 --- a/test/parallel/test-http2-compat-socket.js +++ b/test/parallel/test-http2-compat-socket.js @@ -14,8 +14,8 @@ const net = require('net'); const errMsg = { code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION', type: Error, - message: 'HTTP/2 sockets should not be directly read from, written to, ' + - 'paused and/or resumed.' + message: 'HTTP/2 sockets should not be directly manipulated ' + + '(e.g. read and written)' }; const server = h2.createServer(); diff --git a/test/parallel/test-http2-create-client-secure-session.js b/test/parallel/test-http2-create-client-secure-session.js index 7447d4e5c6..62a79148dc 100644 --- a/test/parallel/test-http2-create-client-secure-session.js +++ b/test/parallel/test-http2-create-client-secure-session.js @@ -1,3 +1,5 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -8,6 +10,7 @@ if (!common.hasCrypto) const assert = require('assert'); const fixtures = require('../common/fixtures'); const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); const tls = require('tls'); function loadKey(keyname) { @@ -15,7 +18,7 @@ function loadKey(keyname) { } function onStream(stream, headers) { - const socket = stream.session.socket; + const socket = stream.session[kSocket]; assert(headers[':authority'].startsWith(socket.servername)); stream.respond({ 'content-type': 'text/html', @@ -55,7 +58,7 @@ function verifySecureSession(key, cert, ca, opts) { assert.strictEqual(jsonData.servername, opts.servername || 'localhost'); assert.strictEqual(jsonData.alpnProtocol, 'h2'); server.close(); - client.socket.destroy(); + client[kSocket].destroy(); })); req.end(); }); diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 0218f7357a..8291c41528 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -1,10 +1,13 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const h2 = require('http2'); const assert = require('assert'); +const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); const { HTTP2_HEADER_METHOD, @@ -24,7 +27,7 @@ function onStream(stream) { }); stream.write('test'); - const socket = stream.session.socket; + const socket = stream.session[kSocket]; // When the socket is destroyed, the close events must be triggered // on the socket, server and session. diff --git a/test/parallel/test-http2-server-socketerror.js b/test/parallel/test-http2-server-socketerror.js index cad92fd299..24945e531a 100644 --- a/test/parallel/test-http2-server-socketerror.js +++ b/test/parallel/test-http2-server-socketerror.js @@ -1,3 +1,5 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -5,6 +7,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const http2 = require('http2'); +const { kSocket } = require('internal/http2/util'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { @@ -19,12 +22,12 @@ server.on('session', common.mustCall((session) => { type: Error, message: 'test' })(error); - assert.strictEqual(socket, session.socket); + assert.strictEqual(socket, session[kSocket]); }); const isNotCalled = common.mustNotCall(); session.on('socketError', handler); server.on('socketError', isNotCalled); - session.socket.emit('error', new Error('test')); + session[kSocket].emit('error', new Error('test')); session.removeListener('socketError', handler); server.removeListener('socketError', isNotCalled); @@ -35,10 +38,10 @@ server.on('session', common.mustCall((session) => { type: Error, message: 'test' })(error); - assert.strictEqual(socket, session.socket); + assert.strictEqual(socket, session[kSocket]); assert.strictEqual(session, session); })); - session.socket.emit('error', new Error('test')); + session[kSocket].emit('error', new Error('test')); })); server.listen(0, common.mustCall(() => { diff --git a/test/parallel/test-http2-socket-proxy.js b/test/parallel/test-http2-socket-proxy.js new file mode 100644 index 0000000000..60f3183779 --- /dev/null +++ b/test/parallel/test-http2-socket-proxy.js @@ -0,0 +1,88 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const h2 = require('http2'); +const net = require('net'); + +// Tests behaviour of the proxied socket on Http2Session + +const errMsg = { + code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION', + type: Error, + message: 'HTTP/2 sockets should not be directly manipulated ' + + '(e.g. read and written)' +}; + +const server = h2.createServer(); + +server.on('stream', common.mustCall(function(stream, headers) { + const socket = stream.session.socket; + const session = stream.session; + + assert.ok(socket instanceof net.Socket); + + assert.strictEqual(socket.writable, true); + assert.strictEqual(socket.readable, true); + assert.strictEqual(typeof socket.address(), 'object'); + + socket.setTimeout(987); + assert.strictEqual(session._idleTimeout, 987); + + common.expectsError(() => socket.destroy, errMsg); + common.expectsError(() => socket.emit, errMsg); + common.expectsError(() => socket.end, errMsg); + common.expectsError(() => socket.pause, errMsg); + common.expectsError(() => socket.read, errMsg); + common.expectsError(() => socket.resume, errMsg); + common.expectsError(() => socket.write, errMsg); + + common.expectsError(() => (socket.destroy = undefined), errMsg); + common.expectsError(() => (socket.emit = undefined), errMsg); + common.expectsError(() => (socket.end = undefined), errMsg); + common.expectsError(() => (socket.pause = undefined), errMsg); + common.expectsError(() => (socket.read = undefined), errMsg); + common.expectsError(() => (socket.resume = undefined), errMsg); + common.expectsError(() => (socket.write = undefined), errMsg); + + assert.doesNotThrow(() => (socket.on = socket.on)); + assert.doesNotThrow(() => (socket.once = socket.once)); + + stream.respond(); + + socket.writable = 0; + socket.readable = 0; + assert.strictEqual(socket.writable, 0); + assert.strictEqual(socket.readable, 0); + + stream.session.destroy(); + + socket.setTimeout = undefined; + assert.strictEqual(session.setTimeout, undefined); + + stream.session.on('close', common.mustCall(() => { + assert.strictEqual(session.socket, undefined); + })); +})); + +server.listen(0, common.mustCall(function() { + const port = server.address().port; + const url = `http://localhost:${port}`; + const client = h2.connect(url, common.mustCall(() => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.on('end', common.mustCall(() => { + client.destroy(); + server.close(); + })); + request.end(); + request.resume(); + })); +}));