Generated classes have too low privileges

RESOLVED FIXED in 1.7R1

Status

Rhino
Core
P5
normal
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Attila Szegedi)

Tracking

Details

Attachments

(3 attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20021003
Build Identifier: Rhino CVS from 2003-01-27

Currently any generated class will have lowest possible privileges which does
not allow to use the optimizer or scripting of Java classes with SecurityManager
installed even if the Rhino classes are allowed to carry corresponding task.


Reproducible: Always

Steps to Reproduce:
Run Rhino shell with security manager installed and a polycy file that gives
rhino classes all permissions, like in:

java -Djava.security.manager \
  -Djava.security.policy=file:/home/some-path/java.policy \
  -Drhino.classes=file:/home/some-path/js.jar \
  -jar /home/some-path/js.jar \
  sectest.js

where file:/home/some-path/java.policy is a modified java.policy from a JDK
distribution with the following lines added:

grant codeBase "${rhino.classes}" {
	permission java.security.AllPermission;
};

and sectest.js contains the single line:

print(java.lang.System.getProperty("user.home"));

Note that it is essential to use a separated file for the test as in the
interactive mode the shell uses pure interpreter which works.
Actual Results:  
js: uncaught JavaScript exception: java.security.AccessControlException: access
denied (java.util.PropertyPermission user.home read)


Expected Results:  
A path to user home directory, like /home/igor
(Reporter)

Comment 1

14 years ago
Created attachment 112789 [details] [diff] [review]
Define generated classes with the same privileges as rhino classes themselves

The patch effectively makes omj/DefiningClassLoader to call
ClassLoader.defineClass with ProtectionDomain argument containg necessary
privileges.
(Reporter)

Comment 2

14 years ago
The above patch is a post 1.5R4 stuff and I added it now just in case if someone
will have that problem when scripting java classes via JavaAdapter. There is a
potential issue with the patch that may arise if Sun finally fix a nasty
security problem that effectively allows to gain any permission as long as the
application can create class loaders.

Another problem is that it uses to match reflection to stay within 1.1
compatibility. It may be better to have something like omj.jdk12 package for
code like that.

Comment 3

12 years ago
Created attachment 193149 [details]
A jar file which contains two Java source files that use Proxy to do JavaAdapter's job

A problem with patches above is that it still depends on custom ClassLoader. 
At least in Java 1.4 such approach will generate permission exception whenever
a custom ClassLoader is created.  As the result, codes that requires
JavaAdapter to function will run into problems.

Rhino's default behavior cannot be run in Java Web Start Sandbox due to two
reasons.

1. Optimization flag is on by default, which would produce byte code.
2. JavaAdapter requires some byte compilation of the code.

As the result, both requires custom ClassLoader to load new classes from byte
code.

In JDK 1.3, java.lang.reflect.Proxy allows dynamic creation of Java classes
without using ClassLoader.  If we also turn off Context optimization flag,
custom ClassLoader won't be created and permission exception can be avoided.

My software CookJS (http://cookxml.sourceforge.net/cookjs/) has the
ProxyJavaAdapter class implemented.  The attached jar file contains the two
java files cookxml.cookjs.util.ProxyHandler and
cookxml.cookjs.util.ProxyJavaAdapter that can solve the problem.  It is not as
feature rich as JavaAdapter and mostly depend on JavaAdapter to do all the hard
work.

The following code shows how it works:

Scriptable scope = cx.initStandardObjects ();
cx.setOptimizationLevel (-1);
ProxyJavaAdapter.init (cx, scope, true);
Object result = cx.evaluateString (scope, code, src, 1, null);

Basically it overrides the default JavaAdapter object in the scope.  Nothing in
Rhino 1.6R1 needs any changes.

Comment 4

11 years ago
I found Heng's workaround very helpful, and pointed out how to grab just those bits from CookXML, here:

http://kylecordes.com/2006/03/24/rhino-web-start/

This is not helpful for fixing Rhino, but it might be helpful for the next person who lands here via a web search (which I how I landed here).

Comment 5

11 years ago
Also if you're looking to disable Rhino's compilation mode, you can remove all the classes in the org.mozilla.classfile package and Rhino is smart enough just to start up in interpreter mode. 
(Assignee)

Updated

11 years ago
Assignee: nboyd → szegedia
(Assignee)

Comment 6

11 years ago
This is a real problem - I want to preserve JDK 1.1 compatibility for 1.6R3, but once it is out, we'll move over to at least 1.2 compatibility, and we'll be able to solve it by using a JDK 1.2-introduced defineClass() method that takes a ProtectionDomain argument
Status: NEW → ASSIGNED

Comment 7

10 years ago
Changing priority to P5 based on recent bug triage.
Priority: -- → P5

Comment 8

10 years ago
I encountered a similar bug in Rhino 1.6R7. 

I was using JavaAdapter in my script. According to the documentation of JavaAdapter, if I call

    new JavaAdapter(java-class, javascript-object)

Rhino generates a class on the fly which delegates all calls to the methods of the javascript-object. However, the generated class does not have any security privileges so I got AccessControlException when SecurityManager was installed.

After some debugging, I found the problem exists in 'loadAdapterClass' method in JavaAdapter.

       static Class loadAdapterClass(String className, byte[] classBytes)
       {
           GeneratedClassLoader loader
               = SecurityController.createLoader(null, null);
           Class result = loader.defineClass(className, classBytes);
           loader.linkClass(result);
           return result;
       }

Note the SecurityController.createLoader call has two NULL parameters. The second parameter should be a security domain, and the method creates a class loader with restrictions imposed by the security domain. Since the parameter is null in Rhino 1.6R7 implementation, the generated adapter class is loaded with no security domain, and there will be AccessControlException when Java Security Manager checks security permissions on this class.

To fix this bug, I think we need pass in proper security domain parameters in the SecurityController.createLoader call. The security domain should be the same one associated with the javascript-object (which is being adaptered).

Actually, we may follow the example in org.mozilla.javascript.optimizer.CodeGen class. The CodeGen class generates 'javascript-object' which can be used with JavaAdapter and the generated byte codes are loaded with proper security domain information. The 'defineClass' method in the class make use of security domain information properly:

    private Class defineClass(Object bytecode,
                              Object staticSecurityDomain)
    {
        Object[] nameBytesPair = (Object[])bytecode;
        String className = (String)nameBytesPair[0];
        byte[] classBytes = (byte[])nameBytesPair[1];

        // The generated classes in this case refer only to Rhino classes
        // which must be accessible through this class loader
        ClassLoader rhinoLoader = getClass().getClassLoader();
        GeneratedClassLoader loader;
        loader = SecurityController.createLoader(rhinoLoader,
                                                 staticSecurityDomain);
        Exception e;
        try {
            Class cl = loader.defineClass(className, classBytes);
            loader.linkClass(cl);
            return cl;
        } catch (SecurityException x) {
            e = x;
        } catch (IllegalArgumentException x) {
            e = x;
        }
        throw new RuntimeException("Malformed optimizer package " + e);
    }

If JavaAdapter can obtain the security domain associated with the 'javascript-object' class, then this bug can be fixed.
  

Comment 9

9 years ago
Created attachment 302760 [details]
Proposed bug fix on org.mozilla.javascript.JavaAdapter

This JavaAdapter.java is modified based on release 1.6R7.

The changes are in method loadAdapterClass() and getAdapterClass(), which now loads the adapter class using proper classloader and protection domain.

Comment 10

9 years ago
Attila, please have a look my proposal. I'm willing to make this issue closed in 1.7 release.
(Assignee)

Comment 11

9 years ago
Well, I've looked into it, and unfortunately I don't think it is valid. The problem is that you'll mostly end up running with the CodeSource of js.jar, as the "obj" will more often than not be a NativeObject. 

Now, on the other hand, since the generated code is actually generated by Rhino, I believe we should use the CodeSource of JavaAdapter itself (as the meat of the generated methods delegate anyway to JavaAdapter static methods). When individual functions are invoked, they'll get their CodeSource applied to the current access context anyway, so all should be well. 

Therefore, I think the correct solution is to replace

SecurityController.createLoader(null, null)

with something similar to:

SecureClassLoader cl = (SecureClassLoader)JavaAdapter.class.getClassLoader();
SecurityController.createLoader(cl, cl.getProtectionDomain().getCodeSource());

of course, with safeguard provisions for cases when "cl" is not a SecureClassLoader, or when the SecurityController in effect does not, in fact, understand CodeSource (such is the price of an overgeneric SecurityController interface...).

If you agree, I can implement this fairly quickly.
(Assignee)

Comment 12

9 years ago
Ok, the logic I came up with is in the end:

        Object staticDomain;
        Class domainClass = SecurityController.getStaticSecurityDomainClass();
        if(domainClass == CodeSource.class || domainClass == ProtectionDomain.class) {
            ProtectionDomain protectionDomain = JavaAdapter.class.getProtectionDomain();
            if(domainClass == CodeSource.class) {
                staticDomain = protectionDomain == null ? null : protectionDomain.getCodeSource();
            }
            else {
                staticDomain = protectionDomain;
            }
        }
        else {
            staticDomain = null;
        }
        GeneratedClassLoader loader = SecurityController.createLoader(null, 
                staticDomain);

I added a "SecurityController.getStaticSecurityDomainClass()" method so that the security controller can declare the kind of the security domain object it expects -- our PolicySecurityController accepts CodeSource while our JavaPolicySecurity (found in shell) uses a ProtectionDomain, although I believe that's flawed, but don't want to touch it for now, so I'll rather make the JavaAdapter code more flexible.

Do you agree with this solution?

Comment 13

9 years ago
Attila, thanks for looking into this issue!

Actually, our ideas are not very different.

There are two decisions we need to make here about the two parameters passing to SecurityController.createLoader():

1/ Which class loader should we pass to SecurityController? 

I think "JavaAdapter.class.getClassLoader()" should be fine. This means we use the class loader that loads Rhino. (You leave this parameter as null in your last post, is it a typo?)

2/ Which ProtectionDomain/CodeSource to use?

I think "JavaAdapter.class.getProtectionDomain()" should be fine. Since JavaAdapter is loaded from js.jar, in effect we assign js.jar's security privileges to the generated adapter class. 

In your post, you were saying "The problem is that you'll mostly end up running with the CodeSource of js.jar.." I still don't see the problem of using CodeSource of js.jar. Your solution also has the same effect, isn't it?

In my proposed fix, "obj.getClass().getProtectionDomain()" will get the domain for NativeObject class in most of the cases; when the 'obj' is not a NativeObject, I don't know what will happen. So "JavaAdapter.class.getProtectionDomain()" is more clear.
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> 
> 1/ Which class loader should we pass to SecurityController? 
> 
> I think "JavaAdapter.class.getClassLoader()" should be fine. This means we use
> the class loader that loads Rhino. (You leave this parameter as null in your
> last post, is it a typo?)

Actually, no. null will cause it to use Context.getApplicationClassLoader(), which I think is a better choice. In reality, it shouldn't really matter much, as it's really just used as the parent for a newly created class loader that will be used to load this single adapter only.

> 
> 2/ Which ProtectionDomain/CodeSource to use?
> 
> I think "JavaAdapter.class.getProtectionDomain()" should be fine. Since
> JavaAdapter is loaded from js.jar, in effect we assign js.jar's security
> privileges to the generated adapter class. 
> 
> In your post, you were saying "The problem is that you'll mostly end up running
> with the CodeSource of js.jar.." I still don't see the problem of using
> CodeSource of js.jar. Your solution also has the same effect, isn't it?

Yes. I was writing a reply to you as I was thinking about the issue, and didn't properly edit it post fact :-). I was going "hm... this will usually use js.jar code source... hey, wait a minute, shouldn't it actually always use js.jar code source? Let's see if the generated code will ensure that further protection domain acrobatics are performed for individual function invocations... check, check... yes they are... okay, then we should really always use js.jar code source, as the generated code is basically constant modulo function names".

So yes, we should always use the code source (or less preferably, protection domain) of js.jar for JavaAdapter generated code.

> In my proposed fix, "obj.getClass().getProtectionDomain()" will get the domain
> for NativeObject class in most of the cases; when the 'obj' is not a
> NativeObject, I don't know what will happen. So
> "JavaAdapter.class.getProtectionDomain()" is more clear.
> 

(Assignee)

Comment 15

9 years ago
Committed the fix:

    Checking in src/org/mozilla/javascript/JavaAdapter.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/JavaAdapter.java,v  <--  JavaAdapter.java
    new revision: 1.111; previous revision: 1.110

(Assignee)

Updated

9 years ago
Component: Compiler → Core
OS: Linux → All
Hardware: PC → All
(Assignee)

Updated

9 years ago
Target Milestone: --- → 1.7R1
(Assignee)

Comment 16

9 years ago
Also:

    Checking in toolsrc/org/mozilla/javascript/tools/shell/JavaPolicySecurity.java;
    /cvsroot/mozilla/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/JavaPolicySecurity.java,v  <--  JavaPolicySecurity.java
    new revision: 1.11; previous revision: 1.10
    done
    Checking in src/org/mozilla/javascript/SecurityController.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/SecurityController.java,v  <--  SecurityController.java
    new revision: 1.13; previous revision: 1.12
    done
    Checking in src/org/mozilla/javascript/PolicySecurityController.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/PolicySecurityController.java,v  <--  PolicySecurityController.java
    new revision: 1.8; previous revision: 1.7
    done

Comment 17

9 years ago
I have verified the fix in my application. It solves my problem.

Attila, thanks for your hard work! 

Comment 18

9 years ago
Marking fixed since the changes are checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.