Skip to content

Commit e9e2997

Browse files
committed
fix: validate server_max_window_bits range in permessage-deflate
The isValidClientWindowBits() function only checked for ASCII digits, allowing out-of-range values like "1000" to pass validation. When these values were passed to zlib's createInflateRaw(), it threw an unhandled RangeError that crashed the process. Changes: - Update isValidClientWindowBits() to validate range 8-15 (per RFC 7692) - Add try-catch around createInflateRaw() as defense in depth - Add comprehensive tests for windowBits validation (cherry picked from commit cb79c57)
1 parent 4b4f93a commit e9e2997

File tree

3 files changed

+257
-2
lines changed

3 files changed

+257
-2
lines changed

lib/web/websocket/permessage-deflate.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,12 @@ class PerMessageDeflate {
5959
windowBits = Number.parseInt(this.#options.serverMaxWindowBits)
6060
}
6161

62-
this.#inflate = createInflateRaw({ windowBits })
62+
try {
63+
this.#inflate = createInflateRaw({ windowBits })
64+
} catch (err) {
65+
callback(err)
66+
return
67+
}
6368
this.#inflate[kBuffer] = []
6469
this.#inflate[kLength] = 0
6570

lib/web/websocket/util.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,12 @@ function parseExtensions (extensions) {
266266
* @param {string} value
267267
*/
268268
function isValidClientWindowBits (value) {
269+
// Must have at least one character
270+
if (value.length === 0) {
271+
return false
272+
}
273+
274+
// Check all characters are ASCII digits
269275
for (let i = 0; i < value.length; i++) {
270276
const byte = value.charCodeAt(i)
271277

@@ -274,7 +280,9 @@ function isValidClientWindowBits (value) {
274280
}
275281
}
276282

277-
return true
283+
// Check numeric range: zlib requires windowBits in range 8-15
284+
const num = Number.parseInt(value, 10)
285+
return num >= 8 && num <= 15
278286
}
279287

280288
// https://nodejs.org/api/intl.html#detecting-internationalization-support
Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
'use strict'
2+
3+
const { describe, test, after } = require('node:test')
4+
const { once } = require('node:events')
5+
const http = require('node:http')
6+
const crypto = require('node:crypto')
7+
const zlib = require('node:zlib')
8+
const { WebSocket } = require('../..')
9+
const { isValidClientWindowBits } = require('../../lib/web/websocket/util')
10+
11+
/**
12+
* Creates a minimal WebSocket server that responds with a custom
13+
* server_max_window_bits value and sends a compressed frame.
14+
*/
15+
function createMaliciousServer (windowBitsValue) {
16+
const server = http.createServer()
17+
const sockets = new Set()
18+
19+
server.on('upgrade', (req, socket) => {
20+
sockets.add(socket)
21+
socket.on('close', () => sockets.delete(socket))
22+
23+
const key = req.headers['sec-websocket-key']
24+
const accept = crypto
25+
.createHash('sha1')
26+
.update(key + '258EAFA5-E914-47DA-95CA-C5AB0DC85B11')
27+
.digest('base64')
28+
29+
const headers = [
30+
'HTTP/1.1 101 Switching Protocols',
31+
'Upgrade: websocket',
32+
'Connection: Upgrade',
33+
`Sec-WebSocket-Accept: ${accept}`,
34+
`Sec-WebSocket-Extensions: permessage-deflate; server_max_window_bits=${windowBitsValue}`,
35+
'', ''
36+
]
37+
38+
socket.write(headers.join('\r\n'))
39+
40+
// Send a compressed frame to trigger decompression
41+
setTimeout(() => {
42+
if (!socket.destroyed) {
43+
const payload = zlib.deflateRawSync(Buffer.from('Hello'))
44+
// Remove trailing 00 00 ff ff if present
45+
const trimmed = payload.subarray(0, payload.length - 4)
46+
const frame = makeWsFrame({ opcode: 2, rsv1: true, payload: trimmed })
47+
socket.write(frame)
48+
}
49+
}, 50)
50+
})
51+
52+
// Override close to also destroy sockets
53+
const originalClose = server.close.bind(server)
54+
server.close = (cb) => {
55+
for (const socket of sockets) {
56+
socket.destroy()
57+
}
58+
sockets.clear()
59+
originalClose(cb)
60+
}
61+
62+
return server
63+
}
64+
65+
/**
66+
* Creates a WebSocket frame (server-to-client, unmasked)
67+
*/
68+
function makeWsFrame ({ opcode, rsv1, payload }) {
69+
const fin = 1
70+
const b0 = (fin << 7) | ((rsv1 ? 1 : 0) << 6) | (opcode & 0x0f)
71+
const len = payload.length
72+
73+
let header
74+
if (len <= 125) {
75+
header = Buffer.from([b0, len])
76+
} else if (len <= 0xffff) {
77+
header = Buffer.alloc(4)
78+
header[0] = b0
79+
header[1] = 126
80+
header.writeUInt16BE(len, 2)
81+
} else {
82+
header = Buffer.alloc(10)
83+
header[0] = b0
84+
header[1] = 127
85+
header.writeUInt32BE(0, 2)
86+
header.writeUInt32BE(len, 6)
87+
}
88+
89+
return Buffer.concat([header, payload])
90+
}
91+
92+
describe('isValidClientWindowBits', () => {
93+
test('rejects empty string', (t) => {
94+
t.assert.strictEqual(isValidClientWindowBits(''), false)
95+
})
96+
97+
test('rejects values below 8', (t) => {
98+
t.assert.strictEqual(isValidClientWindowBits('0'), false)
99+
t.assert.strictEqual(isValidClientWindowBits('1'), false)
100+
t.assert.strictEqual(isValidClientWindowBits('7'), false)
101+
})
102+
103+
test('accepts values 8-15', (t) => {
104+
for (let i = 8; i <= 15; i++) {
105+
t.assert.strictEqual(isValidClientWindowBits(String(i)), true, `${i} should be valid`)
106+
}
107+
})
108+
109+
test('rejects values above 15', (t) => {
110+
t.assert.strictEqual(isValidClientWindowBits('16'), false)
111+
t.assert.strictEqual(isValidClientWindowBits('100'), false)
112+
t.assert.strictEqual(isValidClientWindowBits('1000'), false)
113+
t.assert.strictEqual(isValidClientWindowBits('999999'), false)
114+
})
115+
116+
test('rejects non-numeric values', (t) => {
117+
t.assert.strictEqual(isValidClientWindowBits('abc'), false)
118+
t.assert.strictEqual(isValidClientWindowBits('12a'), false)
119+
t.assert.strictEqual(isValidClientWindowBits('-1'), false)
120+
t.assert.strictEqual(isValidClientWindowBits('8.5'), false)
121+
})
122+
})
123+
124+
describe('permessage-deflate server_max_window_bits', () => {
125+
test('server_max_window_bits=8 works correctly', async (t) => {
126+
const server = createMaliciousServer('8')
127+
await new Promise(resolve => server.listen(0, resolve))
128+
after(() => server.close())
129+
130+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
131+
132+
const [event] = await once(client, 'message')
133+
t.assert.ok(event.data)
134+
client.close()
135+
})
136+
137+
test('server_max_window_bits=15 works correctly', async (t) => {
138+
const server = createMaliciousServer('15')
139+
await new Promise(resolve => server.listen(0, resolve))
140+
after(() => server.close())
141+
142+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
143+
144+
const [event] = await once(client, 'message')
145+
t.assert.ok(event.data)
146+
client.close()
147+
})
148+
149+
test('server_max_window_bits=0 is rejected gracefully', async (t) => {
150+
const server = createMaliciousServer('0')
151+
await new Promise(resolve => server.listen(0, resolve))
152+
after(() => server.close())
153+
154+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
155+
156+
const [event] = await once(client, 'error')
157+
t.assert.ok(event.error instanceof Error)
158+
})
159+
160+
test('server_max_window_bits=7 is rejected gracefully', async (t) => {
161+
const server = createMaliciousServer('7')
162+
await new Promise(resolve => server.listen(0, resolve))
163+
after(() => server.close())
164+
165+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
166+
167+
const [event] = await once(client, 'error')
168+
t.assert.ok(event.error instanceof Error)
169+
})
170+
171+
test('server_max_window_bits=16 is rejected gracefully', async (t) => {
172+
const server = createMaliciousServer('16')
173+
await new Promise(resolve => server.listen(0, resolve))
174+
after(() => server.close())
175+
176+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
177+
178+
const [event] = await once(client, 'error')
179+
t.assert.ok(event.error instanceof Error)
180+
})
181+
182+
test('server_max_window_bits=1000 is rejected gracefully (PoC attack)', async (t) => {
183+
const server = createMaliciousServer('1000')
184+
await new Promise(resolve => server.listen(0, resolve))
185+
after(() => server.close())
186+
187+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
188+
189+
const [event] = await once(client, 'error')
190+
// The key assertion: we got an error event instead of crashing
191+
t.assert.ok(event.error instanceof Error, 'Should receive an Error')
192+
})
193+
194+
test('no uncaught exception with invalid windowBits', async (t) => {
195+
let uncaughtException = false
196+
197+
const handler = () => {
198+
uncaughtException = true
199+
}
200+
process.on('uncaughtException', handler)
201+
202+
const server = createMaliciousServer('1000')
203+
await new Promise(resolve => server.listen(0, resolve))
204+
after(() => {
205+
server.close()
206+
process.off('uncaughtException', handler)
207+
})
208+
209+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
210+
211+
await Promise.race([
212+
once(client, 'error'),
213+
once(client, 'close'),
214+
new Promise(resolve => setTimeout(resolve, 1000))
215+
])
216+
217+
t.assert.strictEqual(uncaughtException, false, 'No uncaught exception should occur')
218+
})
219+
220+
test('invalid windowBits closes connection without crashing process', async (t) => {
221+
const server = createMaliciousServer('999999')
222+
await new Promise(resolve => server.listen(0, resolve))
223+
after(() => server.close())
224+
225+
const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`)
226+
227+
// Wait for either error or close event
228+
const result = await Promise.race([
229+
once(client, 'error').then(([e]) => ({ type: 'error', event: e })),
230+
once(client, 'close').then(([e]) => ({ type: 'close', event: e }))
231+
])
232+
233+
// Either error or close is acceptable, as long as we didn't crash
234+
t.assert.ok(
235+
result.type === 'error' || result.type === 'close',
236+
'Connection should close gracefully'
237+
)
238+
239+
// This assertion is reached = process did not crash
240+
t.assert.ok(true, 'Process did not crash')
241+
})
242+
})

0 commit comments

Comments
 (0)