1258238 - Verify that the layers ID sent to the parent process for visibility visualization is valid

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Kats pointed out in bug 1257315 comment 26:

> Drive-by comment: these patches send the layers id from the child process to
> the parent process, which is a security risk because a compromised child
> process can then start affecting other child processes' layers trees. This
> was mostly relevant to b2g but will be relevant for desktop some day when we
> have multiple child processes (or if you manually enable that behavior).
> Also it's not critical because this is debugging code behind a pref but if
> you do touch this code again please modify it so that the parent doesn't
> assume a layers id produced by a child is valid. In general the parent side
> should be able to either already know the layers id (if using PBrowser) or
> should be able to verify that the layers id exists in the process that is
> communicating with it.

Let's get this fixed.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Verify that the layers ID sent for visibility visualization is valid.

This patch makes us verify that the layers ID sent by the content process
appears in sIndirectLayerTrees. Kats, is this the kind of validation you were
looking for?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8738255 [details] [diff] [review]
Verify that the layers ID sent for visibility visualization is valid.

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

I don't think this is sufficient. This will check that the layers id being sent by the child maps to *some* layer tree, but it doesn't check that the layers id maps to a layer tree owned by that content process. In other words, a hijacked child could still send a valid layer tree id that belongs to some other process.

I think to do this properly we need to move the two API functions from PCompositorBridge to PLayerTransaction. AIUI, there can be multiple layer trees per process, and there is one PLayerTransaction per layer tree, while there is only one PCompositorBridge per process. PCompositorBridge doesn't really track which layer ids belong to which processes, it relies on the implicit separation afforded by the PLayerTransaction to do that. That is, each PLayerTransaction maps to a single layer tree, and the LayerTransactionParent->GetId() provides a trustworthy source for the layers id of that layer tree. Any messages that come over the PLayerTransaction can be safely assumed to be belonging to that layer tree and corresponding to that layers id.

So I think what you need to do is:
1) Move your two functions from PCompositorBridge to PLayerTransaction
2) Update the sending code (at [1], for example) to send over the PLayerTransaction. You can do this by taking the layer manager, and calling layerManager->AsShadowForwarder()->GetShadowManager()->SendClearApproximatelyVisibleRegions, with appropriate null guards and whatnot.
3) Move the receiving code which you currently have in CompositorBridgeParent to LayerTransactionParent. It looks like on the compositor side your code is mostly just passing stuff to the compositor layer manager, which can be accessed via the |mLayerManager| field of the LayerTransactionParent.
4) Verify the layers id. The LayerTransactionParent has a GetId() function which returns the layers id of that layer tree. If you're sending over a ScrollableLayerGuid, you should check that the layers id in that guid matches GetId(). Alternatively, you could stop sending over the ScrollableLayerGuid and just send the presShellId/viewID bits of it, and then use GetId() on the parent side to obtain the layers id and construct the ScrollableLayerGuid there. The former approach is probably simpler given the way the code is structured now.

Does that make sense?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=f1ee7dcaa1cb#5699

Comment 3

[Seth Fowler [:seth] [:s2h]]

Yeah, that makes sense. Thanks for the detailed explanation! I'll write a patch to do that, though I may have to land some other stuff first since otherwise I'll end up creating a terrible rebase for myself.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Any plans to finish this off?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I'm going to delete this code. It's a debugging tool that's unmaintained and while it doesn't appear to be an actively-exploitable problem, it's a disaster waiting to happen.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60a4c5bd24ab8db10a2811c6212076b1465257ec

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

I typo'd the bug number for the patch, and it ended up on bug 1259238 instead. Matt r+'d it there (bug 1259238 comment 4). I'm reuploading it here, carrying r+ from mattwoodrow.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e389cffd5132
Remove code that displays visibility visualization on the APZ minimap. r=mattwoodrow

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e389cffd5132