Open Bug 1207775 Opened 5 years ago Updated 5 months ago

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

Categories

(Toolkit :: Safe Browsing, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: francois, Unassigned)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

(Keywords: meta, Whiteboard: tp-leak)

Attachments

(2 obsolete files)

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
Blocks: 1149867
Depends on: 1216793
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
(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.
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.
Whiteboard: tpe-seceng
I would like to work on this one
Assignee: nobody → dlee
Status: NEW → ASSIGNED
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
(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
Priority: -- → P2
Depends on: 523095
Depends on: 1262339
Depends on: 1262406
Assignee: dlee → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Depends on: 1231543
See Also: → 1442496
Whiteboard: tpe-seceng
Depends on: 1437626
Depends on: 1447935
Whiteboard: tp-leak
Depends on: 1456486
Blocks: antitracking
Blocks: cookierestrictions
No longer blocks: antitracking
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.
Summary: Ensure that the channel classifier is used for all HTTP connections → Ensure that the channel classifier is used for all HTTP requests
Depends on: CVE-2019-11725
Blocks: 1502573
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.
Attachment #8734639 - Attachment is obsolete: true
Attached patch bug1207775-outline.patch (obsolete) — Splinter Review
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?
Assignee: nobody → francois
Status: NEW → ASSIGNED
Attachment #9020911 - Flags: feedback?(honzab.moz)
Priority: P3 → P2
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!
(In reply to Honza Bambas (:mayhemer) from comment #12)
> I believe we should revert 

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

tentative f- in comment 12
Attachment #9020911 - Flags: feedback?(honzab.moz) → feedback-
(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).
Depends on: 1442496
See Also: 1442496
(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
Attachment #9020911 - Attachment is obsolete: true
Assignee: francois → nobody
Status: ASSIGNED → NEW
Depends on: 1522412
Type: defect → enhancement
Keywords: meta
Priority: P2 → P3
Summary: Ensure that the channel classifier is used for all HTTP requests → [meta] Ensure that the channel classifier is used for all HTTP requests
Whiteboard: tp-leak → tp-leak,
Whiteboard: tp-leak, → tp-leak
Depends on: 1640574
You need to log in before you can comment on or make changes to this bug.