Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use zlib:safeinflate/2 in cow_ws.erl? #139

Open
benbro opened this issue Aug 16, 2024 · 3 comments
Open

Use zlib:safeinflate/2 in cow_ws.erl? #139

benbro opened this issue Aug 16, 2024 · 3 comments
Milestone

Comments

@benbro
Copy link

benbro commented Aug 16, 2024

Will it be better to replace zlib:inflate/2 with zlib:safeinflate/2
to prevent zlib-bomb as mentioned here?
https://github.com/ninenines/cowlib/blob/master/src/cow_ws.erl#L546 and few other places.

Is there a benchmark for cowboy WebScokets with and without compression? I'm trying to understand the memory, cpu and time overhead with compression and if it's stable and scalable for 1K connections.

Safari 15 had an issue with WebSocket connection when deflate is on. Maybe only when frames are fragmented. Cowboy never send fragmented packets, right?

@essen essen added this to the 2.14.0 milestone Jan 15, 2025
@essen
Copy link
Member

essen commented Jan 15, 2025

I've added benchmarks without compression in Cowboy's ws_perf_SUITE. It should be simple enough to add benchmarks with compression as well. We should also benchmark fragmented packets.

Using safeInflate sounds like the right thing to do but I will evaluate if it is possible.

@essen
Copy link
Member

essen commented Jan 21, 2025

Adding a configurable ratio limit (mirroring cowboy_decompress_h) is more difficult than expected because we (optionally) keep the compression context over multiple Websocket frames.

So this can't be done per frame, it has to be done over the entire connection. But then an attacker might just send normal highly compressed data and much later send a very large frame, circumventing the protection.

What can be done instead is having a limit similar to max_frame_size, or perhaps use max_frame_size for the decompressed frame limit. I'm leaning toward the latter at the moment. Then an uncompressed frame and a compressed frame will be subject to the same limits, instead of letting the compressed frame cheat.

If later on we need to have a separate limit we can always add it then.

@benbro
Copy link
Author

benbro commented Jan 21, 2025

Using max_frame_size for the decompressed sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants