1223111 - Assertion: ValidatePictureRect(t->mTexture->GetSize(), t->mPictureRect) in CompositableTransactionParent.cpp

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

Assertion failure: ValidatePictureRect(t->mTexture->GetSize(), t->mPictureRect), at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/layers/ipc/CompositableTransactionParent.cpp:185

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.webm

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Milan - looks graphicsy.

Comment 3

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

Let's see if there is something obvious here.

Comment 4

[Daniel Veditz [:dveditz]]

Edwin: how bad (in a security sense) is this assertion? Will subsequent code do bad things to memory or just display things wrong?

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> Edwin: how bad (in a security sense) is this assertion? Will subsequent code
> do bad things to memory or just display things wrong?

It's not terribad, though it looks like it could end up letting you read memory past the buffer.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

The width of the frame size from the container (33x16) is larger than the width of the actual frame returned by libvpx (8x16). We're interpreting the container frame size as the crop rect, which doesn't appear to be correct.

This is handled differently by everybody. Firefox 42 seems to happily overrun the buffer, Chrome stretches the image to fit the frame, and GStreamer renders 8x16 and then repeats the last column to fill the frame which is... interesting.

The most reasonable thing to do here is to stretch to the container-reported frame size.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1223111.patch

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8691924 [details] [diff] [review]
1223111.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Somewhat easily, for somebody reasonably familiar with WebM.

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?
42 and up.

If not all supported branches, which bug introduced the flaw?

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

How likely is this patch to cause regressions; how much testing does it need?
Regressions unlikely. Existing tests are sufficient.

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> The width of the frame size from the container (33x16) is larger than the
> width of the actual frame returned by libvpx (8x16). We're interpreting the
> container frame size as the crop rect, which doesn't appear to be correct.

this is exactly what container size is supposed to be:
the size at which it is displayed.

I need to have a better think of this. but as it is, this patch appears to me more a work around than the proper fix, which would be to display the frame as it's supposed to be.

Basically any modifications to TrackInfo that is only being used for a particular decoder indicate that this is not the right approach as TrackInfo is supposed to be content agnostic.

Comment 11

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

This needs a security rating before it can get sec-approval (and before you know if sec-approval even applies).

Based on comment 5, this sounds like a sec-moderate since it just reads a bit after the end. Is that right?

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8691924 [details] [diff] [review]
1223111.patch

Review of attachment 8691924 [details] [diff] [review]:
-----------------------------------------------------------------

r- in the premise that it would fail in a similar fashion when using the ffvp8/ffvp9 decoder (through ffmpeg) and isn't limited to the webm container.

Additionally, knowing that Chrome or VLC don't properly handle margins they can't be used as a reference.

I will have a look for another approach, but on a first look, this approach isn't a good one; more a quick workaround.

Comment 13

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

Comment on attachment 8691924 [details] [diff] [review]
1223111.patch

clearing sec-approval? until we have an r+ patch (and a security rating).

Comment 14

[Jean-Yves Avenard [:jya]]

This is similar to the problem we are attempting to solve for bug 1232045.

Once bug 1232045 is solved, so will this one.

Comment 15

[Jean-Yves Avenard [:jya]]

I don't think we need a security rating, it's just a DoS due to a crappy file causing an assertion

Comment 16

[Ryan VanderMeulen [:RyanVM]]

So this is fixed now?

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

This looks solved to me, not seeing any issue with playing back the attached file.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Should we land this test as a crashtest or are the ones from bug 1232045 sufficient?

Comment 19

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I think the one from the other bug is sufficient.