Allow configuring maxDecompressedMessageSize

[simong]

In order to address the potential per-message deflate decompress bomb, the message size was limited to 4MB in 2ee00cb with a configuration option.

That configuration option was then removed in #4891 .

I understand the reasoning behind removing the configuration option, but it effectively limits any decompressed message to 4MB without providing the consumer the ability to set their own limit. AFAICT, the only way forward is to use a different library such as ws.

The removal PR mentions the ability to provide configuration via a dispatcher. Creating this issue to track progress on that enhancement as we would like to continue using undici bundled in node, but need the ability process messages larger than 4MB.

[KhafraDev]

What's the issue with using ws?

FWIW I don't consider GHSA-vrm6-8vpv-qv8q a real vulnerability either.

Node.js trusts that applications make connections/requests which will avoid payload sizes that will result in a Denial of Service. 

(https://github.com/nodejs/node/blob/HEAD/SECURITY.md)

[headlessme]

This fix lead to some behaviour changes in a production system for us, with some genuine large messages starting to fail unexpectedly after a node update. Not ideal that this happened without a major version bump or without any way to configure the behaviour. Porting to a whole new websocket library is not really a short-term workable solution, particularly as this is used as part of an internal dependency with many consumers (both node based and browser) that would all need to validate a change like that. Pinning to an older node version also wouldn't really work for us as we'd get out of compliance with our vulnerability management policies pretty quickly.

[KhafraDev]

@mcollina Should we reconsider this? In ws, as far as I can tell, it's disabled by default, and has an option to enable (permessage-deflate is enabled by default for clients and disabled by default for servers). faye-websocket doesn't implement a limit, configurable or otherwise, at all (or at least, https://www.npmjs.com/package/permessage-deflate does not, which they mention to enable permessage-deflate support in their README).

Porting to a whole new websocket library is not really a short-term workable solution, particularly as this is used as part of an internal dependency with many consumers (both node based and browser) that would all need to validate a change like that.

Making it configurable would only work if we had a global setting, but node/undici would still fail by default which seems silly, since Bun, Deno, ws, and every other browser work by default.

As mentioned above, it does not seem like it is part of the node security model and, if you are connecting to untrusted servers, permessage-deflate can be disabled via modifying the Sec-WebSocket-Extensions header.

[mcollina]

What's the issue with using ws?

FWIW I don't consider GHSA-vrm6-8vpv-qv8q a real vulnerability either.

Node.js trusts that applications make connections/requests which will avoid payload sizes that will result in a Denial of Service. 

(https://github.com/nodejs/node/blob/HEAD/SECURITY.md)

This is on the response side unfortunately, connecting to an untrusted server is part of the security model.

[mcollina]

@KhafraDev we should make this configurable in some way. What would be a way that would be spec-friendly?

cc @RafaelGSS

[mcollina]

An alternative is to disable per-message deflate (again via a config).

[headlessme]

Disabling per-message deflate is also non-ideal for our particular use case. Our payloads are very compressible so we pay a big bandwidth cost if we have to turn it off. A way to configure the max payload size would still be a more ideal solution.

[KhafraDev]

What would be a way that would be spec-friendly?

Document the limitation. If it was a vulnerability, I would expect other runtimes or libraries to implement similar measures by default.

I don't see a way forward that would satisfy everyone. We cannot follow the spec and implement a limit. People can disable permessage-deflate for themselves already.

This is on the response side unfortunately, connecting to an untrusted server is part of the security model.

The wording is really confusing me.

• Node does not trust "[t]he data received from the remote end of outbound network connections ... except with respect to payload length"
• "Node.js trusts that applications make connections/requests which will avoid payload sizes that will result in a Denial of Service."

In other words, if the data passing through Node.js to/from the application can trigger actions other than those documented for the APIs, there is likely a security vulnerability. Examples of unwanted actions are polluting globals, causing an unrecoverable crash, or any other unexpected side effects that can lead to a loss of confidentiality, integrity, or availability.

If we document this, is that good enough then? I unfortunately don't see a way we can make everyone happy. If it is opt-in, then (if you consider this a vulnerability), you are vulnerable by default. If it is opt-out, code that works in every other environment and popular library does not work in node by default.

[mcollina]

@KhafraDev this is the relevant piece: https://github.com/nodejs/node/blob/main/SECURITY.md#denial-of-service-dos-vulnerabilities.

Basically, it has a roughly 10x expansion rate and is remotely controlled. A user cannot do anything to protect against this attack either.

[KhafraDev]

You can disable permessage-deflate via a dispatcher