Closed Bug 616733 Opened 14 years ago Closed 14 years ago

disable and/or remove WebSockets for gecko 2.0 due to security problems

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- beta8+
fennec 2.0b3+ ---

People

(Reporter: briansmith, Assigned: mcmanus)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

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
blocking2.0: --- → ?
tracking-fennec: --- → ?
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
Summary: Completely disable and/or remove WebSockets due to security problems → disable and/or remove WebSockets for gecko 2.0 due to security problems
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.
Blocks: 537787
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?
(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.
Assignee: nobody → mcmanus
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.
Can we still make it into b8?  Agree that we should remove sooner rather than later.
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.
Attachment #495839 - Flags: review?(jst)
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.
blocking2.0: ? → beta8+
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?
Daniel, I don't think that's a good idea because it's security facing.  Has too much of the "whatever" button syndrome.
Status: NEW → ASSIGNED
Fair enough, just thought I would try :)
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)
  ...
tracking-fennec: ? → 2.0b3+
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.
Attachment #495839 - Flags: review?(jst) → review+
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.
Attachment #495839 - Flags: review+ → review-
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.
Attachment #495839 - Attachment is obsolete: true
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/
Attachment #496038 - Attachment is obsolete: true
Attachment #496048 - Flags: review?(peterv)
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.
Attachment #496048 - Flags: review?(peterv) → review+
Will we be prefixing in addition to disabling by default? I've seen mixed messages on that.
(In reply to comment #18)
> Will we be prefixing in addition to disabling by default? I've seen mixed
> messages on that.

no prefix
addresses comments from review in comment 17

peterv r+
Attachment #496048 - Attachment is obsolete: true
Attachment #496194 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/98d58c46e409
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
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.
(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.
Attachment #496325 - Flags: review?(jduell.mcbugs)
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
Target Milestone: --- → mozilla2.0b8
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.
Attachment #496325 - Flags: review?(jduell.mcbugs)
You need to log in before you can comment on or make changes to this bug.