Conversation
src/Tasks/Unzip.cs
Outdated
|
|
||
| FileInfo destinationPath = new FileInfo(Path.Combine(destinationDirectory.FullName, zipArchiveEntry.FullName)); | ||
| string fullDestinationPath = Path.GetFullPath(Path.Combine(destinationDirectory.FullName, zipArchiveEntry.FullName)); | ||
| string fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory.FullName + Path.DirectorySeparatorChar); |
There was a problem hiding this comment.
Can you pull this to outside the loop?
There was a problem hiding this comment.
The documentation seemed to want each ZipArchiveEntry checked individually, presumably because even if one entry is at a good path, the next could be at ..\..\..\..., so I don't think so.
There was a problem hiding this comment.
fullDestinationDirectoryPath will be identical for each pass through the loop, so I'd rather compute it only once. fullDestinationPath will vary and must be computed for each file.
There was a problem hiding this comment.
Ah, yeah, good point. I can do that.
src/Tasks/Resources/Strings.resx
Outdated
| <data name="Unzip.DidNotUnzipBecauseOfFilter"> | ||
| <value>Did not unzip file "{0}" because it didn't match the include filter or because it matched the exclude filter.</value> | ||
| </data> | ||
| <data name="Unzip.ZipSlipExploit">\ |
There was a problem hiding this comment.
| <data name="Unzip.ZipSlipExploit">\ | |
| <data name="Unzip.ZipSlipExploit"> |
| /// <param name="destinationDirectory">The <see cref="DirectoryInfo"/> to extract files to.</param> | ||
| private void Extract(ZipArchive sourceArchive, DirectoryInfo destinationDirectory) | ||
| { | ||
| string fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory.FullName + Path.DirectorySeparatorChar); |
There was a problem hiding this comment.
Check that FullName doesn't already end in DirectorySeperatorChar, having two runs the risk of upsetting some file systems
Fixes AB#1645160
Context
We should check whether a zip archive is attempting to unzip into a parent directory, as that would be a security risk.
Changes Made
Fail if the user is attempting to unzip to a location the user has not given them permission to unzip to.
Testing
Notes