Update convolver.cpp and related imported Chromium Source Code

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tjr, Assigned: lsalzman)

Tracking

({sec-audit})

Trunk
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5255+ wontfix, firefox54 wontfix, firefox55+ wontfix, firefox56+ fixed)

Details

(Whiteboard: [third-party-lib-audit][adv-main56-][post-critsmash-triage])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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)
(Reporter)

Comment 1

2 years ago
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'.
(Reporter)

Comment 2

2 years ago
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 <teodora.petrovic@gmail.com>, originally reviewed at  https://codereview.chromium.org/14929006/).
(Reporter)

Comment 3

2 years ago
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.
Flags: needinfo?(jmuizelaar)
(Reporter)

Comment 4

2 years ago
Chrome's bug is now public, it's a heap buffer overflow: https://bugs.chromium.org/p/chromium/issues/detail?id=247081
Group: core-security → gfx-core-security
Milan, can we get someone to dig through the "sad history of this file" as Andrew McCreight put it?
Flags: needinfo?(milan)
Keywords: sec-critical
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?
Assignee: nobody → lsalzman
Flags: needinfo?(milan) → needinfo?(lsalzman)
(Reporter)

Updated

2 years ago
Whiteboard: [third-party-lib-audit]
(Assignee)

Comment 7

2 years ago
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.
Flags: needinfo?(lsalzman)
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(jmuizelaar)
(Reporter)

Comment 9

2 years ago
(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...
(Assignee)

Comment 10

2 years ago
(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
(Assignee)

Comment 11

2 years ago
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.
Attachment #8880019 - Flags: review?(jmuizelaar)
Attachment #8880019 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 12

2 years ago
Can we remove the sec-critical from this since this does not actually affect us at all?
Flags: needinfo?(abillings)
Sure, I'll make this a sec-audit.
Flags: needinfo?(abillings)
Keywords: sec-criticalsec-audit
https://hg.mozilla.org/mozilla-central/rev/eafa5fdcb767
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Per IRC discussion with Lee, the risks of uplifting this outweigh any rewards from doing so, so let's let it ride the trains.
Group: gfx-core-security → core-security-release
(Assignee)

Updated

2 years ago
Depends on: 1375842

Updated

2 years ago
Depends on: 1389733
Whiteboard: [third-party-lib-audit] → [third-party-lib-audit][adv-main56-]
Flags: qe-verify-
Whiteboard: [third-party-lib-audit][adv-main56-] → [third-party-lib-audit][adv-main56-][post-critsmash-triage]
Depends on: 1421191
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.