1544181 - (CVE-2020-6822) Potential out-of-bounds write in GMPDecodeData

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Deian Stefan]

I believe there is a potential out-of-bounds write in GMPDecodeData [1]:

  GMPDecodeData(const webrtc::EncodedImage& aInputImage, bool aMissingFrames,
                int64_t aRenderTimeMs)
      : mImage(aInputImage),
        mMissingFrames(aMissingFrames),
        mRenderTimeMs(aRenderTimeMs) {
    // We want to use this for queuing, and the calling code recycles the
    // buffer on return from Decode()
    mImage._length = aInputImage._length;
    mImage._size =
        aInputImage._length +
        webrtc::EncodedImage::GetBufferPaddingBytes(webrtc::kVideoCodecH264); // <-- pick huge enough _length such that _length + 8 overflows
    mImage._buffer = new uint8_t[mImage._size];                               // <-- allocate array smaller than _length
    memcpy(mImage._buffer, aInputImage._buffer, aInputImage._length);         // <-- copy _length bytes

I'm not sure if this is easy to trigger or if it's really any more serious than
a crash, but seemed worth reporting. Apologies if this turns out to not be a
concern.

[1] https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h#129-134

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

a) could only apply to 32-bit builds (but that's valid)
b) _length and the buffer it points to would have to be an actual memory buffer of ~4GB (which is very unlikely to be possible)
c) encoded input images are very likely constrained in size before this point (to way less than 4G), but that can be checked
d) adding a backup error-out-on-too-big (or release-assert) here wouldn't hurt even if c) is correct that this can't happen currently.

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1544181 - Check for large frames in GMPDecodeData; r=ng!

Comment 3

[Dan Minor [:dminor]]

Comment on attachment 9129110 [details]
Bug 1544181 - Check for large frames in GMPDecodeData; r=ng!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily.
• 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?: 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?: Attached patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely, this adds an assertion for a condition we're unlikely to ever hit.

Comment 4

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

Comment on attachment 9129110 [details]
Bug 1544181 - Check for large frames in GMPDecodeData; r=ng!

Approved to land and for uplift.

Comment 5

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

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 6

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

https://hg.mozilla.org/integration/autoland/rev/2dfe1500d02eebf29c7f02d042246e7fc843ead9
https://hg.mozilla.org/mozilla-central/rev/2dfe1500d02e

Comment 7

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

https://hg.mozilla.org/releases/mozilla-beta/rev/7142378de78e47f7bd1d2c9f613db53043cc18c5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/afea1e80c367

Comment 9

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

Attachment: advisory.txt

Comment 10

[Deian Stefan]

@Tom can you give Fraser Brown credit too :-) She's the real hero that built the tool that found this bug.

Comment 11

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

Attachment: advisory.txt