Bug 140624

Summary: Web sockets should be treated as active mixed content
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebCore Misc.Assignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ap, beidson, buildbot, cgarcia, commit-queue, mcatanzaro, rniwa, sam, zan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 140625    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Patch
none
Block mixed content web sockets
none
Patch none

Description Michael Catanzaro 2015-01-19 10:30:23 PST
A page loaded via HTTPS should not be allowed to open insecure (ws://) web sockets; only secure (wss://) web sockets should be permitted.

Chrome, Internet Explorer, and Firefox all block mixed content web sockets by default [1] (although Chrome began blocking after [1] was published) so there should be no compatibility concerns.

[1] https://community.qualys.com/blogs/securitylabs/2014/03/19/https-mixed-content-still-the-easiest-way-to-break-ssl
Comment 1 Michael Catanzaro 2015-01-19 10:33:15 PST
Created attachment 244906 [details]
Patch
Comment 2 Build Bot 2015-01-19 11:17:29 PST
Comment on attachment 244906 [details]
Patch

Attachment 244906 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5779264867663872

New failing tests:
http/tests/security/mixedContent/websocket/insecure-websocket.html
Comment 3 Build Bot 2015-01-19 11:17:33 PST
Created attachment 244909 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 4 Build Bot 2015-01-19 11:25:10 PST
Comment on attachment 244906 [details]
Patch

Attachment 244906 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5237636742512640

New failing tests:
http/tests/security/mixedContent/websocket/insecure-websocket.html
Comment 5 Build Bot 2015-01-19 11:25:15 PST
Created attachment 244911 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 6 Michael Catanzaro 2015-01-19 12:26:33 PST
Created attachment 244918 [details]
Patch
Comment 7 Michael Catanzaro 2015-02-01 09:19:40 PST
Created attachment 245831 [details]
Block mixed content web sockets
Comment 8 Alexey Proskuryakov 2015-06-17 14:27:38 PDT
rdar://problem/21427511
Comment 9 Michael Catanzaro 2015-06-17 17:05:43 PDT
Hey Alexy, I'd like to pick up work on mixed content blocking again. Could you take a look at bug #142469 please? I'm hesitant to proceed further without approval for how to handle that issue.

In the meantime, I will update the patch in this bug to apply cleanly on master (and not depend on bug #140940). That will require removing the test insecure-websocket-in-main-frame-allowed, since we can't change settings until we solve bug #142469.

Unrelated WARNING to make sure I don't forget: the patch I posted above is missing a protector RefPtr for the lambda in WebSocket::connect.
Comment 10 Brady Eidson 2015-06-17 21:16:50 PDT
Doing this has always obviously been a good idea, and because none of the other major browsers allow it I'm not too concerned about the compatibility risk.

(In reply to comment #9)
> In the meantime, I will update the patch in this bug to apply cleanly on
> master (and not depend on bug #140940). That will require removing the test
> insecure-websocket-in-main-frame-allowed, since we can't change settings
> until we solve bug #142469.

I don't think we need a setting to allow insecure web sockets from a secure page, so I don't think that test is important.

Please update the patch to trunk and I'll review when it's ready.
Comment 11 Michael Catanzaro 2015-06-18 05:01:30 PDT
Thanks.

(In reply to comment #10)
> I don't think we need a setting to allow insecure web sockets from a secure
> page, so I don't think that test is important.

I agree in the case of web sockets, where there is no compatibility risk to blocking unconditionally, but it's a problem for other types of content. For GTK, we will need to revert the patch in bug #142378 for our next release in order to keep our existing API working, unless we come up with a solution for bug #142469 in the meantime, because bug #142378 actually _removed_ the ability for browsers to choose to block content that we don't want to block by default.
Comment 12 Brady Eidson 2015-06-18 08:45:28 PDT
(In reply to comment #11)
> Thanks.
> 
> (In reply to comment #10)
> > I don't think we need a setting to allow insecure web sockets from a secure
> > page, so I don't think that test is important.
> 
> I agree in the case of web sockets, where there is no compatibility risk to
> blocking unconditionally, but it's a problem for other types of content. For
> GTK, we will need to revert the patch in bug #142378 for our next release in
> order to keep our existing API working, unless we come up with a solution
> for bug #142469 in the meantime, because bug #142378 actually _removed_ the
> ability for browsers to choose to block content that we don't want to block
> by default.

Understood. My comment was truly only about this specific feature and this specific test.
Comment 13 Michael Catanzaro 2015-06-18 09:05:07 PDT
Turns out I didn't need a protector RefPtr due to the use of ActiveDOMObject::setPendingActivity(), which I had forgotten modifies the reference count.
Comment 14 Michael Catanzaro 2015-06-18 09:06:33 PDT
Created attachment 255117 [details]
Patch
Comment 15 Alexey Proskuryakov 2015-06-18 09:17:56 PDT
Is this going to break 1Password extension in Safari?
Comment 16 Michael Catanzaro 2015-06-18 09:42:35 PDT
I guess someone with a Mac could investigate that, but why do you think it would break? Surely they don't send passwords over an insecure WebSocket?
Comment 17 Brady Eidson 2015-06-18 10:07:16 PDT
(In reply to comment #16)
> I guess someone with a Mac could investigate that, but why do you think it
> would break? Surely they don't send passwords over an insecure WebSocket?

I believe their extension communicates with their native app on localhost with a WebSocket. I would be surprised if it were plaintext, but I suppose it's possible.
Comment 18 Alexey Proskuryakov 2015-06-18 11:50:19 PDT
I don't know, but I would be surprised if it weren't plaintext - there is no reason to use wss when talking to localhost.
Comment 19 Michael Catanzaro 2015-06-18 12:07:15 PDT
Looks like 1Password also supports Firefox, Chrome, and Opera, all of which block ws:// web sockets on https:// pages.
Comment 20 Alexey Proskuryakov 2015-06-18 12:42:50 PDT
Other browsers also have different mechanisms for extensions, we don't know if 1Password even uses WebSockets there. We should check, I doubt that pure logic will be good enough.
Comment 21 Michael Catanzaro 2015-06-20 20:52:10 PDT
Comment on attachment 255117 [details]
Patch

I'll mark this cq? instead of cq+ due to the concern about 1Password; please give it cq+ when you're comfortable committing it.
Comment 22 Brady Eidson 2015-06-20 22:17:51 PDT
If nobody with a Mac dev environment has taken a look by Monday, I should be able to give 1 password a shot sometime Monday afternoon.
Comment 23 Sam Weinig 2015-06-21 19:25:39 PDT
1Password uses WebSockets from their global page which is not http: but rather safari-extension: (or something close to that). Its worth testing, but I don't think it will be an issue.
Comment 24 WebKit Commit Bot 2015-06-22 15:23:42 PDT
Comment on attachment 255117 [details]
Patch

Clearing flags on attachment: 255117

Committed r185848: <http://trac.webkit.org/changeset/185848>
Comment 25 WebKit Commit Bot 2015-06-22 15:23:47 PDT
All reviewed patches have been landed.  Closing bug.