Conditional jump/move depends on uninitialized data in Quartz backend

RESOLVED FIXED in Firefox 19

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: milan)

Tracking

({sec-high, valgrind})

unspecified
mozilla21
x86
Mac OS X
sec-high, valgrind
Points:
---

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ fixed, firefox20+ fixed, firefox21+ fixed, firefox-esr1719+ fixed, b2g18+ wontfix, b2g18-v1.0.0 wontfix)

Details

(Whiteboard: [adv-main19+][adv-esr1703+], URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Launching Firefox on 10.7 inside VMWare, pointing at the URL mentioned here, gives me a slew of warnings of use of uninitialized data, starting with this warning. I suspect that we're using the Quartz backend to draw a screenshot, so it might help to have an empty profile (so Firefox wants to draw a screenshot of the page you're on).

==45780== Conditional jump or move depends on uninitialised value(s)
==45780==    at 0x9F61430: mozilla::gfx::DrawTargetCG::Init(mozilla::gfx::BackendType, unsigned char*, mozilla::gfx::IntSize const&, int, mozilla::gfx::SurfaceFormat) (DrawTargetCG.cpp:884)
==45780==    by 0x9F5E587: mozilla::gfx::DrawTargetCG::Init(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat&) (DrawTargetCG.cpp:974)
==45780==    by 0x9F4A83B: mozilla::gfx::Factory::CreateDrawTarget(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Factory.cpp:193)
==45780==    by 0x9C82B52: gfxPlatform::CreateDrawTargetForBackend(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:728)
==45780==    by 0x9C82BDB: gfxPlatform::CreateOffscreenDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:736)
==45780==    by 0x9CB432F: mozilla::layers::LayerManager::CreateDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Layers.cpp:250)
==45780==    by 0x8A96905: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (CanvasRenderingContext2D.cpp:792)
==45780==    by 0x8A9772F: mozilla::dom::CanvasRenderingContext2D::Scale(double, double, mozilla::ErrorResult&) (CanvasRenderingContext2D.cpp:1905)
==45780==    by 0x9AE83E2: mozilla::dom::CanvasRenderingContext2DBinding::scale(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:116)
==45780==    by 0x9AEF0C0: mozilla::dom::CanvasRenderingContext2DBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:3244)
==45780==    by 0xA2D301B: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:364)
==45780==    by 0xA2CE73E: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:362)
==45780==  Uninitialised value was created by a heap allocation
==45780==    at 0x5237: malloc (in /usr/local/Cellar/valgrind/3.8.1/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==45780==    by 0x15D68D: operator new(unsigned long) (in /usr/lib/libstdc++.6.0.9.dylib)
==45780==    by 0x9F4A811: mozilla::gfx::Factory::CreateDrawTarget(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Factory.cpp:192)
==45780==    by 0x9C82B52: gfxPlatform::CreateDrawTargetForBackend(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:728)
==45780==    by 0x9C82BDB: gfxPlatform::CreateOffscreenDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:736)
==45780==    by 0x9CB432F: mozilla::layers::LayerManager::CreateDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Layers.cpp:250)
==45780==    by 0x8A96905: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (CanvasRenderingContext2D.cpp:792)
==45780==    by 0x8A9772F: mozilla::dom::CanvasRenderingContext2D::Scale(double, double, mozilla::ErrorResult&) (CanvasRenderingContext2D.cpp:1905)
==45780==    by 0x9AE83E2: mozilla::dom::CanvasRenderingContext2DBinding::scale(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:116)
==45780==    by 0x9AEF0C0: mozilla::dom::CanvasRenderingContext2DBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:3244)
==45780==    by 0xA2D301B: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:364)
==45780==    by 0xA2CE73E: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:362)
How are you running 10.7 inside VMware?
Keywords: sec-high
(Reporter)

Comment 2

5 years ago
Per VMWare: 

From Mac OS X 10.7 (Lion), Apple allows full virtualization of its operating system, provided that it is installed on Apple hardware which is also running OS X 10.7. 

<http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=2005334>

If you have a CD or disk image of OS X, you can just point Fusion at it, and it'll work.
(Assignee)

Comment 3

5 years ago
Created attachment 706618 [details] [diff] [review]
Doesn't look like mCg is guaranteed to be set on Init

I don't know if the "else" can ever happen, but the code doesn't seem to guarantee initializing mCg after all is said and done, and that 884 seems to be using mCg (in the December 4th version at least), so...
Attachment #706618 - Flags: feedback?(jmuizelaar)
(Assignee)

Comment 4

5 years ago
Created attachment 707685 [details] [diff] [review]
Initialize mCg in the constructor

It seems to be safer to just initialize mCg to nullptr in the constructor, but this could also be done in one of the else branches in the Init method (see feedback patch.)
Attachment #706618 - Attachment is obsolete: true
Attachment #706618 - Flags: feedback?(jmuizelaar)
Attachment #707685 - Flags: review?(jmuizelaar)
Comment on attachment 707685 [details] [diff] [review]
Initialize mCg in the constructor

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

Seems reasonable to me.
Attachment #707685 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 6

5 years ago
Comment on attachment 707685 [details] [diff] [review]
Initialize mCg in the constructor

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
We are not initializing the pointer variable, then potentially writing to that location.  This is a class member variable, however, so I don't think the risk is high.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The comment does say that we are now initializing the pointer in the constructor.  It does not say where that pointer would have been used uninitialized, but the code inspection could discover this location.

Which older supported branches are affected by this flaw?
See the bug below.

If not all supported branches, which bug introduced the flaw?
775226 - landed https://hg.mozilla.org/integration/mozilla-inbound/rev/ad87f9ac3e5d

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports until decision is made that they are needed, but they would be trivial.

How likely is this patch to cause regressions; how much testing does it need?
This is setting a pointer that should be null to null.  I do not anticipate real regressions, but it could unearth other bugs.
Attachment #707685 - Flags: sec-approval?
status-b2g18: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
tracking-firefox-esr17: --- → ?
Attachment #707685 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

5 years ago
Attachment #707685 - Flags: checkin?(jmuizelaar)
(Reporter)

Comment 7

5 years ago
Better to just set checkin-needed so anybody who's looking for something to do can check it in.
Keywords: checkin-needed
Attachment #707685 - Flags: checkin?(jmuizelaar)
*wanders on by*

https://hg.mozilla.org/integration/mozilla-inbound/rev/944d04257592
Keywords: checkin-needed
Assignee: jmuizelaar → milan
tracking-b2g18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: ? → +
tracking-firefox21: ? → +
tracking-firefox-esr17: ? → 19+
Tracking for branches, please nominate for uplift asap so we can get this into the second to last beta 19.
(Assignee)

Comment 10

5 years ago
Created attachment 708257 [details] [diff] [review]
Same patch, but for beta for initializing mCg to nullptr. r=me
Attachment #708257 - Flags: review+
Attachment #708257 - Flags: approval-mozilla-beta?
(Assignee)

Comment 11

5 years ago
Created attachment 708259 [details] [diff] [review]
Same patch, but for aurora for initializing mCg to nullptr. r=me

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 775226
User impact if declined: security crash
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): security crash
String or UUID changes made by this patch: none
Attachment #708259 - Flags: review+
Attachment #708259 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 12

5 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #9)
> Tracking for branches, please nominate for uplift asap so we can get this
> into the second to last beta 19.

Done, I think, but let me know if I didn't do the right thing.  Aurora=central, beta just has different line numbers, but same change.
Attachment #708257 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #708259 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 13

5 years ago
If somebody can check this into beta & aurora, it'd be great.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/944d04257592
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g18-v1.0.0: --- → affected
status-firefox18: affected → wontfix
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
https://hg.mozilla.org/releases/mozilla-aurora/rev/be4da114d75f
https://hg.mozilla.org/releases/mozilla-beta/rev/112c12834bd7

FWIW, for something as trivial as this, you probably don't need to worry about attaching branch-specific patches in the future :). Also, I assume you're going want to request esr17 and b2g18 uplift at some point too.
status-firefox19: affected → fixed
status-firefox20: affected → fixed
Keywords: checkin-needed
(In reply to Ryan VanderMeulen from comment #15)
> Also, I assume
> you're going want to request esr17 and b2g18 uplift at some point too.

For Mac-only (platform-specific) code, is b2g18 uplift relevant?
(Assignee)

Comment 17

5 years ago
I don't think b2g18 uplift is necessary "now", but I don't know how often that branch will update, and Jeff, I don't know if this code would ever kick in with some B2G configuration.
Flags: needinfo?(jmuizelaar)
(In reply to Milan Sreckovic [:milan] from comment #13)
> If somebody can check this into beta & aurora, it'd be great.

Milan, can you please get an esr patch ready and nominate it for uplift asap ? Thanks !
Yes this doesn't affect b2g at all.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 20

5 years ago
(In reply to bhavana bajaj [:bajaj] from comment #18)
> (In reply to Milan Sreckovic [:milan] from comment #13)
> > If somebody can check this into beta & aurora, it'd be great.
> 
> Milan, can you please get an esr patch ready and nominate it for uplift asap
> ? Thanks !

It's actually the same as the beta patch.
a=bajaj over IRC
https://hg.mozilla.org/releases/mozilla-esr17/rev/42d9eb844dbc
status-b2g18: affected → wontfix
status-b2g18-v1.0.0: affected → wontfix
status-firefox-esr17: affected → fixed

Comment 22

5 years ago
Joe, or anybody else with the necessary environment, can you please verify this fix?
Whiteboard: [adv-main19+][adv-esr1703+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.