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

feat: cannot extract files outside the target directory #205

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

letFunny
Copy link
Collaborator

  • Have you signed the CLA?

This commits adds several security controls and tests so that files
cannot be extracted outside of the target directory, and hardlinks
cannot point to files outside of the target directory.

This commits adds several security controls and tests so that files
cannot be extracted outside of the target directory, and hardlinks
cannot point to files outside of the target directory.
Copy link

github-actions bot commented Feb 14, 2025

Command Mean [s] Min [s] Max [s] Relative
BASE 8.122 ± 0.021 8.090 8.151 1.00
HEAD 8.461 ± 0.038 8.396 8.520 1.04 ± 0.01

}},
},
},
error: `cannot extract from package "test-package": cannot create path /[a-z0-9\-\/]*/file outside of root /[a-z0-9\-\/]*`,
Copy link
Member

Choose a reason for hiding this comment

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

/[a-z0-9-/]*/file

What is this path? Is it to hide the root?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes indeed

Comment on lines +16 to +17
// Root defaults to "/" if empty.
Root string
Copy link
Member

Choose a reason for hiding this comment

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

Won't that be a bit risky to have / as the default? What if Chisel overwrites the files in the system?

How about /var/lib/chisel or $PWD? The snap may not be able to write to the former.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the root of the filesystem, I am not sure we can provide a good default as users may choose whatever folder they wish. But, more importantly, I put the root as / because this is a general API and, in my opinion, the default shouldn't correspond to one use-case.

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.

2 participants