[ https://issues.apache.org/jira/browse/GROOVY-8163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16009353#comment-16009353 ]
Dimitry Polivaev commented on GROOVY-8163: ------------------------------------------ [~blackdrag] I would appreciate any feedback about the patch I submitted. > Groovy scripts can disable java security manager and escape sandbox > ------------------------------------------------------------------- > > Key: GROOVY-8163 > URL: https://issues.apache.org/jira/browse/GROOVY-8163 > Project: Groovy > Issue Type: Bug > Affects Versions: 2.5.0-alpha-1, 2.4.9, 2.4.10 > Reporter: Dimitry Polivaev > > Consider following test > {code} > package groovytest; > import groovy.util.Eval; > import org.junit.*; > import java.net.URL; > import java.security.AccessController; > import java.security.PrivilegedAction; > public class GroovySecurityTest { > public static final String > RESTRICTED_PERMISSIONS_FOR_SCRIPT_ONLY_POLICY = > "/restrictedPermissionsForScriptOnlyPolicy.txt"; > public static final String POLICY = > RESTRICTED_PERMISSIONS_FOR_SCRIPT_ONLY_POLICY; > @BeforeClass > public static void setPolicy() throws Exception { > final String dirTest = > GroovySecurityTest.class.getProtectionDomain().getCodeSource().getLocation().toString(); > final String dirGroovy = > Eval.class.getProtectionDomain().getCodeSource().getLocation().toString(); > System.setProperty("dir.test",dirTest + "-"); > System.setProperty("dir.groovy",dirGroovy); > final URL policy = GroovySecurityTest.class.getResource(POLICY); > System.setProperty("java.security.policy", policy.toString()); > } > > > @Before > public void setSecurityManager() throws Exception { > System.setSecurityManager(new SecurityManager()); > } > @After > public void removeSecurityManager() throws Exception { > AccessController.doPrivileged(new PrivilegedAction<Void>() { > @Override > public Void run() { > System.setSecurityManager(null); > return null; > } > }); > } > @Test > public void doesNotChangeScriptPermissionsUsungPrivateFieldAccess() > throws Exception { > try { > AccessController.doPrivileged(new > PrivilegedAction<Void>() { > @Override > public Void run() { > Eval.me("getClass().protectionDomain0.hasAllPerm = true;" > + "System.setSecurityManager(null);" > + "1"); > return null; > } > }); > } catch (Exception e) { > } > Assert.assertNotNull(System.getSecurityManager()); > } > } > {code} > with following policy file restrictedPermissionsForScriptOnlyPolicy.txt > {code} > grant codeBase "${dir.test}" { > permission java.security.AllPermission; > }; > grant codeBase "${dir.groovy}" { > permission java.security.AllPermission; > }; > grant { > }; > {code} > It fails: security manager is not set any more when the test assertion is > checked. > It happens because CachedField from org.codehaus.groovy.reflection is created > withing trusted code base (groovy jar) and gives access to the field to > untrusted scripts without any security checks. The same problem relates to > CachedMethod which would allow any script to access protected method > java.lang.ClassLoader#defineClass(java.lang.String, byte[], int, int, > java.security.ProtectionDomain) that can be misused to manipulate code > sources of classes loaded from script to give them all permissions. > It also appears that if I remove permissions from groovy.jar using more > restrictive policy using following policy file restrictedPermissionsPolicy.txt > {code} > grant codeBase "${dir.test}" { > permission java.lang.RuntimePermission "*"; > permission java.security.SecurityPermission "*"; > permission java.io.FilePermission "<<ALL FILES>>", "read"; > permission java.util.PropertyPermission "*", "read"; > permission groovy.security.GroovyCodeSourcePermission "*"; > }; > grant codeBase "${dir.groovy}" { > permission java.lang.RuntimePermission "*"; > permission java.security.SecurityPermission "*"; > permission java.io.FilePermission "<<ALL FILES>>", "read"; > permission java.util.PropertyPermission "*", "read"; > permission groovy.security.GroovyCodeSourcePermission "*"; > }; > grant { > permission java.lang.RuntimePermission "accessDeclaredMembers"; > }; > {code} > it has a consequence that groovy can not access even some public methods on > bean properties as shown in the following test > {code} > package groovytest; > import groovy.util.Eval; > import org.junit.*; > import java.net.URL; > import java.security.AccessController; > import java.security.PrivilegedAction; > public class GroovyBeanTest { > public static final String RESTRICTED_PERMISSIONS_POLICY = > "/restrictedPermissionsPolicy.txt"; > public static final String POLICY = RESTRICTED_PERMISSIONS_POLICY; > @BeforeClass > public static void setPolicy() throws Exception { > final String dirTest = > GroovyBeanTest.class.getProtectionDomain().getCodeSource().getLocation().toString(); > final String dirGroovy = > Eval.class.getProtectionDomain().getCodeSource().getLocation().toString(); > System.setProperty("dir.test",dirTest + "-"); > System.setProperty("dir.groovy",dirGroovy); > final URL policy = GroovyBeanTest.class.getResource(POLICY); > System.setProperty("java.security.policy", policy.toString()); > } > > > @Before > public void setSecurityManager() throws Exception { > System.setSecurityManager(new SecurityManager()); > } > @After > public void removeSecurityManager() throws Exception { > AccessController.doPrivileged(new PrivilegedAction<Void>() { > @Override > public Void run() { > System.setSecurityManager(null); > return null; > } > }); > } > > public interface BeanInterface{ > public String getName(); > } > private class Bean implements BeanInterface{ > private String _name = "bean"; > public String getName(){ > return _name; > } > } > @Test > public void returnsBeanPropertyValue() throws Exception { > AccessController.doPrivileged(new PrivilegedAction<Void>() { > @Override > public Void run() { > Assert.assertEquals("bean", Eval.x(new Bean(), > "x.name")); > return null; > } > }); > } > static class BeanClass { > private String name = "bean"; > } > @Test > public void returnsBeanClassName() throws Exception { > AccessController.doPrivileged(new PrivilegedAction<Void>() { > @Override > public Void run() { > Assert.assertEquals(BeanClass.class.getName(), > Eval.x(new BeanClass(), "x.class.name")); > return null; > } > }); > } > } > {code} > The both tests fail as the bean properties can not be found by groovy. > It turned out that I can not run the both tests in one process, make sure you > run them separately. > In order to fix this issue for my open source project Freeplane I have to > patch groovy code. It turned out to be possible. > So I want to share the fix with you and ask you to integrate it. > The fix is based on groovy version 2.4.9 and I think that it can be applied > to any Groovy version. > You only need to check for access permissions at the relevant places to make > sure that they are not leaked from groovy (which needs them to work properly) > to the untrusted script > {code} > package org.codehaus.groovy.reflection; > import java.lang.reflect.Field; > import java.lang.reflect.Method; > import java.lang.reflect.Modifier; > import java.lang.reflect.ReflectPermission; > import groovy.lang.GroovyObject; > class AccessPermissionChecker { > private static final ReflectPermission REFLECT_PERMISSION = new > ReflectPermission("suppressAccessChecks"); > static private void checkAccessPermission(Class<?> declaringClass, > final int modifiers, boolean isAccessible) { > final SecurityManager securityManager = > System.getSecurityManager(); > if (isAccessible && securityManager != null) { > if ((modifiers & (Modifier.PUBLIC | > Modifier.PROTECTED)) == 0 > && > !GroovyObject.class.isAssignableFrom(declaringClass)) { > securityManager.checkPermission(REFLECT_PERMISSION); > } > else if ((modifiers & (Modifier.PUBLIC)) == 0 > && > declaringClass.equals(ClassLoader.class)){ > > securityManager.checkCreateClassLoader(); > } > } > } > static public void checkAccessPermission(Method method) { > checkAccessPermission(method.getDeclaringClass(), > method.getModifiers(), method.isAccessible() > ); > } > public static void checkAccessPermission(Field field) { > checkAccessPermission(field.getDeclaringClass(), > field.getModifiers(), field.isAccessible() > ); > } > } > {code} > patching your classes as follows: > CachedField.java: > {code} > /** > * @return the property of the given object > * @throws RuntimeException if the property could not be evaluated > */ > public Object getProperty(final Object object) { > try { > AccessPermissionChecker.checkAccessPermission(field); > } > catch (AccessControlException ex) { > throw new IllegalArgumentException("Illegal access to field" + " > " + field.getName()); > } > try { > return field.get(object); > } catch (IllegalAccessException e) { > throw new GroovyRuntimeException("Cannot get the property '" + > name + "'.", e); > } > } > /** > * Sets the property on the given object to the new value > * > * @param object on which to set the property > * @param newValue the new value of the property > * @throws RuntimeException if the property could not be set > */ > public void setProperty(final Object object, Object newValue) { > try { > AccessPermissionChecker.checkAccessPermission(field); > } > catch (AccessControlException ex) { > throw new IllegalArgumentException("Illegal access to field" + " > " + field.getName()); > } > final Object goalValue = > DefaultTypeTransformation.castToType(newValue, field.getType()); > if (isFinal()) { > throw new GroovyRuntimeException("Cannot set the property '" + > name + "' because the backing field is final."); > } > try { > field.set(object, goalValue); > } catch (IllegalAccessException ex) { > throw new GroovyRuntimeException("Cannot set the property '" + > name + "'.", ex); > } > } > {code} > CachedMethod.java: > {code} > public final Object invoke(Object object, Object[] arguments) { > try { > AccessPermissionChecker.checkAccessPermission(cachedMethod); > } > catch (AccessControlException ex) { > throw new InvokerInvocationException(new > IllegalArgumentException("Illegal access to method" + > cachedMethod.getName())); > } > try { > return cachedMethod.invoke(object, arguments); > } catch (IllegalArgumentException e) { > throw new InvokerInvocationException(e); > } catch (IllegalAccessException e) { > throw new InvokerInvocationException(e); > } catch (InvocationTargetException e) { > Throwable cause = e.getCause(); > throw (cause instanceof RuntimeException && !(cause instanceof > MissingMethodException)) ? > (RuntimeException) cause : new > InvokerInvocationException(e); > } > } > public final Method setAccessible() { > try { > AccessPermissionChecker.checkAccessPermission(cachedMethod); > } > catch (AccessControlException ex) { > throw new IllegalArgumentException("Illegal access to method" + > cachedMethod.getName()); > } > // if (queuedToCompile.compareAndSet(false,true)) { > // if (isCompilable()) > // CompileThread.addMethod(this); > // } > return cachedMethod; > } > public Method getCachedMethod() { > try { > AccessPermissionChecker.checkAccessPermission(cachedMethod); > } > catch (AccessControlException ex) { > throw new IllegalArgumentException("Illegal access to method" + > cachedMethod.getName()); > } > return cachedMethod; > } > } > {code} > In order to apply the patch in Freeplane I created a separate project > https://github.com/dpolivaev/securegroovy which generates the patch at > runtime using bytebuddy. > I would appreciate if you could integrate the patch in groovy code. > There is one subtle issue with AccessPermissionChecker: as you see it allows > access to all protected methods except for ClassLoader without any further > checks. > But for the protected methods from ClassLoader is makes one additional check: > Otherwise a script could access a class loader by getClass().getClassLoader() > and misuse its defineClass method to let malicious code appear trusted. -- This message was sent by Atlassian JIRA (v6.3.15#6346)