Repository navigation
Fix Windows case sensitive zip extraction - #131202
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression |
There was a problem hiding this comment.
Pull request overview
This PR hardens System.IO.Compression.ZipFile extraction path validation to prevent a traversal-style escape that can occur when the destination-root containment check is case-insensitive but the underlying filesystem treats differently-cased sibling directories as distinct.
Changes:
- Tighten
ZipArchiveEntryextraction containment validation: after resolving the full destination path, require both the existing platform comparison and an ordinal root-prefix match on case-insensitive platforms to detect case-only sibling escapes. - Add regression coverage for the case-insensitive sibling scenario (e.g., extracting
../dest/pwn.txtinto a root namedDest). - Add tests ensuring benign
..(that resolves back inside the root) and.segments still extract successfully.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs | Adds tests for the newly rejected case-insensitive sibling escape and for allowed . / in-root .. segments. |
| src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.cs | Strengthens destination-root prefix validation by adding an ordinal root-prefix requirement on case-insensitive platforms. |
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
Holding for offline feedback.
Sent reference materials & framing documents offline. Unblocking. (I've not reviewed the PR.)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.cs:239
- The rationale comment is a bit misleading: a resolved path can share the root's exact casing and still have traversed outside the root and back in (e.g. "../Dest/x"). The important property you're enforcing is preventing escape + re-descent into a differently-cased sibling on case-insensitive platforms. Rewording this avoids baking incorrect reasoning into a security-sensitive check.
// Reject entries that resolve outside the destination root. GetFullPath collapses "." and ".."
// but never re-cases the segments it keeps. The root is combined in verbatim with a trailing
// separator. That means a resolved path that shares the root's exact casing never climbed above the root;
// one that matches the root only case-insensitively did climb out and re-descend under a
// different spelling (e.g. "../dest/x" into root "Dest"), which is a distinct directory on a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.cs:231
- The new destination-boundary check fails when
fullDestinationalready ends with a directory separator (notably for drive/share roots likeC:\or\\server\share\). In that casefileDestinationPath[fullDestination.Length]is the first character of the next segment (or the path ends), so extraction incorrectly throwsIO_ExtractingResultsInOutsidefor valid in-root entries.
To keep the intended ordinal (case-sensitive) check while handling roots and avoiding prefix collisions (e.g., Dest vs Destinations), compare against a destination prefix that always ends in a separator, and allow the normalized path to equal the destination itself (for directory entries like . / subdir/..).
// Ensure the path stays within the destination directory boundary
if (!fileDestinationPath.StartsWith(fullDestination, StringComparison.Ordinal) ||
fileDestinationPath.Length <= fullDestination.Length ||
fileDestinationPath[fullDestination.Length] != Path.DirectorySeparatorChar)
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs:109
- The comment uses present tense to describe a “case-insensitive” destination-root prefix check, but the implementation under test now uses
StringComparison.Ordinal. Rewording this as historical behavior avoids confusion about what the current code does and why the test exists.
// An entry that normalizes into a differently-cased sibling of the destination root must be
// rejected. On case-insensitive platforms (Windows, macOS, iOS, tvOS) the destination-root
// prefix check is case-insensitive, so extracting "../dest/pwn.txt" into a root named "Dest"
// would otherwise be treated as staying inside the root even though the file system can keep
// "Dest" and "dest" as distinct directories.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.cs:235
- The boundary check intentionally uses StringComparison.Ordinal (instead of platform-specific comparison) to handle directories that are case-sensitive even on typically case-insensitive platforms. Consider capturing that rationale in the comment so it isn’t accidentally reverted later.
// Ensure the path stays within the destination directory boundary.
src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs:109
- This comment describes the destination-root prefix check as case-insensitive on Windows/macOS, but the implementation now uses an Ordinal comparison. Updating the wording to focus on per-directory/volume case sensitivity avoids the comment going stale/misleading.
// An entry that normalizes into a differently-cased sibling of the destination root must be
// rejected. On case-insensitive platforms (Windows, macOS, iOS, tvOS) the destination-root
// prefix check is case-insensitive, so extracting "../dest/pwn.txt" into a root named "Dest"
// would otherwise be treated as staying inside the root even though the file system can keep
// "Dest" and "dest" as distinct directories.
|
/ba-g failures not related to my change |
Fixes dotnet#131066 Update checks for zip extraction to better protect case sensitive environments.
Fixes #131066
Update checks for zip extraction to better protect case sensitive environments.