IPC: heap-buffer-overflow crash [@mozilla::layers::TileHost::ReadUnlock]

RESOLVED FIXED in Firefox 34, Firefox OS v1.4

Status

()

--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: posidron, Assigned: nical)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla35
x86_64
Mac OS X
crash, csectype-bounds, sec-critical, testcase
Points:
---

Firefox Tracking Flags

(firefox32 disabled, firefox33+ disabled, firefox34+ fixed, firefox35 fixed, firefox-esr31 disabled, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8495170 [details]
faulty-ReadUnlock.txt

This happened while opening http://mozilla.github.io/webrtc-landing/pc_test.html in a build with '--enable-ipc-fuzzer' enabled.
(Reporter)

Comment 1

4 years ago
This crash is very persistent and block fuzzing of GMP.
Assignee: nobody → nical.bugzilla
Whiteboard: [fuzzblocker]
(Assignee)

Comment 2

4 years ago
Created attachment 8496890 [details] [diff] [review]
Check that tile locks are backed by shmems when running OOP
Attachment #8496890 - Flags: review?(bjacob)
Comment on attachment 8496890 [details] [diff] [review]
Check that tile locks are backed by shmems when running OOP

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +81,5 @@
> +            // the cross-process case is a bad security violation.
> +            NS_ERROR("A client process may be trying to peek at the host's address space!");
> +            // This tells the TiledContentHost that deserialization failed so that
> +            // it can propagate the error.
> +            mUninitialized = true;

I don't like the name mUninitialized. It contains a negation ("un-"). It's not specific enough about what went on here.

Generally speaking, in a project that doesn't use exceptions (such as us), there is no good way to report errors from the constructor, so I understand that you have to resort to that hack, but at least please find name that's more specific and that's free of negation.

Or, what would be better: consider if it might be possible to just MOZ_CRASH here and instead fix callers to avoid this code path if !isSameProcess.
Attachment #8496890 - Flags: review?(bjacob) → review-
In fact, since NS_ERROR records a test failure on TBPL, I suppose that by definition you _can_ MOZ_CRASH here, right?   (The only alternative is we're relying on not having tests!)
(Assignee)

Comment 5

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #4)
> In fact, since NS_ERROR records a test failure on TBPL, I suppose that by
> definition you _can_ MOZ_CRASH here, right?   (The only alternative is we're
> relying on not having tests!)

I assume we should no be rebooting the phone if this happens (we are in the compositor process), and kill the child process rather than the parent. If crashing the Compositor process is good enough, that simplifies everything (but should we do this?).
(Assignee)

Comment 6

4 years ago
Created attachment 8497441 [details] [diff] [review]
v2

renamed mUninitialized into !mIsValid for clarity. mValid is explicitly set to fals in the falsy code path (lets me keep the comment that explains that mIsValid lets the caller know that deserialization failed).
Attachment #8497441 - Flags: review?(bjacob)
(Assignee)

Updated

4 years ago
Attachment #8496890 - Attachment is obsolete: true
Comment on attachment 8497441 [details] [diff] [review]
v2

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

r+ but please comment on:

::: gfx/layers/composite/TiledContentHost.cpp
@@ +29,5 @@
>  
>  TiledLayerBufferComposite::TiledLayerBufferComposite()
>    : mFrameResolution(1.0)
>    , mHasDoubleBufferedTiles(false)
> +  , mIsValid(false)

So a TiledLayerBufferComposite created by this default constructor will never ever be valid? Why, then, do we need this constructor?
Attachment #8497441 - Flags: review?(bjacob) → review+
(Assignee)

Comment 8

4 years ago
(In reply to Benoit Jacob [:bjacob] from comment #7)
> Comment on attachment 8497441 [details] [diff] [review]
> v2
> 
> Review of attachment 8497441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but please comment on:
> 
> ::: gfx/layers/composite/TiledContentHost.cpp
> @@ +29,5 @@
> >  
> >  TiledLayerBufferComposite::TiledLayerBufferComposite()
> >    : mFrameResolution(1.0)
> >    , mHasDoubleBufferedTiles(false)
> > +  , mIsValid(false)
> 
> So a TiledLayerBufferComposite created by this default constructor will
> never ever be valid? Why, then, do we need this constructor?

TiledLayerBufferComposite are allocated directly in their TiledContentHost like this:
mTiledBuffer = TiledLayerBufferComposite(...); // no "new"

The default constructor is needed because we don't always have a TiledLayerBuffer (at least not until the first OpUseTiledLayerBuffer message is received). Then the slot is reassigned using the other constructor. So the memory in which we assign the defult constructor will be valid eventually when we reassign with the other constructor.
Keywords: sec-critical
https://hg.mozilla.org/mozilla-central/rev/2d6e7c9887e5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox35: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Does this impact ESR?
Flags: needinfo?(dveditz)
(Assignee)

Comment 12

4 years ago
(In reply to Benjamin Kerensa [:bkerensa] from comment #11)
> Does this impact ESR?

It doesn't. It impacts aurora and beta though.
(Assignee)

Comment 13

4 years ago
(In reply to Nicolas Silva [:nical] from comment #12)
> (In reply to Benjamin Kerensa [:bkerensa] from comment #11)
> > Does this impact ESR?
> 
> It doesn't. It impacts aurora and beta though.

Oh wait, I confused this with another bug. But this only affects b2g and e10s so I think that ESR is not concerned.
Nicolas: security bug fixes need to request sec-approval? before landing
https://wiki.mozilla.org/Security/Bug_Approval_Process

Please retroactively request approval and fill out the template so we can figure out where we need to port this fix.
status-b2g-v1.4: --- → ?
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox32: --- → disabled
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr31: --- → disabled
tracking-firefox33: --- → +
tracking-firefox34: --- → +
Flags: needinfo?(dveditz) → needinfo?(nical.bugzilla)
https://hg.mozilla.org/releases/mozilla-aurora/rev/8664164db6aa
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/442693c6354d
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ea144d59c01e

Calling this disabled for Beta33 based on comment 13 since we don't ship a B2G release off it and e10s is still trunk-only for now. Feel free to override me if you feel otherwise, though :)
status-b2g-v1.4: ? → fixed
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-firefox33: affected → disabled
status-firefox34: affected → fixed
(Assignee)

Comment 16

4 years ago
Comment on attachment 8497441 [details] [diff] [review]
v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Hard, I guess. This patch prevents an already compromised process from sending raw pointers to the browser process. I can't remotely imagine web content being able to do this, but someone who could manipulate directly what goes in the IPC (such as the fuzzer) could produce this issue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

b2g since tiling landed, so I think 2.0.

If not all supported branches, which bug introduced the flaw?

Tiling on b2g: Bug 963073

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch should apply or be easy to port to older branch if needed.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #8497441 - Flags: sec-approval?
Flags: needinfo?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #16)
> Which older supported branches are affected by this flaw?
> 
> b2g since tiling landed, so I think 2.0.
> 
> If not all supported branches, which bug introduced the flaw?
> 
> Tiling on b2g: Bug 963073

b2g30 = v1.4

> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> 
> The patch should apply or be easy to port to older branch if needed.

Already done, see comment 15 :)
Comment on attachment 8497441 [details] [diff] [review]
v2

After the fact, sec-approval+.
Attachment #8497441 - Flags: sec-approval? → sec-approval+
CC'ing julienw & azasypkin (owner/peer of Gaia SMS app) who are investigating a rendering regression in the SMS app, with a regression range that strongly points to this bug.  (Discussion of this regression should go on the new bug, but I figured they'd benefit from being in the loop here as well, if indeed this does turn out to be the cause.)
Thanks. I'll do a bisect tomorrow and I'll know better.
Group: core-security
Keywords: csectype-bounds
You need to log in before you can comment on or make changes to this bug.