1916476 - Crash in [@ memcpy_repmovs | mozilla::gmp::GMPPlaneImpl::Copy]

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

Crash report: https://crash-stats.mozilla.org/report/index/7fe7cef5-0179-4f4c-9957-d434a0240901

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  VCRUNTIME140.dll  memcpy_repmovs()  D:\a\_work\1\s\src\vctools\crt\vcruntime\src\string\amd64\memcpy.asm:50
1  xul.dll  mozilla::gmp::GMPPlaneImpl::Copy(int, int, unsigned char const*)  dom/media/gmp/GMPVideoPlaneImpl.cpp:143
1  xul.dll  mozilla::gmp::GMPVideoi420FrameImpl::CreateFrame(int, unsigned char const*, i...  dom/media/gmp/GMPVideoi420FrameImpl.cpp:194
2  gmpopenh264.dll  OpenH264VideoDecoder::Decode_m(GMPVideoEncodedFrame*, TagBufferInfo*, unsigne...  \\?\Z:\task_168263428371319\workspace\openh264\module\gmp-openh264.cpp:1095
3  gmpopenh264.dll  gmp_args_m_5<OpenH264VideoDecoder*, void (OpenH264VideoDecoder::*)(GMPVideoEn...  \\?\Z:\task_168263428371319\workspace\openh264\module\task_utils_generated.h:493
4  xul.dll  mozilla::gmp::GMPSyncRunnable::Run()  dom/media/gmp/GMPPlatform.cpp:79
5  xul.dll  mozilla::detail::RunnableMethodArguments<>::apply<nsITargetShutdownTask, void...  xpcom/threads/nsThreadUtils.h:1085
5  xul.dll  std::invoke(mozilla::detail::RunnableMethodArguments<>::apply<nsITargetShutdo...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/type_traits:1729
5  xul.dll  std::_Apply_impl(mozilla::detail::RunnableMethodArguments<>::apply<nsITargetS...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/tuple:1077
5  xul.dll  std::apply(mozilla::detail::RunnableMethodArguments<>::apply<nsITargetShutdow...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/tuple:1088

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Crashes from each of the signatures, some quite old:
https://crash-stats.mozilla.org/report/index/7fe7cef5-0179-4f4c-9957-d434a0240901
https://crash-stats.mozilla.org/report/index/a84c00de-e7d7-4dca-a2ec-7f3590240516
https://crash-stats.mozilla.org/report/index/44f79962-3e83-47c7-a3b7-ca9290240721

This led me to realize we do not validate the buffer size (aSize_y, aSize_u and aSize_v) is sufficient for the given strides:
https://searchfox.org/mozilla-central/rev/aee7c3a0dbf33af0c4f6648f391db62b35895e50/dom/media/gmp/GMPVideoi420FrameImpl.cpp#182

I'm not sure if it will fix the crashes, but obviously we shouldn't trust the loaded plugin.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1916476.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9422292 [details]
Bug 1916476.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would depend on whether or not they could get the plugin into a state of providing a buffer that is too small. The only relevant input is encoded video frames, so presumably they would need to craft a video decoded by OpenH264 that is able to produce that result from it.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 4

[Tom Ritter [:tjr]]

Comment on attachment 9422292 [details]
Bug 1916476.

Approved to land and request uplift

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6d1d0decfa6
r=media-playback-reviewers,padenot

Comment 6

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:aosmond, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/b6d1d0decfa6

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9422292 [details]
Bug 1916476.

Beta/Release Uplift Approval Request

• User impact if declined: Sec issue, related crash in the wild
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): Just adds bounds checking.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec issue, related crash in the wild
• User impact if declined:
• Fix Landed on Version: 132
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adds bounds checking.

Comment 9

[Dianna Smith [:diannaS]]

Comment on attachment 9422292 [details]
Bug 1916476.

Approved for 131.0b3

Comment 10

[Pulsebot]

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

Comment 11

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:

Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All

At the time you answered that, NONE of the release status flags reflected that answer; they were all in the "---" unknown state. The ESR flags still are though release drivers updated the ones for the branches that got fixed.

The crash distribution here is odd. In the last 6 months there was an ESR 102 crash in May, an ESR 115 crash in July (bp-44f79962-3e83-47c7-a3b7-ca9290240721), and then in September a cluster of 131/132 nightly crashes. But no earlier nightly crashes? Is it just a super rare crash and coincidentally happened to affect only those two nightly users on the same weekend? Or did something change to make the crash more likely in recent versions?

I'm trying to figure out if we have to uplift this to the ESRs at this point. Super rare in the wild but does seem to affect them, and the structure of the fix points toward ways someone could craft content to try to trigger it intentionally.

Comment 12

[Dianna Smith [:diannaS]]

Comment on attachment 9422292 [details]
Bug 1916476.

Approved for 128.3esr

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/00735b7a093d

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9422292 [details]
Bug 1916476.

Approved for 115.16esr

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/1d420cca971d