Closed Bug 1220450 (CVE-2016-1935) Opened 4 years ago Closed 4 years ago

global-buffer-overflow (write) at BufferSubData

Categories

(Core :: Canvas: WebGL, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox42 --- wontfix
firefox43 + wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 --- fixed
firefox-esr38 44+ fixed
firefox-esr45 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- wontfix
b2g-v2.5 --- fixed
b2g-v2.2r --- wontfix
b2g-master --- fixed

People

(Reporter: aki.helin, Assigned: jgilbert)

Details

(Keywords: csectype-bounds, sec-critical, Whiteboard: [adv-main44+][adv-esr38.6+])

Attachments

(4 files, 2 obsolete files)

Attached file ff-bofw-glbounds.html
The attached page triggers a global buffer overflow write. This happens every time at least on a freshly installed Debian machine using a recent tindebox build. Not tested on other machines or display drivers yet.

==4966==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7fc0d209890f at pc 0x7fc0c8efdd95 bp 0x7fff3db96cb0 sp 0x7fff3db96ca8
WRITE of size 31337 at 0x7fc0d209890f thread T0 (Web Content)
    #0 0x7fc0c8efdd94 in BufferSubData /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/canvas/WebGLElementArrayCache.cpp:492
    #1 0x7fc0c8efdd94 in ?? ??:0
    #2 0x7fc0c8e73e45 in BufferSubDataT<mozilla::dom::ArrayBufferView_base<&js::UnwrapArrayBufferView, &js::GetArrayBufferViewLengthAndData, &JS_GetArrayBufferViewType> > /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/canvas/WebGLContextBuffers.cpp:313
    #3 0x7fc0c8e73e45 in ?? ??:0
    #4 0x7fc0c840d310 in bufferSubData /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./WebGLRenderingContextBinding.cpp:10849
    #5 0x7fc0c840d310 in ?? ??:0
    #6 0x7fc0c8d2e637 in GenericBindingMethod /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2691
    #7 0x7fc0c8d2e637 in ?? ??:0
    #8 0x7fc0ce3694db in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #9 0x7fc0ce3694db in Invoke /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:772
    #10 0x7fc0ce3694db in ?? ??:0
   [...]
Group: core-security → gfx-core-security
Could you look at this, Jeff? Thanks.
Flags: needinfo?(jgilbert)
Assignee: nobody → edwin
Looks like we're failing an allocation at [1] but that doesn't stop the following bufferSubData call from going through. Hmm...

[1] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextBuffers.cpp#181
Attached patch 1220450.patch (obsolete) — Splinter Review
Feels a bit hacky, but this seems to fix it...
Attachment #8689012 - Flags: review?(jgilbert)
Flags: needinfo?(jgilbert)
Which branches does this affect? Do you think it will need uplift? If so, it will also need sec-approval.
Flags: needinfo?(edwin)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> Which branches does this affect? Do you think it will need uplift? If so, it
> will also need sec-approval.

Crashes for me all the way down to release.
Flags: needinfo?(edwin)
Comment on attachment 8689012 [details] [diff] [review]
1220450.patch

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

This can leave us in an unsafe state, where we think the length of the buffer is different from what GL's internal buffer has.
Attachment #8689012 - Flags: review?(jgilbert) → review-
GL_OOM lets us mess with state before returning. Let's mess with state in order to stay safe.
Assignee: edwin → jgilbert
Attachment #8689012 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8691190 - Flags: review?(dglastonbury)
Attachment #8691190 - Flags: review?(dglastonbury) → review+
Flags: sec-bounty?
Adding tracking and affected flags. We could still take this in the RC build next week. Please nominate it for sec approval and uplift if you think that is the right course to take for 43 release.
Jeff, could request the sec-approval asap so that it lands in 44? Thanks
Flags: needinfo?(jgilbert)
Comment on attachment 8691190 [details] [diff] [review]
0001-Clear-length-on-cache-OOM.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Hard.

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 of them.

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

How likely is this patch to cause regressions; how much testing does it need?
It should be fine.
Flags: needinfo?(jgilbert)
Attachment #8691190 - Flags: sec-approval?
sec-approval+ for trunk.

We'll want nominations for Aurora, Beta, and ESR38.
Attachment #8691190 - Flags: sec-approval? → sec-approval+
Needs rebasing.
Flags: needinfo?(jgilbert)
Flags: in-testsuite?
Keywords: checkin-needed
Was a trivial rebase so I went ahead and did it myself. For the branch uplifts, the patch as-landed also applies cleanly to Aurora, while Beta can use the attached patch without any conflicts.

https://hg.mozilla.org/integration/mozilla-inbound/rev/740e4ce7bb16

However, while the attached patch applies cleanly to ESR38 as well, the attached testcase still instacrashes with it applied. So it appears that it'll need a branch-specific patch.
https://hg.mozilla.org/mozilla-central/rev/dd6de89a16f2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Flags: sec-bounty? → sec-bounty+
Group: gfx-core-security → core-security-release
Comment on attachment 8691190 [details] [diff] [review]
0001-Clear-length-on-cache-OOM.patch

Nom'ing patch for uplift to Beta and Aurora based on RyanVM's comment 13 that they can be applied as is.
Attachment #8691190 - Flags: approval-mozilla-beta?
Attachment #8691190 - Flags: approval-mozilla-aurora?
Comment on attachment 8691190 [details] [diff] [review]
0001-Clear-length-on-cache-OOM.patch

JGilbert agrees that these patches apply as is on aurora and beta. I am unsure about esr38 but I've approved that as well bcuz we want sec patches to land at the same time on all branches. beta44+, aurora45+, esr38.6+
Attachment #8691190 - Flags: approval-mozilla-esr38+
Attachment #8691190 - Flags: approval-mozilla-beta?
Attachment #8691190 - Flags: approval-mozilla-beta+
Attachment #8691190 - Flags: approval-mozilla-aurora?
Attachment #8691190 - Flags: approval-mozilla-aurora+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/2674cea8413d

hm that still has problems to apply to beta:

grafting 322856:2674cea8413d "Bug 1220450 - Clear length on cache OOM. r=kamidphish, a=ritu"
merging dom/canvas/WebGLContextBuffers.cpp
warning: conflicts while merging dom/canvas/WebGLContextBuffers.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Jeff, can you take a look?
Milan, this is a sec-crit that ideally should land at the same time on all branches. The request for rebasing was made over 10 days back and we still don't have the patches for beta and esr38. As you know Beta44 enters RC mode next week, could you please help get us a timely patch on this one? Many thanks!
Flags: needinfo?(milan)
(In reply to Carsten Book [:Tomcat] from comment #18)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/2674cea8413d
> 
> hm that still has problems to apply to beta:

Re-read comment 13. The *attached* patch is what you need for beta, not the rebased patch that landed on m-c/aurora. But yes, still need an esr38 patch that doesn't crash still :)
So I'm working on a rebase for esr38, but I'm stalled on getting esr38 to build on my machine.
Here's the ESR38 patch. I don't have the environment to test it, but it protects a third instance of ElementArrayCacheBufferData() that doesn't exist in current trunk. (which should be why it's still bad after patching)
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
Any chance you can verify the fix, :ryanvm?
Flags: needinfo?(ryanvm)
Tomcat ni'ing you so this can be uplifted to Beta. Thanks!
Flags: needinfo?(cbook)
(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> Any chance you can verify the fix, :ryanvm?

Still crashes with the patch applied.

msvcr120.dll!00007ff86a2ac3f9()	Unknown
0000025f1291ecd0()	Unknown
0000025f1c527a58()	Unknown
libGLESv2.dll!rx::Buffer11::NativeBuffer11::setData(D3D11_MAP mapMode, const unsigned char * data, unsigned __int64 size, unsigned __int64 offset) Line 818	C++
libGLESv2.dll!rx::Buffer11::setSubData(const void * data, unsigned __int64 size, unsigned __int64 offset) Line 277	C++
libGLESv2.dll!rx::Buffer11::setData(const void * data, unsigned __int64 size, unsigned int usage) Line 185	C++
libGLESv2.dll!gl::Buffer::bufferData(const void * data, __int64 size, unsigned int usage) Line 39	C++
libGLESv2.dll!glBufferData(unsigned int target, __int64 size, const void * data, unsigned int usage) Line 540	C++
xul.dll!mozilla::gl::GLContext::fBufferData(unsigned int target, __int64 size, const void * data, unsigned int usage) Line 935	C++
xul.dll!mozilla::WebGLContext::CheckedBufferData(unsigned int target, __int64 size, const void * data, unsigned int usage) Line 535	C++
xul.dll!mozilla::WebGLContext::BufferData(unsigned int target, __int64 size, unsigned int usage) Line 186	C++
xul.dll!mozilla::dom::WebGLRenderingContextBinding::bufferData(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::WebGLContext * self, const JSJitMethodCallArgs & args) Line 8721	C++
xul.dll!mozilla::dom::GenericBindingMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 2543	C++
xul.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 491	C++

The crashing line in question from rx::Buffer11::NativeBuffer11::setData:

    uint8_t *offsetBufferPointer = reinterpret_cast<uint8_t *>(mappedResource.pData) + offset;
    memcpy(offsetBufferPointer, data, size);

>   context->Unmap(mNativeBuffer, 0);

    return gl::Error(GL_NO_ERROR);

I looked for you on IRC to see if you wanted more information while I have it up in a debugger. Feel free to ping me later if you're around.
Flags: needinfo?(ryanvm)
Crashes in the same place without the patch attached. Though FWIW, I just realized that I can only reproduce the crash on my system with 64-bit builds.
Per comment 27, the attached patch doesn't work?
Flags: needinfo?(cbook)
backed this out per irc discussion with Ryan.

Ritu: how we should proceed here, the patch seems to have problems
Flags: needinfo?(cbook) → needinfo?(rkothari)
Hi Jeff, Milan: We have only a few days to land this sec-crit fix into ESR38.6. Ideally we want sec fixes to land on all branches at the same time which clearly did not happen in this issue. Appreciate your support in making this happen asap. If we don't land this soon, we could find ourselves in an ESR chemspill situation. :(
Flags: needinfo?(rkothari)
Flags: needinfo?(milan)
Flags: needinfo?(jgilbert)
So, I'm somewhat confused (that's OK, it happens often.)  Is this problem fixed in any of the trains?  Is it OK everywhere *except on* ESR38, or is it still a problem everywhere that was tested?
Flags: needinfo?(ryanvm)
And if it is only on ESR38, is it a problem with what landed in comment 29, or the patch attached to the bug in attachment 8706726 [details] [diff] [review], because they're not the same from what I can tell.
Flags: needinfo?(milan)
I was able to reproduce the crash on beta/aurora/trunk builds prior to this patch landing and not afterwards. However, ESR38 still crashes with or without Jeff's attached patch.

Looks like Tomcat uplifted the Beta patch rather than the attached ESR38 patch. No idea why. I was using the ESR38 patch attached when testing locally.
Flags: needinfo?(ryanvm)
After some discussion with Milan today, I decided to do another round of ESR38 testing with the attached branch patch.

win32: No crashes with or without the patch. Tested on Win10 x64 (native) and Win7 x32 (VM).
win64: Crashes with or without the patch. Tested on Win10 x64.

OSX 10.11: Beachballs for a few seconds, no crashes. Same behavior with or without patch. Tested on 2013 MBP Retina.

linux32: No crashes with or without the patch. Tested on an Ubuntu 15.10 x32 VM.
linux64: Crashes without the patch. Doesn't crash with the patch (!!!). Tested on an Ubuntu 15.10 x64 VM.

So, the patch appears to be effective on Linux64, but not Win64. Every other platform was inconclusive. I have no idea what conclusions to draw from that. While we don't technically support Win64 on ESR38, my concern is that the underlying issue may still be lurking given a tweaked testcase and a sufficiently-clever bad guy.
Liz,

 This one is in jeopardy for ESR38 given the behavior mentioned in comment 36.
Flags: needinfo?(lhenry)
Whiteboard: [adv-main44+]
(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> Per comment 27, the attached patch doesn't work?

It fixes a security vuln, even if it may not fix your testcase. We should definitely take it.
"My" testcase is the one attached to this bug, FWIW.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> (In reply to Jeff Gilbert [:jgilbert] from comment #23)
> > Any chance you can verify the fix, :ryanvm?
> 
> Still crashes with the patch applied.
> 
> msvcr120.dll!00007ff86a2ac3f9()	Unknown
> 0000025f1291ecd0()	Unknown
> 0000025f1c527a58()	Unknown
> libGLESv2.dll!rx::Buffer11::NativeBuffer11::setData(D3D11_MAP mapMode, const
> unsigned char * data, unsigned __int64 size, unsigned __int64 offset) Line
> 818	C++
> libGLESv2.dll!rx::Buffer11::setSubData(const void * data, unsigned __int64
> size, unsigned __int64 offset) Line 277	C++
> libGLESv2.dll!rx::Buffer11::setData(const void * data, unsigned __int64
> size, unsigned int usage) Line 185	C++
> libGLESv2.dll!gl::Buffer::bufferData(const void * data, __int64 size,
> unsigned int usage) Line 39	C++
> libGLESv2.dll!glBufferData(unsigned int target, __int64 size, const void *
> data, unsigned int usage) Line 540	C++
> xul.dll!mozilla::gl::GLContext::fBufferData(unsigned int target, __int64
> size, const void * data, unsigned int usage) Line 935	C++
> xul.dll!mozilla::WebGLContext::CheckedBufferData(unsigned int target,
> __int64 size, const void * data, unsigned int usage) Line 535	C++
> xul.dll!mozilla::WebGLContext::BufferData(unsigned int target, __int64 size,
> unsigned int usage) Line 186	C++
> xul.dll!mozilla::dom::WebGLRenderingContextBinding::bufferData(JSContext *
> cx, JS::Handle<JSObject *> obj, mozilla::WebGLContext * self, const
> JSJitMethodCallArgs & args) Line 8721	C++
> xul.dll!mozilla::dom::GenericBindingMethod(JSContext * cx, unsigned int
> argc, JS::Value * vp) Line 2543	C++
> xul.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct
> construct) Line 491	C++
> 
> The crashing line in question from rx::Buffer11::NativeBuffer11::setData:
> 
>     uint8_t *offsetBufferPointer = reinterpret_cast<uint8_t
> *>(mappedResource.pData) + offset;
>     memcpy(offsetBufferPointer, data, size);
> 
> >   context->Unmap(mNativeBuffer, 0);
> 
>     return gl::Error(GL_NO_ERROR);
> 
> I looked for you on IRC to see if you wanted more information while I have
> it up in a debugger. Feel free to ping me later if you're around.

Yeah, this is a different issue. This is a bug in ANGLE with handling these large values. We can just cap the max size of a buffer.
This should limit ANGLE to UINT32_MAX for buffer size, which should fix it if it's an issue with ANGLE. (which it seems to be)

I'll test when I get home.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> After some discussion with Milan today, I decided to do another round of
> ESR38 testing with the attached branch patch.
> 
> win32: No crashes with or without the patch. Tested on Win10 x64 (native)
> and Win7 x32 (VM).
> win64: Crashes with or without the patch. Tested on Win10 x64.
> 
> OSX 10.11: Beachballs for a few seconds, no crashes. Same behavior with or
> without patch. Tested on 2013 MBP Retina.
> 
> linux32: No crashes with or without the patch. Tested on an Ubuntu 15.10 x32
> VM.
> linux64: Crashes without the patch. Doesn't crash with the patch (!!!).
> Tested on an Ubuntu 15.10 x64 VM.
> 
> So, the patch appears to be effective on Linux64, but not Win64. Every other
> platform was inconclusive. I have no idea what conclusions to draw from
> that. While we don't technically support Win64 on ESR38, my concern is that
> the underlying issue may still be lurking given a tweaked testcase and a
> sufficiently-clever bad guy.

I'll still give this a test, but we should go ahead with the previous one-patch esr38-marked patch, since win64 isn't supported on ESR38, and this bug looks like an ANGLE bug that was probably fixed in an ANGLE update.

It looks like intptr_t is an int32_t on win32, which supports this hypothesis.
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/releases/mozilla-esr38/rev/f9aad6c0253a

With this in, all that remains is the crash in ANGLE on (unsupported) Win64 on ESR38.
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crash if they use an unsupported win64 build of esr38.
Fix Landed on Version: not needed on newer branches
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.



This fixes the crash on Win64 ESR38 for me. I'll nominate this for esr38, though since win64 isn't supported there, I don't think we have much reason to take it.
Attachment #8709808 - Attachment is obsolete: true
Attachment #8709861 - Flags: review?(jmuizelaar)
Attachment #8709861 - Flags: approval-mozilla-esr38?
Agreed that the urgency is much lower now that we know it's a win64-only issue with ANGLE. Thanks for doing the analysis, Jeff!
Comment on attachment 8709808 [details] [diff] [review]
0002-Limit-max-buffers-size-for-ANGLE.patch (esr38 patch 2)

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

It would be worth adding a comment about why ANGLE needs this.
Attachment #8709808 - Flags: review+
Attachment #8709861 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8709861 [details] [diff] [review]
0002-Limit-max-buffers-size-for-ANGLE.patch (esr38 win64 patch)

Approved for uplift to ESR38. This should be in 38.6.0esr.
Flags: needinfo?(lhenry)
Attachment #8709861 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [adv-main44+] → [adv-main44+][adv-esr38.6+]
Alias: CVE-2016-1935
(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> I was able to reproduce the crash on beta/aurora/trunk builds prior to this
> patch landing and not afterwards.
So, can we mark this verified across branches, Ryan?
Flags: needinfo?(ryanvm)
Sure, sounds reasonable.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ryanvm)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.