Closed Bug 971678 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- disabled
firefox29 --- disabled
firefox30 + fixed
firefox-esr24 --- unaffected
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files)

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.
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.
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.
Attachment #8374785 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8374785 - Flags: sec-approval?
>> 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.
Assignee: nobody → bjacob
Comment on attachment 8374785 [details] [diff] [review]
validate the compositable type in SetCompositableHost

sec-approval=dveditz
Attachment #8374785 - Flags: sec-approval? → sec-approval+
landed on central https://hg.mozilla.org/mozilla-central/rev/11e06d77e4f9
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
v1.1 will be EOL on Monday, so it seems highly unlikely that this is going to be uplifted there :)
If we were going to backport these to 1.2 and 1.3, that time was a long time ago.
Marking [qa-] for desktop QA verification. FxOS QA may choose to verify at a later date.
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.