From e6e99eb4472a0162c4b27df76479b1814c8cca82 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 | 8 +- .../parallel/test-http2-server-socketerror.js | 12 +-- test/parallel/test-http2-socket-proxy.js | 88 +++++++++++++++++++ 14 files changed, 200 insertions(+), 73 deletions(-) create mode 100644 test/parallel/test-http2-socket-proxy.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 7120476656d1ea..eac21bae562413 100755 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -725,8 +725,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 e6261ad62e035c..abf0ac5b5225d8 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 1ebc1b3ca2d7e9..ca08652e636af6 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -199,8 +199,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 84f15b2ed8a19d..c96fc931969f39 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 77f7ae40c9151e..71489ba4ec9bad 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 4610be7e4d376e..2426a409cfc6cf 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 f10bf60ce34fc0..8b91f2d21041a0 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 f56a45cc3b5ed6..faf4643b0304e3 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 63bbd7dbd4f1cc..f62c782a45d8ea 100644 --- a/test/parallel/test-http2-compat-socket-set.js +++ b/test/parallel/test-http2-compat-socket-set.js @@ -13,8 +13,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 e153b29bb78cfb..ae8b4255a39484 100644 --- a/test/parallel/test-http2-compat-socket.js +++ b/test/parallel/test-http2-compat-socket.js @@ -15,8 +15,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 7447d4e5c635ed..62a79148dcac47 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 6bf5934e3e0413..8291c415284571 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -1,11 +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, @@ -25,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 069297aa3fa814..24945e531af42e 100644 --- a/test/parallel/test-http2-server-socketerror.js +++ b/test/parallel/test-http2-server-socketerror.js @@ -1,4 +1,5 @@ -// +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -6,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) => { @@ -20,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); @@ -36,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 00000000000000..60f31837790d51 --- /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(); + })); +}));