Open
Bug 1207775
Opened 9 years ago
Updated 2 years ago
[meta] Ensure that the channel classifier is used for all HTTP requests
Categories
(Toolkit :: Safe Browsing, enhancement, P3)
Toolkit
Safe Browsing
Tracking
()
NEW
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
Reporter | ||
Comment 1•9 years ago
|
||
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
Reporter | ||
Comment 2•9 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
Comment 3•9 years ago
|
||
(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 | ||
Updated•9 years ago
|
Reporter | ||
Comment 4•9 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
Comment 5•9 years ago
|
||
I would like to work on this one
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Comment 6•9 years ago
|
||
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•9 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•9 years ago
|
Priority: -- → P2
Updated•8 years ago
|
Assignee: dlee → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P3
Reporter | ||
Updated•7 years ago
|
Whiteboard: tpe-seceng
Reporter | ||
Comment 8•7 years ago
|
||
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
Reporter | ||
Updated•7 years ago
|
Whiteboard: tp-leak
Reporter | ||
Updated•7 years ago
|
Blocks: antitracking
Updated•6 years ago
|
Comment 9•6 years ago
|
||
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•6 years ago
|
Depends on: CVE-2019-11725
Reporter | ||
Comment 10•6 years 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•6 years ago
|
||
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•6 years ago
|
Priority: P3 → P2
Comment 12•6 years ago
|
||
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•6 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #12)
> I believe we should revert
*invert
Comment 14•6 years ago
|
||
Comment on attachment 9020911 [details] [diff] [review]
bug1207775-outline.patch
tentative f- in comment 12
Attachment #9020911 -
Flags: feedback?(honzab.moz) → feedback-
Comment 15•6 years 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•6 years ago
|
Reporter | ||
Comment 16•6 years 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•6 years ago
|
Attachment #9020911 -
Attachment is obsolete: true
Reporter | ||
Updated•6 years ago
|
Assignee: francois → nobody
Status: ASSIGNED → NEW
Updated•5 years ago
|
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,
Updated•5 years ago
|
Whiteboard: tp-leak, → tp-leak
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•