Closed
Bug 168316
Opened 22 years ago
Closed 22 years ago
Violating same-origin with Java
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
VERIFIED
FIXED
People
(Reporter: security-bugs, Assigned: security-bugs)
References
Details
(Whiteboard: [sg:blocker])
Attachments
(4 files, 5 obsolete files)
704 bytes,
text/html
|
Details | |
1.11 KB,
text/plain
|
Details | |
1.17 KB,
application/octet-stream
|
Details | |
12.47 KB,
patch
|
security-bugs
:
review+
security-bugs
:
superreview+
chofmann
:
approval+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
Assignee | ||
Comment 3•22 years ago
|
||
Comment 4•22 years ago
|
||
On further testing, latest mozilla trunk with the netscape 7beta java plugin is affected by this bug.
Assignee | ||
Comment 5•22 years ago
|
||
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
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?
Comment 9•22 years ago
|
||
The problem is also present ot latest trunk at least on linux.
This is super serious ... it should block 1.2.
Whiteboard: [sg:blocker]
Comment 11•22 years ago
|
||
Agreed, this is serious.
Assignee | ||
Comment 12•22 years ago
|
||
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.
Blocks: 1.2
Comment 13•22 years ago
|
||
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).
Comment 15•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
> 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 18•22 years ago
|
||
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+
Assignee | ||
Comment 19•22 years ago
|
||
Attachment #103019 -
Attachment is obsolete: true
Attachment #104065 -
Attachment is obsolete: true
Assignee | ||
Comment 20•22 years ago
|
||
Comment on attachment 104086 [details] [diff] [review] Patch v3 carrying forward jst's sr.
Attachment #104086 -
Flags: superreview+
Comment 21•22 years ago
|
||
Feel free to cc: me on any such bug. /be
Comment 22•22 years ago
|
||
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(¤tCX))) > { >- JSContext* currentCX; >- if(NS_SUCCEEDED(mContextStack->Peek(¤tCX))) >+ // 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
Comment 23•22 years ago
|
||
adt approval once drivers approve
Assignee | ||
Comment 24•22 years ago
|
||
This should do it...
Attachment #104086 -
Attachment is obsolete: true
Comment 25•22 years ago
|
||
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()?
Comment 26•22 years ago
|
||
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.
Comment 27•22 years ago
|
||
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.
Assignee | ||
Comment 28•22 years ago
|
||
Joe, Could you send me the testcase that's crashing, or post a link to it here?
Comment 29•22 years ago
|
||
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):
Assignee | ||
Comment 30•22 years ago
|
||
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?
Comment 31•22 years ago
|
||
Yes, I tried it on Solaris. I'll try it on Linux, and see what happens.
Comment 32•22 years ago
|
||
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); } }
Assignee | ||
Comment 33•22 years ago
|
||
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 34•22 years ago
|
||
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+
Comment 35•22 years ago
|
||
Mitch, Patch v5 seems working fine, passed all my test cases.
Comment 36•22 years ago
|
||
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+
Comment 37•22 years ago
|
||
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!
Assignee | ||
Comment 38•22 years ago
|
||
OK, all issues fixed, and tests are passing. Now I just need a=
Attachment #104437 -
Attachment is obsolete: true
Assignee | ||
Comment 39•22 years ago
|
||
Comment on attachment 104589 [details] [diff] [review] patch v6 Carrying over review marks
Attachment #104589 -
Flags: superreview+
Attachment #104589 -
Flags: review+
Comment 40•22 years ago
|
||
Comment on attachment 104589 [details] [diff] [review] patch v6 a=chofmann 1.0.2 and 1.2 trunk
Attachment #104589 -
Flags: approval+
Assignee | ||
Comment 41•22 years ago
|
||
Fixed on trunk and branch.
Comment 42•22 years ago
|
||
Comment on attachment 104589 [details] [diff] [review] patch v6 + if (origin != originBuf1) + PR_Free(origin); Indentation glitch. Fix when you can. /be
Comment 43•22 years ago
|
||
adding gayatri
Comment 44•22 years ago
|
||
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
Keywords: fixed1.0.2 → verified1.0.2
Assignee | ||
Updated•22 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•