Closed
Bug 239122
Opened 20 years ago
Closed 20 years ago
Liveconnect can be used to read any file on user's filesystem
Categories
(Core Graveyard :: Java: Live Connect, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.7final
People
(Reporter: darin.moz, Assigned: yuanyi21)
Details
(Keywords: regression, Whiteboard: [sg:fix]doesnotaffect1.4)
Attachments
(2 files, 3 obsolete files)
569 bytes,
text/html
|
Details | |
16.30 KB,
patch
|
jst
:
review+
brendan
:
superreview+
chofmann
:
approval1.7+
|
Details | Diff | Splinter Review |
Liveconnect can be used to read any file on user's filesystem. The patch for bug 146458 makes it possible to use Liveconnect to read any file on the user's filesystem. The change to nsCSecurityContext.cpp in the Implies method appears to be the problem. Attaching test case and patch. The exploit is possible with Mozilla 1.7 beta. I've only tested the Linux build.
Reporter | ||
Comment 1•20 years ago
|
||
Assignee: live-connect → darin
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•20 years ago
|
||
this patch backs out part of the patch for bug 146458.
Comment 3•20 years ago
|
||
the comment no longer matches the code with this patch..,.
Reporter | ||
Updated•20 years ago
|
Attachment #145052 -
Flags: superreview?(brendan)
Attachment #145052 -
Flags: review?(jst)
Reporter | ||
Updated•20 years ago
|
Attachment #145052 -
Flags: superreview?(brendan)
Attachment #145052 -
Flags: review?(jst)
Reporter | ||
Updated•20 years ago
|
Attachment #145053 -
Flags: superreview?(brendan)
Attachment #145053 -
Flags: review?(jst)
Reporter | ||
Updated•20 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
Please hold on for a moment. I'd like to dig into this problem a little more and report what I get asap.
Comment 6•20 years ago
|
||
This is a serious security problem, setting blocking1.7+.
Flags: blocking1.7+
Comment on attachment 145053 [details] [diff] [review] v1.1 patch sorry, darin, I think your patch will break the fix of bug 146458. I'll post a new patch soon.
my new patch set the UniversalBrowserRead to be true *only* when there is a liveconnect call from javascript to java applet (which is what people complain in bug 146458). I'm sorry that I did not find java plugin uses UniversalBrowserRead this way on Linux before.
Attachment #145067 -
Flags: review?(jst)
Reporter | ||
Comment 9•20 years ago
|
||
(In reply to comment #7) > (From update of attachment 145053 [details] [diff] [review]) > sorry, darin, I think your patch will break the fix of bug 146458. I'll post a > new patch soon. > yeah, lesser of two evils... better to avoid a security bug in favor of limiting functionality, etc. however, i'm all for a better solution.
Comment 10•20 years ago
|
||
Comment on attachment 145067 [details] [diff] [review] my patch Uh, is this safe from attacks using multiple windows to make several simultanous calls to Java (applet or liveconnect). This uses global state to know if a call is to an applet or not, that doesn't seem safe. Can this boolean be pushed onto the security context, or some other object that's per JSContext (window, that is)?
Assignee | ||
Comment 11•20 years ago
|
||
jst, simultanous calls from different JSContext are not allowed in liveconnect design, see http://lxr.mozilla.org/seamonkey/source/js/src/liveconnect/jsj_utils.c#473 Before every interaction between java and javascript, jsj_EnterJava must be called. The above line shows that only one call at the same time or some simultanous calls with the same JSCOntext are allowed. Maybe we should change the JS_ASSERT to a |if (..) return;| statement. Also, when nsCSecurityContext::Implies gets called, it does not pass any information about what JSContext is currently using. So I can't retrieve that boolean value from JSContext or some thing equivalent at that time.
Comment 12•20 years ago
|
||
Is nsCSecurityContext a singleton? Could you you not put the boolean value on it? And yeah, we need more than a JS_ASSERT() there, that's a nop in release builds and ensures nothing.
Assignee | ||
Comment 13•20 years ago
|
||
As I described above, simultaneous calls from different JSContext should be always disabled, rather than just disabled in the debug build.
Attachment #145067 -
Attachment is obsolete: true
Assignee | ||
Comment 14•20 years ago
|
||
(In reply to comment #12) > Is nsCSecurityContext a singleton? Could you you not put the boolean value on it? > Oops, I was wrong. nsCSecurityContext does have a JSContext argument in its constrctor. nsCSecurityContext is not a singleton, but it is created by java code, I can't get its instance from liveconnect code.
Attachment #145067 -
Flags: review?(jst)
Assignee | ||
Comment 15•20 years ago
|
||
Comment on attachment 145152 [details] [diff] [review] previous patch + a JS_ASSERT change in jsj_EnterJava moving review request forward.
Attachment #145152 -
Flags: review?(jst)
Comment 16•20 years ago
|
||
Comment on attachment 145152 [details] [diff] [review] previous patch + a JS_ASSERT change in jsj_EnterJava I really don't like this, but I guess we don't have much of a choise here. r=jst
Attachment #145152 -
Flags: review?(jst) → review+
Assignee | ||
Comment 17•20 years ago
|
||
Comment on attachment 145152 [details] [diff] [review] previous patch + a JS_ASSERT change in jsj_EnterJava Thanks, jst. Yes, I don't like this kind of hacking either. Hope we can get rid of them in our new JEA implemetation.
Attachment #145152 -
Flags: superreview?(brendan)
Attachment #145152 -
Flags: approval1.7?
Reporter | ||
Updated•20 years ago
|
Attachment #145053 -
Flags: superreview?(brendan)
Attachment #145053 -
Flags: review?(jst)
Reporter | ||
Updated•20 years ago
|
Attachment #145053 -
Attachment is obsolete: true
Comment 18•20 years ago
|
||
Comment on attachment 145152 [details] [diff] [review] previous patch + a JS_ASSERT change in jsj_EnterJava The Hungarian notation (bFoo for a boolean) is alien, but your choice. However, how about renaming it bJSIsCallingApplet. sr=me anyway. How's the JEA going, is there a document I can read somewhere, or a branch. /be
Attachment #145152 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 19•20 years ago
|
||
(In reply to comment #18) > (From update of attachment 145152 [details] [diff] [review]) > The Hungarian notation (bFoo for a boolean) is alien, but your choice. > However, how about renaming it bJSIsCallingApplet. > > sr=me anyway. How's the JEA going, is there a document I can read somewhere, > or a branch. > > /be > OK, I'll rename |bJSCallApplet| to |JSIsCallingApplet| when checkin. The most coding work of JEA was finished. But we still need more time for testing and bug fix. We target to land the new module in June or maybe July. The document will be available when the feature freezed, I think. CCing more drivers, in case they can't access this bug. We need to check this patch in as soon as possible to get more time frame for other potential security issues.
Comment 20•20 years ago
|
||
Comment on attachment 145152 [details] [diff] [review] previous patch + a JS_ASSERT change in jsj_EnterJava a=chofmann for 1.7
Attachment #145152 -
Flags: approval1.7? → approval1.7+
Assignee | ||
Comment 21•20 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 22•20 years ago
|
||
timeless pointed out comment 19 to me after the checkin was done, asking whether naming a global variable JS* wasn't an invasion of SpiderMonkey's C namespace. It is. I've renamed by adding a jsj_ prefix. /be
Comment 23•20 years ago
|
||
Adding Jon Granrose to CC list to help round up QA resources for verification
Updated•20 years ago
|
Whiteboard: doesnotaffect1.4
Updated•20 years ago
|
Whiteboard: doesnotaffect1.4 → [sg:fix]doesnotaffect1.4
Comment 24•20 years ago
|
||
Removing security-sensitive flag for bugs on the known-vulnerabilities list
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•