1303713 - Array out-of-bounds memory read/write/exec in CamerasParent

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Hector [:tedd] [:jhector]]

When the parent receives the message 'GetCaptureCapability' from the PCameras protocol it is possible to trigger an array out-of-bounds using the |engine| parameter.

A compromised child process (web content process) can escalate it's privileges using this because the object retrieved from the array on the parent side contains function pointer which are then later used. This makes getting code execution on the parent side much easier. The object is also used for read/write access.

The array in question is called mEngines [1]. RecvGetCaptureCapability [2] is the entry point where the malicious data from the child is handled.

> if (self->EnsureInitialized(aCapEngine)) {
> error = self->mEngines[aCapEngine].mPtrViECapture->GetCaptureCapability(
>   unique_id.get(), MediaEngineSource::kMaxUniqueIdLength, num, webrtcCaps);
> }

|aCapEngine| is a user controlled integer. Once EnsureInitialized() returns true, |aCapEngine| is used to call a function which can be seen in the above snippet.

EnsureInitialized() [3] doesn't really validate the value of |aCapEngine|, all that is required here is that the value of |aCapEngine| passes the SetupEngine() call (as seen below)

> CaptureEngine capEngine = static_cast<CaptureEngine>(aEngine);
> if (!SetupEngine(capEngine)) {

the static_cast above, does not bind the aEngine value to the size of the |CaptureEngine| enum, so |capEngine| can be any integer.

SetupEngine() [4] gets a |EngineHelper| object of the |mEngines| array and checks if it is already initialized, if so it returns (and therefore the "check" is bypassed, see below)

> EngineHelper *helper = &mEngines[aCapEngine];
>
> // Already initialized
> if (helper->mEngine) {
>   return true;
> }

The array access 
> EngineHelper *helper = &mEngines[aCapEngine];

looks like this in the assembled code (x64 Linux)

> 0x7fab9c07ed71 <CamerasParent::SetupEngine(...)+93>:	movsxd rax,r14d
> 0x7fab9c07ed74 <CamerasParent::SetupEngine(...)+96>:	imul   rax,rax,0x58
> 0x7fab9c07ed78 <CamerasParent::SetupEngine(...)+100>:	lea    r13,[rbx+rax*1+0x40]

with r14d being the value retrieved from the child.

By carefully crafting the right value, a fake EngineHelper object (fully/partially controlled by an attacker) can be accessed. Given that |capEngine| is a 32 bit integer, the possible access range is big.

I debugged this, by using the gum test [5] and setting breakpoints in the child and the parent. Before the child sent the message over to the parent, I modified it to be 0xdeadbeef and verified that r14d in the above snippet is indeed r14d.

With sandboxing being in the works, this would lead to a sandbox escape.


[1] https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/dom/media/systemservices/CamerasParent.h#148
[2] https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/dom/media/systemservices/CamerasParent.cpp#589
[3] https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/dom/media/systemservices/CamerasParent.cpp#494
[4] https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/dom/media/systemservices/CamerasParent.cpp#356
[5] https://mozilla.github.io/webrtc-landing/gum_test.html

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Pass-enum-type.diff

Feel free to forward to another reviewer if you're too busy.

Comment 2

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8792542 [details] [diff] [review]
Pass-enum-type.diff

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

Thanks for the quick fix.

I don't know if I am allowed to give a r+ for this code, but that is the kind of mitigation that I was thinking about, because the ContiguousEnumSerializer will enforce the enum boundaries once the object is being read on the receiving side.
I give you the r+ (if I am not allowed please request someone else), but you will probably also need to request sec-approval since it is a security bug.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8792542 [details] [diff] [review]
Pass-enum-type.diff

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

-> It will be fairly obvious what the patch protects against, and the type of bug is generally quite exploitable. However, it's a sandbox escape, so it requires that you already have control over the content process.

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

-> No, the code does though.

Which older supported branches are affected by this flaw?

-> Bug was introduced in Firefox 43, so all of them. It's only a security issue in e10s + sandboxing releases, though, so we should only care about branches that will release that.

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

-> Bug 1104616.

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

-> Assuming ContiguousEnumSerializer exists it will apply directly.

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

-> Unlikely to cause regressions.

Comment 4

[Tyson Smith [:tsmith]]

Paul: How are we rating this class of issue? sec-moderate?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

"The vulnerability combined with another moderate vulnerability could result in an attack of high or critical severity (aka stepping stone)."

Seems to fit the sec-moderate description.

When we roll out sandboxing we'll have to consider whether we want to treat sandboxing escapes specifically. I've argued to jimm that we should make them eligible for bounties (though we already have a loophole that allows some sec-moderates to qualify).

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Agree with gcp here - both on the rating of sec-moderate, and the policy on paying for _some_ sandbox escapes. On the bounty question, I think can see the logic for rewarding bugs which further our knowledge of the sandboxing attack surface, but I don't think we are really at a stage where a sandbox bypass should be an automatic qualification for a bounty. I'd like to see a clear criteria for which ones we considered bounty-worthy (both for our sake, and for fairness to researchers).

Comment 7

[Tyson Smith [:tsmith]]

gcp, paul, thanks for the feedback.

Comment 8

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

Comment on attachment 8792542 [details] [diff] [review]
Pass-enum-type.diff

As a moderate rated security issue, this doesn't need sec-approval to land on trunk. It can just go in.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/145c594a51c549012c621caacd1b24a0541d68e9
Bug 1303713. r=tedd

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/145c594a51c5

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Reassigning as high to more accurately reflect this issue today.