Skia: stack-buffer-overread in SkLinearGradient::LinearGradient4fContext::LinearIntervalProcessor

RESOLVED FIXED in Firefox 50

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tsmith, Assigned: lsalzman)

Tracking

(4 keywords)

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 fixed, firefox51 fixed, firefox52 fixed, firefox53+ fixed)

Details

(Whiteboard: [adv-main50.1+])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
I found this using the fuzzer in the skia repo. The only test case available is for use with the skia api fuzzer. I believe this issue affects Firefox.

I reported the issue upstream as https://bugs.chromium.org/p/skia/issues/detail?id=5835

It is fixed by:
https://chromium.googlesource.com/skia/+/8f457591e6a21c240fd2fca56a97281b556ce796

==36519==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffd5462dc4 at pc 0x0000013128e5 bp 0x7fffd54613d0 sp 0x7fffd54613c8
READ of size 4 at 0x7fffd5462dc4 thread T0
    #0 0x13128e4 in SkLinearGradient::LinearGradient4fContext::LinearIntervalProcessor<((anonymous namespace)::DstType)0, (SkShader::TileMode)0>::advance_interval(float) src/effects/gradients/Sk4fLinearGradient.cpp:351:33
    #1 0x13128e4 in SkLinearGradient::LinearGradient4fContext::LinearIntervalProcessor<((anonymous namespace)::DstType)0, (SkShader::TileMode)0>::advance(float) src/effects/gradients/Sk4fLinearGradient.cpp:315
    #2 0x13128e4 in void SkLinearGradient::LinearGradient4fContext::shadeSpanInternal<((anonymous namespace)::DstType)0, ((anonymous namespace)::ApplyPremul)0, (SkShader::TileMode)0>(int, int, (anonymous namespace)::DstTraits<((anonymous namespace)::DstType)0, ((anonymous namespace)::ApplyPremul)0>::Type*, int) const src/effects/gradients/Sk4fLinearGradient.cpp:271
    #3 0x13128e4 in void SkLinearGradient::LinearGradient4fContext::shadePremulSpan<((anonymous namespace)::DstType)0, ((anonymous namespace)::ApplyPremul)0>(int, int, (anonymous namespace)::DstTraits<((anonymous namespace)::DstType)0, ((anonymous namespace)::ApplyPremul)0>::Type*, int) const src/effects/gradients/Sk4fLinearGradient.cpp:214
    #4 0x1307cfe in SkLinearGradient::LinearGradient4fContext::shadeSpan(int, int, unsigned int*, int) src/effects/gradients/Sk4fLinearGradient.cpp:182:9
    #5 0xa16073 in SkARGB32_Shader_Blitter::blitRect(int, int, int, int) src/core/SkBlitter_ARGB32.cpp:460:17
    #6 0x899e16 in blitrect(SkBlitter*, SkIRect const&) src/core/SkScan.cpp:16:5
    #7 0x899e16 in SkScan::FillIRect(SkIRect const&, SkRegion const*, SkBlitter*) src/core/SkScan.cpp:44
    #8 0x89ac0f in SkScan::FillIRect(SkIRect const&, SkRasterClip const&, SkBlitter*) src/core/SkScan.cpp:74:9
    #9 0x69a1f1 in SkDraw::drawPaint(SkPaint const&) const src/core/SkDraw.cpp:288:5
    #10 0x635d1e in SkCanvas::internalDrawPaint(SkPaint const&) src/core/SkCanvas.cpp:2088:9
    #11 0x635408 in SkCanvas::onDrawPaint(SkPaint const&) src/core/SkCanvas.cpp:2081:5
    #12 0x5b0136 in fuzzLinearGradient(Fuzz*) fuzz/FuzzGradients.cpp:120:13
    #13 0x5b3f2a in fuzz_Gradients(Fuzz*) fuzz/FuzzGradients.cpp:280:13
    #14 0x5bd941 in fuzz_api(sk_sp<SkData>) fuzz/fuzz.cpp:99:13
    #15 0x5bd941 in main fuzz/fuzz.cpp:57
    #16 0x7f73b752182f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #17 0x4cb488 in _start (/home/user/workspace/afl/skia/fuzz+0x4cb488)

Address 0x7fffd5462dc4 is located in stack of thread T0 at offset 3684 in frame
    #0 0x699c6f in SkDraw::drawPaint(SkPaint const&) const src/core/SkDraw.cpp:253
Lee, should we just cherry pick this change?
Flags: needinfo?(lsalzman)
Keywords: sec-high
(Assignee)

Comment 2

3 years ago
(In reply to Milan Sreckovic [:milan] from comment #1)
> Lee, should we just cherry pick this change?

The patch doesn't look complicated, so I can look into it.
(Assignee)

Updated

3 years ago
Flags: needinfo?(lsalzman)
(Assignee)

Comment 3

3 years ago
This is just a straight backport of the upstream fix with some minor compatibility changes: https://chromium.googlesource.com/skia/+/8f457591e6a21c240fd2fca56a97281b556ce796
Attachment #8812874 - Flags: review?(mchang)
Attachment #8812874 - Flags: review?(mchang) → review+
(Assignee)

Comment 4

3 years ago
Comment on attachment 8812874 [details] [diff] [review]
Skia clamped gradient fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Via commentary available in the upstream Skia fix that is already published, it might be possible to at least construct an input that would cause a crash via an overrun, though controlling the resulting overrun to do something more malicious seems more difficult since it is writing gradient data.
 
> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. It doesn't mention the possibly of buffer overruns anywhere, so one would have to realize that passing in large invalid gradient inputs could be used in that way to do so. The upstream Skia fix, which is already published, however, does mention the possibility of causing an overrun.

> 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?
Should apply against at least aurora, will require some relatively easy adaptation to work for beta or release.

> How likely is this patch to cause regressions; how much testing does it need?
Already tested and used upstream without any complaint.
Attachment #8812874 - Flags: sec-approval?
Comment on attachment 8812874 [details] [diff] [review]
Skia clamped gradient fix

sec-approval+. We should backport this to branches and maybe consider for 50.1.
Flags: needinfo?(rkothari)
Attachment #8812874 - Flags: sec-approval? → sec-approval+
Hi Lee, could you please nominate the patches for uplift to Release50, Beta51 and Aurora52? Thanks!
Flags: needinfo?(rkothari) → needinfo?(lsalzman)
(Assignee)

Comment 7

2 years ago
Just a rebase of the patch to apply against beta/release.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1265131
[User impact if declined]: Already disclosed buffer overrun bug vulnerability that would be left unpatched.
[Is this code covered by automated tests?] Only tested upstream.
[Has the fix been verified in Nightly?] Already fixed upstream.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: beta, release
[Is the change risky?]: No
[Why is the change risky/not risky?]: Already tested/patched upstream
[String changes made/needed]: None
Flags: needinfo?(lsalzman)
Attachment #8815023 - Flags: review+
Attachment #8815023 - Flags: approval-mozilla-release?
Attachment #8815023 - Flags: approval-mozilla-beta?
(Assignee)

Comment 8

2 years ago
Comment on attachment 8812874 [details] [diff] [review]
Skia clamped gradient fix

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1265131
[User impact if declined]: Already disclosed buffer overrun bug vulnerability that would be left unpatched.
[Is this code covered by automated tests?] Only tested upstream.
[Has the fix been verified in Nightly?] Already fixed upstream.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: beta, release
[Is the change risky?]: No
[Why is the change risky/not risky?]: Already tested/patched upstream
[String changes made/needed]: None
Attachment #8812874 - Flags: approval-mozilla-aurora?
Comment on attachment 8815023 [details] [diff] [review]
Skia clamped gradient fix (beta/release)

Sec-high, approved for uplift to all branches.
Attachment #8815023 - Flags: approval-mozilla-release?
Attachment #8815023 - Flags: approval-mozilla-release+
Attachment #8815023 - Flags: approval-mozilla-beta?
Attachment #8815023 - Flags: approval-mozilla-beta+
Attachment #8812874 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/cdff330f17a4
Assignee: nobody → lsalzman
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Whiteboard: [adv-main50.1+]
Group: gfx-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.