Bug 1333858 (CVE-2017-5459)

SEGV in AddressIsPoisoned

RESOLVED FIXED in Firefox -esr45

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: attekett, Assigned: jgilbert)

Tracking

({csectype-bounds, sec-critical, testcase})

unspecified
mozilla55
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr4553+ fixed, firefox51 wontfix, firefox52 wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: gfx-noted [adv-main53+][adv-esr45.9+][adv-esr52.1+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Posted file 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."
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.
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?
(Assignee)

Comment 8

2 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

2 years ago
Priority: -- → P1
Whiteboard: gfx-noted
(Assignee)

Comment 9

2 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.
(Assignee)

Comment 11

2 years ago
Attachment #8838638 - Attachment is obsolete: true
Attachment #8838638 - Flags: review?(jgilbert)
Attachment #8841850 - 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+
(Assignee)

Comment 14

2 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 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.
https://hg.mozilla.org/mozilla-central/rev/a7590be606ab
Status: NEW → RESOLVED
Last Resolved: 2 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 :)
(Assignee)

Comment 32

2 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

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

2 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

2 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

2 years ago
(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+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.