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.

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

Fix regression when extracting tar archive #816

Merged
merged 2 commits into from Sep 5, 2019
Merged

Fix regression when extracting tar archive #816

merged 2 commits into from Sep 5, 2019

Conversation

arminha
Copy link
Contributor

@arminha arminha commented May 17, 2019

Summary

Commit 93d77ff introduced a check that files extracted from a tar archive will not be written outside of the destination directory. Unfortunately it also introduced a regression prventing the extraction of any tar archive when the path to the destination directory contains a symbolic link.

For example on a jenkins agent where /var/lib/jenkins is a symbolic link to /data/jenkins and a job tries to extact nodejs to /var/lib/jenkins/workspace/example-project/target/node/tmp. The old code would then check if canonical path to a tar entry like /data/jenkins/workspace/example-project/target/node/tmp/XXX starts with /var/lib/jenkins/workspace/example-project/target/node/tmp which always fails.

The new check compares the canonical extraction paths of the tar entries with the canonical path of the destination directory. This also works when the destination directory path contains a symbolic link and still protects against extracting files outside the destination directory.

…on directory

Commit 93d77ff introduced a check that
files extracted from a tar archive will not be written outside of the
destination directory. Unfortunately it also introduced a regression
prventing the extraction of any tar archive when the path to the
destination directory contains a symbolic link.

For example on a jenkins agent where /var/lib/jenkins is a symbolic
link to /data/jenkins and a job tries to extact nodejs to
/var/lib/jenkins/workspace/example-project/target/node/tmp. The old
code would then check if canonical path to a tar entry like
/data/jenkins/workspace/example-project/target/node/tmp/XXX starts
with /var/lib/jenkins/workspace/example-project/target/node/tmp which
always fails.

This commit compares the canonical extraction paths of the tar entries
with the canonical path of the destination directory, which fixes the
regression and still checks that no file is extracted outside of the
destination directory.
@DiabolusExMachina
Copy link

Could this please get merged? I cannot build my projects anymore this is kind of really bad...

@eirslett
Copy link
Owner

eirslett commented Sep 5, 2019

Ok, looks good.

@eirslett eirslett merged commit b50455d into eirslett:master Sep 5, 2019
while (tarEntry != null) {
// Create a file for this tarEntry
final File destPath = new File(destinationDirectory + File.separator + tarEntry.getName());
prepDestination(destPath, tarEntry.isDirectory());
if (!destPath.getCanonicalPath().startsWith(destinationDirectory)) {
if (!destPath.getCanonicalPath().startsWith(canonicalDestinationDirectory)) {
Copy link

@crisu crisu Dec 2, 2019

Choose a reason for hiding this comment

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

this fix was lost in 26 aug 2019

if (!startsWithPath(destPath.getCanonicalPath(), destinationDirectory)) {

@crisu
Copy link

crisu commented Dec 2, 2019

i think this fix was lost in 26 August

if (!startsWithPath(destPath.getCanonicalPath(), destinationDirectory)) {

@arminha
Copy link
Contributor Author

arminha commented Dec 5, 2019

@crisu: you're right. It was indeed forgotten. Probably my fault for not writing tests 😉

Please have a look at #864. It should fix the issue again and also adds some unit tests.

@eirslett
Copy link
Owner

There we go, 1.9.0 is released, merry christmas! 🎅 ❄️ 🎄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants