Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-8163

Groovy scripts can disable java security manager and escape sandbox

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.0-alpha-1, 2.4.9, 2.4.10
    • Fix Version/s: 2.5.0-beta-2
    • Component/s: None
    • Labels:
      None

      Description

      Consider following test

      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());
      	}
      
      }
      

      with following policy file restrictedPermissionsForScriptOnlyPolicy.txt

      grant codeBase "${dir.test}" {
      	permission java.security.AllPermission;
      };
      
      grant codeBase "${dir.groovy}" {
      	permission java.security.AllPermission;
      };
      
      grant {
      };
      
      

      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

      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";
      };
      
      

      it has a consequence that groovy can not access even some public methods on bean properties as shown in the following test

      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;
      			}
      		});
      	}
      }
      

      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

      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()
      		);
      	}
      }
      

      patching your classes as follows:

      CachedField.java:

          /**
           * @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);
              }
          }
      

      CachedMethod.java:

          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;
          }
      }
      
      

      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.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                jwagenleitner John Wagenleitner
                Reporter:
                dpolivaev Dimitry Polivaev
              • Votes:
                3 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: