You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by dpolivaev <gi...@git.apache.org> on 2017/05/01 20:59:20 UTC
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
GitHub user dpolivaev opened a pull request:
https://github.com/apache/groovy/pull/532
Prevent CachedField and CachedMethod from leaking access permissions \u2026
\u2026to scripts
https://issues.apache.org/jira/browse/GROOVY-8163
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dpolivaev/groovy master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/532.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #532
----
commit 20741fe4f61940a2e5ab56c67d0710a17ac5583f
Author: Dimitry Polivaev <dp...@gmx.de>
Date: 2017-05-01T20:58:12Z
Prevent CachedField and CachedMethod from leaking access permissions to scripts
https://issues.apache.org/jira/browse/GROOVY-8163
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377501
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -324,6 +337,12 @@ else if (o2 instanceof CachedMethod)
}
public Method getCachedMethod() {
+ try {
+ AccessPermissionChecker.checkAccessPermission(cachedMethod);
+ }
+ catch (AccessControlException ex) {
+ throw new IllegalArgumentException("Illegal access to method" + cachedMethod.getName());
--- End diff --
As above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365112
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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();
--- End diff --
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 ...`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117177601
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117188688
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377435
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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.PRIVATE) != 0
+ || ((modifiers & (Modifier.PUBLIC | Modifier.PROTECTED)) == 0
+ && packageCanNotBeAddedAnotherClass(declaringClass))
+ && !GroovyObject.class.isAssignableFrom(declaringClass)) {
+ securityManager.checkPermission(REFLECT_PERMISSION);
+ }
+ else if ((modifiers & (Modifier.PROTECTED)) != 0
+ && declaringClass.equals(ClassLoader.class)){
+ securityManager.checkCreateClassLoader();
+ }
+ }
+ }
+
+ private static boolean packageCanNotBeAddedAnotherClass(Class<?> declaringClass) {
+ return declaringClass.getName().startsWith("java.");
+ }
+
+ static public void checkAccessPermission(Method method) {
+ checkAccessPermission(method.getDeclaringClass(), method.getModifiers(), method.isAccessible()
+ );
+ }
+
+ public static void checkAccessPermission(Field field) {
--- End diff --
fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365223
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -65,6 +72,12 @@ public Object getProperty(final Object object) {
* @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());
--- End diff --
see above comment about `GroovyRuntimeException`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377432
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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.PRIVATE) != 0
+ || ((modifiers & (Modifier.PUBLIC | Modifier.PROTECTED)) == 0
+ && packageCanNotBeAddedAnotherClass(declaringClass))
+ && !GroovyObject.class.isAssignableFrom(declaringClass)) {
+ securityManager.checkPermission(REFLECT_PERMISSION);
+ }
+ else if ((modifiers & (Modifier.PROTECTED)) != 0
+ && declaringClass.equals(ClassLoader.class)){
+ securityManager.checkCreateClassLoader();
+ }
+ }
+ }
+
+ private static boolean packageCanNotBeAddedAnotherClass(Class<?> declaringClass) {
+ return declaringClass.getName().startsWith("java.");
+ }
+
+ static public void checkAccessPermission(Method method) {
--- End diff --
fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117188013
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
--- End diff --
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`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377367
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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 {
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365151
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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.PRIVATE) != 0
+ || ((modifiers & (Modifier.PUBLIC | Modifier.PROTECTED)) == 0
+ && packageCanNotBeAddedAnotherClass(declaringClass))
+ && !GroovyObject.class.isAssignableFrom(declaringClass)) {
+ securityManager.checkPermission(REFLECT_PERMISSION);
+ }
+ else if ((modifiers & (Modifier.PROTECTED)) != 0
+ && declaringClass.equals(ClassLoader.class)){
+ securityManager.checkCreateClassLoader();
+ }
+ }
+ }
+
+ private static boolean packageCanNotBeAddedAnotherClass(Class<?> declaringClass) {
+ return declaringClass.getName().startsWith("java.");
+ }
+
+ static public void checkAccessPermission(Method method) {
+ checkAccessPermission(method.getDeclaringClass(), method.getModifiers(), method.isAccessible()
+ );
+ }
+
+ public static void checkAccessPermission(Field field) {
--- End diff --
`public` isn't needed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377418
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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();
--- End diff --
Signature changed. The check is performed only if (securityManager != null && isAccessible)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377495
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -65,6 +72,12 @@ public Object getProperty(final Object object) {
* @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());
--- End diff --
As above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117188820
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
--- End diff --
How do you see it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365210
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -51,6 +52,12 @@ public int getModifiers() {
*/
public Object getProperty(final Object object) {
try {
+ AccessPermissionChecker.checkAccessPermission(field);
+ }
+ catch (AccessControlException ex) {
+ throw new IllegalArgumentException("Illegal access to field" + " " + field.getName());
--- End diff --
Any reason for using `IllegalArgumentException`? I think the following may be more appropriate:
```java
throw new GroovyRuntimeException("Illegal access to field " + field.getName() + ".", ex);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377497
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +91,12 @@ public CachedClass getDeclaringClass() {
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()));
--- End diff --
As above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117177738
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +92,12 @@ public CachedClass getDeclaringClass() {
public final Object invoke(Object object, Object[] arguments) {
try {
+ AccessPermissionChecker.checkAccessPermission(cachedMethod);
+ }
+ catch (AccessControlException ex) {
+ throw new InvokerInvocationException(new GroovyRuntimeException("Illegal access to method" + cachedMethod.getName()));
+ }
+ try {
return cachedMethod.invoke(object, arguments);
--- End diff --
can we move thos try-catch into checkAccessPermission ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365354
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +91,12 @@ public CachedClass getDeclaringClass() {
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()));
--- End diff --
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)
)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365147
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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.PRIVATE) != 0
+ || ((modifiers & (Modifier.PUBLIC | Modifier.PROTECTED)) == 0
+ && packageCanNotBeAddedAnotherClass(declaringClass))
+ && !GroovyObject.class.isAssignableFrom(declaringClass)) {
+ securityManager.checkPermission(REFLECT_PERMISSION);
+ }
+ else if ((modifiers & (Modifier.PROTECTED)) != 0
+ && declaringClass.equals(ClassLoader.class)){
+ securityManager.checkCreateClassLoader();
+ }
+ }
+ }
+
+ private static boolean packageCanNotBeAddedAnotherClass(Class<?> declaringClass) {
+ return declaringClass.getName().startsWith("java.");
+ }
+
+ static public void checkAccessPermission(Method method) {
--- End diff --
`public` isn't needed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365514
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -324,6 +337,12 @@ else if (o2 instanceof CachedMethod)
}
public Method getCachedMethod() {
+ try {
+ AccessPermissionChecker.checkAccessPermission(cachedMethod);
+ }
+ catch (AccessControlException ex) {
+ throw new IllegalArgumentException("Illegal access to method" + cachedMethod.getName());
--- End diff --
see comment on `setAccessible`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/532
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117188226
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -90,6 +92,12 @@ public CachedClass getDeclaringClass() {
public final Object invoke(Object object, Object[] arguments) {
try {
+ AccessPermissionChecker.checkAccessPermission(cachedMethod);
+ }
+ catch (AccessControlException ex) {
+ throw new InvokerInvocationException(new GroovyRuntimeException("Illegal access to method" + cachedMethod.getName()));
+ }
+ try {
return cachedMethod.invoke(object, arguments);
--- End diff --
I think that InvokerInvocationException is specific to call of invoke() and it should be not thrown by AccessPermissionChecker directly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117609738
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377482
--- Diff: src/main/org/codehaus/groovy/reflection/CachedField.java ---
@@ -51,6 +52,12 @@ public int getModifiers() {
*/
public Object getProperty(final Object object) {
try {
+ AccessPermissionChecker.checkAccessPermission(field);
+ }
+ catch (AccessControlException ex) {
+ throw new IllegalArgumentException("Illegal access to field" + " " + field.getName());
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117078128
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
+ // can't access the field directly but there may be a getter
+ mp = null;
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r117087531
--- Diff: src/main/groovy/lang/MetaClassImpl.java ---
@@ -1832,6 +1832,9 @@ public Object getProperty(Class sender, Object object, String name, boolean useS
} catch (IllegalArgumentException e) {
// can't access the field directly but there may be a getter
mp = null;
+ } catch (GroovyRuntimeException e) {
+ // can't access the field directly but there may be a getter
+ mp = null;
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365638
--- Diff: src/main/org/codehaus/groovy/reflection/AccessPermissionChecker.java ---
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+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 {
--- End diff --
Might be good to add a private constructor to signal that no instances of this method are desired, since it only contains static methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by dpolivaev <gi...@git.apache.org>.
Github user dpolivaev commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116377499
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -124,6 +131,12 @@ public String getSignature() {
}
public final Method setAccessible() {
+ try {
+ AccessPermissionChecker.checkAccessPermission(cachedMethod);
+ }
+ catch (AccessControlException ex) {
+ throw new IllegalArgumentException("Illegal access to method" + cachedMethod.getName());
--- End diff --
As above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #532: Prevent CachedField and CachedMethod from leaking ...
Posted by jwagenleitner <gi...@git.apache.org>.
Github user jwagenleitner commented on a diff in the pull request:
https://github.com/apache/groovy/pull/532#discussion_r116365503
--- Diff: src/main/org/codehaus/groovy/reflection/CachedMethod.java ---
@@ -124,6 +131,12 @@ public String getSignature() {
}
public final Method setAccessible() {
+ try {
+ AccessPermissionChecker.checkAccessPermission(cachedMethod);
+ }
+ catch (AccessControlException ex) {
+ throw new IllegalArgumentException("Illegal access to method" + cachedMethod.getName());
--- End diff --
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---