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)
Core
Networking: WebSockets
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: briansmith, Assigned: mcmanus)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 3 obsolete files)
8.62 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
tracking-fennec: --- → ?
Reporter | ||
Comment 1•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Summary: Completely disable and/or remove WebSockets due to security problems → disable and/or remove WebSockets for gecko 2.0 due to security problems
Reporter | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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?
Assignee | ||
Comment 4•14 years ago
|
||
(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
Reporter | ||
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #495839 -
Flags: review?(jst)
Comment 8•14 years ago
|
||
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+
Comment 9•14 years ago
|
||
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•14 years ago
|
||
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
Comment 11•14 years ago
|
||
Fair enough, just thought I would try :)
Comment 12•14 years ago
|
||
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) ...
Updated•14 years ago
|
tracking-fennec: ? → 2.0b3+
Comment 13•14 years ago
|
||
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-
Assignee | ||
Comment 15•14 years ago
|
||
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
Comment 16•14 years ago
|
||
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 17•14 years ago
|
||
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+
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 18•14 years ago
|
||
Will we be prefixing in addition to disabling by default? I've seen mixed messages on that.
Assignee | ||
Comment 19•14 years ago
|
||
(In reply to comment #18) > Will we be prefixing in addition to disabling by default? I've seen mixed > messages on that. no prefix
Assignee | ||
Comment 20•14 years ago
|
||
addresses comments from review in comment 17 peterv r+
Attachment #496048 -
Attachment is obsolete: true
Attachment #496194 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 21•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/98d58c46e409
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: checkin-needed
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
(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.
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #496325 -
Flags: review?(jduell.mcbugs)
Comment 26•14 years ago
|
||
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
Updated•14 years ago
|
Target Milestone: --- → mozilla2.0b8
Comment 27•14 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•13 years ago
|
Attachment #496325 -
Flags: review?(jduell.mcbugs)
You need to log in
before you can comment on or make changes to this bug.
Description
•