974356 - MemoryTexture's do not validate that the client is same-process

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

MemoryTexture's are like ShmemTexture's, but sharing a plain pointer. This of course only makes sense in the case of same-process clients, but this isn't validated at the moment. As a result, untrusted client processes can tell the parent process to dereference arbitrary pointers, leading to incorrect rendering, crashes, and a sec-vector that in conjunction with another flaw exposing the compositor's output, would allow an attacker to read data illegally from the parent process.

Comment 1

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

Attachment: Part 1: add an IsSameProcess method to ISurfaceAllocator

Question 1: In IPDL, who knows whether the link is same-process?

Answer: The actors do, and expose it via their OtherProcess() method, which returns 0 in the case of a same-process link.

Question 2: Can we call OtherProcess() during actor construction?

Answer: No, since OtherProcess() needs member data that is only set after the AllocPFoo method has returned.

Question 3: OK, so we need to call OtherProcess() on an actor that is already constructed before we start constructing PTexture actors, and that is accessible from there. What would that be?

Answer: PTexture actors know about an ISurfaceAllocator, which knows about a PLayerTransaction or PImageBridge actor, which is constructed earlier.


---> so this patch adds a IsSameProcess() method to ISurfaceAllocator, getting information from the PLayerTransaction or PImageBridge actor.

Comment 2

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

Attachment: Part 2: in IPDL actors, initialize the mOtherProcess field to zero

As a matter of fact, current widget code is constructing PCompositorParent's by hand, without going through the normal IPC dance (create child, call SendConstructor method). The resulting PCompositorParent then has an uninitialized mOtherProcess field = 0xa5a5a5a5a5...

Comment 3

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

Attachment: Part 3: Make TextureHost::CreateIPDLActor check for bad MemoryTextures

Comment 4

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

Attachment: Part 4: (bonus) make CreateIPDLActor error out if TextureParent::Init() failed

I'm not certain that we strictly need this, but this seemed like the right thing to do.

Comment 5

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

Comment on attachment 8378255 [details] [diff] [review]
Part 1: add an IsSameProcess method to ISurfaceAllocator

*** Note: I'm writing only one sec-approval request for all 4 patches here. The patch actually closing the security issue here is part 3. ***

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

The present bug is not AFAICS exploitable by itself. By itself, it's only allowing a client to crash the parent process, i.e. a mere DOS. But maybe the more worrying aspect of this bug is as a vector of exploiting other bugs. This bug allows making the compositor use untrusted memory buffers as sources i.e. it could be used to feed sensitive data into the compositor, and then exploit another bug to get at the compositor output, to read that sensitive data.

The present patch makes it quite clear how to use the vector (how to feed bad data to the compositor) but does not contain a hint about any other bug that this vector could be used on, to gain access to sensitive data.

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?

All of them.

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

No backport, and would be medium complexity to write.

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

Medium risk. We're talking about backporting 3 patches here, that's not nothing.

Comment 6

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

Comment on attachment 8378257 [details] [diff] [review]
Part 2: in IPDL actors, initialize the mOtherProcess field to zero

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

::: ipc/ipdl/ipdl/lower.py
@@ +2884,5 @@
>                               [ ExprVar.THIS ]) ]),
>                  ExprMemberInit(p.lastActorIdVar(),
>                                 [ p.actorIdInit(self.side) ]),
> +                ExprMemberInit(p.otherProcessVar(),
> +                               [ ExprLiteral.ZERO ]),

Hm, 0 isn't really correct. We need to use -1 on POSIX, INVALID_HANDLE_VALUE on Windows. Something like http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/platform_file.h#19

Comment 7

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

Comment on attachment 8378255 [details] [diff] [review]
Part 1: add an IsSameProcess method to ISurfaceAllocator

sec-approval+ for trunk.

We should get Aurora, Beta, and ESR24 patches for this after it goes into mozilla-central.

Comment 8

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

Attachment: Part 1: in IPDL actors, initialize the mOtherProcess field to an invalid handle value

Comment 9

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

Attachment: Part 2: add an IsSameProcess method to ISurfaceAllocator (r=nical)

Comment 10

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

Comment on attachment 8378258 [details] [diff] [review]
Part 3: Make TextureHost::CreateIPDLActor check for bad MemoryTextures

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

The patch gives away that MemoryTexture, which is essentially a same-process-IPC protocol to share plain pointers, doesn't check that the client really is same-process. In the parent process, MemoryTextures are used only by the compositor. So this patch gives away that an attacker can cause the compositor to use arbitrary pointers instead of the buffers that it should be reading from. However, that isn't an exploit by itself, because the output of the compositor is not exposed to attackers unless we have another flaw that would be a sec-high by itself. So the present bug is more like a vector to (considerably) worsen the impact of other sec-high's.

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?

All of them.

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

No, and I don't think we should worry about backporting select IPC fuzzing fixes like this until we've fixed the majority of our IPC security problems.

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

Medium risk.

Comment 11

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

https://tbpl.mozilla.org/?tree=Try&rev=ddadef151af2

Comment 12

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

Comment on attachment 8378258 [details] [diff] [review]
Part 3: Make TextureHost::CreateIPDLActor check for bad MemoryTextures

sec-approval+ for trunk.

Comment 13

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/490d3e8706c3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a5fe646f27
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20c507b7615e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/321b2a16f0d7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/490d3e8706c3
https://hg.mozilla.org/mozilla-central/rev/b4a5fe646f27
https://hg.mozilla.org/mozilla-central/rev/20c507b7615e
https://hg.mozilla.org/mozilla-central/rev/321b2a16f0d7

Comment 15

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

esr24- based on comment 10 and Andrew marking 24 as disabled.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

If we were going to backport these to 1.2 and 1.3, that time was a long time ago.

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] w/r/t verification, no test case.