1408410 - nsChannelClassifier::ShouldEnableTrackingProtectionInternal bails on failure of chan->GetTopWindowURI (NS_ERROR_ILLEGAL_VALUE aka NS_ERROR_INVALID_ARG)

Status: RESOLVED WORKSFORME

(Core :: Networking, defect, P3)

Description

[Honza Bambas (:mayhemer)]

Looks like it happens for chrome initiated requests like https://firefox.settings.services.mozilla.com/v1/buckets/pinning/collections/pins/records?_sort=-last_modified

GetAssociatedWindow in ThirdPartyUtil::GetTopWindowForChannel returns (obviously) null.

Comment 1

[Valentin Gosu [:valentin]]

Do you have an assignee in mind, or the next steps we should take here?

Comment 2

[Honza Bambas (:mayhemer)]

First, we need to find out if this is really a problem.  Then, we would take further steps.

Kershaw was doing a lot of changes around this code lately.  Maybe him?

Comment 3

[Kershaw Chang [:kershaw]]

It seems to me we should disable tracking protection for all chrome initiated requests.

Francois, do you think can we add a check to see if this channel's principal is system principal or not before calling GetTopWindowURI()? If it's system principal, we can return false in ShouldEnableTrackingProtectionInternal().

Comment 4

[François Marier [:francois]]

(In reply to Kershaw Chang [:kershaw] from comment #3)
> Francois, do you think can we add a check to see if this channel's principal
> is system principal or not before calling GetTopWindowURI()? If it's system
> principal, we can return false in ShouldEnableTrackingProtectionInternal().

Would that also include all about: pages?

We have a bug open (bug 1380448) to run about: pages through the classifier since some of them now include remote content that could include trackers.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to François Marier [:francois] from comment #4)
> (In reply to Kershaw Chang [:kershaw] from comment #3)
> > Francois, do you think can we add a check to see if this channel's principal
> > is system principal or not before calling GetTopWindowURI()? If it's system
> > principal, we can return false in ShouldEnableTrackingProtectionInternal().
> 
> Would that also include all about: pages?
> 

I am not 100% sure, but it looks like it won't include all about pages.

I think the most common network requests issued from about pages are loading page thumbnails [1]?
If yes, the channel's principal is not system principal.

Taking loading thumbnails as an example, the problem is that GetTopWindowURI() still fails, so we still can not do tracking protection for this channel. It seems that GetTopWindowURI() fails because the channel is created from a window less browser [2].

I think we may need to reconsider whether to use top window uri here. For example, if www.google-analytics.com/analytics.js is loaded from twitter.com and twitter.com is loaded from about:newtab, what uri should we used to determine if www.google-analytics.com/analytics.js is a third party load? It's about:newtab or twitter.com?




[1] https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#107
[2] https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#194

Comment 6

[François Marier [:francois]]

(In reply to Kershaw Chang [:kershaw] from comment #5)
> I think we may need to reconsider whether to use top window uri here. For
> example, if www.google-analytics.com/analytics.js is loaded from twitter.com
> and twitter.com is loaded from about:newtab, what uri should we used to
> determine if www.google-analytics.com/analytics.js is a third party load?
> It's about:newtab or twitter.com?

That's an interesting one. I can think of arguments for either of these:

1. It's a third-party load compared to about:newtab. If someone enables TP, they don't want Firefox to track them using a third-party service.

2. It's a third-party load compared to twitter.com because the browser UI is part of the user agent, not the first party that the user talks to.

The reason why we switched to using the topWindowURI in bug 1108017 was for the following example:

  facebook.com -> game.com (iframe) -> facebook.com (sub-resource)

Maybe what we need to do is assume that a load is third-party if there's no topWindowURI? Would that cause problems with internals loads maybe?

Comment 7

[Kershaw Chang [:kershaw]]

> I think the most common network requests issued from about pages are loading
> page thumbnails [1]?
> If yes, the channel's principal is not system principal.
> 
> Taking loading thumbnails as an example, the problem is that
> GetTopWindowURI() still fails, so we still can not do tracking protection
> for this channel. It seems that GetTopWindowURI() fails because the channel
> is created from a window less browser [2].
> 
> I think we may need to reconsider whether to use top window uri here. For
> example, if www.google-analytics.com/analytics.js is loaded from twitter.com
> and twitter.com is loaded from about:newtab, what uri should we used to
> determine if www.google-analytics.com/analytics.js is a third party load?
> It's about:newtab or twitter.com?

After a deeper look, I found what I said here is not correct.
In this case, GetTopWindowURI() only fails at the first request made to twitter.com, which is issued by about:blank.
So, I think loading page thumbnails is not a problem anymore.

Comment 8

[Kershaw Chang [:kershaw]]

   (In reply to Honza Bambas (:mayhemer) from comment #0)
> Looks like it happens for chrome initiated requests like
> https://firefox.settings.services.mozilla.com/v1/buckets/pinning/collections/
> pins/records?_sort=-last_modified
> 
> GetAssociatedWindow in ThirdPartyUtil::GetTopWindowForChannel returns
> (obviously) null.

Pre discussion with Francois, we think that LOAD_CLASSIFY_URI should not be applied to those chrome initiated requests, since we know those URIs are safe.
However, it's difficult to remove LOAD_CLASSIFY_URI because it's the default load flag for xhr and fetch requests.

Maybe we might want to:
1. Remove LOAD_CLASSIFY_URI for the requests created from nsIXMLHttpRequest like [1].
2. Use nsIXMLHttpRequest to replace those xhr and fetch requests so that we can remove LOAD_CLASSIFY_URI for them.

Or we can choose not to fix this bug, since GetTopWindowURI() still fails and TP is disabled anyway.
What do you think, Honza? 



[1] https://searchfox.org/mozilla-central/source/toolkit/components/captivedetect/captivedetect.js#31

Comment 9

[Honza Bambas (:mayhemer)]

I don't want to add code that would be selectively removing LOAD_CLASSIFY_URI from requests.

So, if chan->GetTopWindowURI fails, it's not a problem?  Can you once more list all cases for which it's OK to fail, if it's possible to determine during the GetTopWindowURI call?

The failure only shows in text console output of debug builds, anyway.  It's hard to say if there is a negligence on our side for top-level navigational requests.  If you navigate from about:newtab to a test URI that is classified as malicious, do we interrupt the navigation?  I hope we have tests for this that would yell if not, right?

I think if it's easy to figure out cases when GetTopWindowURI may fail, just remove the NS_ENSURE_SUCCESS (or what is writing to the console) and, with a good comment, make it an "ok to fail" code path.

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> I don't want to add code that would be selectively removing
> LOAD_CLASSIFY_URI from requests.
> 

At least we can remove LOAD_CLASSIFY_URI at [1] or some other places that we know the URI is completely safe?

[1] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/captivedetect/captivedetect.js#35-39

> So, if chan->GetTopWindowURI fails, it's not a problem?  Can you once more
> list all cases for which it's OK to fail, if it's possible to determine
> during the GetTopWindowURI call?
> 

I think it's not a problem for chrome initiated requests, since there is no top window for those requests.
However, Francois mentioned that in bug 1380448, tracking protection is not working for some about pages. Unfortunately, I can't reproduce the problem in bug 1380448 comment #0.
ni Francois for more information here.

Additionally, it's definitely a problem when a request is initiated from a service worker. In this case, it is not easy to get the top window uri. However, I think we can use the URI from channel loadinfo's LoadingPrincipal, which should be the service worker's script URI.

> The failure only shows in text console output of debug builds, anyway.  It's
> hard to say if there is a negligence on our side for top-level navigational
> requests.  If you navigate from about:newtab to a test URI that is
> classified as malicious, do we interrupt the navigation?  I hope we have
> tests for this that would yell if not, right?
> 

Top window URI is only used for tracking protection. We don't need it when checking malicious and phishing list.

> I think if it's easy to figure out cases when GetTopWindowURI may fail, just
> remove the NS_ENSURE_SUCCESS (or what is writing to the console) and, with a
> good comment, make it an "ok to fail" code path.

I think it's not easy to figure out cases why GetTopWindowURI fails, but I still think we can remove NS_ENSURE_SUCCESS, since what really matters is the URI, not the return value.

Comment 11

[Kershaw Chang [:kershaw]]

> I think it's not a problem for chrome initiated requests, since there is no
> top window for those requests.
> However, Francois mentioned that in bug 1380448, tracking protection is not
> working for some about pages. Unfortunately, I can't reproduce the problem
> in bug 1380448 comment #0.
> ni Francois for more information here.
> 

I just found that for the case mentioned in bug 1380448 GetTopWindowURI() returns NS_OK but the top window URI is empty.
I'd like to cancel the ni to Francois since I already got what I want to know.

Comment 12

[Honza Bambas (:mayhemer)]

OK, so, 

- what exactly do we need the toplevelwindow uri for?
- is there a case we should have it (as there is one), but querying it fails?  [service worker initiated requests, requests from about: pages?]
- if that happens, what is the impact on the user?

Sorry, i'm kinda getting lost in this.

Comment 13

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> OK, so, 
> 
> - what exactly do we need the toplevelwindow uri for?
We need it for:
  1. Check whether it's a third party load [1].
  2. Create a whitelist URL [2].

Note that both two uses are for tracking protection.

> - is there a case we should have it (as there is one), but querying it
> fails?  [service worker initiated requests, requests from about: pages?]

Currently, I've observed two cases where we can't get a top window uri.
1. Service worker
  I am not sure if there is a top window in this case, since a service worker can entirely run in background?
2. about: pages
  Currently, I only see one case in about:addons, but GetTopWindowURI() returns an empty URI.
  There could be more, but I am not aware for now.

There are other cases for chrome initiated xhr and fetch requests, which also have no top window URI. I think it's fine to ignore these.

> - if that happens, what is the impact on the user?

Tracking protection and channel annotation would be broken.


[1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/netwerk/base/nsChannelClassifier.cpp#364
[2] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/netwerk/base/nsChannelClassifier.cpp#1062-1064

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #13)
> > - is there a case we should have it (as there is one), but querying it
> > fails?  [service worker initiated requests, requests from about: pages?]
> 
> Currently, I've observed two cases where we can't get a top window uri.
> 1. Service worker
>   I am not sure if there is a top window in this case, since a service
> worker can entirely run in background?

I *think* the document that has started the worker should stand as a top window here, but this is more question for :bkelly, :bz or :jdm.

Note that there are also shared workers, but I - again - *think* that those can be shared only within the same origin, so not a problem (there would always be only a single source/top-level domain)

I also think this could be separated to a different bug (in the service worker component perhaps?) and start discussion there what we should do.  But first, see below.

> 2. about: pages
>   Currently, I only see one case in about:addons, but GetTopWindowURI()
> returns an empty URI.

This could be fine, since about:addons is more or less a chrome page, so anything coming out of it might be considered a chrome initiated load, IMO, unless I'm missing something.  Not sure who to ask hot this exactly works.

>   There could be more, but I am not aware for now.

OK, so we need to assess the impact and prioritize if we need to catch them all or not.

> 
> There are other cases for chrome initiated xhr and fetch requests, which
> also have no top window URI. I think it's fine to ignore these.

Yep, chrome initiated are fine, IMO.

> 
> > - if that happens, what is the impact on the user?
> 
> Tracking protection and channel annotation would be broken.

OK, but how exactly?  Sorry, to decide how serious this whole matter is is to determine "this brokenness" as precisely as we can.  Then we can go on.

Thanks!

Comment 15

[François Marier [:francois]]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> > 2. about: pages
> >   Currently, I only see one case in about:addons, but GetTopWindowURI()
> > returns an empty URI.
> 
> This could be fine, since about:addons is more or less a chrome page, so
> anything coming out of it might be considered a chrome initiated load, IMO,
> unless I'm missing something.  Not sure who to ask hot this exactly works.

Unfortunately, I'm not sure we can rely on these pages being non-tracking anymore. What led to bug 1380448 was the fact that about:addons used to load Google Analytics. That got fixed, but it highlights the fact that about: pages can be hosted on the network and should probably be run through the classifier.

Comment 16

[Kershaw Chang [:kershaw]]

> OK, but how exactly?  Sorry, to decide how serious this whole matter is is
> to determine "this brokenness" as precisely as we can.  Then we can go on.
> 
To be more specifically, if GetTopWindow() fails or returns a null uri, the result of nsChannelClassifier::ShouldEnableTrackingProtectionInternal() is fasle. That means tracking protection check will be bypassed for the channel.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #16)
> > OK, but how exactly?  Sorry, to decide how serious this whole matter is is
> > to determine "this brokenness" as precisely as we can.  Then we can go on.
> > 
> To be more specifically, if GetTopWindow() fails or returns a null uri, the
> result of nsChannelClassifier::ShouldEnableTrackingProtectionInternal() is
> fasle. That means tracking protection check will be bypassed for the channel.

That is the worst possible outcome.

So, chrome initiated lone (windowless) requests are OK here.  For this we should not log an error and also accept null top-win URI.  triggering principal == system principal is probably the way to recognize those.

Different case are sub-resource loads that this fails for (including OK+null top URI) and could be trackers, could happen for the top level content pages we are not controlling the content of (or even when we do and tracker could somehow slip in, like for about:addons).

Some about: pages and moz-extension: pages could fall under this.  Cases should be found.  If we find out that we lack top-level uri for such sub-requests, we should try to fix it.  Along with it , a stronger action should be taken.  We could fail the load completely when top-uri can't be determine or simply engage TP w/o checking the whitelist (could have a similar outcome as not-load when found to be a tracker)

I think it would be good to put some effort to this to try to be on the safer side than to let trackers leak out despite TP being on and claiming we do block trackers.

Comment 18

[Kershaw Chang [:kershaw]]

I think we can close this, since tracking protection changed a lot since this bug was filed.