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
Michael Catanzaro
2015-01-19 10:30:23 PST
Created attachment 244906 [details]
Patch
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 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 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 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
Created attachment 244918 [details]
Patch
Created attachment 245831 [details]
Block mixed content web sockets
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. 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. 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. (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. Turns out I didn't need a protector RefPtr due to the use of ActiveDOMObject::setPendingActivity(), which I had forgotten modifies the reference count. Created attachment 255117 [details]
Patch
Is this going to break 1Password extension in Safari? 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? (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. 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. Looks like 1Password also supports Firefox, Chrome, and Opera, all of which block ws:// web sockets on https:// pages. 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 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.
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. 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 on attachment 255117 [details] Patch Clearing flags on attachment: 255117 Committed r185848: <http://trac.webkit.org/changeset/185848> All reviewed patches have been landed. Closing bug. |