From f52da9484d0c6c28ea5a4b32a9a2818e91d8a2ab Mon Sep 17 00:00:00 2001 From: Chris Opperwall Date: Mon, 19 Oct 2020 18:24:08 -0700 Subject: [PATCH 1/2] debugger: validate sec-websocket-accept response header This addresses a TODO to validate that the sec-websocket-accept header in the WebSocket handshake response is valid. To do this we need to append the WebSocket GUID to the original key sent in sec-websocket-key, sha1 hash it, and then compare the base64 encoding with the value sent in the sec-websocket-accept response header. If they don't match, an error is thrown. PR-URL: https://github.com/nodejs/node/pull/39357 Refs: https://github.com/nodejs/node-inspect/pull/93 Reviewed-By: Colin Ihrig --- lib/internal/debugger/inspect_client.js | 26 +++++++++++++++++++++---- src/node_native_module.cc | 1 + 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/lib/internal/debugger/inspect_client.js b/lib/internal/debugger/inspect_client.js index cd277a4cf5a946..419b984cc9f473 100644 --- a/lib/internal/debugger/inspect_client.js +++ b/lib/internal/debugger/inspect_client.js @@ -11,6 +11,7 @@ const { } = primordials; const Buffer = require('buffer').Buffer; +const crypto = require('crypto'); const { ERR_DEBUGGER_ERROR } = require('internal/errors').codes; const { EventEmitter } = require('events'); const http = require('http'); @@ -35,6 +36,10 @@ const kTwoBytePayloadLengthField = 126; const kEightBytePayloadLengthField = 127; const kMaskingKeyWidthInBytes = 4; +// This guid is defined in the Websocket Protocol RFC +// https://tools.ietf.org/html/rfc6455#section-1.3 +const WEBSOCKET_HANDSHAKE_GUID = '258EAFA5-E914-47DA-95CA-C5AB0DC85B11'; + function unpackError({ code, message, data }) { const err = new ERR_DEBUGGER_ERROR(`${message} - ${data}`); err.code = code; @@ -42,6 +47,19 @@ function unpackError({ code, message, data }) { return err; } +function validateHandshake(requestKey, responseKey) { + const expectedResponseKeyBase = requestKey + WEBSOCKET_HANDSHAKE_GUID; + const shasum = crypto.createHash('sha1'); + shasum.update(expectedResponseKeyBase); + const shabuf = shasum.digest(); + + if (shabuf.toString('base64') !== responseKey) { + throw new ERR_DEBUGGER_ERROR( + `WebSocket secret mismatch: ${requestKey} did not match ${responseKey}` + ); + } +} + function encodeFrameHybi17(payload) { var i; @@ -287,8 +305,8 @@ class Client extends EventEmitter { _connectWebsocket(urlPath) { this.reset(); - const key1 = require('crypto').randomBytes(16).toString('base64'); - debuglog('request websocket', key1); + const requestKey = crypto.randomBytes(16).toString('base64'); + debuglog('request WebSocket', requestKey); const httpReq = this._http = http.request({ host: this._host, @@ -297,7 +315,7 @@ class Client extends EventEmitter { headers: { 'Connection': 'Upgrade', 'Upgrade': 'websocket', - 'Sec-WebSocket-Key': key1, + 'Sec-WebSocket-Key': requestKey, 'Sec-WebSocket-Version': '13', }, }); @@ -314,7 +332,7 @@ class Client extends EventEmitter { }); const handshakeListener = (res, socket) => { - // TODO: we *could* validate res.headers[sec-websocket-accept] + validateHandshake(requestKey, res.headers['sec-websocket-accept']); debuglog('websocket upgrade'); this._socket = socket; diff --git a/src/node_native_module.cc b/src/node_native_module.cc index 2642982330e8cf..a5aa1edaf91143 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -70,6 +70,7 @@ void NativeModuleLoader::InitializeModuleCategories() { std::vector prefixes = { #if !HAVE_OPENSSL "internal/crypto/", + "internal/debugger/", #endif // !HAVE_OPENSSL "internal/bootstrap/", From 36bcc2925f633153da7e9ccda46b8ff75b062c95 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 11 Jul 2021 14:43:15 -0700 Subject: [PATCH 2/2] test: add test for WebSocket secret verification in debugger PR-URL: https://github.com/nodejs/node/pull/39357 Refs: https://github.com/nodejs/node-inspect/pull/93 Reviewed-By: Colin Ihrig --- ...test-debugger-websocket-secret-mismatch.js | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 test/parallel/test-debugger-websocket-secret-mismatch.js diff --git a/test/parallel/test-debugger-websocket-secret-mismatch.js b/test/parallel/test-debugger-websocket-secret-mismatch.js new file mode 100644 index 00000000000000..9b7b50c518f186 --- /dev/null +++ b/test/parallel/test-debugger-websocket-secret-mismatch.js @@ -0,0 +1,54 @@ +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const childProcess = require('child_process'); +const http = require('http'); + +let port; + +const server = http.createServer(common.mustCall((req, res) => { + if (req.url.startsWith('/json')) { + res.writeHead(200); + res.end(`[ { + "description": "", + "devtoolsFrontendUrl": "/devtools/inspector.html?ws=localhost:${port}/devtools/page/DAB7FB6187B554E10B0BD18821265734", + "cid": "DAB7FB6187B554E10B0BD18821265734", + "title": "Fhqwhgads", + "type": "page", + "url": "https://www.example.com/", + "webSocketDebuggerUrl": "ws://localhost:${port}/devtools/page/DAB7FB6187B554E10B0BD18821265734" + } ]`); + } else { + res.setHeader('Upgrade', 'websocket'); + res.setHeader('Connection', 'Upgrade'); + res.setHeader('Sec-WebSocket-Accept', 'fhqwhgads'); + res.setHeader('Sec-WebSocket-Protocol', 'chat'); + res.writeHead(101); + res.end(); + } +}, 2)).listen(0, common.mustCall(() => { + port = server.address().port; + const proc = + childProcess.spawn(process.execPath, ['inspect', `localhost:${port}`]); + + let stdout = ''; + proc.stdout.on('data', (data) => { + stdout += data.toString(); + assert.doesNotMatch(stdout, /\bok\b/); + }); + + let stderr = ''; + proc.stderr.on('data', (data) => { + stderr += data.toString(); + }); + + proc.on('exit', common.mustCall((code, signal) => { + assert.match(stderr, /\bWebSocket secret mismatch\b/); + assert.notStrictEqual(code, 0); + assert.strictEqual(signal, null); + server.close(); + })); +}));