Closed Bug 968833 (PReinterpretCast) Opened 6 years ago Closed 6 years ago

PLayerTransaction protocol allows reinterpret_casting between layer types based on untrusted IPC message parameters


(Core :: Graphics: Layers, defect)

Not set



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


(Reporter: bjacob, Assigned: bjacob)



(Keywords: sec-critical, Whiteboard: [qa-])


(2 files)

Fuzzing Gfx IPC, various crash scenarios show a Layer object of a particular layer type being reinterpreted in-place as being of a different layer type, depending on untrusted IPC message parameters. More details to follow.
See the dependent bugs list now. I'm not sure about all of them, but some are very clearly falling into this bucket.

A simple one is bug 968204 where we are reinterpreting a CanvasLayer as a ColorLayer.

A nastier one is bug 968191 where a type mismatch has LayerTransactionParent::RecvUpdate calling ThebesLayerComposite::InvalidateRegion, which the source code doesn't. This bad function call is made possible by this function being virtual and us looking up the wrong vtable due to being confused about layer types.

A yet nastier case is bug 968168 where the reinterpreting of a layer into another layer type causes us to reinterprete untrusted integer values as a vptr, dereference it, and call virtual methods based on it. That is probably a sec-crit.
Here is my current theory of what is happening.

In LayerTransactionParent::RecvUpdate, :

    switch (edit.type()) {
    // Create* ops
    case Edit::TOpCreateThebesLayer: {
      MOZ_LAYERS_LOG(("[ParentSide] CreateThebesLayer"));

      nsRefPtr<ThebesLayerComposite> layer =
    case Edit::TOpCreateContainerLayer: {
    // ... snip ... there are cases for all layer types

Here edit is an untrusted message parameter. We are using it (specifically, its type()) to decide what kind of Layer to construct and bind to that PLayer. The C++ implementation for that PLayer actor here is ShadowLayerParent. We are getting to the ShadowLayerParent by calling AsLayerComposite on the PLayer, and then we call ShadowLayerParent::Bind.

// Convenience accessors
static ShadowLayerParent*
cast(const PLayerParent* in)
  return const_cast<ShadowLayerParent*>(
    static_cast<const ShadowLayerParent*>(in));

// ... snip ...

template<class OpCreateT>
static ShadowLayerParent*
AsLayerComposite(const OpCreateT& op)
  return cast(op.layerParent());

So the AsLayerComposite(edit.get_OpCreateThebesLayer()) expression is just getting at the ShadowLayerParent object from the edit; that part is not the problem.

But then we call Bind() on it.

The effect of that is to make this ShadowLayerParent reference specifically the ThebesLayerComposite that we just created. That's where the untrusted IPC parameter, edit.type(), determines what type of layer is actually referenced by this PLayer actor.

LayerTransactionParent::RecvUpdate is subsequently called again with new Edit's which may reference the same PLayer actor. We run again that same switch statement, where other cases will access the existing PLayer actor, and will interpreting it as of being of the type determined by the untrusted Edit, overruling the type of the existing Layer object already bound to the PLayer actor. For example, in the Edit::TOpSetLayerAttributes case:

    case Edit::TOpSetLayerAttributes: {
      MOZ_LAYERS_LOG(("[ParentSide] SetLayerAttributes"));

      const OpSetLayerAttributes& osla = edit.get_OpSetLayerAttributes();
      Layer* layer = AsLayerComposite(osla)->AsLayer();
      const LayerAttributes& attrs = osla.attrs();

This gets back the layer that we had previously bound to our PLayer actor, see above Bind() call. And then, this gets LayerAttributes from the untrusted Edit (osla.attrs()).

A few lines below, these two unrelated things (the trusted layer object, and the untrusted attrs) are used together like this:

      typedef SpecificLayerAttributes Specific;
      const SpecificLayerAttributes& specific = attrs.specific();
      switch (specific.type()) {
      case Specific::Tnull_t:

      case Specific::TThebesLayerAttributes: {
        MOZ_LAYERS_LOG(("[ParentSide]   thebes layer"));

        ThebesLayerComposite* thebesLayer =
        const ThebesLayerAttributes& attrs =


      // ... snip ... other layer types in other cases

So regardless of the actual layer type of our trusted layer, if the untrusted attrs tell us to interprete it as a ThebesLayer, we will:

        ThebesLayerComposite* thebesLayer =

And then we will call methods on it:


While this particular method call isn't virtual, others are. This is how we end up reinterpreting arbitrary, generally untrusted, layer data, as the vptr of a different layer type.
And by 'dynamically' you mean 'based on runtime information stored in the (already trusted) object being casted', as opposed to 'based on other runtime information such as untrusted message params'.
I think that we should stop pretending to write software.

I think I'll go raise sheep in the mountains.
Alias: PReinterpretCast
See above comments for an explanation of what this fixes.
Attachment #8372416 - Flags: review?(jmuizelaar)
Attachment #8372416 - Attachment is patch: true
Filed bug 969549 about similar problems in the PCompositableTransaction protocol.
Attachment #8372416 - Flags: review?(jmuizelaar) → review+
So the above patch broke basic page rendering, as all ThebesLayerComposite's got rejected by the compositor. Why was that? Because with the new check, introduced by that patch, that a Layer is really TYPE_THEBES before we cast it as a ThebesLayerComposite, suddently GetType()'s return value started to matter a lot... and we had a spurious line of code, introduced by a huge changeset on bug 856079, that made ThebesLayerComposite to be TYPE_SHADOW while all other *LayerComposite types were specific non-TYPE_SHADOW types. According to DXR, nothing else than ThebesLayerComposite was TYPE_SHADOW. Removing this line makes everything look fine locally for me.
Attachment #8373788 - Flags: review?(nical.bugzilla)
Attachment #8373788 - Flags: review?(nical.bugzilla) → review+
Assigning to bjacob because he has a patch.
Assignee: nobody → bjacob
Comment on attachment 8372416 [details] [diff] [review]
No more PReinterpretCast

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

Not easily. An attacker would first need to be quite versed in how our IPC system works, to infer how to craft IPC messages triggering bugs there. Then, they would have to find a suitable pair of layer types such that reinterpreting one layer into another layer type causes an arbitrary value to be reinterpreted as a pointer, to cause their arbitrary pointer to be reinterpreted as a vptr.

So while this is not trivial to infer, there is no question that the present bug is exploitable.

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

No such comments exist.

Which older supported branches are affected by this flaw?

All branches. Outside of B2G, this is less of a security concern, because there is only one process anyway, so if a client is compromised then all is already lost.

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

No backport. Should be easy to backport to recent branches, and medium-difficulty to backport to older supported branches. However, this is not a big deal on desktop, and B2G 1.3 is already near frozen, and this is a quite intrusive patch, so I am not sure that we'll want much backporting.

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

Medium risk. The patch is not doing anything clever, but it's not very small either, and this is a little-understood part of our codebase, so I can't be sure that there is no regression here.
Attachment #8372416 - Flags: sec-approval?
Thanks for investigating and fixing all of these IPC problems, Benoit!  They are very scary.
Comment on attachment 8372416 [details] [diff] [review]
No more PReinterpretCast

sec-approval+ for trunk.

I'll let Dan weigh in but I'm inclined to not backport unless we want it for B2G 1.3.
Attachment #8372416 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(dveditz)
Benoit (salut), could you prepare uplifts for 28, 29 & esr24 (and probably b2g)? Merci
We probably only really care about backporting this to B2G branches, but feel free to disagree Benoit.
Given that I am in the process of landing 25 different patches for the issues found by this gfx IPC fuzzing, I am very interested in minimizing the amount of backporting work.

The other thing is, outside of B2G, we're single-process anyway, so the security issue is not so bad or even concrete, while the destabilizing effect of landing nontrivial patches changing core code like this is real.

So I vote for not backporting outside of B2G branches (if even that).
We can call the status of Firefox (desktop) releases "unaffected" or "disabled" I think (disabled is probably better). wontfix makes it look like we're leaving a potential sec-critical in those releases.
Depends on: 987993
If we were going to backport these to 1.2 and 1.3, that time was a long time ago.
No verify for desktop Fx30.
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.