Closed Bug 168316 Opened 22 years ago Closed 22 years ago

Violating same-origin with Java

Categories

(Core :: Security, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: security-bugs, Assigned: security-bugs)

References

Details

(Whiteboard: [sg:blocker])

Attachments

(4 files, 5 obsolete files)

There is a serious bug with Java/Javascript which affects at least Netscape 7
beta and the latest AOL build I have.
The problem is Java can access properties from other windows.
The collected data may be sent either from Java or from Javascript with
Liveconnect.
On further testing, latest mozilla trunk with the netscape 7beta java plugin is
affected by this bug.
From Georgi:
With a combination of bugs 59767 and 168316 it is possible to read local
files. Check attached files. Inline comments show what to change to read another
local file.
Severity: normal → critical
Ccing some OJI folks to look at this serious bug.
Seemed WFM with mozilla 1.1beta (released on 07/21) and JRE 1.4.1 on Windows2000
and Solaris: no source (of mozilla.org page) was displayed in the alert box,
instead, only a small alert box was popped up. In the java console, it showed
that a "null" was retrived, instead of the source page. But no js security
exception shown in the javascript console though.

On the other hand, the problem was reproduced with the later builds: mozilla 1.1
(released on 08/26), netscape7.0, all with the same JRE build, regardless on
Windows or Solaris.

The problem was also reproced with mozill 1.1alpha (released on 06/11).

This is odd, so far it seems only mozilla 1.1 beta (released on 07/21) working
OK (no showing the page source of mozilla.org). What makes it different?
The problem is also present ot latest trunk at least on linux.
This is super serious ... it should block 1.2.
Whiteboard: [sg:blocker]
Agreed, this is serious.
Attached patch Patch - work in progress (obsolete) — Splinter Review
it compiles - that's all I can say right now. We still need to make sure
security exceptions get handled right. Needs lots of testing too.
I applied the patch to my trunk build, and it seemed working OK, and the source
page did not show, instead, a "java.lang.NullPointerException" was displayed in
the java console.
Comment on attachment 103019 [details] [diff] [review]
Patch - work in progress

This is mostly style review for now, I would need someone to explain what some
of the things really do before I can give review.

A general comment first: there are some tab chars here which mess up the
indentation, please get rid of them.

>Index: caps/idl/nsIScriptSecurityManager.idl
>===================================================================
>Index: caps/src/nsScriptSecurityManager.cpp
>===================================================================
>+    nsCOMPtr<nsIScriptContext> scriptContext =
>+        NS_REINTERPRET_CAST(nsIScriptContext*, JS_GetContextPrivate(cx));

Does this require reinterpret cast or would static cast do?

>+        scriptContext->GetGlobalObject(getter_AddRefs(global));
>+        if (global)
>+        {
>+            nsCOMPtr<nsIScriptObjectPrincipal> globalData(do_QueryInterface(global));
>+            if (globalData)

You can avoid the |if (global)| statement because QI is null safe.

>+                nsCOMPtr<nsIPrincipal> principal;

This line is unneeded.

>Index: js/src/liveconnect/Makefile.in
>===================================================================
>+		  caps \
>+		  necko \
>+		  string \
>+		  pref \

I don't really like adding this many new deps, but I must say I am surprised
liveconnect was not already depending on CAPS. It seems like we could easily
remove some of these direct deps.

>Index: js/src/liveconnect/nsCLiveconnect.cpp
>===================================================================
>+#include "nsScriptSecurityManager.h"

Why is this needed?

>+    char origin[4096];
>+    rv = securityContext->GetOrigin(origin, sizeof(origin));
>+    if (NS_FAILED(rv)) return rv;

Does this method fail if there is not enough space?

>+		memset(&mFrame, 0, sizeof mFrame);

Missing parenthesis?

>+        goto done;

When using goto, be careful with potentially uninitialized variables, we have a
few in the tree and want to get rid of them. Compiling on Linux will show them,
but you can check that manually as well (look for variables that are defined
after that goto, or assigned values after the goto).
Did some more testing, using some of Java plugin folks' test cases, and the
patch still seemed working OK.
So as far as fixing the bug, the patch seems doing its job.
> You can avoid the |if (global)| statement because QI is null safe.
I'm not sure what you mean by this. It's possible for the QI to return null,
and if so we don't want to pass the result on.

The additional dependencies are unfortunate but apparently unavoidable. I
managed to remove the "pref" dependency, but there seems to be no way to create
an NSIURI without the "string" dependency. This could be solved by putting this
whole function in NSScriptSecurityManager, but I don't want to do that, since
that interface is getting cluttered with special functions that are only called
in one place.

> Does this method fail if there is not enough space?
Yes.

> Missing parenthesis?
sizeof doesn't actually require parenthesis. beard wrote it without the parens,
but I think that looks weird, so I put them back in.
Attachment #103019 - Attachment is obsolete: true
I meant that instead of:

+        scriptContext->GetGlobalObject(getter_AddRefs(global));
+        if (global)
+        {
+            nsCOMPtr<nsIScriptObjectPrincipal>
globalData(do_QueryInterface(global));
+            if (globalData)
+                globalData->GetPrincipal(result);
+        }

you can do:

+        scriptContext->GetGlobalObject(getter_AddRefs(global));
+        nsCOMPtr<nsIScriptObjectPrincipal> globalData(do_QueryInterface(global));
+        if (globalData)
+            globalData->GetPrincipal(result);

because if global==0 then QI of that will still give 0, and you can just test
for the QId value.
Comment on attachment 103019 [details] [diff] [review]
Patch - work in progress

- In nsScriptSecurityManager::GetPrincipalFromContext():

+    nsCOMPtr<nsIScriptContext> scriptContext =
+	 NS_REINTERPRET_CAST(nsIScriptContext*, JS_GetContextPrivate(cx));

This code, as well as all other code that gets an nsIScriptContext out of a
JSContext, should check that ::JS_GetOptions(aContext) &
JSOPTION_PRIVATE_IS_NSISUPPORTS is true before assuming that the context
private data is an nsISupports. And to be correct, the above code should cast
to nsISupports, and then QI to nsIScriptContext.

+    if (scriptContext)
+    {
+	 nsCOMPtr<nsIScriptGlobalObject> global;
+	 scriptContext->GetGlobalObject(getter_AddRefs(global));
+	 if (global)
+	 {
+	     nsCOMPtr<nsIScriptObjectPrincipal>
globalData(do_QueryInterface(global));
+	     if (globalData)
+	     {
+		 nsCOMPtr<nsIPrincipal> principal;
+		 globalData->GetPrincipal(result);
+	     }
+	 }
+    }
+    return NS_OK;

Alternatively, you could call nsIXPConnect's GetWrappedNativeOfJSObject() on
::JS_GetGlobalObject(cx) and get the native out of that wrapper, and then QI it
to nsIScriptObjectPrincipal, that way you'd avoid having to check that the
context's private really is an nsIScriptContext.

- In nsCLiveconnect.cpp:

-    AutoPushJSContext(JSContext *cx)
+    AutoPushJSContext(nsISupports* aSecuritySupports, JSContext *cx, nsresult*
rv)
+		:	mContext(cx)

Remove the tabs, fix the indentation...

And reading on I think that, given that you're adding a fair amount of new code
to the AutoPushJSContext class, you should un-inline the constructor, i.e.
break the code out of the class declaration to avoid duplicating the code all
over when compiled.

@@ -99,6 +133,53 @@
		     // Leave the reference to the mContextStack to
		     // indicate that we need to pop it in our dtor.	        
		 }
+
+	     }
+	 }
+
+		memset(&mFrame, 0, sizeof mFrame);

Loose the tabs, and put parens around the mFrame 'argument' to sizeof, for
consistency, if nothing else...

+	 PRBool hasScript = PR_FALSE;
+	 JSStackFrame* tempFP = cx->fp;
+	 while (tempFP && !hasScript)
+	 {
+	     if (tempFP->script)
+		 hasScript = PR_TRUE;
+	     tempFP = tempFP->down;
+	 };

Checking !hasScript in every iteration of that loop is not necessary, I'd
suggest this in stead:

+	 PRBool hasScript = PR_FALSE;
+	 JSStackFrame* tempFP = cx->fp;
+	 while (tempFP)
+	 {
+	     if (tempFP->script) {
+		 hasScript = PR_TRUE;
+
+		 break;
+	     }
+
+	     tempFP = tempFP->down;
+	 };

...

+	     if(aSecuritySupports)

Add a space after 'if'.

...
+			JSPrincipals* jsprinc;
+			principal->GetJSPrincipals(&jsprinc);
+		
+			mFrame.script = JS_CompileScriptForPrincipals(cx,
JS_GetGlobalObject(cx),
+									       
				  jsprinc, "", 0, "", 1);

More tabs...

+			JSPRINCIPALS_DROP(cx, jsprinc);
+
+			if (mFrame.script)
+		 {
+				mFrame.down = cx->fp;
+				cx->fp = &mFrame;
+			}

And again more tabs...

@@ -110,10 +191,16 @@
	     mContextStack->Pop(nsnull);
	     mContextStack = nsnull;
	 }
+
+		if (mFrame.script) {
+			mContext->fp = mFrame.down;
+		}
     }

Tabs... There's a ton of these, please search for all of them (at least the
ones you added) and get rid of them.

...
-    AutoPushJSContext autopush(cx);
+    nsresult rv;
+    AutoPushJSContext autopush(securitySupports, cx, &rv);
+    if (NS_FAILED(rv)) {
+	 JS_ReportError(cx, "failed to get a principal");
+	 goto done;
+    }

Given that the above code appears in a lot of places, you may want to store rv
in AutoPushJSContext and put a little DidSucceed() inline method in
AutoPushJSContext() and hide the JS_ReportError() call in there too.

Other than that, this looks fine with me. sr=jst, but if there's time, I'd like
to see the new patch (post-checkin, if nothing else).
Attachment #103019 - Attachment is obsolete: false
Attachment #103019 - Flags: superreview+
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #103019 - Attachment is obsolete: true
Attachment #104065 - Attachment is obsolete: true
Comment on attachment 104086 [details] [diff] [review]
Patch v3

carrying forward jst's sr.
Attachment #104086 - Flags: superreview+
Feel free to cc: me on any such bug.

/be
Comment on attachment 104086 [details] [diff] [review]
Patch v3

>+#include "jsinterp.h"  // XXX private API so we can auto-push a JSStackFrame
>+#include "jsscript.h"  // XXX private declaration of JSScript

You don't need jsscript.h, but you should include jscntxt.h explicitly, as you
dereference JSContext pointers (cx->fp, e.g.).

>+    char origin[4096];

That seems overlarge.  Domain names are restricted to some smaller length, 256
if memory serves (maybe the limit has been raised since the old days when I
hacked on 4.3bsd Unix kernel code).  URI schemes can't be that long, either. 
I'm concerned that this will blow the stack on stack-challenged platforms. 
Anyway, having a magic size like 4096 seems wrong.

>@@ -81,40 +114,107 @@
> class AutoPushJSContext
> {
> public:
>-    AutoPushJSContext(JSContext *cx)
>-    {
>-        mContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+    AutoPushJSContext(nsISupports* aSecuritySupports, JSContext *cx);
>+
>+    ~AutoPushJSContext();
>+
>+    nsresult ResultOfPush() { return mPushResult; };
> 
>-        if(mContextStack)
>+private:
>+    nsCOMPtr<nsIJSContextStack> mContextStack;
>+    JSContext*                  mContext;
>+    JSStackFrame                mFrame;
>+    nsresult                    mPushResult;
>+};
>+
>+AutoPushJSContext::AutoPushJSContext(nsISupports* aSecuritySupports,
>+                                     JSContext *cx) 
>+                                     : mContext(cx), mPushResult(NS_OK)
>+{
>+    mContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+
>+    if(mContextStack)
>+    {
>+        JSContext* currentCX;
>+        if(NS_SUCCEEDED(mContextStack->Peek(&currentCX)))
>         {
>-            JSContext* currentCX;
>-            if(NS_SUCCEEDED(mContextStack->Peek(&currentCX)))
>+            // Is the current context already on the stack?
>+            if(cx == currentCX)
>+                mContextStack = nsnull;
>+            else
>             {
>-                // Is the current context already on the stack?
>-                if(cx == currentCX)
>-                    mContextStack = nsnull;
>-                else
>-                {
>-                    mContextStack->Push(cx);
>-                    // Leave the reference to the mContextStack to
>-                    // indicate that we need to pop it in our dtor.                                               
>-                }
>+                mContextStack->Push(cx);
>+                // Leave the reference to the mContextStack to
>+                // indicate that we need to pop it in our dtor.                                               
>             }
>+
>         }
>     }
> 
>-    ~AutoPushJSContext()
>+    memset(&mFrame, 0, sizeof(mFrame));

I wrote the original version of this code, so I'm to blame for the lack of
parens around mFrame in 'sizeof mFrame' -- but c'mon, reviewers -- sizeof is a
unary operator, not a function.  Anyway, blame me, and use caps prevailing
style, or do both -- I don't care, but I wanted to say my peace.

>+            mFrame.script = JS_CompileScriptForPrincipals(cx, JS_GetGlobalObject(cx),
>+                                                          jsprinc, "", 0, "", 1);
>+            JSPRINCIPALS_DROP(cx, jsprinc);
>+
>+            if (mFrame.script)
>+            {
>+                mFrame.down = cx->fp;
>+                cx->fp = &mFrame;
>+            }
>+            else
>+                mPushResult = NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY is more precise, but...

>         }
>     }
>+    
>+    if (NS_FAILED(mPushResult))
>+        JS_ReportError(cx, "failed to get a principal");

If you failed because JS_CompileScriptForPrincipals failed, you don't want this
JS_ReportError call -- an error was already reported by JS_CompileScript.... 
Keep track of that case and avoid a double-report.

Also, this if (NS_FAILED(mPushResult)) ...; can go in the then-part of the if
above it, because there's no way mPushResult could be other than its initial
value, NS_OK, if we do have a script (if (hasScript)).	Moving it up may help
avoid the double report, too.

Fix those little issues in a new patch, and r=brendan@mozilla.org.

/be
adt approval once drivers approve
Blocks: blackbird
Keywords: adt1.0.2+
Attached patch patch v4 (obsolete) — Splinter Review
This should do it...
Attachment #104086 - Attachment is obsolete: true
When applying the second file of the patch, got the following error when compiling:
"...
nsScriptSecurityManager.cpp", line 1783: Error:
nsScriptSecurityManager::GetPrincipalFromContext(JSContext*, nsIPrincipal**)
already had a body defined.
..."

After removed the other (older?) GetPrincipalFromContext (), the compiling went OK. 
Is the patch missing the part that removing the old GetPrincipalFromContext()?
Never mind, it looks like I was applying the new patch to the file with the
previous patch. The patch file is OK.
In the process of re-testing the new patch. I will let you know about the results.
The new patch seems to have some problem. When I ran a javascript-to-java test
after applying the patch, the browser crashed (or exited) with the following
error:
"Assertion failure: !(*jEnv)->ExceptionOccurred(jEnv), at jsj_utils.c:69".

If I swapped back the previous patch (the patches in
js/src/liveconnect/Makefile.in and js/src/liveconnect/nsCLiveconnect.cpp), then
the test went OK.

Please let me know if wanting me to post my js-to-java test case.
Joe,
   Could you send me the testcase that's crashing, or post a link to it here?
Here is the javascript-to-java test case I used:

Html file:

<html>
  <head>
    <title>JS to Java Test</title>
  </head>

  <body>
    <center>
      <h1>LiveConnect JS to Java Test</h1>
        <APPLET NAME="MyApplet" CODE = HelloWorld.class WIDTH= 100
                HEIGHT = 100 ALIGN = testtop HSPACE = 50 VSPACE = 100>
        </APPLET>
    </center>

    <INPUT TYPE="Button" VALUE="Print" NAME="Print Button"
        onClick="document.MyApplet.print()">
  </body>
</html>

public class HelloWorld extends Applet {

        public void init()
        {
                System.err.println("calling HelloWorld.init().");
                System.out.println("calling getClass().");
                Class c = getClass();
                System.out.println("got class: " + c);
                System.out.println("calling getClassLoader().");
                ClassLoader cl = c.getClassLoader();
                System.err.println("got class loader: "+cl);
                System.err.println("HelloWorld.init() called.");
        }

        public void print()
        {
                System.err.println("HelloWorld.print() called.");

        }

        public void paint(Graphics g) 
        {
                System.err.println("HelloWorld.paint() called.");
                g.drawString("Hello World!", 25, 50);
        }

}
Java code (HolloWorld.java):




Joe,
   The testcase you just sent works fine for me, without crashing, on Win XP
with patch v4 above. Are you only seeing the crash on Solaris? Can you post a
stack trace of the crash?
Yes, I tried it on Solaris. I'll try it on Linux, and see what happens.
Did some debuging, and found out the problem (assertion failure) some times
occured in the js-to-java case (the first test case already posted), bug always
occured in the java-to-js case when getMember() is called (see the test case
attached below, press "click" button to reproduce the problem). After printing a
message to the console, "Assertion failure: !(*jEnv)->ExceptionOccurred(jEnv),
at jsj_utils.c:69", aborted in JS_Assert() (see below).

The assertion occured to debug builds on all platforms, but only aborted in Unix
(see JS_Assert() code belowy). On the other hand, when using the original patch
(v3), there is no assertion at all. 

Mitch,
This may not be a big problem, but please verify that the exception (which
caused the assertion) is not critical. I tried to find out what the exception
was complaining about, but since I was not familiar with the securityenv code
(where the exception seemed to occur) and could not pin point it.


***** JS_Assert() code:***********

JS_PUBLIC_API(void) JS_Assert(const char *s, const char *file, JSI
ntn ln)
{
#ifdef XP_MAC
    dprintf("Assertion failure: %s, at %s:%d\n", s, file, ln);
#else
    fprintf(stderr, "Assertion failure: %s, at %s:%d\n", s, file,
ln);
#endif
#if defined(WIN32) || defined(XP_OS2)
    DebugBreak();
#endif
#ifndef XP_MAC
    abort();
#endif
}


************ Java-to-js Test case ********************
Html file:

<html>
  <head>
    <title>LiveConnect Test 1</title>
        <SCRIPT LANGUAGE="JavaScript">
        function showAlert(txt)
        {
                alert("JS showAlert: being called " + txt);
        }
        function click_doit()
        {
                document.MyApplet.getMemberTest();
        }
        </SCRIPT>
  </head>

  <body>
    <center>
      <h1>Liveconnect Test: java to javascript</h1>
        <APPLET NAME=MyApplet CODE = java2js.class WIDTH= 100
                HEIGHT = 100 ALIGN = testtop HSPACE = 50 VSPACE = 100 MAYSCRIPT>
        </APPLET>
    </center>

    <form name=myForm>
        Test with getMember called in init:
        <input type=button name=myButton value="Click"
          onClick=click_doit()>
    </form>
  </body>
</html>


Java code (java2js.java):
import java.applet.*;
import java.awt.*;
import netscape.javascript.*;

public class java2js extends Applet {
        JSObject win, doc, app;
        Object app0;

        public void init()
        {
                System.err.println("calling getWindow().");
                win = JSObject.getWindow(this);
                System.err.println("calling getWindow().");
                if (win == null) {
                        System.err.println("getWindow() returned null.");
                        return;
                }
                System.err.println("getWindow() returned non null.");
        }

        public void getMemberTest()
        {

                System.err.println("getMemberTest called.");

                System.err.println("calling getMember() doc.");
                doc = (JSObject) win.getMember("document");
                if (doc == null) {
                        System.err.println("getMember() doc returned null.");
                        return;
                }
                System.err.println("getMember() doc returned non null.");
        }

        public void paint(Graphics g)
        {
                System.err.println("Hi from Java.");
                g.drawString("Hello World 1", 25, 50);
        }

}
Attached patch patch v5 (obsolete) — Splinter Review
Joe,
   I think I found the problem - I was passing a bad size param to
securityContext->GetOrigin. Please try this patch instead, and let me know if
it still asserts - it works for me.
Attachment #104208 - Attachment is obsolete: true
Comment on attachment 104437 [details] [diff] [review]
patch v5

Please remove the extra tabs, thanks.

>+    char originBuf1[512];
>+    char* origin = originBuf1;
>+	size_t originSize = sizeof(originBuf1);
>+    rv = securityContext->GetOrigin(origin, sizeof(origin));

sizeof(origin) should be originSize

sr=dveditz
Attachment #104437 - Flags: superreview+
Mitch,
Patch v5 seems working fine, passed all my test cases.
Comment on attachment 104437 [details] [diff] [review]
patch v5

- In nsScriptSecurityManager::GetPrincipalFromContext():

+    NS_ENSURE_TRUE(::JS_GetOptions(cx) & JSOPTION_PRIVATE_IS_NSISUPPORTS,
+		    NS_ERROR_FAILURE);
+    nsCOMPtr<nsISupports> scriptContextSupports =
+	 NS_REINTERPRET_CAST(nsISupports*, JS_GetContextPrivate(cx));

No need for a strong nsISupports reference here, a raw pointer would do here
and that would avoid the unnecessary AddRef() and Release() on the context.
|scriptContext| (below) will hold a strong reference for you...

+    nsCOMPtr<nsIScriptContext>
scriptContext(do_QueryInterface(scriptContextSupports));

- In nsCLiveconnect.cpp:

>+CreatePrincipal(nsISupports* securitySupports,

+    char originBuf1[512];
+    char* origin = originBuf1;
+	size_t originSize = sizeof(originBuf1);

Tab.

+    rv = securityContext->GetOrigin(origin, sizeof(origin));
+	while (NS_FAILED(rv) && originSize < 65536U)

Tab.

+    {	// Try allocating a larger buffer on the heap
+		if (origin != originBuf1)
+			PR_Free(origin);
+		originSize *= 2;

More tabs.

+	 origin = (char*)PR_Malloc(originSize);
+	 if (!origin)
+	     return NS_ERROR_OUT_OF_MEMORY;
+	 rv = securityContext->GetOrigin(origin, originSize);
+    }

I don't like this loop, it's IMO too fragile. It assumes that GetOrigin() only
fails if the origin needs more space than what's passed in. If GetOrigin()
fails (and it does fail in cases other than the one you expect here) this loop
will run until PR_Malloc() returns null (i.e. we're out of memory), and then
it'll return NS_ERROR_OUT_OF_MEMORY (after swapping like mad, probably), and
that's wrong. IMO the API nsISecurityConext::GetOrigin() is broken, and doing
the right thing with it is hard. I'd suggest you cap off the max size you'll
try to allocate in this code, at some insanely large number (say 32k?) and
return the error code you get from GetOrigin() if you run into a case where
GetOrigin() fails and you're passing it enough space that it shouldn't fail.

- In AutoPushJSContext::~AutoPushJSContext():

+    if(mContextStack)
+    {
+	 mContextStack->Pop(nsnull);
+	 mContextStack = nsnull;

No need to explicitly null out mContextStack here, the nsCOMPtr dtor will do
that for you.

r/sr=jst if you fix those issues.
Attachment #104437 - Flags: review+
Duh! Never mind my ranting about that loop, it's already checked that we don't
allocate too much, I can't believe I didn't see that!
Attached patch patch v6Splinter Review
OK, all issues fixed, and tests are passing. Now I just need a=
Attachment #104437 - Attachment is obsolete: true
Comment on attachment 104589 [details] [diff] [review]
patch v6

Carrying over review marks
Attachment #104589 - Flags: superreview+
Attachment #104589 - Flags: review+
Comment on attachment 104589 [details] [diff] [review]
patch v6

a=chofmann 1.0.2 and 1.2 trunk
Attachment #104589 - Flags: approval+
Fixed on trunk and branch.
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: fixed1.0.2
Resolution: --- → FIXED
Comment on attachment 104589 [details] [diff] [review]
patch v6

+	 if (origin != originBuf1)
+	 PR_Free(origin);

Indentation glitch.  Fix when you can.

/be
adding gayatri
Verified on 2002-10-30-branch build on Win 2000.

Ran the test cases : http://warp/u/mstoltz/bugs/168316.html
 and                 http://warp/u/mstoltz/bugs/168316a.html

And the results were correct.
Status: RESOLVED → VERIFIED
Group: security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: