Bug 343979

Comment 1 Grant Gayed 2011-04-27 19:18:59 EDT

Created attachment 194222 [details]
patch

Patch contains java changes for all Browser implementations.  The recompiled external.xpt will be in the next comment.

Comment 2 Grant Gayed 2011-04-27 19:19:30 EDT

Created attachment 194223 [details]
recompiled external.xpt

Comment 3 Jun Yue Liu 2011-04-28 04:58:57 EDT

The current patch is good in cross-platform, but I am not sure if it is strong enough.

At least, we should use a bigger token, e.g. two 32bits int. One 32bit int seem easy to be detected.  The firefox 4 on my PC take less than 5 minutes to finish below code.
--------------------------------------------
function loopToken(){
	var i = 0;	
	while ( i < 0x6FFFFFFF ){
		var j = 0;
		while (j < 0xFF) {
			j++;
			i++;
		}
		document.getElementById("prog").innerHTML = i;
		var txt = document.getElementById("prog2").innerHTML;
		txt = ".." + txt;
		if (txt.length > 100)
			txt = "";
		document.getElementById("prog2").innerHTML = txt;
	}
}
-------------------------------------------------------

After all, I would prefer to leverage the browser's security framework, e.g. the nsIScriptSecurityManager and nsIPrincipal for firefox/xulrunner.

Comment 4 B. Chen 2011-04-28 23:26:52 EDT

The token came into the callJava is a pointer value. After called to convertToJava, it returned the correct double object. However I am getting a java core dump when the code processes args pointer on this line

rc = variant.GetDataType (type);

I am running on Windows 7, SWT browser with XULRunner 1.9.2.10

Comment 5 Jun Yue Liu 2011-05-05 23:33:57 EDT

Grant, are you working for a bigger token and better token generator? Do you have an ETA?

Comment 6 guoyun 2011-05-06 01:26:45 EDT

Created attachment 194901 [details]
secure iframe patch

In the patch, remove the for loop code of "iframe[i].theJavaFunction = topFrame.theJavaFunction ", which was used for copying the function reference from top frame to child frame, and which may violate the cross-domain policy between frames.

Comment 7 Grant Gayed 2011-05-06 10:56:47 EDT

This is still targetted for RC1.  The token generation has been changed to the following, which now uses 53 bits and gives a better distribution of values:

Random random = new Random ();
long value = random.nextLong ();
token = ((value & 0xFFE0000000000000L) >>> 11) ^ (value & 0x1FFFFFFFFFFFFFL) ^ hashCode ();

Comment 8 Jun Yue Liu 2011-05-08 22:35:03 EDT

Grant, the new token generation sound great. Thank you!   And, any thought on Yun's patch at comment#6?

Comment 9 guoyun 2011-05-09 03:41:00 EDT

(In reply to comment #7)
Grant, seems the 53 bits token can not be passed into External:callJava() method, since the parameter of XPCOMObject's callback methods are all int types(e.g. "int /*long*/[] callbackArgs"). The token is truncated to 32bit when run into callJava method. 
May use 2 int type tokens is a good idea, what's your opinion?

Comment 10 Grant Gayed 2011-05-09 18:30:18 EDT

Created attachment 195159 [details]
patch with 53-bit token

The attached patch now uses 53-bit tokens.  It has not been reviewed yet, and does not currently contain the changes in comment 6.  This patch and the comment 6 patch will be reviewed/considered tomorrow.

Comment 11 Grant Gayed 2011-05-09 18:31:14 EDT

Created attachment 195160 [details]
external.xpt for 53-bit tokens

Comment 12 guoyun 2011-05-10 01:27:33 EDT

(In reply to comment #10)
The patch worked. But from security view, the token used still is 32bit, not 53bit, because the token used in window.external.callJava is an int type pointer, which is 32bit. And I verified the address pointer can be accessed by another browser instance with window.external.callJava js code( e.g. window.external.callJava(1,4072923143722503,['hi']);).

Comment 13 Grant Gayed 2011-05-10 16:02:06 EDT

(In reply to comment #12)
> The patch worked. But from security view, the token used still is 32bit, not
> 53bit, because the token used in window.external.callJava is an int type
> pointer, which is 32bit. And I verified the address pointer can be accessed by
> another browser instance with window.external.callJava js code( e.g.
> window.external.callJava(1,4072923143722503,['hi']);).

The 32-bit tokenVariant argument to callJava is a pointer to an nsIVariant containing the token, not to the token itself.  It contains the 53-bit token, which is accessed in convertToJava() with nsIVariant.GetAsDouble().

Comment 14 Grant Gayed 2011-05-10 16:09:15 EDT

Created attachment 195272 [details]
latest patch

This patch has a couple of small changes, following the review with SSQ.  It is released > 20110510.

Regarding not hooking listeners on child frames, we still need to investigate this further because it is a behavior change.  It can still be considered for 3.7RC1.

Comment 15 Grant Gayed 2011-05-11 10:47:08 EDT

Marking as FIXED since calls to window.external.callJava() now require a 52-bit token, which was the focus of this report.  The patch from comment 6 is applicable to original bug 333972, which is still open.

Comment 16 B. Chen 2011-05-11 11:49:30 EDT

Grant, thanks for fixing this ! Would it be possible for you to attach a patch against 3.6.2 release?

Comment 17 Grant Gayed 2011-05-11 18:16:00 EDT

I'll look into this, it could be a couple of days.

Comment 18 Grant Gayed 2011-05-12 17:02:35 EDT

Created attachment 195546 [details]
final 3.7-stream patch

The previous 3.7-stream patch had an error in its WebKit on GTK implementation.  This is now fixed in HEAD, and is contained in this updated patch.

Comment 19 Grant Gayed 2011-05-12 17:11:42 EDT

Created attachment 195550 [details]
patch for 3.6.2

Here's a patch that applies this report's changes against 3.6.2, including the small fix that was added in the previous comment.  Remember that the external.xpt attached in comment 11 is also needed.