Closed Bug 1044181 Opened 10 years ago Closed 10 years ago

All loaders that create channels with LOAD_CLASSIFY_URI need to annotate the DOM with blocked nodes

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: mmc, Assigned: geko1702+bugzilla)

References

Details

Attachments

(3 files, 16 obsolete files)

object loader now annotates classified tracking nodes
1.49 KB, patch
mmc
: review+
Details | Diff | Splinter Review
test elements are now actually loaded when tracking protection is disabled/broken. added style loader test
6.71 KB, patch
mmc
: review+
Details | Diff | Splinter Review
style loader now annotates classified tracking nodes
1.77 KB, patch
Details | Diff | Splinter Review 
Only the script and image and iframe loaders are doing this right now. Similar to  bug 1029887.
Assignee: nobody → gkontaxis
Attachment #8462930 - Attachment is obsolete: true
Attachment #8467432 - Attachment is obsolete: true
Attachment #8467449 - Attachment is obsolete: true
Attachment #8467452 - Attachment is obsolete: true
Comment on attachment 8467453 [details] [diff] [review]
object loader now annotates classified tracking nodes

Similar and complementary to bug 1029887

https://tbpl.mozilla.org/?tree=Try&rev=a70a684de762
Attachment #8467453 - Flags: review?(khuey)
Attachment #8467450 - Attachment is obsolete: true
(In reply to gkontaxis from comment #8)
> Created attachment 8467458 [details] [diff] [review]
> style loader now annotates classified tracking nodes

Notice the existingData patch. The loader has logic to piggyback on existing load requests for the same URI but this has two side effects: 1) it may piggyback on a prefetching request (for the same URI) 2) it may replace an aLoadData object carrying a valid mOwningElement with an existingData object carrying a null mOwningElement. This results in a null mOwningElement when all bundled requests reach OnStreamComplete(). We require a valid mOwingElement (DOM node) to annotate it as a tracking node. Therefore this patch piggybacks on an existingData object only if it has a valid mOwningElement.

https://tbpl.mozilla.org/?tree=Try&rev=d250af5e83ec
Blocks: 1048643
Attachment #8467458 - Attachment is obsolete: true
(In reply to gkontaxis from comment #9)
> (In reply to gkontaxis from comment #8)
> > Created attachment 8467458 [details] [diff] [review]
> > style loader now annotates classified tracking nodes
> 
> Notice the existingData patch. The loader has logic to piggyback on existing
> load requests for the same URI but this has two side effects: 1) it may
> piggyback on a prefetching request (for the same URI) 2) it may replace an
> aLoadData object carrying a valid mOwningElement with an existingData object
> carrying a null mOwningElement. This results in a null mOwningElement when
> all bundled requests reach OnStreamComplete(). We require a valid
> mOwingElement (DOM node) to annotate it as a tracking node. Therefore this
> patch piggybacks on an existingData object only if it has a valid
> mOwningElement.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d250af5e83ec

Scratch that. What actually happens is that the piggybacking creates a linked list of all sheet load instances depending on a single request. When OnStreamComplete() fires one must process that linked list to locate all sheet load instances that have been served.

It may be the case the first instance in the list does not have an owning element (e.g., case of a prefetching load instance) but the second one does because it originates from an actual link element in the DOM and as such has a valid owning element we can annotate.
Attachment #8467925 - Attachment is obsolete: true
Attachment #8467977 - Attachment is obsolete: true
Comment on attachment 8467981 [details] [diff] [review]
style loader now annotates classified tracking nodes

Once the loader finishes processing a request (which could actually be tied to multiple sheet loads) we check its status and if it's NS_ERROR_TRACKING_URI then that means the request was cancelled because the URI is a tracking URI. We want to store weak references to the link nodes that initiated the respective sheet loads. Ultimately a UI will let the user know which and how many nodes have been blocked for this reason.
Attachment #8467981 - Flags: review?(dbaron)
No longer blocks: 1048643
Could you explain what this is needed for?  I'm trying to understand whether the patch's handling of various null-pointers is appropriate, and that requires understanding what it's for.

I'm worried about two things:

 (1) null doc - I *think* this will only happen if a load completes at some point during teardown of the document, but I'm not 100% sure of that

 (2) null data->mOwningElement - this can happen for @imported sheets (child sheets) and for sheets loaded through some internal APIs.  What does calling doc->AddBlockedTrackingNode(nullptr) do?



Stylistic nits:
 * no "this->"
 * space before ( in "while(data" -- although I'd actually suggest using a for() instead.
Flags: needinfo?(gkontaxis)
We have back-end logic in place that cancels the channel for URIs that are part of a tracking blacklist we maintain. Then, when the respective elements fail to load we want to add weak references to them in a special "blocked elements" array under their respective document. This is useful for when the UI wants to provide the user with information about what has been blocked.

So for style links that fail to load because their URLs were found in the blacklist we want to do this annotation here.

(1) I'm covering my basis.

(2) Yes. This includes prefetching. I observed that usually the prefetching sheet load was bundled with the actual load so that the first mOwningElement was null and the second (in the linked list) was the actual link node. AddBlockedTrackingNode() handles null pointers just fine (checks and returns immediately)
Flags: needinfo?(gkontaxis)
(In reply to gkontaxis from comment #18)
> We have back-end logic in place that cancels the channel for URIs that are
> part of a tracking blacklist we maintain. Then, when the respective elements
> fail to load we want to add weak references to them in a special "blocked
> elements" array under their respective document. This is useful for when the
> UI wants to provide the user with information about what has been blocked.
> 
> So for style links that fail to load because their URLs were found in the
> blacklist we want to do this annotation here.
> 
> (1) I'm covering my basis.
> 
> (2) Yes. This includes prefetching. I observed that usually the prefetching
> sheet load was bundled with the actual load so that the first mOwningElement
> was null and the second (in the linked list) was the actual link node.
> AddBlockedTrackingNode() handles null pointers just fine (checks and returns
> immediately)

https://bug1041748.bugzilla.mozilla.org/attachment.cgi?id=8459851
Attachment #8467981 - Flags: review?(dbaron)
Attachment #8467981 - Attachment is obsolete: true
Attachment #8469651 - Flags: review?(dbaron)
Attachment #8468790 - Flags: review?(mmc)
Comment on attachment 8468790 [details] [diff] [review]
test elements are now actually loaded when tracking protection is disabled/broken. added style loader test

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

::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.html
@@ +24,5 @@
>  ];
>  
>  function checkLoads() {
> +  window.parent.is(scriptItem1, "untouched", "Should not load tracking javascript");
> +  window.parent.is(scriptItem2, "untouched", "Should not load tracking javascript (2)");

over 80
Attachment #8468790 - Flags: review?(mmc) → review+
Don't forget to set r=reviewer in all of the patch descriptions please.
Status: NEW → ASSIGNED
Attachment #8468790 - Attachment is obsolete: true
Attachment #8467453 - Attachment is obsolete: true
Attachment #8469651 - Attachment is obsolete: true
Attachment #8469651 - Flags: review?(dbaron)
Attachment #8470110 - Attachment is obsolete: true
Attachment #8470109 - Attachment is obsolete: true
Attachment #8470107 - Attachment is obsolete: true
Attachment #8470111 - Flags: review?(dbaron)
Comment on attachment 8470112 [details] [diff] [review]
object loader now annotates classified tracking nodes

carrying over r+ from comment 15.5
Attachment #8470112 - Flags: review+
Comment on attachment 8470113 [details] [diff] [review]
test elements are now actually loaded when tracking protection is disabled/broken. added style loader test

carrying over r+ from comment 21
Attachment #8470113 - Flags: review+
Comment on attachment 8470111 [details] [diff] [review]
style loader now annotates classified tracking nodes

So assuming that you're ok with no blocked tracking node being added for @import sheets that are blocked, I guess this is ok, though I'm still not entirely sure how severe false negatives are here.

I thought about asking you to move the handling to the existing loop over data in Loader::DoSheetComplete, but that would have the side-effect that the @import-blocked status might or might not propagate to the parent depending on whether it was the last child sheet remaining for the parent to be complete, which wouldn't be good.

(I kinda wish bz was around to review this since he knows this code substantially better than I do, but I also don't want to delay you or to leave a pile of reviews around for bz when he gets back from vacation.)

It might not be a bad idea to add a code comment that |cont| might be null, but AddBlockedTrackingNode is ok with that.  Also, please rename that variable from cont to content.

r=dbaron with that
Attachment #8470111 - Flags: review?(dbaron) → review+
Attachment #8470111 - Attachment is obsolete: true
I took the liberty of fixing a missing <body> tag in test_classified_annotations.html and also rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf21c0388a3, since adding image loads with the same URL was hitting the cache.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bde52b0b5431
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b1a569d165
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3088e9079d8b
https://hg.mozilla.org/mozilla-central/rev/bde52b0b5431
https://hg.mozilla.org/mozilla-central/rev/c9b1a569d165
https://hg.mozilla.org/mozilla-central/rev/3088e9079d8b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: