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

RESOLVED FIXED in mozilla1.7final

Status

Core Graveyard
Java: Live Connect
P1
blocker
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: Darin Fisher, Assigned: Kyle Yuan)

Tracking

({regression})

Trunk
mozilla1.7final
x86
Linux
regression
Bug Flags:
blocking1.7 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix]doesnotaffect1.4)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
Created attachment 145051 [details]
testcase
Assignee: live-connect → darin
Status: NEW → ASSIGNED
(Reporter)

Comment 2

13 years ago
Created attachment 145052 [details] [diff] [review]
v1 patch

this patch backs out part of the patch for bug 146458.
the comment no longer matches the code with this patch..,.
(Reporter)

Updated

13 years ago
Attachment #145052 - Flags: superreview?(brendan)
Attachment #145052 - Flags: review?(jst)
(Reporter)

Comment 4

13 years ago
Created attachment 145053 [details] [diff] [review]
v1.1 patch

biesi: thanks!
Attachment #145052 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Attachment #145052 - Flags: superreview?(brendan)
Attachment #145052 - Flags: review?(jst)
(Reporter)

Updated

13 years ago
Attachment #145053 - Flags: superreview?(brendan)
Attachment #145053 - Flags: review?(jst)
(Reporter)

Updated

13 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
(Assignee)

Comment 5

13 years ago
Please hold on for a moment. I'd like to dig into this problem a little more and
report what I get asap.
This is a serious security problem, setting blocking1.7+.
Flags: blocking1.7+
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
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.
(Assignee)

Updated

13 years ago
Assignee: darin → kyle.yuan
(Assignee)

Updated

13 years ago
Attachment #145067 - Flags: review?(jst)
(Reporter)

Comment 9

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

13 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.
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

13 years ago
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.
Attachment #145067 - Attachment is obsolete: true
(Assignee)

Comment 14

13 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.
(Assignee)

Updated

13 years ago
Attachment #145067 - Flags: review?(jst)
(Assignee)

Comment 15

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

13 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

13 years ago
Attachment #145053 - Flags: superreview?(brendan)
Attachment #145053 - Flags: review?(jst)
(Reporter)

Updated

13 years ago
Attachment #145053 - Attachment is obsolete: true
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

13 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

13 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

13 years ago
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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
Adding Jon Granrose to CC list to help round up QA resources for verification
Whiteboard: doesnotaffect1.4
Whiteboard: doesnotaffect1.4 → [sg:fix]doesnotaffect1.4
Removing security-sensitive flag for bugs on the known-vulnerabilities list
Group: security

Updated

7 years ago
Component: Java: Live Connect → Java: Live Connect
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.