Uh oh!
There was an error while loading. Please reload this page.
Fix zip slip vulnerability, other zip issues and tests - #93
Conversation
brandonpayton
commented
Mar 26, 2024
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? |
reimic
commented
Mar 27, 2024
Sure, @brandonpayton - let's start here: #94 |
reimic
commented
Mar 27, 2024
Annnd, @brandonpayton - then progress to here: #95 |
| $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.
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.
Also let's test for a path starting with /
There was a problem hiding this comment.
Also let's test for zipped symlinks
🚧 Work in progress 🚧
What does this PR do?
zip_extract_tomethod 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_tomethodHow to test if it works?