239122 - Liveconnect can be used to read any file on user's filesystem

Status: RESOLVED FIXED

(Core Graveyard :: Java: Live Connect, defect, P1)

Description

[Darin Fisher]

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]

Attachment: testcase

Comment 2

[Darin Fisher]

Attachment: 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)]

the comment no longer matches the code with this patch..,.

Comment 4

[Darin Fisher]

Attachment: v1.1 patch

biesi: thanks!

Comment 5

[Kyle Yuan]

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)]

This is a serious security problem, setting blocking1.7+.

Comment 7

[Kyle Yuan]

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]

Attachment: 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]

(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)]

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]

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)]

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]

Attachment: 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]

(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]

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)]

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]

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]]

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]

(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]

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]

checked in.

Comment 22

[Brendan Eich [:brendan]]

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]]

Adding Jon Granrose to CC list to help round up QA resources for verification

Comment 24

[Daniel Veditz [:dveditz]]

Removing security-sensitive flag for bugs on the known-vulnerabilities list