1207775 - [meta] Ensure that the channel classifier is used for all HTTP requests

Status: NEW

(Toolkit :: Safe Browsing, enhancement, P3)

Description

[François Marier [:francois]]

We should make sure that all HTTP connections created in Necko go through the URL classifier (i.e. use Safe Browsing).

If that's the case, we'll need to whitelist a few internal uses:

- Firefox updates
- add-on blocklist updates
- Safe Browsing downloads and gethash requests

Comment 1

[François Marier [:francois]]

We only classify URLs if the nsIChannel::LOAD_CLASSIFY_URI loadflag is set:

https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/netwerk/base/nsBaseChannel.cpp#305

and we're not out of memory:

https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/netwerk/base/nsBaseChannel.cpp#310

In fact, DocShell has a handy flag to bypass the classifier:

https://dxr.mozilla.org/mozilla-central/rev/abe43c30d78d7546ed7c622c5cf62d265709cdfd/docshell/base/nsDocShell.cpp#10798

Comment 2

[François Marier [:francois]]

Some ideas from today's content security meeting:

- flip the flag around (LOAD_DONT_CLASSIFY_URI) so that call sites have to opt out instead of having to opt in

- add assertions to catch channel creation that doesn't specify that load flag

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to François Marier [:francois] from comment #2)
> Some ideas from today's content security meeting:
> 
> - flip the flag around (LOAD_DONT_CLASSIFY_URI) so that call sites have to
> opt out instead of having to opt in

I think that should definitely the ultimate goal for this bug so that we can somehow ensure that we always enforced for Safebrowsing.

> - add assertions to catch channel creation that doesn't specify that load
> flag

To catch all of the low hanging fruit we could add some kind of assertion here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.inl#151

Something like:
uri.scheme is not http and uri.scheme not https then all good, otherwise make sure the classifier flag is set.

Comment 4

[François Marier [:francois]]

As a first step, we should look for all of the places where LOAD_CLASSIFY_URI is missing (and should be there) and then create sub-bugs for each one.

The fixes are simple (see for example the dependent bugs that have already been fixed) but testing can be hard depending on what the load does.

Comment 5

[Dimi Lee [:dimi]]

I would like to work on this one

Comment 6

[Dimi Lee [:dimi]]

Attachment: callsites that do not use LOAD_CLASSIFY_URI

https://docs.google.com/spreadsheets/d/1Pii93dBTatYI0S_81otfUipNPTBpqxLgbUQgQM6Wt_Q/edit#gid=0

List all callsites that call NS_NewChannel or NS_NewChannelWithTriggeringPrincipal without setting  LOAD_CLASSIFY_URI

Comment 7

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #6)
> https://docs.google.com/spreadsheets/d/
> 1Pii93dBTatYI0S_81otfUipNPTBpqxLgbUQgQM6Wt_Q/edit#gid=0

That list is not as long as I feared :)

There are a couple on there that stand out to me as things we should probably fix:

EventSource
HTMLTrackElement
Loader::LoadSheet
PeerConnectionMedia
AsyncFetchAndSetIconFromNetwork
nsManifest
FontFace

Comment 8

[François Marier [:francois]]

We will also need to search for all of the callsites that set loadflags on the docshell, for example:

https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#43-47

Comment 9

[Honza Bambas (:mayhemer)]

You probably mean 'request' instead of 'connection'.  Connection is a shared resource that either is connecting to a TP domain or not.  But TP annotation and malware protection happens on the level of a request (URL).

I don't think this bug is about isolating http (1, 2) connections to tracking domain by the first party origin, either.  Long keep-alive connections could be used for user tracking if referrers are sent on tracking requests.

Comment 10

[François Marier [:francois]]

Comment on attachment 8734639 [details]
callsites that do not use LOAD_CLASSIFY_URI

Updated list of callsites at https://docs.google.com/spreadsheets/d/1A3pwchORIIcSrIt4u6mQehvbjsPWks51m2A_scYqCKc/edit#gid=0.

Comment 11

[François Marier [:francois]]

Attachment: bug1207775-outline.patch

Honza, what do you think of this approach to making classification through the URL classifier the default?

Essentially, we'd like to force any channel that is associated with a content window to go through the URL classifier. The idea is that this would exclude all internal channels that perform critical functions (e.g. Firefox updates, TLS validation, etc.).

Do you think the check for associatedWindow (and then loadcontext) is enough? Or are there other load flags we should look at when determining whether or not to run a request through the classifier?

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 9020911 [details] [diff] [review]
bug1207775-outline.patch

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

This needs some more thinking, but my first thought is that this will not enforce the do-classify flag on e.g. downloads and actually any top-level navigation.  When you open a new tab, the URL you enter is starting to load (hit the code path you modify) BEFORE there is a content window associated with the channel, we don't know what content we are loading this soon.

I believe we should revert the flag to bypass classification and set it only for channel we specifically select.  Firefox Updates, OCSP (="TLS validation?"), Classification database updates/downloads, add-updates (from trusted parties), all of them creates channels on specific places.  So, defining the "bypass-classify" flag there seems reasonable to me.

But if your research suggests it's too complicated to go that way (I can't read any conclusion for it anywhere in bugzilla or the doc you link to) please elaborate.

Thanks for doing this, btw!

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> I believe we should revert 

*invert

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 9020911 [details] [diff] [review]
bug1207775-outline.patch

tentative f- in comment 12

Comment 15

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> This needs some more thinking, but my first thought is that this will not
> enforce the do-classify flag on e.g. downloads and actually any top-level
> navigation.  When you open a new tab, the URL you enter is starting to load
> (hit the code path you modify) BEFORE there is a content window associated
> with the channel, we don't know what content we are loading this soon.

Good point.  The initial navigation case at least will have the LOAD_DOCUMENT_URI load flag.  The download case...  I'm not sure of.

IINM this is the call site in question: <https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/toolkit/components/downloads/DownloadCore.jsm#1903>.  I see that here we override the private browsing handling for the channel, which is needed in cases where we can't find an associated content window.  So if we want to go with something like the patch Francois has, we may need to handle this case as a special case.  (Note to Francois, pay attention to places where we call nsIPrivateBrowsingChannel::SetPrivate, there may be more special cases like this hiding elsewhere).

Comment 16

[François Marier [:francois]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> I believe we should revert the flag to bypass classification and set it only
> for channel we specifically select.  Firefox Updates, OCSP (="TLS
> validation?"), Classification database updates/downloads, add-updates (from
> trusted parties), all of them creates channels on specific places.  So,
> defining the "bypass-classify" flag there seems reasonable to me.

Based on your feedback and discussions with a few other people, I have flipped this around. Here's the revised plan:

https://docs.google.com/document/d/1byFAOCVhg5669AwbU5i0cfs_MrzTojXTiK2EMn_-Kno/edit