Closed Bug 1163109 (CVE-2015-2743) Opened 9 years ago Closed 9 years ago

Inline JPEG images fail to load

Categories

(Core :: DOM: Core & HTML, defect)

40 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox39 + fixed
firefox38.0.5 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox-esr31 39+ verified
firefox-esr38 39+ verified
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: Snuffleupagus, Assigned: bent.mozilla)

References

Details

(Keywords: csectype-priv-escalation, regression, sec-high, Whiteboard: [pdfjs-c-ff-integration][adv-main39+][adv-esr38.1+][adv-esr31.8+])

Attachments

(2 files, 1 obsolete file)

Starting with today's Nightly (exact version below), all inline JPEG images fail to load.
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0 ID:20150508030204 CSet: 86203ac87a08

STR:
Open https://bug1065245.bugzilla.mozilla.org/attachment.cgi?id=8486973 in the latest Nightly.

ER:
The inline images should load. (Compare with the latest Aurora.)

AR:
The inline images does *not* load, and the following is printed in the browser console
> Security Error: Content at resource://pdf.js/web/viewer.html may not load data from blob:null/8b97441b-5c39-460c-906b-3d689dc8febf.
> Warning: Error during JPEG image loading pdf.js:235:5
> Security Error: Content at resource://pdf.js/web/viewer.html may not load data from blob:null/14f13ad1-6fe9-4e14-98fa-d4a8c48c58f6.
> Warning: Error during JPEG image loading pdf.js:235:5
> Security Error: Content at resource://pdf.js/web/viewer.html may not load data from blob:null/70576a77-ea12-4196-b87e-b1720a314075.
> Warning: Error during JPEG image loading pdf.js:235:5
> Security Error: Content at resource://pdf.js/web/viewer.html may not load data from blob:null/4d5a83f2-9371-4ce6-ae3e-3eac52f920cd.
> Warning: Error during JPEG image loading pdf.js:235:5
> Warning: Dependent image isn't ready yet pdf.js:235:5

Initial investigation suggests that bug 1156875 might be responsible.
10:50.98 LOG: MainThread Bisector INFO Last good revision: 81b2c07f94ef
10:50.98 LOG: MainThread Bisector INFO First bad revision: f15de6365040
10:50.98 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=81b2c07f94ef&tochange=f15de6365040
The first bad revision is:
changeset:   242750:522f3549ec5b
user:        Andrea Marchesini <amarchesini@mozilla.com>
date:        Thu May 07 08:06:24 2015 +0100
summary:     Bug 1156875 - patch 2 - Unify the registration of blob URIs in WorkerPrivate and nsIGlobalObject, r=bent


Andrea, do you have an idea why Bug 1156875 causing images not to be loaded by embedded PDF Viewer?
Flags: needinfo?(amarchesini)
The main page resource://test/test.html:

<script>
  var worker = new Worker('resource://test/worker.js');
  worker.onmessage = function (e) {
    var url = e.data;
    var img = document.createElement('img');
    img.src = url;
    document.body.appendChild(img);
  };
</script>

The worker code resource://test/worker.js:

var bin = new Uint8Array([0x89,0x50,... ,0x82]);
var blob = new Blob([bin], {type: 'image/png'});
var url = URL.createObjectURL(blob);
self.postMessage(url);
Assignee: ydelendik → nobody
Component: PDF Viewer → DOM: Core & HTML
Product: Firefox → Core
Presumably this is due to this change:

https://hg.mozilla.org/integration/mozilla-inbound/rev/522f3549ec5b#l3.44 which changed the principal that gets used for the URL from the principal of the worker's owning window to the principal of the worker.  Since resource:// URLs are non-hierarchical, they have principals which are not serializable... and presumably something goes wrong (because I'd think we would do the CheckLoadURI check on the principal of the worker, not its serialization).  I wonder what the principal of the worker actually is, in this case.
[Tracking Requested - why for this release]: Breaks URL API use in workers from chrome.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
> serialization).  I wonder what the principal of the worker actually is, in
> this case.

The worker is using the system principal. The page a 'normal' content principal. This makes
nsScriptSecurityManager::CheckLoadURIWithPrincipal to fail here:

https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#691
Attached patch p.patch (obsolete) — — Splinter Review
Attachment #8604052 - Flags: review?(bugs)
Whoa.  How did a non-system-principal page end up running a system-principal worker?  That seems pretty fishy...
Indeed. How do we end up having system principal on worker? Could we add some assertions to catch this kind of errors?
The patch doesn't change the type of principal the worker actually has, it just changes which
principal is used for various URL* checks.
Could you explain what kind of setup we have here?
Flags: needinfo?(amarchesini)
And if worker is using system principal, don't we still end up using that principal in case of
SharedWorker, ChromeWorker or ServiceWorker?
https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1004

This comes from here. For resource:// we force the system principal.
Flags: needinfo?(amarchesini)
Uh, that's not good.  See bug 1144991 for an example of the sort of problem you can get when resource:// can escalate to full system privileges.  Can someone please explain to me why this is not a security hole?
Flags: needinfo?(bent.mozilla)
Marking s-s for now.
Group: core-security
Dunno, we've handled workers from resource:// this way since 2011... Added in bug 615153.
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #14)
> Dunno, we've handled workers from resource:// this way since 2011... Added
> in bug 615153.

Resource is supposed to be unprivileged - I can't believe we've been shipping this.
Whiteboard: [pdfjs-c-ff-integration]
Ok, talked with bz about this and I think we're in agreement that we need to tweak the scriptloader to do this:

  if (loadPrincipal is system) {
    if (channelPrincipal is not system) {
      if (finalURI is not resource) {
        return NS_ERROR_DOM_BAD_URI;
      } // else resource, allow
    } // else channelPrincipal is system, allow
  } else {
    // CheckMayLoad here
  }

Basically we need to *never* upgrade the principal, we just need to allow resource:// urls loaded by the system principal.
So some more historical background...

Workers have this restriction that system code is not allowed to load a non-system-principled worker.  This was added in bug 615153.

That restriction, if strictly enforced, causes JSMs to not be able to load workers from resource://.  So before making that check, the worker code upgrades resource:// to system.  this was added in the same patch in bug 615153.  Ben and I suspect it was only added because otherwise things broke badly due to that first check.

So the plan is to do the minimal thing here: explicitly allow system loaders to load workers that have system principal _or_ are loaded from resource:// URIs.  And stop messing with the principal of the resulting worker at all.
> And stop messing with the principal of the resulting worker at all.

Ben pointed out that this would break XHR to random endpoints from a resource:// worker loaded from a JSM.

So I guess for now we'll just upgrade resource:// workers _only_ when they're loaded from system.  But then we should look into moving people who need system-principal workers to using chrome:// instead of resource:// or something.

Why do we load JSMs from resource:// again?  :(
Attached patch Patch, v1 — — Splinter Review
Attachment #8604254 - Flags: review?(bzbarsky)
Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

r=me, but we should verify that this fixes the issue this bug was actually filed on.  ;)

And please file a followup to remove the "channelPrincipal = loadPrincipal" line, as we discussed.
Attachment #8604254 - Flags: review?(bzbarsky) → review+
I think attachment 8604254 [details] [diff] [review] fixes the testcase. Yury, can you confirm?
Flags: needinfo?(ydelendik)
(In reply to Not doing reviews right now from comment #20)
> And please file a followup to remove the "channelPrincipal = loadPrincipal"
> line, as we discussed.

Bug 1163753.
Attachment #8604052 - Attachment is obsolete: true
Attachment #8604052 - Flags: review?(bugs)
The attachment 8604254 [details] [diff] [review] fixes test case and PDF Viewer (issue from comment 0).
Flags: needinfo?(ydelendik)
Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Easily.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes.
Which older supported branches are affected by this flaw?
Everything since 2011!
If not all supported branches, which bug introduced the flaw?
bug 615153
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same patch should work everywhere
How likely is this patch to cause regressions; how much testing does it need?
Worker test coverage is decent, and I don't see anything broken. But it's possible that this will break some addons if they're doing dumb stuff.
Attachment #8604254 - Flags: sec-approval?
Sec-approval+ for checkin on 5/26, two weeks into next cycle.
Attachment #8604254 - Flags: sec-approval? → sec-approval+
Assignee: amarchesini → bent.mozilla
Should this be sec-moderate like bug 1144991, or is it more severe and it should be sec-high (or maybe sec-critical)?
Flags: needinfo?(bzbarsky)
Comment 12 is private: false
Ben and I talked this over yesterday, and we _think_ that given the current APIS exposed to workers the only thing that would break today is that a worker that shouldn't have it would get the ability to do XHR to arbitrary places.  And even then, it's a worker loaded from resource://.  So probably sec-moderate or maybe sec-high, but not sec-critical.
Flags: needinfo?(bzbarsky)
We are start getting report similar to this one (e.g. bug 1164199) as related to PDF Viewer functionality. How we shall handle those: mark them as a dup of this one, as depends on this one or bug 1156875, or just start dup them on the e.g. bug 1164199?
If we land this patch under the bug number of the non-hidden bug 1164199 and don't mention this bug in the check-in (we can make 1164199 "depend on" this bug and/or add a hidden comment explaining that) then maybe it's OK to land sooner?
How's this plan?
 - check in to 40/41 now under public bug 1164199
 - wait a couple of weeks to land on 39, ESR-38, and ESR-31
Flags: needinfo?(lmandel)
The plan in comment 31 wfm.

bent - Can you please get an uplift approval request on this bug?
Flags: needinfo?(lmandel) → needinfo?(bent.mozilla)
Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

Approval Request Comment
[Feature/regressing bug #]: Bug 615153
[User impact if declined]: Moderate risk security hole that could allow cross-site xhr
[Describe test coverage new/current, TreeHerder]: I'm not aware of any current tests that exercise this new code path
[Risks and why]: It's possible that addons could be affected here.
[String/UUID change made/needed]: None
Flags: needinfo?(bent.mozilla)
Attachment #8604254 - Flags: approval-mozilla-aurora?
Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

Approved for uplift to aurora; the plan in comment 31 sounds good to me. We can probably get this into 39 beta 2 next Friday if everything looks ok on nightly & aurora.
Attachment #8604254 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm confused - Ben is on PTO until June 8. Who's handling getting this landed this week?
Boris, could please you handle landing this this week, given that bent is on PTO?  Thanks.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/integration/mozilla-inbound/rev/13fda43e7e95

Do I need to land this on branches too, or will sheriffs handle that?
Flags: needinfo?(bzbarsky) → needinfo?(continuation)
(In reply to Not doing reviews right now from comment #37)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/13fda43e7e95
Thanks!

> Do I need to land this on branches too, or will sheriffs handle that?
RyanVM will land stuff on branches (that is has approval+ for, of course).
Flags: needinfo?(continuation)
https://hg.mozilla.org/mozilla-central/rev/13fda43e7e95
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

See comment 33.
Attachment #8604254 - Flags: approval-mozilla-esr38?
Attachment #8604254 - Flags: approval-mozilla-esr31?
Attachment #8604254 - Flags: approval-mozilla-beta?
Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

On second thought after some discussion with Sylvestre, this is sec-high so let's take it on beta and esr now. It can make it into beta 2 this way. 
Approved for uplift to beta, esr31, esr38.
Attachment #8604254 - Flags: approval-mozilla-esr38?
Attachment #8604254 - Flags: approval-mozilla-esr38+
Attachment #8604254 - Flags: approval-mozilla-esr31?
Attachment #8604254 - Flags: approval-mozilla-esr31+
Attachment #8604254 - Flags: approval-mozilla-beta?
Attachment #8604254 - Flags: approval-mozilla-beta+
This isn't applying nicely to esr31. Worth getting a branch patch?
Flags: needinfo?(bent.mozilla)
Whiteboard: [pdfjs-c-ff-integration] → [pdfjs-c-ff-integration][adv-main39+][adv-esr38.1+][adv-esr31.8+]
Alias: CVE-2015-2743
Verified as fixed using Firefox 38.1esr under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5 with the pdf from comment 0 and Yury's testcase.

Yury's testcase is not compatible with Firefox 31.8esr. The sample pdf is not entirely loaded (loading indicator remains on the screen) and the images are missing: https://goo.gl/W9GTCz. There are no errors in browser's console. The pdf from bug 1164199 and from it's duplicates seems to be loaded fine in 31.8esr.
Marking as affected (based also on comment 44).
(In reply to Petruta Rasa [QA] [:petruta] from comment #48)
> Verified as fixed using Firefox 38.1esr under Win 7 64-bit, Ubuntu 12.04
> 32-bit and Mac OS X 10.9.5 with the pdf from comment 0 and Yury's testcase.
...
> The sample pdf is not entirely loaded (loading indicator remains on the screen) and the images
> are missing: https://goo.gl/W9GTCz. There are no errors in browser's
> console.

The sample pdf bug is reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1065245 and fixed in FF35, and it's out of the scope of this bug.
(In reply to Yury Delendik (:yury) from comment #49)
> The sample pdf bug is reported at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1065245 and fixed in FF35, and
> it's out of the scope of this bug.

Thanks Yury! Based on this and the pdf's verified from bug 1164199 and from it's duplicates, I mark this as verified for 31.8esr.
To avoid confusion with PDF.js and based on attachment 8603568 [details], removing [PDFViewer] from the bug title. See also bug 1180271.
Summary: [PDFViewer] Inline JPEG images fail to load → Inline JPEG images fail to load
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.