Closed Bug 352064 Opened 18 years ago Closed 18 years ago

Error finalizing JS objects causes LiveConnect crash (possibly exploitable)

Categories

(Core :: JavaScript Engine, defect)

1.8 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: smichaud, Assigned: smichaud)

Details

(Keywords: verified1.8.0.9, verified1.8.1.1, Whiteboard: [sg:critical?] uses freed memory)

Attachments

(5 files, 1 obsolete file)

I will attach a test-case Java applet (plus invoking HTML code) that
demonstrates the problem, plus crash logs (generated in Firefox
1.5.0.6 on Mac OS X, SuSE Linux 9.2 and Windows XP Pro) that
demonstrate the potential for exploitability.

I'll also attach a patch to JavaObject_finalize() and
jsj_GC_callback() in js/src/liveconnect/jsj_JavaObject.c that seems to
fix the problem.

At least in the context of my test case, JavaObject_finalize() is
called from js_FinalizeObject() in js/src/jsobj.c when the JavaScript
garbage collector wants to get rid of a "JavaObject".  Whether or not
JavaObject_finalize() has succeeded, js_FinalizeObject() drops the
object map and frees the slots for the JavaObject (or any other kind
of JavaScript object) that it's trying to finalize.  But, under some
circumstances, JavaObject_finalize() _doesn't_ succeed, and _doesn't_
stop the JavaObject from continuing to be used.  As it does keep
getting re-used, eventually you get a crash.

The crashes are almost always under
JS_EvaluateUCScriptForPrincipals(), and in turn under js_Invoke(),
js_Execute() or js_Interpret().  They appear always to be access
violations, and often (but not always) the instruction pointer gets
set to values that appear to be inside strings (e.g. 0x20202020) --
which suggests heap corruption and/or buffer overflows.  But I suspect
that these crashes would be hard to exploit -- since it would probably
be hard to control _which_ data structure (and which part of that
structure) the instruction pointer ends up being set to.

I don't really know who to cc this bug to -- so feel free to add cc's
for others you think should be aware of it.  (I just chose the last
two people who'd made changes to jsj_JavaObject.c, plus Jesse
Ruderman, who I know is interested in finding security holes at least
in the Mac versions of Mozilla.org browsers.)
Here's my test case.  You'll have to reload the applet at least once
to get a crash.  In my experience, crashes are easiest to get on Mac
OS X and Windows (I'm not sure why).  On Linux I generally have to
reload the applet many times, quickly, before I get a crash.
Attached file OS X crash log
These crash logs are selected to demonstrate the potential for
exploitation.  Only about half the crashes I've seen set the
instruction pointer to what's obviously the inside of some data
structure.
Attached file Linux crash logs
The Linux and Windows crash logs were generated by running Firefox
1.5.0.6 inside gdb.  Without gdb, the browser always dies without
putting up any OS or Talkback crash dialog.
Attached patch Patch that fixes the problem (obsolete) — Splinter Review
In the original code for JavaObject_finalize(), jsj_EnterJava() is
always called -- regardless of whether that's actually necessary
(regardless of whether or not jEnv needs to be set).  But
jsj_EnterJava() can fail if it's re-entered from a different JSContext
-- and that can happen while a JavaScript "alert" window is open
during a Java-to-JavaScript call to "alert()" (e.g. while a call to
JS_CallFunctionValue() from nsCLiveconnect::Call() still hasn't
returned).  (I'm still not sure _why_ this happens, but I've seen that
it does happen.  It may have something to do with the fact that the
"alert window" and the window it was popped up from have different
JSContexts.)

If the original code's call to jsj_EnterJava() fails for any reason,
the JavaObject that's being finalized doesn't get removed from the
java_obj_reflections hashtable -- so it keeps getting re-used.  But,
though its ClassDescriptor also doesn't get released,
js_FinalizeObject() still drops its map and frees its slots -- which
means that when the JavaObject is re-used, references will be made to
structures which have already been finalized.

I've tested this patch with Firefox 1.5.0.6 on Mac OS X, Windows XP
and SuSE Linux.  Since jsj_JavaObject.c is the same on the trunk and
all branches, this patch should apply to any of them without offsets
(and should work without problems).
Here's something I forgot to mention that makes it impossible to test
this bug on Mac OS X on the trunk -- a change to the
nsIScriptSecurityManager interface on 2006-08-21 broke the Java
Embedding Plugin's support for JavaScript-to-Java LiveConnect.  For
more information see bug 350664.
Well?
(In reply to comment #7)
> Well?
> 

Could you attach the patch without printf statements? 

Here it is.
Attachment #237654 - Attachment is obsolete: true
Guys, who should review this? Also this probably belong to Java-LiveConnect, right?
Comment on attachment 238446 [details] [diff] [review]
Same patch without printf statements


> JS_EXPORT_API(void)
> JavaObject_finalize(JSContext *cx, JSObject *obj)
> {
>     JavaObjectWrapper *java_wrapper;
>     jobject java_obj;
>     JNIEnv *jEnv;
>     JSJavaThreadState *jsj_env;
> 
>     java_wrapper = JS_GetPrivate(cx, obj);
>     if (!java_wrapper)
>         return;
>     java_obj = java_wrapper->java_obj;
> 
>-    jsj_env = jsj_EnterJava(cx, &jEnv);
>-    if (!jEnv)
>-        return;
>-
>     if (java_obj) {
>         remove_java_obj_reflection_from_hashtable(java_obj, java_wrapper->u.hash_code);
>-

Nit: please don't remove this blank line.

Patch looks good to me.  Sorry for delay in responding to bugmail, problem is that liveconnect is ownerless at the moment (Sun folks who owned it changed jobs).  Javier, could you review?  If you r+, please land on the trunk it with the nit picked, or ask for someone else to do it if you are as jammed up with other bugs as I am ;-).

/be
Attachment #238446 - Flags: superreview+
Attachment #238446 - Flags: review?(jhpedemonte)
Attachment #238446 - Flags: approval1.8.1?
Attachment #238446 - Flags: approval1.8.0.8?
Pushing to 1.8.1.1 - can you land on trunk asap so we can start the baking
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.8?
Comment on attachment 238446 [details] [diff] [review]
Same patch without printf statements

pushing to 1.8.1.1 since we need some time on trunk to bake this...
Attachment #238446 - Flags: approval1.8.1? → approval1.8.1-
Comment on attachment 238446 [details] [diff] [review]
Same patch without printf statements

>+        jsj_env = jsj_EnterJava(cx, &jEnv);
>+        if (jEnv) {
>+            jsj_ReleaseJavaClassDescriptor(cx, jEnv, java_wrapper->class_descriptor);
>+            JS_free(cx, java_wrapper);
>+            jsj_ExitJava(jsj_env);
>+        } else {
>+            java_wrapper->u.next = deferred_wrappers;
>+            deferred_wrappers = java_wrapper;
>+        }

Why do you have the 'deferred_wrappers' code in the jsj_EnterJava 'failure' case?
> Why do you have the 'deferred_wrappers' code in the jsj_EnterJava
> 'failure' case?

Because otherwise the ClassDescriptor, "java_wrapper" and global ref
will never be freed.
Comment on attachment 238446 [details] [diff] [review]
Same patch without printf statements

Looks good.  I can't check in and keep track of tboxen today, so if someone else could check this in, that would be great.  Otherwise, I'll check it in tomorrow.
Attachment #238446 - Flags: review?(jhpedemonte) → review+
> and global ref

Oops, this doesn't need to be freed in this case (since it doesn't
exist).  And I suppose it _might_ be the case that, in the absence of
a java_obj you'll never have a ClassDescriptor or "java_wrapper" that
need to be freed.  But I was just being very conservative.
i don't understand why this didn't block the release.
(In reply to comment #18)
> i don't understand why this didn't block the release.

I think you should know why.  You've been around long enough.  The security bug policy requires us to keep bugs security-sensitive until patches are well-proven and distributed downstream.  That's not obviously the case here. I say this having sr+'ed the patch; my point is that the hour is late and liveconnect is unowned and undertested; testimony of the high-quality submitter, smichaud; and code review by people associated with js/src, me; only go so far in the end-game of a release.

What's more, we will always have bugs such as this coming in as we try to freeze and finalize a release candidate.  If we do as you seem to suggest, we will never ship any software -- and that's completely counterproductive to improving the real world security of the products that people actually use.

With the patch update system, we can guarantee to fix this bug for everyone who downloads Firefox 2 in six to eight weeks, at the first regularly scheduled dot release.  That's what we should do too, in order to get all the other security fixes that have been in 1.8.1/Firefox 2, baking for weeks or months, out into users' hands.

So what exactly did you not understand, really?

/be
(In reply to comment #17)
> > and global ref
> 
> Oops, this doesn't need to be freed in this case (since it doesn't
> exist).  And I suppose it _might_ be the case that, in the absence of
> a java_obj you'll never have a ClassDescriptor or "java_wrapper" that
> need to be freed.  But I was just being very conservative.

Can you prove one way or the other whether this code is needed?  If not, can you prove it doesn't hurt -- that it's conservative in all cases?  "Prove" meaning make a convincing argument, nothing fancy :-).

I'll try to find time to read the relevant code, but I thought I would throw this out now, since at least you and Javier may have more time to look.  Thanks,

/be
(In reply to comment #20)

I'll assume that you're talking about the patch as a whole, and not
just part of it (e.g. not just the jsj_EnterJava failure case).

I think I've already proven that you can't put the call to
remove_java_obj_reflection_from_hashtable() inside a
jsj_EnterJava()/jsj_ExitJava() block.  In other respects I tried to
leave the code as close as possible to what it was originally.

Aside from having too inclusive a jsj_EnterJava()/jsj_ExitJava()
block, I think the original code already behaved quite safely:  All
possibilities were covered with regard to java_obj (whether or not it
was NULL) and jsj_EnterJava() (whether or not it succeeded).  I don't
know for sure whether it's necessary to postpone deleting the
java_obj's global ref -- but it's not unreasonable, and does't cost
much.  And in any case that's how the code was written before I got to
it.  I've added the possibility of deferring part of the finalization
of a java_wrapper object even when no java_obj exists -- but again I
don't think this is unreasonable or costs too much (especially since
the deferral mechanism already existed, and was already being used).

I think this takes care of the "conservative" side -- I think I've
been as conservative as possible.

As to whether the code might (and might have been) doing more than is
strictly necessary, I don't yet know.  (I'll look into it over the
next few days.)  But it's always better to err on the side of caution.
Let me say that I think Mike has the best strategy (from comment 12
and comment 13):  If no-one can find something specifically wrong with
the patch (or can come up with a better alternative), please put land
it on the trunk as soon as possible, so it can "bake" there.

I don't know the JavaScript code very well (either the Liveconnect
part or the rest of it).  And it doesn't seem like _anyone_ currently
working on Mozilla.org browsers knows the Liveconnect part very well.

But I've found a serious problem, plus a very simple and
straightforward solution to it.

So clearly you guys need to do _something_, and your best bet is
probably my patch (or some variation on it).  But it should also be in
the hands of testers for a while before it gets into a release
version.
> And I suppose it _might_ be the case that, in the absence of a
> java_obj you'll never have a ClassDescriptor or "java_wrapper" that
> need to be freed.

I've found out that you _can_ have a "java_wrapper" and
ClassDescriptor without a java_obj.  So my patch (and the previous
code) was right to assume this might be possible.

Starting at line 245 in jsj_JavaObject.c (in jsj_WrapJavaObject()) you
have the following three lines:

    java_obj = (*jEnv)->NewGlobalRef(jEnv, java_obj);
    java_wrapper->java_obj = java_obj;
    if (!java_obj)
        goto out_of_memory;

So if (*jEnv)->NewGlobalRef() fails, goto out_of_memory.  At
out_of_memory you have:

  out_of_memory:
    /* No need to free js_wrapper_obj, as it will be finalized by GC. */
    JS_ReportOutOfMemory(cx);
    return NULL;

This code assumes the garbage collector will free js_wrapper_obj --
i.e. that JavaObject_finalize() will be called on it.
> Starting at line 245 in jsj_JavaObject.c (in jsj_WrapJavaObject())
> you have the following three lines:

Should be line 238 (I was looking at my patched copy of this file).
I'll take care of checking this into the trunk shortly.
Checked in to trunk. ->FIXED
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Restoring lost blocking flag
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.8?
test added to an internal test suite.
Flags: in-testsuite+
Flags: blocking1.8.0.8?
Whiteboard: [sg:critical?] uses freed memory
verified fixed 1.9 20060929 windows/linux
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Any volunteers for checking into the branches?
Attachment #238446 - Flags: approval1.8.1.1?
Comment on attachment 238446 [details] [diff] [review]
Same patch without printf statements

a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin
Attachment #238446 - Flags: approval1.8.1.1?
Attachment #238446 - Flags: approval1.8.1.1+
Attachment #238446 - Flags: approval1.8.0.9?
Attachment #238446 - Flags: approval1.8.0.9+
Could use help with branch landings.

/be
Assignee: general → smichaud
mozilla/js/src/liveconnect/jsj_JavaObject.c 	1.40.20.1
mozilla/js/src/liveconnect/jsj_JavaObject.c 	1.40.12.1
Target Milestone: --- → mozilla1.8.1
Version: Trunk → 1.8 Branch
no crash in 20061130 1.8.0.9, 1.8.1.1, 1.9 nightly or debug builds on winxp but with a debug 1.8.0.9 build I see the following on shutdown:

JS engine warning: 257 GC roots remain after destroying the JSRuntime.
                   These roots may point to freed memory. Objects reachable
                   through them have not been finalized.
I don't think this patch can be the cause of your problem -- the patch
never prevents a "JavaObject" from being garbage-collected.

In any case, does backing out the patch make any difference to your
results?
I should have been clearer:  This patch never prevents a "JavaObject"
from being garbage-collected, and (as far as I can tell) never even
postpones its gargage-collection.  At most it may postpone freeing the
"java_wrapper" and its "class_descriptor".  But the "GC root" has
already been freed.
(In reply to comment #35)

> In any case, does backing out the patch make any difference to your
> results?

I assume it will crash without the patch.
And if you disable the test for this bug (which you mentioned in
comment #28) you don't see the "GC roots remain" problem?
(In reply to comment #38)
> And if you disable the test for this bug (which you mentioned in
> comment #28) you don't see the "GC roots remain" problem?
> 

I'm not quite sure what you mean by disable the test, but if I just start and stop the browser, or load the java implementation test at java.com I don't see the GC roots issue.
So I guess the _only_ formal test you were running (with which you got
the "GC roots remain" problem) was the test for this bug (mentioned in
comment #28).  What happens if you run a bunch of other JavaScript
tests (not necessarily involving LiveConnect)?  I'll bet you still see
the "GC roots remain" problem.
(In reply to comment #40)
> So I guess the _only_ formal test you were running (with which you got
> the "GC roots remain" problem) was the test for this bug (mentioned in
> comment #28).  

yes.

> What happens if you run a bunch of other JavaScript
> tests (not necessarily involving LiveConnect)?  I'll bet you still see
> the "GC roots remain" problem.
> 

Nope. I don't see that message on any of the non liveconnect js tests from this mornings run on any of the branches|platforms.
I still don't think there's any way my patch can be causing the "GC
roots remain" problem.  I believe you've found some other bug that's
also triggered by the test(s) for this bug.

But, since backing out my patch will just bring back the old crashes,
my hypothesis is difficult to test.

Please point me to the test(s) you've been running.  I'm not familiar
with Mozilla.org's testing infrastructure.  But if I can look at your
test script(s), and (ideally) also play with them, I may be able to
find a way to trigger the "GC roots remain" bug without triggering the
crashes for which I originally opened this bug ... which (of course)
would make it possible to test what happens when you back out my
patch.
> I'm just using attachment 237646 [details].

Oh.  And here I thought you'd been using all the fancy testing
infrastructure that I just heard about at the Firefox Summit :-)

I have a bad feeling about this -- I suspect I'll only be able to
prove my point by finding the other bug(s) and fixing it (or them).
But teasing out my fix for _this_ bug took a solid two weeks, and it's
going to be a while before I once more have that kind of time to
spend.

In the meantime, does the "GC roots remain" problem happen only on the
1.8.0.9 branch?  (And is this branch what will become Firefox
1.5.0.9?)
(In reply to comment #44)
> 
> In the meantime, does the "GC roots remain" problem happen only on the
> 1.8.0.9 branch?  (And is this branch what will become Firefox
> 1.5.0.9?)
> 

yes (windows and linux) and yes. Do you have a 1.8.0.9 build without this patch handy? Maybe the GC root thing will show up if you don't do the multiple refresh? If you don't have a build, let me know and I'll make one.

verified fixed 20061130 1.8.1.1 windows/linux

note I get ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../dist/include/xpcom/nsCOMPtr.h, line 594 on the trunk on linux but not Windows. Not sure if it is related though.
> yes (windows and linux)

So you don't see the problem on OS X, even with the 1.8.0.9 builds?
Or is it just that you haven't tested on OS X?

> Do you have a 1.8.0.9 build without this patch handy?

I don't (though I could make one sometime this weekend).  Please make
one for me, and let me know where I can download it.  That way you can
test it, too :-)

> ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr'

I doubt this is related.  But try breaking on Assert_NoQueryNeeded()
(while running in gdb), and get a stack trace.
without this patch I still see the GC root problem in 1.8.0.9 with this testcase. I'm looking for the cause, but it is not related to this patch.

verified fixed 1.8.0.9
Bob: do you still see a GC root problem with the testcase? If so please spin off another bug to cover it.
(In reply to comment #48)
> Bob: do you still see a GC root problem with the testcase? If so please spin
> off another bug to cover it.
> 

filed Bug 374680 on a thebes crash in trunk and Bug 374681 for the GC roots on 1.8.0.

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

Attachment

General

Created:
Updated:
Size: