Possible security sensitive crash [@ mozilla::WebGLProgram::DetachShaders() ]

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bas, Assigned: posidron)

Tracking

unspecified
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox5+ fixed, blocking2.0 -, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?], URL)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
When playing with possible infinite loop shaders I hit a crash that appears like it might be dangerous:

Crash Reason	EXCEPTION_ACCESS_VIOLATION_WRITE
Crash Address	0x253e70fc

0 	xul.dll 	mozilla::WebGLProgram::DetachShaders 	content/canvas/src/WebGLContext.h:1313
1 	xul.dll 	mozilla::WebGLContext::UseProgram 	content/canvas/src/WebGLContextGL.cpp:3573
2 	xul.dll 	nsIDOMWebGLRenderingContext_UseProgram 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:31987
3 		@0x47ef8b 	
4 	mozjs.dll 	CheckStackAndEnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:778
5 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:802
6 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:646
7 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:740
8 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:863
9 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5173
10 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1914
11 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:9107
12 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:9452
13 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:425
14 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:517
15 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633
16 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:134
17 	xul.dll 	xul.dll@0xb367c7 	
18 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
19 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
20 	mozcrt19.dll 	_VEC_memzero 	
21 	xul.dll 	xul.dll@0x35d7cd 	
22 	firefox.exe 	firefox.exe@0x1bb7 	
23 	ntdll.dll 	WinSqmSetIfMaxDWORD 	
24 	ntdll.dll 	_RtlUserThreadStart 	
25 	firefox.exe 	firefox.exe@0x186f 	
26 	firefox.exe 	firefox.exe@0x186f 

1311 void DetachShaders() {
1312   for (PRUint32 i = 0; i < mAttachedShaders.Length(); ++i) {
1313     mAttachedShaders[i]->DecrementAttachCount();
1314   }
1315   mAttachedShaders.Clear();
1316 }
I can see this bug, but can't see any of the comments. Do I need more permissions?
No, we just need to un-double-hide the initial description: done.
Whiteboard: [sg:critical?]
Bas, when you can reproduce this bug, can you print the values of:
  i
  mAttachedShaders.Length()

Also, a test-case would greatly help.
(Reporter)

Comment 4

6 years ago
(In reply to comment #3)
> Bas, when you can reproduce this bug, can you print the values of:
>   i
>   mAttachedShaders.Length()
> 
> Also, a test-case would greatly help.

What I was doing was playing around with for (float i = 0.0; i < a; i += 1.0);

Where I was varying the output colors and 'a' between 10 and 100000.

On the ShaderToy site.
@ Christoph: could you please try to find a test-case for this, based on the description given in comment 4?
(Assignee)

Comment 6

6 years ago
Of course.
Over to Christoph to get a testcase.
Assignee: nobody → christoph.diehl
The crash location

    void DetachShaders() {
        for (PRUint32 i = 0; i < mAttachedShaders.Length(); ++i) {
            mAttachedShaders[i]->DecrementAttachCount();
        }
        mAttachedShaders.Clear();
    }

and the fact that it's an invalid write at a random address means that the mAttachedShaders[i] pointer is stale.

This is a plain pointer, not a refptr, so one can be tempted to replace that by a refptr, but I don't understand how this pointer can have gone stale, as the WebGLContext keeps refptrs to all the shaders. As long as the WebGLContext is live, all shaders should be, and therefore pointers to them should stay valid.
Ah, WebGLContext::DeleteShader() causes the reference count to be decremented, so it's conceivable that calling UseProgram() after DeleteShader() would crash in this way. For this reason I think that we should let WebGLProgram store refptrs instead of plain ptrs to the attached shaders.
Some people have written to us about their intention to release a blog post about a WebGL security flaw we allegedly have on Monday, without giving us any usable information to start fixing it.

In anticipation of that, I will implement comment 9.
Created attachment 530639 [details] [diff] [review]
fix dangling pointers by making them nsRefPtrs

This should fix it --- although I can't be sure, as I have yet to find a test case for this bug.

I tried first using WebGLObjectRefPtr, but got assertions (pasted below) so used nsRefPtr instead, as that's really just what I want:

#0  0x00007fe5c58db78d in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#1  0x00007fe5c58db600 in __sleep (seconds=<value optimized out>)
    at ../sysdeps/unix/sysv/linux/sleep.c:138
#2  0x00007fe5c7713e40 in ah_crap_handler (signum=11)
    at /home/bjacob/mozilla-central/toolkit/xre/nsSigHandlers.cpp:119
#3  0x00007fe5c77193e7 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fff8c63c2f0, 
    context=0x7fff8c63c1c0) at /home/bjacob/build/firefox/toolkit/profile/nsProfileLock.cpp:226
#4  <signal handler called>
#5  0x00007fe5c7e6096b in mozilla::WebGLObjectBaseRefPtr::Zero (this=0x282bb30)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:121
#6  0x00007fe5c7e60c35 in mozilla::WebGLZeroingObject::ZeroOwners (this=0x2adaed8)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:605
#7  0x00007fe5c7e60db5 in mozilla::WebGLShader::Delete (this=0x2adaed0)
    at /home/bjacob/mozilla-central/content/canvas/src/WebGLContext.h:1236
#8  0x00007fe5c7e6826e in mozilla::WebGLContext::DeleteShader (this=0x7fe5985185c0, 
    sobj=0x2adaed0) at /home/bjacob/mozilla-central/content/canvas/src/WebGLContextGL.cpp:1037
#9  0x00007fe5c87149ed in nsIDOMWebGLRenderingContext_DeleteShader (cx=0x2b5cd20, argc=1, 
    vp=0x7fe5b56ed248) at /home/bjacob/build/firefox/js/src/xpconnect/src/dom_quickstubs.cpp:29672
#10 0x00007fe5c92ed564 in js::CallJSNative (cx=0x2b5cd20, 
    native=0x7fe5c871489b <nsIDOMWebGLRenderingContext_DeleteShader>, argc=1, vp=0x7fe5b56ed248)
    at /home/bjacob/mozilla-central/js/src/jscntxtinlines.h:277
#11 0x00007fe5c954a972 in js::Interpret (cx=0x2b5cd20, entryFrame=0x7fe5b56ed048, 
    inlineCallCount=2, interpMode=js::JSINTERP_NORMAL)
    at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:4664
Attachment #530639 - Flags: review?(joe)
Attachment #530639 - Flags: review?(joe) → review+
http://hg.mozilla.org/mozilla-central/rev/f65a3762c7de

With that in, I really don't think that this bug could still happen; but without anyone being able to reproduce this bug, I don't know if/how I can close it.

Should this be a WORKSFORME?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
OK, I'm feeling confident enough to mark this fixed. Reopen if needed.
Resolution: WORKSFORME → FIXED

Comment 14

6 years ago
Does this impact Firefox 5 (aurora)? Should we consider taking the fix there?
tracking-firefox5: --- → ?
I would say that's a good idea. Also in Firefox 4 if there will be another release of it.

Just wait until TBPL shows a green mochitest-1, to be sure.
To be clear: yes, this impacts Firefox 4 and 5 in exactly the same way.
It's green on TBPL. Recommend backporting this to Firefox 4 and 5. Changeset link:
  http://hg.mozilla.org/mozilla-central/rev/f65a3762c7de
Attachment #530639 - Flags: approval2.0?
Attachment #530639 - Flags: approval-mozilla-aurora?
blocking2.0: --- → -
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox5: ? → +

Updated

6 years ago
Attachment #530639 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 18

6 years ago
Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.

Comment 19

6 years ago
Please land this on releases/mozilla-aurora TODAY or approval will be rescinded.
Landed on mozilla-aurora:
http://hg.mozilla.org/mozilla-aurora/rev/f8b08657dfea

Updated

6 years ago
status-firefox5: affected → fixed
Comment on attachment 530639 [details] [diff] [review]
fix dangling pointers by making them nsRefPtrs

Approved for the mozilla2.0 repository, a=dveditz for release-drivers if Cameron needs this for TenFourFox (although that may not support WebGL).

When landed please add a link to the changeset and flip the status2.0 field to ".x-fixed". If you don't need this on the 2.0 branch please unset the approval requests so we don't have an unfixed approval to track in bug triage.
Attachment #530639 - Flags: approval2.0? → approval2.0+
It doesn't, but I certainly wouldn't mind it landed just in case. Thanks very much!
Group: core-security
You need to log in before you can comment on or make changes to this bug.