1217663 - (CVE-2016-1971) Missing status check in I420VideoFrame::CreateFrame creates memory-safety bug

Status: RESOLVED FIXED

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

Description

[q1]

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

Comment 1

[q1]

There is a similar bug in I420VideoFrame::CreateEmptyFrame.

Comment 2

[q1]

The bug in I420VideoFrame::CreateFrame also affectsI420VideoFrame::CopyFrame, I420VideoFrame::CloneFrame, and their callers.

Comment 3

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

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

Comment 4

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

Fixed by landing upstream update to branch 43 in bug 1198458

Comment 5

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 6

[Daniel Veditz [:dveditz]]

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

Comment 7

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

(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).

Comment 8

[Daniel Veditz [:dveditz]]

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

Comment 9

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

I concur

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!