-
Notifications
You must be signed in to change notification settings - Fork 9
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 zip slip vulnerability, other zip issues and tests #93
base: trunk
Are you sure you want to change the base?
Conversation
Hi @reimic, thank you for this PR. As I was reading, I found it a bit difficult to review due to multiple purposes in the changes:
Would you be willing to break this into multiple single-purpose PRs so we can focus more clearly on each issue? |
Sure, @brandonpayton - let's start here: #94 |
Annnd, @brandonpayton - then progress to here: #95 |
tests/unit/zip/ZipFunctionsTest.php
Outdated
|
||
$filesystem->remove( dirname( $filename ) ); | ||
$slipped_file = Path::canonicalize(__DIR__ . "../../../../../../../../tmp/zip-slip-test.txt"); | ||
self::assertFileDoesNotExist( $slipped_file ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checks for a single, very specific path. Why not test for a single, simple case like ../tmp-zip-slip-test.txt
? And then confirm where the file was actually created – if anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's test for a path starting with /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's test for zipped symlinks
🚧 Work in progress 🚧
What does this PR do?
zip_extract_to
method will now throw an exception if it encounters files with a relative path Zip Slip Vulnerability #73 ( I recommend throwing an exception instead of skipping dangerous files in zips. I was swayed by the argument that letting only some files through breaks user-data integrity.)What problem does it fix?
zip_extract_to
methodHow to test if it works?