Skip to content

Commit 9c41a16

Browse files
dwnusbaumdwnusbaum
committed
[SECURITY-2824]
1 parent 84da9c5 commit 9c41a16

File tree

3 files changed

+143
-6
lines changed

3 files changed

+143
-6
lines changed

‎pom.xml

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,16 @@
6363
</pluginRepositories>
6464
<properties>
6565
<changelist>999999-SNAPSHOT</changelist>
66-
<jenkins.version>2.289.3</jenkins.version>
66+
<jenkins.version>2.346.1</jenkins.version>
6767
<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
68+
<workflow-cps-plugin.version>2803.v1a_f77ffcc773</workflow-cps-plugin.version>
6869
</properties>
6970
<dependencyManagement>
7071
<dependencies>
7172
<dependency>
7273
<groupId>io.jenkins.tools.bom</groupId>
73-
<artifactId>bom-2.289.x</artifactId>
74-
<version>1500.ve4d05cd32975</version>
74+
<artifactId>bom-2.346.x</artifactId>
75+
<version>1607.va_c1576527071</version>
7576
<scope>import</scope>
7677
<type>pom</type>
7778
</dependency>
@@ -101,6 +102,7 @@
101102
<dependency>
102103
<groupId>org.jenkins-ci.plugins.workflow</groupId>
103104
<artifactId>workflow-cps</artifactId>
105+
<version>${workflow-cps-plugin.version}</version>
104106
</dependency>
105107
<dependency>
106108
<groupId>org.jenkins-ci.plugins.workflow</groupId>
@@ -117,6 +119,7 @@
117119
<dependency>
118120
<groupId>org.jenkins-ci.plugins</groupId>
119121
<artifactId>script-security</artifactId>
122+
<version>1184.v85d16b_d851b_3</version>
120123
</dependency>
121124

122125
<dependency>
@@ -166,6 +169,7 @@
166169
<dependency>
167170
<groupId>org.jenkins-ci.plugins.workflow</groupId>
168171
<artifactId>workflow-cps</artifactId>
172+
<version>${workflow-cps-plugin.version}</version>
169173
<classifier>tests</classifier>
170174
<scope>test</scope>
171175
</dependency>

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryStep.java

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import java.util.logging.Logger;
6060
import edu.umd.cs.findbugs.annotations.CheckForNull;
6161
import edu.umd.cs.findbugs.annotations.NonNull;
62+
import groovy.lang.MissingPropertyException;
6263
import javax.inject.Inject;
6364
import jenkins.model.Jenkins;
6465
import jenkins.scm.impl.SingleSCMSource;
@@ -75,6 +76,8 @@
7576
import org.jenkinsci.plugins.workflow.steps.StepContextParameter;
7677
import org.kohsuke.accmod.Restricted;
7778
import org.kohsuke.accmod.restrictions.DoNotUse;
79+
import org.kohsuke.groovy.sandbox.GroovyInterceptor;
80+
import org.kohsuke.groovy.sandbox.impl.Checker;
7881
import org.kohsuke.stapler.AncestorInPath;
7982
import org.kohsuke.stapler.DataBoundConstructor;
8083
import org.kohsuke.stapler.DataBoundSetter;
@@ -270,11 +273,15 @@ public static final class LoadedClasses extends GroovyObjectSupport implements S
270273
if (clazz != null) {
271274
// Field access?
272275
try {
273-
// not doing a Whitelist check since GroovyClassLoaderWhitelist would be allowing it anyway
276+
if (isSandboxed()) {
277+
return Checker.checkedGetAttribute(loadClass(prefix + clazz), false, false, property);
278+
}
274279
return loadClass(prefix + clazz).getField(property).get(null);
275-
} catch (NoSuchFieldException x) {
280+
} catch (MissingPropertyException | NoSuchFieldException x) {
276281
// guessed wrong
277-
} catch (IllegalAccessException x) {
282+
} catch (SecurityException x) {
283+
throw x;
284+
} catch (Throwable x) {
278285
throw new GroovyRuntimeException(x);
279286
}
280287
}
@@ -284,6 +291,8 @@ public static final class LoadedClasses extends GroovyObjectSupport implements S
284291
loadClass(prefix + fullClazz);
285292
// OK, class really exists, stash it and await methods
286293
return new LoadedClasses(library, trusted, changelog, prefix, fullClazz, srcUrl);
294+
} else if (clazz != null) {
295+
throw new MissingPropertyException(property, loadClass(prefix + clazz));
287296
} else {
288297
// Still selecting package components.
289298
return new LoadedClasses(library, trusted, changelog, prefix + property + '.', null, srcUrl);
@@ -293,13 +302,43 @@ public static final class LoadedClasses extends GroovyObjectSupport implements S
293302
@Override public Object invokeMethod(String name, Object _args) {
294303
Class<?> c = loadClass(prefix + clazz);
295304
Object[] args = _args instanceof Object[] ? (Object[]) _args : new Object[] {_args}; // TODO why does Groovy not just pass an Object[] to begin with?!
305+
if (isSandboxed()) {
306+
try {
307+
if (name.equals("new")) {
308+
return Checker.checkedConstructor(c, args);
309+
} else {
310+
return Checker.checkedStaticCall(c, name, args);
311+
}
312+
} catch (SecurityException x) {
313+
throw x;
314+
} catch (Throwable x) {
315+
throw new GroovyRuntimeException(x);
316+
}
317+
}
296318
if (name.equals("new")) {
297319
return InvokerHelper.invokeConstructorOf(c, args);
298320
} else {
299321
return InvokerHelper.invokeStaticMethod(c, name, args);
300322
}
301323
}
302324

325+
/**
326+
* Check whether the current thread has at least one active {@link GroovyInterceptor}.
327+
* <p>
328+
* Typically, {@code GroovyClassLoaderWhitelist} will allow access to everything defined in a class in a
329+
* library, but there are some synthetic constructors, fields, and methods which should not be accessible.
330+
* <p>
331+
* As a result, when getting properties or invoking methods using this class, we need to apply sandbox
332+
* protection if the Pipeline code performing the operation is sandbox-transformed. Unfortunately, it is
333+
* difficult to detect that case specifically, so we instead intercept all calls if the Pipeline itself is
334+
* sandboxed. This results in a false positive {@code RejectedAccessException} being thrown if a trusted
335+
* library uses the {@code library} step and tries to access static fields or methods that are not permitted to
336+
* be used in the sandbox.
337+
*/
338+
private static boolean isSandboxed() {
339+
return !GroovyInterceptor.getApplicableInterceptors().isEmpty();
340+
}
341+
303342
// TODO putProperty for static field set
304343

305344
private Class<?> loadClass(String name) {

‎src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryStepTest.java

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import org.junit.Test;
5252
import static org.junit.Assert.*;
5353
import org.junit.ClassRule;
54+
import org.junit.Ignore;
5455
import org.junit.Rule;
5556
import org.jvnet.hudson.test.BuildWatcher;
5657
import org.jvnet.hudson.test.Issue;
@@ -124,6 +125,99 @@ public class LibraryStepTest {
124125
r.assertLogContains("using constant vs. constant", b);
125126
}
126127

128+
@Test public void missingProperty() throws Exception {
129+
sampleRepo.init();
130+
sampleRepo.write("src/some/pkg/MyClass.groovy", "package some.pkg; class MyClass { }");
131+
sampleRepo.git("add", "src");
132+
sampleRepo.git("commit", "--message=init");
133+
Folder f = r.jenkins.createProject(Folder.class, "f");
134+
f.getProperties().add(new FolderLibraries(Collections.singletonList(new LibraryConfiguration("stuff", new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true))))));
135+
WorkflowJob p = f.createProject(WorkflowJob.class, "p");
136+
p.setDefinition(new CpsFlowDefinition(
137+
"def lib = library 'stuff@master'\n" +
138+
"lib.some.pkg.MyClass.no_field_with_this_name\n" , true));
139+
WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
140+
r.assertLogContains("MissingPropertyException: No such property: no_field_with_this_name for class: some.pkg.MyClass", b);
141+
}
142+
143+
@Test public void reflectionInLoadedClassesIsIntercepted() throws Exception {
144+
sampleRepo.init();
145+
sampleRepo.write("src/some/pkg/MyThread.groovy", "package some.pkg; class MyThread extends Thread { }");
146+
sampleRepo.git("add", "src");
147+
sampleRepo.git("commit", "--message=init");
148+
Folder f = r.jenkins.createProject(Folder.class, "f");
149+
f.getProperties().add(new FolderLibraries(Collections.singletonList(new LibraryConfiguration("stuff", new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true))))));
150+
WorkflowJob p = f.createProject(WorkflowJob.class, "p");
151+
p.setDefinition(new CpsFlowDefinition(
152+
"def lib = library 'stuff@master'\n" +
153+
"catchError() { lib.some.pkg.MyThread.new(null) }\n" +
154+
"catchError() { lib.some.pkg.MyThread.__$stMC }\n" +
155+
"catchError() { lib.some.pkg.MyThread.$getCallSiteArray() }\n" , true));
156+
WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
157+
r.assertLogContains("Rejecting illegal call to synthetic constructor", b);
158+
r.assertLogContains("staticField some.pkg.MyThread __$stMC", b);
159+
r.assertLogContains("staticMethod some.pkg.MyThread $getCallSiteArray", b);
160+
}
161+
162+
@Issue("SECURITY-2824")
163+
@Test public void constructorInvocationInLoadedClassesIsIntercepted() throws Exception {
164+
sampleRepo.init();
165+
sampleRepo.write("src/pkg/Superclass.groovy",
166+
"package pkg;\n" +
167+
"class Superclass { Superclass(String x) { } }\n");
168+
sampleRepo.write("src/pkg/Subclass.groovy",
169+
"package pkg;\n" +
170+
"class Subclass extends Superclass {\n" +
171+
" def wrapper\n" +
172+
" Subclass() { super('secret.key'); def $cw = $cw; wrapper = $cw }\n" +
173+
"}\n");
174+
sampleRepo.write("src/pkg/MyFile.groovy",
175+
"package pkg;\n" +
176+
"class MyFile extends File {\n" +
177+
" MyFile(String path) {\n" +
178+
" super(path)\n" +
179+
" }\n" +
180+
"}\n");
181+
sampleRepo.git("add", "src");
182+
sampleRepo.git("commit", "--message=init");
183+
Folder f = r.jenkins.createProject(Folder.class, "f");
184+
f.getProperties().add(new FolderLibraries(Collections.singletonList(new LibraryConfiguration("stuff", new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true))))));
185+
WorkflowJob p = f.createProject(WorkflowJob.class, "p");
186+
p.setDefinition(new CpsFlowDefinition(
187+
"def lib = library 'stuff@master'\n" +
188+
"def wrapper = lib.pkg.Subclass.new().wrapper\n" +
189+
"def file = lib.pkg.MyFile.new(wrapper, 'unused')\n" +
190+
"echo(/${[file, file.class]}/)", true));
191+
WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
192+
r.assertLogContains("Rejecting illegal call to synthetic constructor: private pkg.MyFile", b);
193+
}
194+
195+
@Ignore("Trusted libraries should never get a RejectedAccessException, but this case should be uncommon and is difficult to handle more precisely")
196+
@Test public void falsePositiveRejectedAccessExceptionInTrustedLibrary() throws Exception {
197+
sampleRepo.init();
198+
sampleRepo.git("branch", "myBranch");
199+
sampleRepo.write("vars/doStuff.groovy",
200+
"def call() {\n" +
201+
" def lib = library('stuff2@myBranch')\n" +
202+
" lib.some.pkg.MyClass.$getCallSiteArray()\n" +
203+
"}\n");
204+
sampleRepo.git("add", ".");
205+
sampleRepo.git("commit", "--message=init");
206+
sampleRepo.git("checkout", "myBranch");
207+
sampleRepo.write("src/some/pkg/MyClass.groovy", "package some.pkg; class MyClass { }");
208+
sampleRepo.git("add", ".");
209+
sampleRepo.git("commit", "--message=myBranch");
210+
GlobalLibraries.get().setLibraries(Arrays.asList(
211+
new LibraryConfiguration("stuff", new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true))),
212+
new LibraryConfiguration("stuff2", new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true)))));
213+
WorkflowJob p = r.createProject(WorkflowJob.class, "p");
214+
p.setDefinition(new CpsFlowDefinition(
215+
"@Library('stuff@master')\n" +
216+
"import doStuff\n" +
217+
"doStuff()\n", true));
218+
WorkflowRun b = r.buildAndAssertSuccess(p);
219+
}
220+
127221
@Test public void classesFromWrongPlace() throws Exception {
128222
sampleRepo.init();
129223
sampleRepo.write("src/some/pkg/Lib.groovy", "package some.pkg; class Lib {static void m() {}}");

0 commit comments

Comments
 (0)