Fix #3907 tar file placed outside of target location

[wilsonfv]

Add a warning if one of the tar file will be placed outside of target location causing potential security issue.

Fixes #3907.

[cjerdonek]

If we detect a file being written outside of the location, I think we should fail rather than warn.

[wilsonfv]

If we detect a file being written outside of the location, I think we should fail rather than warn.

we could fail it immediately, i am just not sure if its a bug or a design issue so just a warning in case i break anything

we could roll out this changes if no further complain, we can then fail it in next release
what do you think @cjerdonek

[pradyunsg]

I like the idea of making this an error message now and then breaking behavior in the future.

[cjerdonek]

Since this is broken behavior and also a security issue, I think we should simply prevent this from happening if we're already going to the trouble of detecting it. Also, if you read the original report, it said,

pip processes invalid packages .tar/.zip archives containing "../" directory traversal chars and places package files outside of target directories even after pip complains and throws errors and aborts the install.

Especially if pip is already erroring out, it seems like it doesn't really help things to log a warning in addition to the error. Pip is already throwing an error.

[cjerdonek]

By the way, when I asked about possible backwards compatibility concerns on the original issue, I was also thinking about things like paths that might resolve to something within the target directory, but that contained ... In other words, we don't just want to rule out any path that contains ... Resolving the path and collapsing occurrences of .. (as this PR does) before doing a path comparison to see if it lies outside of the target directory addresses that backwards compatibility issue I was concerned about.

[xavfernandez]

Since this is broken behavior and also a security issue, I think we should simply prevent this from happening if we're already going to the trouble of detecting it.

I agree.

[pradyunsg]

Since this is broken behavior and also a security issue, I think we should simply prevent this from happening if we're already going to the trouble of detecting it.

Let's do this then. :D

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[cjerdonek]

One thing I would like to see as part of this PR is a helper function to create a zip file containing files with given paths.

Then, there can be two tests of unzip_file() side-by-side: one testing a zip file whose paths are all okay, and one testing a zip file with a path that would go outside the target directory. Only the latter should fail, and it should fail with the right error message.

And similarly for a tar file.

I think this would provide better assurance that the check is working correctly than just testing the correctness of a helper function used inside unzip_file() and untar_file().

[wilsonfv]

One thing I would like to see as part of this PR is a helper function to create a zip file containing files with given paths.

Then, there can be two tests of unzip_file() side-by-side: one testing a zip file whose paths are all okay, and one testing a zip file with a path that would go outside the target directory. Only the latter should fail, and it should fail with the right error message.

And similarly for a tar file.

I think this would provide better assurance that the check is working correctly than just testing the correctness of a helper function used inside unzip_file() and untar_file().

this is now a breaking behavior
and unit test cases have been added. The added test.zip and test.tgz files contain .. path

[cjerdonek]

I had suggested creating a helper function to create the zip and tar files, and also including the success case and not just the failure case. I think it’s better not to check in binary files, especially if it’s easy to avoid.

[wilsonfv]

I had suggested creating a helper function to create the zip and tar files, and also including the success case and not just the failure case. I think it’s better not to check in binary files, especially if it’s easy to avoid.

The existing test_unpack_tgz and test_unpack_zip already validate the successful cases

[cjerdonek]

Not with the helper function I’m suggesting that you create. I think it’s important to have a test case to show that the zip files it creates are valid. It won’t even be any extra work. Just calling the same test function but with a different input argument.

[wilsonfv]

Not with the helper function I’m suggesting that you create. I think it’s important to have a test case to show that the zip files it creates are valid. It won’t even be any extra work. Just calling the same test function but with a different input argument.

Sorry, I am not sure what you mean. There seems to be no existing test case that creates a zip file
Could you point me to the source code you are referring to

[cjerdonek]

Yes, that’s why I said the function needs to be created. Python’s zipfile module provides that ability: https://docs.python.org/3/library/zipfile.html

[wilsonfv]

Yes, that’s why I said the function needs to be created. Python’s zipfile module provides that ability: https://docs.python.org/3/library/zipfile.html

We have prepared the zip files in the data package for the test cases. I do not see creating a zip file at run time make any difference

[cjerdonek]

There are a number of differences. Some of the advantages of doing it at runtime are — We don’t need to check in binary files, which I want to avoid if we can. There’s no easy way for reviewers to review the contents of a binary file or know by looking at it what properties it has. It’s harder to update a binary file if we need to make changes to it. Binary also makes it harder to create more test cases: with runtime you can test whatever paths or properties you need to very easily. For example, maybe we want to test different types of characters in the file names in addition to “..”, or paths containing “..” that resolve to inside the target. There are different test cases even with this particular bug alone.

[wilsonfv]

@cjerdonek @pradyunsg please review changes

[chrahunt]

Sorry if I'm missing something - I see several comments and some discussion about zip file test helpers and other higher-level tests (which is what I was going to comment on for this PR), but the diff for all commits only shows tests of is_within_directory. Is this expected, or are there tests/changes missing?

[cjerdonek]

Hmm, I seem to remember there being issues with some of the rebases. The code from before isn't present anymore. :( @wilsonfv, do you have your old commits? If not, we could try to help you restore those using Git's reflog.

[cjerdonek]

Some portions of the previous code are visible if you click to expand the (outdated) comments of the previous iterations.

[chrahunt]

If you expand the unchanged sections of test_utils.py in the commit diff here it shows the tests I think are now missing.

[wilsonfv]

If you expand the unchanged sections of test_utils.py in the commit diff here it shows the tests I think are now missing.

@chrahunt this has been added back, thanks for checking

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[nicferrier]

Guys? can you please merge this? pip is continually blocked by security tooling because of this silly little bug.

It will make a lot of people's live's easier if it's merged before poor Wilson has to do another load of work to keep the PR in line!

Thanks!

[chrahunt]

This looks good to me. 👍 Just a few very minor style comments.

[pradyunsg]

The code change looks fine to me -- I second @chrahunt's style change suggestions.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[chrahunt]

Rebased and addressed review comments. IMO this can be merged once the checks pass.

[chrahunt]

I reviewed the existing comments and believe all of them have been addressed, so I will merge this. If I missed anything, we can definitely address it in a follow up.

Thanks @wilsonfv for your time and patience. :) We greatly appreciate your contribution.