[
https://issues.apache.org/jira/browse/GROOVY-8163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15990743#comment-15990743
]
Jochen Theodorou commented on GROOVY-8163:
------------------------------------------
I think the idea of the patch is good and we should think about integrating it.
Since it won´t do anything without a security manager being set it should be
not causing trouble for example for testing code. But I also think the patch
will not solve all the attack vectors. For example if a subclass of ClassLoader
overwrites defineClass, your patch will not catch that. Your code will also
catch a lot less if invokedynamic is enabled.
But anyway, I still think this would be a good start..
btw Dimitry, it would be even better if you provided this as pull request on
github
> 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)
