Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Eliminate Zip-Slip vulnerability #908

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Eliminate Zip-Slip vulnerability #908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/ZipUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.common.io.ByteStreams;
import java.io.BufferedInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand All @@ -37,13 +38,19 @@ public class ZipUtil {
* @throws IOException when I/O error occurs
*/
public static void unzip(Path archive, Path destination) throws IOException {
String canonicalDestination = destination.toFile().getCanonicalPath();

try (InputStream fileIn = Files.newInputStream(archive);
ZipInputStream zipIn = new ZipInputStream(new BufferedInputStream(fileIn))) {

for (ZipEntry entry = zipIn.getNextEntry(); entry != null; entry = zipIn.getNextEntry()) {
// TODO: check Zip-Slip vulnerability: https://snyk.io/research/zip-slip-vulnerability#java
Path entryPath = destination.resolve(entry.getName());

String canonicalTarget = entryPath.toFile().getCanonicalPath();
if (!canonicalTarget.startsWith(canonicalDestination + File.separator)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be done with NIO? Like maybe entryPath.toAbsolutePath().startsWith(destination.toAbsolutePath())?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A canonical path is an absolute path, but the opposite is not true. For example,

System.out.println(Paths.get("/temp1/../temp/test.txt").toAbsolutePath());

prints /temp1/../temp/test.txt.

The code here is what https://snyk.io/research/zip-slip-vulnerability actually suggests, so I think it is safe to stick with the canonical path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, though it feels off having to use the deprecated java.io.File API to account for a security vulnerability. Perhaps we could use Path#normalize?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think Path.normalize() would work. A canonical path is a unique path for any given file; there can ever be only one canonical path. Computing a canonical path may involve "resolving symbolic links (on UNIX platforms), and converting drive letters to a standard case (on Microsoft Windows platforms)" according to its Javadoc. So I think it's safe to stick with the canonical path approach.

throw new IOException("Blocked unzipping files outside destination: " + entry.getName());
}

if (entry.isDirectory()) {
Files.createDirectories(entryPath);
} else {
Expand Down
29 changes: 29 additions & 0 deletions jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/ZipUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.hamcrest.CoreMatchers;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
Expand All @@ -45,6 +47,22 @@ public void testUnzip_nonExistingDestination() throws URISyntaxException, IOExce
Assert.assertTrue(Files.exists(destination));
}

@Test
public void testZipSlipVulnerability_windows() throws URISyntaxException {
Assume.assumeTrue(System.getProperty("os.name").startsWith("Windows"));

Path archive = Paths.get(Resources.getResource("test-archives/zip-slip-win.zip").toURI());
verifyZipSlipSafe(archive);
}

@Test
public void testZipSlipVulnerability_unix() throws URISyntaxException {
Assume.assumeFalse(System.getProperty("os.name").startsWith("Windows"));

Path archive = Paths.get(Resources.getResource("test-archives/zip-slip.zip").toURI());
verifyZipSlipSafe(archive);
}

private void verifyUnzip(Path destination) throws URISyntaxException, IOException {
Path archive = Paths.get(Resources.getResource("test-archives/test.zip").toURI());

Expand All @@ -59,4 +77,15 @@ private void verifyUnzip(Path destination) throws URISyntaxException, IOExceptio
Assert.assertEquals("file2", Files.readAllLines(file2).get(0));
Assert.assertEquals("file3", Files.readAllLines(file3).get(0));
}

private void verifyZipSlipSafe(Path archive) {
try {
ZipUtil.unzip(archive, tempFolder.getRoot().toPath());
Assert.fail("Should block Zip-Slip");
} catch (IOException ex) {
Assert.assertThat(
ex.getMessage(),
CoreMatchers.startsWith("Blocked unzipping files outside destination: "));
}
}
}
Binary file added jib-plugins-common/src/test/resources/test-archives/zip-slip-win.zip
View file
Open in desktop
Binary file not shown.
Binary file added jib-plugins-common/src/test/resources/test-archives/zip-slip.zip
View file
Open in desktop
Binary file not shown.