1306883 - (CVE-2017-5377) SEGV in sk_memset32 from SkLinearGradient::LinearGradientContext::shade4_dx_clamp

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[Atte Kettunen]

Attachment: firefox-SEGV-6f9-6f9-6f9-6f4-1ba0-min.html

Tested on:

OS: Ubuntu 16.04.1 LTS

Firefox: ASAN-build moz_source_stamp: f713114b8c8d352b668b3e8052bc51ece4df34e0

prefs.js from https://github.com/MozillaSecurity/fuzzdata/blob/master/settings/firefox/prefs.js

ASAN-trace:

$./firefox firefox-SEGV-6f9-6f9-6f9-6f4-1ba0-min.html 
ATTENTION: default value of option force_s3tc_enable overridden by environment.
ATTENTION: default value of option force_s3tc_enable overridden by environment.
ASAN:DEADLYSIGNAL
=================================================================
==30297==ERROR: AddressSanitizer: SEGV on unknown address 0x7f07ea7d9628 (pc 0x7f0a1fac92aa bp 0x7fff76db3950 sp 0x7fff76db38c0 T0)
    #0 0x7f0a1fac92a9 in sk_memset32 /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkUtils.h:31:19
    #1 0x7f0a1fac92a9 in fill<false> /home/worker/workspace/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:538
    #2 0x7f0a1fac92a9 in void SkLinearGradient::LinearGradientContext::shade4_dx_clamp<false, false>(unsigned int*, int, float, float, float, float const*) /home/worker/workspace/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:651
    #3 0x7f0a1fabc2a4 in SkLinearGradient::LinearGradientContext::shade4_clamp(int, int, unsigned int*, int) /home/worker/workspace/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:750:13
    #4 0x7f0a1fabbd6a in SkLinearGradient::LinearGradientContext::shadeSpan(int, int, unsigned int*, int) /home/worker/workspace/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:280:9
    #5 0x7f0a1f8152d4 in SkARGB32_Shader_Blitter::blitRect(int, int, int, int) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkBlitter_ARGB32.cpp:435:17
    #6 0x7f0a1f86643f in antifilldot8(int, int, int, int, SkBlitter*, bool) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Antihair.cpp:678:17
    #7 0x7f0a1f864c44 in antifillrect /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Antihair.cpp:692:5
    #8 0x7f0a1f864c44 in antifillrect /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Antihair.cpp:765
    #9 0x7f0a1f864c44 in SkScan::AntiFillRect(SkRect const&, SkRegion const*, SkBlitter*) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Antihair.cpp:788
    #10 0x7f0a1f86508f in SkScan::AntiFillRect(SkRect const&, SkRasterClip const&, SkBlitter*) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkScan_Antihair.cpp:807:9
    #11 0x7f0a1fd03658 in SkDraw::drawRect(SkRect const&, SkPaint const&, SkMatrix const*, SkRect const*) const /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:877:21
    #12 0x7f0a1fa48072 in SkCanvas::onDrawRect(SkRect const&, SkPaint const&) /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkCanvas.cpp:2109:9
    #13 0x7f0a18029068 in mozilla::gfx::DrawTargetSkia::FillRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&) /home/worker/workspace/build/src/gfx/2d/DrawTargetSkia.cpp:668:3
    #14 0x7f0a186a3a4e in gfxContext::FillAzure(mozilla::gfx::Pattern const&, float) /home/worker/workspace/build/src/gfx/thebes/gfxContext.cpp:1069:5
    #15 0x7f0a186a3428 in gfxContext::Fill(mozilla::gfx::Pattern const&) /home/worker/workspace/build/src/gfx/thebes/gfxContext.cpp:200:3
    #16 0x7f0a18698ab3 in gfxContext::Fill() /home/worker/workspace/build/src/gfx/thebes/gfxContext.cpp:192:3
    #17 0x7f0a1d1514d5 in nsCSSRendering::PaintGradient(nsPresContext*, nsRenderingContext&, nsStyleGradient*, nsRect const&, nsRect const&, nsRect const&, nsSize const&, mozilla::gfx::IntRectTyped<mozilla::CSSPixel> const&, nsSize const&) /home/worker/workspace/build/src/layout/base/nsCSSRendering.cpp:3026:7
    #18 0x7f0a1d167c3b in nsImageRenderer::Draw(nsPresContext*, nsRenderingContext&, nsRect const&, nsRect const&, nsRect const&, nsPoint const&, nsSize const&, mozilla::gfx::IntRectTyped<mozilla::CSSPixel> const&) /home/worker/workspace/build/src/layout/base/nsCSSRendering.cpp:5472:7
    #19 0x7f0a1d145f7a in DrawBackground /home/worker/workspace/build/src/layout/base/nsCSSRendering.cpp:5574:10
.
.
.
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/gfx/skia/skia/src/core/SkUtils.h:31:19 in sk_memset32
==30297==ABORTING

Comment 1

[Tyson Smith [:tsmith]]

I believe this has been logged and fixed up stream.

https://bugs.chromium.org/p/skia/issues/detail?id=5821

Comment 2

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

Tyson, I don't have a permission to access this bug; can you give us some detail on the fix?

Comment 3

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

Depending on the complexity of the fix, we may choose to fix (and uplift) just the fix, without waiting for the Skia update that has it.

Comment 4

[Tyson Smith [:tsmith]]

I don't seems to be able to add anyone to the bug.

It is fixed by https://chromium.googlesource.com/skia/+/088e21ba652ceaa4abb4ba8cdd2ec1bc8afc32ed

Comment 5

[Atte Kettunen]

If you manage to give/get access to that bug, I would also like to have access. I'm interested to see the time table of the bug.

Comment 6

[Lee Salzman [:lsalzman]]

NI'ing myself to backport this during/after the Skia update.

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #6)
> NI'ing myself to backport this during/after the Skia update.

Ah, this is already rolled into the Skia m55 update, so it will be fixed when I land the update. So it's just a question of uplifting.

Comment 8

[Tyson Smith [:tsmith]]

(In reply to Atte Kettunen from comment #5)
> If you manage to give/get access to that bug, I would also like to have
> access. I'm interested to see the time table of the bug.

Atte, I logged that bug on Oct 4th.

Comment 9

[Atte Kettunen]

Tyson: So you found this bug on your own also?

Comment 10

[Tyson Smith [:tsmith]]

(In reply to Atte Kettunen from comment #9)
> Tyson: So you found this bug on your own also?

Yes, but 3 days after you logged it.

Comment 11

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

We probably need to take a Skia update.

Comment 12

[Lee Salzman [:lsalzman]]

The Skia update has landed, so 52 is safe for now.  It's only earlier versions that are going to still be affected.

Comment 13

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

Is there a reason why we wouldn't want to at least land this in Aurora as well? Otherwise, this isn't going to show up in a public release until well into next year.

Comment 14

[Lee Salzman [:lsalzman]]

(In reply to Al Billings [:abillings] from comment #13)
> Is there a reason why we wouldn't want to at least land this in Aurora as
> well? Otherwise, this isn't going to show up in a public release until well
> into next year.

I will look into backporting the upstream patch today or tomorrow.

Comment 15

[Lee Salzman [:lsalzman]]

Attachment: check for finite transforms when creating Skia gradient contexts

Nothing fancy. This is a straight backport of the upstream fix: https://chromium.googlesource.com/skia/+/088e21ba652ceaa4abb4ba8cdd2ec1bc8afc32ed

This patch will apply against aurora, beta, and release, since the bug was introduced with the update to Skia's m51 branch in 49, cf: bug 1265131.

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 8804984 [details] [diff] [review]
check for finite transforms when creating Skia gradient contexts

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The patch does hint that their is an issue with non-finite transforms used to make gradients. However, it does not directly imply that this could be used to overrun the destination bounds.

Further, the fix is already public in Skia's upstream repository, so the cat is somewhat out of the bag.

> Which older supported branches are affected by this flaw?
49+

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

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The Skia m55 update (bug 1299435) already includes the fix for this issue. So we just need to apply this patch to the older branches.

> How likely is this patch to cause regressions; how much testing does it need?
This patch is already deployed in nightly and is not causing any problems there. Also, it does not really introduce semantic changes, just implements a bit more validation on instantiating gradients, so should be fairly innocuous.

Comment 17

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

If we take the update to Skia on Trunk and Aurora in Bug 1299435 (which we should) and it is too late for Beta changes (which it is), is there any point in taking this bug anywhere? We're at the end of the 50 (Beta) cycle now and we aren't taking new changes there without very good reasons. We'd have to get release management to approve taking this on Beta.

Comment 18

[Lee Salzman [:lsalzman]]

(In reply to Al Billings [:abillings] from comment #17)
> If we take the update to Skia on Trunk and Aurora in Bug 1299435 (which we
> should) and it is too late for Beta changes (which it is), is there any
> point in taking this bug anywhere? We're at the end of the 50 (Beta) cycle
> now and we aren't taking new changes there without very good reasons. We'd
> have to get release management to approve taking this on Beta.

This Skia update was unfortunately not a simple "update", requiring some substantial changes to make things work due to a lot of API reorganization. So I don't think it's feasible to uplift the entire update to aurora. I am rather in favor of just doing this security spot fix for aurora instead.

As for whether or not management wants to uplift the security fix to beta, I will defer that decision to them.

Comment 19

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

Comment on attachment 8804984 [details] [diff] [review]
check for finite transforms when creating Skia gradient contexts

Giving sec-approval. Let's do an Aurora patch as well and get it nominated.

Comment 20

[Lee Salzman [:lsalzman]]

Comment on attachment 8804984 [details] [diff] [review]
check for finite transforms when creating Skia gradient contexts

Approval Request Comment
[Feature/regressing bug #]: Bug 1265131
[User impact if declined]: Potential memory overrun exploits from both content and canvas.
[Describe test coverage new/current, TreeHerder]: layout/reftests/css-gradients
[Risks and why]: Already deployed without incident in nightly as part of the Skia m55 update in bug 1299435, so shouldn't cause trouble. Patch is just concerned with validating dangerous inputs and preventing them from getting to downstream code, without adding any new behavior.
[String/UUID change made/needed]: None

Comment 21

[Andrew McCreight [:mccr8]]

Can you go ahead and land this now that it has approval? Thanks.

Comment 22

[Andrew McCreight [:mccr8]]

Oh, I see, this just needs an uplift of this patch to Aurora. Maybe marking it fixed will get it on somebody's list.

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8804984 [details] [diff] [review]
check for finite transforms when creating Skia gradient contexts

Fix a sec-critical. Take it in 51 aurora.

Comment 24

[Carsten Book [:Tomcat]]

(In reply to Andrew McCreight [:mccr8] from comment #22)
> Oh, I see, this just needs an uplift of this patch to Aurora. Maybe marking
> it fixed will get it on somebody's list.

yep worked :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffd987010139

Comment 25

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
We need to consider taking this and other Skia fixes in 50.1, otherwise they don't ship until late January and they are essentially public in Google's repo.

Comment 26

[Lee Salzman [:lsalzman]]

(In reply to Daniel Veditz [:dveditz] from comment #25)
> [Tracking Requested - why for this release]:
> We need to consider taking this and other Skia fixes in 50.1, otherwise they
> don't ship until late January and they are essentially public in Google's
> repo.

I just verified that the patch applies as-is against release, so should be okay to use it.

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Atte, can you take a look and see if this fixes the issue for you? Thank you.

Comment 28

[Andrei Vaida [:avaida]]

Reproduced the bug on an affected ASAN build (52.0a1, 20161001062357) using the testcase from Comment 0 on Ubuntu 16.04 x64.

This is verified fixed on:

    - 51.1.0 (ASAN, 20170125043835)
    - 52.0b1 (ASAN, 20170124174548)
  
  
* Note that regular builds did not show this issue in the first place and the latest regular ones are not showing it either.

Comment 29

[Atte Kettunen]

The crash doesn't reproduce anymore on Nightly. Also I'm not see this stack trace on my fuzzing cluster anymore.