1371689 - Update convolver.cpp and related imported Chromium Source Code

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tom Ritter [:tjr]]

Long ago in Bug 486918 we imported Chromium Source code related to skia (but not actually part of skia). This may not have been updated 5 or so years.

It's not going to be a trivial update, as the files have diverged a bit, but there's not a ton of commits in between at least.  

We're also going to add these files to the third party library alert service.

(Until I have gone through the logs on this I'm going to flag it as a potential security item)

Comment 1

[Tom Ritter [:tjr]]

It has been kind-of integrated into the Third Party Library Alert system. (https://github.com/mozilla-services/third-party-library-alert/commit/d2449c09a1d68eaacdf55dd9494f756082ac1251 ) Because there is no README.mozilla that contains the latest commit imported from Chromium, I cannot accurately retrieve what current upstream version is in the tree.

When this bug is fixed I would request that we add a READEME.mozilla that contains the DATE (I can parse any machine readable format, such as '2012-08-23 15:36' as long as the format doesn't change) that we last imported from Chromium. I will generate a new bug when a new commit happens upstream, and if we close that bug as WONTFIX I can mark that date as 'ignore anything in upstream older than this'.

Comment 2

[Tom Ritter [:tjr]]

This is a (semi-)automated bug making you aware that there is an available upgrade for an embedded third-party library. You can leave this bug open, and it will be updated if a newer version of the library becomes available. If you close it as WONTFIX, please indicate if you do not wish to receive any future bugs upon new releases of the library.

image-operations is currently at version 2012-08-23 15:36:00 in mozilla-central, and the latest version of the library released is 2017-06-06 18:26:58. 

I fetched the latest version of the library from https://chromium.googlesource.com/chromium/src.git/+log/master/skia/ext/image_operations.cc.


-----------------------
Most Recent Commit Message for image_operations.h:
Remove Lanczos2 filters
-----------------------
Most Recent Commit Message for image_operations.cc:
Fix a DCHECK ordering problem in ImageOperations.
-----------------------
Most Recent Commit Message for convolver.cc:
Cleanup: Remove some unneeded SSE2 checks and unused code. (try 2)
-----------------------
Most Recent Commit Message for convolver.h:
Switch to standard integer types in skia/.
-----------------------
Most Recent Commit Message for convolver_SSE2.cc:
Switch to standard integer types in skia/.
-----------------------
Most Recent Commit Message for convolver_SSE2.h:
(Patch by Teodora Novkovic <[e-mail]>, originally reviewed at  https://codereview.chromium.org/14929006/).

Comment 3

[Tom Ritter [:tjr]]

I reviewed the commit logs for the files. I identified the following diffs that may be security concerns.

In general, a lot of code has been removed, so there is no telling if there were errors in that code we are still living with.

https://chromium.googlesource.com/chromium/src.git/+/c1cbafd6e83b8360c326a3e539064494dbc880c0%5E%21/#F0
Attached to a as-yet-unviewed security bug (trying to get access)

https://chromium.googlesource.com/chromium/src.git/+/813432d35b6d559737d6f7995fa8124168f8b553%5E%21/#F0
Probably not security related?

https://chromium.googlesource.com/chromium/src.git/+/61076179916109d927541bd93cf888a7f858f645%5E%21/#F4
Missing va_end call - not sure if this can be exploited, just a bug, or neither

https://chromium.googlesource.com/chromium/src.git/+/be44c2b278ccbb8602ff254806e160e606661c3b%5E%21/#F0
Avoiding error cases?

Jeff, can you review these (mostly the first one) and see if we need to bring in their patch? I confirmed we have code in-tree that does not apply the patch. If so, I think we should open a new security bug and make that patch individually, while leaving this for upstreaming the rest of the changed.

Comment 4

[Tom Ritter [:tjr]]

Chrome's bug is now public, it's a heap buffer overflow: https://bugs.chromium.org/p/chromium/issues/detail?id=247081

Comment 5

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

Milan, can we get someone to dig through the "sad history of this file" as Andrew McCreight put it?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Last time we updated from upstream was bug 1180225, in Sep. 2015.  Let's see if we can refresh.  Lee, can you take a look?

Comment 7

[Lee Salzman [:lsalzman]]

I looked through the code and change logs. In so far as it matters, it appears this code is actually up to date. Jeff updated this in 2015. What small changes happened since then don't really apply otherwise.

Comment 8

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #7)
> I looked through the code and change logs. In so far as it matters, it
> appears this code is actually up to date. Jeff updated this in 2015. What
> small changes happened since then don't really apply otherwise.

It should also be noted there is no actual security issue here.

Comment 9

[Tom Ritter [:tjr]]

(In reply to Lee Salzman [:lsalzman] from comment #8)
> (In reply to Lee Salzman [:lsalzman] from comment #7)
> > I looked through the code and change logs. In so far as it matters, it
> > appears this code is actually up to date. Jeff updated this in 2015. What
> > small changes happened since then don't really apply otherwise.
> 
> It should also be noted there is no actual security issue here.

I'm confused by this. Comparing 
https://chromium.googlesource.com/chromium/src.git/+/master/skia/ext/image_operations.cc#331
and 
https://dxr.mozilla.org/mozilla-central/source/gfx/2d/image_operations.cpp#316

Shows Chrome's code with `if (!source.readyToDraw() || source.colorType() != kN32_SkColorType)` and ours with `if (!source.readyToDraw())` which is the fix they added in https://chromium.googlesource.com/chromium/src.git/+/c1cbafd6e83b8360c326a3e539064494dbc880c0%5E%21/#F0 which is attached to the Heap Buffer Overflow: https://bugs.chromium.org/p/chromium/issues/detail?id=247081

There were a number of small changes made otherwise (a lot of deleting and refactoring around SSE2 and MIPS stuff) - I didn't look at them too closely but I think the above is a change we need to take...

Comment 10

[Lee Salzman [:lsalzman]]

(In reply to Tom Ritter [:tjr] from comment #9)
> (In reply to Lee Salzman [:lsalzman] from comment #8)
> > (In reply to Lee Salzman [:lsalzman] from comment #7)
> > > I looked through the code and change logs. In so far as it matters, it
> > > appears this code is actually up to date. Jeff updated this in 2015. What
> > > small changes happened since then don't really apply otherwise.
> > 
> > It should also be noted there is no actual security issue here.
> 
> I'm confused by this. Comparing 
> https://chromium.googlesource.com/chromium/src.git/+/master/skia/ext/
> image_operations.cc#331
> and 
> https://dxr.mozilla.org/mozilla-central/source/gfx/2d/image_operations.
> cpp#316
> 
> Shows Chrome's code with `if (!source.readyToDraw() || source.colorType() !=
> kN32_SkColorType)` and ours with `if (!source.readyToDraw())` which is the
> fix they added in
> https://chromium.googlesource.com/chromium/src.git/+/
> c1cbafd6e83b8360c326a3e539064494dbc880c0%5E%21/#F0 which is attached to the
> Heap Buffer Overflow:
> https://bugs.chromium.org/p/chromium/issues/detail?id=247081
> 
> There were a number of small changes made otherwise (a lot of deleting and
> refactoring around SSE2 and MIPS stuff) - I didn't look at them too closely
> but I think the above is a change we need to take...

We're unaffected by this. We only call ImageOperations::Resize from one place, and that place already validates the format: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Scale.cpp#32

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: reuse Skia's SkConvolutionFilter1D instead of forked code from Chromium

Since the original convolution filter code was forked from Skia by Chromium, and the code exists in shinier/newer fashion inside Skia still, it makes more sense to just use Skia's code for this instead of the fork of the fork that we have now.

This lets us delete a pretty huge chunk of nasty code, and we gain free ARM NEON optimizations as a bonus.

Only small snag is that I had isolate the parts where I reach into Skia's guts to do it into this ConvolutionFilter wrapper class. I also needed to borrow the ComputeResizeFilter plumbing since there was no clean way of using Skia's own on that, but since we already were doing that in our fork-of-a-fork, no real loss.

Comment 12

[Lee Salzman [:lsalzman]]

Can we remove the sec-critical from this since this does not actually affect us at all?

Comment 13

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

Sure, I'll make this a sec-audit.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eafa5fdcb767

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Per IRC discussion with Lee, the risks of uplifting this outweigh any rewards from doing so, so let's let it ride the trains.