680300 - Restrict discoverability of protocol handlers [Tor 1623]

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

In the link, the presence of an extension that registers a protocol handler is detected by:

<img src="sacore:green.gif" onerror="alert('McAfee Site Advisor not Installed.')" onload="alert('McAfee Site Advisor Installed!')"></img>

This is then used to target an exploit against the extension's binary components.

Can we restrict discoverability of non-built-in protocol handlers from the web by not allowing loads from weird protocols with a content page can receive error/load events?

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

As far as I can tell, SiteAdvisor's latest incarnation doesn't register a protocol handler for 'sacore'.  I still think we should consider doing this though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Isn't this up to the extension?  It can list the protocol as not loadable by untrusted sites.  So either sacore: was explicitly listed as loadable by untrusted sites, or the extension listed no security flags on the protocol at all (which defaults to the old "allow loads from anywhere" behavior) and got a warning in the console the moment someone tried to use the protocol in an untrusted page saying that the extension is not providing this information.

I suppose we could go ahead and default to "no load" if none of the protocol security flags are set now that we've had that warning for a few years.  Or we could move the warning earlier in the function so it happens even for loads from inside chrome and then give extension authors a cycle or two to catch any warnings from that, I suppose.  That comes at a performance cost, sadly.

Comment 3

[Tom Ritter [:tjr]]

http://pseudo-flaw.net/tor/torbutton/scan-protocol-handlers.html implies this may be fixed? On Firefox I got popups about two protocol handlers I had installed, but the page said that nothing was discoverable before I clicked cancel or otherwise interacted.

Comment 4

[Ethan Tseng [:ethan]]

The original Tor ticket: https://trac.torproject.org/projects/tor/ticket/1623

According to the Tor ticket and comment 3, nothing needs to be done for this issue.  Might be works for me or won't fix.
But let's move it to the backlog for tracking.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

(In reply to Tom Ritter [:tjr] from comment #3)
> http://pseudo-flaw.net/tor/torbutton/scan-protocol-handlers.html implies
> this may be fixed? On Firefox I got popups about two protocol handlers I had
> installed, but the page said that nothing was discoverable before I clicked
> cancel or otherwise interacted.

There seems to be a small flaw in the test page.  It thinks it discovers a protocol when it catches an error that's not NS_ERROR_UNKNOWN_PROTOCOL, but it doesn't consider the case where no error occurred.

I tweaked the page a little bit, and now it can discover the smb handler on my laptop.
https://people-mozilla.org/~jhao/scan-protocol-handlers.html

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[Tom Ritter [:tjr]]

We think this is still an issue, we're going to bump it to P2 to make a POC that actually works. Then we'll re-triage.

Comment 11

[Tim Huang[:timhuang]]

We would like to make Firefox not to report an exception when the external protocol is not found if fingerprinting resistance mode is enabled. However, we think this should apply to even the normal browsing mode since this is a privacy issue and other browsers, like Edge and Chrome, won't report an exception, see Bug 1437349. But first, we change the behavior only in fingerprinting resistance mode since we don't know that is there any consumer of this. Then, we can send an intent-to-implement mail to see feedbacks regarding this to decide whether we change this on the normal browsing mode.

I've run a try on this [1]. It seems nothing weird happens after changing this behavior.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9c9e958540fe16d01c4695b8411759659aa6cb

What do you think about this, bz?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I would be fine with just aligning with the behavior of the other browsers (and in an ideal world the spec) here, even when we're not in fingerprinting-resistance mode.

We should double-check whether the spec requires firing an "error" event if the load goes off to an external protocol handler, and if so whether that's conditional on it existing.  In general, my preference would be that once we decide it's an external protocol there are no exceptions and no events firing.

Comment 13

[Tim Huang[:timhuang]]

I've checked the spec of navigating [1]. Within that, there is a small paragraph which talks about handling external protocol [2] and it doesn't mention anything regarding the error or event reporting when navigating to an unknown protocol handler. So, IMO, we could change the behavior here.

[1] https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate
[2] https://html.spec.whatwg.org/multipage/browsing-the-web.html#hand-off-to-external-software

Comment 14

[Tim Huang[:timhuang]]

Attachment: Bug 680300 - Part 1: Stopping reporting errors when loading an unknown external protocol.

This patch makes the docshell not to report an error if it is a unknown
protocol error. However, we will still display the error page in this
case.

Comment 15

[Tim Huang[:timhuang]]

Attachment: Bug 680300 - Part 2: Add a test case for ensuring no error reporting when loading an unknown protocol.

This test case will try to navigate an iframe to an unknown protocol and
check whether no errors been reported.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9001564 [details]
Bug 680300 - Part 2: Add a test case for ensuring no error reporting when loading an unknown protocol.

Olli Pettay [:smaug] has approved the revision.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9001563 [details]
Bug 680300 - Part 1: Stopping reporting errors when loading an unknown external protocol.

Olli Pettay [:smaug] has approved the revision.

Comment 18

[Tim Huang[:timhuang]]

It turns out this patch breaks the web-platform-test 'windowclient-navigate.https.html'. In this test, it will use serviceworker.client to navigate an iframe to different URLs. One of those URLs is 'view-source://example.com' which is an invalid URL. According to this [1], the inner URL of a view-source URL should be an absolute URL, that's why this URL is invalid. However, it looks like we won't treat this as an invalid URL since we would resolve this invalid URL into 'view-source:https//example.com' according to its baseURL. And then, we will run into here [2], which reports an NS_ERROR_UNKNOWN_PROTOCOL eventually.

IMHO, this error should be an NS_ERROR_MALFORMED_URI error instead of an NS_ERROR_UNKNOWN_PROTOCOL error in this case. So, we should fail here [3] when creating a URI like this. But, I don't really know how to do it here because an invalid 'view-source' url would be treated and resolved into a valid URL.

What do you think about this, smaug?

[1] https://tools.ietf.org/html/draft-yevstifeyev-view-source-uri-00
[2] https://searchfox.org/mozilla-central/rev/a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/docshell/base/nsDocShell.cpp#10531
[3] https://searchfox.org/mozilla-central/rev/a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/dom/clients/manager/ClientNavigateOpChild.cpp#189

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Fixing view-source sounds right, especially if other browsers behave the same way. (not sure which all support view-source. At least Chrome)

Comment 20

[Tim Huang[:timhuang]]

Hi Henri,

The change of allowing view-source to reference baseURL was made by you in Bug 656904. Was this intentional for all protocols or only for the 'file:' protocol? If this is for the later, maybe we can change to only allow using relative inner URL for a view-source URL if its baseURL has a 'file:' protocol. For other protocols, we report an NS_ERROR_MALFORMED_URI error.

What do you think?

Comment 21

[Henri Sivonen (:hsivonen)]

I think it was intentional for all URIs in the sense that if you are viewing the source of a document that has relative links, the links in the View Source viewer should work by letting you view the source of the linked pages by following links in the View Source view.

When viewing the source of a file:-URL document, it should be possible to follow relative links to other file:-URL documents. Likewise, when viewing the source of a http(s):-URL document, it should be possible to follow relative links to other http(s):-URL documents.

Consequences beyond that are incidental.

Comment 22

[Tim Huang[:timhuang]]

According to what you said, the relative link for view-source URL is for the purpose of following links in a view source view. So, could we only allow the relative links of view-source URL if its baseURL is also a view-source URL? Would this change break things?

Comment 23

[Henri Sivonen (:hsivonen)]

(In reply to Tim Huang[:timhuang] from comment #22)
> According to what you said, the relative link for view-source URL is for the
> purpose of following links in a view source view. So, could we only allow
> the relative links of view-source URL if its baseURL is also a view-source
> URL? Would this change break things?

It might break taking <base href> into account in View Source unless it's special-cased somehow.

Comment 24

[Tim Huang[:timhuang]]

(In reply to Henri Sivonen (:hsivonen) from comment #23)
> (In reply to Tim Huang[:timhuang] from comment #22)
> > According to what you said, the relative link for view-source URL is for the
> > purpose of following links in a view source view. So, could we only allow
> > the relative links of view-source URL if its baseURL is also a view-source
> > URL? Would this change break things?
> 
> It might break taking <base href> into account in View Source unless it's
> special-cased somehow.

I think we might be able to do that. We can still allow the creating of relative view-source URL and check the base URL inside somewhere in the docShell. If the top-level URL is not the view-source URL, then we return NS_ERROR_MALFORMED_URI. 

What do you think about this, smaug?

Comment 25

[Tim Huang[:timhuang]]

It looks like I was wrong about this. There is no way that docShell can do for this. It is because of that the URI of relative view-source will be resolved before it been passed into the docShell. In other words, docShell won't see the relative view-source URL at all. So, it cannot do anything about this.

I think we should keep the behavior of the view-source since changing it could bring some unexpected behaviors according to comment 23, and try to fix the web-platform-test by explicitly checking the validity of URL here [1].

[1] https://searchfox.org/mozilla-central/rev/a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/dom/clients/manager/ClientNavigateOpChild.cpp#143

Comment 26

[Tim Huang[:timhuang]]

Attachment: Bug 680300 - Part 3: Make the client.navigate() not to reference the baseURL if it navigates to a view-source URL

The suppressing of the error NS_ERROR_UNKNOWN_PROTOCOL will break the
web-platform-test 'windowclient-navigate.https.html' since navigating
to an invalid view-source url through the client API won't receive
any error due to the suppressing. So the test will time-out since it
waits for an error.

While navigating to an invalid view-source url with its inner url as
relative, this will pass the validity check we have right now and
do the navigation because of it takes account the baseURL while doing
the check. The invalid view-source url will be resolved into a valid
view-source url in the case. Fortunately, we won't encounter any issue
in the test in the past since the docShell will block this loading
because it's loading a view-source url inside an iframe and reports a
NS_ERROR_UNKNOWN_PROTOCOL error. But, we should faild with a
NS_ERROR_MALFORMED_URI error when doing the URL validity check.

For addressing this, this patch makes the client.navigate to not take
the baseURL into account if it is a view-source URL.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9011323 [details]
Bug 680300 - Part 3: Make the client.navigate() not to reference the baseURL if it navigates to a view-source URL

Andrew Sutherland [:asuth] has approved the revision.

Comment 28

[Tim Huang[:timhuang]]

Try looks good. Ready to land.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee97f8e0f33edce6e4261cdf856c27873f99416

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/075283b600c9
Part 1: Stopping reporting errors when loading an unknown external protocol. r=smaug
https://hg.mozilla.org/integration/autoland/rev/480efe465820
Part 2: Add a test case for ensuring no error reporting when loading an unknown protocol. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8c7ce2d0a95f
Part 3: Make the client.navigate() not to reference the baseURL if it navigates to a view-source URL r=asuth

Comment 30

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/075283b600c9
https://hg.mozilla.org/mozilla-central/rev/480efe465820
https://hg.mozilla.org/mozilla-central/rev/8c7ce2d0a95f

Comment 31

[Kohei Yoshino]

Posted site compatibility note just in case: https://www.fxsitecompat.com/en-CA/docs/2018/loading-unknown-protocol-no-longer-raises-exception/