From e7cc06e7458303913498779af2c2fa3353f4dab1 Mon Sep 17 00:00:00 2001 From: aviadatsnyk Date: Sun, 6 May 2018 16:44:17 +0300 Subject: [PATCH 1/2] handle absolute paths correctly when unzipping When checking that an unzipped file does not break out of the target directory - this handles '..' in absolute paths correctly, where the previous implementation might not. --- core/src/main/java/hudson/FilePath.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index 0bcc31c32b..089d8f8ff8 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -598,7 +598,7 @@ public final class FilePath implements Serializable { while (entries.hasMoreElements()) { ZipEntry e = entries.nextElement(); File f = new File(dir, e.getName()); - if (!f.toPath().normalize().startsWith(dir.toPath())) { + if (!f.getCanonicalPath().startsWith(dir.getCanonicalPath())) { throw new IOException( "Zip " + zipFile.getPath() + " contains illegal file name that breaks out of the target directory: " + e.getName()); } From 36ab43f6af38b948bac878cbc909d17e57210467 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Mon, 8 Apr 2019 11:58:33 +0200 Subject: [PATCH 2/2] Add tests (also for #3402) --- test/src/test/java/hudson/FilePathTest.java | 161 ++++++++++++++++++ .../zip_with_relative/zip-rel-one-up.zip | Bin 0 -> 514 bytes .../zip_with_relative/zip-slip-win.zip | Bin 0 -> 547 bytes .../zip_with_relative/zip-slip.zip | Bin 0 -> 545 bytes .../zip_with_relative/zip-with-folder.zip | Bin 0 -> 442 bytes 5 files changed, 161 insertions(+) create mode 100644 test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-rel-one-up.zip create mode 100644 test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-slip-win.zip create mode 100644 test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-slip.zip create mode 100644 test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-with-folder.zip diff --git a/test/src/test/java/hudson/FilePathTest.java b/test/src/test/java/hudson/FilePathTest.java index bc5dfd3f4d..abf6d377cc 100644 --- a/test/src/test/java/hudson/FilePathTest.java +++ b/test/src/test/java/hudson/FilePathTest.java @@ -29,9 +29,14 @@ import org.apache.tools.ant.DirectoryScanner; import static org.hamcrest.Matchers.*; import org.junit.Test; import static org.junit.Assert.*; +import static org.junit.Assume.*; import org.junit.Rule; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.recipes.LocalData; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Method; public class FilePathTest { @@ -56,4 +61,160 @@ public class FilePathTest { } } + @Test + @Issue("JENKINS-32778") + @LocalData("zip_with_relative") + public void zipRelativePathHandledCorrectly() throws Exception { + assumeTrue(Functions.isWindows()); + FilePath zipFile = r.jenkins.getRootPath().child("zip-with-folder.zip"); + FilePath targetLocation = r.jenkins.getRootPath().child("unzip-target"); + + FilePath simple1 = targetLocation.child("simple1.txt"); + FilePath simple2 = targetLocation.child("child").child("simple2.txt"); + + assertThat(simple1.exists(), is(false)); + assertThat(simple2.exists(), is(false)); + + zipFile.unzip(targetLocation); + + assertThat(simple1.exists(), is(true)); + assertThat(simple2.exists(), is(true)); + } + + @Test + @Issue("JENKINS-32778") + @LocalData("zip_with_relative") + public void zipAbsolutePathHandledCorrectly_win() throws Exception { + assumeTrue(Functions.isWindows()); + + // this special zip contains a ..\..\ [..] \..\Temp\evil.txt + FilePath zipFile = r.jenkins.getRootPath().child("zip-slip-win.zip"); + + FilePath targetLocation = r.jenkins.getRootPath().child("unzip-target"); + + FilePath good = targetLocation.child("good.txt"); + + assertThat(good.exists(), is(false)); + + try { + zipFile.unzip(targetLocation); + fail("The evil.txt should have triggered an exception"); + } + catch(IOException e){ + assertThat(e.getMessage(), containsString("contains illegal file name that breaks out of the target directory")); + } + + // as the unzip operation failed, the good.txt was potentially unzipped + // but we need to make sure that the evil.txt is not there + File evil = new File(r.jenkins.getRootDir(), "..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\Temp\\evil.txt"); + assertThat(evil.exists(), is(false)); + } + + @Test + @Issue("JENKINS-32778") + @LocalData("zip_with_relative") + public void zipAbsolutePathHandledCorrectly_unix() throws Exception { + assumeFalse(Functions.isWindows()); + + // this special zip contains a ../../../ [..] /../tmp/evil.txt + FilePath zipFile = r.jenkins.getRootPath().child("zip-slip.zip"); + + FilePath targetLocation = r.jenkins.getRootPath().child("unzip-target"); + + FilePath good = targetLocation.child("good.txt"); + + assertThat(good.exists(), is(false)); + + try { + zipFile.unzip(targetLocation); + fail("The evil.txt should have triggered an exception"); + } + catch(IOException e){ + assertThat(e.getMessage(), containsString("contains illegal file name that breaks out of the target directory")); + } + + // as the unzip operation failed, the good.txt was potentially unzipped + // but we need to make sure that the evil.txt is not there + File evil = new File(r.jenkins.getRootDir(), "../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt"); + assertThat(evil.exists(), is(false)); + } + + @Test + @Issue("JENKINS-32778") + @LocalData("zip_with_relative") + public void zipRelativePathHandledCorrectly_oneUp() throws Exception { + // internal structure: + // ../simple3.txt + // child/simple2.txt + // simple1.txt + FilePath zipFile = r.jenkins.getRootPath().child("zip-rel-one-up.zip"); + FilePath targetLocation = r.jenkins.getRootPath().child("unzip-target"); + + FilePath simple3 = targetLocation.getParent().child("simple3.txt"); + + assertThat(simple3.exists(), is(false)); + + try { + zipFile.unzip(targetLocation); + fail("The ../simple3.txt should have triggered an exception"); + } + catch(IOException e){ + assertThat(e.getMessage(), containsString("contains illegal file name that breaks out of the target directory")); + } + + assertThat(simple3.exists(), is(false)); + } + + @Test + @Issue("XXX") + @LocalData("zip_with_relative") + public void zipTarget_regular() throws Exception { + assumeTrue(Functions.isWindows()); + File zipFile = new File(r.jenkins.getRootDir(), "zip-with-folder.zip"); + File targetLocation = new File(r.jenkins.getRootDir(), "unzip-target"); + FilePath targetLocationFP = r.jenkins.getRootPath().child("unzip-target"); + + FilePath simple1 = targetLocationFP.child("simple1.txt"); + FilePath simple2 = targetLocationFP.child("child").child("simple2.txt"); + + assertThat(simple1.exists(), is(false)); + assertThat(simple2.exists(), is(false)); + + Method unzipPrivateMethod; + unzipPrivateMethod = FilePath.class.getDeclaredMethod("unzip", File.class, File.class); + unzipPrivateMethod.setAccessible(true); + + FilePath fp = new FilePath(new File(".")); + unzipPrivateMethod.invoke(fp, targetLocation, zipFile); + + assertThat(simple1.exists(), is(true)); + assertThat(simple2.exists(), is(true)); + } + + @Test + @Issue("XXX") + @LocalData("zip_with_relative") + public void zipTarget_relative() throws Exception { + assumeTrue(Functions.isWindows()); + File zipFile = new File(r.jenkins.getRootDir(), "zip-with-folder.zip"); + // the main difference is here, the ./ + File targetLocation = new File(r.jenkins.getRootDir(), "./unzip-target"); + FilePath targetLocationFP = r.jenkins.getRootPath().child("unzip-target"); + + FilePath simple1 = targetLocationFP.child("simple1.txt"); + FilePath simple2 = targetLocationFP.child("child").child("simple2.txt"); + + assertThat(simple1.exists(), is(false)); + assertThat(simple2.exists(), is(false)); + + Method unzipPrivateMethod; + unzipPrivateMethod = FilePath.class.getDeclaredMethod("unzip", File.class, File.class); + unzipPrivateMethod.setAccessible(true); + + FilePath fp = new FilePath(new File(".")); + unzipPrivateMethod.invoke(fp, targetLocation, zipFile); + + assertThat(simple1.exists(), is(true)); + assertThat(simple2.exists(), is(true)); + } } diff --git a/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-rel-one-up.zip b/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-rel-one-up.zip new file mode 100644 index 0000000000000000000000000000000000000000..7baedb7cf3ce382b801b0d813fac632204ef7fa0 GIT binary patch literal 514 zcmWIWW@h1H0D zmsW5yFtWU0W?%pl5M{c?0dVuK0nIzKyf_MG9yicDh$=(4VPBY5$2>d}7#M;0e7oS G!vFw7+H>{* literal 0 HcmV?d00001 diff --git a/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-slip-win.zip b/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-slip-win.zip new file mode 100644 index 0000000000000000000000000000000000000000..3474c88bec74e6381fb9e1e598f98076c64f2d68 GIT binary patch literal 547 zcmWIWW@h1H0D=Au{XYEp{-1?`Y!K#PkYPyA&ri`SsVE5z;bdU8U359BLG@ZxX$3a} zBg;2N1_l-ppt_Qb%wh!~N>l);R>;pw*3=BYDGc0KYu@GUy3JDvua0&2cWD;S< n9Sk5dKwwE@D3BbG5CK|>5-0)QtZX1BF##bX(5E~g-!cFIhBtZG literal 0 HcmV?d00001 diff --git a/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-slip.zip b/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-slip.zip new file mode 100644 index 0000000000000000000000000000000000000000..38b3f499de0163e62ca15ce18350a9d9a477a51b GIT binary patch literal 545 zcmWIWW@h1H0D=Au{XYEp{-1?`Y!K#PkYPyA&ri`SsVE5z;bdU8U359h4v0%DxEUB( zzA-W|u!sQFm1JZVD*#cV0!Xz&eqJh90MJm76a&LlprHwl)s`S02)6*So}T`Ippx7I z{nWC|9FT|Lj?Pm62|-=W$Rx*%D=;L0E@xl>dYWNLBZ!3v8dgZqpan~SHzSh>Gwx6T jnE?Vz8bg8PfCLE8QsgiR@MdKLxrhk}K_2A>d6oeH^pk5C literal 0 HcmV?d00001 diff --git a/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-with-folder.zip b/test/src/test/resources/hudson/FilePathTest/zip_with_relative/zip-with-folder.zip new file mode 100644 index 0000000000000000000000000000000000000000..b76cfb67918100ee0c57822296f4a6d1599e0a95 GIT binary patch literal 442 zcmWIWW@Zs#0D*L$4nHshO0WUx^Fbwc!WU^<#BCJ+#SP*AsVF(`of zj0_SC`(<{_y8HGDBaF7sJ2?@=hnS1Y2QYI%=787`KO^}E*^0Rzq?dt=tdHC_1DQs) u0@Jt1Rzw4>K=M1X;VJb87lI5m5t-G4Ob2+gf+B-~83-j985nv%90mYPDP=VP literal 0 HcmV?d00001