Closed Bug 974356 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- disabled
firefox28 + disabled
firefox29 + disabled
firefox30 + fixed
firefox-esr24 - disabled
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [qa-])

Attachments

(4 files, 2 obsolete files)

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.
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.
Attachment #8378255 - Flags: review?(nical.bugzilla)
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...
Attachment #8378257 - Flags: review?(bent.mozilla)
I'm not certain that we strictly need this, but this seemed like the right thing to do.
Attachment #8378259 - Flags: review?(nical.bugzilla)
Attachment #8378255 - Flags: review?(nical.bugzilla) → review+
Attachment #8378258 - Flags: review?(nical.bugzilla) → review+
Attachment #8378259 - Flags: review?(nical.bugzilla) → review+
Assignee: nobody → bjacob
Assignee: bjacob → nobody
Assignee: nobody → bjacob
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.
Attachment #8378255 - Flags: sec-approval?
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
Attachment #8378257 - Flags: review?(bent.mozilla) → review-
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.
Attachment #8378255 - Flags: sec-approval? → sec-approval+
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.
Attachment #8378258 - Flags: sec-approval?
Attachment #8381042 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8378258 [details] [diff] [review]
Part 3: Make TextureHost::CreateIPDLActor check for bad MemoryTextures

sec-approval+ for trunk.
Attachment #8378258 - Flags: sec-approval? → sec-approval+
esr24- based on comment 10 and Andrew marking 24 as disabled.
If we were going to backport these to 1.2 and 1.3, that time was a long time ago.
Marking [qa-] w/r/t verification, no test case.
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.