Closed
Bug 1303713
Opened 8 years ago
Closed 8 years ago
Array out-of-bounds memory read/write/exec in CamerasParent
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: tedd, Assigned: gcp)
References
Details
(Keywords: csectype-bounds, csectype-sandbox-escape, sec-high, Whiteboard: [post-critsmash-triage][adv-main52+])
Attachments
(1 file)
(deleted),
patch
|
tedd
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
Feel free to forward to another reviewer if you're too busy.
Assignee: nobody → gpascutto
Attachment #8792542 -
Flags: review?(julian.r.hector)
Reporter | ||
Comment 2•8 years ago
|
||
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.
Attachment #8792542 -
Flags: review?(julian.r.hector) → review+
Assignee | ||
Comment 3•8 years ago
|
||
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.
Attachment #8792542 -
Flags: sec-approval?
Comment 4•8 years ago
|
||
Paul: How are we rating this class of issue? sec-moderate?
Flags: needinfo?(ptheriault)
Assignee | ||
Comment 5•8 years ago
|
||
"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).
Updated•8 years ago
|
Group: core-security → media-core-security
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).
Flags: needinfo?(ptheriault)
Comment 8•8 years ago
|
||
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.
Attachment #8792542 -
Flags: sec-approval?
Assignee | ||
Comment 9•8 years ago
|
||
Comment 10•8 years ago
|
||
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Updated•7 years ago
|
Group: core-security-release
Updated•6 years ago
|
Keywords: csectype-bounds
You need to log in
before you can comment on or make changes to this bug.
Description
•