Bug 1217663 (CVE-2016-1971)

Missing status check in I420VideoFrame::CreateFrame creates memory-safety bug

RESOLVED FIXED

Status

()

defect
P1
normal
Rank:
5
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: jesup)

Tracking

({csectype-bounds, sec-moderate})

41 Branch
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox41 wontfix, firefox44 wontfix, firefox45 fixed, firefox46 fixed, firefox47 fixed, firefox-esr38- wontfix)

Details

(Whiteboard: [adv-main45+][post-critsmash-triage])

Reporter

Description

4 years ago
I420VideoFrame::CreateFrame (media\webrtc\trunk\webrtc\common_video\i420_video_frame.cc) doesn't check (or propagate) the return code from its calls to Plane::Copy. In consequence, if Plane::Copy fails due to OOM (which could be intermittent), CreateFrame still sets its |width_| and |height_| members. If the new height and/or width are larger than the old one(s), this causes other code (such as ViEExternalRendererImpl::RenderFrame) to use the larger height and/or width when reading the frame's plane buffers, which causes that code to read memory to which it has no right.

The bug is still present in today's trunk: http://hg.mozilla.org/mozilla-central/file/5a6ef7c09c12/media/webrtc/trunk/webrtc/common_video/i420_video_frame.cc .

Details
-------

The bug is in lines 57-9: 

48: int I420VideoFrame::CreateFrame(int size_y, const uint8_t* buffer_y,
49:                                 int size_u, const uint8_t* buffer_u,
50:                                 int size_v, const uint8_t* buffer_v,
51:                                 int width, int height,
52:                                 int stride_y, int stride_u, int stride_v) {
53:   if (size_y < 1 || size_u < 1 || size_v < 1)
54:     return -1;
55:   if (CheckDimensions(width, height, stride_y, stride_u, stride_v) < 0)
56:     return -1;
57:   y_plane_.Copy(size_y, stride_y, buffer_y);
58:   u_plane_.Copy(size_u, stride_u, buffer_u);
59:   v_plane_.Copy(size_v, stride_v, buffer_v);
60:   width_ = width;
61:   height_ = height;
62:   return 0;
63: }

The definition of Plane::Copy (from media\webrtc\trunk\webrtc\common_video\plane.cc) is thus:

69: int Plane::Copy(int size, int stride, const uint8_t* buffer) {
70:   if (MaybeResize(size) < 0)
71:     return -1;
72:   memcpy(buffer_.get(), buffer, size);
73:   plane_size_ = size;
74:   stride_ = stride;
75:   return 0;
76: }


The bug can be simulated in the debugger as follows:

   1.  Start FF with one window, one tab;
   2.  Start FF Hello and create a conversation;
   3.  Undock the Hello window from the main window;
   4.  Copy the conversation link into the address bar and join the conversation;
   5.  Share tabs in the Hello window after it's connected;
   6.  Attach a debugger to FF;
   7.  Set a breakpoint on I420VideoFrame::CreateFrame at the call to |y_plane_.Copy| with the condition |height != height_|.
   8.  Rapidly increase the vertical size of the main window;
   9.  When the breakpoint fires:
       a. record the values of |height| and |height_|; and
       b. step into each call to |plane::Copy| and simulate an OOM by skipping the call to |MaybeResize| and instead giving control to |return -1;|

   10. Notice how I420VideoFrame::CreateFrame unconditionally sets |height_ = height|.
   11. Set a breakpoint on ViEExternalRendererImpl::RenderFrame (media\webrtc\trunk\webrtc\video_engine\vie_renderer.cc), where it calls ExtractBuffer.
   12. Let execution proceed.
   13. When the breakpoint fires, step into ExtractBuffer (module webrtc_libyuv.cc) and examine |input_frame.height|. It should be the same as the new value you saw at the breakpoint in I420VideoFrame::CreateFrame;
   14. Now compute |input_frame.y_plane.allocated_size_ / input_frame.y_plane.stride_| and notice that it equals the old, smaller value of |height_| instead of |input_frame.height|.
   15. Notice how ExtractBuffer uses |input_frame.height()| (== |input_frame.height|) to compute the bounds for its inner |for| loop;
   16. If you're using WinDBG (but not VS, alas) set a read breakpoint on the byte just beyond the buffer's end, let execution proceed, and watch it fire (or, if you increased the size of the window a lot, watch it read access-violate).
Group: core-security → media-core-security
Reporter

Comment 1

4 years ago
There is a similar bug in I420VideoFrame::CreateEmptyFrame.
Reporter

Comment 2

4 years ago
The bug in I420VideoFrame::CreateFrame also affectsI420VideoFrame::CopyFrame, I420VideoFrame::CloneFrame, and their callers.
Tentatively sec-high.  The allocation (in AlignedMalloc()) is using malloc(), and so is fallible (unlike new).  We should check all other derivatives of AlignedMalloc and Plane::Copy, this may well have more duplicates.  Pretty hard to do anything with as it's predicated on an OOM, but using very large planes may allow you to OOM "early" with the system still largely usable.

The update to webrtc.org 43 under way rewrites this code such that it's based on new (and gets rid of the independently-allocated Planes as well).

A related bug: bug 881742, which landed in 41.

Bug from upstream; since fixed (between upstream branch 40 and 43) by rewrite
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
backlog: --- → webrtc/webaudio+
Rank: 5
Priority: -- → P1
Flags: sec-bounty?
Fixed by landing upstream update to branch 43 in bug 1198458
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Group: media-core-security → core-security-release
Randell, what is the ESR38 plan here? We are supposed to take sec-high issue patches on ESR branches.

Also, is the "branch 43" code landing on what branches? 43? Trunk?
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #4)
> Fixed by landing upstream update to branch 43 in bug 1198458

What is "branch 43"? Does that mean status-firefox43 should be "fixed" rather than "affected"? The branch status flags on the other bug weren't set either, but it looks like it landed in Firefox 45, not 43.
(In reply to Daniel Veditz [:dveditz] from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > Fixed by landing upstream update to branch 43 in bug 1198458
> 
> What is "branch 43"? Does that mean status-firefox43 should be "fixed"
> rather than "affected"? The branch status flags on the other bug weren't set
> either, but it looks like it landed in Firefox 45, not 43.

Sorry - branch 43 means webrtc.org (upstream) stable branch 43, which landed in our tree in Firefox 45.  Upstream stable branches of webrtc.org use numbering that matches the version of Chrome they shipped in (for the last year or so).

As for ESR-38 - The issue would be we need to create a patch to do the right checking (which shouldn't be hard) and land it in 44 and ESR-38.  This would be (very) hard to exploit since it depends on an allocation failure on a reallocation to a larger size, and then there's a good chance that it would just lead to a crash.  However, as the checking should be simple we could look to land a patch for 44/ESR-38 early next week (I'm on PTO until the 21st, but am checking sec issues).
Flags: needinfo?(rjesup)
[Tracking Requested - why for this release]:
This fix is about to ship in Firefox 45, so we have to decide whether to fix this on ESR-38. The description of how to trigger this bug in comment 7 makes it sound more like a sec-moderate issue.
I concur
Dan, if it is sec-moderate, we would most likely wontfix it for esr38. Should I go ahead and do that? Or do we need more discussion/comments on this? Thanks!
Flags: needinfo?(dveditz)
Whiteboard: [adv-main45+]
Whiteboard: [adv-main45+] → [adv-main45+][post-critsmash-triage]
Alias: CVE-2016-1971
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.