Last Comment Bug 692067 - WebSockets don't trigger content policies
: WebSockets don't trigger content policies
Status: RESOLVED FIXED
[parity-Chrome][sg:low]
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
http://html5demos.com/web-socket
Depends on: 667490
Blocks: abp 698411
  Show dependency treegraph
 
Reported: 2011-10-05 03:57 PDT by Wladimir Palant
Modified: 2012-02-27 13:26 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.26 KB, patch)
2011-10-31 05:57 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bugs: review+
Details | Diff | Splinter Review
Patch (7.87 KB, patch)
2011-11-02 08:21 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Now with qref (6.35 KB, patch)
2011-11-02 08:27 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bugs: review+
dveditz: review+
Details | Diff | Splinter Review

Description Wladimir Palant 2011-10-05 03:57:54 PDT
It seems that the current WebSockets implementation creates a connection without triggering content policies. I would have expected a content policies call in  nsWebSocket::Init(), probably with type nsIContentPolicy.TYPE_OTHER and the requesting document as context (similar to XMLHttpRequest). It is generally strange that aPrincipal parameter in this function is unused, shouldn't there be some security checks beyond HTTPS-to-HTTP check? Steps to reproduce:

* Install Adblock Plus from (https://addons.mozilla.org/addon/adblock-plus/), restart browser.
* Go to http://html5demos.com/web-socket
* Press Ctrl-Shift-V to open the list of blockable items that Adblock Plus shows.

Expected results:

ws://node.remysharp.com:8001 shows up in the list of requests made by the page.

Actual results (Firefox 7.0.1 and current 10.0a1 nightly):

No such request, web socket requests go completely unnoticed by extensions like Adblock Plus.

Note that Chrome correctly reports WebSocket requests via its webRequest API (http://code.google.com/chrome/extensions/trunk/experimental.webRequest.html).
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-31 05:57:58 PDT
Created attachment 570671 [details] [diff] [review]
Patch
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-10-31 06:06:03 PDT
Comment on attachment 570671 [details] [diff] [review]
Patch


>+  // Check content policy.
>+  PRInt16 shouldLoad = nsIContentPolicy::ACCEPT;
>+  rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_XMLHTTPREQUEST,
TYPE_DATAREQUEST

>+                                 mURI,
>+                                 mPrincipal,
>+                                 originDoc,
>+                                 EmptyCString(),
>+                                 nsnull,
>+                                 &shouldLoad,
>+                                 nsContentUtils::GetContentPolicy(),
>+                                 nsContentUtils::GetSecurityManager());
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_CP_REJECTED(shouldLoad)) {
>+    // Disallowed by content policy.
>+    return NS_ERROR_CONTENT_BLOCKED;
Ok, this is consistent with XHR.

Could you file a followup to change the error type. It should be something defined
in some spec. Maybe SECURITY_ERR or NETWORK_ERR.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-31 06:07:16 PDT
Sure.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-31 06:31:04 PDT
https://hg.mozilla.org/mozilla-central/rev/d40e649ff250
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-31 08:42:08 PDT
https://hg.mozilla.org/mozilla-central/rev/4271b14e0c0b
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-31 09:53:33 PDT
This failed on tinderbox because nsNoDataProtocolContentPolicy is vetoing WebSockets loads since they come from a protocol that has URI_DOES_NOT_RETURN_DATA set.

I'm not really sure which side is broken here, the protocol or the content policy.  Boris?
Comment 7 Boris Zbarsky [:bz] 2011-10-31 11:18:13 PDT
The content policy is doing this very much on purpose.  It only exists to not allow loads of protocols that would cause external apps to run, unknown protocol alerts to pop up, and whatnot and whatnot, except in a very restricted set of contexts.

On the assumption that the protocol actually has DOES_NOT_RETURN_DATA for a reason, seems like the right fix is to use a new nsIContentPolicy type for this check and whitelist it in the content policy...
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-02 08:21:04 PDT
Created attachment 571335 [details] [diff] [review]
Patch

More like this, then.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-02 08:27:01 PDT
Created attachment 571336 [details] [diff] [review]
Now with qref

Forgot to qref
Comment 10 Boris Zbarsky [:bz] 2011-11-02 10:18:25 PDT
Comment on attachment 571335 [details] [diff] [review]
Patch

Nix the trailing comma in the old comment?
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-04 07:33:56 PDT
dveditz, can you review this before the next aurora merge?
Comment 12 Daniel Veditz [:dveditz] 2011-12-05 10:24:21 PST
If web sockets weren't triggering content policies then they were also not controllable by CSP.
Comment 13 Daniel Veditz [:dveditz] 2011-12-05 18:58:40 PST
Comment on attachment 571336 [details] [diff] [review]
Now with qref

Review of attachment 571336 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz with those changes, especially the contentSecurityPolicy.js one

::: content/base/public/nsIContentPolicy.idl
@@ +145,1 @@
>    /* Please update nsContentBlocker when adding new content types. */

Websockets will bypass CSP, unlike EventSource which uses TYPE_DATAREQUEST. you'll need to add a line like the one for XHR at https://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#104

   csp._MAPPINGS[cp.TYPE_WEBSOCKET]    = cspr_sd.XHR_SRC;

The "XHR_SRC" is on purpose, we're aliasing those together per the CSP spec (needs to be renamed "connect-src", but that's bug 666056)

::: extensions/permissions/nsContentBlocker.cpp
@@ +68,5 @@
>                                      "objectsubrequest",
>                                      "dtd",
>                                      "font",
> +                                    "media",
> +                                    "websocket"};

I guess "websocket" should be added to NS_CP_ContentTypeName(), too, but I can't find any callers so maybe we can just kill the whole function as DEADC0DE

https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentPolicyUtils.h#124
Comment 14 Daniel Veditz [:dveditz] 2011-12-05 19:04:19 PST
+  /**
+   * Indicated a WebSocket load.
+   */

Also, s/Indicated/Indicates/
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-08 05:06:10 PST
https://hg.mozilla.org/mozilla-central/rev/0ca37155a577

Note You need to log in before you can comment on or make changes to this bug.