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

Delay creating symlinks until after all regular files #23

Merged
merged 1 commit into from
Sep 3, 2021
Merged

Conversation

CGA1123
Copy link
Owner

@CGA1123 CGA1123 commented Sep 3, 2021

When decompressing a tarball there isn't any guarantee of the order
which tar headers are presented to us by (*tar.Reader).Next(), this
means that it's possible for us to evaluate and try to create a symlink
before the file which it points to has been created.

This is usually fine, a symlink isn't required to point to anything.
However, for security reasons (mitigating against "Zip-slip") we wan't
to evaluate that a symlink resolves to a file that is within the
expected directory (i.e. it should not escape outside of the directory
which we are decompressing into).

For this reason, we need to collect all symlinks and process them after
we've gone through all regular files.

There might still be a chance of problems being caused by symlinks that
point to other symlinks -- I'll cross that bridge if I get to it.

When decompressing a tarball there isn't any guarantee of the order
which tar headers are presented to us by (*tar.Reader).Next(), this
means that it's possible for us to evaluate and try to create a symlink
before the file which it points to has been created.

This is usually fine, a symlink isn't required to point to anything.
However, for security reasons (mitigating against "Zip-slip") we wan't
to evaluate that a symlink resolves to a file that is within the
expected directory (i.e. it should not escape outside of the directory
which we are decompressing into).

For this reason, we need to collect all symlinks and process them after
we've gone through all regular files.

There might still be a chance of problems being caused by symlinks that
point to other symlinks -- I'll cross that bridge if I get to it.
@CGA1123 CGA1123 self-assigned this Sep 3, 2021
@CGA1123 CGA1123 added the run-acceptance-tests Trigger a PR to run acceptance test suite label Sep 3, 2021
@CGA1123 CGA1123 merged commit 0eb358d into main Sep 3, 2021
@CGA1123 CGA1123 deleted the symlink-fix branch September 3, 2021 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-acceptance-tests Trigger a PR to run acceptance test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant