1428947 - (CVE-2018-5129) OOB Write in CopyPlane within ImageContainer.cpp

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P2)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Received a vuln report via email from a friend. I've not confirmed (on paternity leave) but someone should check this. Bug sounds hard to exploit, but reporter want to check with us before blogging about it just in case.


--VIA EMAIL BELOW ------

In the process of creating a full writeup / exploit for the Pwn2Own 2017 bug (https://www.mozilla.org/en-US/security/advisories/mfsa2017-08/) I believe have come across another bug. I am unsure whether the Chaitin team who discovered the Integer Overflow overlooked the vulnerable code, chose to ignore it, reported it alongside their Integer Overflow, or someone else found it subsequently and reported it but I haven't yet found any indication of this being the case.

What's the bug?
The vulnerable function is `CopyPlane` within ImageContainer.cpp (https://searchfox.org/mozilla-central/source/gfx/layers/ImageContainer.cpp#539). The issue here is that the signed 32-bit integer `width` used in the 'Slow Path' of the copy (or when aSkip is anything other than 0) is unchecked. This means that in the case of Firefox 52 a specially crafted Layout can achieve a very simple out of bounds write purely by setting the width value of a layout object to more than the calculated height. (Inside the poc.zip folder I have provided the oobwrite.html file for a simple demonstration of what i mean).

What's the impact?
After a bit of research and scouring through the current Firefox source, I have come to the conclusion that it is _very_ difficult to actually trigger this vulnerable function however it is not impossible. The function `CopyPlane` is referenced only within the `RecyclingPlanarYCbCrImage::CopyData`. The `CopyData` function is then referenced 9 times outside of ImageContainer however, 1 is mitigated by the fact that you removed the overloaded constructor for ImageBitmap objects in Firefox 52. I haven't had a chance to go through and attempt to trigger the bug via the remaining 8 references but i have the suspicion it could be very difficult to trigger from any other code path. For this reason i understand the impact is low but i think it is definitely worth looking at! 

I have actually included this out of bounds write within my writeup but - because I realised the bug may not have been reported / discovered prior to now i thought it would be best to let you know and make sure it got fixed or marked as wont-fix before i go and publish the details publicly.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Attachment: oobwrite.html

PoC. Needs canvas.imagebitmap_extensions.enabled set to true.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So note that while the PoC requires imagebitmap_extensions enabled, the vulnerable function exists in FF for a long time, is just hard to hit. So that's really the question here: can you trigger this exploit this using some other way?

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I'm not sure if this is strictly related but FYI it shows up in the analysis from 1279569.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thanks Paul & friend.

I had a quick look at the path going through MediaData.cpp, and that one seemed possibly safe thanks to the validation at:
https://searchfox.org/mozilla-central/source/dom/media/MediaData.cpp#308

But this should be verified, and there are other paths using CopyPlane.
Blake, could you (or someone on the media team) please have a look?

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Gerald Squelart [:gerald] from comment #4)
> Thanks Paul & friend.
> 
> I had a quick look at the path going through MediaData.cpp, and that one
> seemed possibly safe thanks to the validation at:
> https://searchfox.org/mozilla-central/source/dom/media/MediaData.cpp#308

Note that these checks only check overflow - it doesn't validate that the width is actually the size of the buffer. 

> But this should be verified, and there are other paths using CopyPlane.
> Blake, could you (or someone on the media team) please have a look?

For reference for ever is joining this thread the issue is here:
https://searchfox.org/mozilla-central/source/gfx/layers/ImageContainer.cpp#552

If width isn't what we expect, we write out of bounds.

Comment 6

[bzdllx]

Hey all, 

I have taken a look at the ValidateBufferAndPicture in terms of validating / preventing the bug and have realised that it only prevents the following situations:
1. the Planes (destination buffers) from becoming out of sync for whatever reason, maybe a realloc or an accidental free or something unexpected. (117-123)
2. the width and height becoming negative (good check to have) or being zero. (126-130)
3. the `ValidatePlane` function then ensures that the width and height are less than 16384 AND the width * height is less than 8192 * 4608 AND that the stride is > 0. (131-136)
4. the xLimit & yLimit for the aPicture do not have any sort of Integer Overflow / Underflow in their width or height compared to their value (Im assuming to prevent one from referencing out of bound pixels or whatever when rendering) (138-148)

Based on those 4, it is still possible that the height can be set to 1/4 of the width (for example). Then, because the Plane ( destination buffer ) is allocated based on the calculation: mData.mCbCrStride * mData.mCbCrSize.height * 2 + mData.mYStride * mData.mYSize.height, the width can just be 2 * the height and it will iterate past the end of the destination buffer. 

I think it might be good to mention also that the vulnerable function is also called from within ChromiumCDMParent. Firstly the IPC function RecvDecodedData (https://searchfox.org/mozilla-central/source/dom/media/gmp/ChromiumCDMParent.cpp#686) takes the untrusted input and passes it to CreateVideoFrame (https://searchfox.org/mozilla-central/source/dom/media/gmp/ChromiumCDMParent.cpp#783) which will then pass the untrusted data down the line to our CopyData / CopyPlane allowing us to crash the Parent GMP process / escape the sandbox.

Comment 7

[Blake Wu [:bwu][:blakewu]]

Ni John to help check this bug.

Comment 8

[John Lin [:jhlin][:jolin]]

Unfortunately, ValidateBufferAndPicture() can only guarantee the region to copy (aPicture) lies within the source buffer (aBuffer) by checking the plane info because the actual source buffer length is not available in the YCbCrBuffer structure. Unless that information is provided, the caller should be responsible for the correctness of plane info.

Currently there are 8 callers to VideoData::CreateAndCopyData() that fills the info. All seems fine, and no validation is done in any of them. AFAICT, only ChromiumCDMParent::CreateVideoFrame() is able to do it for others receive raw pointers and cannot get the buffer length.

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla9VideoData17CreateAndCopyDataERKNS_9VideoInfoEPNS_6layers14ImageContainerElRKNS_5media8TimeUnitESA_RKNS0_11YCbCrBufferEbSA_RKNS_3gfx12IntRectTypedINSE_12UnknownUnitsEEEPNS4_15KnowsCompositorE&redirect=false

Comment 9

[John Lin [:jhlin][:jolin]]

(In reply to bzdllx from comment #6)
> 
> Based on those 4, it is still possible that the height can be set to 1/4 of
> the width (for example). Then, because the Plane ( destination buffer ) is
> allocated based on the calculation: mData.mCbCrStride *
> mData.mCbCrSize.height * 2 + mData.mYStride * mData.mYSize.height, the width
> can just be 2 * the height and it will iterate past the end of the
> destination buffer. 
> 

 bzdllx, could you please elaborate? I don't quite get your concern here. The destination buffer length is Y plane length + Cb + Cr where each plane is stride * height bytes long. It has nothing to do with the propotion of width to height.

Comment 10

[John Lin [:jhlin][:jolin]]

It seems 2 layers of protections are needed:
1. Examine the info inside aData before buffer allocation in RecyclingPlanarYCbCrImage::CopyData() to make sure it's safe to copy image data.
2. (For Paul's PoC) check the plane layout and reject invalid ImageBitmap creation.

Comment 11

[bzdllx]

(In reply to John Lin [:jolin][:jhlin] from comment #9)
> (In reply to bzdllx from comment #6)
> > 
> > Based on those 4, it is still possible that the height can be set to 1/4 of
> > the width (for example). Then, because the Plane ( destination buffer ) is
> > allocated based on the calculation: mData.mCbCrStride *
> > mData.mCbCrSize.height * 2 + mData.mYStride * mData.mYSize.height, the width
> > can just be 2 * the height and it will iterate past the end of the
> > destination buffer. 
> > 
> 
>  bzdllx, could you please elaborate? I don't quite get your concern here.
> The destination buffer length is Y plane length + Cb + Cr where each plane
> is stride * height bytes long. It has nothing to do with the propotion of
> width to height.

Sure! So my main concern is actually highlighted by your comment - "It has nothing to do with the proportion of width to height." The issue I see here is actually best viewed from a 'How would I fix it' standpoint. Currently, the destination buffer is calculated with the Stride and Height from the Cb / Cr / Y Layout objects. However, when it comes to the vulnerable 'Slow path' within the `CopyPlane` function, the nested for-loop copy is based on `width * height`, not `height * stride`. So the 'fixed' approach to the 'Slow Path' copy would either be to copy based on `height * stride` or, when allocating the buffer in `CopyData`, somehow integrate the width property in the equation so the size is calculated based on that instead of the stride. 

As an example of what i mean - If you take a look at the PoC Paul attached (crashes in FF52), you will see that the uLayout.width property is actually double the size of the yLayout.height property. If we do the maths for the destination buffer allocation with the numbers included in my PoC the resulting destination buffer is allocated as follows: 
 = 1 * 1 * 2 + 1024 * 1
 = 1026 bytes

Then, if we use the current 'Slow Path' copy of that `CopyPlane` function with our uLayout.width property set to 2048. We will enter the top for-loop with a height of 1 and then iterate our width 2048 times, allowing us to overrun the 1026 destination buffer. 

In terms of a trigger / crashing the latest version of Firefox, a specially crafted video file would definitely be able to trigger the aforementioned vulnerable 'Slow Copy' code. Would take a fair amount of effort however it would by no means be impossible.

Comment 12

[John Lin [:jhlin][:jolin]]

(In reply to bzdllx from comment #11)
> (In reply to John Lin [:jolin][:jhlin] from comment #9)
> 
> Sure! So my main concern is actually highlighted by your comment - "It has
> nothing to do with the proportion of width to height." The issue I see here
> is actually best viewed from a 'How would I fix it' standpoint. Currently,
> the destination buffer is calculated with the Stride and Height from the Cb
> / Cr / Y Layout objects. However, when it comes to the vulnerable 'Slow
> path' within the `CopyPlane` function, the nested for-loop copy is based on
> `width * height`, not `height * stride`. So the 'fixed' approach to the
> 'Slow Path' copy would either be to copy based on `height * stride` or, when
> allocating the buffer in `CopyData`, somehow integrate the width property in
> the equation so the size is calculated based on that instead of the stride. 
>

 Agreed. I think we should take the 2nd approach for it also saves some memory when the source buffer has padding (strike > width), or is semi-planar (stride >= 2 * width) or interleaved (stride >= 3 * width)

 BTW, it seems there are other problems in CopyData():
  - mData is modified [1] even when copy fails
  - incorrect mData.*Skip values. (Should all be 0 because destination buffer is planar)

[1] https://searchfox.org/mozilla-central/source/gfx/layers/ImageContainer.cpp#565

Comment 13

[John Lin [:jhlin][:jolin]]

Attachment: Use source buffer width rather than stride.

Comment 14

[Frederik Braun [:freddy]]

The security rating is a guess. Please someone feel free to rate again.

Comment 15

[bzdllx]

(In reply to John Lin [:jolin][:jhlin] from comment #13)
> Created attachment 8942572 [details] [diff] [review]
> Use source buffer width rather than stride.

Hi John,

I've reviewed the patch you've created and only have one question - I noticed that on lines 587 & 591 you have set the Stride for `mY` and `mCbCr` equal to the width before passing them to `CopyPlane`. `CopyPlane` then performs the 'Slow' copy the same as before however now, the `aDst` buffer is using `+=width` instead of `+=stride`. Is there a reason why you couldn't keep the original `+=stride` for the `aDst` buffer as well seeing as you are setting the Stride equal to the width prior to the function call anyway? I'm asking purely to better my understanding :) 

I will make the changes in my build of FF52 and test them to make sure I can't write OOB! 

Thanks.

Comment 16

[John Lin [:jhlin][:jolin]]

(In reply to bzdllx from comment #15)
> (In reply to John Lin [:jolin][:jhlin] from comment #13)
> > Created attachment 8942572 [details] [diff] [review]
> > Use source buffer width rather than stride.
> 
> Hi John,
> 
> I've reviewed the patch you've created and only have one question - I
> noticed that on lines 587 & 591 you have set the Stride for `mY` and `mCbCr`
> equal to the width before passing them to `CopyPlane`. `CopyPlane` then
> performs the 'Slow' copy the same as before however now, the `aDst` buffer
> is using `+=width` instead of `+=stride`. Is there a reason why you couldn't
> keep the original `+=stride` for the `aDst` buffer as well seeing as you are
> setting the Stride equal to the width prior to the function call anyway? I'm
> asking purely to better my understanding :) 

 aStride in CopyPlane() is the stride of source (aData.m*Stride in CopyData()); Before calling CopyPlane() it’s the destination plane strides (mData.m*Stride) which were modified.

 Maybe we could rename aData to aSource, or change CopyPlane() parameters to include both source and destination strides like libyuv::CopyPlane(), to make it easier to read.

> 
> I will make the changes in my build of FF52 and test them to make sure I
> can't write OOB! 
> 
> Thanks.

Thanks for giving it a try.

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

My guess is that this bug needs a new owner (FYI Jeff when you get to reviewing this). Feel free to correct me if I'm wrong here.

Comment 18

[bzdllx]

Hey all,

I have applied the patch created by John and tested it against FF52. I know you guys have your own testing process but i used the 'oobwrite.html' file to count the number of crashes / 50 runs on both an unpatched and patched version. 

Results:
 - The unpatched version resulted in 36 crashes and 14 without.
 - The patched version resulted in 50 without. (Woo!)

I will double check every path here to be absolutely certain but i think the ability to write OOB has been removed with the changes.

Not sure who the new owner will be but it looks like you guys just need to test and then merge in :)

Comment 19

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I'm concerned about the lack of movement on this bug. I know we have a difficult situation on our hands -  but comment 6 worries me - as far as I can tell, the comment is correct.  IIUC, we take data from IPC in ChromiumCDMParent::RecvDecodedShmem, pass it to VideoData::CreateAndCopyData which calls the vulnerable Copydata/Copyplace. 

Corrent me if I'm wrong here, but this would appear to be a serious sandbox escape (especially if it can be used to escape the GMP process which would seem likely). Upgrading criticality as a prudential measure.

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Ralph - can you follow up with Jeff for review and find a new owner to land the patch?

Comment 21

[Ralph Giles (:rillian)]

Dan, can you please take over getting this landed. As a sec-critical you'll need to ask for approval, prepare backports to beta if necessary and be ready to land on all branches at once.

Comment 22

[u480271]

Hi, I'm taking over landing this patch. From #c19, am I to determine that the patches aren't ready to land yet?

Comment 23

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Dan Glastonbury :kamidphish from comment #22)
> Hi, I'm taking over landing this patch. From #c19, am I to determine that
> the patches aren't ready to land yet?

No,  sorry for confusing things. As far as I can tell, the patch here does seem to fix the underlying issue here. My understanding of the state of the bug is that the patch needs to be reviewed, to make sure that it does actually fix the issue, and that this is the best place to add validation etc. 

I was only commenting on the criticality in comment 19 as to me this seems like a bug which is both web exploitable (via a maliciously crafted video) and also one could lead to code execution in the parent (based on looking at the code path in comment 6). We don't have a PoC for either of these (the original PoC is based on the pref'd off createImageBitmap function) but in the end it doesn't really matter whether this is sec-high or sec-critical, it needs to be fixed asap.

Comment 24

[u480271]

I reviewed the code and the changes made by :jhlin should fix the OOB memory write in CopyPlanes.  I looked at the code path via the CDM and the parameters are validated, although I believe it's possible to craft an IPDL message that causes an OOB memory read, since the image parameters aren't checked against the size of aData received in `ChromiumCDMParent::RecvDecodedData()`.

What I can't comment on is that :jhlin changed an assumption of `RecyclingPlanarYCbCrImage`, so I'm going to NI? :mattwoodrow to get an answer on whether it is OK that Y, Cb, Cr image planes have mWidth = mStride.

Comment 25

[Jean-Yves Avenard [:jya]]

I believe the original description is invalid and in particular a way to reproduce it.

It is not possible to generate a YCbCrImage using a specially crafted layout: the code path to access libyuv is never reached.

The only producers of YCbCrImage objects are media decoders, and of those decoders only the VPXDecoder, FFmpegVideoDecoder and TheoraDecoder can produce such objects.

Why this is important, is that it's not possible for a decoder to produce an image whose width is big enough that it would generate an overflow later.
When we demux webm (vp8/vp9) and Theora (ogg) media content, the validity of such content is checked by the demuxer using:
https://searchfox.org/mozilla-central/source/dom/media/VideoUtils.cpp#176
the value of PlanarYCbCrImage::MAX_DIMENSIONS is set https://searchfox.org/mozilla-central/source/gfx/layers/ImageContainer.h#825
that's 16384 pixels wide

Other decoders use:
https://searchfox.org/mozilla-central/source/dom/media/MediaData.cpp#106
which checks that neither the width nor the height would be greater than 16384 pixels wide.

This bug is non-existent as such, and we would be attempting to fix a problem that doesn't exist at the expense of slowing down everything else.

In a YCbCrImage a stride typically is 16 bytes aligned due to the hardware code that generated it, and libyuv makes use of SSE accelerated code when the stride have such alignment.
If we made the stride == width, libyuv would then have to use a slow C code path.

When playing a 60fps video, slowing down what is already a heavy process will have a great impact.

I would close this bug as invalid

Comment 26

[Jean-Yves Avenard [:jya]]

(In reply to Paul Theriault [:pauljt] from comment #23)
> the code path in comment 6). We don't have a PoC for either of these (the
> original PoC is based on the pref'd off createImageBitmap function) but in
> the end it doesn't really matter whether this is sec-high or sec-critical,
> it needs to be fixed asap.

then fix createImageBitmap to prevent creating such images... It would be a one-liner.

Do not touch a low-level routine that is called in a critical path.

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8942572 [details] [diff] [review]
Use source buffer width rather than stride.

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

the fix will do what it's supposed to do.
However, this is the wrong approach.

Fix the call site instead

Comment 28

[Matt Woodrow (:mattwoodrow)]

It seems like the fundamental issue here is that stride < width. The stride is the number of bytes per row, and should always be greater or equal than the width * bytes-per-pixel.

We're allocating the destination buffer based on stride (as you'd expect, we don't want to bother trying to compact things), and then copying all the valid bytes into it (using the width).

That seems like expected behaviour, unfortunately the PoC testcase is passing a completely garbage stride, and nobody bothers to check.

We should just add some validation for the untrusted callers (createImageBitmap, and ChromiumCDMParent::RecvDecodedShmem) to ensure that the stride actually covers all bytes of the width, and the extents of the planes are within the buffer provided.

We could run this validation unconditionally in debug builds, so that we'd catch broken decoders, and just run it for the untrusted callers in opt builds.

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #28)
> It seems like the fundamental issue here is that stride < width. The stride
> is the number of bytes per row, and should always be greater or equal than
> the width * bytes-per-pixel.

So why not check this? Is it too much of a performance cost? The basic issue here seems to be a difference in the allocation in CopyPlane vs the loop in CopyData. The size of the buffer is based on height and stride, and the loop in CopyData is based on height and width. I don't know enough about video formats to understand why there is a difference here, but the naive solution would seem to be to make the buffer allocation and the CopyData function use the same calculation(i.e. both take width into account, or neither).

But taking a step back: here we have a dangerous low-level function which has undocumented assumptions which has lead to a number of security issues. I'm struggling to understand why it's not better to fix this in the calling functions, rather than add check to the dangerous low-level function. Performance I guess, but I'm worried we haven't addressed all the issues. I'm pretty certain that the Sandboxing issue in ChromiumCDMParent needs to be fixed, but I'm unclear about other issues. Let me try to outline the issues that I see, so we can be sure we have resolved everything here. 

1. createImageBitmap 
This was the original PoC. I don't much care about this - its pref'd off, and known to be really broken. We should add the validation here I guess, but it's not a priority, since there is a lot more fixing that is needed before we would consider re-enabling this. (and as far as I know we don't plan to do this any time soon). 

2. Video decoding leading to CopyPlane 
So in comment 25, jya seems pretty sure that _all_ of the video decoding paths to this issue are safe due to validation. To be honest I'm not convinced - mainly because in comment 25 he is talking about checking the maximum size, and that's not the point of the bug. This is NOT an overflow bug - it's a data validation issue leading to out-of-bounds write. This is complex code and I trust his knowledge better than mine, but I'm not sure if we are doing the correct validation here. I'm looking more into this (I don't completely understand comment 25 yet).

3. The sandbox escape issue  
ChromiumCDMParent seems to me to pretty clearly be a sandbox escape. An attacker who has compromised the GMP sandbox could send arbitrary IPC messages to ChromiumCDMParent::RecvDecodedData or ChromiumCDMParent::RecvDecodedShmem. (PS Matt, why did you only mention RecvDecodedShmem? Aren't both of these functions a concern? I also wonder if these IPC message can ONLY be called from GMP, or can a regular child process also invoke these IPC calls? Either way, we need some validation here to avoid the sandbox escape. 

4. Other call sites
Is MediaEngineRemoteVideoSource::DeliverFrame[1] another call site? Are there others that we haven't discussed above? I haven't done an audit because I assumed we would fix the vulnerable function.
[1] https://searchfox.org/mozilla-central/source/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#553

5. Correctness issues
At the end of comment 12, several correctness issues were highlighted. Are these valid issues that need fixing, or is comment 12 incorrect?

I think that's everything. IF we are going down the route of fix the callsites, then someone needs to an audit to make sure we have got all the callers. Personally I'd rather that we just fix the dangerous function (if possible).

Comment 30

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Correction: paragraph two, sentence two - I mean I'm struggling to understand why we dont fix CopyPlane/CopyData instead the calling functions.

Comment 31

[John Lin [:jhlin][:jolin]]

(In reply to Jean-Yves Avenard [:jya] from comment #25)
> 
> In a YCbCrImage a stride typically is 16 bytes aligned due to the hardware
> code that generated it, and libyuv makes use of SSE accelerated code when
> the stride have such alignment.
> If we made the stride == width, libyuv would then have to use a slow C code
> path.
> 
> When playing a 60fps video, slowing down what is already a heavy process
> will have a great impact.

My understanding was that libyuv::CopyPlane() uses row width, not stride, to choose copy routine [1].  And when the width is not SIMD aligned, it still makes use of SIMD accelerated code as much as it can, so only the end of the row will be copied using memcpy().

[1] https://searchfox.org/mozilla-central/source/media/libvpx/libvpx/third_party/libyuv/source/planar_functions.cc#51

Comment 32

[Matt Woodrow (:mattwoodrow)]

(In reply to Paul Theriault [:pauljt] from comment #30)
> Correction: paragraph two, sentence two - I mean I'm struggling to
> understand why we dont fix CopyPlane/CopyData instead the calling functions.

I think the behaviour of CopyPlane is what we want, so I'd aruge that it isn't broken.

We should definitely add a runtime assertion that detects that exploitable case (width > stride), so that we crash safely if it happens.

It's really up to the callers to detect it normally though, since only they know what the right rejection behaviour is.

Comment 33

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #32)
> (In reply to Paul Theriault [:pauljt] from comment #30)
> > Correction: paragraph two, sentence two - I mean I'm struggling to
> > understand why we dont fix CopyPlane/CopyData instead the calling functions.
> 
> I think the behaviour of CopyPlane is what we want, so I'd aruge that it
> isn't broken.

Are you saying that comment 12 is wrong then? Or are you talking in a general sense?

> 
> We should definitely add a runtime assertion that detects that exploitable
> case (width > stride), so that we crash safely if it happens.

Do you mean add a runtime assertion to release builds to crash safely in this case? Or to debug builds? Its not much use adding it to debug builds unless we are actually fuzzing to try to hit these cases.

Comment 34

[Matt Woodrow (:mattwoodrow)]

(In reply to Paul Theriault [:pauljt] from comment #33)
 
> Are you saying that comment 12 is wrong then? Or are you talking in a
> general sense?

For the general case, having stride and width specified separately lets us have each row start at an aligned address even for odd numbered widths. We don't want to allocate our destination based on the width, since it'll undo this alignment and potentially give worse performance.

The other bugs mentioned there seem valid, and in the case where copying is converting from interleaved to planar, we probably want to recompute the stride to be something sane for the destination format.


> Do you mean add a runtime assertion to release builds to crash safely in
> this case? Or to debug builds? Its not much use adding it to debug builds
> unless we are actually fuzzing to try to hit these cases.

Right, I meant a release assertion as a last line of defense.

Comment 35

[u480271]

Attachment: Check plane width & stride constraints. r?mattwoodrow

MozReview-Commit-ID: HEcMG4JoEl3

Comment 36

[u480271]

I'm in agreement with :mattwoodrow about the required behaviour for `CopyPlane`. I've added a MOZ_ASSERT_RELEASE to `CopyPlane` and add a check for `width < stride` in `MediaData::ValidatePlane`.

The problem with the `createImageBitmap` code is that there is *no* validation of the values in `ChannelPixelLayout` received from JS. Is this API supposed to be generally accessible? ie. Is it planned that the pref gate be removed?

Comment 37

[u480271]

Attachment: Check plane width & stride constraints. r?mattwoodrow

MozReview-Commit-ID: HEcMG4JoEl3

Comment 38

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8949244 [details] [diff] [review]
Check plane width & stride constraints. r?mattwoodrow

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

::: gfx/layers/ImageContainer.cpp
@@ +595,5 @@
> +  CopyPlane(mData.mYChannel,
> +            aData.mYChannel,
> +            mData.mYSize,
> +            mData.mYStride,
> +            mData.mYSkip);

Don't we want aData.mYSkip here (the skip for the source), since mData.mYSkip is always 0?

Comment 39

[u480271]

Attachment: Check plane width & stride constraints. r=mattwoodrow

MozReview-Commit-ID: HEcMG4JoEl3

Comment 40

[u480271]

(In reply to Matt Woodrow (:mattwoodrow) from comment #38)
> Don't we want aData.mYSkip here (the skip for the source), since
> mData.mYSkip is always 0?

Correct, I messed up.

Comment 41

[u480271]

Attachment: Check plane width & stride constraints. r=mattwoodrow

MozReview-Commit-ID: HEcMG4JoEl3

Comment 42

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #34)
> (In reply to Paul Theriault [:pauljt] from comment #33)
>  
> > Are you saying that comment 12 is wrong then? Or are you talking in a
> > general sense?
> 
> For the general case, having stride and width specified separately lets us
> have each row start at an aligned address even for odd numbered widths. We
> don't want to allocate our destination based on the width, since it'll undo
> this alignment and potentially give worse performance.
> 
> The other bugs mentioned there seem valid, and in the case where copying is
> converting from interleaved to planar, we probably want to recompute the
> stride to be something sane for the destination format.
> 
> 
> > Do you mean add a runtime assertion to release builds to crash safely in
> > this case? Or to debug builds? Its not much use adding it to debug builds
> > unless we are actually fuzzing to try to hit these cases.
> 
> Right, I meant a release assertion as a last line of defense.

Ah great, this approach works for me - thanks!

Comment 43

[u480271]

Attachment: Check plane width & stride constraints. r=mattwoodrow

MozReview-Commit-ID: HEcMG4JoEl3

Comment 44

[u480271]

try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed1e7cbb99544761f2beafea18415948a613a447

Comment 45

[u480271]

Comment on attachment 8950113 [details] [diff] [review]
Check plane width & stride constraints. r=mattwoodrow

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

    This bug, before patch, is dangerous if the imagebitmap extensions are
    enabled, but that requires a pref to be enabled by a user.

    The other code paths to CopyPlane are protected by check
    MediaData::ValidatePlane, which was tighted to assert width < stride. To
    exploit this would require video decoders to return incorrect values,
    which is unlikely, or crafting an incorrect IPDL message from CDM.

    :mattwoodrow suggest this patch could be re-classified as sec-low

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

    The comment hints at an issue with width & stride but it's not obvious how
    to trigger the behaviour.

Which older supported branches are affected by this flaw? 
 
    57, 58, 59, 60

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 the code affected code hasn't changed since 2012.

How likely is this patch to cause regressions; how much testing does it need?
 
    Low, the code is tested by the existing media/video mochitests.

Comment 46

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Dan Glastonbury :kamidphish from comment #45)
> Comment on attachment 8950113 [details] [diff] [review]
> Check plane width & stride constraints. r=mattwoodrow
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> 
>     This bug, before patch, is dangerous if the imagebitmap extensions are
>     enabled, but that requires a pref to be enabled by a user.
> 
>     The other code paths to CopyPlane are protected by check
>     MediaData::ValidatePlane, which was tighted to assert width < stride. To
>     exploit this would require video decoders to return incorrect values,
>     which is unlikely, or crafting an incorrect IPDL message from CDM.
> 
>     :mattwoodrow suggest this patch could be re-classified as sec-low

I don't think this is sec-low. It might not be sec-critical - as I said above, I was changing the criticality because it looked exploitable from the web, even without imagebitmap. I'm not sure if this bug was exploitable or not via video or webrtc paths (at least comment 25 doesn't convince me it safe).  But even assuming this bug was ONLY exploitable via a malformed IPC message, we typically rate these sorts of sandbox escape bugs sec-high, and this one being exploitable from the CDM is rare (and hence more valuable to an attacker). 

My gut feel here is bug is probably only sec-high, due to the sandbox escape, and the difficulty of exploitation via video paths (if thats even possible). At the end of the day it doesn't really matter. Its clearly a bug and we have a fix available.

Comment 47

[Julien Cristau [:jcristau] (back April 22)]

(In reply to Dan Glastonbury :kamidphish from comment #45)
> Which older supported branches are affected by this flaw? 
>  
>     57, 58, 59, 60
> 
> 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 the code affected code hasn't changed since 2012.
> 
If the code hasn't changed since 2012 how come 52 is not affected?

Comment 48

[u480271]

(In reply to Julien Cristau [:jcristau] from comment #47)
> If the code hasn't changed since 2012 how come 52 is not affected?

You're right, 52 is affected. Why does 58 get a "won't fix"?

Comment 49

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

> You're right, 52 is affected. Why does 58 get a "won't fix"?

Because we already shipped 58 and lack a time machine.

sec-approval+ for trunk (60). Please nominate a patch for beta (59) as well.

I'm not sure this is a sec-critical really if this is disabled by a pref by default for users but I'll give the sec-approval anyway.

Comment 50

[u480271]

Attachment: ESR 52 - Check plane width & stride constraints.

MozReview-Commit-ID: 328ETwMdVnq

Comment 51

[u480271]

Comment on attachment 8950113 [details] [diff] [review]
Check plane width & stride constraints. r=mattwoodrow

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1428947
[User impact if declined]: If a user has canvas.imagebitmap_extensions.enabled set, incorrectly specifying image parameters can lead to OOB write.
[Is this code covered by automated tests?]: Correct behaviour, yes, Incorrect behaviour, no. (because it asserts)
[Has the fix been verified in Nightly?]: Yes 
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Small, easily code reviewed change.
[String changes made/needed]: None

Comment 52

[u480271]

Comment on attachment 8951132 [details] [diff] [review]
ESR 52 - Check plane width & stride constraints.

[Approval Request Comment]
This is a sec:{high,crit} bug.

Comment 53

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

Comment on attachment 8950113 [details] [diff] [review]
Check plane width & stride constraints. r=mattwoodrow

Sec-crit, Beta59+

Comment 54

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

Comment on attachment 8951132 [details] [diff] [review]
ESR 52 - Check plane width & stride constraints.

sec-crit, ESR52+

Comment 55

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/bcf52fbf8445

Comment 56

[Julien Cristau [:jcristau] (back April 22)]

Did you also push this to trunk?

Comment 57

[u480271]

I thought this had to be coordinated any would all be checked in together.

Pushing to trunk.

Comment 58

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c26e5d346800ba720f32d0a8f1a872c34f31184
https://hg.mozilla.org/mozilla-central/rev/5c26e5d34680

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/456913d7e8b5

Comment 61

[Daniel Veditz [:dveditz]]

Premature bounty marking, restoring nomination.

Comment 62

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

(In reply to Paul Theriault [:pauljt] from comment #46)
> (In reply to Dan Glastonbury :kamidphish from comment #45)
> > Comment on attachment 8950113 [details] [diff] [review]
> > Check plane width & stride constraints. r=mattwoodrow
> > 
> > [Security approval request comment]
> > How easily could an exploit be constructed based on the patch?
> > 
> >     This bug, before patch, is dangerous if the imagebitmap extensions are
> >     enabled, but that requires a pref to be enabled by a user.
> > 
> >     The other code paths to CopyPlane are protected by check
> >     MediaData::ValidatePlane, which was tighted to assert width < stride. To
> >     exploit this would require video decoders to return incorrect values,
> >     which is unlikely, or crafting an incorrect IPDL message from CDM.
> > 
> >     :mattwoodrow suggest this patch could be re-classified as sec-low
> 
> I don't think this is sec-low. It might not be sec-critical - as I said
> above, I was changing the criticality because it looked exploitable from the
> web, even without imagebitmap.

I'm confused. The devs here says that it isn't exploitable if canvas.imagebitmap_extensions.enabled isn't set and that is not set by default for users. How would this be a sec-critical then?

> I'm not sure if this bug was exploitable or
> not via video or webrtc paths (at least comment 25 doesn't convince me it
> safe).

Do we have any evidence that it is exploitable that way?

> But even assuming this bug was ONLY exploitable via a malformed IPC
> message, we typically rate these sorts of sandbox escape bugs sec-high, and
> this one being exploitable from the CDM is rare (and hence more valuable to
> an attacker). 

 But it is still pref'd off by default, right?
 
> My gut feel here is bug is probably only sec-high, due to the sandbox
> escape, and the difficulty of exploitation via video paths (if thats even
> possible). At the end of the day it doesn't really matter. Its clearly a bug
> and we have a fix available.

 Well, it matters for things like advisories and bounties. We don't award bounties on sec-low bugs and we don't write advisories for things that are disabled by default. 

I'd love a definitive answer from the devs since we're shipping Firefox 59 in a week and I have to figure out if this bug is rated correctly and if it needs a security advisory or not.

Comment 63

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Al Billings [:abillings] from comment #62)
> (In reply to Paul Theriault [:pauljt] from comment #46)
> > (In reply to Dan Glastonbury :kamidphish from comment #45)
> > > Comment on attachment 8950113 [details] [diff] [review]
> > > Check plane width & stride constraints. r=mattwoodrow
> > > 
> > > [Security approval request comment]
> > > How easily could an exploit be constructed based on the patch?
> > > 
> > >     This bug, before patch, is dangerous if the imagebitmap extensions are
> > >     enabled, but that requires a pref to be enabled by a user.
> > > 
> > >     The other code paths to CopyPlane are protected by check
> > >     MediaData::ValidatePlane, which was tighted to assert width < stride. To
> > >     exploit this would require video decoders to return incorrect values,
> > >     which is unlikely, or crafting an incorrect IPDL message from CDM.
> > > 
> > >     :mattwoodrow suggest this patch could be re-classified as sec-low
> > 
> > I don't think this is sec-low. It might not be sec-critical - as I said
> > above, I was changing the criticality because it looked exploitable from the
> > web, even without imagebitmap.
> 
> I'm confused. The devs here says that it isn't exploitable if
> canvas.imagebitmap_extensions.enabled isn't set and that is not set by
> default for users. How would this be a sec-critical then?

Because the vulnerable code in question, highlighted in comment 0, is reachable by many code paths. The 4 main ones are:
 - The createImageBitmap thing, which we dont really care about
 - Via a malformed video (but jya thinks this is actually impossible but I didnt know that when I rated it)
 - possibly via webrtc  (I dont think this was inviestigated but the fix will fix this case if it was vulnerable)
 - via a malformed IPC message from the GMP sandbox (requires an additional exploit to get code exec in sandbox first)

ONLY the last one has been confirmed to be an issue. I'll be honest, I'm not convinced that _all_ other paths are safe, but it doesn't really matter, its easier just to fix it. 

> 
> > I'm not sure if this bug was exploitable or
> > not via video or webrtc paths (at least comment 25 doesn't convince me it
> > safe).
> 
> Do we have any evidence that it is exploitable that way?

Only from code review. The only one Im sure about is the sandbox escape But it IS clear that this function is unsafe, and the only comment which address this is comment 25, but that seem to miss the point of the vulnerability.  

> 
> > But even assuming this bug was ONLY exploitable via a malformed IPC
> > message, we typically rate these sorts of sandbox escape bugs sec-high, and
> > this one being exploitable from the CDM is rare (and hence more valuable to
> > an attacker). 
> 
>  But it is still pref'd off by default, right?

The pref only affect the createImageBitmap vector. The sandbox escape is valid regardless.

>  
> > My gut feel here is bug is probably only sec-high, due to the sandbox
> > escape, and the difficulty of exploitation via video paths (if thats even
> > possible). At the end of the day it doesn't really matter. Its clearly a bug
> > and we have a fix available.
> 
>  Well, it matters for things like advisories and bounties. We don't award
> bounties on sec-low bugs and we don't write advisories for things that are
> disabled by default. 
> 
> I'd love a definitive answer from the devs since we're shipping Firefox 59
> in a week and I have to figure out if this bug is rated correctly and if it
> needs a security advisory or not.

The sandbox escape is definitely valid. jya thinks the video path is not exploitable and In not sure anyone looked at the webrtc path. So assuming there ISNT a web exploitable vector here (we dont have proof of one in any case), I would suggest that you rate this bug based on the sandbox escape, which I think is the main issue here.

Comment 64

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS Al if you need a description for advisory,  I'd suggest:

A lack of parameter validation on IPC messages results in a potential out-of-bounds write. This could potentially allow for sandbox escape via memory corruption in the parent process.