Closed
Bug 1223111
Opened 9 years ago
Closed 8 years ago
Assertion: ValidatePictureRect(t->mTexture->GetSize(), t->mPictureRect) in CompositableTransactionParent.cpp
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: tsmith, Assigned: bryce)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, csectype-bounds, testcase)
Attachments
(3 files)
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
Reporter | ||
Comment 1•9 years ago
|
||
Milan - looks graphicsy.
Component: Audio/Video: Playback → Graphics
Flags: needinfo?(milan)
Let's see if there is something obvious here.
Assignee: nobody → edwin
Flags: needinfo?(milan)
Comment 4•9 years ago
|
||
Edwin: how bad (in a security sense) is this assertion? Will subsequent code do bad things to memory or just display things wrong?
Group: media-core-security → gfx-core-security
Flags: needinfo?(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.
Flags: needinfo?(edwin)
Reporter | ||
Updated•9 years ago
|
Keywords: csectype-bounds
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.
Attachment #8691924 -
Flags: review?(jyavenard)
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.
Attachment #8691924 -
Flags: sec-approval?
Comment 10•9 years ago
|
||
(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•9 years ago
|
||
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?
Flags: needinfo?(edwin)
Comment 12•9 years ago
|
||
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.
Attachment #8691924 -
Flags: review?(jyavenard) → review-
Comment 13•9 years ago
|
||
Comment on attachment 8691924 [details] [diff] [review] 1223111.patch clearing sec-approval? until we have an r+ patch (and a security rating).
Attachment #8691924 -
Flags: sec-approval?
Comment 14•8 years ago
|
||
This is similar to the problem we are attempting to solve for bug 1232045. Once bug 1232045 is solved, so will this one.
Assignee: edwin → bvandyk
Flags: needinfo?(edwin)
Comment 15•8 years ago
|
||
I don't think we need a security rating, it's just a DoS due to a crappy file causing an assertion
Reporter | ||
Updated•8 years ago
|
Group: gfx-core-security
Updated•8 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 17•8 years ago
|
||
This looks solved to me, not seeing any issue with playing back the attached file.
Comment 18•8 years ago
|
||
Should we land this test as a crashtest or are the ones from bug 1232045 sufficient?
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox45:
affected → ---
status-firefox48:
--- → fixed
Depends on: 1232045
Flags: needinfo?(jyavenard) → needinfo?(bvandyk)
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 19•8 years ago
|
||
I think the one from the other bug is sufficient.
Flags: needinfo?(bvandyk)
Updated•8 years ago
|
Flags: in-testsuite? → in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•