Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Sanitize symlink target#768

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
e3krisztian wants to merge3 commits intomain
base:main
Choose a base branch
Loading
fromsanitize_symlink_target
Draft

Conversation

@e3krisztian
Copy link
Contributor

@e3krisztiane3krisztian commentedFeb 14, 2024•
edited
Loading

Split off of#763 . There are still problems to solve here, see954c1cd#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 ofPathlib asPathlib.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 aroundMaliciousSymlinks, 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.

e3krisztian referenced this pull requestFeb 14, 2024
We can rewrite symlinks to ensure they are always relativeand remain within the extraction directory.
@qkaiserqkaiser self-requested a reviewFebruary 16, 2024 09:07
Copy link
Contributor

@qkaiserqkaiser 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.

@qkaiserqkaiser marked this pull request as draftFebruary 16, 2024 09:12
@AndrewFasano
Copy link

AndrewFasano commentedFeb 21, 2024•
edited
Loading

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

@e3krisztian
Copy link
ContributorAuthor

e3krisztian commentedFeb 22, 2024•
edited
Loading

@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 thetar fix covering these problems are merged.

qkaiser reacted with thumbs up emoji

@vlacivlaciforce-pushed themain branch 4 times, most recently fromfe05dec to064e1adCompareFebruary 4, 2025 17:10
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@qkaiserqkaiserqkaiser left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@e3krisztian@AndrewFasano@qkaiser
[8]ページ先頭
©2009-2025 Movatter.jp