Last Comment Bug 239122 - Liveconnect can be used to read any file on user's filesystem
: Liveconnect can be used to read any file on user's filesystem
Status: RESOLVED FIXED
[sg:fix]doesnotaffect1.4
: regression
Product: Core Graveyard
Classification: Graveyard
Component: Java: Live Connect (show other bugs)
: Trunk
: x86 Linux
: P1 blocker (vote)
: mozilla1.7final
Assigned To: Kyle Yuan
: Phil Schwartau
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-03-29 15:10 PST by Darin Fisher
Modified: 2013-12-27 14:30 PST (History)
10 users (show)
jst: blocking1.7+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (569 bytes, text/html)
2004-03-29 15:12 PST, Darin Fisher
no flags Details
v1 patch (1.30 KB, patch)
2004-03-29 15:13 PST, Darin Fisher
no flags Details | Diff | Review
v1.1 patch (1.49 KB, patch)
2004-03-29 15:18 PST, Darin Fisher
no flags Details | Diff | Review
my patch (15.75 KB, patch)
2004-03-29 20:22 PST, Kyle Yuan
no flags Details | Diff | Review
previous patch + a JS_ASSERT change in jsj_EnterJava (16.30 KB, patch)
2004-03-30 22:07 PST, Kyle Yuan
jst: review+
brendan: superreview+
chofmann: approval1.7+
Details | Diff | Review

Description Darin Fisher 2004-03-29 15:10:39 PST
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.
Comment 1 Darin Fisher 2004-03-29 15:12:08 PST
Created attachment 145051 [details]
testcase
Comment 2 Darin Fisher 2004-03-29 15:13:50 PST
Created attachment 145052 [details] [diff] [review]
v1 patch

this patch backs out part of the patch for bug 146458.
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2004-03-29 15:15:57 PST
the comment no longer matches the code with this patch..,.
Comment 4 Darin Fisher 2004-03-29 15:18:38 PST
Created attachment 145053 [details] [diff] [review]
v1.1 patch

biesi: thanks!
Comment 5 Kyle Yuan 2004-03-29 16:40:24 PST
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 Johnny Stenback (:jst, jst@mozilla.com) 2004-03-29 17:17:20 PST
This is a serious security problem, setting blocking1.7+.
Comment 7 Kyle Yuan 2004-03-29 20:19:19 PST
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.
Comment 8 Kyle Yuan 2004-03-29 20:22:58 PST
Created attachment 145067 [details] [diff] [review]
my patch

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.
Comment 9 Darin Fisher 2004-03-30 10:43:02 PST
(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 Johnny Stenback (:jst, jst@mozilla.com) 2004-03-30 14:46:37 PST
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)?
Comment 11 Kyle Yuan 2004-03-30 21:53:44 PST
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 Johnny Stenback (:jst, jst@mozilla.com) 2004-03-30 22:06:25 PST
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.
Comment 13 Kyle Yuan 2004-03-30 22:07:55 PST
Created attachment 145152 [details] [diff] [review]
previous patch + a JS_ASSERT change in jsj_EnterJava

As I described above, simultaneous calls from different JSContext should be
always disabled, rather than just disabled in the debug build.
Comment 14 Kyle Yuan 2004-03-30 22:16:42 PST
(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.
Comment 15 Kyle Yuan 2004-03-30 22:55:55 PST
Comment on attachment 145152 [details] [diff] [review]
previous patch + a JS_ASSERT change in jsj_EnterJava

moving review request forward.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2004-03-31 16:24:11 PST
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
Comment 17 Kyle Yuan 2004-03-31 17:02:16 PST
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.
Comment 18 Brendan Eich [:brendan] 2004-04-02 18:50:27 PST
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
Comment 19 Kyle Yuan 2004-04-05 18:35:06 PDT
(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 chris hofmann 2004-04-05 19:35:33 PDT
Comment on attachment 145152 [details] [diff] [review]
previous patch + a JS_ASSERT change in jsj_EnterJava

a=chofmann for 1.7
Comment 21 Kyle Yuan 2004-04-05 20:16:44 PDT
checked in.
Comment 22 Brendan Eich [:brendan] 2004-04-06 17:03:01 PDT
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 Daniel Veditz [:dveditz] 2004-06-17 13:37:27 PDT
Adding Jon Granrose to CC list to help round up QA resources for verification
Comment 24 Daniel Veditz [:dveditz] 2004-07-22 02:34:20 PDT
Removing security-sensitive flag for bugs on the known-vulnerabilities list

Note You need to log in before you can comment on or make changes to this bug.