Closed Bug 1282246 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Canvas: 2D, defect, critical)

50 Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- unaffected
firefox50 + fixed

People

(Reporter: tsmith, Assigned: lsalzman)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, sec-high, testcase, Whiteboard: [gfx-noted][adv-main48+])

Attachments

(6 files)

Attached file stack_trace.txt
This was found while fuzzing an inbound ASan build from June 25th.
Attached file test_case.html
Assignee: nobody → mchang
Whiteboard: [gfx-noted]
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.
Assignee: mchang → lsalzman
Status: NEW → ASSIGNED
Attachment #8765601 - Flags: review?(mchang)
Attachment #8765601 - Flags: review?(mchang) → review+
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.
Attachment #8765601 - Flags: sec-approval?
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.
Attachment #8765601 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/052416b4789f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I missed the lack of a security rating in this. It needs one.
Flags: needinfo?(lsalzman)
Flags: needinfo?(dveditz)
(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?
Flags: needinfo?(lsalzman)
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
Ah, I might have tested against a fixed release build already which would explain the lack of a crash...
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
Attachment #8765601 - Flags: approval-mozilla-aurora?
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
Attachment #8767226 - Flags: review+
Attachment #8767226 - Flags: approval-mozilla-beta?
Tyson, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(twsmith)
Verified. I also ran 12h of fuzzing last night with no sign of this issue.
Status: RESOLVED → VERIFIED
Flags: needinfo?(twsmith)
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.
Attachment #8767226 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(dveditz)
Keywords: sec-high
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')
Flags: needinfo?(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.
Flags: needinfo?(lsalzman) → needinfo?(cbook)
(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 :(
Flags: needinfo?(cbook)
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.
Flags: needinfo?(cbook)
Attachment #8768461 - Flags: review+
(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
Flags: needinfo?(cbook)
Group: gfx-core-security → core-security-release
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.
Attachment #8765601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [gfx-noted] → [gfx-noted][adv-main48+]
Blocks: 1289929, grizzly
Flags: in-testsuite?
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.