1767360 - (CVE-2022-3266) Crash in UpdateSubresource when playing H264

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: vid.mp4

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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."

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 4

[John Lin [:jhlin][:jolin]]

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

[Willy R. Vasquez]

Attachment: Smaller crashing example with H264 Syntax Elements

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

[Willy R. Vasquez]

Attachment: og_vid.zip

For completeness, I've also attached the original MP4 that produced the crash, along with .264 and syntax element log files.

Comment 7

[Daniel Veditz [:dveditz]]

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?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

In comment 3 I was referring to a read violation. i.e. the source buffer is smaller than the destination buffer.

Comment 9

[Hovav Shacham]

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

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 11

[Willy R. Vasquez]

Attachment: fuzz_results.zip

I've attached some Grizzly fuzzing results that cause ASAN read errors.

Comment 12

[Willy R. Vasquez]

Attachment: libyuv_crash.zip

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

[Ryan VanderMeulen [:RyanVM]]

Hi John, any updates on this bug?

Comment 14

[John Lin [:jhlin][:jolin]]

(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

[Dianna Smith [:diannaS]]

: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

[Jim Mathies [:jimm]]

John is still working on this, currently 105 or 106 for a target.

Comment 17

[Alastor Wu [:alwu]]

NI myself for checking this bug later.

Comment 18

[Alastor Wu [:alwu]]

Attachment: Bug 1767360 - use correct image size retrieved from the output type to create video frames buffer.

Comment 19

[Alastor Wu [:alwu]]

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

[Jim Mathies [:jimm]]

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.

Comment 21

[Alastor Wu [:alwu]]

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

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this ready to land?

Comment 24

[Alastor Wu [:alwu]]

Yes, it's ready.

Comment 25

[Alastor Wu [:alwu]]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 27

[Alastor Wu [:alwu]]

Working on investigating those failures, will update my patches again later. Keep NI.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdc3d95f5125

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/e9d94bf2aced

Comment 32

[Alexandru Trif, Desktop QA [:atrif]]

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.

Comment 33

[Jim Mathies [:jimm]]

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

[Ryan VanderMeulen [:RyanVM]]

That crash spike started before this patch landed and we haven't seen any crash spikes on Beta where this was also uplifted to.

Comment 35

[Daniel Veditz [:dveditz]]

This was from an external reporter originally, Jeff was just the bug-filer here. Should be eligible for a bug bounty.

Comment 36

[Frederik Braun [:freddy]]

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

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Attaching draft advisory text.