From 18fc7c0b466553cbd4f790db3270964305bee7f9 Mon Sep 17 00:00:00 2001 From: Jeff Thompson Date: Tue, 2 Jul 2019 12:47:11 -0600 Subject: [PATCH] [SECURITY-1424] --- .../java/hudson/model/FileParameterValue.java | 4 +- .../hudson/model/FileParameterValueTest.java | 117 +++++++++++++++++- 2 files changed, 118 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/FileParameterValue.java b/core/src/main/java/hudson/model/FileParameterValue.java index f911927285ae..33336cd6b2f3 100644 --- a/core/src/main/java/hudson/model/FileParameterValue.java +++ b/core/src/main/java/hudson/model/FileParameterValue.java @@ -37,6 +37,7 @@ import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; +import java.util.regex.Pattern; import javax.servlet.ServletException; import org.apache.commons.fileupload.FileItem; @@ -65,6 +66,7 @@ */ public class FileParameterValue extends ParameterValue { private static final String FOLDER_NAME = "fileParameters"; + private static final Pattern PROHIBITED_DOUBLE_DOT = Pattern.compile(".*[\\\\/]\\.\\.[\\\\/].*"); /** * Escape hatch for SECURITY-1074, fileParameter used to escape their expected folder. @@ -162,7 +164,7 @@ public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener l if (ws == null) { throw new IllegalStateException("The workspace should be created when setUp method is called"); } - if (!ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE && !ws.isDescendant(location)) { + if (!ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE && (PROHIBITED_DOUBLE_DOT.matcher(location).matches() || !ws.isDescendant(location))) { listener.error("Rejecting file path escaping base directory with relative path: " + location); // force the build to fail return null; diff --git a/test/src/test/java/hudson/model/FileParameterValueTest.java b/test/src/test/java/hudson/model/FileParameterValueTest.java index 911f88431b2b..0e8c7f65ec30 100644 --- a/test/src/test/java/hudson/model/FileParameterValueTest.java +++ b/test/src/test/java/hudson/model/FileParameterValueTest.java @@ -94,7 +94,64 @@ public void fileParameter_cannotCreateFile_outsideOfBuildFolder() throws Excepti // overlong utf-8 encoding checkUrlNot200AndNotContains(wc, build.getUrl() + "parameters/parameter/%c0%2e%c0%2e%c0%af%c0%2e%c0%2e%c0%af%c0%2e%c0%2e%c0%af%c0%2e%c0%2e%c0%af%c0%2e%c0%2e%c0%afroot-level.txt/uploaded-file.txt", uploadedContent); } - + + @Test + @Issue("SECURITY-1424") + public void fileParameter_cannotCreateFile_outsideOfBuildFolder_SEC1424() throws Exception { + // you can test the behavior before the correction by setting FileParameterValue.ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE to true + + FilePath root = j.jenkins.getRootPath(); + + FreeStyleProject p = j.createFreeStyleProject(); + p.addProperty(new ParametersDefinitionProperty(Collections.singletonList( + new FileParameterDefinition("dir/../../../pwned", null) + ))); + + assertThat(root.child("pwned").exists(), equalTo(false)); + + String uploadedContent = "test-content"; + File uploadedFile = tmp.newFile(); + FileUtils.write(uploadedFile, uploadedContent); + + FreeStyleBuild build = p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction( + new FileParameterValue("dir/../../../pwned", uploadedFile, "uploaded-file.txt") + )).get(); + + assertThat(build.getResult(), equalTo(Result.FAILURE)); + assertThat(root.child("pwned").exists(), equalTo(false)); + + // ensure also the file is not reachable by request + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + } + + @Test + public void fileParameter_cannotCreateFile_outsideOfBuildFolder_LeadingDoubleDot() throws Exception { + FilePath root = j.jenkins.getRootPath(); + + FreeStyleProject p = j.createFreeStyleProject(); + p.addProperty(new ParametersDefinitionProperty(Collections.singletonList( + new FileParameterDefinition("../pwned", null) + ))); + + assertThat(root.child("pwned").exists(), equalTo(false)); + + String uploadedContent = "test-content"; + File uploadedFile = tmp.newFile(); + FileUtils.write(uploadedFile, uploadedContent); + + FreeStyleBuild build = p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction( + new FileParameterValue("../pwned", uploadedFile, "uploaded-file.txt") + )).get(); + + assertThat(build.getResult(), equalTo(Result.FAILURE)); + assertThat(root.child("pwned").exists(), equalTo(false)); + + // ensure also the file is not reachable by request + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + } + private void checkUrlNot200AndNotContains(JenkinsRule.WebClient wc, String url, String contentNotPresent) throws Exception { Page pageForEncoded = wc.goTo(url, null); assertThat(pageForEncoded.getWebResponse().getStatusCode(), not(equalTo(200))); @@ -104,7 +161,7 @@ private void checkUrlNot200AndNotContains(JenkinsRule.WebClient wc, String url, @Test @Issue("SECURITY-1074") public void fileParameter_cannotCreateFile_outsideOfBuildFolder_backslashEdition() throws Exception { - Assume.assumeTrue("Backslash are only dangerous on Windows", Functions.isWindows()); + Assume.assumeTrue("Backslashes are only dangerous on Windows", Functions.isWindows()); // you can test the behavior before the correction by setting FileParameterValue.ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE to true @@ -267,4 +324,60 @@ public void fileParameter_canStillUse_internalHierarchy() throws Exception { String workspaceParentContent = workspaceParentPage.getWebResponse().getContentAsString(); assertThat(workspaceParentContent, containsString("child2.txt")); } + + @Test + public void fileParameter_canStillUse_doubleDotsInFileName() throws Exception { + FreeStyleProject p = j.createFreeStyleProject(); + p.addProperty(new ParametersDefinitionProperty(Arrays.asList( + new FileParameterDefinition("weird..name.txt", null) + ))); + + File uploadedFile = tmp.newFile(); + FileUtils.write(uploadedFile, "test1"); + + FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction( + new FileParameterValue("weird..name.txt", uploadedFile, "uploaded-file.txt") + ))); + + // files are correctly saved in the build "fileParameters" folder + File directChild = new File(build.getRootDir(), "fileParameters/weird..name.txt"); + assertTrue(directChild.exists()); + + // both are correctly copied inside the workspace + assertTrue(build.getWorkspace().child("weird..name.txt").exists()); + + // and reachable using request + JenkinsRule.WebClient wc = j.createWebClient(); + HtmlPage workspacePage = wc.goTo(p.getUrl() + "ws"); + String workspaceContent = workspacePage.getWebResponse().getContentAsString(); + assertThat(workspaceContent, containsString("weird..name.txt")); + } + + @Test + public void fileParameter_canStillUse_TildeInFileName() throws Exception { + FreeStyleProject p = j.createFreeStyleProject(); + p.addProperty(new ParametersDefinitionProperty(Arrays.asList( + new FileParameterDefinition("~name", null) + ))); + + File uploadedFile = tmp.newFile(); + FileUtils.write(uploadedFile, "test1"); + + FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction( + new FileParameterValue("~name", uploadedFile, "uploaded-file.txt") + ))); + + // files are correctly saved in the build "fileParameters" folder + File directChild = new File(build.getRootDir(), "fileParameters/~name"); + assertTrue(directChild.exists()); + + // both are correctly copied inside the workspace + assertTrue(build.getWorkspace().child("~name").exists()); + + // and reachable using request + JenkinsRule.WebClient wc = j.createWebClient(); + HtmlPage workspacePage = wc.goTo(p.getUrl() + "ws"); + String workspaceContent = workspacePage.getWebResponse().getContentAsString(); + assertThat(workspaceContent, containsString("~name")); + } }