1163109 - (CVE-2015-2743) Inline JPEG images fail to load

Status: RESOLVED FIXED

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

Description

[Jonas Jenwald [:Snuffleupagus]]

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.

Comment 1

[Yury Delendik (:yury)]

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

Comment 2

[Yury Delendik (:yury)]

The first bad revision is:
changeset:   242750:522f3549ec5b
user:        Andrea Marchesini <[e-mail]>
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?

Comment 3

[Yury Delendik (:yury)]

Attachment: Smaller test case (unsigned extension, xpi)

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);

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Breaks URL API use in workers from chrome.

Comment 6

[Andrea Marchesini [:baku]]

> 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

Comment 7

[Andrea Marchesini [:baku]]

Attachment: p.patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Whoa.  How did a non-system-principal page end up running a system-principal worker?  That seems pretty fishy...

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

And if worker is using system principal, don't we still end up using that principal in case of
SharedWorker, ChromeWorker or ServiceWorker?

Comment 11

[Andrea Marchesini [:baku]]

https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1004

This comes from here. For resource:// we force the system principal.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Bobby Holley (:bholley)]

Marking s-s for now.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Dunno, we've handled workers from resource:// this way since 2011... Added in bug 615153.

Comment 15

[Bobby Holley (:bholley)]

(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.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> 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?  :(

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Comment 20

[Boris Zbarsky [: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.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think attachment 8604254 [details] [diff] [review] fixes the testcase. Yury, can you confirm?

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 23

[Yury Delendik (:yury)]

The attachment 8604254 [details] [diff] [review] fixes test case and PDF Viewer (issue from comment 0).

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on 5/26, two weeks into next cycle.

Comment 26

[Andrew McCreight [:mccr8]]

Should this be sec-moderate like bug 1144991, or is it more severe and it should be sec-high (or maybe sec-critical)?

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Andrew McCreight [:mccr8]]

Thanks.

Comment 29

[Yury Delendik (:yury)]

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?

Comment 30

[Daniel Veditz [:dveditz]]

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?

Comment 31

[Daniel Veditz [:dveditz]]

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

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

The plan in comment 31 wfm.

bent - Can you please get an uplift approval request on this bug?

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

I'm confused - Ben is on PTO until June 8. Who's handling getting this landed this week?

Comment 36

[Andrew McCreight [:mccr8]]

Boris, could please you handle landing this this week, given that bent is on PTO?  Thanks.

Comment 37

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13fda43e7e95

Do I need to land this on branches too, or will sheriffs handle that?

Comment 38

[Andrew McCreight [:mccr8]]

(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).

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/13fda43e7e95

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e5f126660072

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8604254 [details] [diff] [review]
Patch, v1

See comment 33.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f49be454944b

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This isn't applying nicely to esr31. Worth getting a branch patch?

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/338671f8601f

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/337be23e0bb5

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d23e117039f3
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/d23e117039f3
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/73515564757c
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/73515564757c
https://hg.mozilla.org/releases/mozilla-esr31/rev/088d28723fcd

Comment 48

[Petruta Horea [:phorea], Desktop Test Engineering]

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).

Comment 49

[Yury Delendik (:yury)]

(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.

Comment 50

[Petruta Horea [:phorea], Desktop Test Engineering]

(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.

Comment 51

[Yury Delendik (:yury)]

To avoid confusion with PDF.js and based on attachment 8603568 [details], removing [PDFViewer] from the bug title. See also bug 1180271.