Closed Bug 1333858 (CVE-2017-5459) Opened 8 years ago Closed 8 years ago

SEGV in AddressIsPoisoned

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 53+ fixed
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: attekett, Assigned: jgilbert)

References

Details

(Keywords: csectype-bounds, sec-critical, testcase, Whiteboard: gfx-noted [adv-main53+][adv-esr45.9+][adv-esr52.1+])

Attachments

(3 files, 1 obsolete file)

Attached file repro-file.html (deleted) —
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."
Jeff: this /might/ be a bug in ASAN (that we should report to the maintainers), but could be a WebGL problem.
Group: core-security → gfx-core-security
Flags: needinfo?(jgilbert)
Milan, is there somebody who can look into this? Thanks. For what it is worth, Tyson was unable to reproduce this.
Flags: needinfo?(milan)
I did manage repro it on my laptop but not in a VM (which reasonable looking at the stack).
I'm guessing we're not quite doing the right thing in WebGLContext Intersect, and overflow int. I'll take a quick look.
Attached patch Source end should be positive. r=jgilbert (obsolete) (deleted) — — Splinter Review
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Attachment #8838638 - Flags: review?(jgilbert)
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.
Assignee: nobody → milan
[Tracking Requested - why for this release]:
Flags: sec-bounty?
I'll fix this tomorrow. I don't feel that this is the right place to fix this.
Assignee: milan → jgilbert
Priority: -- → P1
Whiteboard: gfx-noted
Yeah, Intersect should just use CheckedInts and be fallible.
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.
Attachment #8838638 - Attachment is obsolete: true
Attachment #8838638 - Flags: review?(jgilbert)
Attachment #8841850 - Flags: review?(jmuizelaar)
Attached patch 0002-Bug-1333858-Add-tests.-r-daoshengmu.patch (deleted) — — Splinter Review
Attachment #8841851 - Flags: review?(jmuizelaar)
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.
Attachment #8841850 - Flags: review?(jmuizelaar) → review+
Attachment #8841851 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8841850 - Flags: sec-approval?
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.
Attachment #8841850 - Flags: sec-approval? → sec-approval+
Please nominate branch patches once this lands on trunk.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
This grafts cleanly to all supported branches. Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.
Flags: needinfo?(jgilbert)
Group: gfx-core-security → core-security-release
If you can do that today, it can still potentially make the beta 10 build but if not, the 53 RC build next Monday.
Flags: needinfo?(milan)
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
Attachment #8841850 - Flags: approval-mozilla-esr52?
Attachment #8841850 - Flags: approval-mozilla-esr45?
Attachment #8841850 - Flags: approval-mozilla-beta?
Attachment #8841850 - Flags: approval-mozilla-aurora?
Flags: needinfo?(milan)
Comment on attachment 8841850 [details] [diff] [review] 0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch Fix a sec-critical. Aurora54+ & Beta53+.
Attachment #8841850 - Flags: approval-mozilla-beta?
Attachment #8841850 - Flags: approval-mozilla-beta+
Attachment #8841850 - Flags: approval-mozilla-aurora?
Attachment #8841850 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Milan's assessment on manual testing needs (see Comment 21) and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8841850 [details] [diff] [review] 0001-Bug-1333858-Intersect-should-be-fallible-on-overflow.patch sec-crit fix for esr45/esr52
Attachment #8841850 - Flags: approval-mozilla-esr52?
Attachment #8841850 - Flags: approval-mozilla-esr52+
Attachment #8841850 - Flags: approval-mozilla-esr45?
Attachment #8841850 - Flags: approval-mozilla-esr45+
This needs rebasing for ESR45 uplift.
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Flags: sec-bounty? → sec-bounty+
Keywords: testcase
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.
Depends on: 1347866
Flags: needinfo?(milan)
(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.
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 :)
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.
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.
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!
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.
(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!
(hopefully I won't be needed, but I'll be back online in 8 hours)
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.
Flags: needinfo?(jgilbert) → needinfo?(abillings)
At least two weeks post, release (May 3 or after) and preferably four weeks.
Alias: CVE-2017-5459
Flags: needinfo?(abillings)
Whiteboard: gfx-noted → gfx-noted [adv-main53+][adv-esr45.9+][adv-esr52.1+]
Attachment #8856600 - Attachment description: attekett@gmail.com,4000?,2017-01-25,2017-04-05,2017-04-10,true,,, → attekett@gmail.com,4000,2017-01-25,2017-04-05,2017-04-10,true,,,
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: