1299766 - [e10s] Content policy call for Web Sockets is missing context

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Wladimir Palant]

With bug 1271198, the content policy call for a Web Sockets connection is happening in the parent process. This means however that it is missing context information - no DOM nodes can be accessed at this point any more. Adblock Plus relies on that context information to work correctly however.

It seems that CSP also relies on information only available in the child process, so the patch in bug 1271198 introduced a hack calling its ShouldLoad method explicitly (https://hg.mozilla.org/mozilla-central/file/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/dom/base/WebSocket.cpp#l1575). I wonder why this isn't just a regular content policy call, this would allow other content policy implementation to work correctly as well.

Comment 1

[Andrew Overholt [:overholt]]

Christoph, needinfoing so this is on your radar.

Comment 2

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

Attachment: bug_1299766_bring_back_contentpolicy_check_for_websockets.patch

Jason, I suppose Wladimir makes a good point. I think we should bring back the full blown content policy check instead of just checking CSPs. We should also uplift this change so we can provide a context for addon implemented nsIContentPolicies.

Potentially we could also remove the content policy check from within nsContentSecurityManager which is consulted by AsyncOpen2() [2], but to remain on the safe side I would rather keep it. Especially because we are going to uplift that bug.


[1] https://hg.mozilla.org/mozilla-central/rev/9ad3bd17f3e9#l1.30
[2] https://hg.mozilla.org/mozilla-central/rev/9ad3bd17f3e9#l3.47

Comment 3

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

Comment on attachment 8788154 [details] [diff] [review]
bug_1299766_bring_back_contentpolicy_check_for_websockets.patch

Approval Request Comment
Addons implementing nsIContentPolicy might not be able to block websockets correctly because of a missing 'context' in e10s.

[Feature/regressing bug #]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1271198

[User impact if declined]:
see above

[Describe test coverage new/current, TreeHerder]:
we have mochitests e.g. test_upgrade_insecure.html to make sure we are not regressing websockets governed by CSP (which also implements nsIContentPolicy within Gecko).

[Risks and why]:
low, because we are bringing back a full blown content policy check instead of a simple CSP check. The replacment of those two checks happenend within Bug 1271198.
 
[String/UUID change made/needed]:
no

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Christoph, does this patch need to land on Nightly first and then uplifted to Aurora?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd86c5c1b2e1
Bring back contentpolicy check for websockets in child to avoid missing context for addon implemented content policies within parent. r=jduell

Comment 6

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

(In reply to Ritu Kothari (:ritu) from comment #4)
> Hi Christoph, does this patch need to land on Nightly first and then
> uplifted to Aurora?

Yes, it needs to land on central first. Next time I will be more explicit. But it seems ryan already pushed it to inbound (Comment 5). Once in Nightly we should uplift. thanks!

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd86c5c1b2e1

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788154 [details] [diff] [review]
bug_1299766_bring_back_contentpolicy_check_for_websockets.patch

Fix has stabilized on Nightly for a few days, Aurora50+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76e76e26bfe7