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

#62 Canonical path fix #63

Merged
merged 3 commits into from
Sep 12, 2019
Merged

#62 Canonical path fix #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0693bb4
zip slip fix
LeeYoung624 Sep 10, 2019
46a6916
fix exceptions that may caused by not normalized
LeeYoung624 Sep 10, 2019
75de1a7
revert getCanonicalPath back to getPath in tests
LeeYoung624 Sep 10, 2019
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
2 changes: 1 addition & 1 deletion src/main/java/net/lingala/zip4j/tasks/AbstractExtractFileTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ protected void extractFile(ZipInputStream zipInputStream, FileHeader fileHeader,
// make sure no file is extracted outside of the target directory (a.k.a zip slip)
String fileName = fileHeader.getFileName();
String completePath = outPath + fileName;
if (!new File(completePath).getCanonicalPath().startsWith(new File(outPath).getPath())) {
if (!new File(completePath).getCanonicalPath().startsWith(new File(outPath).getCanonicalPath())) {
throw new ZipException("illegal file name that breaks out of the target directory: "
+ fileHeader.getFileName());
}
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/net/lingala/zip4j/tasks/AddFolderToZipTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ protected long calculateTotalWork(AddFolderToZipTaskParameters taskParameters) t
return calculateWorkForFiles(filesToAdd, taskParameters.zipParameters);
}

private void setDefaultFolderPath(AddFolderToZipTaskParameters taskParameters) {
private void setDefaultFolderPath(AddFolderToZipTaskParameters taskParameters) throws IOException {
Copy link
Owner

Choose a reason for hiding this comment

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

Is it necessary to also change this method? I think just changing it above in AbstractExtractFileTask.java is enough?

Copy link
Contributor Author

@LeeYoung624 LeeYoung624 Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Currently this change will not work because the getDefaultFolderPath is only used when calling getRelativeFileName in my following change.
But this setDefaultFolderPath here will set default folder path to some path like /home/test/testDir/../testDir if I'm adding a folder with not normalized folder path. I think this may lead to some other exceptions if some other method calls getDefaultFolderPath in future.
I think it's better to fix it this time. What do you think?

String rootFolderPath;
File folderToAdd = taskParameters.folderToAdd;
if (taskParameters.zipParameters.isIncludeRootFolder()) {
rootFolderPath = folderToAdd.getParentFile().getPath();
rootFolderPath = folderToAdd.getParentFile().getCanonicalPath();
} else {
rootFolderPath = folderToAdd.getAbsolutePath();
rootFolderPath = folderToAdd.getCanonicalPath();
}

taskParameters.zipParameters.setDefaultFolderPath(rootFolderPath);
Expand Down
57 changes: 31 additions & 26 deletions src/main/java/net/lingala/zip4j/util/FileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,41 +181,46 @@ public static List<File> getSplitZipFiles(ZipModel zipModel) throws ZipException
return splitZipFiles;
}

public static String getRelativeFileName(String file, String rootFolderPath) {
public static String getRelativeFileName(String file, String rootFolderPath) throws ZipException {
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as above also here: Is it necessary to also change this method? I think just changing it above in AbstractExtractFileTask.java is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary. Please refer to my following comment.


String fileName;
if (isStringNotNullAndNotEmpty(rootFolderPath)) {
File rootFolderFile = new File(rootFolderPath);
String rootFolderFileRef = rootFolderFile.getPath();
try {
String fileCanonicalPath = new File(file).getCanonicalPath();
if (isStringNotNullAndNotEmpty(rootFolderPath)) {
File rootFolderFile = new File(rootFolderPath);
String rootFolderFileRef = rootFolderFile.getCanonicalPath();

if (!rootFolderFileRef.endsWith(FILE_SEPARATOR)) {
rootFolderFileRef += FILE_SEPARATOR;
}
if (!rootFolderFileRef.endsWith(FILE_SEPARATOR)) {
rootFolderFileRef += FILE_SEPARATOR;
}

String tmpFileName = file.substring(rootFolderFileRef.length());
if (tmpFileName.startsWith(System.getProperty("file.separator"))) {
tmpFileName = tmpFileName.substring(1);
}
String tmpFileName = fileCanonicalPath.substring(rootFolderFileRef.length());
if (tmpFileName.startsWith(System.getProperty("file.separator"))) {
tmpFileName = tmpFileName.substring(1);
}

File tmpFile = new File(file);
File tmpFile = new File(fileCanonicalPath);

if (tmpFile.isDirectory()) {
tmpFileName = tmpFileName.replaceAll("\\\\", "/");
tmpFileName += ZIP_FILE_SEPARATOR;
} else {
String bkFileName = tmpFileName.substring(0, tmpFileName.lastIndexOf(tmpFile.getName()));
bkFileName = bkFileName.replaceAll("\\\\", "/");
tmpFileName = bkFileName + tmpFile.getName();
}
if (tmpFile.isDirectory()) {
tmpFileName = tmpFileName.replaceAll("\\\\", "/");
tmpFileName += ZIP_FILE_SEPARATOR;
} else {
String bkFileName = tmpFileName.substring(0, tmpFileName.lastIndexOf(tmpFile.getName()));
bkFileName = bkFileName.replaceAll("\\\\", "/");
tmpFileName = bkFileName + tmpFile.getName();
}

fileName = tmpFileName;
} else {
File relFile = new File(file);
if (relFile.isDirectory()) {
fileName = relFile.getName() + ZIP_FILE_SEPARATOR;
fileName = tmpFileName;
} else {
fileName = relFile.getName();
File relFile = new File(fileCanonicalPath);
if (relFile.isDirectory()) {
fileName = relFile.getName() + ZIP_FILE_SEPARATOR;
} else {
fileName = relFile.getName();
}
}
} catch (IOException e) {
throw new ZipException(e);
}

return fileName;
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/net/lingala/zip4j/AddFilesToZipIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public void testAddFileWithDifferentFileNameSetsTheNewFileName() throws IOExcept
assertThat(zipFile.getFileHeaders()).hasSize(1);
assertThat(zipFile.getFileHeader("/data/newfile.txt")).isNotNull();
assertThat(zipFile.getFileHeader("sample_text_large.txt")).isNull();
zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());
}

@Test
Expand Down
50 changes: 33 additions & 17 deletions src/test/java/net/lingala/zip4j/ExtractZipFileIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import net.lingala.zip4j.testutils.TestUtils;
import net.lingala.zip4j.testutils.ZipFileVerifier;
import net.lingala.zip4j.util.FileUtils;
import net.lingala.zip4j.util.InternalZipConstants;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Expand All @@ -34,7 +35,7 @@ public void testExtractAllStoreAndNoEncryptionExtractsSuccessfully() throws IOEx
ZipFile zipFile = new ZipFile(generatedZipFile);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -47,7 +48,7 @@ public void testExtractAllStoreAndZipStandardEncryptionExtractsSuccessfully() th
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -60,7 +61,7 @@ public void testExtractAllStoreAndAes128EncryptionExtractsSuccessfully() throws
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -73,7 +74,7 @@ public void testExtractAllStoreAndAes256EncryptionExtractsSuccessfully() throws
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -84,7 +85,7 @@ public void testExtractAllDeflateAndNoEncryptionExtractsSuccessfully() throws IO
ZipFile zipFile = new ZipFile(generatedZipFile);
zipFile.addFiles(FILES_TO_ADD);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -96,7 +97,7 @@ public void testExtractAllDeflateAndZipStandardEncryptionExtractsSuccessfully()
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -108,7 +109,7 @@ public void testExtractAllDeflateAndAes128EncryptionExtractsSuccessfully() throw
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -120,7 +121,7 @@ public void testExtractAllDeflateAndAes256EncryptionExtractsSuccessfully() throw
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD, zipParameters);

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

ZipFileVerifier.verifyFolderContentsSameAsSourceFiles(outputFolder);
verifyNumberOfFilesInOutputFolder(outputFolder, 3);
Expand All @@ -133,7 +134,7 @@ public void testExtractFileWithFileHeaderWithAes128() throws IOException {
zipFile.addFiles(FILES_TO_ADD, zipParameters);

FileHeader fileHeader = zipFile.getFileHeader("sample_text_large.txt");
zipFile.extractFile(fileHeader, outputFolder.getCanonicalPath());
zipFile.extractFile(fileHeader, outputFolder.getPath());

File[] outputFiles = outputFolder.listFiles();
assertThat(outputFiles).hasSize(1);
Expand All @@ -147,7 +148,7 @@ public void testExtractFileWithFileHeaderWithAes128AndInDirectory() throws IOExc
zipFile.addFolder(TestUtils.getTestFileFromResources(""), zipParameters);

FileHeader fileHeader = zipFile.getFileHeader("test-files/öüäöäö/asöäööl");
zipFile.extractFile(fileHeader, outputFolder.getCanonicalPath());
zipFile.extractFile(fileHeader, outputFolder.getPath());

File outputFile = getFileWithNameFrom(outputFolder, "asöäööl");
ZipFileVerifier.verifyFileContent(TestUtils.getTestFileFromResources("öüäöäö/asöäööl"), outputFile);
Expand All @@ -161,7 +162,7 @@ public void testExtractFileWithFileHeaderWithAes256AndWithANewFileName() throws

String newFileName = "newFileName";
FileHeader fileHeader = zipFile.getFileHeader("sample_text_large.txt");
zipFile.extractFile(fileHeader, outputFolder.getCanonicalPath(), newFileName);
zipFile.extractFile(fileHeader, outputFolder.getPath(), newFileName);

File outputFile = getFileWithNameFrom(outputFolder, newFileName);
ZipFileVerifier.verifyFileContent(TestUtils.getTestFileFromResources("sample_text_large.txt"), outputFile);
Expand All @@ -184,7 +185,7 @@ public void testExtractFileWithFileNameWithZipStandardEncryption() throws IOExce
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFolder(TestUtils.getTestFileFromResources(""), zipParameters);

zipFile.extractFile("test-files/sample_directory/favicon.ico", outputFolder.getCanonicalPath());
zipFile.extractFile("test-files/sample_directory/favicon.ico", outputFolder.getPath());

File outputFile = getFileWithNameFrom(outputFolder, "favicon.ico");
ZipFileVerifier.verifyFileContent(TestUtils.getTestFileFromResources("sample_directory/favicon.ico"), outputFile);
Expand All @@ -197,7 +198,7 @@ public void testExtractFileWithFileNameWithZipStandardEncryptionAndNewFileName()
zipFile.addFolder(TestUtils.getTestFileFromResources(""), zipParameters);

String newFileName = "newFileName";
zipFile.extractFile("test-files/sample_directory/favicon.ico", outputFolder.getCanonicalPath(), newFileName);
zipFile.extractFile("test-files/sample_directory/favicon.ico", outputFolder.getPath(), newFileName);

File outputFile = getFileWithNameFrom(outputFolder, newFileName);
ZipFileVerifier.verifyFileContent(TestUtils.getTestFileFromResources("sample_directory/favicon.ico"), outputFile);
Expand All @@ -211,7 +212,7 @@ public void testExtractFilesThrowsExceptionForWrongPasswordForAes() throws IOExc

try {
zipFile = new ZipFile(generatedZipFile, "WRONG_PASSWORD".toCharArray());
zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());
fail("Should throw an exception");
} catch (ZipException e) {
assertThat(e).isNotNull();
Expand All @@ -227,7 +228,7 @@ public void testExtractFilesThrowsExceptionForWrongPasswordForZipStandardAndDefl

try {
zipFile = new ZipFile(generatedZipFile, "WRONG_PASSWORD".toCharArray());
zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());
fail("Should throw an exception");
} catch (ZipException e) {
assertThat(e).isNotNull();
Expand All @@ -244,7 +245,7 @@ public void testExtractFilesThrowsExceptionForWrongPasswordForZipStandardAndStor

try {
zipFile = new ZipFile(generatedZipFile, "WRONG_PASSWORD".toCharArray());
zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());
fail("Should throw an exception");
} catch (ZipException e) {
assertThat(e).isNotNull();
Expand All @@ -263,7 +264,7 @@ public void testExtractFilesForAZipMadeWithZip4jv1AndStoreCompressionWithAES() t
public void testExtractFilesForZipFileWhileWithCorruptExtraDataRecordLength() throws IOException {
ZipFile zipFile = new ZipFile(getTestArchiveFromResources("corrupt_extra_data_record_length.zip"));

zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());

assertThat(zipFile.getFileHeaders()).hasSize(44);
assertThat(Files.walk(outputFolder.toPath()).filter(Files::isRegularFile)).hasSize(44);
Expand All @@ -280,6 +281,21 @@ public void testExtractFilesWithSetPasswordSetter() throws IOException {
zipFile.extractAll(outputFolder.getCanonicalPath());
}

@Test
public void testAddFolderWithNotNormalizedPath() throws IOException {
ZipFile zipFile = new ZipFile(generatedZipFile);
ZipParameters parameters = new ZipParameters();

String folderToAddPath = TestUtils.getTestFileFromResources("").getPath() + InternalZipConstants.FILE_SEPARATOR + ".." + InternalZipConstants.FILE_SEPARATOR + TestUtils.getTestFileFromResources("").getName();
File folderToAdd = new File(folderToAddPath);
zipFile.addFolder(folderToAdd, parameters);

File fileToAdd = TestUtils.getTestFileFromResources("file_PDF_1MB.pdf");
zipFile.addFile(fileToAdd, parameters);

ZipFileVerifier.verifyZipFileByExtractingAllFiles(generatedZipFile, outputFolder, 13);
}

private void verifyNumberOfFilesInOutputFolder(File outputFolder, int numberOfExpectedFiles) {
assertThat(outputFolder.listFiles()).hasSize(numberOfExpectedFiles);
}
Expand Down
8 changes: 8 additions & 0 deletions src/test/java/net/lingala/zip4j/MiscZipFileIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,14 @@ public void testZipSlipFix() throws Exception {
}
}

@Test
public void testUnzipFileZipSlipWithNotNormalizedTarget() throws IOException {
ZipFile zipFile = new ZipFile(generatedZipFile, PASSWORD);
zipFile.addFiles(FILES_TO_ADD);
zipFile.extractAll(new File(outputFolder.getPath(),
".." + InternalZipConstants.FILE_SEPARATOR + outputFolder.getName()).getAbsolutePath());
}

@Test
public void testExtractFileDeletesOutputFileWhenWrongPassword() throws IOException {
ZipParameters zipParameters = createZipParameters(EncryptionMethod.ZIP_STANDARD, AesKeyStrength.KEY_STRENGTH_256);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/net/lingala/zip4j/testutils/ZipFileVerifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public static void verifyZipFileByExtractingAllFiles(File zipFileToExtract, char
assertThat(zipFileToExtract).exists();

ZipFile zipFile = new ZipFile(zipFileToExtract, password);
zipFile.extractAll(outputFolder.getCanonicalPath());
zipFile.extractAll(outputFolder.getPath());
assertThat(zipFile.getFileHeaders().size()).as("Number of file headers").isEqualTo(expectedNumberOfEntries);

List<File> extractedFiles = FileUtils.getFilesInDirectoryRecursive(outputFolder, true, true);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/net/lingala/zip4j/util/FileUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ public void testGetSplitZipFilesReturnsValidWhenSplitFile() throws ZipException
}

@Test
public void testGetRelativeFileNameWhenRootFoldersAreNull() {
public void testGetRelativeFileNameWhenRootFoldersAreNull() throws ZipException {
assertThat(FileUtils.getRelativeFileName("somefile.txt", null)).isEqualTo("somefile.txt");
}

Expand Down