Crash in UpdateSubresource when playing H264
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
People
(Reporter: jrmuizel, Assigned: alwu)
Details
(Keywords: csectype-bounds, reporter-external, sec-high, Whiteboard: [adv-main105+][adv-esr102.3+])
Crash Data
Attachments
(7 files)
1.32 KB,
video/mp4
|
Details | |
5.98 KB,
application/x-zip-compressed
|
Details | |
34.00 KB,
application/x-zip-compressed
|
Details | |
110.51 KB,
application/x-zip-compressed
|
Details | |
2.77 KB,
application/x-zip-compressed
|
Details | |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr102+
tjr
:
sec-approval+
|
Details | Review |
178 bytes,
text/plain
|
Details |
https://crash-stats.mozilla.org/report/index/7dd89fc9-df31-4fc0-ab38-2c1f80220502
We're trying to upload 496x1040 from a buffer that's not that big.
Reporter | ||
Comment 1•3 years ago
|
||
Reporter | ||
Comment 2•3 years ago
|
||
From the reporter: "The first SPS sets a frame size of 496x1040 while the second SPS sets it to 496x32. The final output frame is the size of the first, but only drawn up to the second size. The rest of the output seems to be leaked memory."
Reporter | ||
Comment 3•3 years ago
|
||
I need to try playing the video a couple of times to reproduce the crash.
When I was looking at this it seemed like the IMFMediaBuffer was not large enough to fit the full 496x1040 frame.
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Comment 4•3 years ago
|
||
MP4Box -info
shows different frame size:
...
Visual Sample Entry Info: width=144 height=82 (depth=24 bits)
Visual Track layout: x=0 y=0 width=144 height=82
MPEG-4 Config: Visual Stream - ObjectTypeIndication 0x21
AVC/H264 Video - Visual Size 144 x 82
AVC Info: 2 SPS - 2 PPS - Profile Extended @ Level 2
...
For further investigation, I'll add some more logs to the demuxer and H.264 media change monitor.
Comment 5•3 years ago
|
||
I've added as an attachment a zip file containing (1) an MP4 that should lead to a crash with less refreshes; (2) the H.264 file in Annex B format contained within the MP4; (3) the list of syntax element values of the H.264 file.
Comment 6•3 years ago
|
||
For completeness, I've also attached the original MP4 that produced the crash, along with .264 and syntax element log files.
Comment 7•3 years ago
|
||
comment 0 sounds like a read access violation, but comment 3 sounds like a write violation which is potentially more exploitable.
Have you tried running this in an ASAN Nightly build?
Reporter | ||
Comment 8•3 years ago
|
||
In comment 3 I was referring to a read violation. i.e. the source buffer is smaller than the destination buffer.
Comment 9•3 years ago
|
||
Y'all, please do also see the e-mail thread that preceded this bug for context. It is plausible that the codepaths triggered by example video can be used only for DoS and memory disclosure, but I don't know that out-of-bounds writes are out of the question.
Comment 10•3 years ago
|
||
The severity field is not set for this bug.
:jimm, could you have a look please?
For more information, please visit auto_nag documentation.
![]() |
||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 11•3 years ago
|
||
I've attached some Grizzly fuzzing results that cause ASAN read errors.
Comment 12•3 years ago
|
||
When outputting the video output to a canvas we get the following crash in libyuv: https://crash-stats.mozilla.org/report/index/6ee36e4d-2b70-4237-bc6f-a5f830220601
The attached HTML file causes this crash.
Comment 13•3 years ago
|
||
Hi John, any updates on this bug?
Comment 14•3 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
Hi John, any updates on this bug?
Sorry for the delay. I'm fixing an error in my patches and should be able to upload them for review later this week.
Comment 15•3 years ago
|
||
:Jolin we only have 2 betas left in the 104 cycle, do you think this would get fixed in time or is going to ride the 105 train?
![]() |
||
Comment 16•2 years ago
|
||
John is still working on this, currently 105 or 106 for a target.
Updated•2 years ago
|
Assignee | ||
Comment 18•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 19•2 years ago
|
||
Here I take the video in the comment 5 as an example to explain what happens.
From the SPS in the comment 5, the initial SPS indicates that the video has size [112, 416]. In the same bytestream, there is another SPS indicating the size would change to [326, 56] later. (check pic_width_in_mbs_minus1
and pic_height_in_map_units_minus1
and see how to calculate the width and height here)
However, the first sample media foundation returns to us is already not fitting the initial SPS. All decoded video frames are in in the sizing of [326, 56]. But we still construct the buffer by using the wrong size [112, 415]. That is the reason of causing out of bound access.
Here are an explanation for the solution.
By using MF_MT_FRAME_SIZE
, we can know the size of decoded frame, but that value might include the padding. Therefore, we use GetPictureRegion()
to get the region that should be displayed without padding. Then we can use the correct size to setup the buffer to avoid the crash.
![]() |
||
Comment 20•2 years ago
|
||
Summary from John -
The bug is triggered by carefully crafted MP4 files. In those files, the metadata (frame width & height, to be specific) is manipulated to disagree with the actual encoded data. In our current implementation, the initial configuration of the video decoder relies on the metadata and the width and height info are saved for creating the video images used for rendering. If it doesn't match the actual decoded data, there could be buffer overrun in the GPU or RDD utility processes.
To avoid the overrun, some checks must be performed when the decoder returns output buffers. Instead of using the saved width and height values, the output images should use the width and height of the decoder output buffers. That's what my first patch does.
While I was investigating the issue, I also found that the current implementation doesn't support files that have more than one parameter sets in one metadata segment. (It took me a while to find the spec and understand the parts that are involved) To fix this, 'H264ChangeMoniter' must check for multiple SPS/PPS. Although the parser is already capable of parsing a series of parameter sets in the bitstream, we never actually check that after the metadata is converted into 'avcC' blob (called extra data in our code). My second patch (p2) adds some checking in the monitor code and extends the parser to support that.
Strictly speaking, the second patch is not mandatory to fix the crash, but it helps in making sure that we use the correct metadata for each video frame and should reduce the chances that the video is rendered incorrectly.
Assignee | ||
Comment 21•2 years ago
|
||
Comment on attachment 9289353 [details]
Bug 1767360 - use correct image size retrieved from the output type to create video frames buffer.
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Not easy, because this would only affect a small size (width or height <= 132) of H264 videos which have multiple different SPS and change its format during playback. Also, this is Windows only issue.
- 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?: Not sure what is the oldest version, but at least from 100
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: Yes
- If not, how different, hard to create, and risky will they be?: Current patch should be able to backport to old branches as well
- How likely is this patch to cause regressions; how much testing does it need?: This patch introduces a new check to detect the display size for decoded video frame. The possible risk of causing regression is for all software decoded H264 videos on Windows.
The estimated risk would be low.
In term of the check we added, that is performed by Windows Media Foundation API, which should be trustworthy because it performs the decoding task as well, which should be able to return correct size.
In term of possible files being affected, as it would only affect those software decoded H264 videos which has changed the format during the playback, that seems less common. Otherwise, we should get more crash volumes.
- Is Android affected?: No
Comment 22•2 years ago
|
||
Comment on attachment 9289353 [details]
Bug 1767360 - use correct image size retrieved from the output type to create video frames buffer.
Approved to land and uplift
Updated•2 years ago
|
Assignee | ||
Comment 25•2 years ago
|
||
Comment on attachment 9289353 [details]
Bug 1767360 - use correct image size retrieved from the output type to create video frames buffer.
Beta/Release Uplift Approval Request
- User impact if declined: crash
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This patch introduces a new check to detect the display size for decoded video frame, and the estimated risk would be low.
In term of the check we added, that is performed by Windows Media Foundation API, which should be trustworthy because it performs the decoding task as well, which should be able to return correct size.
In term of possible files being affected, as it would only affect those software decoded H264 videos which has changed the format during the playback, that seems less common. Otherwise, we should get more crash volumes.
- String changes made/needed:
- Is Android affected?: Yes
![]() |
||
Comment 26•2 years ago
|
||
This landed: https://hg.mozilla.org/integration/autoland/rev/b1648cab08f03ae6a0d7e0d3a92c909b9b1d6124
Backed out for causing bc failures in browser_resizeVideo.js:
https://hg.mozilla.org/integration/autoland/rev/050a7125f954b037329f27705c8e19489bd15e7d
Push with failures
Failure log
mda failures
wpt failure 1, wpt failure 2
Assignee | ||
Comment 27•2 years ago
|
||
Working on investigating those failures, will update my patches again later. Keep NI.
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
![]() |
||
Comment 28•2 years ago
|
||
use correct image size retrieved from the output type to create video frames buffer. r=media-playback-reviewers,jolin
https://hg.mozilla.org/integration/autoland/rev/1ce65df85abc69150fcecdbd9e6315a1f4359881
https://hg.mozilla.org/mozilla-central/rev/1ce65df85abc
Comment 29•2 years ago
|
||
Comment on attachment 9289353 [details]
Bug 1767360 - use correct image size retrieved from the output type to create video frames buffer.
Approved for 105.0b6 and 102.3esr.
Comment 30•2 years ago
|
||
uplift |
Comment 31•2 years ago
|
||
uplift |
Updated•2 years ago
|
Updated•2 years ago
|
Comment 32•2 years ago
•
|
||
Hello! I managed to reproduce the silent crash stated in comment 0 (https://crash-stats.mozilla.org/report/index/a185024c-980b-4e43-afd8-526a20220905) with Firefox 102.0a1 (20220502213947) and Firefox 104.0 (https://crash-stats.mozilla.org/report/index/ff1b63a4-afb5-4e7a-b83e-cf2d70220905) on Windows 10x64 after loading the attached test case and videos or after refreshing the pages multiple times. Also, loading the test case and videos will display a glitched video preview.
However, I could also see https://crash-stats.mozilla.org/report/index/cdb8ab10-53de-452b-8ab2-26d470220905 crashes in the about:crashes page after refreshing the pages or changing the seekbar.
I can no longer see any of the above crashes in about:crashes on Windows 10x64 or Windows 7x64 after loading the attached videos and test case and refreshing the pages multiple times or changing the seekbar with Firefox 106.0a1 (20220904213226), 105.0b7 (20220904185841) and 102.3.0esr (20220901164226). The glitches are no longer displayed as well on the fixed versions.
Updated•2 years ago
|
![]() |
||
Comment 33•2 years ago
•
|
||
Of note, this might be the cause of a nighty crasher - Bug 1788174 - Crash in [@ BaseThreadInitThunk]
Update - looks like that top crasher started on 8-30, this landed 8-31.
Comment 34•2 years ago
|
||
That crash spike started before this patch landed and we haven't seen any crash spikes on Beta where this was also uplifted to.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 35•2 years ago
|
||
This was from an external reporter originally, Jeff was just the bug-filer here. Should be eligible for a bug bounty.
Comment 36•2 years ago
|
||
Apologies for omitting the real author credit. Instead of rolling this up in our work to find and fix memory hazards, we usually announce the bugs individually when reported externally. Let's use CVE-2022-3266. Advisory fixes will follow later in the day.
Comment 37•2 years ago
|
||
Attaching draft advisory text.
Updated•2 years ago
|
Updated•2 years ago
|
Updated•8 months ago
|
Description
•