Uninitialized local variable with childNodes, lookupGetter

RESOLVED DUPLICATE of bug 767273

Status

()

Core
JavaScript Engine
--
critical
RESOLVED DUPLICATE of bug 767273
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86_64
Mac OS X
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox13 unaffected, firefox14 unaffected, firefox15 unaffected, firefox16 fixed, firefox17 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [advisory-tracking-][sg:dupe 767273])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 634712 [details]
testcase (asserts fatally when loaded)
(Reporter)

Updated

5 years ago
Group: core-security
I can't reproduce this on my trunk build (debug, macosx64). Jesse, any chance this was fixed by bug 764289?
(Reporter)

Comment 2

5 years ago
Hrm, it still asserts/crashes for me, with a blank profile, both with https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1340204295/ and with a local build (mozilla-central-97119-e240d6e43c9a-debug).

Do you want to debug on my computer?
Created attachment 635232 [details]
updated testcase

I had to flesh the testcase out a little bit to get it to crash. Updating.
Attachment #634712 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
And now I need a setTimeout to reproduce :/
(In reply to Jesse Ruderman from comment #4)
> And now I need a setTimeout to reproduce :/

I can't reproduce with my old testcase. Can you still reproduce with your setTimeout version? If so, can you upload it? If not, I can rewind by repo and debug there.
(Reporter)

Comment 6

5 years ago
Created attachment 636461 [details]
testcase with setTimeout
(Reporter)

Comment 7

5 years ago
Created attachment 636466 [details]
testcase that tries harder

This one requires https://www.squarefree.com/extensions/domFuzzLite3.xpi
(Reporter)

Comment 8

5 years ago
Now I'm getting a non-deterministic crash :(  I haven't tried Valgrind.
(Reporter)

Comment 9

5 years ago
Created attachment 640385 [details]
stack trace 1
(Assignee)

Comment 10

5 years ago
I'm guessing sec-crit on this because it is compartment mismatch.
Keywords: sec-critical
status-firefox16: --- → affected
tracking-firefox16: --- → +
Do we know whether this affects older versions, like ESR or Firefox 14?
(Assignee)

Comment 12

5 years ago
I can take a crack at trying to reproduce this, as I'm mostly stalled out on my other XPConnect bugs.
Assignee: nobody → continuation
I had just managed to reproduce this reliable yesterday by rewinding my repo to rev 4d0aa137268b. I didn't get any farther than that though, so I'm happy for mccr8 to take a crack at it! :-)
(Assignee)

Comment 14

5 years ago
The newest version reproduces reliably on my local tree, which I updated last on July 3.

We're in some kind of CallJSNative, and it looks like we're crashing when we try to get the compartment of the return value.  I guess the next step is to figure out what code is actually being run in the native.
(Assignee)

Comment 15

5 years ago
So, this looks it might be a regression from bug 766430, but I can't be sure.  That landed a week before this bug was filed.

We're going into |obj_lookupGetter|, then hitting the |obj->isProxy()| case, then setting vp using |*vp = CastAsObjectJsval(desc.getter)|.  We return from lookupGetter, then attempt to examine the return value, and end up either crashing attempting to get the compartment of the return value, or we fail with a compartment mismatch, with a bogus looking compartment.

My guess is that desc.getter isn't a GC thing.  Either that or it was rooted improperly at some earlier point, but I'd think poisoning would cause use to die in a different way.  Probably Proxy::getPropertyDescriptor is doing something wrong, so I'll poke around in there.
(Assignee)

Comment 16

5 years ago
Jesse points out that I linked back to this bug.  I meant bug 762432.
(Assignee)

Comment 17

5 years ago
Created attachment 642039 [details] [diff] [review]
root moar, still needs a test

Regression from bug 762432.  The descriptor that was added to obj_lookupGetter isn't properly rooted.  As far as I can see, there's only a couple of lines where it should be unrooted (assuming that CallArgs is properly rooted somewhere up the line), so I'm not sure why this is as reproduceable as it is.  It makes me a little suspicious that this is the right fix, but it seems to work.
(Assignee)

Updated

5 years ago
Blocks: 762432
status-firefox-esr10: --- → unaffected
status-firefox13: --- → unaffected
status-firefox14: --- → unaffected
status-firefox15: --- → unaffected
Keywords: regression
(Assignee)

Updated

5 years ago
Component: DOM → JavaScript Engine
(Assignee)

Updated

5 years ago
Summary: Compartment mismatch with childNodes, lookupGetter → Use-after-free with childNodes, lookupGetter
(Assignee)

Comment 18

5 years ago
There are a few other uses that should probably also be rooted:

- obj_lookupSetter (basically the same code as the getter)

- JS_GetPropertyAttrsGetterAndSetterById ( http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#3967 ).  I guess this should be changed?

- XrayWrapper<Base, Traits>::defineProperty ( http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#1417 ) seems to need it in the second use, but not the first.  The first use just checks if obj is null.
(Assignee)

Comment 19

5 years ago
Really, it seems to me that we should just kill off every use of PropertyDescriptor and JSPropertyDescriptor, except for arguments, where they function as a sort of already_addreffed<> kind of thing.  I'm not sure what the best way to enforce that is, though.  Probably out of scope for this particular bug, which I'd like to keep limited in scope.  Then I can file a public bug about cleaning up this class.
(Assignee)

Comment 20

5 years ago
Created attachment 642425 [details] [diff] [review]
this got a little out of hand, WIP
Ah, looks like this is the same issue as bug 766430. Hopefully luke r+es my patch over there. :-)
Or maybe this is the more correct fix? Let's see what luke thinks.
Doh! bug 767273. ;-)
(Assignee)

Comment 24

5 years ago
So, yeah, as per 767273 this is just a case of getPropertyDescriptor being called with an uninitialized struct, not a use-after-free.

I tried explicitly initializing PropertyDescriptor with 0xDEADBEEF, and you end up crashing with a compartment mismatch assertion.  When it is properly initialized, as happens in the constructor for the rooting descriptor, it is okay.
Summary: Use-after-free with childNodes, lookupGetter → Uninitialized local variable with childNodes, lookupGetter
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 767273
(Assignee)

Comment 26

5 years ago
Clearing the tracking flag because this is a dupe of another tracked (and fixed) bug.
status-firefox16: affected → fixed
status-firefox17: --- → fixed
tracking-firefox16: + → ---
Whiteboard: [advisory-tracking-]
Are these testcases different enough from bug 767273 that they're worth checking into the tree? If not please set the flag to in-testsuite- (minus).
Group: core-security
Flags: in-testsuite?
Whiteboard: [advisory-tracking-] → [advisory-tracking-][sg:dupe 767273]
A testcase for this bug was already added in the original bug (bug 767273).
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.