feat: add configurable maxPayloadSize for WebSocket #4955
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,18 +39,23 @@ class ByteParser extends Writable { | |
| /** @type {import('./websocket').Handler} */ | ||
| #handler | ||
|
|
||
| /** @type {number} */ | ||
| #maxPayloadSize | ||
|
|
||
| /** | ||
| * @param {import('./websocket').Handler} handler | ||
| * @param {Map<string, string>|null} extensions | ||
| * @param {{ maxPayloadSize?: number }} [options] | ||
| */ | ||
| constructor (handler, extensions, options = {}) { | ||
| super() | ||
|
|
||
| this.#handler = handler | ||
| this.#extensions = extensions == null ? new Map() : extensions | ||
| this.#maxPayloadSize = options.maxPayloadSize ?? 0 | ||
|
|
||
| if (this.#extensions.has('permessage-deflate')) { | ||
| this.#extensions.set('permessage-deflate', new PerMessageDeflate(extensions, options)) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -66,6 +71,20 @@ class ByteParser extends Writable { | |
| this.run(callback) | ||
| } | ||
|
|
||
| #validatePayloadLength () { | ||
| if ( | ||
| this.#maxPayloadSize > 0 && | ||
| !isControlFrame(this.#info.opcode) && | ||
| !this.#info.compressed && | ||
|
Member
There was a problem hiding this comment. I don't understand why we ignore compressed messages here, but then perform the same logic in the permessage-deflate handler?
Member
Author
There was a problem hiding this comment. we are doing this kind of check in 3 places:
The 2nd could be optional, but it saves CPU cycles.
Member
There was a problem hiding this comment. but we're skipping compressed packets here, it's not much of an early return if you still have to go all the way to the decompression handler what I'm asking for is why we're skipping compressed packets here
Member
Author
There was a problem hiding this comment. We are skipping compressed packets here because it's a different logic as @tsctx has asked.
Member
There was a problem hiding this comment. The logic is not different. Assuming we send an uncompressed and compressed frame that's 129MB:
Member
There was a problem hiding this comment. I understand that, but why skip them? Couldn't an attacker just send a compressed frame that has an unlimited size, where undici will collect all fragments, attempt to inflate, then fail much later?
Member
Author
There was a problem hiding this comment. The check is done chunk by chunk on compression, as soon as it goes over the limit, the connection is interrupted.
Member
There was a problem hiding this comment. I’ve looked into it, and while we check the max length per message unit, we seem to be missing a check for the total buffered size of fragmented messages. We don't actually need a separate check for compression; we should simply trigger an early exit if the accumulated size plus the current payload exceeds the limit. Even with compressed data, we can perform this early check if the payload itself is already too large. This is already handled for uncompressed fragments, but for the compressed path, we should also pass the current buffered size to the decompression side to ensure the same validation.
Member
There was a problem hiding this comment. I've written some pseudo-code to illustrate the issue more clearly. createServer((socket) => {
// Start by fragmenting the message.
// At this stage, 64MB has been accumulated in the buffer.
socket.send(alloc(64 * 1024 * 1024), { fin: false });
// Case 1: Early termination is not triggered because compressed messages
// are currently being skipped.
// The pre-compressed size is 100MB. Since 64MB is already buffered,
// it clearly exceeds the limit and should be terminated immediately.
socket.send(alloc_post_compressed(100 * 1024 * 1024), { fin: true });
// alloc_post_compressed: The size represents the "expected final output."
// Case 2: The current accumulated size is not passed to the decompressor,
// so this segment is treated as only 100MB, bypassing the decompression limit.
// We should pass the current buffer size to the decompressor and
// validate the total size (current buffer + decompressed payload).
socket.send(alloc_pre_compressed(100 * 1024 * 1024), { fin: true });
// alloc_pre_compressed: The size represents the "actual memory footprint."
});
Member
There was a problem hiding this comment. Thank you @tsctx this is what I was trying to articulate, lol. The issue is if the compressed data is larger than the maxPayloadSize, we shouldn't bother going to READ_DATA and inflating the data which is already larger than the maxPayloadSize. The permessage-deflate handler catches it, once the decompressed data is larger than the maxPayloadSize. This only happens once we have a full frame, so an attacker could theoretically send n fragmented frames each with unlimited size that are kept in memory until we receive the entirety of the frame. From READ_DATA, should be helpful in understanding: if (this.#byteOffset < this.#info.payloadLength) {
return callback()
}
const body = this.consume(this.#info.payloadLength) |
||
| this.#fragmentsBytes + this.#info.payloadLength > this.#maxPayloadSize | ||
|
Member
There was a problem hiding this comment. This is completely wrong. #fragmentBytes is the actual amount buffered. payLoadlength is a field telling us the final frame size, so we're adding the total frame size to the currently buffered frame size...
Member
Author
There was a problem hiding this comment.
No. The idea is that this check interprets "already buffered fragments + current frame". |
||
| ) { | ||
| failWebsocketConnection(this.#handler, 1009, 'Payload size exceeds maximum allowed size') | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| /** | ||
| * Runs whenever a new chunk is received. | ||
| * Callback is called whenever there are no more chunks buffering, | ||
|
|
@@ -169,6 +188,10 @@ class ByteParser extends Writable { | |
| this.#info.masked = masked | ||
| this.#info.fin = fin | ||
| this.#info.fragmented = fragmented | ||
|
|
||
| if (this.#state === parserStates.READ_DATA && !this.#validatePayloadLength()) { | ||
|
Member
There was a problem hiding this comment. there's only one branch where state can be set to READ_DATA from here, it should be moved |
||
| return | ||
| } | ||
| } else if (this.#state === parserStates.PAYLOADLENGTH_16) { | ||
| if (this.#byteOffset < 2) { | ||
| return callback() | ||
|
|
@@ -178,6 +201,10 @@ class ByteParser extends Writable { | |
|
|
||
| this.#info.payloadLength = buffer.readUInt16BE(0) | ||
| this.#state = parserStates.READ_DATA | ||
|
|
||
| if (!this.#validatePayloadLength()) { | ||
| return | ||
| } | ||
| } else if (this.#state === parserStates.PAYLOADLENGTH_64) { | ||
| if (this.#byteOffset < 8) { | ||
| return callback() | ||
|
|
@@ -200,6 +227,10 @@ class ByteParser extends Writable { | |
|
|
||
| this.#info.payloadLength = lower | ||
| this.#state = parserStates.READ_DATA | ||
|
|
||
| if (!this.#validatePayloadLength()) { | ||
| return | ||
| } | ||
| } else if (this.#state === parserStates.READ_DATA) { | ||
| if (this.#byteOffset < this.#info.payloadLength) { | ||
| return callback() | ||
|
|
@@ -224,29 +255,34 @@ class ByteParser extends Writable { | |
|
|
||
| this.#state = parserStates.INFO | ||
| } else { | ||
| this.#extensions.get('permessage-deflate').decompress( | ||
| body, | ||
| this.#info.fin, | ||
| (error, data) => { | ||
| if (error) { | ||
| // Use 1009 (Message Too Big) for decompression size limit errors | ||
| const code = error instanceof MessageSizeExceededError ? 1009 : 1007 | ||
| failWebsocketConnection(this.#handler, code, error.message) | ||
| return | ||
| } | ||
|
|
||
| this.writeFragments(data) | ||
|
|
||
| if (!this.#info.fin) { | ||
| this.#state = parserStates.INFO | ||
| this.#loop = true | ||
| this.run(callback) | ||
| return | ||
| } | ||
|
|
||
| websocketMessageReceived(this.#handler, this.#info.binaryType, this.consumeFragments()) | ||
|
|
||
| this.#loop = true | ||
| this.#state = parserStates.INFO | ||
| this.run(callback) | ||
| }, | ||
| this.#info.payloadLength | ||
| ) | ||
|
|
||
| this.#loop = false | ||
| break | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.