1414282 - LayerTransactionParent::RecvUpdate - Arbitrary gfx::ScaledFont Object Pointer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement, P1)

Description

[Stephen Fewer]

As part of a code review I am conducting for Paul Theriault against the sandbox, the following vulnerability has been discovered.

The LayerTransactionParent operates in either the UI or GPU process and receives IPDL messages from the child (either the content process or UI process). A compromised content process may send a malicious IPDL message to call LayerTransactionParent::RecvUpdate and provide an arbitrary pointer value which is then cast into an object pointer of type gfx::ScaledFont. The attacker could point the object into a buffer they control (perhaps a shared memory buffer) in order to construct a fake gfx::ScaledFont object in order to generate an exploit primitive, such as gaining EIP control if the fake object had a vftable based call performed on it.

The method LayerTransactionParent::RecvUpdate receives a TransactionInfo object from the child describing the update to perform. First a series of Edit objects are processed, allowing for Layers to be created, before later operating on these layers. If an attacker forces an Edit::TOpCreateTextLayer edit type to be performed, a TextLayer is created. This TextLayer can then be operated on in the later call to SetLayerAttributes.

> mozilla::ipc::IPCResult
> LayerTransactionParent::RecvUpdate(const TransactionInfo& aInfo)
> {
> 
>   // ...snip...
> 
>   for (EditArray::index_type i = 0; i < aInfo.cset().Length(); ++i) { // <-- process edits as supplied by child in aInfo.
>     const Edit& edit = const_cast<Edit&>(aInfo.cset()[i]);
> 
>     switch (edit.type()) {
>     
>     // ...snip...
>     
>     case Edit::TOpCreateTextLayer: {
>       MOZ_LAYERS_LOG(("[ParentSide] CreateTextLayer"));
> 
>       RefPtr<TextLayer> layer = mLayerManager->CreateTextLayer(); // <-- child can instruct parent to create a TextLayer.
>       if (!BindLayer(layer, edit.get_OpCreateTextLayer())) {
>         return IPC_FAIL_NO_REASON(this);
>       }
> 
>       UpdateHitTestingTree(layer, "CreateTextLayer");
>       break;
>     }
>     
>     // ...snip...
>     
>   }
> 
>   // ...snip...
>   
>   // Process attribute updates.
>   for (const auto& op : aInfo.setAttrs()) {
>     MOZ_LAYERS_LOG(("[ParentSide] SetLayerAttributes"));
>     if (!SetLayerAttributes(op)) { // <-- now we can operate on the TextLayer created above.
>       return IPC_FAIL_NO_REASON(this);
>     }
>   }

In SetLayerAttributes a previously created TextLayer can be modified if the child specifies a Specific::TTextLayerAttributes operation. This lets several properties of the TextLayer be set, including a pointer to a gfx::ScaledFont object. However this pointer comes from the child process as a uintptr_t value and is simply cast via reinterpret_cast to a gfx::ScaledFont object pointer. This is a security violation as arbitrary pointers like this should not be passesed across sand-boxed process.

> bool
> LayerTransactionParent::SetLayerAttributes(const OpSetLayerAttributes& aOp)
> {
>   Layer* layer = AsLayer(aOp.layer()); // <-- pull out the Layer object from the remote handle.
>   if (!layer) {
>     return false;
>   }
>   
>   const LayerAttributes& attrs = aOp.attrs(); // <-- get attributes supplied by child.
>   
>   // ...snip...
>    
>   const SpecificLayerAttributes& specific = attrs.specific();
>   switch (specific.type()) {
>   
>   // ...snip...
>   
>   case Specific::TTextLayerAttributes: {
>     MOZ_LAYERS_LOG(("[ParentSide]   text layer"));
> 
>     TextLayer* textLayer = layer->AsTextLayer();
>     if (!textLayer) {
>       return false;
>     }
>     const auto& tla = specific.get_TextLayerAttributes(); // <-- get TextLayerAttributes supplied by child.
>     textLayer->SetBounds(tla.bounds());
>     textLayer->SetGlyphs(Move(const_cast<nsTArray<GlyphArray>&>(tla.glyphs())));
>     textLayer->SetScaledFont(reinterpret_cast<gfx::ScaledFont*>(tla.scaledFont())); // <-- cast a uintptr_t supplied by child into a gfx::ScaledFont object pointer!
>     break;
>   }
  
There seem to be multiple types of LayerManagers (BasicLayerManager, ClientLayerManager, LayerManagerComposite and LayerManagerMLGPU) which are responsible for creating the layers for the parent (The call to "mLayerManager->CreateTextLayer()" in RecvUpdate), however LayerManagerMLGPU currently does not implement CreateTextLayer as we can see below.

> already_AddRefed<TextLayer>
> LayerManagerMLGPU::CreateTextLayer()
> {
>   MOZ_ASSERT_UNREACHABLE("Not yet implemented");
>   return nullptr;
> }

I assume LayerManagerMLGPU is for the GPU process and as such this issue may *currently* not be exploitable if the GPU process is used, as the attacker can never create the TextLayer for which to operate on during SetLayerAttributes. I am unclear where the other layer managers are used. This issue would benefit from somebody more familiar with this subsystem in order to triage the impact better.

I would recommend that the processing of Specific::TTextLayerAttributes be prevented if the child in the IPDL message is a different process from the parent in order to avoid a malicious child process ever being able to reach this code path.

Comment 1

[Stephen Fewer]

Blocks: 1041862

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Milan, can someone from your team take a look at this?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Lee, what's your take on this?  I'm not clear on what kind of options we have with scaled fonts here

Comment 4

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Lee, what's your take on this?  I'm not clear on what kind of options we
> have with scaled fonts here

Since Matt Woodrow implemented this, he would be the best person to offer insight here on why he made this design choice.

Comment 5

[Matt Woodrow (:mattwoodrow)]

This was all experimental code that was never meant to work in e10s anyway.

I guess it's possible for a compromised child process to send messages as if they were enabled though.

We have bug 1406231 for deleting this code, we should prioritize that.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Offending code has been deleted on m-c.