1039012 - nsChannelClassifier should only cancel 3rd party channels with NS_ERROR_TRACKING_URI

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Only cancel third party channels with NS_ERROR_TRACKING_URI (

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8458176 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Brian, can you do the necko review?

This obviates the need for bug 1032480.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Only cancel third party channels with NS_ERROR_TRACKING_URI (

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8458185 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8458185 [details] [diff] [review]:
-----------------------------------------------------------------

Mmm, does that really belong there? I'd make more sense to me that ChannelClassifier doesn't know at all about the third-party status, but just reports that this URL matches list "guyswedontlikebeingathirdparty". It's then up to the caller to reason if that is problematic and warrants a block or not.

Is it hard to move the logic more towards the callers?

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)
> Comment on attachment 8458185 [details] [diff] [review]
> Only cancel third party channels with NS_ERROR_TRACKING_URI (
> 
> Review of attachment 8458185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mmm, does that really belong there? I'd make more sense to me that
> ChannelClassifier doesn't know at all about the third-party status, but just
> reports that this URL matches list "guyswedontlikebeingathirdparty". It's
> then up to the caller to reason if that is problematic and warrants a block
> or not.

I don't know the whole history of this code, but nsChannelClassifier is the only consumer of netwerk/base/public/nsIURIClassifier.idl. The API that you mention is already implemented by nsIUrlClassifierCallback which receives the matching tables from nsIUrlClassifierDBService.lookup. I can only guess that whoever implemented this in necko did not want necko to know anything about safebrowsing tables, and just wanted to receive an NS_ERROR status.
 
> Is it hard to move the logic more towards the callers?

We could change nsChannelClassifier to call nsIUrlClassifierDBService.lookup instead of nsIURIClassifier.classify, which would move all of the table logic from nsUrlClassifierDBService to necko. Then nsBaseChannel and nsHttpChannel would have to make the decision. I sense this will be a game of hot potato :)

Whoever does the third party check must know the channel in order to get the handle to the window though, so I don't think that will solve your first concern.

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/mozIThirdPartyUtil.idl#124

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Only cancel third party channels with NS_ERROR_TRACKING_URI (

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8459044 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8459044 [details] [diff] [review]:
-----------------------------------------------------------------

I had to change this patch a little bit because defaulting to isThirdParty=false led to problems with chrome:// uris in mochitests.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=810c4e14a5d4

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8459044 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8459044 [details] [diff] [review]:
-----------------------------------------------------------------

After working on bug 1033871 for a little bit, this is not quite the right interface change for nsIURIClassifier.idl. Updated patch coming (or I might just roll this bug into that other one).

Comment 10

[Gian-Carlo Pascutto [:gcp]]

>We could change nsChannelClassifier to call nsIUrlClassifierDBService.lookup instead of nsIURIClassifier.classify, which would move all of the table logic from nsUrlClassifierDBService to necko. Then nsBaseChannel and nsHttpChannel would have to make the decision. I sense this will be a game of hot potato :)

It would make much more sense to me if the code that is checking the third party status would get back the tableList containing "thirdpartydisallowedlist", see that this is relevant for this URI, and take action on that. But you're right that this means leaking knowledge about the lists and what they are outside nsIUrlClassifierDBService, and that's not making things clearer at all.

So I withdraw my objection. Like, we can probably come up with a better interface design, but that's really not on the List Of Things We Want To Actually Implement Right Now.

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Only cancel third party channels with NS_ERROR_TRACKING_URI (

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Only cancel third party channels with NS_ERROR_TRACKING_URI (

Comment 13

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8460582 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8460582 [details] [diff] [review]:
-----------------------------------------------------------------

Well, the allowlisting stuff turns out to be slightly more complicated, but I think the upshot is that instead of passing isThirdParty to nsIURIClassifier.classify we should be passing enableTrackingProtection.

The reason is that UX has requested allowlisting top-level URLs from these checks, and either necko or the loaders that actually create channels is going to have to check the permission manager for this allowlist. Therefore the nsIURIClassifier.classify is going to have to know about isThirdParty or the allowlist in some way. In my view it's better to encapsulate those in a single flag (enableTrackingProtection) rather than pass things in piecemeal.

Try: https://tbpl.mozilla.org/?tree=Try&rev=1fc2c2fb73b5

Thanks,
Monica

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8460582 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8460582 [details] [diff] [review]:
-----------------------------------------------------------------

and this doesn't seem to work in e10s :(

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8460582 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8460582 [details] [diff] [review]:
-----------------------------------------------------------------

Bill, help! mozIThirdPartyUtil.isThirdPartyChannel finds the window associated with the channel and walks up to the top window to find the URI and see if the channel URI is third party relative to it.

This code does not work in new e10s windows. What should I do instead?

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8460582 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8460582 [details] [diff] [review]:
-----------------------------------------------------------------

Chatted with jimm and filed https://bugzilla.mozilla.org/show_bug.cgi?id=1042377 for the e10s issue.

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8460582 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8460582 [details] [diff] [review]:
-----------------------------------------------------------------

I like this a lot better than the first one, because conceptually "enable this list" makes more sense to Classifier than "isThirdParty".

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1214,5 @@
>  
>  // nsChannelClassifier is the only consumer of this interface.
>  NS_IMETHODIMP
>  nsUrlClassifierDBService::Classify(nsIPrincipal* aPrincipal,
> +                                   bool trackingEnabled,

aTrackingEnabled

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8460582 [details] [diff] [review]
Only cancel third party channels with NS_ERROR_TRACKING_URI (

Review of attachment 8460582 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsChannelClassifier.cpp
@@ +46,5 @@
> +
> +    // Third party checks don't work for chrome:// URIs in mochitests, so just
> +    // default to isThirdParty = true
> +    bool isThirdParty = true;
> +    thirdPartyUtil->IsThirdPartyChannel(aChannel, nullptr, &isThirdParty);

Nit: if you ignore the return value intentionally, please use (void) to indicate that. Also, XPIDL allows you to mark a function is infallible, which allows you to write:

bool isThirdParty = thirdPartUtil->IsThirdPartChannel(...) instead.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1214,5 @@
>  
>  // nsChannelClassifier is the only consumer of this interface.
>  NS_IMETHODIMP
>  nsUrlClassifierDBService::Classify(nsIPrincipal* aPrincipal,
> +                                   bool trackingEnabled,

Shouldn't this parameter be called aTrackingProtectionEnabled? "tracking enabled" would mean that we're allowing tracking; i.e. no protection.

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

Fixed name to be aTrackingProtectionEnabled everywhere.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67912630f225

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Bill, help! mozIThirdPartyUtil.isThirdPartyChannel finds the window
> associated with the channel and walks up to the top window to find the URI
> and see if the channel URI is third party relative to it.
> 
> This code does not work in new e10s windows. What should I do instead?

There isn't much you can do in C++ right now. You can access topFrameElement on the nsILoadContext to get the <browser> element associated with the load. If you were in JS, you could just call .currentURI on that. But in C++, there's no way to do that. Bug 596109 is supposed to cover that.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/67912630f225