971678 - ThebesLayerComposite::SetCompositableHost accepts arbitrary CompositableHost* and downcasts them to ContentHost

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067

This is a big deal, because it's parent process code called by handlers for untrusted PLayerTransaction messages, and not checking this means that malicious clients can cause the parent process to reinterpret to ContentHost* arbitrary CompositableHost* pointers. So this is quite similar to bug 968833.

Comment 1

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

Attachment: Faulty/ASan session showing this triggered by a malicious IPC client

See this line in the Faulty log:

[Faulty] pickle field {int} of value: 9 changed to: 8

This is where a CompositableType is tweaked.

enum CompositableType
{
  BUFFER_UNKNOWN,
  // the deprecated compositable types
  BUFFER_IMAGE_SINGLE,    // image/canvas with a single texture, single buffered
  BUFFER_IMAGE_BUFFERED,  // canvas, double buffered
  BUFFER_BRIDGE,          // image bridge protocol
  BUFFER_CONTENT,         // thebes layer interface, single buffering
  BUFFER_CONTENT_DIRECT,  // thebes layer interface, double buffering
  BUFFER_CONTENT_INC,     // thebes layer interface, only sends incremental
                          // updates to a texture on the compositor side.
  BUFFER_TILED,           // tiled thebes layer
  // the new compositable types
  COMPOSITABLE_IMAGE,     // image with single buffering
  COMPOSITABLE_CONTENT_SINGLE,  // thebes layer interface, single buffering
  COMPOSITABLE_CONTENT_DOUBLE,  // thebes layer interface, double buffering
  BUFFER_COUNT
};

So 9 is COMPOSITABLE_CONTENT_SINGLE and 8 is COMPOSITABLE_IMAGE.

The next line in the log,

[Parent 25989] ###!!! ASSERTION: should be implemented or not used: 'Error', file /hack/mozilla-central/gfx/layers/composite/CompositableHost.h, line 138

Is CompositableHost::UpdateThebes. So we have a ImageHost here, which we created because the above CompositableType had been tweaked to COMPOSITABLE_IMAGE, and we are trying to use it as if it were a ContentHost for a ThebesLayer.

The crash, then, is that we call a ContentHost virtual method on this object, but since it's really a ImageHost with a smaller vtable, we end up addressing the ImageHost vtable out-of-bounds. This probably makes it a sec-critical, I suppose.

Comment 2

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

Attachment: Faulty session with a judicious assertion added, much more clear

This was obtained by adding a crashy assertion in ThebesLayerComposite::SetCompositableHost, checking that what we are about to downcast to ContentHost really is one. We hit this assertion after exactly the same kind of CompositableType tweaking from 9 to 8, thus proving that SetCompositableHost's static_cast was responsible for the bad type reinterpretation described above.

Comment 3

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

Attachment: validate the compositable type in SetCompositableHost

Comment 4

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

Comment on attachment 8374785 [details] [diff] [review]
validate the compositable type in SetCompositableHost

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

Not easily. To an attacker sufficiently versed in our codebase, the patch gives away that it is possible to cause unsafe memory accesses in the parent process by faking compositable ids. It remains to know how to fake these ids (not hard) and how to actually exploit these unsafe memory accesses (harder).

That said, the bug looks probably 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.

Which older supported branches are affected by this flaw?

All of them.

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

I don't have backports. Should be easy to write.

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

Not risky, but not 100% trivial either (non-canonical lists of compositable ids in these switch statements). I wouldn't rush to land this on b2g 1.3 if that's the question.

Comment 5

[Daniel Veditz [:dveditz]]

>> How likely is this patch to cause regressions; how much testing does it need?
> 
> Not risky, but not 100% trivial either (non-canonical lists of compositable ids in
> these switch statements). I wouldn't rush to land this on b2g 1.3 if that's the question.

That is, indeed, the question.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8374785 [details] [diff] [review]
validate the compositable type in SetCompositableHost

sec-approval=dveditz

Comment 7

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/11e06d77e4f9

Comment 8

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/11e06d77e4f9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

v1.1 will be EOL on Monday, so it seems highly unlikely that this is going to be uplifted there :)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

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

Marking [qa-] for desktop QA verification. FxOS QA may choose to verify at a later date.