Skip to content

Commit 71c3f0a

Browse files
dwnusbaumdwnusbaum
committed
[SECURITY-2463][SECURITY-2491]
1 parent d43c65d commit 71c3f0a

File tree

2 files changed

+216
-13
lines changed

2 files changed

+216
-13
lines changed

‎src/main/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStep.java

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@
3939
import hudson.model.TopLevelItem;
4040
import hudson.scm.SCM;
4141
import hudson.slaves.WorkspaceList;
42+
43+
import java.io.File;
4244
import java.io.IOException;
4345
import javax.inject.Inject;
4446
import jenkins.branch.Branch;
@@ -48,6 +50,7 @@
4850
import jenkins.scm.api.SCMRevision;
4951
import jenkins.scm.api.SCMRevisionAction;
5052
import jenkins.scm.api.SCMSource;
53+
import jenkins.security.HMACConfidentialKey;
5154
import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition;
5255
import org.jenkinsci.plugins.workflow.cps.steps.LoadStepExecution;
5356
import org.jenkinsci.plugins.workflow.flow.FlowDefinition;
@@ -69,6 +72,9 @@
6972
*/
7073
public class ReadTrustedStep extends AbstractStepImpl {
7174

75+
// Intentionally using the same key as CpsScmFlowDefinition.
76+
private static final HMACConfidentialKey CHECKOUT_DIR_KEY = new HMACConfidentialKey(CpsScmFlowDefinition.class, "filePathWithSuffix", 32);
77+
7278
private final String path;
7379
// TODO encoding
7480

@@ -122,31 +128,31 @@ public static class Execution extends AbstractSynchronousNonBlockingStepExecutio
122128
}
123129
}
124130
Node node = Jenkins.get();
125-
FilePath dir;
131+
FilePath baseWorkspace;
126132
if (job instanceof TopLevelItem) {
127-
FilePath baseWorkspace = node.getWorkspaceFor((TopLevelItem) job);
133+
baseWorkspace = node.getWorkspaceFor((TopLevelItem) job);
128134
if (baseWorkspace == null) {
129135
throw new AbortException(node.getDisplayName() + " may be offline");
130136
}
131-
dir = getFilePathWithSuffix(baseWorkspace);
132137
} else { // should not happen, but just in case:
133138
throw new IllegalStateException(job + " was not top level");
134139
}
135-
FilePath file = dir.child(step.path);
136-
if (!file.absolutize().getRemote().replace('\\', '/').startsWith(dir.absolutize().getRemote().replace('\\', '/') + '/')) { // TODO JENKINS-26838
137-
throw new IOException(file + " is not inside " + dir);
138-
}
139140
Computer computer = node.toComputer();
140141
if (computer == null) {
141142
throw new IOException(node.getDisplayName() + " may be offline");
142143
}
143144
if (standaloneSCM != null) {
145+
FilePath dir = getFilePathWithSuffix(baseWorkspace, standaloneSCM);
146+
FilePath file = dir.child(step.path);
144147
try (WorkspaceList.Lease lease = computer.getWorkspaceList().acquire(dir)) {
148+
dir.withSuffix("-scm-key.txt").write(standaloneSCM.getKey(), "UTF-8");
145149
SCMStep delegate = new GenericSCMStep(standaloneSCM);
146150
delegate.setPoll(true);
147151
delegate.setChangelog(true);
148152
delegate.checkout(build, dir, listener, node.createLauncher(listener));
149-
if (!file.exists()) {
153+
if (!isDescendant(file, dir)) {
154+
throw new AbortException(file + " references a file that is not inside " + dir);
155+
} else if (!file.exists()) {
150156
throw new AbortException(file + " not found");
151157
}
152158
return file.readToString();
@@ -187,22 +193,30 @@ public static class Execution extends AbstractSynchronousNonBlockingStepExecutio
187193
listener.getLogger().println("Obtained " + step.path + " from " + trusted);
188194
} else {
189195
listener.getLogger().println("Checking out " + head.getName() + " to read " + step.path);
196+
SCM trustedScm = scmSource.build(head, trusted);
197+
FilePath dir = getFilePathWithSuffix(baseWorkspace, trustedScm);
198+
FilePath file = dir.child(step.path);
190199
try (WorkspaceList.Lease lease = computer.getWorkspaceList().acquire(dir)) {
200+
dir.withSuffix("-scm-key.txt").write(trustedScm.getKey(), "UTF-8");
191201
if (trustCheck) {
192202
SCMStep delegate = new GenericSCMStep(scmSource.build(head, tip));
193203
delegate.setPoll(false);
194204
delegate.setChangelog(false);
195205
delegate.checkout(build, dir, listener, node.createLauncher(listener));
196-
if (!file.exists()) {
206+
if (!isDescendant(file, dir)) {
207+
throw new AbortException(file + " references a file that is not inside " + dir);
208+
} else if (!file.exists()) {
197209
throw new AbortException(file + " not found");
198210
}
199211
untrustedFile = file.readToString();
200212
}
201-
SCMStep delegate = new GenericSCMStep(scmSource.build(head, trusted));
213+
SCMStep delegate = new GenericSCMStep(trustedScm);
202214
delegate.setPoll(true);
203215
delegate.setChangelog(true);
204216
delegate.checkout(build, dir, listener, node.createLauncher(listener));
205-
if (!file.exists()) {
217+
if (!isDescendant(file, dir)) {
218+
throw new AbortException(file + " references a file that is not inside " + dir);
219+
} else if (!file.exists()) {
206220
throw new AbortException(file + " not found");
207221
}
208222
content = file.readToString();
@@ -215,14 +229,28 @@ public static class Execution extends AbstractSynchronousNonBlockingStepExecutio
215229
return content;
216230
}
217231

218-
private FilePath getFilePathWithSuffix(FilePath baseWorkspace) {
219-
return baseWorkspace.withSuffix(getFilePathSuffix() + "script");
232+
private FilePath getFilePathWithSuffix(FilePath baseWorkspace, SCM scm) {
233+
return baseWorkspace.withSuffix(getFilePathSuffix() + "script").child(CHECKOUT_DIR_KEY.mac(scm.getKey()));
220234
}
221235

222236
private String getFilePathSuffix() {
223237
return System.getProperty(WorkspaceList.class.getName(), "@");
224238
}
225239

240+
/**
241+
* Checks whether a given child path is a descendent of a given parent path using {@link File#getCanonicalFile}.
242+
*
243+
* If the child path does not exist, this method will canonicalize path elements such as {@code /../} and
244+
* {@code /./} before comparing it to the parent path, and it will not throw an exception. If the child path
245+
* does exist, symlinks will be resolved before checking whether the child is a descendant of the parent path.
246+
*/
247+
private static boolean isDescendant(FilePath child, FilePath parent) throws IOException, InterruptedException {
248+
if (child.isRemote() || parent.isRemote()) {
249+
throw new IllegalStateException();
250+
}
251+
return new File(child.getRemote()).getCanonicalFile().toPath().startsWith(parent.absolutize().getRemote());
252+
}
253+
226254
private static final long serialVersionUID = 1L;
227255

228256
}

‎src/test/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStepTest.java

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,11 @@
2424

2525
package org.jenkinsci.plugins.workflow.multibranch;
2626

27+
import hudson.Functions;
2728
import hudson.model.Result;
29+
import hudson.scm.SubversionSCM;
30+
import java.io.File;
31+
import java.nio.charset.StandardCharsets;
2832
import jenkins.branch.BranchSource;
2933
import jenkins.plugins.git.GitSampleRepoRule;
3034
import jenkins.plugins.git.GitStep;
@@ -40,11 +44,29 @@
4044
import org.jvnet.hudson.test.Issue;
4145
import org.jvnet.hudson.test.JenkinsRule;
4246

47+
import java.nio.file.Files;
48+
import java.nio.file.Path;
49+
import java.nio.file.Paths;
50+
import jenkins.plugins.git.GitSCMSource;
51+
import jenkins.scm.impl.subversion.SubversionSCMSource;
52+
import jenkins.scm.impl.subversion.SubversionSampleRepoRule;
53+
import org.apache.commons.io.FileUtils;
54+
import org.junit.Ignore;
55+
import org.jvnet.hudson.test.FlagRule;
56+
57+
import static org.hamcrest.MatcherAssert.assertThat;
58+
import static org.hamcrest.Matchers.equalTo;
59+
import static org.hamcrest.Matchers.not;
60+
import static org.hamcrest.io.FileMatchers.anExistingFile;
61+
import static org.junit.Assume.assumeFalse;
62+
4363
public class ReadTrustedStepTest {
4464

4565
@ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
4666
@Rule public JenkinsRule r = new JenkinsRule();
4767
@Rule public GitSampleRepoRule sampleRepo = new GitSampleRepoRule();
68+
@Rule public SubversionSampleRepoRule sampleRepoSvn = new SubversionSampleRepoRule();
69+
@Rule public FlagRule<Boolean> heavyweightCheckoutFlag = new FlagRule<>(() -> SCMBinder.USE_HEAVYWEIGHT_CHECKOUT, v -> { SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = v; });
4870

4971
@Test public void smokes() throws Exception {
5072
sampleRepo.init();
@@ -193,4 +215,157 @@ public class ReadTrustedStepTest {
193215
}
194216
}
195217

218+
@Test
219+
public void pathTraversalRejected() throws Exception {
220+
SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = true;
221+
sampleRepo.init();
222+
sampleRepo.write("Jenkinsfile", "node { checkout scm; echo \"${readTrusted '../../secrets/master.key'}\"}");
223+
Path secrets = Paths.get(sampleRepo.getRoot().getPath(), "secrets");
224+
Files.createSymbolicLink(secrets, Paths.get(r.jenkins.getRootDir() + "/secrets"));
225+
sampleRepo.git("add", ".");
226+
sampleRepo.git("commit", "-m", "init");
227+
228+
WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p");
229+
mp.getSourcesList().add(new BranchSource(new SCMBinderTest.WarySource(null, sampleRepo.toString(), "", "*", "", false)));
230+
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "master");
231+
r.waitUntilNoActivity();
232+
233+
WorkflowRun b = p.getLastBuild();
234+
assertEquals(1, b.getNumber());
235+
r.assertLogContains("secrets/master.key references a file that is not inside " + r.jenkins.getWorkspaceFor(p).getRemote(), b);
236+
}
237+
238+
@Issue("SECURITY-2491")
239+
@Test
240+
public void symlinksInReadTrustedCannotEscapeWorkspaceContext() throws Exception {
241+
SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = true;
242+
sampleRepo.init();
243+
sampleRepo.write("Jenkinsfile", "node { checkout scm; echo \"${readTrusted 'secrets/master.key'}\"}");
244+
Path secrets = Paths.get(sampleRepo.getRoot().getPath(), "secrets");
245+
Files.createSymbolicLink(secrets, Paths.get(r.jenkins.getRootDir() + "/secrets"));
246+
sampleRepo.git("add", ".");
247+
sampleRepo.git("commit", "-m", "init");
248+
249+
WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p");
250+
mp.getSourcesList().add(new BranchSource(new SCMBinderTest.WarySource(null, sampleRepo.toString(), "", "*", "", false)));
251+
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "master");
252+
r.waitUntilNoActivity();
253+
254+
WorkflowRun run = p.getLastBuild();
255+
assertEquals(1, run.getNumber());
256+
r.assertLogContains("secrets/master.key references a file that is not inside " + r.jenkins.getWorkspaceFor(p).getRemote(), run);
257+
}
258+
259+
@Issue("SECURITY-2491")
260+
@Test
261+
public void symlinksInUntrustedRevisionCannotEscapeWorkspace() throws Exception {
262+
SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = true;
263+
sampleRepo.init();
264+
sampleRepo.write("Jenkinsfile", "node { checkout scm; echo \"${readTrusted 'secrets/master.key'}\"}");
265+
sampleRepo.write("secrets/master.key", "secret info");
266+
sampleRepo.git("add", ".");
267+
sampleRepo.git("commit", "-m", "init");
268+
sampleRepo.git("checkout", "-b", "feature");
269+
Path secrets = Paths.get(sampleRepo.getRoot().getPath(), "secrets");
270+
Files.delete(Paths.get(secrets.toString(), "master.key"));
271+
Files.delete(secrets);
272+
Files.createSymbolicLink(secrets, Paths.get(r.jenkins.getRootDir() + "/secrets"));
273+
sampleRepo.git("add", ".");
274+
sampleRepo.git("commit", "-m", "now with unsafe symlink");
275+
276+
WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p");
277+
mp.getSourcesList().add(new BranchSource(new SCMBinderTest.WarySource(null, sampleRepo.toString(), "", "*", "", false)));
278+
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "feature");
279+
r.waitUntilNoActivity();
280+
281+
WorkflowRun run = p.getLastBuild();
282+
assertEquals(1, run.getNumber());
283+
r.assertLogContains("secrets/master.key references a file that is not inside ", run);
284+
}
285+
286+
@Issue("SECURITY-2491")
287+
@Test
288+
public void symlinksInNonMultibranchCannotEscapeWorkspaceContextViaReadTrusted() throws Exception {
289+
SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = true;
290+
sampleRepo.init();
291+
sampleRepo.write("Jenkinsfile", "echo \"${readTrusted 'master.key'}\"");
292+
Path secrets = Paths.get(sampleRepo.getRoot().getPath(), "master.key");
293+
Files.createSymbolicLink(secrets, Paths.get(r.jenkins.getRootDir() + "/secrets/master.key"));
294+
sampleRepo.git("add", ".");
295+
sampleRepo.git("commit", "-m", "init");
296+
297+
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
298+
GitStep step = new GitStep(sampleRepo.toString());
299+
p.setDefinition(new CpsScmFlowDefinition(step.createSCM(), "Jenkinsfile"));
300+
WorkflowRun run = r.buildAndAssertStatus(Result.FAILURE, p);
301+
302+
r.assertLogContains("master.key references a file that is not inside " + r.jenkins.getWorkspaceFor(p), run);
303+
}
304+
305+
@Ignore("There are two checkouts, one from CpsScmFlowDefinition via SCMBinder and one from ReadTrustedStep. Fixing the former requires an updated version of workflow-cps.")
306+
@Issue("SECURITY-2463")
307+
@Test public void multibranchCheckoutDirectoriesAreNotReusedByDifferentScms() throws Exception {
308+
SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = true;
309+
assumeFalse(Functions.isWindows()); // Checkout hook is not cross-platform.
310+
sampleRepo.init();
311+
sampleRepo.git("checkout", "-b", "trunk"); // So we end up using the same project for both SCMs.
312+
sampleRepo.write("Jenkinsfile", "echo('git library'); readTrusted('Jenkinsfile')");
313+
sampleRepo.git("add", "Jenkinsfile");
314+
sampleRepo.git("commit", "--message=init");
315+
sampleRepoSvn.init();
316+
sampleRepoSvn.write("Jenkinsfile", "echo('svn library'); readTrusted('Jenkinsfile')");
317+
// Copy .git folder from the Git repo into the SVN repo as data.
318+
File gitDirInSvnRepo = new File(sampleRepoSvn.wc(), ".git");
319+
FileUtils.copyDirectory(new File(sampleRepo.getRoot(), ".git"), gitDirInSvnRepo);
320+
String jenkinsRootDir = r.jenkins.getRootDir().toString();
321+
// Add a Git post-checkout hook to the .git folder in the SVN repo.
322+
Files.write(gitDirInSvnRepo.toPath().resolve("hooks/post-checkout"), ("#!/bin/sh\ntouch '" + jenkinsRootDir + "/hook-executed'\n").getBytes(StandardCharsets.UTF_8));
323+
sampleRepoSvn.svnkit("add", sampleRepoSvn.wc() + "/Jenkinsfile");
324+
sampleRepoSvn.svnkit("add", sampleRepoSvn.wc() + "/.git");
325+
sampleRepoSvn.svnkit("propset", "svn:executable", "ON", sampleRepoSvn.wc() + "/.git/hooks/post-checkout");
326+
sampleRepoSvn.svnkit("commit", "--message=init", sampleRepoSvn.wc());
327+
// Run a build using the SVN repo.
328+
WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p");
329+
mp.getSourcesList().add(new BranchSource(new SubversionSCMSource("", sampleRepoSvn.prjUrl())));
330+
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "trunk");
331+
r.waitUntilNoActivity();
332+
// Run a build using the Git repo. It should be checked out to a different directory than the SVN repo.
333+
mp.getSourcesList().clear();
334+
mp.getSourcesList().add(new BranchSource(new GitSCMSource("", sampleRepo.toString(), "", "*", "", false)));
335+
WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "trunk");
336+
r.waitUntilNoActivity();
337+
assertThat(p.getLastBuild().getNumber(), equalTo(2));
338+
assertThat(new File(r.jenkins.getRootDir(), "hook-executed"), not(anExistingFile()));
339+
}
340+
341+
@Ignore("There are two checkouts, one from CpsScmFlowDefinition and one from ReadTrustedStep. Fixing the former requires an updated version of workflow-cps.")
342+
@Issue("SECURITY-2463")
343+
@Test public void checkoutDirectoriesAreNotReusedByDifferentScms() throws Exception {
344+
SCMBinder.USE_HEAVYWEIGHT_CHECKOUT = true;
345+
assumeFalse(Functions.isWindows()); // Checkout hook is not cross-platform.
346+
sampleRepo.init();
347+
sampleRepo.write("Jenkinsfile", "echo('git library'); readTrusted('Jenkinsfile')");
348+
sampleRepo.git("add", "Jenkinsfile");
349+
sampleRepo.git("commit", "--message=init");
350+
sampleRepoSvn.init();
351+
sampleRepoSvn.write("Jenkinsfile", "echo('subversion library'); readTrusted('Jenkinsfile')");
352+
// Copy .git folder from the Git repo into the SVN repo as data.
353+
File gitDirInSvnRepo = new File(sampleRepoSvn.wc(), ".git");
354+
FileUtils.copyDirectory(new File(sampleRepo.getRoot(), ".git"), gitDirInSvnRepo);
355+
String jenkinsRootDir = r.jenkins.getRootDir().toString();
356+
// Add a Git post-checkout hook to the .git folder in the SVN repo.
357+
Files.write(gitDirInSvnRepo.toPath().resolve("hooks/post-checkout"), ("#!/bin/sh\ntouch '" + jenkinsRootDir + "/hook-executed'\n").getBytes(StandardCharsets.UTF_8));
358+
sampleRepoSvn.svnkit("add", sampleRepoSvn.wc() + "/Jenkinsfile");
359+
sampleRepoSvn.svnkit("add", sampleRepoSvn.wc() + "/.git");
360+
sampleRepoSvn.svnkit("propset", "svn:executable", "ON", sampleRepoSvn.wc() + "/.git/hooks/post-checkout");
361+
sampleRepoSvn.svnkit("commit", "--message=init", sampleRepoSvn.wc());
362+
// Run a build using the SVN repo.
363+
WorkflowJob p = r.createProject(WorkflowJob.class);
364+
p.setDefinition(new CpsScmFlowDefinition(new SubversionSCM(sampleRepoSvn.trunkUrl()), "Jenkinsfile"));
365+
r.buildAndAssertSuccess(p);
366+
// Run a build using the Git repo. It should be checked out to a different directory than the SVN repo.
367+
p.setDefinition(new CpsScmFlowDefinition(new GitStep(sampleRepo.toString()).createSCM(), "Jenkinsfile"));
368+
WorkflowRun b2 = r.buildAndAssertSuccess(p);
369+
assertThat(new File(r.jenkins.getRootDir(), "hook-executed"), not(anExistingFile()));
370+
}
196371
}

0 commit comments

Comments
 (0)