|
Would it make any sense to, rather than having a class with static methods, having an interface/abstract class and a static INSTANCE? Then some initialization (static) code could decide which implementation is appropriate for the environment: e.g. if no security manager is installed we wouldn't need the doPrivileged code? I don't know if going through the doPrivileged code when it isn't doing anything incurs enough expense to make this worthwhile.
If you choose not to have an interface and two classes selected by whether the security manager is active or not, I'd suggest adding a
static final boolean isSecure = System.getSecurityManager() != null; and then testing it for each method, e.g. public static final ClassLoader getClassLoader (final Class clazz) { if(isSecure) { return (ClassLoader) AccessController .doPrivileged(new PrivilegedAction() { public Object run () { return clazz.getClassLoader(); } }); } else { return clazz.getClassLoader(); } } If the variable isSecure is static final we need to be sure that the class is never loaded before the security manager is activated, otherwise we will get the wrong answer. As the patch is written as of June 3, each call will at minimum create a new object on the heap that needs to be garbage collected, and calling doPriv when there is no security manager is a waste. In any case, it's good to isolate the calls to this class so I'm +1 on making the rest of the changes and we can worry about the implementation details when we see if there is any performance impact one way or the other. Including a class like J2DoPrivHelper as part of trusted code might be dangerous from security point of view. For example. some malicious code that otherwise does not enough privileges can now call J2DoPrivHelper.getDeclaredFields() to get access to fields of a class. AFAIK, there is no easy way to completely factor out doPrivileged blocks without compromising secuity :(
Mitesh, You bring up a very good point.
I need to investigate more and see if there are alternatives other than in-lining doPrivileged in the openjpa code base. Thanks for you insight into this matter. Albert Lee What is possible is to hide the nasty mechanical construction of a new instance of the anonymous inner class by a wrapper method.
Perhaps Mitesh can post examples (used in CDDL-licensed TopLink Essentials) that preserve the doPrivileged method call in the correct place but delegates the construction of the instance to a wrapper. The resulting code is much more readable than the usual inline doPrivileged and avoids the security hole. I noticed that there are many openjpa classes have a static final caching the line.separator (or something similiar) from the system properties. This kind of general resource that required doPriv can be customized in the helper without any security exposure and still make the code readable.
private static final String SEP = J2DoPrivHelper.getLineSeparator(); public static final String J2DoPrivHelper.getLineSeparator() { if (System.getSecurityManager() != null) { return (Properties) AccessController .doPrivileged(new PrivilegedAction() { public Object run () { return System.getProperty("line.separator"); } }); } else { return System.getProperty("line.separator"); } } These type of functions can also be cached in the helper for performance too. Albert Lee. What the example code does is to allow any untrusted code with access to the J2DoPrivHelper class to get the value for the line separator.
While this doesn't sound like a big deal, it's still violating the security model. A solution is to code this in the trusted code: private static final String SEP = (Properties) AccessController.doPrivileged( J2DoPrivHelper.getLineSeparatorAction()); And then the helper class is responsible for: public static PrivilegedAction getLineSeparatorAction() { return new PrivilegedAction() { public Object run () { return System.getProperty("line.separator"); } }); } >What is possible is to hide the nasty mechanical construction of a new instance of the anonymous inner class by a wrapper method.
Here is an example for invoking a method reflectively try { AccessController.doPrivileged(new PrivilegedMethodInvoker(method, null, args)); } catch (PrivilegedActionException exception) { Exception throwableException = exception.getException(); } where PrivilegedMethodInvoker is as follows public class PrivilegedMethodInvoker implements PrivilegedExceptionAction { private Method method; private Object target; private Object[] args; public PrivilegedMethodInvoker(Method method, Object target, Object[] args){ this.method = method; this.target = target; this.args = args; } public Object run() throws IllegalAccessException, InvocationTargetException { return PrivilegedAccessHelper.invokeMethod(method, target, args); } } Having specific function(s) in J2DoPrivHelp that required doPriv is to isolate common access function without security exposure and hopefully improve performance. Since there is no user parameters allowed by the helpers, even the getLineSeparator() is public, user is only allowed to get the lineSeparator string and no other resources security leak.
The wrapper method approach is to provide means to create the Privilege(Exception)Action object but the doPrivileged will still be in-lined in the openjpa code to avoid undesirable security exposure. Here is a new proposal: 1) The J2DoPrivHelper defines the common security safe functions and PrivilegedAction getters (example): public static final String getLineSeparator(); public static final Object getOtherSpecificSecuritySafeResource(); ....... public static final PrivilegedAction getClassLoaderAction(final ClassLoader loader); public static final PrivilegedExceptionAction getDeclaredMethodAction(final Class clazz, final String name, final Class[] parameterTypes); ....... 2) J2DoPrivHelper usage: private static final String SEP = J2DoPrivHelper.getLineSeparator(); ClassLoader loader = (ClassLoader) AccessController.doPrivileged( J2DoPrivHelper.getClassLoaderAction( clazz ) ); try { AccessController.doPrivileged(J2DoPrivHelper.getDeclaredMethodAction( clazz, name, args)); } catch (PrivilegedActionException exception) { NoSuchMethodException ex = (NoSuchMethodException)exception.getException(); } 3) If there is any situation where testing security is enabled before the doPriv pattern is used, it will need to be in-lined in user code. Please comment and indicate if this is an acceptable solution. Thanks. Albert Lee. I think that an important design goal here is minimal invasiveness into the code. Java 2 security is something that many of us have never seen as an issue in practice, so ensuring that the security-friendly mechanisms are just as easy to use as the unfriendly versions is pretty important IMO.
Additionally, I'm concerned about the extra overhead incurred by these calls, which makes me think that caching might be a good idea. Given that you demonstrate in point 2 above that it is legit to cache the return values of the security-wrapping calls, can we achieve better encapsulation? For example, why not just have a J2DoPrivHelper.getDeclaredMethod() call that does the right thing internally? Additionally, from a performance standpoint, it seems like we should make the J2DoPrivHelper methods non-static, create an interface, and provide access to the instance via the OpenJPAConfiguration object. This will allow us to have an impl that doesn't do security checks at all and a separate impl that does the security checks.
>> I think that an important design goal here is minimal invasiveness into the code. Java 2 security is something that many of us have never seen as an issue in practice, so ensuring that the security-friendly mechanisms are just as easy to use as the unfriendly versions is pretty important IMO.
I agree with you. This is one of the goal set right from the beginning. See the first comment . >> Additionally, I'm concerned about the extra overhead incurred by these calls, which makes me think that caching might be a good idea. We have also considered and discussed the performance aspects. There will be 2 extra calls, i.e. AccessController and PrivelegedAction.run if security is enabled. This can not be avoid. However the JIT may optimize/in-line the call as the runtime is stablized. Architectually, I agree making the helper methods non-static to encapsulate the doPriv wrapper function for the security disable scenario is desirable. However this will sill incur a minimum of 1 and 3 method calls for security disable and enable scernario respectively. e.g. helper.getDeclaredMethod() and helper.getDeclaredMethod() -> AccessController -> run(). >> Given that you demonstrate in point 2 above that it is legit to cache the return values of the security-wrapping calls, can we achieve better encapsulation? For example, why not just have a J2DoPrivHelper.getDeclaredMethod() call that does the right thing internally? If I hear you right, the sugguestion is to have all the doPriv processing in the J2DoPrivHelper.getDeclaredMethod(). This was the original proposal and Mitesh has pointed out that this is a security leak. See previous comment. >> Additionally, from a performance standpoint, it seems like we should make the J2DoPrivHelper methods non-static, create an interface, and provide access to the instance via the OpenJPAConfiguration object. This will allow us to have an impl that doesn't do security checks at all and a separate impl that does the security checks. Sounds good. I'll draft another proposal and post it back here soon. Thanks. Albert Lee > > Given that you demonstrate in point 2 above that it is legit to cache the
> > return values of the security-wrapping calls, can we achieve better > > encapsulation? For example, why not just have a J2DoPrivHelper.getDeclaredMethod() > > call that does the right thing internally? > > If I hear you right, the sugguestion is to have all the doPriv processing in the > J2DoPrivHelper.getDeclaredMethod(). This was the original proposal and Mitesh > has pointed out that this is a security leak. See previous comment. ... but it looks like your most recent proposal shares this leak, since it has methods like getLineSeparator(). We should either make all of the methods secure and consider it a goal to not allow that leak, or make all of the helper methods behave the same, no? -Patrick The getLineSeparator() will not compromise security because this function only return specifically the line.separator String and nothing else, hence there is no side effect. This method may cache the value for performance and better code readability and maintenance.
In the original proposal, application can call one of the J2DoPrivHelper public methods which may grant user resource privileges on behalf of the caller, which is not good. If there is a resource requires privilege, the AccessController.doPrivileged() must be in-line in the openjpa code base, but the "new Privilege(Exception)Action" can be common or factor out. Even if the doPrivilege() is inlined, one must also be careful not to allow any public method to be accessed by the application with user specified resource that eventually grant privilege to access the resource. >>> Additionally, from a performance standpoint, it seems like we should make the J2DoPrivHelper methods non-static, create an interface, and provide access to the instance via the OpenJPAConfiguration object. This will allow us to have an impl that doesn't do security checks at all and a separate impl that does the security checks. >Sounds good. After some thought on this topics, I just realized that use of interface will not work. The same reason as just described. The interface can only get the PrivilegedAction because the doPriv must be in-lined. E.g. The use cases are: private static final String SEP = J2DoPrivHelper.getLineSeparator(); ClassLoader loader = (ClassLoader) (System.getSecurityManager() == null) ? clazz.getClassLoader() : AccessController.doPrivileged( J2DoPrivHelper.getClassLoaderAction( clazz ) ); try { method = ( System.getSecurityManager() == null ) ? clazz.getDeclaredMethod(name,parameterType) : (Method) AccessController.doPrivileged( J2DoPrivHelper.getDeclaredMethodAction( clazz, name, parameterType) ); } catch( PrivilegedActionException pae ) { throws (NoSuchMethodException)pae.getException() } Based on the previous design/implementation discussions. attached is a fix to enable Java 2 security in openjpa implementation.
Tested platforms ---------------- Basic tests runs on Win32 system and have verified on WebSphere Application Server with security both disabled/enabled. Test profile option ------------------- A new profile 'enabled-security' is added to openjpa pom.xml to enable running tests with security on. The default is seourity off. E.g. To run test with security on, mvn test -P enabled-security,test-derby TCK --- Tests passed and no regression. Performance ----------- Overall no difference when security is off but showed a 8.4% degradation with security on. 1) Results: base revision run before any changes were made. > mvn test [INFO] ------------------------------------------------------------------------ [INFO] Reactor Summary: [INFO] ------------------------------------------------------------------------ [INFO] OpenJPA ............................................... SUCCESS [0.016s] [INFO] OpenJPA Utilities ..................................... SUCCESS [6.718s] [INFO] OpenJPA Kernel ........................................ SUCCESS [3.532s] [INFO] OpenJPA JDBC .......................................... SUCCESS [0.328s] [INFO] OpenJPA XML Store ..................................... SUCCESS [0.047s] [INFO] OpenJPA Kernel 1.5 .................................... SUCCESS [0.078s] [INFO] OpenJPA JPA ........................................... SUCCESS [0.078s] [INFO] OpenJPA JDBC 1.5 ...................................... SUCCESS [0.031s] [INFO] OpenJPA JPA JDBC ...................................... SUCCESS [1:10.547s] [INFO] OpenJPA Aggregate Jar ................................. SUCCESS [1.547s] [INFO] OpenJPA Distribution .................................. SUCCESS [0.141s] [INFO] OpenJPA Integration Tests ............................. SUCCESS [0.000s] [INFO] OpenJPA Examples Integration Tests .................... SUCCESS [0.000s] [INFO] OpenJPA JPA TCK Integration Tests ..................... SUCCESS [0.000s] [INFO] OpenJPA Xmlmapping 1.5 ................................ SUCCESS [0.015s] [INFO] OpenJPA Persistence Examples .......................... SUCCESS [0.031s] [INFO] ------------------------------------------------------------------------ [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESSFUL [INFO] ------------------------------------------------------------------------ [INFO] Total time: 1 minute 23 seconds [INFO] Finished at: Fri Jun 29 10:18:45 CDT 2007 [INFO] Final Memory: 10M/28M [INFO] ------------------------------------------------------------------------ 2) Results: base revision plus patch applied, run with default security disabled. > mvn test [INFO] ------------------------------------------------------------------------ [INFO] Reactor Summary: [INFO] ------------------------------------------------------------------------ [INFO] OpenJPA ............................................... SUCCESS [0.000s] [INFO] OpenJPA Utilities ..................................... SUCCESS [7.718s] [INFO] OpenJPA Kernel ........................................ SUCCESS [3.625s] [INFO] OpenJPA JDBC .......................................... SUCCESS [0.407s] [INFO] OpenJPA XML Store ..................................... SUCCESS [0.031s] [INFO] OpenJPA Kernel 1.5 .................................... SUCCESS [0.094s] [INFO] OpenJPA JPA ........................................... SUCCESS [0.093s] [INFO] OpenJPA JDBC 1.5 ...................................... SUCCESS [0.032s] [INFO] OpenJPA JPA JDBC ...................................... SUCCESS [1:09.453s] [INFO] OpenJPA Aggregate Jar ................................. SUCCESS [1.156s] [INFO] OpenJPA Distribution .................................. SUCCESS [0.125s] [INFO] OpenJPA Integration Tests ............................. SUCCESS [0.000s] [INFO] OpenJPA Examples Integration Tests .................... SUCCESS [0.000s] [INFO] OpenJPA JPA TCK Integration Tests ..................... SUCCESS [0.000s] [INFO] OpenJPA Xmlmapping 1.5 ................................ SUCCESS [0.031s] [INFO] OpenJPA Persistence Examples .......................... SUCCESS [0.032s] [INFO] ------------------------------------------------------------------------ [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESSFUL [INFO] ------------------------------------------------------------------------ [INFO] Total time: 1 minute 23 seconds [INFO] Finished at: Fri Jun 29 09:30:18 CDT 2007 [INFO] Final Memory: 10M/23M [INFO] ------------------------------------------------------------------------ 3) Results: base revision plus patch applied, run with security enabled. > mvn test -P enable-security,test-derby [INFO] ------------------------------------------------------------------------ [INFO] Reactor Summary: [INFO] ------------------------------------------------------------------------ [INFO] OpenJPA ............................................... SUCCESS [0.016s] [INFO] OpenJPA Utilities ..................................... SUCCESS [7.969s] [INFO] OpenJPA Kernel ........................................ SUCCESS [3.234s] [INFO] OpenJPA JDBC .......................................... SUCCESS [0.438s] [INFO] OpenJPA XML Store ..................................... SUCCESS [0.093s] [INFO] OpenJPA Kernel 1.5 .................................... SUCCESS [0.110s] [INFO] OpenJPA JPA ........................................... SUCCESS [0.078s] [INFO] OpenJPA JDBC 1.5 ...................................... SUCCESS [0.047s] [INFO] OpenJPA JPA JDBC ...................................... SUCCESS [1:16.312s] [INFO] OpenJPA Aggregate Jar ................................. SUCCESS [2.094s] [INFO] OpenJPA Distribution .................................. SUCCESS [0.125s] [INFO] OpenJPA Integration Tests ............................. SUCCESS [0.000s] [INFO] OpenJPA Examples Integration Tests .................... SUCCESS [0.000s] [INFO] OpenJPA JPA TCK Integration Tests ..................... SUCCESS [0.000s] [INFO] OpenJPA Xmlmapping 1.5 ................................ SUCCESS [0.016s] [INFO] OpenJPA Persistence Examples .......................... SUCCESS [0.046s] [INFO] ------------------------------------------------------------------------ [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESSFUL [INFO] ------------------------------------------------------------------------ [INFO] Total time: 1 minute 30 seconds [INFO] Finished at: Fri Jun 29 09:39:55 CDT 2007 [INFO] Final Memory: 10M/23M [INFO] ------------------------------------------------------------------------ Albert Lee. After quick review, the patch looks good to me. Anyone else have any opinions an committing it?
I'm reviewing it right now. Will be done in an hour.
Very nice piece of work, Albert.
The original patch should be removed since it's no longer valid. Just a few comments on the patch itself. 1. Typos in javadoc for almost all the methods in J2DoPrivHelper, e.g. PrivilegeExceptionAction should be PrivilegedExceptionAction 2. The cases where you call this.getClass().getClassLoader()) don't need to be wrapped in a doPrivileged block. (From the javadoc of getClassLoader, If a security manager is present, and the caller's class loader is not null and the caller's class loader is not the same as or an ancestor of the class loader for the class whose class loader is requested, then this method calls the security manager's checkPermission method with a RuntimePermission("getClassLoader") permission to ensure it's ok to access the class loader for the class.) 3. In openjpa-lib/src/main/java/org/apache/openjpa/lib/util/MultiClassLoader.java you might have missed this case: @@ -238,12 +246,18 @@ if (loader == THREAD_LOADER) loader = Thread.currentThread().getContextClassLoader(); 4. Several cases of try or catch with the { on the following line instead of on the same line. 5. In openjpa-xmlstore/src/main/java/org/apache/openjpa/xmlstore/XMLFileHandler.java don't you need to have a doPrivileged around f.length() ? The javadoc would suggest so. 6. javadoc typos in newFIleOutputStreamAction methods in J2Helper class 7. I don't understand the rationale for newInstanceOfAction. I guess I don't know what a BCClass is and why its behavior is different from Class. Craig, Thanks for the detailed review.
>> 1. Typos in javadoc for almost all the methods in J2DoPrivHelper, e.g. PrivilegeExceptionAction should be PrivilegedExceptionAction Done. >> 2. The cases where you call this.getClass().getClassLoader()) don't need to be wrapped in a doPrivileged block. >> >> (From the javadoc of getClassLoader, If a security manager is present, and the caller's class loader is not null and the caller's class loader is not the same as or an ancestor of the class loader for the class whose class loader is requested, then this method calls the security manager's checkPermission method with a RuntimePermission("getClassLoader") permission to ensure it's ok to access the class loader for the class.) Done. There are 14 instances that can take advantage of these performance short-cut. >> 3. In openjpa-lib/src/main/java/org/apache/openjpa/lib/util/MultiClassLoader.java you might have missed this case: >> @@ -238,12 +246,18 @@ if (loader == THREAD_LOADER) loader = Thread.currentThread().getContextClassLoader(); Good eye.. Done. >> 4. Several cases of try or catch with the { on the following line instead of on the same line. All fixed up >> 5. In openjpa-xmlstore/src/main/java/org/apache/openjpa/xmlstore/XMLFileHandler.java >> >> don't you need to have a doPrivileged around f.length() ? The javadoc would suggest so. You are correct. Somehow I don't get any security exception even without the doPriv wrapping. I have added a new lengthAction and "do the right thing" now. >> 6. javadoc typos in newFIleOutputStreamAction methods in J2Helper class Done. >> 7. I don't understand the rationale for newInstanceOfAction. I guess I don't know what a BCClass is and why its behavior is different from Class. The BCClass is from serp and it needs getClassLoader permission to perform some privileged operations. Since we cannot instrument the doPriv wrapping in Serp, we delegated and moved up the doPriv in OpenJPA code base, hence the need for the 2 newCodeAction and isInstanceOfAction. Thank everyone's input to enable the security feature in openjpa. If there is no further comments, we'll commit this changes. This completed and concluded Jira-244. Albert Lee. >> 3. In openjpa-lib/src/main/java/org/apache/openjpa/lib/util/MultiClassLoader.java you might have missed this case:
>> @@ -238,12 +246,18 @@ >> if (loader == THREAD_LOADER) >> loader = Thread.currentThread().getContextClassLoader(); >Good eye.. Done. >> 5. In openjpa-xmlstore/src/main/java/org/apache/openjpa/xmlstore/XMLFileHandler.java don't you need to have a doPrivileged around f.length() ? The javadoc would suggest so. > You are correct. Somehow I don't get any security exception even without the doPriv wrapping. I have added a new lengthAction and "do the right thing" now. My only concern now is that without the above changes, your tests ran correctly. Do you have a test bench where the caller is not privileged and the OpenJPA is privileged? One suspicious change is in the security permissions file: +// ================================================================ +// The following permissions are needed to invoke the 'test' target in OpenJPA maven build. +grant { + permission java.security.AllPermission; +}; + This would appear to grant everyone AllPermissions, which might explain why the tests all work. Can this be restricted to granting permission to just the test framework (javax.junit) and see what happens? Craig,
I wrote a small program to test the validity of the required permission for File.length() and it showed below that proper permission/doPriv is required for this method and it matches the javadoc description. ------------------------------------------ package test; import java.io.File; public class TestFileLength { public static void main(String[] args) { File f = new File("C:\\a.workspace\\eclipse.workspace\\ejb3.serv1\\testSer\\src\\test\\TestFileLength.java"); System.out.println(f.length()); } } C:\a.workspace\eclipse.workspace\ejb3.serv1\testSer\build\classes>type j2.security.test.policy grant { permission java.io.FilePermission "<<ALL FILES>>", "read"; }; C:\a.workspace\eclipse.workspace\ejb3.serv1\testSer\build\classes>java -cp . test.TestFileLength 1266 C:\a.workspace\eclipse.workspace\ejb3.serv1\testSer\build\classes>java -cp . -Djava.security.manager test.TestFileLength Exception in thread "main" java.security.AccessControlException: Access denied (java.io.FilePermission C:\a.workspace\ec lipse.workspace\ejb3.serv1\testSer\src\test\TestFileLength.java read) at java.security.AccessController.checkPermission(AccessController.java:104) at java.lang.SecurityManager.checkPermission(SecurityManager.java:547) at java.lang.SecurityManager.checkRead(SecurityManager.java:886) at java.io.File.length(File.java:839) at test.TestFileLength.main(TestFileLength.java:38) C:\a.workspace\eclipse.workspace\ejb3.serv1\testSer\build\classes>java -cp . -Djava.security.manager -Djava.security.policy=j2.security.test.policy test.TestFileLength 1266 C:\a.workspace\eclipse.workspace\ejb3.serv1\testSer\build\classes> ------------------------------------------ May be the reason the tests passed before is because the first condition of the expression in XMLFileHandler is false and the f.length() is not evaluated. - if (!f.exists() || f.length() == 0) BTW, scanning for .length() in the source code one more time and I found another instance of _file.length() that needs the doPriv. I corrected the problem and a new patch is attached. Albert Lee. Thanks for your attention to detail here. I'm still concerned that the patch to the security policy file that grants all permissions to all code bases masks the issues. See my comments from earlier today 10:52 AM.
Craig Craig,
After some experimentation to narrow down the code bases and permissions required for the test bucket, here is the refined policy: // derby code base grant CodeBase "file:///${user.home}/.m2/repository/org/apache/derby/derby/-" { permission java.io.FilePermission "<<ALL FILES>>", "read,write,delete"; permission java.lang.RuntimePermission "createClassLoader"; permission java.util.PropertyPermission "derby.*", "read"; }; // openjpa code base. grant CodeBase "file:///${test.basedir}/-" { permission java.io.FilePermission "<<ALL FILES>>", "read,write"; permission java.io.SerializablePermission "enableSubstitution"; permission java.lang.RuntimePermission "accessDeclaredMembers"; permission java.lang.RuntimePermission "createClassLoader"; permission java.lang.RuntimePermission "getClassLoader"; permission java.lang.RuntimePermission "setIO"; permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; permission java.util.PropertyPermission "*", "read,write"; }; // depending packages code base, e.g junit, surefire etc. grant CodeBase "file:///${user.home}/.m2/repository/-" { permission java.io.FilePermission "<<ALL FILES>>", "read,write"; permission java.io.SerializablePermission "enableSubstitution"; permission java.lang.RuntimePermission "accessDeclaredMembers"; permission java.lang.RuntimePermission "createClassLoader"; permission java.lang.RuntimePermission "getClassLoader"; permission java.lang.RuntimePermission "setContextClassLoader"; permission java.lang.RuntimePermission "setIO"; permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; permission java.util.PropertyPermission "*", "read,write"; }; Attached is a new patch with this new policy. I hope this has addressed your concern. Albert Lee, I'm re-opening the issue because it looks like there are at least a few secure calls that were missed. I ran a test by building a new rt.jar with a java.lang.SecurityException that extends java.lang.Exception (instead of java.lang.RuntimeException), and then compiling the openjpa classes with the new rt.jar in the bootclasspath, which does a nice job at finding all the calls to methods that might throw SecutiryException.
For example, FieldMetaData.java:1477 contains the line "Method[] methods = cls.getMethods()". Are these oversights, or is there some reason that these calls don't need to be wrapped in doPriv blocks? I almost commented on this earlier. I'm not sure that the grant of CodeBase "file:///${user.home}/.m2/repository/-" { and grant CodeBase "file:///${test.basedir}/-" { are correct.
The grants might mask the required grants in OpenJPA by "catching" the illegal access by the test case or the framework. Do we know why the test.basedir needs e.g. suppressAccessChecks? Do the test cases themselves use reflection? Marc,
Finding 'ALL' the security sensitive calls are tedious and error-prone. I probably have missed some along the way. I like your approach to get a more complete picture and catch them all. I'll try to see if I can go through it one more time to be sure. Stay tune. Albert Lee. >> I'm re-opening the issue because it looks like there are at least a few secure calls that were missed. I ran a test by building a new rt.jar with a java.lang.SecurityException that extends java.lang.Exception (instead of java.lang.RuntimeException), and then compiling the openjpa classes with the new rt.jar in the bootclasspath, which does a nice job at finding all the calls to methods that might throw SecutiryException.
This is a reasonable technique to catch some/most of the methods. If the calling method has a try { } catch (Exception e)... bracket the security sensitive call, it will still compile without error. I'll need to use some other means, in combination to this suggestion, to get a more complete picture. >> For example, FieldMetaData.java:1477 contains the line "Method[] methods = cls.getMethods()". >> Are these oversights, or is there some reason that these calls don't need to be wrapped in doPriv blocks? There are a set of getter methods in Class.class, e.g. getMethod(s), getField(s), getClasses, getConstructor(s) etc.. documented to throw SecurityException. The contract for these methods are: ----------------------------- SecurityException - If a security manager, s, is present and any of the following conditions is met: * invocation of s.checkMemberAccess(this, Member.PUBLIC) denies access to the field * the caller's class loader is not the same as or an ancestor of the class loader for the current class and invocation of s.checkPackageAccess() denies access to the package of this class ----------------------------- The reason no doPriv is needed for these conditons are: 1) Since all these methods returns public/visible constructs, s.CheckMemberAccess(PUBLIC) will always pass with no exception. 2) If s.checkPackageAccess() is tested, we need to make sure the package of this class is not in java.security 's "package.access" list. If it is, explicit permission must be specified in the policy, Typically, I only see "sun." package is in this "package.access" list, therefore, we do not see any security violation for this condition. I'm continuing to investigate and make sure no security sensitive method calls are missed. I'll leave the Jira report open for further required changes. Thanks Marc. Albert Lee. >> I almost commented on this earlier. I'm not sure that the grant of CodeBase "file:///${user.home}/.m2/repository/-" { and grant CodeBase "file:///${test.basedir}/-" { are correct.
Typically a security exception stack looks something like this: java.security.AccessControlException: Access denied (java.util.PropertyPermission localRepository write) at java.security.AccessController.checkPermission(AccessController.java:104) at java.lang.SecurityManager.checkPermission(SecurityManager.java:547) at java.lang.System.setProperty(System.java:385) at org.apache.maven.surefire.booter.SurefireBooter.setSystemProperties(SurefireBooter.java:624) at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:692) or java.security.AccessControlException: Access denied (java.lang.reflect.ReflectPermission suppressAccessChecks) at java.security.AccessController.checkPermission(AccessController.java:104) at java.lang.SecurityManager.checkPermission(SecurityManager.java:547) at java.lang.reflect.AccessibleObject.setAccessible(AccessibleObject.java:119) at org.apache.openjpa.event.MethodLifecycleCallbacks.makeCallback(MethodLifecycleCallbacks.java:87) at org.apache.openjpa.event.LifecycleEventManager.makeCallbacks(LifecycleEventManager.java:329) at org.apache.openjpa.event.LifecycleEventManager.fireEvent(LifecycleEventManager.java:291) at org.apache.openjpa.kernel.BrokerImpl.fireLifecycleEvent(BrokerImpl.java:671) at org.apache.openjpa.kernel.BrokerImpl.persist(BrokerImpl.java:2393) at org.apache.openjpa.kernel.BrokerImpl.persist(BrokerImpl.java:2244) at org.apache.openjpa.kernel.DelegatingBroker.persist(DelegatingBroker.java:1010) at org.apache.openjpa.persistence.EntityManagerImpl.persist(EntityManagerImpl.java:541) at org.apache.openjpa.persistence.callbacks.TestExceptionsFromCallbacks.testPrePersistException(TestExceptionsFromCallbacks.java:50) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:615) at junit.framework.TestCase.runTest(TestCase.java:154) at junit.framework.TestCase.runBare(TestCase.java:127) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:118) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:615) at org.apache.maven.surefire.junit.JUnitTestSet.execute(JUnitTestSet.java:210) at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:135) at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:122) at org.apache.maven.surefire.Surefire.run(Surefire.java:129) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:615) at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:225) at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:747) The 3 major packages that needs security permission are org.apache.maven.surefire.*, junit.* and org.apache.*.Test*. The CodeBase "file:///${user.home}/.m2/repository/-" is for the first 2 package category and CodeBase "file:///${test.basedir}/-" is for the openjpa test cases. So the 'grant's are needed for these CodeBase. I can further narrow down the surefire and junit. However some of the openjpa tests use the same packages as the code (e.g. org.apache.openjpa.persistence.jdbc), so the grant codebase for the tests may not be specific just to the test packages. >> The grants might mask the required grants in OpenJPA by "catching" the illegal access by the test case or the framework. I agree, see reason before. >> Do we know why the test.basedir needs e.g. suppressAccessChecks? Do the test cases themselves use reflection? This is a oversight because AccessibleObject.setAccessible is not being bracketted with doPrive. I'll get this fix. Albert Lee. After a more comprehensive search, here is the second ( and hopefully last ) round of changes needed to enable Java 2 security.
Looks good. Just a couple of comments:
1. Lots of cases of white space differences. Here's an example: Index: openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java =================================================================== --- openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java (revision 562121) +++ openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java (working copy) @@ -14,7 +14,7 @@ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * KIND, either express or implied. See the License for the * specific language governing permissions and limitations - * under the License. + * under the License. I have no objection to cleaning up white space (removing trailing spaces, converting tabs to spaces, etc.) but it should be a separate checkin with an innocuous "Fixed white space" comment. 2. In file Index: openjpa-kernel/src/main/java/org/apache/openjpa/util/Serialization.java should we define a new action for this case: @@ -104,7 +105,12 @@ throws IOException { super(delegate); _ctx = ctx; - enableReplaceObject(true); + AccessController.doPrivileged(new PrivilegedAction() { + public Object run() { + enableReplaceObject(true); + return null; + } + }); } like J2DoPrivHelper.replaceObject()? 3. And the same for enableResolveObject? Craig,
Thanks for reviewing the patch. Regarding the Serialization changes that the enableReplaceObject PrivilegedAction is created in-line in the code and not in J2DoPrivHelper, the reasons are: 1) PersistentObjectOutputStream extends ObjectOutputStream. 2) enabelReplaceObject is defined in ObjectOutputStream and is qualified as protected. 3) enabelReplaceObject is called in the PersistentObjectOutputStream constructor. 4) Initially, I had the following helper to get the enableReplaceObjectAction, public static final PrivilegedAction enableReplaceObjectAction( final ObjectOutputStream oos, final boolean enable) { return new PrivilegedAction() { public Object run() { oos.enableReplaceObject(enable); return null; } }; } but this will not compile due to: J2DoPrivHelper.java: enableReplaceObject(boolean) has protected access in java.io.ObjectOutputStream oos.enableReplaceObject(enable); ^ Hence the in-line alternative is used to perform the doPrivlege call. The same reason applies to the ObjectInputStream.enableResolveObject method call. Albert Lee. I believe Albert's patch that I just committed now resolves this Issue. As Albert pointed out, hopefully we have found and fixed all of these java 2 security holes.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Java 2 Security enablement
The goals of the enhancements are:
1) non-intrusive changes.
2) easy readability and future usages
3) sensitive to downstream security exposure
4) maintanence of the additional code.
Approach to the solution:
1) Create a static helper class J2DoPrivHelper.java in openjpa-lib\src\main\java\org\apache\openjpa\lib\util. See attachment for the content. The purpose of this class is isolate Java 2 security related code in one place for control and maintenance.
2) Each JDK functions that required doPrivileged encasement associated to a static method in the helper class. The name of the method is closely related to the JDK function. If it is an instance method, the first argument is the instance object. So far I have identified 24 JDK helper methods.
3) Where there is a usage of the security sensitive method call, it can be translated to one of the helper method.
E.g.
a) From
return _url.openStream();
To
return J2DoPrivHelper.openStream(_url);
b) From
ClassLoader loader = cls.getClassLoader();
To
ClassLoader loader = J2DoPrivHelper.getClassLoader(cls);
c) From
loader = ClassLoader.getSystemClassLoader();
To
loader = J2DoPrivHelper.getSystemClassLoader();
To
4) These method call translations will be to the closest place where the doPriv is needed. This will eliminate the possible security "leak" in the down stream code. E.g. callback to unsecured code inside the doPriv encasement.
5) There are approximately 71 files affected, excluding test cases that use the same security sensitive methods.
6) Document the permissions required by Java 2 security used in openjpa.
E.g.
permission java.lang.RuntimePermission "getClassLoader";
permission java.io.FilePermission "<<ALL FILES>>", "read";
I have a prototype of these changes and it is working in the WebSphere environment.
I am open for suggestions and ideas. I continue to work on this path unless I hear there is any objection otherwise.
Thanks.
Albert Lee