Last Comment Bug 616733 - disable and/or remove WebSockets for gecko 2.0 due to security problems
: disable and/or remove WebSockets for gecko 2.0 due to security problems
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: Trunk
: All All
: -- blocker with 1 vote (vote)
: mozilla2.0b8
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 602028
Blocks: 537787
  Show dependency treegraph
 
Reported: 2010-12-04 13:50 PST by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2011-03-22 01:09 PDT (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta8+
2.0b3+


Attachments
Add network.websocket.override-security-block preference (6.35 KB, patch)
2010-12-07 08:25 PST, Patrick McManus [:mcmanus]
jonas: review-
Details | Diff | Splinter Review
Add network.websocket.override-security-block preference v2 (7.52 KB, patch)
2010-12-07 17:30 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
Also turns off "WebSocket" in window (8.86 KB, patch)
2010-12-07 18:29 PST, Jason Duell [:jduell] (needinfo me)
peterv: review+
Details | Diff | Splinter Review
: Also turns off "WebSocket" in window v4 (8.62 KB, patch)
2010-12-08 10:37 PST, Patrick McManus [:mcmanus]
mcmanus: review+
Details | Diff | Splinter Review
fix content/base/test/test_ws_basic_tests.html (4.99 KB, patch)
2010-12-08 15:26 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2010-12-04 13:50:59 PST
Google did experiments with draft 76 that show serious security problems with that protocol:

http://www.adambarth.com/experimental/websocket.pdf 
http://www.ietf.org/mail-archive/web/hybi/current/msg04744.html
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2010-12-04 13:55:39 PST
See http://www.ietf.org/mail-archive/web/hybi/current/msg04782.html

> > At Opera we do not plan to implement the new framing
> until the spec uses a > CONNECT-based handshake. We
> support Adam and Eric's handshake proposal.

> Speaking personally, I am also in favor of Adam and
> Eric's proposal for a CONNECT-based handshake, as
> well as the associated payload masking. I expect
> many of my Apple colleagues would agree.

> We would be hesitant to ship protocol updates that
> do not fix the handshake. Given the security issues
> identified by the paper from Adam and company, we
> would even consider disabling WebSocket entirely
> in future releases until there is a more robust
> handshake.

> (However, I make no specific promises about
> future Safari releases.)

Regards,
Maciej
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2010-12-06 14:45:52 PST
IMO, if we are going to do this then it must be done for Firefox B8 / Fennec B3 since the previous betas were already supposed to have API frozen. But, I am busy helping with Sync B8 blockers so I am unlikely to be free to do this myself.

So, first, we need a decision as to whether we disable it and if so then we need to find somebody right away to actually disable it. In private email, it seems like blizzard, jst and I agree that it should be disabled. Please speak up if you have a plan that doesn't involve disabling WebSockets.
Comment 3 Christopher Blizzard (:blizzard) 2010-12-06 15:50:03 PST
I talked with Brian, Johnny, Pat, Jonas and others about this and we have consensus.  We're going to disable this with a pref due to the reported security problems.

We'd like to keep the code in so that when we're ready to update to a new version of the protocol, we can do so easily.

Pat you said on IRC that you would be willing to do that, is that still the case?
Comment 4 Patrick McManus [:mcmanus] 2010-12-06 16:07:32 PST
(In reply to comment #3)

> Pat you said on IRC that you would be willing to do that, is that still the
> case?

yep, will do tomorrow.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2010-12-06 16:10:13 PST
We also decided that the pref that controls whether WebSockets is enabled should be different than the one that is currently controlling WebSockets (if any) so that we can force it off for all users in B8, including users that might have explicitly enabled any old pref.
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-12-06 16:39:39 PST
Can we still make it into b8?  Agree that we should remove sooner rather than later.
Comment 7 Patrick McManus [:mcmanus] 2010-12-07 08:25:10 PST
Created attachment 495839 [details] [diff] [review]
Add network.websocket.override-security-block preference

Add network.websocket.override-security-block preference, defaulting to false, which must be set to true along with the existing network.websocket.enabled to initialize a websocket connection. 

This pref can be removed when we have a suitable protocol implementation.

also updated mochitest test_websocket.html and test_websocket_hello.html to set the override preference. Will submit to try to see if any tests were missed.
Comment 8 Johnathan Nightingale [:johnath] 2010-12-07 09:06:45 PST
Speculatively marking this blocking beta8+. If it becomes the last bug and we encounter unexpected issues, we can re-assess, but if you get it in nownownow then we're fine.
Comment 9 Daniel Buchner [:dbuc] 2010-12-07 09:14:34 PST
If you do opt to make this preference blocked, is it possible to treat it as we do geolocation and offer the user the choice to enable Web Sockets for a site they trust?
Comment 10 Christopher Blizzard (:blizzard) 2010-12-07 10:59:43 PST
Daniel, I don't think that's a good idea because it's security facing.  Has too much of the "whatever" button syndrome.
Comment 11 Daniel Buchner [:dbuc] 2010-12-07 11:02:26 PST
Fair enough, just thought I would try :)
Comment 12 Nochum Sossonko [:Natch] 2010-12-07 11:07:47 PST
Comment on attachment 495839 [details] [diff] [review]
Add network.websocket.override-security-block preference

This is gonna suck since the feature detection will be totally thrown off.

E.g.:

if (WebSocket in window)
  ...
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2010-12-07 14:12:24 PST
Comment on attachment 495839 [details] [diff] [review]
Add network.websocket.override-security-block preference

- In content/base/test/test_websocket.html:

 function testWebSocket ()
 {
+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+  var prefService =
+      Components.classes["@mozilla.org/preferences-service;1"]
+      .getService(Components.interfaces.nsIPrefService);
+  var domBranch = prefService.getBranch("network.websocket.");
+  domBranch.setBoolPref("override-security-block", true);
   doTest(first_test);
 }

This code also needs to unset the pref once the tests have run, otherwise these prefs remain set for all our following tests. Same goes for the other test that does the same thing.

r=jst with that fixed.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2010-12-07 14:21:19 PST
Comment on attachment 495839 [details] [diff] [review]
Add network.websocket.override-security-block preference

jst: sorry to override your review.

However we need to not just make the object not work when websockets are disabled through pref, but also make js code like

if (WebSocket) {
  doSpiffyThingsWithWebSockets();
}
else {
  doFallbackUsingXHRorSomething();
}

use the fallback path.
Comment 15 Patrick McManus [:mcmanus] 2010-12-07 17:30:20 PST
Created attachment 496038 [details] [diff] [review]
Add network.websocket.override-security-block preference v2

address jst's comment 13 regarding prefs in the test cases.

jonas's comment 14, regarding "if (Websocket)" is not yet addressed, but I'm updating the patch now in case someone with insight into how to do that can build on it.
Comment 16 Jason Duell [:jduell] (needinfo me) 2010-12-07 18:29:27 PST
Created attachment 496048 [details] [diff] [review]
Also turns off "WebSocket" in window

Added code in dom/base/nsDOMClassInfo.cpp to make "Websocket" not show up as property in window if prefs not set for it.

sicking told me to put it there, but wants peterv to review.   (We tried putting similar code in FindConstructorContractID() but that didn't prevent ws property from still showing up).

Note that the precise behavior here is that an existing window which fails to find "WebSocket" will see it once network.websocket.override-security-block has been set to true.   If the pref is then turned off again, existing windows that saw =true will still have websockets enabled, but new windows won't see it. I.e., windows seem to search the pref until they see 'true', then assume true after that even if it changes.   That should be fine for our purposes.

Tested with 

  http://jimbergman.net/websocket-web-browser-test/
Comment 17 Peter Van der Beken [:peterv] - away till Aug 1st 2010-12-08 09:27:42 PST
Comment on attachment 496048 [details] [diff] [review]
Also turns off "WebSocket" in window

>diff --git a/content/base/src/nsWebSocket.h b/content/base/src/nsWebSocket.h

>+  PRBool PrefEnabled();

This should be static.

>diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp

>+    // For now don't expose web sockets unless user has explicitly enabled them
>+    if (name_struct->mDOMClassInfoID == eDOMClassInfo_WebSocket_id) {
>+      if (!nsContentUtils::GetBoolPref("network.websocket.enabled", PR_TRUE) ||
>+          !nsContentUtils::GetBoolPref("network.websocket.override-security-block",
>+                                      PR_FALSE))

This should call nsWebSocket::PrefEnabled (unless it's really hard to include nsWebSocket.h).

>+        return NS_OK;

Need braces around this.
Comment 18 Eric Shepherd [:sheppy] 2010-12-08 09:50:33 PST
Will we be prefixing in addition to disabling by default? I've seen mixed messages on that.
Comment 19 Patrick McManus [:mcmanus] 2010-12-08 09:55:15 PST
(In reply to comment #18)
> Will we be prefixing in addition to disabling by default? I've seen mixed
> messages on that.

no prefix
Comment 20 Patrick McManus [:mcmanus] 2010-12-08 10:37:27 PST
Created attachment 496194 [details] [diff] [review]
: Also turns off "WebSocket" in window v4

addresses comments from review in comment 17

peterv r+
Comment 21 Jason Duell [:jduell] (needinfo me) 2010-12-08 14:16:13 PST
http://hg.mozilla.org/mozilla-central/rev/98d58c46e409
Comment 22 Jason Duell [:jduell] (needinfo me) 2010-12-08 14:48:32 PST
30032 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_ws_basic_tests.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - WebSocket is not defined at http://mochi.test:8888/tests/content/base/test/test_ws_basic_tests.html:34

Looks like we missed a web sockets test.  Patrick, can you fix and post a patch?  I've got to be AFK for an hour or two.
Comment 23 Patrick McManus [:mcmanus] 2010-12-08 14:52:38 PST
(In reply to comment #22)
> 30032 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_ws_basic_tests.html | [SimpleTest/SimpleTest.js,
> window.onerror] An error occurred - WebSocket is not defined at
> http://mochi.test:8888/tests/content/base/test/test_ws_basic_tests.html:34
> 
> Looks like we missed a web sockets test.  Patrick, can you fix and post a
> patch?  I've got to be AFK for an hour or two.

yep, I'll get on it.
Comment 24 Patrick McManus [:mcmanus] 2010-12-08 15:26:32 PST
Created attachment 496325 [details] [diff] [review]
fix content/base/test/test_ws_basic_tests.html
Comment 25 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-12-08 15:36:35 PST
http://hg.mozilla.org/mozilla-central/rev/3a8b2f1490f3
Comment 26 Mark Roggenkamp 2010-12-09 03:27:09 PST
Can't you only disable websockets for non-ssl based connections? Aren't SSL/TLS based websocket connections unaffected since they go through proxies in much the same way as is being proposed for the new handshake?

Thx
Mark
Comment 27 Eric Shepherd [:sheppy] 2010-12-13 08:59:08 PST
Created new document:

https://developer.mozilla.org/en/WebSockets

All this page does is link to the spec and tell you how to turn on WebSockets. It also mentions the security concern.

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