Closed Bug 200016 Opened 21 years ago Closed 20 years ago

Crash accessing Java package from JS

Categories

(Core Graveyard :: Java: OJI, defect)

x86
All
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: security-bugs, Assigned: yuanyi21)

References

Details

(Keywords: crash, testcase, Whiteboard: redesign)

Attachments

(1 file, 2 obsolete files)

From Bugtraq:

Hi,

executing

<scr1pt language="Javascript">
t = new Packages.sun.plugin.javascript.navig5.JSObject(1,1);
</scr1pt>

crashes Netscape 7.02 and Opera 7 on Windows XP.
The active JVM in both tested browsers is Java 1.4.1_02 from Sun.

This liveconnect (javascript-2-java-communication) stuff seems
to be still very dangerous.

Sincerely
Marc Schoenefeld
In jsj_JSObject.c

	/* JNI returns a NULL handle for a Java 'null' */
    if (!handle)
        return NULL;

    return handle->js_obj;

get a invalid handle (handle != 0) from JPI side, just like #183092
Status: NEW → ASSIGNED
I'm sorry, no -- if an exception was thrown, the return value should be null. 
Anything else is a bug in the JNI, JPI, JVM interface glue, or whatever it's
called these days.

/be
I remember that another bug is also happen on the reason. (crash because of
handle->js_obj;)

Yes, I will push them (Sun 's JPI team member) to fix this on JPI side.
It is not elegant to check exception anywhere and reduce the performance.

I inserted checking exception code in this case to test, can't get exception, I
will check inside JPI.

Brendan,
Are you on IRC mozilla channel?
should we talk about this kind of bugs?
I can't get exception from JPI and the handle is also invalid,
Xiaobin:
Please check this.
Thanks!
->All OS
OS: Windows 2000 → All
filed a P1 bug in Sun's bugtraq #4850496
this is a dupe of bug 199694  ?
*** Bug 199694 has been marked as a duplicate of this bug. ***
Keywords: crash
Severity: normal → critical
Keywords: testcase
that was backwards

this is the dupe.  bug 199694 is the original.
IMHO this bug should block 1.5 since people at bugtraq start in believing that
we are not intressted in fixing criticial bugs.
Flags: blocking1.5?
Flags: blocking1.4.1?
Joshua, any hope of a fix for 1.5final?  We need it this week, if possible.

/be
This bug can't be fixed on mozilla side. It must be fixed on Java Plugin side.
I will push JPI member to fix it.
The problem is due to invalid cast between jint and JSObjectHandle. 

jsj_JSObject.c Ln 313:
  handle = (JSObjectHandle*)JSJCallbacks->unwrap....

It calls to lcglue.cpp Ln 400. That function returned a jint. JPI side returns 
the right value "1". 

The fix should be either JPI side or browser side correctly malloc 
JSObjectHandle and assign the returned value from callback function to handle-
>js_obj.

A typo in my last comment, 
The fix should be either OJI side or liveconnect side to correctly allocate 
JSObjectHandle and assign the right value to the struct.
Xiaobin, I am not an expert on this code, and I agree the cast is ugly, but that
doesn't seem to be the cause of the crash.  Is it not the case that every handle
returned by unwrap_java_wrapper must have previously been passed into the OJI
implementation via a get_java_wrapper call?  The only call to get_java_wrapper
that I can see is in jsj_WrapJSObject, and jsj_WrapJSObject does indeed allocate
a JSObjectHandle for the OJI to associate with a Java object.

Is the problem that some other Java object than one created as a wrapper for a
JS object is being "unwrapped"?

/be
Ok, let me try to clarify.

When the user uses t = new Packages.sun.plugin.javascript.navig5.JSObject(1, 1),
what the browser really does is, it invokes invoke_java_constructor (see
jsj_method.c) and calls to Java to create a java object, in this case,
sun.plugin.javascript.navig5.JSObject. After that, it calls
jsj_ConvertJavaObjectToJSValue (jsj_convert.c, ln 820) to convert the java
object to a js value, the logic of that function is if the passed in java object
is a jsobject (java type), it will just call jsj_
UnwrapJSObjectWrapper(jsj_JSObject.c, ln 293) to unwrap it to get the contained
native jsobject. 

From the whole call sequence, there is no JSObjectHandle gets created. So it is
 wrong to assume unwrap_java_wrapper will return a JSObjectHandle. It actually
just retrieves the private field "nativeJSObject" which is 1 in this case.

So I guess the communication protocol of the JVM glue interface is breaking here.
 
Yes, I also checked this and found that:

Before "unwrap_java_wrapper_impl" (in lcglue.cpp) is invoked, the
"get_java_wrapper_impl" (in lcglue.cpp) should be invoked to set field
"nativeJSObject" in JSObject.class. In general, mozilla set JSObjectHandle 's
pointer as a int into JSObject.class 's "nativeJSObject" field when
"get_java_wrapper_impl", and get back JSObjectHandle 's pointer when
"unwrap_java_wrapper_impl".

But in this case,  "get_java_wrapper_impl" is not invoked before
"unwrap_java_wrapper_impl", so the JSObjectHandle that get from Java Plugin is
invalid (which is 0x1). it should be JSObjectHandle 's pointer address, and
JSObjectHandle->js_obj = 1.

Xiaobin,
In this testcase: t = new Packages.sun.plugin.javascript.navig5.JSObject(1, 1);
did this "JSObject" conflict with JPI 's "working" JSObject?

or why did JSObject in JPI 's JSObjectHandle be initialized to be 0x1 ?
I modified testcase to be:

<scr1pt language="Javascript">
t = new Packages.sun.plugin.javascript.navig5.JSObject(1, 0);
</scr1pt>

the crash gone, so that the "JSObjectHandle" return from JPI is really the
second parameter.

I think this is not right. checking java_wrapper_obj.
I think this testcase is too special. it conflict JPI 's working "JSObject" with
client "JSObject"
Hi Xiaobin,

Is "netscape/javascript/JSObject" 's class (njJSObject) same to
"sun.plugin.javascript.navig5.JSObject" 's class?

Why "sun.plugin.javascript.navig5.JSObject" 's java_object isInstance of
"netscape/javascript/JSObject"?
Because of "sun.plugin.javascript.navig5.JSObject" 's java_object isInstance of
"netscape/javascript/JSObject". so that it unwrap without wrap.

Too late for 1.4.1
Flags: blocking1.4.1? → blocking1.4.1-
Probably too late for 1.5 but let us know if you get a fix into 1.6 soon and
we'll consider it for 1.5.
Flags: blocking1.5? → blocking1.5-
I looked into the code and I don't think there is an easy fix for this bug. If 
we can fix it, we need to understand why there is a need to use JSObjectHandle 
and find an alternative way to do it.

For most cases, JSObject is wrapped inside JSObjectHandle, however, for the 
testcase mentioned, it directly invokes the constructor of s.p.j.n.JSObject 
without first converting the second param to JSObjectHandle. That is why the 
crashes from.
Did this regress during 1.5?  Did the testcase work in 1.4?

/be
This bug is duplicate of #200016
*** Bug 221982 has been marked as a duplicate of this bug. ***
sorry,  #221982 is mark as dup of this bug.
Whiteboard: redesign
->kyle
Assignee: joshua.xia → kyle.yuan
Status: ASSIGNED → NEW
Brendan, no, the test case does not work in 1.4 too. I'm looking into this 
right now, is there any chance to get it into 1.6?
Status: NEW → ASSIGNED
We'd have to see a patch and evaluate its risk before granting actual approval
for 1.6, but this is definitely something that we'd like to have fixed. Some of
the comments imply that the fix isn't going to be easy, though, so no
guarantees. If it looks like something that needs quite a bit of real-world
testing it may get bumped to 1.7a since it won't even have the 1.6beta shakedown
period.
please nominate this bug as soon as a patch appears to help in getting it
evaluated for the next release
I'm discussing with the java team about preventing users from creating
sun.plugin.javascript.navig5.JSObject in the javascript side (that was a mistake
because JSObject is only used for communication from java to javascript, it's
not supposed to be created in the javascript side). But before the possible fix
in the JRE side could happen (not earlier than JRE 1.5.1, I guess), we have to
hack in the OJI side to block this call. That's ugly but low-risky. 
I think Xiaobin's comment 23 did not exactly cover the real problem.
I don't think blocking sun.plugin.javascript.navig5.JSObject is a good idea 
since that is not the only one public class the user can access. It is 
impractical to block all such calls to construct an object or whatever.


When the creation of sun.plugin.javascript.navig5.JSObject is done, javascript
wants to wrap the java object to a js object, but unfortunately,
sun.plugin.javascript.navig5.JSObject is derived from
netscape.javascript.JSObject, i.e. it's not an ordinary java object, so js turns
to unwrap the java object, that's the real problem. See:
http://lxr.mozilla.org/seamonkey/source/js/src/liveconnect/jsj_convert.c#854

Actually, I'm not just thinking about blocking
sun.plugin.javascript.navig5.JSObject, I'm thinking about blocking all the
access to sun.* packages from js.
Re: comment 33
Though there are thousands of other packages user can access, but only the
classes derived from netscape.javascript.JSObject have this kind of problem.
Kyle,

The easy fix will be to insert some logic in invoke_java_constructor before
calling jsj_ConvertJavaObjectToJSValue.

if (the returned java object instanceof netscape.javascript.jsobject)
   don't try to grap the js_obj from JSObjectHandle

You can add another flag to jsj_UnwrapJSObjectWrapper to indicate that the 
passed in java_wrapper_obj is a raw jsobject instead of being a JSObjectHandle.

Comment on attachment 136917 [details] [diff] [review]
prevent JSObject and its derived classes from being created in js

Xiaobin, I don't understand the 2nd change you said in comment 36. Is that
necessary for this bug?
Attachment #136917 - Flags: review?(Xiaobin.Lu)
Attachment #136917 - Attachment description: prevent JSObject and its derived classes from creating in js → prevent JSObject and its derived classes from being created in js
Kyle,

You change looks good to me assuming remove the support of JSObject 
instantiation through sun package would be agreed by most of people here.

Actually if I understand correctly, the whold problem is the current 
liveconnect code assumes all the JSObject passed from Java side is a 
JSObjectHandle wrapper around the real underlying jsobject. That assumption 
won't be correct if the creation of the JSObject is implicilty created by 
liveconnect engine through OJI liveconnect glue interface.

Coming back, if we still want to support something like  new Packages.sun.
plugin.javascript.navig5.JSObject(1,1), liveconnect code should be able to 
differentiate the normal creation of jsobject and any jsobject passed to 
javaside. This is what I proposed in comment 36. We need a flag to indicate that 
this is a normal creation of jsobject, not anything originally passed from 
Javascript side.


>Actually if I understand correctly, the whold problem is the current 
>liveconnect code assumes all the JSObject passed from Java side is a 
>JSObjectHandle wrapper around the real underlying jsobject.

I'd like to agree to this assumption. If we allow the JSObject to be created in
js, don't you think the following code is weird?

jsfunction () {
  jsobj = new Packages.sun.plugin.javascript.navig5.JSObject(1,this);
  jsobj.call("jsfunction", null);
}
Frankly to say, I don't like this patch which takes such a big effort to
support an useless scenario. But in the interest of fixing this asap, I'd like
to listen to anybody else's option on those two patches.
I was just wondering if the bug I submitted 9 months ago is fixed already, 
but unfortunately not, next time i will check is the 31st of March, wenn this 
bug is getting one year old ..... 
Flags: blocking1.7a?
lets try to make some progress for 1.7beta
Flags: blocking1.7b?
Flags: blocking1.7a?
Flags: blocking1.7a-
Please see the comments in my patch for detail.

I discussed this issue with Xiaobin via emails. We all agree to use this
workaround fix in mozilla side first, then do a better job in the next jre
release.
Attachment #136917 - Attachment is obsolete: true
Attachment #137165 - Attachment is obsolete: true
Attachment #136917 - Flags: review?(Xiaobin.Lu)
Attachment #141712 - Flags: review?(Xiaobin.Lu)
Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

sorry for the indent problem. I should use space instead of tab.
Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

Kyle, please remove the comment about invoking java.lang.SecurityManager....
comment. That comment really needs to be investigated further.
Attachment #141712 - Flags: review?(Xiaobin.Lu) → review+
Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

Okay, I'll take those words back until Xiaobin and I find out the ultimate
solution.
Attachment #141712 - Flags: superreview?(brendan)
Comment on attachment 141712 [details] [diff] [review]
workaround fix - block accessing to all classes in sun.plugin package

I see tabs in the + lines -- please use spaces, and indent to the right column.
 sr=brendan@mozilla.org with that nit picked.

/be
Attachment #141712 - Flags: superreview?(brendan) → superreview+
checked in with comments/indent revised.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Flags: blocking1.7b?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: