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

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

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)

Comment 1

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

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

Comment 2

[Dan Minor [:dminor]]

Attachment: Use CheckedInt when calculating allocation sizes

Comment 3

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

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

Comment 4

[Dan Minor [:dminor]]

Attachment: Use CheckedInt when calculating allocation sizes

Comment 5

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

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.

Comment 6

[Dan Minor [:dminor]]

Attachment: Use CheckedInt when calculating allocation sizes

Comment 7

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

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.

Comment 8

[Dan Minor [:dminor]]

Attachment: Use CheckedInt when calculating allocation sizes

With review comments addressed.

Comment 9

[Dan Minor [:dminor]]

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.

Comment 10

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

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.

Comment 11

[Dan Minor [:dminor]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56896524a816

Sounds like backporting this isn't necessary.