Skip to content

Commit 1af77ff

Browse files
dwnusbaumdwnusbaum
committed
[SECURITY-2824]
1 parent 5ea6281 commit 1af77ff

File tree

7 files changed

+185
-13
lines changed

7 files changed

+185
-13
lines changed

‎pom.xml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@
6666
<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
6767
<jenkins.version>2.346.1</jenkins.version>
6868
<no-test-jar>false</no-test-jar>
69-
<groovy-cps.version>1.32</groovy-cps.version>
69+
<groovy-cps.version>1.34</groovy-cps.version>
7070
<node.version>16.17.0</node.version>
7171
<npm.version>8.18.0</npm.version>
7272
</properties>
@@ -107,6 +107,7 @@
107107
<dependency>
108108
<groupId>org.jenkins-ci.plugins</groupId>
109109
<artifactId>script-security</artifactId>
110+
<version>1184.v85d16b_d851b_3</version>
110111
</dependency>
111112
<dependency>
112113
<groupId>org.jenkins-ci.plugins</groupId>

‎src/main/java/org/jenkinsci/plugins/workflow/cps/CpsGroovyShell.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,10 @@ private Script doParse(GroovyCodeSource codeSource) throws CompilationFailedExce
139139
try (GroovySandbox.Scope scope = sandbox.enter()) {
140140
if (execution != null) {
141141
try (CpsFlowExecution.Timing t = execution.time(CpsFlowExecution.TimingKind.parse)) {
142-
return super.parse(codeSource);
142+
return scope.parse(CpsGroovyShell.this, codeSource);
143143
}
144144
} else {
145-
return super.parse(codeSource);
145+
return scope.parse(CpsGroovyShell.this, codeSource);
146146
}
147147
}
148148
}

‎src/main/java/org/jenkinsci/plugins/workflow/cps/CpsScript.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,11 @@ public Object evaluate(File file) throws CompilationFailedException, IOException
190190

191191
@Override
192192
public void run(File file, String[] arguments) throws CompilationFailedException, IOException {
193-
$getShell().run(file,arguments);
193+
// GroovyShell.run has a bunch of weird cases related to JUnit and other stuff that we cannot safely support
194+
// without a lot of extra work, so we just approximate its behavior. Regardless, I assume that this method is
195+
// essentially unused since it takes a File and it is not allowed by CpsWhitelist (unlike evaluate).
196+
$getShell().getContext().setProperty("args", arguments);
197+
evaluate(file);
194198
}
195199

196200
/**

‎src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java‎

Lines changed: 58 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import java.lang.reflect.Constructor;
55
import java.lang.reflect.Field;
66
import java.lang.reflect.Method;
7+
import java.lang.reflect.Modifier;
78
import java.util.Arrays;
89
import java.util.Collection;
910
import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
@@ -41,35 +42,85 @@ private boolean permits(Class<?> declaringClass) {
4142
}
4243

4344
@Override public boolean permitsMethod(Method method, Object receiver, Object[] args) {
44-
return permits(method.getDeclaringClass()) || delegate.permitsMethod(method, receiver, args);
45+
return permits(method.getDeclaringClass()) && !isIllegalSyntheticMethod(method) || delegate.permitsMethod(method, receiver, args);
4546
}
4647

4748
@Override public boolean permitsConstructor(Constructor<?> constructor, Object[] args) {
48-
return permits(constructor.getDeclaringClass()) || delegate.permitsConstructor(constructor, args);
49+
return permits(constructor.getDeclaringClass()) && !isIllegalSyntheticConstructor(constructor) || delegate.permitsConstructor(constructor, args);
4950
}
5051

5152
@Override public boolean permitsStaticMethod(Method method, Object[] args) {
52-
return permits(method.getDeclaringClass()) || delegate.permitsStaticMethod(method, args);
53+
return permits(method.getDeclaringClass()) && !isIllegalSyntheticMethod(method) || delegate.permitsStaticMethod(method, args);
5354
}
5455

5556
@Override public boolean permitsFieldGet(Field field, Object receiver) {
56-
return permits(field.getDeclaringClass()) || delegate.permitsFieldGet(field, receiver);
57+
return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field) || delegate.permitsFieldGet(field, receiver);
5758
}
5859

5960
@Override public boolean permitsFieldSet(Field field, Object receiver, Object value) {
60-
return permits(field.getDeclaringClass()) || delegate.permitsFieldSet(field, receiver, value);
61+
return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field) || delegate.permitsFieldSet(field, receiver, value);
6162
}
6263

6364
@Override public boolean permitsStaticFieldGet(Field field) {
64-
return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldGet(field);
65+
return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field) || delegate.permitsStaticFieldGet(field);
6566
}
6667

6768
@Override public boolean permitsStaticFieldSet(Field field, Object value) {
68-
return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldSet(field, value);
69+
return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field) || delegate.permitsStaticFieldSet(field, value);
6970
}
7071

7172
@Override public String toString() {
7273
return super.toString() + "[" + delegate + "]";
7374
}
7475

76+
/**
77+
* Checks whether a given field was created by the Groovy compiler and should be inaccessible even if it is
78+
* declared by a class defined by one of the specified class loaders.
79+
*/
80+
private static boolean isIllegalSyntheticField(Field field) {
81+
if (!field.isSynthetic()) {
82+
return false;
83+
}
84+
Class<?> declaringClass = field.getDeclaringClass();
85+
Class<?> enclosingClass = declaringClass.getEnclosingClass();
86+
if (field.getType() == enclosingClass && field.getName().startsWith("this$")) {
87+
// Synthetic field added to inner classes to reference the outer class.
88+
return false;
89+
} else if (declaringClass.isEnum() && Modifier.isStatic(field.getModifiers()) && field.getName().equals("$VALUES")) {
90+
// Synthetic field added to enum classes to hold the enum constants.
91+
return false;
92+
}
93+
return true;
94+
}
95+
96+
/**
97+
* Checks whether a given method was created by the Groovy compiler and should be inaccessible even if it is
98+
* declared by a class defined by one of the specified class loaders.
99+
*/
100+
private static boolean isIllegalSyntheticMethod(Method method) {
101+
if (!method.isSynthetic()) {
102+
return false;
103+
} else if (Modifier.isStatic(method.getModifiers()) && method.getDeclaringClass().isEnum() && method.getName().equals("$INIT")) {
104+
// Synthetic method added to enum classes used to initialize the enum constants.
105+
return false;
106+
}
107+
return true;
108+
}
109+
110+
/**
111+
* Checks whether a given constructor was created by the Groovy compiler (or groovy-sandbox) and
112+
* should be inaccessible even if it is declared by a class defined by the specified class loader.
113+
*/
114+
private static boolean isIllegalSyntheticConstructor(Constructor constructor) {
115+
if (!constructor.isSynthetic()) {
116+
return false;
117+
}
118+
Class<?> declaringClass = constructor.getDeclaringClass();
119+
Class<?> enclosingClass = declaringClass.getEnclosingClass();
120+
if (enclosingClass != null && constructor.getParameters().length > 0 && constructor.getParameterTypes()[0] == enclosingClass) {
121+
// Synthetic constructor added by Groovy to anonymous classes.
122+
return false;
123+
}
124+
return true;
125+
}
75126
}

‎src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java‎

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import java.io.Serializable;
3434
import java.util.Collections;
3535
import java.util.Set;
36-
3736
import java.util.logging.Level;
3837

3938
import jenkins.model.Jenkins;
@@ -257,7 +256,7 @@ public void traitsSandbox() throws Exception {
257256
WorkflowRun b = job.scheduleBuild2(0).get();
258257
assertNull(jenkins.jenkins.getSystemMessage());
259258
jenkins.assertBuildStatus(Result.FAILURE, b);
260-
jenkins.assertLogContains("org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use staticMethod jenkins.model.Jenkins getInstance", b);
259+
jenkins.assertLogContains("org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object (org.jenkinsci.plugins.workflow.cps.CpsClosure2 getInstance)", b);
261260
return null;
262261
});
263262
// Some safe idioms:
@@ -853,6 +852,81 @@ public void scriptInitializerCallsCpsTransformedMethod() throws Exception {
853852
assertNull(Jenkins.get().getDescription());
854853
}
855854

855+
@Issue("SECURITY-2824")
856+
@Test public void blockCastsPropertiesAndAttributes() throws Exception {
857+
// Instance property
858+
WorkflowJob p = jenkins.createProject(WorkflowJob.class);
859+
p.setDefinition(new CpsFlowDefinition(
860+
"class Test {\n" +
861+
" File file\n" +
862+
"}\n" +
863+
"def t = new Test()\n" +
864+
"t.file = ['secret.key']\n", true));
865+
WorkflowRun b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
866+
jenkins.assertLogContains("Scripts not permitted to use new java.io.File java.lang.String", b);
867+
// Static property
868+
p.setDefinition(new CpsFlowDefinition(
869+
"class Test {\n" +
870+
" static File file\n" +
871+
"}\n" +
872+
"Test.file = ['secret.key']\n", true));
873+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
874+
jenkins.assertLogContains("Scripts not permitted to use new java.io.File java.lang.String", b);
875+
// Instance attribute
876+
p.setDefinition(new CpsFlowDefinition(
877+
"class Test {\n" +
878+
" File file\n" +
879+
"}\n" +
880+
"def t = new Test()\n" +
881+
"t.@file = ['secret.key']\n", true));
882+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
883+
jenkins.assertLogContains("Scripts not permitted to use new java.io.File java.lang.String", b);
884+
// Static attribute
885+
p.setDefinition(new CpsFlowDefinition(
886+
"class Test {\n" +
887+
" static File file\n" +
888+
"}\n" +
889+
"Test.@file = ['secret.key']\n", true));
890+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
891+
jenkins.assertLogContains("Scripts not permitted to use new java.io.File java.lang.String", b);
892+
}
893+
894+
@Issue("JENKINS-33023")
895+
@Test public void groovyEnums() throws Exception {
896+
WorkflowJob p = jenkins.createProject(WorkflowJob.class);
897+
p.setDefinition(new CpsFlowDefinition(
898+
"enum Thing {\n" +
899+
" ONE, TWO\n" +
900+
" Thing() { }\n" +
901+
"}\n" +
902+
"Thing.ONE\n", true));
903+
WorkflowRun b = jenkins.buildAndAssertSuccess(p);
904+
p.setDefinition(new CpsFlowDefinition(
905+
"enum Thing {\n" +
906+
" ONE, TWO\n" +
907+
"}\n" +
908+
"Thing.ONE\n", true));
909+
// Seems undesirable, but this is the current behavior. Requires new java.util.LinkedHashMap and staticMethod ImmutableASTTransformation checkPropNames.
910+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
911+
jenkins.assertLogContains("Scripts not permitted to use new java.util.LinkedHashMap", b);
912+
}
913+
914+
@Test public void blockSyntheticFieldsAndMethods() throws Throwable {
915+
WorkflowJob p = jenkins.createProject(WorkflowJob.class);
916+
p.setDefinition(new CpsFlowDefinition("$getStaticMetaClass()", true));
917+
WorkflowRun b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
918+
jenkins.assertLogContains("Scripts not permitted to use method WorkflowScript $getStaticMetaClass", b);
919+
p.setDefinition(new CpsFlowDefinition("getClass().$getCallSiteArray()", true));
920+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
921+
jenkins.assertLogContains("Scripts not permitted to use staticMethod WorkflowScript $getCallSiteArray", b);
922+
p.setDefinition(new CpsFlowDefinition("class Test { }; new Test().metaClass", true));
923+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
924+
jenkins.assertLogContains("Scripts not permitted to use method groovy.lang.GroovyObject getMetaClass", b);
925+
p.setDefinition(new CpsFlowDefinition("class Test { }; new Test().@metaClass", true));
926+
b = jenkins.buildAndAssertStatus(Result.FAILURE, p);
927+
jenkins.assertLogContains("Scripts not permitted to use field Test metaClass", b);
928+
}
929+
856930
public static class UnsafeParameterStep extends Step implements Serializable {
857931
private final UnsafeDescribable val;
858932
@DataBoundConstructor

‎src/test/java/org/jenkinsci/plugins/workflow/cps/CpsScriptTest.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,20 @@
11
package org.jenkinsci.plugins.workflow.cps;
22

33
import hudson.model.Result;
4+
import java.util.concurrent.atomic.AtomicInteger;
5+
import java.util.function.BiFunction;
46
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
57
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
68
import org.junit.ClassRule;
79

810
import org.junit.Test;
11+
import org.jvnet.hudson.test.BuildWatcher;
912
import org.jvnet.hudson.test.Issue;
1013
import org.jvnet.hudson.test.JenkinsRule;
1114

1215
public class CpsScriptTest {
1316

17+
@ClassRule public static BuildWatcher watcher = new BuildWatcher();
1418
@ClassRule public static JenkinsRule r = new JenkinsRule();
1519

1620
/**
@@ -50,4 +54,34 @@ public void evaluateShallSandbox() throws Exception {
5054
r.buildAndAssertSuccess(p);
5155
}
5256

57+
@Issue("SECURITY-2428")
58+
@Test public void blockImplicitCastingInEvaluate() throws Exception {
59+
AtomicInteger counter = new AtomicInteger();
60+
BiFunction<String, String, String> embeddedScript = (decl, main) -> "" +
61+
"class Test" + counter.incrementAndGet() + " {\\n" +
62+
" " + decl + "\\n" +
63+
" Object map\\n" +
64+
" @NonCPS public void main(String[] args) { " + main + " }\\n" +
65+
"}\\n";
66+
WorkflowJob p = r.createProject(WorkflowJob.class);
67+
p.setDefinition(new CpsFlowDefinition(
68+
"list = ['secret.key']\n" +
69+
"map = [:]\n" +
70+
"evaluate('" + embeddedScript.apply("File list", "map.file = list") + "')\n" +
71+
"file = map.file\n" +
72+
"evaluate('" + embeddedScript.apply("String[] file", "map.lines = file") + "')\n" +
73+
"for (String line in map.lines) { echo(line) }\n", true));
74+
WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
75+
r.assertLogContains("Scripts not permitted to use new java.io.File java.lang.String", b);
76+
}
77+
78+
@Test public void blockRun() throws Exception {
79+
WorkflowJob p = r.createProject(WorkflowJob.class);
80+
p.setDefinition(new CpsFlowDefinition("run(null, ['test'] as String[])\n", true));
81+
WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
82+
// The existence of CpsScript.run leads me to believe that it was intended to be allowed by CpsWhitelist, but
83+
// that is not currently the case, and I see no reason to start allowing it at this point.
84+
r.assertLogContains("Scripts not permitted to use method groovy.lang.Script run java.io.File java.lang.String[]", b);
85+
}
86+
5387
}

‎src/test/java/org/jenkinsci/plugins/workflow/cps/CpsVmExecutorServiceTest.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,4 +204,12 @@ public class CpsVmExecutorServiceTest {
204204
r.assertLogNotContains(CpsVmExecutorService.mismatchMessage("java.util.LinkedHashMap", "action", "org.jenkinsci.plugins.workflow.cps.CpsClosure2", "call"), b);
205205
}
206206

207+
@Test public void wrongCatcherAsBoolean() throws Exception {
208+
p.setDefinition(new CpsFlowDefinition("class C { def asBoolean() { 'never used' } }; if (new C()) { println('casted') } else { println('see what') }", true));
209+
WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
210+
r.assertLogContains(CpsVmExecutorService.mismatchMessage("org.codehaus.groovy.runtime.ScriptBytecodeAdapter", "castToType", "C", "asBoolean"), b);
211+
r.assertLogContains("java.lang.IllegalStateException: C.asBoolean must be @NonCPS; see: https://jenkins.io/redirect/pipeline-cps-method-mismatches/", b);
212+
r.assertLogNotContains("see what", b);
213+
}
214+
207215
}

0 commit comments

Comments
 (0)