option to disable permessage-deflate

[KhafraDev]

No description provided.

[mcollina]

This is a major change to the security model that is incompatible with Node.js threat model.

[ronag]

What does this fix? Seems to just make things worse?

[KhafraDev]

It fixes the vulnerability. A maxPayloadSize does not:

The primary use case is to block frames that exceed a certain size. As a side effect it prevents some zip bombs. It blocks completely legitimate frames too. I assume this is why the original "fix" was to check the expansion size instead of a size check.

[ronag]

I'm not sure I understand the harm of maxPayloadSize? I'm not sure frame > 128m makes any sense in the real world.

[ronag]

I'm fine with the disable option, I just don't see the point of removing the existing ones, especially since there seems to be some disagreement.

[KhafraDev]

https://bugzilla.mozilla.org/show_bug.cgi?id=711205 16 MB frames were too small for chat applications 14 years old for Firefox.

There are other examples I could find of people streaming file uploads/downloads via websockets. It's hard to gauge if this will cause issues with node users because there wasn't previously a limit.

If the vulnerability is that zip bombs from a malicious server can be inflated by the client and cause a DOS, the fix isn't to cap frame sizes (even when permessage-deflate isn't negotiated by the server...) but to allow the users to disable permessage-deflate. Realistically, if you want broad security, there's absolutely nothing wrong with ws.

[ronag]

https://bugzilla.mozilla.org/show_bug.cgi?id=711205 16 MB frames were too small for chat applications 14 years old for Firefox.

Sure... but the default here is 128M

There are other examples I could find of people streaming file uploads/downloads via websockets. It's hard to gauge if this will cause issues with node users because there wasn't previously a limit.

Sounds like a bad use case.

If the vulnerability is that zip bombs from a malicious server can be inflated by the client and cause a DOS, the fix isn't to cap frame sizes (even when permessage-deflate isn't negotiated by the server...) but to allow the users to disable permessage-deflate. Realistically, if you want broad security, there's absolutely nothing wrong with ws.

Sure... but I think we need to pick our fights. This PR as is conflicts with the node security policy. Is this really worth the fight and bad will?

[KhafraDev]

It really is unfortunate that this issue is seen as being in bad will when all I want to do is ensure that the WebSocket client can be maintained in 5 years. I have nothing bad to say about the security team nor am I annoyed at any of them or at them for accepting the initial report, the only person I am annoyed at is the person who spent no due diligence in reporting this after entering a prompt into Claude or ChatGPT.

Please understand from my viewpoint:

• I have written >99% of the websocket client code
• one day I see a commit on main that adds a 4 MB limit to frames, obviously it breaks people (I understand that this is in line with the security policy and security releases)
• a PR is made by Matteo to raise the limit. IIRC I fight to remove the limit entirely but unbreaking these people is more important short-term, I approve it.

Obviously I don't expect to be roped into every vulnerability reported, but I had no say in it but now it's my responsibility to maintain it (despite finding the solution... not great[1]), lest I give up maintaining WebSocket entirely, which I would rather not do. It sucks.

This change, from my understanding of the security policy, does fit the policy. The policy for DOS vulnerabilities states:

For a behavior to be considered a DoS vulnerability, the PoC must meet the following criteria:
[...]
The API doesn't have a warning against its usage in a production environment.

I disagree that this is a vulnerability at all, but it's not important to this issue. Documenting that once the websocket handshake resolves, the frames received from a server are considered trusted shouldn't be controversial nor go against the policy (?).

If an attacker demonstrated that they could send a payload a couple hundred kilobytes large that inflated to just under the 128 MB limit, this would be a DOS under the policy: "the attacker expends significantly fewer resources than what's required by the server to process the attack," but not under the statement "[t]he behavior cannot be reasonably mitigated". Therefore, at least to me, the DOS still likely exists, isn't a vulnerability, but the only way to prevent future issues to users is

• a warning
• an option to completely disable permessage-deflate support on a client

One last thing, I have referred to this comment from ljharb multiple times: "How can we add this when it doesn’t comply with the browser spec for fetch?", which I still find applies here. Users can still use ws, it's an amazing library that has all of these very same limits as configurable options. Users choose to use the global/undici WebSocket because the code that work[ed/s] here works everywhere else. If we remove that guarantee, why bother with the undici WebSocket at all, when we could bundle ws' into node core and guarantee it allows users to mitigate every silly ai-generated security vulnerability?