1333858 - (CVE-2017-5459) SEGV in AddressIsPoisoned

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Atte Kettunen]

Attachment: repro-file.html

I have no idea if this bug actually has security impact, but marking as security issue just to be sure. Also feel free to fill in better summary. :)

Tested on:

OS: Ubuntu 16.04

Firefox: ASAN build from https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2 
sourcestamp: c989c7b352279925edf138373e4ca3f1540dbd5f


ASAN-trace:

=================================================================
==29577==ERROR: AddressSanitizer: SEGV on unknown address 0x10301b08743e (pc 0x00000049c268 bp 0x7ffecf6b6a40 sp 0x7ffecf6b61e0 T0)
    #0 0x49c267 in AddressIsPoisoned /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_mapping.h:297:29
    #1 0x49c267 in QuickCheckForUnpoisonedRegion /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:42
    #2 0x49c267 in memcpy /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:438
    #3 0x7efcdf670ee2  (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so+0x3beee2)
    #4 0x7efcdf671822  (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so+0x3bf822)
    #5 0x7efcdf66bb1a  (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so+0x3b9b1a)
    #6 0x7efcdf3df87e in _init (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so+0x12d87e)
    #7 0x7efcdf3dfa01 in _init (/usr/lib/x86_64-linux-gnu/dri/i965_dri.so+0x12da01)
    #8 0x7efd07655dcc in raw_fReadPixels /home/worker/workspace/build/src/obj-firefox/dist/include/GLContext.h:1553:9
    #9 0x7efd07655dcc in mozilla::gl::GLContext::fReadPixels(int, int, int, int, unsigned int, unsigned int, void*) /home/worker/workspace/build/src/gfx/gl/GLContext.cpp:2881
    #10 0x7efd09e3eea0 in mozilla::WebGLContext::DoReadPixelsAndConvert(mozilla::webgl::FormatInfo const*, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, unsigned int) /home/worker/workspace/build/src/dom/canvas/WebGLContextGL.cpp:1176:9
    #11 0x7efd09e3fede in mozilla::WebGLContext::ReadPixelsImpl(int, int, int, int, unsigned int, unsigned int, void*, unsigned int) /home/worker/workspace/build/src/dom/canvas/WebGLContextGL.cpp:1568:9
    #12 0x7efd09e3f64f in mozilla::WebGLContext::ReadPixels(int, int, int, int, unsigned int, unsigned int, mozilla::dom::ArrayBufferView_base<&js::UnwrapArrayBufferView, &js::GetArrayBufferViewLengthAndData, &(JS_GetArrayBufferViewType(JSObject*))> const&, unsigned int, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/canvas/WebGLContextGL.cpp:1343:5
    #13 0x7efd093a49e1 in ReadPixels /home/worker/workspace/build/src/dom/canvas/WebGLContext.h:690:9
    #14 0x7efd093a49e1 in mozilla::dom::WebGLRenderingContextBinding::readPixels(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/WebGLRenderingContextBinding.cpp:11943
    #15 0x7efd09c5c0c0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2914:13
    #16 0x7efd0f6a96cc in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #17 0x7efd0f6a96cc in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 
.
.
.


Note: if I lower the value of second argument in: "gl.readPixels(0, 2147483647, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE, new Uint8Array(4));" instead of crash I get a warning: "JavaScript warning: file:///tmp/repro-file.html, line 5: Error: WebGL warning: readPixels: Out-of-bounds reads with readPixels are deprecated, and may be slow."

Comment 1

[Daniel Veditz [:dveditz]]

Jeff: this /might/ be a bug in ASAN (that we should report to the maintainers), but could be a WebGL problem.

Comment 2

[Andrew McCreight [:mccr8]]

Milan, is there somebody who can look into this? Thanks.

For what it is worth, Tyson was unable to reproduce this.

Comment 3

[Tyson Smith [:tsmith]]

I did manage repro it on my laptop but not in a VM (which reasonable looking at the stack).

Comment 4

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

I'm guessing we're not quite doing the right thing in WebGLContext Intersect, and overflow int.  I'll take a quick look.

Comment 5

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

Attachment: Source end should be positive. r=jgilbert

Comment 6

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

I think we end up being able to read from "far away" memory, so this is probably fairly high security rating?  Appears it has been around since 45 and bug 1221822.

Comment 7

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]:

Comment 8

[Kelsey Gilbert [:jgilbert]]

I'll fix this tomorrow. I don't feel that this is the right place to fix this.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Yeah, Intersect should just use CheckedInts and be fallible.

Comment 10

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

Tracking this for all upcoming releases. It seems unlikely we'd fix this in time for the 52 release but we can keep an eye on it in case there is a dot release.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Bug-1333858-Add-tests.-r-daoshengmu.patch

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8841850 [details] [diff] [review]
0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

Review of attachment 8841850 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.cpp
@@ +2208,4 @@
>  
>  ////////////////////////////////////////
>  
> +bool

This could use a comment about the conditions under which it will fail.

It would also be better if it returned a Maybe(struct { stuff } ) instead of using a bool and out params so that callers don't accidentally use the results without checking.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8841850 [details] [diff] [review]
0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
medium
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no
Which older supported branches are affected by this flaw?
all
If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
trivial to make if needed
How likely is this patch to cause regressions; how much testing does it need?
low regression likelyhood
Will commit tests to upstream conformance suite after ensuring all browsers are safe.

Comment 15

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

Comment on attachment 8841850 [details] [diff] [review]
0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

sec-approval+ for trunk.
We'll want branch patches made and nominated as well.

Comment 16

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

Please nominate branch patches once this lands on trunk.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f62af954609f9feb5d9d14321de9f70e901c779

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to all supported branches. Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.

Comment 20

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

If you can do that today, it can still potentially make the beta 10 build but if not, the 53 RC build next Monday.

Comment 21

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

Comment on attachment 8841850 [details] [diff] [review]
0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-critical
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: n/a
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes by me, not by author 
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: low risk, this only changes behaviour in the previous overflow case

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8841850 [details] [diff] [review]
0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

Fix a sec-critical. Aurora54+ & Beta53+.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2275f34b6bb1
https://hg.mozilla.org/releases/mozilla-beta/rev/9e3e0759726b

Comment 24

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Milan's assessment on manual testing needs (see Comment 21) and the fact that this fix has automated coverage.

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8841850 [details] [diff] [review]
0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch

sec-crit fix for esr45/esr52

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5ec6fbedb420

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for ESR45 uplift.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

FYI, this is currently the only remaining blocker for the ESR 45.9 RC build, which RelMan wants to do today if at all possible.

Comment 29

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> FYI, this is currently the only remaining blocker for the ESR 45.9 RC build,
> which RelMan wants to do today if at all possible.

Jeff is on it.

Comment 30

[Kelsey Gilbert [:jgilbert]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=45708aa0120a77655a47dc9b16460ace7d35061d

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Jeff, I saw your follow-up Try push. Just wanted to make sure you knew that the Windows GTest timeouts are a known issue and not from your patch :)

Comment 32

[Kelsey Gilbert [:jgilbert]]

OK, good to know!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2316b840040f6d2ef8995cbecfbb132eee69d5ba&selectedJob=90368697
It is surprising to me though that '-p all' gives only those platforms. It's not exactly useful to only test fewer than half the platforms when I ask for all. :(

I'll try pushing this, but back it out if it breaks? I won't be able to watch it this time.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Actually this was non-trivially rebased, so I'll have to run it through the tests first. I'm only 'fairly confident' I got the rebase correct.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

ESR45 is kind of a no-mans land between buildbot and Taskcluster, which is why there's no Linux builds. I'll keep an eye on the Try push and land it if it looks good. I can watch the ESR45 results too afterwards. Thanks for doing the rebase!

Comment 35

[Kelsey Gilbert [:jgilbert]]

Here's with the rebased test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2438c0f4ae910c6082ae0ef877da47284e3edb

If this is clean, land the rebased fix without the test.

When can we land the test, btw? The test is pretty bulls-eye-y.

Comment 36

[Kelsey Gilbert [:jgilbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)
> ESR45 is kind of a no-mans land between buildbot and Taskcluster, which is
> why there's no Linux builds. I'll keep an eye on the Try push and land it if
> it looks good. I can watch the ESR45 results too afterwards. Thanks for
> doing the rebase!

Alright, great, thanks!

Comment 37

[Kelsey Gilbert [:jgilbert]]

(hopefully I won't be needed, but I'll be back online in 8 hours)

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Try push looks good (by ESR45-on-Try standards).

https://hg.mozilla.org/releases/mozilla-esr45/rev/413dc18f25c816c615c323182b6847b1cca5d898

(In reply to Jeff Gilbert [:jgilbert] from comment #35)
> When can we land the test, btw? The test is pretty bulls-eye-y.

That'd be a question for Dan or Al.

Comment 39

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

At least two weeks post, release (May 3 or after) and preferably four weeks.