Merge pull request #298 from dwnusbaum/JENKINS-42214

jglick · web-flow · commit 39ca7f7ed0ac · 2022-11-16T12:39:01.000-05:00
[JENKINS-42214] SandboxInterceptor must account for static members being accessed via objects instead of class references
diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java
@@ -44,6 +44,7 @@
 import java.util.logging.Logger;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import java.lang.reflect.Modifier;
 import org.codehaus.groovy.runtime.DateGroovyMethods;
 import org.codehaus.groovy.runtime.DefaultGroovyMethods;
 import org.codehaus.groovy.runtime.EncodingGroovyMethods;
@@ -157,12 +158,12 @@ final class SandboxInterceptor extends GroovyInterceptor {
             throw new MissingMethodException(method, receiver.getClass(), args);
         } else if (StaticWhitelist.isPermanentlyBlacklistedMethod(m)) {
             throw StaticWhitelist.rejectMethod(m);
-        } else if (whitelist.permitsMethod(m, receiver, args)) {
+        } else if (permitsMethod(whitelist, m, receiver, args)) {
             return super.onMethodCall(invoker, receiver, method, args);
         } else if (method.equals("invokeMethod") && args.length == 2 && args[0] instanceof String && args[1] instanceof Object[]) {
             throw StaticWhitelist.rejectMethod(m, EnumeratingWhitelist.getName(receiver.getClass()) + " " + args[0] + printArgumentTypes((Object[]) args[1]));
         } else {
-            throw StaticWhitelist.rejectMethod(m);
+            throw rejectMethod(m);
         }
     }
 
@@ -206,11 +207,11 @@ final class SandboxInterceptor extends GroovyInterceptor {
                 ? setterMethods.get(0) // If there is only a single setter, the argument will be cast to match the declared parameter type.
                 : GroovyCallSiteSelector.method(receiver, setter, valueArg); // If there are multiple setters, MultipleSetterProperty just calls invokeMethod.
         if (setterMethod != null) {
-            if (whitelist.permitsMethod(setterMethod, receiver, valueArg)) {
+            if (permitsMethod(whitelist, setterMethod, receiver, valueArg)) {
                 preCheckArgumentCasts(setterMethod, valueArg);
                 return super.onSetProperty(invoker, receiver, property, value);
             } else if (rejector == null) {
-                rejector = () -> StaticWhitelist.rejectMethod(setterMethod);
+                rejector = () -> rejectMethod(setterMethod);
             }
         }
         Object[] propertyValueArgs = new Object[] {property, value};
@@ -223,13 +224,13 @@ final class SandboxInterceptor extends GroovyInterceptor {
                 rejector = () -> StaticWhitelist.rejectMethod(setPropertyMethod, receiver.getClass().getName() + "." + property);
             }
         }
-        final Field instanceField = GroovyCallSiteSelector.field(receiver, property);
-        if (instanceField != null) {
-            if (whitelist.permitsFieldSet(instanceField, receiver, value)) {
-                Checker.preCheckedCast(instanceField.getType(), value, false, false, false);
+        final Field field = GroovyCallSiteSelector.field(receiver, property);
+        if (field != null) {
+            if (permitsFieldSet(whitelist, field, receiver, value)) {
+                Checker.preCheckedCast(field.getType(), value, false, false, false);
                 return super.onSetProperty(invoker, receiver, property, value);
             } else if (rejector == null) {
-                rejector = () -> StaticWhitelist.rejectField(instanceField);
+                rejector = () -> rejectField(field);
             }
         }
         if (receiver instanceof Class) {
@@ -276,19 +277,19 @@ final class SandboxInterceptor extends GroovyInterceptor {
         String getter = "get" + MetaClassHelper.capitalize(property);
         final Method getterMethod = GroovyCallSiteSelector.method(receiver, getter, noArgs);
         if (getterMethod != null) {
-            if (whitelist.permitsMethod(getterMethod, receiver, noArgs)) {
+            if (permitsMethod(whitelist, getterMethod, receiver, noArgs)) {
                 return super.onGetProperty(invoker, receiver, property);
             } else if (rejector == null) {
-                rejector = () -> StaticWhitelist.rejectMethod(getterMethod);
+                rejector = () -> rejectMethod(getterMethod);
             }
         }
         String booleanGetter = "is" + MetaClassHelper.capitalize(property);
         final Method booleanGetterMethod = GroovyCallSiteSelector.method(receiver, booleanGetter, noArgs);
         if (booleanGetterMethod != null && booleanGetterMethod.getReturnType() == boolean.class) {
-            if (whitelist.permitsMethod(booleanGetterMethod, receiver, noArgs)) {
+            if (permitsMethod(whitelist, booleanGetterMethod, receiver, noArgs)) {
                 return super.onGetProperty(invoker, receiver, property);
             } else if (rejector == null) {
-                rejector = () -> StaticWhitelist.rejectMethod(booleanGetterMethod);
+                rejector = () -> rejectMethod(booleanGetterMethod);
             }
         }
         // look for GDK methods
@@ -311,12 +312,12 @@ final class SandboxInterceptor extends GroovyInterceptor {
                 }
             }
         }
-        final Field instanceField = GroovyCallSiteSelector.field(receiver, property);
-        if (instanceField != null) {
-            if (whitelist.permitsFieldGet(instanceField, receiver)) {
+        final Field field = GroovyCallSiteSelector.field(receiver, property);
+        if (field != null) {
+            if (permitsFieldGet(whitelist, field, receiver)) {
                 return super.onGetProperty(invoker, receiver, property);
             } else if (rejector == null) {
-                rejector = () -> StaticWhitelist.rejectField(instanceField);
+                rejector = () -> rejectField(field);
             }
         }
         // GroovyObject property access
@@ -391,10 +392,10 @@ private interface Rejector {
         Rejector rejector = null;
         Field field = GroovyCallSiteSelector.field(receiver, attribute);
         if (field != null) {
-            if (whitelist.permitsFieldGet(field, receiver)) {
+            if (permitsFieldGet(whitelist, field, receiver)) {
                 return super.onGetAttribute(invoker, receiver, attribute);
             } else {
-                rejector = () -> StaticWhitelist.rejectField(field);
+                rejector = () -> rejectField(field);
             }
         }
         if (receiver instanceof Class) {
@@ -414,11 +415,11 @@ private interface Rejector {
         Rejector rejector = null;
         Field field = GroovyCallSiteSelector.field(receiver, attribute);
         if (field != null) {
-            if (whitelist.permitsFieldSet(field, receiver, value)) {
+            if (permitsFieldSet(whitelist, field, receiver, value)) {
                 Checker.preCheckedCast(field.getType(), value, false, false, false);
                 return super.onSetAttribute(invoker, receiver, attribute, value);
             } else {
-                rejector = () -> StaticWhitelist.rejectField(field);
+                rejector = () -> rejectField(field);
             }
         }
         if (receiver instanceof Class) {
@@ -442,10 +443,10 @@ private interface Rejector {
         Object[] args = new Object[] {index};
         Method method = GroovyCallSiteSelector.method(receiver, "getAt", args);
         if (method != null) {
-            if (whitelist.permitsMethod(method, receiver, args)) {
+            if (permitsMethod(whitelist, method, receiver, args)) {
                 return super.onGetArray(invoker, receiver, index);
             } else {
-                throw StaticWhitelist.rejectMethod(method);
+                throw rejectMethod(method);
             }
         }
         args = new Object[] {receiver, index};
@@ -469,10 +470,10 @@ private interface Rejector {
         Object[] args = new Object[] {index, value};
         Method method = GroovyCallSiteSelector.method(receiver, "putAt", args);
         if (method != null) {
-            if (whitelist.permitsMethod(method, receiver, args)) {
+            if (permitsMethod(whitelist, method, receiver, args)) {
                 return super.onSetArray(invoker, receiver, index, value);
             } else {
-                throw StaticWhitelist.rejectMethod(method);
+                throw rejectMethod(method);
             }
         }
         args = new Object[] {receiver, index, value};
@@ -546,4 +547,39 @@ private static String printArgumentTypes(Object[] args) {
         }
     }
 
+    private static boolean permitsFieldGet(@NonNull Whitelist whitelist, @NonNull Field field, @NonNull Object receiver) {
+        if (Modifier.isStatic(field.getModifiers())) {
+            return whitelist.permitsStaticFieldGet(field);
+        }
+        return whitelist.permitsFieldGet(field, receiver);
+    }
+
+    private static boolean permitsFieldSet(@NonNull Whitelist whitelist, @NonNull Field field, @NonNull Object receiver, @CheckForNull Object value) {
+        if (Modifier.isStatic(field.getModifiers())) {
+            return whitelist.permitsStaticFieldSet(field, value);
+        }
+        return whitelist.permitsFieldSet(field, receiver, value);
+    }
+
+    private static boolean permitsMethod(@NonNull Whitelist whitelist, @NonNull Method method, @NonNull Object receiver, @NonNull Object[] args) {
+        if (Modifier.isStatic(method.getModifiers())) {
+            return whitelist.permitsStaticMethod(method, args);
+        }
+        return whitelist.permitsMethod(method, receiver, args);
+    }
+
+    public static RejectedAccessException rejectMethod(@NonNull Method m) {
+        if (Modifier.isStatic(m.getModifiers())) {
+            return StaticWhitelist.rejectStaticMethod(m);
+        }
+        return StaticWhitelist.rejectMethod(m);
+    }
+
+    public static RejectedAccessException rejectField(@NonNull Field f) {
+        if (Modifier.isStatic(f.getModifiers())) {
+            return StaticWhitelist.rejectStaticField(f);
+        }
+        return StaticWhitelist.rejectField(f);
+    }
+
 }
diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/EnumeratingWhitelist.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/EnumeratingWhitelist.java
@@ -424,8 +424,7 @@ boolean matches(Field f) {
         }
         @Override boolean exists() throws Exception {
             try {
-                type(type).getField(field);
-                return true;
+                return !Modifier.isStatic(type(type).getField(field).getModifiers());
             } catch (NoSuchFieldException x) {
                 return false;
             }
@@ -444,6 +443,13 @@ static class StaticFieldSignature extends FieldSignature {
         @Override public String toString() {
             return "staticField " + signaturePart();
         }
+        @Override boolean exists() throws Exception {
+            try {
+                return Modifier.isStatic(type(type).getField(field).getModifiers());
+            } catch (NoSuchFieldException x) {
+                return false;
+            }
+        }
     }
 
 }
diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/jenkins-whitelist b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/jenkins-whitelist
@@ -28,9 +28,9 @@ method hudson.scm.ChangeLogSet$Entry getMsg
 method hudson.scm.ChangeLogSet$Entry getMsgAnnotated
 method hudson.scm.ChangeLogSet$Entry getMsgEscaped
 method hudson.scm.ChangeLogSet$Entry getTimestamp
-field hudson.scm.EditType ADD
-field hudson.scm.EditType DELETE
-field hudson.scm.EditType EDIT
+staticField hudson.scm.EditType ADD
+staticField hudson.scm.EditType DELETE
+staticField hudson.scm.EditType EDIT
 method hudson.scm.EditType getName
 method hudson.tools.ToolInstallation getHome
 method hudson.tools.ToolInstallation getName
diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
@@ -52,6 +52,7 @@
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Properties;
@@ -1654,6 +1655,54 @@ public void staticAttributesAreNotShadowedByClassFields() throws Throwable {
         assertEvaluate(new GenericWhitelist(), "foo", "class MyClass { static String name }; MyClass.@name = 'foo'");
     }
 
+    @Issue("JENKINS-42214")
+    @Test
+    public void accessStaticMembersViaInstance() throws Throwable {
+        String fqcn = HasStaticMembers.class.getName();
+        // Make sure that we report the correct signature in RejectedAccessException.
+        assertRejected(new AnnotatedWhitelist(), "staticField " + fqcn + " FOO", "def o = new " + fqcn + "(); o.FOO");
+        assertRejected(new AnnotatedWhitelist(), "staticMethod " + fqcn + " getBAR", "def o = new " + fqcn + "(); o.BAR");
+        assertRejected(new AnnotatedWhitelist(), "staticMethod " + fqcn + " isBAZ", "def o = new " + fqcn + "(); o.BAZ");
+        assertRejected(new AnnotatedWhitelist(), "staticField " + fqcn + " FOO", "def o = new " + fqcn + "(); o.FOO = 1");
+        assertRejected(new AnnotatedWhitelist(), "staticMethod " + fqcn + " setBAR int", "def o = new " + fqcn + "(); o.BAR = 2");
+        assertRejected(new AnnotatedWhitelist(), "staticField " + fqcn + " FOO", "def o = new " + fqcn + "(); o.@FOO");
+        assertRejected(new AnnotatedWhitelist(), "staticField " + fqcn + " FOO", "def o = new " + fqcn + "(); o.@FOO = 1");
+        assertRejected(new AnnotatedWhitelist(), "staticMethod " + fqcn + " method", "def o = new " + fqcn + "(); o.method()");
+        assertRejected(new AnnotatedWhitelist(), "staticMethod " + fqcn + " getAt int", "def o = new " + fqcn + "(); o[0]");
+        assertRejected(new AnnotatedWhitelist(), "staticMethod " + fqcn + " putAt int java.lang.Object", "def o = new " + fqcn + "(); o[0] = null");
+        // Make sure that we check for the correct signature in allowlists.
+        assertEvaluate(HasStaticMembers.allowlist("staticField " + fqcn + " FOO"), 1,
+                "def o = new " + fqcn + "(); o.FOO = o.FOO");
+        assertEvaluate(HasStaticMembers.allowlist("staticField " + fqcn + " FOO"), 1,
+                "def o = new " + fqcn + "(); o.@FOO = o.@FOO");
+        assertEvaluate(HasStaticMembers.allowlist("staticMethod " + fqcn + " getBAR", "staticMethod " + fqcn + " setBAR int"), 2,
+                "def o = new " + fqcn + "(); o.BAR = o.BAR");
+        assertEvaluate(HasStaticMembers.allowlist("staticMethod " + fqcn + " isBAZ"), true,
+                "def o = new " + fqcn + "(); o.BAZ");
+        assertEvaluate(HasStaticMembers.allowlist("staticMethod " + fqcn + " method"), 3,
+                "def o = new " + fqcn + "(); o.method()");
+        assertEvaluate(HasStaticMembers.allowlist("staticMethod " + fqcn + " getAt int", "staticMethod " + fqcn + " putAt int java.lang.Object"), 3,
+                "def o = new " + fqcn + "(); o[0] = o[3]");
+    }
+
+    public static class HasStaticMembers {
+        @Whitelisted public HasStaticMembers() { }
+        public static int FOO = 1; // Groovy will access the field directly
+        public static int BAR = 2; // Groovy will access the field via getBAR and setBAR
+        public static int getBAR() { return BAR; }
+        public static void setBAR(int BAR) { HasStaticMembers.BAR = BAR; }
+        public static boolean BAZ = true; // Groovy will read the field via isBAZ
+        public static boolean isBAZ() { return BAZ; }
+        public static int method() { return 3; }
+        public static int getAt(int index) { return index; }
+        public static void putAt(int index, Object value) { }
+        public static Whitelist allowlist(String... signatures) throws Exception {
+            List<String> signaturesList = new ArrayList<>(Arrays.asList(signatures));
+            signaturesList.add("new " + HasStaticMembers.class.getName());
+            return new StaticWhitelist(signaturesList);
+        }
+    }
+
     /**
      * Checks that the annotation is blocked from being used in the provided script whether it is imported or used via
      * fully-qualified class name.
diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/EnumeratingWhitelistTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/EnumeratingWhitelistTest.java
@@ -33,6 +33,7 @@
 
 import org.junit.Assert;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import static org.junit.Assert.*;
 
 public class EnumeratingWhitelistTest {
@@ -197,4 +198,13 @@ public void testCachingWithWildcards() throws Exception {
         assertEquals(Boolean.TRUE, myList.permittedCache.get(EnumeratingWhitelist.canonicalFieldSig(f)));  // Verifies it can cache that
     }
 
+    @Issue("JENKINS-42214")
+    @Test public void fieldExists() throws Exception {
+        assertTrue(new EnumeratingWhitelist.FieldSignature("hudson.model.Result", "color").exists());
+        assertTrue(new EnumeratingWhitelist.StaticFieldSignature("hudson.model.Result", "ABORTED").exists());
+
+        assertFalse(new EnumeratingWhitelist.StaticFieldSignature("hudson.model.Result", "color").exists());
+        assertFalse(new EnumeratingWhitelist.FieldSignature("hudson.model.Result", "ABORTED").exists());
+    }
+
 }

Original file line number	Diff line number	Diff line change
`@@ -424,8 +424,7 @@ boolean matches(Field f) {`
`424`	`424`	`}`
`425`	`425`	`@Override boolean exists() throws Exception {`
`426`	`426`	`try {`
`427`		`- type(type).getField(field);`
`428`		`- return true;`
	`427`	`+ return !Modifier.isStatic(type(type).getField(field).getModifiers());`
`429`	`428`	`} catch (NoSuchFieldException x) {`
`430`	`429`	`return false;`
`431`	`430`	`}`
`@@ -444,6 +443,13 @@ static class StaticFieldSignature extends FieldSignature {`
`444`	`443`	`@Override public String toString() {`
`445`	`444`	`return "staticField " + signaturePart();`
`446`	`445`	`}`
	`446`	`+ @Override boolean exists() throws Exception {`
	`447`	`+ try {`
	`448`	`+ return Modifier.isStatic(type(type).getField(field).getModifiers());`
	`449`	`+ } catch (NoSuchFieldException x) {`
	`450`	`+ return false;`
	`451`	`+ }`
	`452`	`+ }`
`447`	`453`	`}`
`448`	`454`
`449`	`455`	`}`