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

merged 3 commits into from
Sep 12, 2019

Conversation

LeeYoung624
Copy link
Contributor

These Pull Requests contain the following content:
1.fix zip slip in issue #62
2.revert getCanonicalPath back to getPath in commit #44 (19 occurrences in total)
3.fix exceptions in adding files caused by not normalized file path. Including:
(1) fix getRelativeFileName in FileUtils.java by changing input file and rootFolderPath into canonical path. Throw new ZipException when catch IOException thrown by getCanonicalPath()
(2) adding new testcase testUnzipFileZipSlipWithNotNormalizedTarget in MiscZipFileIT.java for this

I also checked all the occurrences of getPath() and getAbsolutePath(). I think they are working correctly and would not cause exceptions right now. Maybe changing all of them to getCanonicalPath() is a good idea, but I'm not sure about this.

@LeeYoung624
zip slip fix
0693bb4 
zip slip fix. Both the path being compared and comparing should use getCanonicalPath
@LeeYoung624
fix exceptions that may caused by not normalized
46a6916 
fix excepitons that may occur when adding files or folder with not normalized path
srikanth-lingala
srikanth-lingala requested changes Sep 11, 2019
View reviewed changes
src/main/java/net/lingala/zip4j/tasks/AddFolderToZipTask.java
@@ -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?

src/main/java/net/lingala/zip4j/util/FileUtils.java
@@ -181,41 +181,46 @@ public static String getZipFileNameWithoutExtension(String zipFile) throws ZipEx
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.

src/test/java/net/lingala/zip4j/MiscZipFileIT.java Outdated
zipFile.extractAll(new File(outputFolder.getPath(),
".." + InternalZipConstants.FILE_SEPARATOR + outputFolder.getName()).getAbsolutePath());
} catch (ZipException e) {
fail("Should not throw an exception");
Copy link
Owner

Choose a reason for hiding this comment

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

This is not necessary. If there is an exception, this method will fail anyway. Please remove the catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

src/test/java/net/lingala/zip4j/ExtractZipFileIT.java Outdated
zipFile.addFile(fileToAddPath, parameters);
ZipFileVerifier.verifyZipFileByExtractingAllFiles(generatedZipFile, outputFolder, 13);
} catch (IOException e) {
fail("Should not throw an exception");
Copy link
Owner

Choose a reason for hiding this comment

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

This is not necessary. If there is an exception, this method will fail anyway. Please remove the catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@srikanth-lingala srikanth-lingala mentioned this pull request Sep 11, 2019
Closed
@LeeYoung624
Copy link
Contributor Author

LeeYoung624 commented Sep 12, 2019 •
edited
Loading

The changes in AddFolderToZipTask.java and FileUtils.java are necessary. But I'm sorry that I made some mistake in the unit test testAddFolderWithNotNormalizedPath.
It should be like this:

@Test
  public void testAddFolderAndFileWithCanonicalPath() 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);
    zipFile.addFile(TestUtils.getTestFileFromResources("file_PDF_1MB.pdf"), parameters);
    ZipFileVerifier.verifyZipFileByExtractingAllFiles(generatedZipFile, outputFolder, 13);
  }

This test will throw an exception in getRelativeFileName in FileUtils.java when executing removeFilesIfExists. The above 2 changes in setDefaultFolderPath and getRelativeFileName are to fix this exception.

Currently, the change in setDefaultFolderPath 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?

@LeeYoung624
revert getCanonicalPath back to getPath in tests
75de1a7 
revert getCanonicalPath back to getPath in tests in commit srikanth-lingala#44. 19 occurrences in total need to be reverted
@srikanth-lingala srikanth-lingala self-assigned this Sep 12, 2019
@srikanth-lingala srikanth-lingala changed the title Canonical path fix #62 Canonical path fix Sep 12, 2019
@srikanth-lingala srikanth-lingala changed the title #62 Canonical path fix Canonical path fix Sep 12, 2019
@srikanth-lingala srikanth-lingala changed the title Canonical path fix #62 Canonical path fix Sep 12, 2019
@srikanth-lingala srikanth-lingala merged commit 3f15884 into srikanth-lingala:master Sep 12, 2019
@LeeYoung624 LeeYoung624 deleted the canonical_path_fix branch September 16, 2019 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanth-lingala srikanth-lingala srikanth-lingala approved these changes

Assignees

@srikanth-lingala srikanth-lingala

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LeeYoung624 @srikanth-lingala