968833 - (PReinterpretCast) PLayerTransaction protocol allows reinterpret_casting between layer types based on untrusted IPC message parameters

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Here is my current theory of what is happening.

In LayerTransactionParent::RecvUpdate, http://hg.mozilla.org/mozilla-central/file/82991c4302e0/gfx/layers/ipc/LayerTransactionParent.cpp#l213 :

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

      nsRefPtr<ThebesLayerComposite> layer =
        layer_manager()->CreateThebesLayerComposite();
      AsLayerComposite(edit.get_OpCreateThebesLayer())->Bind(layer);
      break;
    }
    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.

http://hg.mozilla.org/mozilla-central/file/82991c4302e0/gfx/layers/ipc/LayerTransactionParent.cpp#l53

//--------------------------------------------------
// 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.

http://hg.mozilla.org/mozilla-central/file/82991c4302e0/gfx/layers/ipc/ShadowLayerParent.h#l30
http://hg.mozilla.org/mozilla-central/file/82991c4302e0/gfx/layers/ipc/ShadowLayerParent.cpp#l24

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:

http://hg.mozilla.org/mozilla-central/file/82991c4302e0/gfx/layers/ipc/LayerTransactionParent.cpp#l263

    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:

http://hg.mozilla.org/mozilla-central/file/82991c4302e0/gfx/layers/ipc/LayerTransactionParent.cpp#l296

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

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

        ThebesLayerComposite* thebesLayer =
          static_cast<ThebesLayerComposite*>(layer);
        const ThebesLayerAttributes& attrs =
          specific.get_ThebesLayerAttributes();

        thebesLayer->SetValidRegion(attrs.validRegion());

        break;
      }
      // ... 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 =
          static_cast<ThebesLayerComposite*>(layer);

And then we will call methods on it:

        thebesLayer->SetValidRegion(attrs.validRegion());

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.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

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'.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

I think that we should stop pretending to write software.

I think I'll go raise sheep in the mountains.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: No more PReinterpretCast

See above comments for an explanation of what this fixes.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 969549 about similar problems in the PCompositableTransaction protocol.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: ThebesLayerComposite is not TYPE_SHADOW

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.

Comment 9

[Andrew McCreight [:mccr8]]

Assigning to bjacob because he has a patch.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 11

[Andrew McCreight [:mccr8]]

Thanks for investigating and fixing all of these IPC problems, Benoit!  They are very scary.

Comment 12

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

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.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c342a70bdae4
http://hg.mozilla.org/integration/mozilla-inbound/rev/e43eaa875fa0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c342a70bdae4
https://hg.mozilla.org/mozilla-central/rev/e43eaa875fa0

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Benoit (salut), could you prepare uplifts for 28, 29 & esr24 (and probably b2g)? Merci

Comment 16

[Andrew McCreight [:mccr8]]

We probably only really care about backporting this to B2G branches, but feel free to disagree Benoit.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

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).

Comment 18

[Daniel Veditz [:dveditz]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

If we were going to backport these to 1.2 and 1.3, that time was a long time ago.

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

No verify for desktop Fx30.