Ensure that the channel classifier is used for all HTTP requests

NEW
Unassigned

Status

()

defect
P2
normal
4 years ago
5 months ago

People

(Reporter: francois, Unassigned)

Tracking

(Depends on 6 bugs, Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tp-leak)

Attachments

(2 obsolete attachments)

Reporter

Description

4 years ago
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
Reporter

Updated

4 years ago
Blocks: 1149867
Reporter

Updated

4 years ago
Depends on: 1216793
Reporter

Comment 2

4 years ago
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.
Reporter

Comment 4

3 years ago
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
Reporter

Comment 7

3 years ago
(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
Reporter

Updated

3 years ago
Priority: -- → P2
Reporter

Updated

3 years ago
Depends on: 523095
Depends on: 1262339
Depends on: 1262406
Assignee: dlee → nobody
Status: ASSIGNED → NEW
Reporter

Updated

3 years ago
Priority: P2 → P3
Reporter

Updated

3 years ago
Depends on: 1231543
Reporter

Updated

Last year
See Also: → 1442496
Reporter

Updated

Last year
Whiteboard: tpe-seceng
Reporter

Updated

Last year
Depends on: 1437626
Reporter

Updated

Last year
Depends on: 1447935
Reporter

Updated

Last year
Whiteboard: tp-leak
Reporter

Updated

Last year
Depends on: 1456486
Reporter

Updated

Last year
Blocks: antitracking

Updated

Last year
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
Reporter

Updated

10 months ago
Depends on: 1483510
Reporter

Updated

8 months ago
Blocks: 1502573
Reporter

Comment 10

8 months ago
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
Reporter

Comment 11

8 months ago
Posted 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)
Reporter

Updated

8 months ago
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-

Comment 15

8 months ago
(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).
Reporter

Updated

7 months ago
Depends on: 1442496
See Also: 1442496
Reporter

Comment 16

7 months ago
(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
Reporter

Updated

7 months ago
Attachment #9020911 - Attachment is obsolete: true
Reporter

Updated

7 months ago
Assignee: francois → nobody
Status: ASSIGNED → NEW
Depends on: 1522412
You need to log in before you can comment on or make changes to this bug.