From dc1781bc319cb347878d11cf730947d0bef69a51 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Wed, 25 May 2022 15:33:05 +0200 Subject: [PATCH] [security] Drop sensitive headers when following insecure redirects Drop the `Authorization` and `Cookie` headers if the original request for the opening handshake is sent over HTTPS and the client is redirected to the same host over plain HTTP (wss: to ws:). If an HTTPS server redirects to same host over plain HTTP, the problem is on the server, but handling this condition is not hard and reduces the risk of leaking credentials due to MITM issues. Refs: https://github.com/websockets/ws/commit/6946f5fe --- lib/websocket.js | 25 +++++--- test/websocket.test.js | 128 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 9 deletions(-) diff --git a/lib/websocket.js b/lib/websocket.js index 1802274b0..9948ffed9 100644 --- a/lib/websocket.js +++ b/lib/websocket.js @@ -684,6 +684,7 @@ function initAsClient(websocket, address, protocols, options) { if (opts.followRedirects) { if (websocket._redirects === 0) { + websocket._originalSecure = isSecure; websocket._originalHost = parsedUrl.host; const headers = options && options.headers; @@ -699,15 +700,21 @@ function initAsClient(websocket, address, protocols, options) { options.headers[key.toLowerCase()] = value; } } - } else if (parsedUrl.host !== websocket._originalHost) { - // - // Match curl 7.77.0 behavior and drop the following headers. These - // headers are also dropped when following a redirect to a subdomain. - // - delete opts.headers.authorization; - delete opts.headers.cookie; - delete opts.headers.host; - opts.auth = undefined; + } else { + const isSameHost = parsedUrl.host === websocket._originalHost; + + if (!isSameHost || (websocket._originalSecure && !isSecure)) { + // + // Match curl 7.77.0 behavior and drop the following headers. These + // headers are also dropped when following a redirect to a subdomain. + // + delete opts.headers.authorization; + delete opts.headers.cookie; + + if (!isSameHost) delete opts.headers.host; + + opts.auth = undefined; + } } // diff --git a/test/websocket.test.js b/test/websocket.test.js index 36d3ba0a6..d604bb00a 100644 --- a/test/websocket.test.js +++ b/test/websocket.test.js @@ -6,6 +6,7 @@ const assert = require('assert'); const crypto = require('crypto'); const https = require('https'); const http = require('http'); +const net = require('net'); const tls = require('tls'); const fs = require('fs'); const { URL } = require('url'); @@ -1037,6 +1038,133 @@ describe('WebSocket', () => { }); }); + describe('When moving away from a secure context', () => { + function proxy(httpServer, httpsServer) { + const server = net.createServer({ allowHalfOpen: true }); + + server.on('connection', (socket) => { + socket.on('readable', function read() { + socket.removeListener('readable', read); + + const buf = socket.read(1); + const target = buf[0] === 22 ? httpsServer : httpServer; + + socket.unshift(buf); + target.emit('connection', socket); + }); + }); + + return server; + } + + it('drops the `auth` option', (done) => { + const httpServer = http.createServer(); + const httpsServer = https.createServer({ + cert: fs.readFileSync('test/fixtures/certificate.pem'), + key: fs.readFileSync('test/fixtures/key.pem') + }); + const server = proxy(httpServer, httpsServer); + + server.listen(() => { + const port = server.address().port; + + httpsServer.on('upgrade', (req, socket) => { + socket.on('error', NOOP); + socket.end( + 'HTTP/1.1 302 Found\r\n' + + `Location: ws://localhost:${port}/\r\n\r\n` + ); + }); + + const wss = new WebSocket.Server({ server: httpServer }); + + wss.on('connection', (ws, req) => { + assert.strictEqual(req.headers.authorization, undefined); + ws.close(); + }); + + const ws = new WebSocket(`wss://localhost:${server.address().port}`, { + auth: 'foo:bar', + followRedirects: true, + rejectUnauthorized: false + }); + + assert.strictEqual( + ws._req.getHeader('Authorization'), + 'Basic Zm9vOmJhcg==' + ); + + ws.on('close', (code) => { + assert.strictEqual(code, 1005); + assert.strictEqual(ws.url, `ws://localhost:${port}/`); + assert.strictEqual(ws._redirects, 1); + + server.close(done); + }); + }); + }); + + it('drops the Authorization, and Cookie headers', (done) => { + const headers = { + authorization: 'Basic Zm9vOmJhcg==', + cookie: 'foo=bar', + host: 'foo' + }; + + const httpServer = http.createServer(); + const httpsServer = https.createServer({ + cert: fs.readFileSync('test/fixtures/certificate.pem'), + key: fs.readFileSync('test/fixtures/key.pem') + }); + const server = proxy(httpServer, httpsServer); + + server.listen(() => { + const port = server.address().port; + + httpsServer.on('upgrade', (req, socket) => { + socket.on('error', NOOP); + socket.end( + 'HTTP/1.1 302 Found\r\n' + + `Location: ws://localhost:${port}/\r\n\r\n` + ); + }); + + const wss = new WebSocket.Server({ server: httpServer }); + + wss.on('connection', (ws, req) => { + assert.strictEqual(req.headers.authorization, undefined); + assert.strictEqual(req.headers.cookie, undefined); + assert.strictEqual(req.headers.host, 'foo'); + + ws.close(); + }); + + const ws = new WebSocket(`wss://localhost:${server.address().port}`, { + headers, + followRedirects: true, + rejectUnauthorized: false + }); + + const firstRequest = ws._req; + + assert.strictEqual( + firstRequest.getHeader('Authorization'), + headers.authorization + ); + assert.strictEqual(firstRequest.getHeader('Cookie'), headers.cookie); + assert.strictEqual(firstRequest.getHeader('Host'), headers.host); + + ws.on('close', (code) => { + assert.strictEqual(code, 1005); + assert.strictEqual(ws.url, `ws://localhost:${port}/`); + assert.strictEqual(ws._redirects, 1); + + server.close(done); + }); + }); + }); + }); + describe('When the redirect host is different', () => { it('drops the `auth` option', (done) => { const wss = new WebSocket.Server({ port: 0 }, () => {