Closed Bug 1218217 Opened 6 years ago Closed 5 years ago

Potential overflow in CalcBufferSize can cause memory-safety bug

Categories

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

41 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 --- wontfix
b2g-master --- fixed
Blocking Flags:

People

(Reporter: q1, Assigned: jesup)

References

Details

(Keywords: sec-low, Whiteboard: [post-critsmash-triage][adv-main46-])

Attachments

(1 file, 1 obsolete file)

CalcBufferSize (media\webrtc\trunk\webrtc\common_video\libyuv\webrtc_libyuv.cc) does not detect overflow. If it is possible for its arguments to be large enough to cause overflow, its caller(s) could allocate buffers too small to contain the available data, causing reading/writing of memory to which they have no right.

I tentatively have ruled out overflow for some uses of CalcBufferSize, but not all. The ones that appear OK are:

   1. VP8 streams, for which the width and height are limited to 0x3fff by ParseVP8FrameSize [1]. 0x3fff is too small to cause CalcBufferSize to overflow;
   2. VP9 streams, for which the width and height appear to be limited by resize_context_buffers [2] to DECODE_WIDTH_LIMIT and DECODE_HEIGHT_LIMIT, which are, respectively, 4000 (decimal) and 3000 (decimal) on all supported platforms;

I have not determined whether there are any limits on the frame size for I420 streams, nor whether there might be other stream types for which CalcBufferSize is called.


Details
-------

There are no overflow checks in CalcBufferSize:

69: int CalcBufferSize(VideoType type, int width, int height) {
70:   int buffer_size = 0;
71:   switch (type) {
72:     case kI420:
73:     case kNV12:
74:     case kNV21:
75:     case kIYUV:
76:     case kYV12: {
77:       int half_width = (width + 1) >> 1;
78:       int half_height = (height + 1) >> 1;
79:       buffer_size = width * height + half_width * half_height * 2;
80:       break;
81:     }
82:     case kARGB4444:
83:     case kRGB565:
84:     case kARGB1555:
85:     case kYUY2:
86:     case kUYVY:
87:       buffer_size = width * height * 2;
88:       break;
89:     case kRGB24:
90:       buffer_size = width * height * 3;
91:       break;
92:     case kBGRA:
93:     case kARGB:
94:       buffer_size = width * height * 4;
95:       break;
96:     default:
97:       assert(false);
98:       return -1;
99:   }
100:  return buffer_size;
101:}
Oops, I missed the footnotes:

[1] media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_format_vp8.cc
[2] media\libvpx\vp9\decoder\vp9_decodeframe.c
See https://bugzilla.mozilla.org/show_bug.cgi?id=1218219 for issues with one caller of CalcBufferSize .
Flags: sec-bounty?
> I have not determined whether there are any limits on the frame size for I420 streams, nor whether there might be other stream types for which CalcBufferSize is called.

We don't support I420 remote streams, so that shouldn't be an issue.  The only video codecs we support currently are VP8 and H.264; VP9 support is behind a pref still as packetization isn't finalized.  H.264 goes through the GMP/OpenH264 interface, which may well (in various places in Gecko or in OpenH264) limit the max width/height.

If there are other sources of width/height values to CalcBufferSize, those will need to be checked to see if this can actually happen in practice.
Assignee: nobody → rjesup
NI to myself to come back and confirm or not after analysis
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(rjesup)
Group: core-security → media-core-security
The remaining video codec, OpenH264, has a max-width of 4096 and height of 2034 internally, so per the analysis above it should not be possible to force an overflow.  We'll land a fix and get upstream to fix it, since this is a latent vulnerability that could become active with new codecs or changes to the codecs.

Thanks to q1 for checking vp8 and vp9 initially; I was almost certain H264 had similar limits but had to track them down.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rjesup)
Keywords: sec-moderate
The API for CalcBufferSize was changed to size_t in update to upstream branch 43, which was underway and has since landed in 45.  This plugs the primary hole (though not in a great manner, and depending on the calling code's handling) by never returning negative numbers.  In cases where overflow could occur (i.e. not in any current codec), then the callers would need to be audited to handle size_t wrap-around.
Duplicate of this bug: 1218219
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Flags: sec-bounty? → sec-bounty+
Attachment #8704265 - Flags: review?(pkerr) → review+
Comment on attachment 8704265 [details] [diff] [review]
avoid buffersize overflow even if codec is unbounded in dimensions

I'm r- ing my patch, since assert() is only valid in debug builds (NDEBUG is defined; I checked).  New patch coming.
Attachment #8704265 - Flags: review+ → review-
Attachment #8704265 - Attachment is obsolete: true
Attachment #8704444 - Flags: review?(pkerr) → review+
https://hg.mozilla.org/mozilla-central/rev/1d15f75f55aa
https://hg.mozilla.org/mozilla-central/rev/02c5e326cd39
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+]
Alias: CVE-2016-2818
For the sake of ESR-based releases which might be interested in this fix, I assume Firefox 45 must be affected but what about ESR-38?
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
Removing CVE and advisory since it turns out that we are not actually affected in practice.
Alias: CVE-2016-2818
Whiteboard: [post-critsmash-triage][adv-main46+] → [post-critsmash-triage][adv-main46-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.