From e814110ee6cd269a8982a529e38b55632c37ad28 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Tue, 22 Jun 2021 12:29:55 +0200 Subject: [PATCH] [major] Make the Sec-WebSocket-Extensions header parser stricter Make the parser throw an error if the header field value is empty or if it begins or ends with a white space. --- lib/extension.js | 13 +++++++------ lib/websocket-server.js | 11 ++++++++--- lib/websocket.js | 37 +++++++++++++++++-------------------- test/extension.test.js | 18 ++++++++++-------- 4 files changed, 42 insertions(+), 37 deletions(-) diff --git a/lib/extension.js b/lib/extension.js index 0d833fee6..3d7895c1b 100644 --- a/lib/extension.js +++ b/lib/extension.js @@ -26,9 +26,6 @@ function push(dest, name, elem) { */ function parse(header) { const offers = Object.create(null); - - if (header === undefined || header === '') return offers; - let params = Object.create(null); let mustUnescape = false; let isEscaping = false; @@ -36,16 +33,20 @@ function parse(header) { let extensionName; let paramName; let start = -1; + let code = -1; let end = -1; let i = 0; for (; i < header.length; i++) { - const code = header.charCodeAt(i); + code = header.charCodeAt(i); if (extensionName === undefined) { if (end === -1 && tokenChars[code] === 1) { if (start === -1) start = i; - } else if (code === 0x20 /* ' ' */ || code === 0x09 /* '\t' */) { + } else if ( + i !== 0 && + (code === 0x20 /* ' ' */ || code === 0x09) /* '\t' */ + ) { if (end === -1 && start !== -1) end = i; } else if (code === 0x3b /* ';' */ || code === 0x2c /* ',' */) { if (start === -1) { @@ -146,7 +147,7 @@ function parse(header) { } } - if (start === -1 || inQuotes) { + if (start === -1 || inQuotes || code === 0x20 || code === 0x09) { throw new SyntaxError('Unexpected end of input'); } diff --git a/lib/websocket-server.js b/lib/websocket-server.js index cb93aeeb3..e2f1af616 100644 --- a/lib/websocket-server.js +++ b/lib/websocket-server.js @@ -212,7 +212,6 @@ class WebSocketServer extends EventEmitter { ? req.headers['sec-websocket-key'] : false; const version = +req.headers['sec-websocket-version']; - const extensions = {}; if ( req.method !== 'GET' || @@ -236,7 +235,13 @@ class WebSocketServer extends EventEmitter { } } - if (this.options.perMessageDeflate) { + const secWebSocketExtensions = req.headers['sec-websocket-extensions']; + const extensions = {}; + + if ( + this.options.perMessageDeflate && + secWebSocketExtensions !== undefined + ) { const perMessageDeflate = new PerMessageDeflate( this.options.perMessageDeflate, true, @@ -244,7 +249,7 @@ class WebSocketServer extends EventEmitter { ); try { - const offers = extension.parse(req.headers['sec-websocket-extensions']); + const offers = extension.parse(secWebSocketExtensions); if (offers[PerMessageDeflate.extensionName]) { perMessageDeflate.accept(offers[PerMessageDeflate.extensionName]); diff --git a/lib/websocket.js b/lib/websocket.js index 37949de15..400a610d1 100644 --- a/lib/websocket.js +++ b/lib/websocket.js @@ -801,28 +801,25 @@ function initAsClient(websocket, address, protocols, options) { const extensionNames = Object.keys(extensions); - if (extensionNames.length) { - if ( - extensionNames.length !== 1 || - extensionNames[0] !== PerMessageDeflate.extensionName - ) { - const message = - 'Server indicated an extension that was not requested'; - abortHandshake(websocket, socket, message); - return; - } - - try { - perMessageDeflate.accept(extensions[PerMessageDeflate.extensionName]); - } catch (err) { - const message = 'Invalid Sec-WebSocket-Extensions header'; - abortHandshake(websocket, socket, message); - return; - } + if ( + extensionNames.length !== 1 || + extensionNames[0] !== PerMessageDeflate.extensionName + ) { + const message = 'Server indicated an extension that was not requested'; + abortHandshake(websocket, socket, message); + return; + } - websocket._extensions[PerMessageDeflate.extensionName] = - perMessageDeflate; + try { + perMessageDeflate.accept(extensions[PerMessageDeflate.extensionName]); + } catch (err) { + const message = 'Invalid Sec-WebSocket-Extensions header'; + abortHandshake(websocket, socket, message); + return; } + + websocket._extensions[PerMessageDeflate.extensionName] = + perMessageDeflate; } websocket.setSocket(socket, head, opts.maxPayload); diff --git a/test/extension.test.js b/test/extension.test.js index 6cfbc1b23..a4b3e749d 100644 --- a/test/extension.test.js +++ b/test/extension.test.js @@ -6,11 +6,6 @@ const { format, parse } = require('../lib/extension'); describe('extension', () => { describe('parse', () => { - it('returns an empty object if the argument is `undefined`', () => { - assert.deepStrictEqual(parse(), { __proto__: null }); - assert.deepStrictEqual(parse(''), { __proto__: null }); - }); - it('parses a single extension', () => { assert.deepStrictEqual(parse('foo'), { foo: [{ __proto__: null }], @@ -73,7 +68,7 @@ describe('extension', () => { }); it('ignores the optional white spaces', () => { - const header = 'foo; bar\t; \tbaz=1\t ; bar="1"\t\t, \tqux\t ;norf '; + const header = 'foo; bar\t; \tbaz=1\t ; bar="1"\t\t, \tqux\t ;norf'; assert.deepStrictEqual(parse(header), { foo: [{ bar: [true, '1'], baz: ['1'], __proto__: null }], @@ -105,10 +100,12 @@ describe('extension', () => { it('throws an error if a white space is misplaced', () => { [ + [' foo', 0], ['f oo', 2], ['foo;ba r', 7], ['foo;bar =', 8], - ['foo;bar= ', 8] + ['foo;bar= ', 8], + ['foo;bar=ba z', 11] ].forEach((element) => { assert.throws( () => parse(element[0]), @@ -147,13 +144,18 @@ describe('extension', () => { it('throws an error if the header value ends prematurely', () => { [ + '', + 'foo ', + 'foo\t', 'foo, ', 'foo;', + 'foo;bar ', 'foo;bar,', 'foo;bar; ', 'foo;bar=', 'foo;bar="baz', - 'foo;bar="1\\' + 'foo;bar="1\\', + 'foo;bar="baz" ' ].forEach((header) => { assert.throws( () => parse(header),