1282246 - SEGV on unknown address [@fill] in src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: stack_trace.txt

This was found while fuzzing an inbound ASan build from June 25th.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: ASAN Mozconfig for Linux

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: don't use gradients with non-finite endpoints in DrawTargetSkia

The test case essentially passes in Inf as one of the gradient's endpoints. Skia does not like this because it causes its internal math to generate NaNs and trigger assertions.

The canvas spec's only guidance about endpoints of a gradient is that if they are equal, nothing should be painted. Let's extend this logic such that if the endpoints are not sane, let's not paint anything either.

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 8765601 [details] [diff] [review]
don't use gradients with non-finite endpoints in DrawTargetSkia

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Fairly easy to cause the browser to trigger the assertion in debug builds, but it has no real other consequences than this. 

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The comments in the patch do not specify what the consequences of specifying a non-finite endpoint to a gradient are, but only that we should skip them.

> Which older supported branches are affected by this flaw?
As of bug 1082598 landing, so 46+

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

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should likely apply against all affected branches, and it is simple to fix if not the case.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. It just intercepts a case where we're triggering an assertion and touches little else.

Comment 5

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

Comment on attachment 8765601 [details] [diff] [review]
don't use gradients with non-finite endpoints in DrawTargetSkia

sec-approval+ for trunk.

Please make and/or nominate Aurora and Beta patches as well to land after this makes it onto trunk.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/052416b4789f

Comment 7

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

I missed the lack of a security rating in this. It needs one.

Comment 8

[Lee Salzman [:lsalzman]]

(In reply to Al Billings [:abillings] from comment #7)
> I missed the lack of a security rating in this. It needs one.

Should be sec-moderate at worst since it is just a debug-only assertion, right?

Comment 9

[Tyson Smith [:tsmith]]

No this is not an assertion. This is a SEGV and it should be reproducible on non-ASan, non-debug builds. Unless I am missing something.

==8396==ERROR: AddressSanitizer: SEGV on unknown address 0x619e00286884 (pc 0x7fe06fcf9698 bp 0x7ffc33f04db0 sp 0x7ffc33f04d20 T0)
    #0 0x7fe06fcf9697 in sk_memset32 /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/core/SkUtils.h:31:19
    #1 0x7fe06fcf9697 in fill<false> /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:538
    #2 0x7fe06fcf9697 in void SkLinearGradient::LinearGradientContext::shade4_dx_clamp<false, false>(unsigned int*, int, float, float, float, float const*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:651
    #3 0x7fe06fcec594 in SkLinearGradient::LinearGradientContext::shade4_clamp(int, int, unsigned int*, int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:750:13
    #4 0x7fe06fcec05a in SkLinearGradient::LinearGradientContext::shadeSpan(int, int, unsigned int*, int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp:280:9
    #5 0x7fe06fa48c39 in SkARGB32_Shader_Blitter::blitAntiH(int, int, unsigned char const*, short const*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/core/SkBlitter_ARGB32.cpp:500:21
    #6 0x7fe07014d445 in SuperBlitter::flush() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/core/SkScan_AntiPath.cpp:169:13
    #7 0x7fe07014ff98 in ~SuperBlitter /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/skia/skia/src/core/SkScan_AntiPath.cpp:108:9

Comment 10

[Lee Salzman [:lsalzman]]

Ah, I might have tested against a fixed release build already which would explain the lack of a crash...

Comment 11

[Lee Salzman [:lsalzman]]

Comment on attachment 8765601 [details] [diff] [review]
don't use gradients with non-finite endpoints in DrawTargetSkia

Approval Request Comment
[Feature/regressing bug #]: Bug 1082598
[User impact if declined]: Canvas can be used to crash browser.
[Describe test coverage new/current, TreeHerder]: Reftests
[Risks and why]: Low, only skips rendering of gradients that have values that can't be properly rendered anyway.
[String/UUID change made/needed]: None

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: don't use gradients with non-finite endpoints in DrawTargetSkia (beta)

Rebased the patch against beta so it can be uplifted. No significant differences.

Approval Request Comment
[Feature/regressing bug #]: Bug 1082598
[User impact if declined]: Canvas can be used to crash browser.
[Describe test coverage new/current, TreeHerder]: Reftests
[Risks and why]: Low, only skips rendering of gradients that have values that can't be properly rendered anyway.
[String/UUID change made/needed]: None

Comment 13

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

Tyson, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 14

[Tyson Smith [:tsmith]]

Verified. I also ran 12h of fuzzing last night with no sign of this issue.

Comment 15

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

Comment on attachment 8767226 [details] [diff] [review]
don't use gradients with non-finite endpoints in DrawTargetSkia (beta)

Crash fix that was verified, seems safe to uplift to Beta48.

Comment 16

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

(In reply to Tyson Smith [:tsmith] from comment #9)
> No this is not an assertion. This is a SEGV and it should be reproducible on
> non-ASan, non-debug builds. Unless I am missing something.

It didn't crash for me on Mac with a debug or release nightly from a few days before you filed this bug. Still, a wild memset is no good.

Comment 17

[Carsten Book [:Tomcat]]

has problems to apply to beta like:

merging gfx/2d/DrawTargetSkia.cpp
warning: conflicts while merging gfx/2d/DrawTargetSkia.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 18

[Lee Salzman [:lsalzman]]

(In reply to Carsten Book [:Tomcat] from comment #17)
> has problems to apply to beta like:
> 
> merging gfx/2d/DrawTargetSkia.cpp
> warning: conflicts while merging gfx/2d/DrawTargetSkia.cpp! (edit, then use
> 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

Did you use the beta version of the patch? I am confused, I just checked right now with a freshly updated beta, and there were no conflicts.

Comment 19

[Carsten Book [:Tomcat]]

(In reply to Lee Salzman [:lsalzman] from comment #18)
> (In reply to Carsten Book [:Tomcat] from comment #17)
> > has problems to apply to beta like:
> > 
> > merging gfx/2d/DrawTargetSkia.cpp
> > warning: conflicts while merging gfx/2d/DrawTargetSkia.cpp! (edit, then use
> > 'hg resolve --mark')
> > abort: unresolved conflicts, can't continue
> > (use 'hg resolve' and 'hg graft --continue')
> 
> Did you use the beta version of the patch? I am confused, I just checked
> right now with a freshly updated beta, and there were no conflicts.

yeah used the beta patch version :(

Comment 20

[Lee Salzman [:lsalzman]]

Attachment: don't use gradients with non-finite endpoints in DrawTargetSkia (beta)

Does this version apply? I rebased it yet again just in case, though the patch is basically exactly the same, minus the parent/node ids.

Otherwise, I'm pretty stumped if this doesn't apply, as it is applying fine locally no matter what I do.

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Lee Salzman [:lsalzman] from comment #20)
> Created attachment 8768461 [details] [diff] [review]
> don't use gradients with non-finite endpoints in DrawTargetSkia (beta)
> 
> Does this version apply? I rebased it yet again just in case, though the
> patch is basically exactly the same, minus the parent/node ids.
> 
> Otherwise, I'm pretty stumped if this doesn't apply, as it is applying fine
> locally no matter what I do.

yeah this one works great

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

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8765601 [details] [diff] [review]
don't use gradients with non-finite endpoints in DrawTargetSkia

This should also land on aurora - it's already on other branches.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/16de5bb4010b