Closed Bug 922921 Opened 12 years ago Closed 12 years ago

global-buffer-overflow on webgl drawArrays()

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
27 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 + fixed
firefox27 --- verified
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed

People

(Reporter: lifeasageek, Assigned: bjacob)

References

Details

(5 keywords)

Attachments

(4 files)

Consistently check attrib accesses. Also rename a few things.
16.98 KB, patch
jgilbert
: review+
lsblakk
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
updated for landing
17.16 KB, text/plain
bjacob
: review+
Details
aurora patch
18.36 KB, patch
Details | Diff | Splinter Review
beta patch
15.99 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.76 Safari/537.36 Steps to reproduce: Tested on 27 branch (asan build 1380106412 & 1380662633). Seems it is not working on 26 branch. Found global-buffer-overflow on webgl drawArrays(). I'm not sure whether this is actually the security issue, but I'm reporting in as the security just in case (Yeah.. I tried to understand the bug, but failed). ------------------------------------------------- <html> <canvas id="my"> </canvas> <script> canvas = document.getElementById('my'); gl = canvas.getContext('webgl'); ext = gl.getExtension('OES_vertex_array_object'); vao0 = ext.createVertexArrayOES(); var vss = [ 'attribute vec4 vPosition;', 'void main() {', ' gl_Position = vPosition;', '}'].join('\n'); var fss = [ 'void main() {}'].join('\n'); vs = gl.createShader(gl.VERTEX_SHADER); fs = gl.createShader(gl.FRAGMENT_SHADER); gl.shaderSource(vs, vss); gl.compileShader(vs); gl.shaderSource(fs, fss); gl.compileShader(fs); p = gl.createProgram(); gl.attachShader(p, vs); gl.attachShader(p, fs); gl.linkProgram(p); gl.useProgram(p); ext.bindVertexArrayOES(vao0); gl.drawArrays(1,1,1); </script> </html> Actual results: ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f05e2941f84 at pc 0x7f05da9fee86 bp 0x7fffa1106930 sp 0x7fffa1106928 READ of size 1 at 0x7f05e2941f84 thread T0 #0 0x7f05da9fee85 in WhatDoesVertexAttrib0Need content/canvas/src/WebGLContextGL.cpp:816 #1 0x7f05daa2f9ef in DrawArrays_check content/canvas/src/WebGLContextVertices.cpp:507 #2 0x7f05daa30047 in DrawArrays content/canvas/src/WebGLContextVertices.cpp:524 #3 0x7f05dd8f42bc in drawArrays obj-firefox/dom/bindings/./WebGLRenderingContextBinding.cpp:7267 #4 0x7f05dd8b82bb in genericMethod obj-firefox/dom/bindings/./WebGLRenderingContextBinding.cpp:11831 #5 0x7f05df4519ca in native js/src/jscntxtinlines.h:218 #6 0x7f05df442085 in Interpret js/src/vm/Interpreter.cpp:2457 #7 0x7f05df433201 in RunScript js/src/vm/Interpreter.cpp:417 #8 0x7f05df453c4b in ExecuteKernel js/src/vm/Interpreter.cpp:601 #9 0x7f05df453fa6 in Execute js/src/vm/Interpreter.cpp:637 #10 0x7f05df65111d in Evaluate js/src/jsapi.cpp:4886 #11 0x7f05db1b649f in EvaluateString dom/base/nsJSUtils.cpp:280 #12 0x7f05db1a41ef in EvaluateString dom/base/nsJSEnvironment.cpp:995 #13 0x7f05da94770a in EvaluateScript content/base/src/nsScriptLoader.cpp:1002 #14 0x7f05da94501c in ProcessRequest content/base/src/nsScriptLoader.cpp:869 #15 0x7f05da9443e2 in ProcessScriptElement content/base/src/nsScriptLoader.cpp:696 #16 0x7f05da93c419 in MaybeProcessScript content/base/src/nsScriptElement.cpp:139 #17 0x7f05db6ce320 in operator-> /dist/include/nsIScriptElement.h:220 #18 0x7f05db6cbebb in RunFlushLoop parser/html/nsHtml5TreeOpExecutor.cpp:593 #19 0x7f05db645ed8 in Run parser/html/nsHtml5StreamParser.cpp:131 #20 0x7f05ddabc6b9 in ProcessNextEvent xpcom/threads/nsThread.cpp:622 #21 0x7f05dd9e4891 in NS_ProcessNextEvent xpcom/glue/nsThreadUtils.cpp:238 #22 0x7f05dc732851 in Run ipc/glue/MessagePump.cpp:85 #23 0x7f05ddbda3d3 in RunInternal ipc/chromium/src/base/message_loop.cc:220 #24 0x7f05dc509dfc in Run widget/xpwidgets/nsBaseAppShell.cpp:161 #25 0x7f05dbef601e in Run toolkit/components/startup/nsAppStartup.cpp:269 #26 0x7f05d93b2260 in XRE_mainRun toolkit/xre/nsAppRunner.cpp:3868 #27 0x7f05d93b319a in XRE_main toolkit/xre/nsAppRunner.cpp:3936 #28 0x7f05d93b40cb in XRE_main toolkit/xre/nsAppRunner.cpp:4138 #29 0x459b8d in do_main browser/app/nsBrowserApp.cpp:275 #30 0x7f05e8a9676c in __libc_start_main buildd/eglibc-2.15/csu/libc-start.c:226 #31 0x45910c in _start ??:0 0x7f05e2941f84 is located 28 bytes to the left of global variable 'mozilla::sIsUnified' from '/builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/build/Omnijar.cpp' (0x7f05e2941fa0) of size 1 'mozilla::sIsUnified' is ascii string '' 0x7f05e2941f84 is located 28 bytes to the right of global variable 'nsTArrayHeader::sEmptyHdr' from '/builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/glue/nsTArray.cpp' (0x7f05e2941f60) of size 8 Shadow bytes around the buggy address: 0x0fe13c5203a0: 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 0x0fe13c5203b0: 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 0x0fe13c5203c0: 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 0x0fe13c5203d0: 00 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 0x0fe13c5203e0: 04 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 f9 f9 f9 =>0x0fe13c5203f0:[f9]f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 0x0fe13c520400: f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9 00 00 f9 f9 0x0fe13c520410: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe13c520420: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe13c520430: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe13c520440: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 ASan internal: fe ==8952==ABORTING
Looks similar to 891987 though the testcase does not use bindVertexArrayOES()
See Also: → 891987
Status: UNCONFIRMED → NEW
Ever confirmed: true
I've also observed many number of heap-use-after-free and heap-buffer-overflow with similar cases (but quite different crash stacks). Not even sure those are caused by the same bug and cannot reproduce it easily. Still I can provide the crash stack if it can be helpful.
lifeasageek, am suspecting that in each of those cases drawArrays|drawElements() is used but please file a bug for each of those reports. The developers will greatly appreciate it.
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Hi, Christoph. I only have a bunch of crash stacks for such cases, but no test cases. My fuzzer still triggers such crashes quite often, but I can't manually reproduce them (even with the messy test cases from my fuzzer). Is the crash stack itself worth to be a separate bug report? (surely I don't expect the bounty for that even though it is worth to be reported)
lifeasageek, absolutely. There is always a chance that our devs can figure out what went wrong, based on the stack alone.
The code there (stack frame #0) is return (gl->IsGLES2() || mBoundVertexArray->mAttribBuffers[0].enabled) ? VertexAttrib0Status::Default : mCurrentProgram->IsAttribInUse(0) ? VertexAttrib0Status::EmulatedInitializedArray : VertexAttrib0Status::EmulatedUninitializedArray; So we're reading from arrays of bools, and one of them (probably mBoundVertexArray->mAttribBuffers) is empty so addressing it[0] is out-of-bounds. Great catch, thanks! Not sure though that a constantly-1-byte out-of-bounds access (since the index here is [0]) can be a security issue by itself. And just getting the wrong value here is not by itself a security issue, as this check only matters for rendering correctness, not for security (we're trying here to check if we need to emulate vertex attrib 0 on desktop OpenGL; getting this wrong can result in nothing rendering at all, as desktop OpenGL renders nothing unless attrib 0 is array-enabled).
As it's an out-of-bounds access into a nsTArray, it asserts very well in a debug build, no need for ASan: #0 0x00007f5fce17283d in nanosleep () at ../sysdeps/unix/syscall-template.S:82 #1 0x00007f5fce1726dc in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:138 #2 0x00007f5fc6e60693 in ah_crap_handler (signum=11) at /hack/mozilla-central/toolkit/xre/nsSigHandlers.cpp:88 #3 0x00007f5fc6e6c96c in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fff81b3b1f0, context=0x7fff81b3b0c0) at /hack/mozilla-central/profile/dirserviceprovider/src/nsProfileLock.cpp:190 #4 0x00007f5fca92178e in AsmJSFaultHandler (signum=11, info=0x7fff81b3b1f0, context=0x7fff81b3b0c0) at /hack/mozilla-central/js/src/jit/AsmJSSignalHandlers.cpp:988 #5 <signal handler called> #6 0x00007f5fc7ab47dd in nsTArray_Impl<mozilla::WebGLVertexAttribData, nsTArrayInfallibleAllocator>::ElementAt (this=0x7f5f99acc380, i=0) at ../../../dist/include/nsTArray.h:867 #7 0x00007f5fc7ab466c in nsTArray_Impl<mozilla::WebGLVertexAttribData, nsTArrayInfallibleAllocator>::operator[] (this=0x7f5f99acc380, i=0) at ../../../dist/include/nsTArray.h:900 #8 0x00007f5fc7ab7f3c in mozilla::WebGLContext::WhatDoesVertexAttrib0Need (this=0x7f5f9c2f9400) at /hack/mozilla-central/content/canvas/src/WebGLContextGL.cpp:886 #9 0x00007f5fc7ab7f9e in mozilla::WebGLContext::DoFakeVertexAttrib0 (this=0x7f5f9c2f9400, vertexCount=2) at /hack/mozilla-central/content/canvas/src/WebGLContextGL.cpp:894 #10 0x00007f5fc7adcd4d in mozilla::WebGLContext::DrawArrays_check (this=0x7f5f9c2f9400, first=1, count=1, primcount=1, info=0x7f5fcb1b2361 "drawArrays") at /hack/mozilla-central/content/canvas/src/WebGLContextVertices.cpp:507 #11 0x00007f5fc7adcdd4 in mozilla::WebGLContext::DrawArrays (this=0x7f5f9c2f9400, mode=1, first=1, count=1) at /hack/mozilla-central/content/canvas/src/WebGLContextVertices.cpp:524 #12 0x00007f5fc9499ee2 in mozilla::dom::WebGLRenderingContextBinding::drawArrays (cx=0x7f5fa8d7b680, obj=(JSObject * const) 0x7f5fb39b5b80 [object WebGLRenderingContext], self=0x7f5f9c2f9400, args=...) at /hack/mozilla-central/obj-firefox-debug/dom/bindings/WebGLRenderingContextBinding.cpp:7267 #13 0x00007f5fc94aaeae in mozilla::dom::WebGLRenderingContextBinding::genericMethod (cx=0x7f5fa8d7b680, argc=3, vp=0x7f5fb37110a8) at /hack/mozilla-central/obj-firefox-debug/dom/bindings/WebGLRenderingContextBinding.cpp:11831 #14 0x00007f5fca5d9d7c in js::CallJSNative (cx=0x7f5fa8d7b680, native=0x7f5fc94aacae <mozilla::dom::WebGLRenderingContextBinding::genericMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /hack/mozilla-central/js/src/jscntxtinlines.h:218 #15 0x00007f5fca5c4350 in js::Invoke (cx=0x7f5fa8d7b680, args=..., construct=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/vm/Interpreter.cpp:460 #16 0x00007f5fca5cd177 in Interpret (cx=0x7f5fa8d7b680, state=...) at /hack/mozilla-central/js/src/vm/Interpreter.cpp:2457 #17 0x00007f5fca5c3fff in js::RunScript (cx=0x7f5fa8d7b680, state=...) at /hack/mozilla-central/js/src/vm/Interpreter.cpp:417
Component: Canvas: WebGL → Build Config
Jeff, can you take this? I haven't looked at vertex arrays and it's not clear to me what is the right fix: we have a _lot_ of unchecked mAttribBuffers[] accesses in this file, many of which with index 0, and I'm not sure if the right fix is to check them all or to make sure that this array always has size >= 1.
Flags: needinfo?(jgilbert)
Component: Build Config → Canvas: WebGL
Updated heap-buffer-overflow test cases (seems quite related to this one) in https://bugzilla.mozilla.org/show_bug.cgi?id=923523
Taking; taking the route of checking all attribbuffers accesses.
Flags: needinfo?(jgilbert)
Assignee: nobody → bjacob
This just checks all attrib accesses. And, in a deeply unprofessional twist, also throws in some renaming, because having this field named mAttribBuffers always (dates back to very early days) was very confusing, as this is not about buffers but about having vertex attribute data at all.
Attachment #814171 - Flags: review?(jgilbert)
(This fixes the crash reported here).
Comment on attachment 814171 [details] [diff] [review] Consistently check attrib accesses. Also rename a few things. Review of attachment 814171 [details] [diff] [review]: ----------------------------------------------------------------- Nits. Vote 'no' on Yoda-code. ::: content/canvas/src/WebGLContextVertices.cpp @@ +195,5 @@ > MakeContextCurrent(); > InvalidateBufferFetching(); > > gl->fEnableVertexAttribArray(index); > + mBoundVertexArray->mAttribs[index].enabled = true; This is fine, but an assert here that it HasIndex(index) would be great. ::: content/canvas/src/WebGLVertexArray.h @@ +60,5 @@ > GLuint GLName() const { return mGLName; } > > + bool EnsureAttrib(GLuint index, const char *info); > + bool HasAttrib(GLuint index) { > + return mAttribs.Length() > index; I would *much* prefer `index < mAttribs.Length()`.
Attachment #814171 - Flags: review?(jgilbert) → review+
Attached file updated for landing —
Attachment #814266 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a9f84f6cbc71 I'm assuming this only affects m-c as it's sec-high and was landed without sec-approval?
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox27: --- → fixed
Resolution: --- → FIXED
I've only been mindless here, I suppose :-) This probably affects gecko 25+.
Summary: global-buffer-overflow on webgl drawyArrays() → global-buffer-overflow on webgl drawArrays()
(In reply to Benoit Jacob [:bjacob] from comment #18) > I've only been mindless here, I suppose :-) > > This probably affects gecko 25+. Can you nominate for uplifts please?
Flags: needinfo?(bjacob)
Ioana, please verify this is fixed.
Keywords: verifyme
This issue is reproducible for me on a Firefox 25 debug beta build and Firefox 27 asan pre-fix builds, but not on Firefox 26 (latest asan and an older debug). Verified as fixed on the 10/16 Firefox 27 ASAN build: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
Keywords: verifyme
QA Contact: ioana.budnar
Comment on attachment 814171 [details] [diff] [review] Consistently check attrib accesses. Also rename a few things. [Approval Request Comment] Bug caused by (feature/regressing bug #): webgl User impact if declined: sec-high Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #814171 - Flags: approval-mozilla-beta?
Attachment #814171 - Flags: approval-mozilla-aurora?
Attachment #814171 - Flags: approval-mozilla-beta?
Attachment #814171 - Flags: approval-mozilla-beta+
Attachment #814171 - Flags: approval-mozilla-aurora?
Attachment #814171 - Flags: approval-mozilla-aurora+
This needs branch-specific patches for uplift.
Keywords: branch-patch-needed
Both those patches were authored by bjacob, but I guess qref attached my name to them. I didn't actually push my own code as r+ by myself. :)
Verified as fixed on the latest beta debug build: Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 (20131021143402). As specified in comment 21, I can't reproduce this issue on Firefox 26 (pre-fix asan and debug builds). If anyone can, please verify this fix on the latest Aurora build (or, soon, beta).
status-firefox25: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: