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

Sanitize symlink target #768

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Sanitize symlink target #768

wants to merge 3 commits into from

Conversation

e3krisztian
Copy link
Contributor

@e3krisztian e3krisztian commented Feb 14, 2024

Split off of #763 . There are still problems to solve here, see 954c1cd#commitcomment-138623089 but tests should run with the exception of 2 failures.

954c1cd rewrites the logic to sanitize symlinks to be relative and kept within the extraction directory. This is done using the os module instead of Pathlib as Pathlib.resolve would fail if a symlink target was missing (which doesn't prevent us from safely converting it to a relative link). With this change I no longer see false positives around MaliciousSymlinks, instead symlinks are created safely within the extraction directory. If a relative symlink originally tried accessing a directory above its own root (i.e., ./bin/sh -> ../../../../../bin/bash), we update the link so it remains within the extraction directory.

Andrew Fasano and others added 3 commits February 14, 2024 16:44
@e3krisztian e3krisztian force-pushed the sanitize_symlink_target branch from fd0365f to cd167e8 Compare February 14, 2024 15:58
e3krisztian referenced this pull request Feb 14, 2024
We can rewrite symlinks to ensure they are always relative
and remain within the extraction directory.
@qkaiser qkaiser self-requested a review February 16, 2024 09:07
Copy link
Contributor

@qkaiser qkaiser left a comment

Choose a reason for hiding this comment

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

I'll mark the PR as draft until ruff is back and we have a cleaner view on what are the logic changes being applied.

unblob/extractor.py Show resolved Hide resolved
unblob/extractor.py Show resolved Hide resolved
unblob/extractor.py Outdated Show resolved Hide resolved
@qkaiser qkaiser marked this pull request as draft February 16, 2024 09:12
@AndrewFasano
Copy link

AndrewFasano commented Feb 21, 2024

I believe there is still a bug in b11fe46: when extracting in a (host) directory within /tmp/, a symlink of /var pointing to /tmp/var will be sanitize to point to var creating a symlink loop. This seems to be caused by the use of os.path.commonpath finding a similarity between the extraction directory and the absolute symlink target.

@e3krisztian
Copy link
Contributor Author

e3krisztian commented Feb 22, 2024

@qkaiser I wanted to make this PR just to have a place to discuss. It was extracted from a larger PR, and wanted to see CI test results, which our code checks (ruff) prevented, to push through the commit I have made some hacky fixes to be thrown away in the final version.

This definitely needs a rewrite, so should have made it draft initially.

I do not plan working on this soon, especially as the tar fix covering these problems are merged.

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.

3 participants