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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: faulty-ReadUnlock.txt

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

Comment 1

[Christoph Diehl [:posidron]]

This crash is very persistent and block fuzzing of GMP.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Check that tile locks are backed by shmems when running OOP

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 5

[Nicolas Silva [:nical]]

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

Comment 6

[Nicolas Silva [:nical]]

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

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

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?

Comment 8

[Nicolas Silva [:nical]]

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

Comment 9

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6e7c9887e5

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/2d6e7c9887e5

Comment 11

[Benjamin Kerensa [:bkerensa]]

Does this impact ESR?

Comment 12

[Nicolas Silva [:nical]]

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

It doesn't. It impacts aurora and beta though.

Comment 13

[Nicolas Silva [:nical]]

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

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Nicolas Silva [:nical]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/442693c6354d

Comment 19

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

Comment on attachment 8497441 [details] [diff] [review]
v2

After the fact, sec-approval+.

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Julien Wajsberg [:julienw]]

Thanks. I'll do a bisect tomorrow and I'll know better.