Closed Bug 1216793 Opened 4 years ago Closed 4 years ago

Subresources loaded via XHR or fetch() are not caught by TP

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: francois, Assigned: francois)

References

(Blocks 1 open bug)

Details

(Keywords: privacy, sec-want, Whiteboard: [adv-main45-])

Attachments

(4 files)

Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
I can confirm that we aren't blocking URLs that are loaded as a result of an XHR. It is very possible that this page does this, though not 100% sure.
Blocks: 1207775
Summary: Subresources not caught by TP → Subresources loaded via XHR or fetch() are not caught by TP
Duplicate of this bug: 1224593
Keywords: sec-want
Attached patch Fix for XHRSplinter Review
Attachment #8692642 - Flags: review?(gpascutto)
Attached patch Fix for FetchSplinter Review
Assignee: nobody → francois
Status: NEW → ASSIGNED
Attachment #8692643 - Flags: review?(gpascutto)
Attached patch TestsSplinter Review
This fixes the leak both on e10s and non-e10s. There is a remaining issue for which I intend to file a follow-up bug if that's OK with you.

There's something weird going on with e10s and XHR/Fetch and that prevents the security UI (i.e. the shield) from appearing when we block loads. Tanvi and I are investigating because it seems to also affect the mixed content blocker to some extent.
Attachment #8692644 - Flags: review?(gpascutto)
Comment on attachment 8692644 [details] [diff] [review]
Tests

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

::: toolkit/components/url-classifier/tests/mochitest/allowlistAnnotatedFrame.html
@@ +28,5 @@
> +      fetchFinished = true;
> +  }
> +  if (!onloadCalled || !xhrFinished || !fetchFinished) {
> +      return; // not every test has run yet
> +  }

Maybe split this function in two? One which does the work of the old one and another that handles the side effects/determines whether all steps have finished that you added now. Can also make the name a bit clearer then.

@@ +102,5 @@
> +oReq.addEventListener("load", reqListener);
> +oReq.addEventListener("error", transferFailed);
> +oReq.addEventListener("abort", transferCanceled);
> +oReq.open("GET", "http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/evil.js");
> + oReq.send();

nit: looks like spurious whitespace
Attachment #8692644 - Flags: review?(gpascutto) → review+
Attachment #8692642 - Flags: review?(gpascutto) → review+
Attachment #8692643 - Flags: review?(gpascutto) → review+
Depends on: 1229177
https://hg.mozilla.org/mozilla-central/rev/709a8aec7c79
https://hg.mozilla.org/mozilla-central/rev/2cdef2a8ed53
https://hg.mozilla.org/mozilla-central/rev/eadb3ad94e0f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Whiteboard: [adv-main45+]
Alias: CVE-2016-1978
Alias: CVE-2016-1978
Whiteboard: [adv-main45+] → [adv-main45-]
I don't see how this can be a "vulnerability" (as implied by a sec-low rating) since we need to keep people safe with or without Tracking protection. We don't even enable it by default! It's simply a bug: TP not working as well as we'd like.
Keywords: sec-lowsec-want
You need to log in before you can comment on or make changes to this bug.