Protect against integer overflow in webrtc/signaling/src/mediapipeline/MediaPipeline.cpp

RESOLVED FIXED in Firefox 56

Status

()

Core
WebRTC: Audio/Video
P1
major
Rank:
19
RESOLVED FIXED
11 months ago
22 days ago

People

(Reporter: decoder, Assigned: dminor)

Tracking

(Blocks: 1 bug, {sec-audit, sec-want})

Trunk
mozilla56
sec-audit, sec-want
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [adv-main56-][post-critsmash-triage])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 months ago
The following code performs unchecked arithmetic operations on integral data types with the result being used in functions related to memory allocation:

https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#266

This pattern occurs in multiple places. I assume this cannot be exploited practically because it would require an image frame of approximately 53509 x 53509 (about 2.7 GB of image data) and according to jesup, this code currently only handles streams from video cameras. I still think that these calculations should use CheckedInt if they are not performance critical.

A potential overflow in "length" here would lead to less memory being allocated than is required for the subsequent writes, which is always a very vulnerable pattern.

Still filing this s-s until a developer has triaged it and decided that this particular code is safe.

If this isn't supposed to be changed, then we should add a static analysis annotation to ignore the code.

(This is part of a custom static analysis we have been working on in bug 1279569)

Updated

11 months ago
Group: core-security → media-core-security

Comment 1

11 months ago
While ForceBlack isn't performance-sensitive, other points in this are slightly-so.  On the other hand, we're probably seeing at most 60 frames per second, perhaps times a couple of devices, plus perhaps a dozen or two incoming streams at 30fps.  So, depending on the overhead of CheckedInt<>, we could use that.

That said... I think most paths into this code have resolution limits of their own.  The biggest concerns would be remote video from webrtc peerconnections, and canvas.captureStream.  The memory requirements to break this would be... insane and likely not even possible.

P1 because we don't want to annoy the static-analysis runs
Rank: 19
Priority: -- → P1
(Assignee)

Updated

9 months ago
Assignee: nobody → dminor
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 months ago
Created attachment 8873534 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes
Attachment #8873534 - Flags: review?(rjesup)
Comment on attachment 8873534 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes

Review of attachment 8873534 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +268,5 @@
>      if (aForceBlack) {
>        IntSize size = aImage->GetSize();
> +      CheckedInt<uint32_t> yPlaneLen = YSIZE(size.width, size.height);
> +      CheckedInt<uint32_t> cbcrPlaneLen = 2 * CRSIZE(size.width, size.height);
> +      CheckedInt<uint32_t> length = yPlaneLen + cbcrPlaneLen;

You can assert that 2*CRSIZE() < YSIZE(), so cbcrPlaneLen can't overflow if yPlaneLen doesn't, so we don't need that one to be a CheckedInt, and that avoids as many validity checks.

Also: does CheckedInt<foo> bar = random_calculation actually catch overflows *in* random_calculation?  I think not.  It would only catch overflows if random_calculation overflows when assigned to type bar.

@@ +283,2 @@
>          // Fill Cb/Cr planes
> +        memset(pixelData.get() + yPlaneLen.value(), 0x80, cbcrPlaneLen.value());

If that doesn't need to be CheckedInt, we save a value call

@@ +406,5 @@
>      int half_height = (size.height + 1) >> 1;
> +    CheckedInt<int> c_size = half_width * half_height;
> +    CheckedInt<int> buffer_size = YSIZE(size.width, size.height) + 2 * c_size;
> +
> +    if (!c_size.isValid() || !buffer_size.isValid()) {

Ditto about where overflows can be noticed - and c_size < YSIZE again, which could lend to optimization.

@@ +426,5 @@
>      }
>  
>      int rv;
> +    CheckedInt<int> cb_offset = YSIZE(size.width, size.height);
> +    CheckedInt<int> cr_offset = cb_offset + c_size;

If we've checked these above, we don't need to check offsets here
Attachment #8873534 - Flags: review?(rjesup) → review-
(Assignee)

Comment 4

9 months ago
Created attachment 8873905 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes
Attachment #8873534 - Attachment is obsolete: true
Attachment #8873905 - Flags: review?(rjesup)
Comment on attachment 8873905 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes

Review of attachment 8873905 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +267,4 @@
>  
>      if (aForceBlack) {
>        IntSize size = aImage->GetSize();
> +      CheckedInt<uint32_t> yPlaneLen = YSIZE(size.width, size.height);

I believe to *actually* catch an overflow here you must involve a Checked type *before* math that can over/underflow *or* guarantee that there can be no overflow except in the final assignment.

typedef IntSizeTyped<UnknownUnits> IntSize;
which uses public BaseSize< int32_t, IntSizeTyped<units>> so width and height are int32_t's.

So YSIZE(x,y) (which is ((x)*(y))) can overflow, and then it assigns it to the CheckedInt<> which doesn't overflow.

Perhaps  CheckedInt<uint32_t> yPlaneLen = YSIZE(static_cast<CheckedInt32>(size.width), size.height); -- but verify

@@ +271,1 @@
>        uint32_t cbcrPlaneLen = 2 * CRSIZE(size.width, size.height);

comment why no CheckedInt<> is needed here

@@ +405,4 @@
>      int half_width = (size.width + 1) >> 1;
>      int half_height = (size.height + 1) >> 1;
>      int c_size = half_width * half_height;
> +    CheckedInt<int> buffer_size = CheckedInt<int>(YSIZE(size.width, size.height)) + 2 * c_size;

Ditto.  Also, do we want YSIZE() to work on signed values?  Perhaps use CheckedUInt32 instead of CheckedInt.
Attachment #8873905 - Flags: review?(rjesup) → review-
(Assignee)

Comment 6

9 months ago
Created attachment 8876800 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes
Attachment #8873905 - Attachment is obsolete: true
Attachment #8876800 - Flags: review?(rjesup)
Comment on attachment 8876800 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes

Review of attachment 8876800 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming the int issue is dealt with in some manner, and I advise making only one entry in ysize Checked

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +98,4 @@
>  };
>  
>  // I420 buffer size macros
> +#define YSIZE(x,y) (CheckedInt<uint64_t>(x)*CheckedInt<uint64_t>(y))

Should only need to make one of them CheckedInt

@@ +432,1 @@
>      int cr_offset = cb_offset + c_size;

uint64_t may not fit in int.  If these need to get passed as an int, you need to change the checks to be int, or range-check here.
Attachment #8876800 - Flags: review?(rjesup) → review+
(Assignee)

Comment 8

9 months ago
Created attachment 8877212 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes

With review comments addressed.
Attachment #8876800 - Attachment is obsolete: true
(Assignee)

Comment 9

9 months ago
Comment on attachment 8877212 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Seems unlikely - a very large resolution camera would be required to make these calculations overflow.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I mentioned CheckedInt in the check-in comment which implies we are trying to avoid overflows.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, but it should be low effort and low risk to create them.

How likely is this patch to cause regressions; how much testing does it need?
Seems very unlikely. I've tested these locally on 64-bit ubuntu.
Attachment #8877212 - Flags: sec-approval?
Comment on attachment 8877212 [details] [diff] [review]
Use CheckedInt when calculating allocation sizes

This isn't a sec-high or critical so, technically, doesn't need sec-approval. Given the bug, I'd say you're fine to just check it in.
Attachment #8877212 - Flags: sec-approval?
(Assignee)

Comment 11

8 months ago
(In reply to Al Billings [:abillings] from comment #10)
> Comment on attachment 8877212 [details] [diff] [review]
> Use CheckedInt when calculating allocation sizes
> 
> This isn't a sec-high or critical so, technically, doesn't need
> sec-approval. Given the bug, I'd say you're fine to just check it in.

Oh ok, thanks! Sorry to have taken your time.
https://hg.mozilla.org/mozilla-central/rev/56896524a816

Sounds like backporting this isn't necessary.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → wontfix
status-firefox55: affected → wontfix
status-firefox56: --- → fixed
status-firefox-esr52: --- → wontfix
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: media-core-security → core-security-release
Whiteboard: [adv-main56-]
Flags: qe-verify-
Whiteboard: [adv-main56-] → [adv-main56-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.