From b0bf62f381f9d5eba42f559063cbd81730298c7c Mon Sep 17 00:00:00 2001 From: Richard Killen Date: Wed, 19 Jun 2019 16:48:06 -0500 Subject: [PATCH] Issue #400 - Check for arbitrary file overwrite vulnerability (zip slip) --- .../weblogic/deploy/util/FileUtils.java | 12 +++- .../deploy/messages/wlsdeploy_rb.properties | 4 +- .../weblogic/deploy/util/FileUtilsTest.java | 67 ++++++++++++++++++- 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/oracle/weblogic/deploy/util/FileUtils.java b/core/src/main/java/oracle/weblogic/deploy/util/FileUtils.java index 6b397d111e..ce63e19edf 100644 --- a/core/src/main/java/oracle/weblogic/deploy/util/FileUtils.java +++ b/core/src/main/java/oracle/weblogic/deploy/util/FileUtils.java @@ -652,6 +652,10 @@ public static void extractZipFileContent(WLSDeployArchive archiveFile, String zi Files.createDirectory(Paths.get(extractPath)); } + // verify that each target file is under the extract directory, + // to protect from the file overwrite security vulnerability (zip slip). + String canonicalExtractPath = extractDir.getCanonicalPath(); + byte[] buffer = new byte[1024]; FileInputStream fis = new FileInputStream(walletZip); ZipInputStream zis = new ZipInputStream(fis); @@ -659,6 +663,11 @@ public static void extractZipFileContent(WLSDeployArchive archiveFile, String zi while (ze != null) { String fileName = ze.getName(); File newFile = new File(extractPath + File.separator + fileName); + String canonicalNewFile = newFile.getCanonicalPath(); + if(!canonicalNewFile.startsWith(canonicalExtractPath + File.separator)) { + throw new WLSDeployArchiveIOException("WLSDPLY-01119", ze.getName()); + } + new File(newFile.getParent()).mkdirs(); FileOutputStream fos = new FileOutputStream(newFile); int len = zis.read(buffer); @@ -677,7 +686,8 @@ public static void extractZipFileContent(WLSDeployArchive archiveFile, String zi Files.delete(Paths.get(walletZip)); } } catch (IOException | WLSDeployArchiveIOException ioe) { - String message = ExceptionHelper.getMessage("WLSDPLY-01118", METHOD, CLASS, ioe.getLocalizedMessage()); + String message = ExceptionHelper.getMessage("WLSDPLY-01118", archiveFile.getArchiveFileName(), + ioe.getLocalizedMessage()); IllegalArgumentException iae = new IllegalArgumentException(message); LOGGER.throwing(CLASS, METHOD, iae); throw iae; diff --git a/core/src/main/resources/oracle/weblogic/deploy/messages/wlsdeploy_rb.properties b/core/src/main/resources/oracle/weblogic/deploy/messages/wlsdeploy_rb.properties index 95b92b4cab..fedb0541f3 100644 --- a/core/src/main/resources/oracle/weblogic/deploy/messages/wlsdeploy_rb.properties +++ b/core/src/main/resources/oracle/weblogic/deploy/messages/wlsdeploy_rb.properties @@ -128,7 +128,9 @@ WLSDPLY-01114=Deleting the file {1} in directory {0} WLSDPLY-01115=Unable to delete file {0} from directory {1} WLSDPLY-01116=Unable to successfully delete the directory {0} WLSDPLY-01117=Model directory {0} has more than one {1} file, found {2} after previously finding {3} -WLSDPLY-01118=Error extracting zipentry zip file {0} +WLSDPLY-01118=Error extracting zipentry zip file {0}: {1} +WLSDPLY-01119=Zip entry is outside of the target directory: {0} + # oracle.weblogic.deploy.util.ProcessHandler.java WLSDPLY-01200=Process for command {0} isRunning() unable to get an exit value: {1} WLSDPLY-01201=ProcessHandler had no registered wait handler when asked to exec() command: {0} diff --git a/core/src/test/java/oracle/weblogic/deploy/util/FileUtilsTest.java b/core/src/test/java/oracle/weblogic/deploy/util/FileUtilsTest.java index 862f207a11..e9af205a72 100644 --- a/core/src/test/java/oracle/weblogic/deploy/util/FileUtilsTest.java +++ b/core/src/test/java/oracle/weblogic/deploy/util/FileUtilsTest.java @@ -1,13 +1,18 @@ /* - * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved. * The Universal Permissive License (UPL), Version 1.0 */ package oracle.weblogic.deploy.util; +import java.io.ByteArrayOutputStream; import java.io.File; +import java.io.FileOutputStream; import java.text.MessageFormat; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; public class FileUtilsTest { @@ -49,6 +54,16 @@ public class FileUtilsTest { private static final String APP_PATH = "wlsdeploy/applications/simpleear.ear"; private static final String APP_FILE_NAME = "src/test/resources/simpleear.ear"; + private static final File UNIT_TEST_TARGET_DIR = new File(WLSDeployZipFileTest.UNIT_TEST_TARGET_DIR, "fileutils"); + private static final String WALLET_PATH = "wlsdeploy/wallet.zip"; + + @Before + public void initialize() throws Exception { + if(!UNIT_TEST_TARGET_DIR.exists() && !UNIT_TEST_TARGET_DIR.mkdirs()) { + throw new Exception("Unable to create unit test directory: " + UNIT_TEST_TARGET_DIR); + } + } + @Test public void testNormalFile_parseFileName() throws Exception { File f = new File(FILE1); @@ -104,6 +119,56 @@ public void testHashing() throws Exception { Assert.assertEquals(appHash, archiveHash); } + @Test + /* A wallet zip inside the archive must not contain an entry such as ../info.txt, + since this creates a file overwrite security vulnerability (zip slip). + */ + public void testZipVulnerability() throws Exception { + String extractPath = UNIT_TEST_TARGET_DIR.getPath(); + + // an entry with a simple name or path works fine + File zipFile = buildWalletArchiveZip("info.txt"); + WLSDeployArchive deployArchive = new WLSDeployArchive(zipFile.getPath()); + FileUtils.extractZipFileContent(deployArchive, WALLET_PATH, extractPath); + + // an entry with parent directory notation should throw an exception + try { + zipFile = buildWalletArchiveZip("../info.txt"); + deployArchive = new WLSDeployArchive(zipFile.getPath()); + FileUtils.extractZipFileContent(deployArchive, WALLET_PATH, extractPath); + Assert.fail("Exception not thrown for zip entry outside extract directory"); + + } catch(IllegalArgumentException e) { + // expected behavior + } + } + + /* Build an archive zip containing a wallet zip. + The wallet contains a single entry with the name of the contentName argument. + */ + private File buildWalletArchiveZip(String contentName) throws Exception { + + // create the wallet zip content + ByteArrayOutputStream walletBytes = new ByteArrayOutputStream(); + try (ZipOutputStream zipStream = new ZipOutputStream(walletBytes)) { + ZipEntry zipEntry = new ZipEntry(contentName); + zipStream.putNextEntry(zipEntry); + byte[] data = "info".getBytes(); + zipStream.write(data, 0, data.length); + zipStream.closeEntry(); + } + + File archiveFile = new File(UNIT_TEST_TARGET_DIR, "archive.zip"); + + try (ZipOutputStream zipStream = new ZipOutputStream(new FileOutputStream(archiveFile))) { + ZipEntry zipEntry = new ZipEntry(WALLET_PATH); + zipStream.putNextEntry(zipEntry); + zipStream.write(walletBytes.toByteArray()); + zipStream.closeEntry(); + } + + return archiveFile; + } private void assertMatch(String name, String got, String expected) { Assert.assertTrue(MessageFormat.format(FILE_ERR_FORMAT, name, got, expected),