Prevent CachedField and CachedMethod from leaking access permissions … #532

dpolivaev · 2017-05-01T20:59:14Z

…to scripts

https://issues.apache.org/jira/browse/GROOVY-8163

jwagenleitner · 2017-05-13T19:01:57Z

src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java

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


Might be good to exit early if securityManager == null, then wont have to check again below.

Also, be good to change order to be private static void ....

Signature changed. The check is performed only if (securityManager != null && isAccessible)

jwagenleitner · 2017-05-13T19:04:11Z

src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java

+		return declaringClass.getName().startsWith("java.");
+	}
+
+	static public void checkAccessPermission(Method method) {


public isn't needed

jwagenleitner · 2017-05-13T19:04:24Z

src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java

+		);
+	}
+
+	public static void checkAccessPermission(Field field) {


public isn't needed

jwagenleitner · 2017-05-13T19:09:23Z

src/main/org/codehaus/groovy/reflection/CachedField.java

+        try {
+            AccessPermissionChecker.checkAccessPermission(field);
+        }
+        catch (AccessControlException ex) {


Any reason for using IllegalArgumentException? I think the following may be more appropriate:

throw new GroovyRuntimeException("Illegal access to field " + field.getName() + ".", ex);

To use GroovyRuntimeException we need to patch also MetaClassImpl.getProperty . It was not possible with byte buddy, but as a groovy patch it is possible. I shall do it in a separate commit.

jwagenleitner · 2017-05-13T19:09:59Z

src/main/org/codehaus/groovy/reflection/CachedField.java

+            AccessPermissionChecker.checkAccessPermission(field);
+        }
+        catch (AccessControlException ex) {
+            throw new IllegalArgumentException("Illegal access to field" + " " + field.getName());


see above comment about GroovyRuntimeException

jwagenleitner · 2017-05-13T19:16:27Z

src/main/org/codehaus/groovy/reflection/CachedMethod.java

+        try {
+            AccessPermissionChecker.checkAccessPermission(cachedMethod);
+        }
+        catch (AccessControlException ex) {


Throwing an IllegalArgumentException could be confusing since it's not the arguments passed to the invoked method causing the problem. Maybe

throw new InvokerInvocationException(ex)

or

throw new InvokerInvocationException( new AccessControlException("Illegal access to method" + cachedMethod.getName(), ex) )

jwagenleitner · 2017-05-13T19:23:29Z

src/main/org/codehaus/groovy/reflection/CachedMethod.java

+            AccessPermissionChecker.checkAccessPermission(cachedMethod);
+        }
+        catch (AccessControlException ex) {
+            throw new IllegalArgumentException("Illegal access to method" + cachedMethod.getName());


No arguments to this method so IllegalArgumentException doesn't seem right. Think it might be good to either just remove the try/catch or wrap in a GroovyRuntimeException (to capture cachedMethod.getName).

jwagenleitner · 2017-05-13T19:24:08Z

src/main/org/codehaus/groovy/reflection/CachedMethod.java

+            AccessPermissionChecker.checkAccessPermission(cachedMethod);
+        }
+        catch (AccessControlException ex) {
+            throw new IllegalArgumentException("Illegal access to method" + cachedMethod.getName());


see comment on setAccessible

jwagenleitner · 2017-05-13T19:30:16Z

src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java

+
+import groovy.lang.GroovyObject;
+
+class AccessPermissionChecker {


Might be good to add a private constructor to signal that no instances of this method are desired, since it only contains static methods.

dpolivaev · 2017-05-14T11:58:53Z

Changes suggested in the review are implemented.
See also https://issues.apache.org/jira/browse/GROOVY-8163?focusedCommentId=16009695&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16009695

jwagenleitner · 2017-05-17T18:33:40Z

src/main/groovy/lang/MetaClassImpl.java

                mp = null;
+            } catch (GroovyRuntimeException e) {
+                // can't access the field directly but there may be a getter
+                mp = null;


I don't understand why this was added. Was there a test that failed because of the other changes that required this. I would have assumed an AccessControlException from CachedField.getProperty would be treated similar to the IllegalAccessException in the same method.

I do not have unit test to explain this catch block. I do have the integration test https://issues.apache.org/jira/browse/GROOVY-8163?focusedCommentId=16009695&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16009695. The problem is that for some classes C with private member called "name" class property C.class.name which corresponds to java calls C.class.getName() does not work unless this catch block is added.

blackdrag · 2017-05-18T07:32:18Z

src/main/groovy/lang/MetaClassImpl.java

            } catch (IllegalArgumentException e) {
                // can't access the field directly but there may be a getter
                mp = null;
+            } catch (GroovyRuntimeException e) {


At this point mp should be a CachedField. Since AccessPermissionChecker.checkAccessPermission(field); may now throw a GroovyRuntimeException, we have here the requirement to catch it. Problem is, that mp might not be a CachedField. If we will call a user method here instead, this would lead to a wrong flow. Thus I think the GroovyRuntimeException usage is a bad idea. Better make a new Exception, that extends GroovyRuntimeException and throw that in AccessPermissionChecker, which we then can catch

I think AccessPermissionChecker should only throw exceptions extending from AccessControlException. because it specifically checks access permissions.

So I suggest to introduce public class CacheAccessControlException extends AccessControlException

On the other side if it does not work because AccessControlExceptions are not handled by the calling code properly public class CacheAccessControlException extends GroovyRuntimeException could also be taken as a hack.

How do you see it?

blackdrag · 2017-05-18T07:33:25Z

src/main/org/codehaus/groovy/reflection/CachedMethod.java

+        catch (AccessControlException ex) {
+            throw new InvokerInvocationException(new GroovyRuntimeException("Illegal access to method" + cachedMethod.getName()));
+        }
        try {


can we move thos try-catch into checkAccessPermission ?

I think that InvokerInvocationException is specific to call of invoke() and it should be not thrown by AccessPermissionChecker directly.

dpolivaev · 2017-05-27T21:05:19Z

Could you please review the changes I implemented on your behalf?

blackdrag · 2017-05-28T06:48:50Z

+1

jwagenleitner · 2017-05-29T01:33:01Z

@dpolivaev can you squash up the commits and change the tabs in AccessPermissionChecker to spaces? If not, just let me know and I can take care of it. In the future, it's helpful to create a separate branch for the PR to make it easier to rebase and avoid merge commits.

@blackdrag I would guess this is a candidate for 2_5_X and above, but maybe too much of a change for 2.4.12, though maybe not breaking in a code sense it could affect those running with security managers. Do you agree?

blackdrag · 2017-05-29T16:10:27Z

agreed

dpolivaev · 2017-06-03T18:07:35Z

Done

… permissions (closes #532)

jwagenleitner · 2017-06-04T23:20:39Z

Thanks.

jwagenleitner reviewed May 13, 2017

View reviewed changes

jwagenleitner reviewed May 17, 2017

View reviewed changes

blackdrag reviewed May 18, 2017

View reviewed changes

Prevent CachedField and CachedMethod from leaking access permissions

78010e9

dpolivaev force-pushed the master branch from 0229044 to 78010e9 Compare June 3, 2017 18:06

asfgit closed this in ddecb50 Jun 4, 2017

asfgit pushed a commit that referenced this pull request Jun 4, 2017

GROOVY-8163: Prevent CachedField and CachedMethod from leaking access…

b039c35

… permissions (closes #532)

asfgit pushed a commit that referenced this pull request Jun 4, 2017

GROOVY-8163: Prevent CachedField and CachedMethod from leaking access…

0305a38

… permissions (closes #532)


		import groovy.lang.GroovyObject;

		class AccessPermissionChecker {

Prevent CachedField and CachedMethod from leaking access permissions … #532

Prevent CachedField and CachedMethod from leaking access permissions … #532

Uh oh!

Conversation

dpolivaev commented May 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpolivaev commented May 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpolivaev commented May 27, 2017

Uh oh!

blackdrag commented May 28, 2017

Uh oh!

jwagenleitner commented May 29, 2017

Uh oh!

blackdrag commented May 29, 2017

Uh oh!

dpolivaev commented Jun 3, 2017

Uh oh!

jwagenleitner commented Jun 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development