WebSocket Compression

[zlatanov]

This is the new improved implementation of per-message deflate compression which plugs into the existing code of ManagedWebSocket without requiring too much to change.

Microbenchmarks show no performance regression for non-compressed messages. Memory allocations profile shows the same results.

A note to the owners of System.IO.Compression - as discussed with @CarnaViire, I've moved two of the files (ZLibNative.ZStream.cs and ZLibNative.cs) to Common so I can use the Interop.zlib.cs. /cc @carlossanlop

Closes #31088

/cc @CarnaViire, @karelz, @scalablecory, @davidfowl, @stephentoub

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is the new improved implementation of per-message deflate compression which plugs into the existing code of ManagedWebSocket without requiring too much to change.

Microbenchmarks show no performance regression for non-compressed messages. Memory allocations profile shows the same results.

A note to the owners of System.IO.Compression - as discussed with @CarnaViire, I've moved two of the files (ZLibNative.ZStream.cs and ZLibNative.cs) to Common so I can use the Interop.zlib.cs.

Closes #31088

/cc @CarnaViire, @karelz, @scalablecory, @davidfowl, @stephentoub

Author:	zlatanov
Assignees:	-
Labels:	area-System.Net, new-api-needs-documentation
Milestone:	-

[blowdart]

Given crime and breach and the concerns around compressing HTTP has any research been done on whether this presents a security problem? I see the RFC acknowledges it,

8. Security Considerations

There is a known exploit when history-based compression is combined
with a secure transport [CRIME]. Implementors should pay attention
to this point when integrating this extension with other extensions
or protocols.

No real useful advice of course, but seems to point "don't enable this over TLS"

Is this on or off by default? Does that depend on the protocol? (ws versus wss)

Tagging @GrabYourPitchforks

[zlatanov]

Is this on or off by default? Does that depend on the protocol? (ws versus wss)

This is off in the client by default. Servers such as AspNetCore can choose otherwise. The per-message-deflate protocol supports opting out per message, but the current API doesn't support it.

Sorry about my ignorance, but I fail to see how using BREACH or CRIME one can exploit the websocket. You can have compression disabled for the https website, so headers aren't affected, but still can have per-message-deflate enabled for the websocket itself. @blowdart what am I missing?

[blowdart]

Compression using the reflection of attacker-controlled data has previously enabled the compromise of the encryption keys. it's reasonable to think websockets will carry attacker-controlled data. This is why Quic/HTTP/3 doesn't compress much except some headers.

[benaadams]

it's reasonable to think websockets will carry attacker-controlled data.

However its over an established connection that's not exchanging secrets, unlike standard HTTP which is passing headers and tokens per request? So would need to carry secrets and the attacker-controlled data in the message for the attack to have anything to extract?

CSWSH aside, which is more about the handshake and is unaffected (not made worse or better) by per message compression.

[zlatanov]

Compression using the reflection of attacker-controlled data has previously enabled the compromise of the encryption keys.

Can you provide more information about this, please? All I can find are references to attacks that use the compression oracle to guess secrets in the payload. How can this be used to leak the private key used to encrypt the payload?

[blowdart]

@benaadams it's enough of a risk to be called out in the rfc, with no mitigations given, which makes it rather dubious. IMO This is one of those cases where you'd need to prove it's safe to proceed.

[davidfowl]

@benaadams it's enough of a risk to be called out in the rfc, with no mitigations given, which makes it rather dubious. IMO This is one of those cases where you'd need to prove it's safe to proceed.

@blowdart Based on what I've seen so far it means you can't send auth tokens (or any secret token) over the web socket channel while compression is on. I don't know what sort of data counts as secret though. It might mean signalr connection ids for example (though those were never safe to send anyways)

[blowdart]

Leave and this is the problem. No one can tell really. Like http2 compression we end up only recommending it for static content only, and why we don't turn it on by default.

[benaadams]

As its over a WebSocket it not as simple as getting the browser to initiate an HTTPS request; you also need to trigger JavaScript to send the message on the WebSocket and need the response to always send the same reply + message + secret; so the length change can be checked.

However if you are already running JavaScript in the users context to send the message; you can just read the response, so haven't you already hijacked the user? What more is trying to be achieved at that point?

Though I do understand the trepidation.

[benaadams]

Saying that... I suppose if you could trigger a WebSocket response; like being on the other end of chat and the same secrets were also always sent with the chat message response (to the other user)... but that would be a weird thing to do? (always including the same secrets as part of the message; as the compression is message scoped)

HTTP is different due to being stateless, so you always send secrets so can use multiple responses to decode, the WebSocket is stateful so you shouldn't?

e.g. don't send a XSRF/AntiForgeryToken token or Cookie or Auth header with every SignalR send as far as I'm aware; because the send is trusted as it is a stateful connection?

[benaadams]

Note: that was not security advice or recommendations; or in anyway suggesting it was safe, just some questions

[benaadams]

It might mean signalr connection ids for example

Are these being sent in the same message and repeatedly (so length changes can be observed) with user controlled data (to change the length); as the compression scope is the message?

[scalablecory]

It does seem unlikely that your typical browser app would have issues here. If data is in the browser in plaintext, you've already got a problem.

Here's a somewhat sophisticated scenario I can think of:

1. Attacker compromises some popular browser extension to get it to force generated data at whatever frequency.
2. Attacker sniffs packets at Server side, and is able to decrypt every connected Client's secrets who is using this extension.

This one could easily just be "browser extension sends the secret data directly to attackers". But, that may be a lot easier to detect depending on how many clients have it.

I could see a modified version of this causing ServerA->ServerB communication via WebSocket to be compromised based on a client sending messages to ServerA --- you may not have secrets client side, but it seems a lot more likely between servers.

[CarnaViire]

@carlossanlop @adamsitnik @jozkee could you please review System.IO.Compression related part?

[CarnaViire]

LGTM, thank you so much @zlatanov! Now all that's left is System.IO.Compression review.

[adamsitnik]

The PR looks great, it's a very impressive contribution @zlatanov! 👍

The System.IO.Compression part looks good!

My only worry is the custom object pool implementation. What alternatives have you considered?

[zlatanov]

@adamsitnik the deflate pool is removed. Let me know if you need anything else.

[adamsitnik]

LGTM, once again big thanks for this awesome contribution @zlatanov !

[danmoseley]

Thank you @zlatanov - if you are interested in another project, we would be happy to help find one!

[zlatanov]

if you are interested in another project, we would be happy to help find one!

@danmoseley that would be lovely :) Do you have something in mind?

[danmoseley]

@zlatanov what are your interests? there are many issues tagged up for grabs:
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aup-for-grabs

specifically there are a number of API's that have been approved to add as features, but do not have a volunteer to implement them. these should be "shovel ready" work:
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aup-for-grabs+label%3Aapi-approved

[davidfowl]

https://up-for-grabs.net/#/filters?names=1%2C5%2C58

[zlatanov]

@danmoseley I don't have any particular interests. Something challenging would be my preference, but I would also take something easier if it has higher priority. Looking through the approved apis, a few stand out to me:

1. Proposal: Vector.Sum(Vector) API for horizontal add (Proposal: Vector.Sum(Vector<T>) API for horizontal add #35626)
2. Expose general purpose Crc32 APIs (Expose general purpose Crc32 APIs #2036)
3. General purpose non-cryptographic hashing API for .NET (General purpose non-cryptographic hashing API for .NET #24328)
4. Add HTTP/3 APIs (Add HTTP/3 APIs #1293) (seems very easy)
5. SetCreationTime, SetLastAccessTime, SetLastWriteTime Should not open a new stream to obtain a SafeFileHandle (SetCreationTime, SetLastAccessTime, SetLastWriteTime Should not open a new stream to obtain a SafeFileHandle #20234)

I went through other issues, but it's hard to know if someone is working on it or not, because there is discussion, but no assignees.

If you have other issues (bugs or features) with higher priority than the aforementioned features, pick one for me and let me know. Otherwise pick any from the list and assign me.

[danmoseley]

@zlatanov what I suggest is that you post on one or more of those issues to ask whether you can take it on or ask about it. You can find the right people to @-ping using this list and check it's still up for grabs, what it involves etc.

I think the HTTP/3 one may not be super interesting if it's just exposing an enum over the functionality the team is now working on, but @karelz will know. The others look good though.

There's still a couple months or so to get features into .NET 6, so whatever you do would likely be in this release.

[danmoseley]

#35626 would be a good fit if you want to work (with help) on a hardware accelerated implementation.

[danmoseley]

@zlatanov would it be possible for you to ping me by email? My address is danmose@myemployer.