Bug 1306883 (CVE-2017-5377)

SEGV in sk_memset32 from SkLinearGradient::LinearGradientContext::shade4_dx_clamp

VERIFIED FIXED in Firefox 51

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: attekett, Assigned: lsalzman)

Tracking

({csectype-bounds, regression, sec-critical})

49 Branch
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
qe-verify +

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr45 unaffected, firefox50+ wontfix, firefox51+ verified, firefox52+ verified)

Details

(Whiteboard: [gfx-noted][post-critsmash-triage][adv-main51+])

Attachments

(2 attachments)

Reporter

Description

3 years ago
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
Group: core-security → gfx-core-security
I believe this has been logged and fixed up stream.

https://bugs.chromium.org/p/skia/issues/detail?id=5821
Tyson, I don't have a permission to access this bug; can you give us some detail on the fix?
Flags: needinfo?(twsmith)
Assignee: nobody → lsalzman
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.
I don't seems to be able to add anyone to the bug.

It is fixed by https://chromium.googlesource.com/skia/+/088e21ba652ceaa4abb4ba8cdd2ec1bc8afc32ed
Flags: needinfo?(twsmith)
Reporter

Comment 5

3 years ago
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.
Assignee

Comment 6

3 years ago
NI'ing myself to backport this during/after the Skia update.
Flags: needinfo?(lsalzman)
Assignee

Comment 7

3 years ago
(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.
(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.
Reporter

Comment 9

3 years ago
Tyson: So you found this bug on your own also?
Flags: sec-bounty?
(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.
We probably need to take a Skia update.
Assignee

Comment 12

3 years ago
The Skia update has landed, so 52 is safe for now.  It's only earlier versions that are going to still be affected.
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.
Assignee

Comment 14

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

Updated

3 years ago
Flags: needinfo?(lsalzman)
Assignee

Updated

3 years ago
Has Regression Range: --- → yes
Has STR: --- → yes
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → 49 Branch
Assignee

Comment 15

3 years ago
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.
Attachment #8804984 - Flags: review?(mchang)
Attachment #8804984 - Flags: review?(mchang) → review+
Assignee

Comment 16

3 years ago
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.
Attachment #8804984 - Flags: sec-approval?
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.
Flags: needinfo?(lsalzman)
Assignee

Comment 18

3 years ago
(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.
Flags: needinfo?(lsalzman)
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.
Attachment #8804984 - Flags: sec-approval? → sec-approval+
Assignee

Comment 20

3 years ago
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
Attachment #8804984 - Flags: approval-mozilla-aurora?
Can you go ahead and land this now that it has approval? Thanks.
Flags: needinfo?(lsalzman)
Oh, I see, this just needs an uplift of this patch to Aurora. Maybe marking it fixed will get it on somebody's list.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(lsalzman)
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Attachment #8804984 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
Target Milestone: --- → mozilla51
[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.
Blocks: 1265131
Flags: sec-bounty? → sec-bounty+
Keywords: regression
Group: gfx-core-security → core-security-release
Assignee

Comment 26

3 years ago
(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.
Flags: qe-verify+
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main51+]
Alias: CVE-2017-5377
Hi Atte, can you take a look and see if this fixes the issue for you? Thank you.
Flags: needinfo?(attekett)
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.
Status: RESOLVED → VERIFIED
Reporter

Comment 29

2 years ago
The crash doesn't reproduce anymore on Nightly. Also I'm not see this stack trace on my fuzzing cluster anymore.
Flags: needinfo?(attekett)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.