1713748 - Partition WebSocket connections

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect, P2)

Description

[Anne (:annevk)]

Step 2 of https://datatracker.ietf.org/doc/html/rfc6455#section-4.1 introduces a cache of sorts in the process of obtaining a WebSocket connection. https://github.com/whatwg/fetch/issues/1243#issuecomment-848992822 explains how this can be used to link two independent tabs.

The most straightforward solution would be to use the top-level site to partition that cache. While this theoretically makes DOS slightly easier, in practice it shouldn't matter a whole lot as it would also require the user to have many tabs open to independent sites that happen to embed the attacker.

Comment 1

[Paul Zühlcke [:pbz]]

I've tested the algorithm described in https://github.com/whatwg/fetch/issues/1243#issuecomment-848992822 with a PoC and can confirm that it works in Firefox. The setup is a bit complex since you need a unique IP for each websocket endpoint. I'll share the code if I can come up with an easy way to set this up.

Comment 2

[Paul Zühlcke [:pbz]]

I have a WIP patch that updates nsWSAdmissionManager to double key the pending connection queue by address (host or IP) and origin attributes of the caller. To do this I'm adding an additional member, the origin suffix containing the partitionKey, to nsOpenConn here: https://searchfox.org/mozilla-central/rev/183b0cfc30f2d40f818a08cbea960f6119e2d196/netwerk/protocol/websocket/WebSocketChannel.cpp#456

Dragana, do you think this approach is reasonable? I agree with Anne here, that DoS isn't that problematic since we still have a limit per top level site and endpoint IP. However, we are deviating from the standard a bit.

Comment 3

[Paul Zühlcke [:pbz]]

Attachment: Bug 1713748 - Partition WebSocket nsWSAdmissionManager queue. r=timhuang!,#necko-reviewers

Comment 4

[Paul Zühlcke [:pbz]]

Attachment: Bug 1713748 - Added test for websocket connection queue. r=timhuang!,#necko-reviewers

Depends on D119534

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22e2e65e5b2d
Partition WebSocket nsWSAdmissionManager queue. r=timhuang,nhnt11
https://hg.mozilla.org/integration/autoland/rev/3de943e6b35d
Added test for websocket connection queue. r=timhuang,nhnt11

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/22e2e65e5b2d
https://hg.mozilla.org/mozilla-central/rev/3de943e6b35d

Comment 7

[Matthew Finkel [:sysrqb]]

Sorry I'm coming late. Can this patch be uplifted to 91 so it's included in the next ESR, please? In addition, I believe this cleanly captures FPI-based OA if isPartitioned is extended to consider || privacy_firstparty_isolate, as well: https://searchfox.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#490. Can you please confirm this is true and amend the patch for it?

Comment 8

[Paul Zühlcke [:pbz]]

(In reply to Matthew Finkel [:sysrqb] from comment #7)

Sorry I'm coming late. Can this patch be uplifted to 91 so it's included in the next ESR, please? In addition, I believe this cleanly captures FPI-based OA if isPartitioned is extended to consider || privacy_firstparty_isolate, as well: https://searchfox.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#490. Can you please confirm this is true and amend the patch for it?

Oh I see, the Tor Browser does not set the network partitioning pref. I've filed Bug 1721858 to make the change.

Nihanth, how comfortable are we with uplifting this patch and the change for Bug 1721858 into 91? I think this should be in Nightly for at least a couple of days.

Comment 9

[Paul Zühlcke [:pbz]]

Ugh, collision, see comment 8.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

I don't have any specific concerns with uplifting this into 91, but the patch is non-trivial so as you said this should ideally have a chance to bake in Nightly.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Sorry I need to clarify my previous comment: it's not just that it should bake in Nightly, but also in Beta where we have more users and so a larger surface for finding issues. If we do uplift this, it should probably be asap so we have a week of early beta bake time.

Comment 12

[Paul Zühlcke [:pbz]]

I'd prefer if we uplift to ESR at a later point. Late beta uplift seems risky. Getting Bug 1721858 into 92 still sounds good though!

Comment 13

[Dragana Damjanovic [:dragana]]

This changes our behavior towards servers/middleboxes and we cannot control them. The change is low risk, but the internet is always full of surprises. So I suggest uplifting this to ESR at some later point.

Comment 14

[Matthew Finkel [:sysrqb]]

I appreciate your consideration here, and I understand the hesitancy. If this can be uplifted in a few months, then I'll appreciate it. I'll carry the patches downstream in the mean time.

Comment 15

[Simon Mainey]

... If this can be uplifted in a few months, then I'll appreciate it ...

it's two cycles already :)

(In reply to Nihanth Subramanya [:nhnt11] from comment #10)

I don't have any specific concerns with uplifting this into 91, but the patch is non-trivial so as you said this should ideally have a chance to bake in Nightly.

Is ESR91.2 (94 cycle) too early to backport this bug and Bug 1721858 ? I haven't seen any regressions