Closed
Bug 1333858
(CVE-2017-5459)
Opened 8 years ago
Closed 8 years ago
SEGV in AddressIsPoisoned
Categories
(Core :: Graphics: CanvasWebGL, defect, P1)
Core
Graphics: CanvasWebGL
Tracking
()
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jrmuizel
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr45+
jcristau
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
Milan, is there somebody who can look into this? Thanks.
For what it is worth, Tyson was unable to reproduce this.
Flags: needinfo?(milan)
Comment 3•8 years ago
|
||
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.
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
Blocks: webgl-tex-refactor
Updated•8 years ago
|
Keywords: csectype-bounds,
sec-critical
Comment 7•8 years ago
|
||
[Tracking Requested - why for this release]:
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox54:
--- → ?
Updated•8 years ago
|
Flags: sec-bounty?
Updated•8 years ago
|
Assignee | ||
Comment 8•8 years ago
|
||
I'll fix this tomorrow. I don't feel that this is the right place to fix this.
Assignee: milan → jgilbert
Assignee | ||
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: gfx-noted
Assignee | ||
Comment 9•8 years ago
|
||
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.
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
tracking-firefox-esr45:
--- → ?
tracking-firefox-esr52:
--- → ?
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8838638 -
Attachment is obsolete: true
Attachment #8838638 -
Flags: review?(jgilbert)
Attachment #8841850 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8841851 -
Flags: review?(jmuizelaar)
Comment 13•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8841851 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
tracking-firefox52:
+ → ---
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Updated•8 years ago
|
Comment 16•8 years ago
|
||
Please nominate branch patches once this lands on trunk.
Comment 17•8 years ago
|
||
Flags: in-testsuite?
Keywords: checkin-needed
Comment 18•8 years ago
|
||
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 19•8 years ago
|
||
This grafts cleanly to all supported branches. Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.
Flags: needinfo?(jgilbert)
Updated•8 years ago
|
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?
Updated•8 years ago
|
Flags: needinfo?(milan)
Comment 22•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
Comment 24•8 years ago
|
||
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 25•8 years ago
|
||
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+
Comment 26•8 years ago
|
||
uplift |
Comment 27•8 years ago
|
||
This needs rebasing for ESR45 uplift.
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Comment 28•8 years ago
|
||
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.
(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.
Assignee | ||
Comment 30•8 years ago
|
||
Comment 31•8 years ago
|
||
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 :)
Assignee | ||
Comment 32•8 years ago
|
||
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.
Assignee | ||
Comment 33•8 years ago
|
||
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•8 years ago
|
||
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!
Assignee | ||
Comment 35•8 years ago
|
||
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.
Assignee | ||
Comment 36•8 years ago
|
||
(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!
Assignee | ||
Comment 37•8 years ago
|
||
(hopefully I won't be needed, but I'll be back online in 8 hours)
Comment 38•8 years ago
|
||
uplift |
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)
Comment 39•8 years ago
|
||
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+]
Updated•8 years ago
|
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,,,
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•