Last Comment Bug 775852 - (CVE-2012-3968) use after free, webgl fragment shader deleted by accessor
(CVE-2012-3968)
: use after free, webgl fragment shader deleted by accessor
Status: RESOLVED FIXED
[asan][advisory-tracking+]
: csectype-uaf, regression, sec-critical
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 748266
  Show dependency treegraph
 
Reported: 2012-07-19 23:06 PDT by miaubiz
Modified: 2014-07-24 13:42 PDT (History)
10 users (show)
rforbes: sec‑bounty+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
verified
+
verified
unaffected


Attachments
shaders2.html (May crash you) (1005 bytes, text/html)
2012-07-19 23:06 PDT, miaubiz
no flags Details
asan log osx (11.10 KB, text/plain)
2012-07-19 23:07 PDT, miaubiz
no flags Details
asan log linux (5.38 KB, text/plain)
2012-07-19 23:07 PDT, miaubiz
no flags Details
crashwrangler log, aurora (1.48 KB, text/plain)
2012-07-19 23:08 PDT, miaubiz
no flags Details
crashwrangler log, aurora, another (7.94 KB, text/plain)
2012-07-19 23:08 PDT, miaubiz
no flags Details
crashwrangler log, aurora, another (1.48 KB, text/plain)
2012-07-19 23:08 PDT, miaubiz
no flags Details
crashwrangler log, aurora, another (1.48 KB, text/plain)
2012-07-19 23:09 PDT, miaubiz
no flags Details
Actually follow the WebIDL spec for creating JS representations of sequences. (2.83 KB, patch)
2012-07-25 21:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
And with the test file (4.43 KB, patch)
2012-07-25 21:18 PDT, Boris Zbarsky [:bz] (still a bit busy)
khuey: review+
Details | Diff | Splinter Review
Same thing, for aurora and beta (3.52 KB, patch)
2012-07-26 21:19 PDT, Boris Zbarsky [:bz] (still a bit busy)
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description miaubiz 2012-07-19 23:06:50 PDT
Created attachment 644186 [details]
shaders2.html (May crash you)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.57 Safari/536.11

Steps to reproduce:

loaded the following file:


<html>
  <head>
  </head>
  <body>
    <canvas id="cx"></canvas>
    <script>
      function gc() {
        function gcRec(n) {
          if (n < 1)
          return {}
          var temp = {i: "ab" + i + (i / 100000)}
          temp += "foo"
          gcRec(n-1)
        }
        for (var i = 0; i < 1000; i++) {
          gcRec(10)
        }
      }
      context = cx.getContext("moz-webgl")
      program = context.createProgram()

      shader1 = context.createShader(context.VERTEX_SHADER)
      context.attachShader(program, shader1)

      Array.prototype.__defineSetter__(0, function() {
        context.detachShader(program, shader2)
        context.deleteShader(shader2)
        shader2 = null
        gc()
      })

      for (var i = 0; i < 30; ++i) {
        shader2 = context.createShader(context.FRAGMENT_SHADER)
        context.attachShader(program, shader2)

        shaders = context.getAttachedShaders(program)
        console.dir(shaders[1])
      }
    </script>
  </body>
</html>



Actual results:

firefox crashed.


exception=EXC_BAD_ACCESS:signal=10:is_exploitable=yes:instruction_disassembly=:instruction_address=0x0000000111e82a60:access_type=exec:access_address=0x0000000111e82a60:

exception=EXC_BAD_ACCESS:signal=10:is_exploitable=yes:instruction_disassembly=:instruction_address=0x0000000108a99700:access_type=exec:access_address=0x0000000108a99700:


==1901== ERROR: AddressSanitizer heap-use-after-free on address 0x00012c1ac080 at pc 0x10519470a bp 0x7fff5fbf6490 sp 0x7fff5fbf6488
READ of size 8 at 0x00012c1ac080 thread T0
    #0 0x10519470a in unsigned int CallQueryInterface<nsISupports, nsWrapperCache>(nsISupports*, nsWrapperCache**) (in XUL) + 170
    #1 0x106c2cfbf in bool mozilla::dom::WrapObject<mozilla::WebGLShader>(JSContext*, JSObject*, mozilla::WebGLShader*, nsWrapperCache*, nsID const*, JS::Value*) (in XUL) + 175


0x00012c1ac080 is located 0 bytes inside of 144-byte region [0x00012c1ac080,0x00012c1ac110)
freed by thread T0 here:
    #0 0x10000d095 in (anonymous namespace)::mz_free(_malloc_zone_t*, void*) (in firefox) + 85
    #1 0x10000cb10 in wrap_free (in firefox) + 80
    #2 0x10519e522 in mozilla::WebGLShader::Release() (in XUL) + 290
Comment 1 miaubiz 2012-07-19 23:07:17 PDT
Created attachment 644187 [details]
asan log osx
Comment 2 miaubiz 2012-07-19 23:07:48 PDT
Created attachment 644188 [details]
asan log linux
Comment 3 miaubiz 2012-07-19 23:08:09 PDT
Created attachment 644189 [details]
crashwrangler log, aurora
Comment 4 miaubiz 2012-07-19 23:08:26 PDT
Created attachment 644190 [details]
crashwrangler log, aurora, another
Comment 5 miaubiz 2012-07-19 23:08:54 PDT
Created attachment 644191 [details]
crashwrangler log, aurora, another
Comment 6 miaubiz 2012-07-19 23:09:14 PDT
Created attachment 644192 [details]
crashwrangler log, aurora, another
Comment 7 miaubiz 2012-07-19 23:09:46 PDT
happens to me on osx and linux. didn't test windows. aurora seems affected, stable does not.
Comment 9 Daniel Veditz [:dveditz] 2012-07-20 00:54:21 PDT
In a non-asan opt build I didn't crash using Aurora on Mac or Windows, but the stack traces sure do look like something nuked the shader out from under us.
Comment 10 Andrew McCreight [:mccr8] 2012-07-25 10:20:20 PDT
This is a use-after-free during a QI to nsWrapperCache.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:05:13 PDT
I can't reproduce any error here on linux x86-64 in valgrind.

Comment 10 strongly suggests an issue relating to the recent port to new DOM bindings: bug 748266.

 -> CCing Boris
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:06:43 PDT
mlaubiz: it would be nice if you could use "experimental-webgl" instead of "moz-webgl" for WebGL context creation. This way, your testcases could run in other browsers and would keep running longer into the future in Firefox.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-07-25 13:13:17 PDT
Any chance of more stack for the READ bit?  And for that matter on the Release bit?

Any idea which lines of the JS those correspond to?
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:18:24 PDT
(In reply to Daniel Veditz [:dveditz] from comment #9)
> In a non-asan opt build I didn't crash using Aurora on Mac or Windows, but
> the stack traces sure do look like something nuked the shader out from under
> us.

The testcase explicitly calls webgl.deleteShader which does free its resources. Code should check if (shader->IsDeleted()) to avoid this pitfall. Apparently we have some code that should check that and doesn't.

Looking at call stacks...
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:19:04 PDT
Boris: this call stack, https://bug775852.bugzilla.mozilla.org/attachment.cgi?id=644187&t=CVYBHPjKYU , shows we're crashing in getAttachedShaders.
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:21:35 PDT
So my understanding is that the webgl impl code is fine, it just returns the existing array of attached shaders, however, in the DOM bindings, when we wrap that as a JS object, we do some work that doesn't like the fact that some of the shaders being wrapped is in webgl deleted state.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 13:41:23 PDT
Could you please explain exactly what 

  Array.prototype.__defineSetter__(0, function() { ...

does?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-07-25 14:41:34 PDT
That's the key to the whole thing, thanks.

What that does is that when the binding code is creating the array for the return value of getAttachedShaders, setting the first item of the array calls deleteShader on the second shader in the list.

I'll poke a little bit at WebIDL to figure out what the right fix is.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2012-07-25 19:02:30 PDT
Spec for creating JS reflections of sequences says:

  Call the [[DefineOwnProperty]] internal method on A with property name P, descriptor 
  { [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true, [[Value]]: E }
  and Boolean flag false.

In terms of JSAPI that becames a JS_SetElement with a value, null getter and setter, and the flag JSPROP_ENUMERATE.  This should avoid invoking anything to do with the prototype, so will fix this bug by simply not running that setter function.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2012-07-25 19:05:36 PDT
The good news is we haven't shipped this in a final release yet.  ;)

And by the way, very nice testcase!
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2012-07-25 21:03:51 PDT
Created attachment 646008 [details] [diff] [review]
Actually follow the WebIDL spec for creating JS representations of sequences.

I think this test is safe to land with the patch, for what it's worth.  But I'm willing to listen to counterarguments.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-07-25 21:18:18 PDT
Created attachment 646014 [details] [diff] [review]
And with the test file
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 15:25:00 PDT
Comment on attachment 646014 [details] [diff] [review]
And with the test file

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

Does JS_DefineProperty not run setters?  The docs don't make that clear ...

Nice testcase.  I think it's safe to check in the test with the fix.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 18:12:57 PDT
> Does JS_DefineProperty not run setters?

Correct.  It reconfigures the property to a new getter/setter/value (if the property is configurable), and hence ignores whatever was there before.
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 21:11:26 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/69f121d8a62e
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 21:19:11 PDT
Created attachment 646463 [details] [diff] [review]
Same thing, for aurora and beta
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 21:21:17 PDT
Comment on attachment 646463 [details] [diff] [review]
Same thing, for aurora and beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 748266 
User impact if declined: Calling virtual methods on deleted objects.
   Probably means exploitability.
Testing completed (on m-c, etc.): Passes attached test.
Risk to taking this patch (and alternatives if risky): Risk is very low.  This
  switches us to follow the spec more closely and no one should have been relying
  on the old behavior in any sane way.  The alternative is to flip off the new
  bindings for the WebGL context, which is much more risky.
String or UUID changes made by this patch: None.
Comment 29 Ed Morley [:emorley] 2012-07-27 08:20:15 PDT
https://hg.mozilla.org/mozilla-central/rev/69f121d8a62e
Comment 30 Alex Keybl [:akeybl] 2012-07-30 11:30:32 PDT
Comment on attachment 646463 [details] [diff] [review]
Same thing, for aurora and beta

[Triage Comment]
Low risk, sg:crit fix for a FF15 regression. Let's land on branches.
Comment 32 Ed Morley [:emorley] 2012-07-31 02:06:25 PDT
This is causing OS X 10.5 M2 failures on aurora+beta:
{
633 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/bindings/test/test_sequence_wrapping.html | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLCanvasElement.getContext] at http://mochi.test:8888/tests/dom/bindings/test/test_sequence_wrapping.html:22
}

(Bear in mind trunk doesn't run 10.5 any more, which I guess is why this hasn't been seen there)

I haven't backed out, since a fairly narrow scope failure, so happy for you to fix in place or whatever you prefer :-)
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2012-07-31 07:14:58 PDT
Hrm.  That's failing because 10.5 has no webgl support.  Let me look into how to condition the whole test off in cases when there's no webgl.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2012-07-31 07:25:01 PDT
Pushed:

http://hg.mozilla.org/releases/mozilla-beta/rev/c6d6f7d87bb7
http://hg.mozilla.org/releases/mozilla-aurora/rev/cd14c477f275
https://hg.mozilla.org/integration/mozilla-inbound/rev/594e26e4d103

to deal with cases when WebGL is disabled.  Thank you for the heads-up and for not backing it out!
Comment 35 Ed Morley [:emorley] 2012-07-31 08:33:23 PDT
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #34)
> to deal with cases when WebGL is disabled.  Thank you for the heads-up and
> for not backing it out!

Np :-)
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2012-07-31 20:16:00 PDT
And followup merged to m-c by Ryan: https://hg.mozilla.org/mozilla-central/rev/594e26e4d103
Comment 37 Mihaela Velimiroviciu (:mihaelav) 2012-09-25 06:46:38 PDT
I've tested this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:16.0) Gecko/20100101 Firefox/16.0 beta 4 and also on
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0.1 release

Both builds didn't crashed using the test case provided.

Just to make sure, I've used the same test case from the attachment using the Nightly build from 2012-07-26 and it's crashing, so this is fixed.

Setting the flag to Verified on Firefox 15 and 16
Comment 38 Mihaela Velimiroviciu (:mihaelav) 2012-10-17 05:46:45 PDT
Verified the fix on 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0 - beta1: no crash occurred.
Comment 39 Raymond Forbes[:rforbes] 2013-07-19 18:26:53 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Comment 40 Tracy Walker [:tracy] 2014-01-10 10:40:20 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.