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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tyson Smith [:tsmith]]

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

Comment 1

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

Lee, should we just cherry pick this change?

Comment 2

[Lee Salzman [:lsalzman]]

(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.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Skia clamped gradient fix

This is just a straight backport of the upstream fix with some minor compatibility changes: https://chromium.googlesource.com/skia/+/8f457591e6a21c240fd2fca56a97281b556ce796

Comment 4

[Lee Salzman [:lsalzman]]

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.

Comment 5

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

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.

Comment 6

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

Hi Lee, could you please nominate the patches for uplift to Release50, Beta51 and Aurora52? Thanks!

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: Skia clamped gradient fix (beta/release)

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

Comment 8

[Lee Salzman [:lsalzman]]

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

Comment 9

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

Comment on attachment 8815023 [details] [diff] [review]
Skia clamped gradient fix (beta/release)

Sec-high, approved for uplift to all branches.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdff330f17a4922320f37bffb50109cbc9c0fc81

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f51d3ab82dc60c54403cc6f64f4e0a5a87a5f5ff
https://hg.mozilla.org/releases/mozilla-beta/rev/6d73e2b511eecab191cbc4f5f66248beb7706d6d
https://hg.mozilla.org/releases/mozilla-release/rev/50e21bee82e6530dde339f6e2cab81b65a7f2e8d

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cdff330f17a4