1555085 - AddressSanitizer: global-buffer-overflow [@ mozilla::dom::XrayResolveOwnProperty] with READ of size 1

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 69.0a1-20190526093457-https://hg.mozilla.org/mozilla-central/rev/8388f1163e54676201327b95dcb1da7b2f920b39.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Andrew McCreight [:mccr8]]

Looks like maybe this is happening on this line:
DOMIfaceAndProtoJSClass::FromJSClass(js::GetObjectClass(obj))
->wantsInterfaceHasInstance) {

Looks like the JSClass is actually a JSFunctionClassSpec and not a DOM JS class, based on this bit from the report:
0x7f79ec16fb51 is located 47 bytes to the left of global variable 'JSFunctionClassSpec' defined in '/builds/worker/workspace/build/src/js/src/vm/JSFunction.cpp:1207:24' (0x7f79ec16fb80) of size 64
0x7f79ec16fb51 is located 1 bytes to the right of global variable 'JSFunction::class_' defined in '/builds/worker/workspace/build/src/js/src/vm/JSFunction.cpp:1211:25' (0x7f79ec16fb20) of size 48

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ugh. This is my fault. The code added in bug 1278583 and bug 1452786 is making some assumptions that don't hold. In particular, being of type eInterface does NOT mean we're an interface object with a DOMIfaceAndProtoJSClass class. We can be a named constructor function.

So a simple testcase is doing this over Xrays:

  xrayToWindow.Image[Symbol.hasInstance]

and that will crash on the line cited in the crash report.

I'm not sure whether to mark this as regressed by bug 1278583 and bug 1452786 or not, nor what security rating to assign it. In a non-debug build, this will read some random memory to get

DOMIfaceAndProtoJSClass::FromJSClass(js::GetObjectClass(obj))
            ->wantsInterfaceHasInstance

and then if it returns true return a non-undefined function (which is then safe to call); if it returns false it will return undefined. The right behavior is to return undefined. I guess we can crash when reading the random memory, but I don't think this is exploitable for anything other than a DoS due to crashing...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and if I can just mark the regression dependencies and add a test in the patch, because this is not a sec issue in practice or is just a csectype-dos , that would be nice. ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1555085. Don't assume that objects whose DOMObjectType is eInterface have a DOMIfaceAndProtoJSClass JSClass. r=peterv

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w for review ease

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

I guess we can crash when reading the random memory, but I don't think this is exploitable for anything other than a DoS due to crashing...

Well, it isn't "random" memory, is it? If it was actually completely user controllable reads, that would be bad. Are JS classes all statically allocated? Or can they be allocated in the heap?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #7)

Well, it isn't "random" memory, is it? If it was actually completely user controllable reads, that would be bad.

(It is actually a small fixed offset from JS classes, and you are only reading a single bit, I believe.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

If it was actually completely user controllable reads, that would be bad

Ah, in the sense that it would allow reading memory?

It's not user-controllable in the sense that this can only happen when the JSClass is JSFunction::class_ so all that matters is what's in the binary around that. I'd expect that in any given Firefox build the value you get is completely deterministic.

Are JS classes all statically allocated? Or can they be allocated in the heap?

They can be allocated in the heap in general, but the only JSClass that we can end up with here when we do the bogus read is statically allocated.

Comment 10

[Andrew McCreight [:mccr8]]

Oh, I see. Yeah, if this is specifically only the JSClass for JSFunction that sounds pretty benign.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7d0c0a328f6
Don't assume that objects whose DOMObjectType is eInterface have a DOMIfaceAndProtoJSClass JSClass.  r=peterv

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d7d0c0a328f6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta backport ahead of the next ESR or can it ride the trains?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I think this can ride the trains. In a non-debug build, I don't think this causes any serious problems, and it's not hard to merge across.