Skip to content

Commit 18fc7c0

Browse files
jeffret-bjeffret-b
committed
1 parent 279d810 commit 18fc7c0

File tree

2 files changed

+118
-3
lines changed

2 files changed

+118
-3
lines changed

‎core/src/main/java/hudson/model/FileParameterValue.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import java.nio.file.Files;
3838
import java.nio.file.InvalidPathException;
3939
import java.nio.file.Path;
40+
import java.util.regex.Pattern;
4041
import javax.servlet.ServletException;
4142

4243
import org.apache.commons.fileupload.FileItem;
@@ -65,6 +66,7 @@
6566
*/
6667
public class FileParameterValue extends ParameterValue {
6768
private static final String FOLDER_NAME = "fileParameters";
69+
private static final Pattern PROHIBITED_DOUBLE_DOT = Pattern.compile(".*[\\\\/]\\.\\.[\\\\/].*");
6870

6971
/**
7072
* 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
162164
if (ws == null) {
163165
throw new IllegalStateException("The workspace should be created when setUp method is called");
164166
}
165-
if (!ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE && !ws.isDescendant(location)) {
167+
if (!ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE && (PROHIBITED_DOUBLE_DOT.matcher(location).matches() || !ws.isDescendant(location))) {
166168
listener.error("Rejecting file path escaping base directory with relative path: " + location);
167169
// force the build to fail
168170
return null;

‎test/src/test/java/hudson/model/FileParameterValueTest.java‎

Lines changed: 115 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,64 @@ public void fileParameter_cannotCreateFile_outsideOfBuildFolder() throws Excepti
9494
// overlong utf-8 encoding
9595
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);
9696
}
97-
97+
98+
@Test
99+
@Issue("SECURITY-1424")
100+
public void fileParameter_cannotCreateFile_outsideOfBuildFolder_SEC1424() throws Exception {
101+
// you can test the behavior before the correction by setting FileParameterValue.ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE to true
102+
103+
FilePath root = j.jenkins.getRootPath();
104+
105+
FreeStyleProject p = j.createFreeStyleProject();
106+
p.addProperty(new ParametersDefinitionProperty(Collections.singletonList(
107+
new FileParameterDefinition("dir/../../../pwned", null)
108+
)));
109+
110+
assertThat(root.child("pwned").exists(), equalTo(false));
111+
112+
String uploadedContent = "test-content";
113+
File uploadedFile = tmp.newFile();
114+
FileUtils.write(uploadedFile, uploadedContent);
115+
116+
FreeStyleBuild build = p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
117+
new FileParameterValue("dir/../../../pwned", uploadedFile, "uploaded-file.txt")
118+
)).get();
119+
120+
assertThat(build.getResult(), equalTo(Result.FAILURE));
121+
assertThat(root.child("pwned").exists(), equalTo(false));
122+
123+
// ensure also the file is not reachable by request
124+
JenkinsRule.WebClient wc = j.createWebClient();
125+
wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
126+
}
127+
128+
@Test
129+
public void fileParameter_cannotCreateFile_outsideOfBuildFolder_LeadingDoubleDot() throws Exception {
130+
FilePath root = j.jenkins.getRootPath();
131+
132+
FreeStyleProject p = j.createFreeStyleProject();
133+
p.addProperty(new ParametersDefinitionProperty(Collections.singletonList(
134+
new FileParameterDefinition("../pwned", null)
135+
)));
136+
137+
assertThat(root.child("pwned").exists(), equalTo(false));
138+
139+
String uploadedContent = "test-content";
140+
File uploadedFile = tmp.newFile();
141+
FileUtils.write(uploadedFile, uploadedContent);
142+
143+
FreeStyleBuild build = p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
144+
new FileParameterValue("../pwned", uploadedFile, "uploaded-file.txt")
145+
)).get();
146+
147+
assertThat(build.getResult(), equalTo(Result.FAILURE));
148+
assertThat(root.child("pwned").exists(), equalTo(false));
149+
150+
// ensure also the file is not reachable by request
151+
JenkinsRule.WebClient wc = j.createWebClient();
152+
wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
153+
}
154+
98155
private void checkUrlNot200AndNotContains(JenkinsRule.WebClient wc, String url, String contentNotPresent) throws Exception {
99156
Page pageForEncoded = wc.goTo(url, null);
100157
assertThat(pageForEncoded.getWebResponse().getStatusCode(), not(equalTo(200)));
@@ -104,7 +161,7 @@ private void checkUrlNot200AndNotContains(JenkinsRule.WebClient wc, String url,
104161
@Test
105162
@Issue("SECURITY-1074")
106163
public void fileParameter_cannotCreateFile_outsideOfBuildFolder_backslashEdition() throws Exception {
107-
Assume.assumeTrue("Backslash are only dangerous on Windows", Functions.isWindows());
164+
Assume.assumeTrue("Backslashes are only dangerous on Windows", Functions.isWindows());
108165

109166
// you can test the behavior before the correction by setting FileParameterValue.ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE to true
110167

@@ -267,4 +324,60 @@ public void fileParameter_canStillUse_internalHierarchy() throws Exception {
267324
String workspaceParentContent = workspaceParentPage.getWebResponse().getContentAsString();
268325
assertThat(workspaceParentContent, containsString("child2.txt"));
269326
}
327+
328+
@Test
329+
public void fileParameter_canStillUse_doubleDotsInFileName() throws Exception {
330+
FreeStyleProject p = j.createFreeStyleProject();
331+
p.addProperty(new ParametersDefinitionProperty(Arrays.asList(
332+
new FileParameterDefinition("weird..name.txt", null)
333+
)));
334+
335+
File uploadedFile = tmp.newFile();
336+
FileUtils.write(uploadedFile, "test1");
337+
338+
FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
339+
new FileParameterValue("weird..name.txt", uploadedFile, "uploaded-file.txt")
340+
)));
341+
342+
// files are correctly saved in the build "fileParameters" folder
343+
File directChild = new File(build.getRootDir(), "fileParameters/weird..name.txt");
344+
assertTrue(directChild.exists());
345+
346+
// both are correctly copied inside the workspace
347+
assertTrue(build.getWorkspace().child("weird..name.txt").exists());
348+
349+
// and reachable using request
350+
JenkinsRule.WebClient wc = j.createWebClient();
351+
HtmlPage workspacePage = wc.goTo(p.getUrl() + "ws");
352+
String workspaceContent = workspacePage.getWebResponse().getContentAsString();
353+
assertThat(workspaceContent, containsString("weird..name.txt"));
354+
}
355+
356+
@Test
357+
public void fileParameter_canStillUse_TildeInFileName() throws Exception {
358+
FreeStyleProject p = j.createFreeStyleProject();
359+
p.addProperty(new ParametersDefinitionProperty(Arrays.asList(
360+
new FileParameterDefinition("~name", null)
361+
)));
362+
363+
File uploadedFile = tmp.newFile();
364+
FileUtils.write(uploadedFile, "test1");
365+
366+
FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
367+
new FileParameterValue("~name", uploadedFile, "uploaded-file.txt")
368+
)));
369+
370+
// files are correctly saved in the build "fileParameters" folder
371+
File directChild = new File(build.getRootDir(), "fileParameters/~name");
372+
assertTrue(directChild.exists());
373+
374+
// both are correctly copied inside the workspace
375+
assertTrue(build.getWorkspace().child("~name").exists());
376+
377+
// and reachable using request
378+
JenkinsRule.WebClient wc = j.createWebClient();
379+
HtmlPage workspacePage = wc.goTo(p.getUrl() + "ws");
380+
String workspaceContent = workspacePage.getWebResponse().getContentAsString();
381+
assertThat(workspaceContent, containsString("~name"));
382+
}
270383
}

0 commit comments

Comments
 (0)