The default bug view has changed. See this FAQ.

JavaScript-to-Java (nsISecureEnv) calls sometimes made with no JavaScript on stack

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

1.9.2 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking1.9.2 .13+, status1.9.2 .13-fixed, blocking1.9.1 .16+, status1.9.1 .16-fixed)

Details

(Whiteboard: [sg:low] [qa-needs-STR])

Attachments

(1 attachment)

This is a problem in OJI and browser-side LiveConnect, which is
present on the current live branches (1.9.2 and 1.9.1), but no longer
on the trunk.

First some background:

nsISecureEnv is an interface implemented by OJI plugins to allow calls
to be made from JavaScript to Java (JavaScript-to-Java LiveConnect
calls).  OJI plugins (both Sun's and the JEP before version 0.9.7.4)
assume that nsISecureEnv calls will always be made with JavaScript on
the stack, because they use the current script principal's origin to
configure the Java code's security environment.

(The JEP uses nsIScriptSecurityManager::GetSubjectPrincipal() to get
the current script's principal, then calls nsIPrincipal::GetOrigin()
to get its origin.)

If by some chance there is no current script principal (if there is no
JavaScript on the stack), the JEP before version 0.9.7.4 assumed that
the Java code being called via the nsISecureEnv interface should run
with unlimited privileges.  And I suspect even the most recent
versions of Sun's OJI plugin for Windows and Linux also make this
assumption.

nsISecureEnv methods (used for JavaScript-to-Java calls) are normally
called with JavaScript on the stack.  But there's a bug in the
implementation of the nsILiveconnect API (used for Java-to-JavaScript
calls) that can cause nsISecureEnv methods to inadvertently be called
without any JavaScript on the stack.

That's this bug -- which I'll explain in more detail in my next
comment.

So this bug is potentially a security hole.  But (as I'll also explain
in a later comment) I strongly doubt there's any way to exploit it.
(Assignee)

Comment 1

7 years ago
Created attachment 489033 [details] [diff] [review]
Fix

Java-to-JavaScript calls (made using the nsILiveconnect API) can be
made "spontaneously" -- from Java code that wasn't called from
JavaScript code, and therefore when there is no JavaScript on the
stack.

To get around this, nsCLiveconnect.cpp's implementations of the
nsILiveconnect methods create an AutoPushJSContext object.  This
object's constructor checks for JavaScript on the stack, and if none
is found fakes it (by adding a dummy frame with a principal to the
stack).

The AutoPushJSContext constructor takes a JSContext parameter.
Normally we only learn after a call to jsj_enter_js() which JSContext
we need to use.  But the first time jsj_enter_js() is called, it
initializes a bunch of Java classes, which requires the use of
nsISecureEnv (JavaScript-to-Java) methods.  And if this first call to
jsj_enter_js() happens during an nsILiveconnect (Java-to-JavaScript)
call, there will be no JavaScript on the stack.

My patch gets around this problem by creating the AutoPushJSContext
object before the call to jsj_enter_js().  It uses a new function
(JSContextForPluginInstance()) to find out which JSContext will be
returned by jsj_enter_js(), and passes this to the AutoPushJSContext
constructor.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Is this the security hole exposed by the logging from the site breakage in bug 607678?
(Assignee)

Comment 3

7 years ago
> Is this the security hole exposed by the logging from the site
> breakage in bug 607678?

Yes.

I'm not yet finished writing up this bug.

Tomorrow I'll add more comments, post some builds made with my patch,
and tell people about them at bug 607678.

Updated

7 years ago
Whiteboard: [sg:moderate]
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
(Assignee)

Updated

7 years ago
Attachment #489033 - Attachment filename: bugzilla607678-patch.txt → bugzilla610525-patch.txt
(Assignee)

Comment 4

7 years ago
Here's a test build made with my patch from comment #1.  Since the
tryservers no longer do branch builds, this is a build I made myself.

http://people.mozilla.com/~stmichaud/bmo/firefox-3.6.13pre-bugzilla610525.en-US.mac.dmg
(Assignee)

Comment 5

7 years ago
I've confirmed this bug's security hole also effects the 1.9.1 branch
on Linux (and presumably Windows) -- though only when using Sun's old
OJI plugin.

Interestingly, the proximate cause (on Linux and Windows) is actually
in our OJI code -- the nsCSecurityContext constructors assume that no
script principal (i.e. no JavaScript on the stack) means that Java
code should run with full privileges.  And this assumption may not be
"wrong" -- nsScriptSecurityManager::SubjectPrincipalIsSystem() does
the same thing.
(Assignee)

Comment 6

7 years ago
JEP 0.9.7.4 "fixed" this bug's security hole on OS X in FF versions
since 3.6.11 and 3.5.14 (and in Camino 2.0.5).  (Though it also
triggered the breakage reported in bug 606737 and bug 607678.)

But as I mentioned in comment #0, it's very unlikely that this bug's
security hole is exploitable -- even using OS X distros that bundle
JEP 0.9.7.3 or earlier, or using the OJI plugin in FF 3.5.X on Linux
and Windows.

The only Java code called (via the nsISecureEnv interface) with full
privileges is code that's called from jsj_enter_js().  As best I can
tell, jsj_enter_js() doesn't call any Java code except the first time
it's run (to help initialize Java-to-JavaScript LiveConnect).  This
code does nothing but load commonly used classes and find their fields
and methods (most of it is in jsj.c's init_netscape_java_classes() and
init_java_VM_reflection()) -- which is entirely harmless.  And there
doesn't seem to be any way for "the user" to run Java code of his/her
own choice.
(Assignee)

Comment 7

7 years ago
I think we should reclassify this bug as "[sg:low]".
OK, sg:low it is -- but we still need to fix it because it's the fix for regressions introduced by bug 598453
blocking1.9.1: ? → .16+
blocking1.9.2: ? → .13+
Whiteboard: [sg:moderate] → [sg:low]
Blocks: 606737
Blocks: 607678
(Assignee)

Comment 9

7 years ago
> but we still need to fix it because it's the fix for
> regressions introduced by bug 598453

Yes.
Comment on attachment 489033 [details] [diff] [review]
Fix

I don't really know who should review my patch -- OJI and
(browser-side) LiveConnect are (infamously) obsolete and un-owned.

Dbaron:  I noticed your name on some of the revisions to the
AutoPushJSContext code, so I'm asking you to review.  Please let me
know if you think someone else should do it.
Attachment #489033 - Flags: review?(dbaron)
Attachment #489033 - Flags: review?(dbaron) → review?(jst)
Comment on attachment 489033 [details] [diff] [review]
Fix

JSContextForPluginInstance() could use nsCOMPtr's instead of doing manual reference counting, but if that's for some reason hard I'm fine with doing it by hand here, as the patch does.

r=jst
Attachment #489033 - Flags: review?(jst) → review+
Comment on attachment 489033 [details] [diff] [review]
Fix

Landed on the 1.9.2 branch with jst's requested changes:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f1ae505ad72d
(Assignee)

Updated

6 years ago
status1.9.2: wanted → .13-fixed
Comment on attachment 489033 [details] [diff] [review]
Fix

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff0b80c67371
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status1.9.1: wanted → .16-fixed
Resolution: --- → FIXED
Smokey, does this need to land on the 1.9.0 branch?

I assume that it does, but please let us know (so I can request approval).
Comment on attachment 489033 [details] [diff] [review]
Fix

> Smokey, does this need to land on the 1.9.0 branch?

I've answered my own question -- this patch does need to land on the 1.9.0 branch, since current Camino releases are off that branch, and Camino 2.0.5 is effected by this bug (and bug 606737 and bug 607678).
Attachment #489033 - Flags: approval1.9.0.next?
Yes, thanks.
What are the STR for this bug for verification of this fix on 1.9.2 and 1.9.1?
Whiteboard: [sg:low] → [sg:low] [qa-needs-STR]
> What are the STR for this bug for verification of this fix on 1.9.2
> and 1.9.1?

The following has worked best for me:

1) Run Applications : Utilities : Java Preferences and

   a) Click on the Advanced tab.
   b) Under Java Console select Show console.

2) Load the testcase from bug 606737
   (http://jnlp.dev.concord.org/test-japplet.html).

3) Look for the following in the Java Console output:

   WINDOW IS: [object DOMWindow]
Group: core-security
You need to log in before you can comment on or make changes to this bug.