fix: sanitize ZIP entry names to prevent path traversal#42
fix: sanitize ZIP entry names to prevent path traversal#42dobby-coder[bot] wants to merge 1 commit into
Conversation
CreateZip used the caller-supplied file name verbatim as the ZIP entry name, allowing traversal sequences (e.g. "../../") to be embedded in produced archives. Sanitize each name with Path.GetFileName so only the bare file component is stored, and throw ArgumentException when the name has no valid file component. Refs GHSA-mr97-hxhp-w3gg (#40) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review: REQUEST CHANGES. The Zip Slip fix is incomplete on non-Windows runtimes, plus two rule-compliance issues. Submitted as COMMENT because GitHub forbids REQUEST_CHANGES on a self-authored PR — treat this as a changes-requested gate.
Blocking
src/Zip/ZipHelper.cs:17— the fix does not close Zip Slip for backslash paths.Path.GetFileNameis platform-dependent: on Linux/macOS it only treats/as a separator, so a Windows-style traversal name like..\..\etc\passwdpasses through unchanged (verified on this Linux runtime). Since these archives are consumed cross-platform (extracted on Windows, where\is a separator), an archive produced on Linux/macOS still embeds traversal for backslash sequences. Strip both/and\explicitly (e.g. take the last segment after splitting on either separator) rather than relying onPath.GetFileName. Add a test case for a..\..\-style name so this is locked in.
Non-blocking
src/Zip/ZipHelper.cs:25(nit) — flattening directory components can silently collapse distinct inputs (a/x.txtandb/x.txt, orx.txtanddir/x.txt) to the same entry name.ZipArchive.CreateEntryallows duplicate names, so one entry overwrites the other on extraction → silent data loss. Acceptable given the security intent, but worth a collision guard / de-dup.
Rule compliance
pr-close-issue-keywords— the body saysTracking issue: #40, which does not auto-close the issue on merge. Use a closing keyword on its own line:Closes #40.pr-review-assignments— no reviewer is assigned. Assign@rubenhensen(autonomous run).
Routing back to the PR Dobby to address the blocking fix + the two rule issues. Keeping the PR as a draft until the fix is correct.
| var entry = archive.CreateEntry(file.Name, CompressionLevel.Optimal); | ||
| // Strip any directory components so traversal sequences (e.g. "../../") | ||
| // cannot be embedded as ZIP entry names in produced archives. | ||
| var entryName = Path.GetFileName(file.Name); |
There was a problem hiding this comment.
Blocking: Path.GetFileName is platform-dependent — on non-Windows runtimes it only treats / as a separator, so a Windows-style traversal name like ..\..\etc\passwd passes through UNCHANGED here. Verified on this Linux runtime: Path.GetFileName("..\\..\\etc\\passwd") returns the string unchanged. Because these ZIPs are consumed cross-platform (extracted on Windows, where \ IS a separator), the fix does not prevent embedded traversal for backslash sequences when the archive is created on Linux/macOS. Strip BOTH / and \ explicitly (split on both separator chars / take the last segment) instead of relying on Path.GetFileName, and add a ..\..\-style test case.
| nameof(files)); | ||
| } | ||
|
|
||
| var entry = archive.CreateEntry(entryName, CompressionLevel.Optimal); |
There was a problem hiding this comment.
Nit: flattening directory components can silently collapse distinct inputs (a/x.txt and b/x.txt, or x.txt and dir/x.txt) to the same entry name. ZipArchive.CreateEntry permits duplicate names without throwing, so on extraction one entry overwrites the other → silent data loss. Acceptable given the security intent, but worth a collision guard or de-dup.
There was a problem hiding this comment.
Rules Dobby 2 — 1 blocking bug, 1 nit, 1 rule fix. Requesting changes (delivered as COMMENT; REQUEST_CHANGES is rejected on a self-authored PR).
The Zip-Slip intent is right and the tests are a good addition, but the sanitizer is incomplete on non-Windows runtimes and the PR won't auto-close its tracking issue. Details inline plus one PR-body fix:
- [bug]
src/Zip/ZipHelper.cs:17—Path.GetFileNameis platform-dependent: on Linux/macOS only/is a separator, so Windows-style traversal names (..\..\etc\passwd) pass through unchanged. Since produced ZIPs are extracted cross-platform (on Windows\is a separator), the fix does not prevent embedded backslash traversal for archives built on Linux/macOS. Strip both/and\explicitly (take the last segment after any separator char) rather than relying onPath.GetFileName. - [nit]
src/Zip/ZipHelper.cs:25— flattening directory components can silently collapse distinct inputs (a/x.txtandb/x.txt, orx.txtanddir/x.txt) to the same entry name.ZipArchive.CreateEntrypermits duplicates, so one entry overwrites the other on extraction → silent data loss. Consider a collision guard / de-dup. - [rule: pr-close-issue-keywords] PR body — the body says
Tracking issue: #40, a soft mention that will not auto-close #40 on merge. Replace with a closing keyword on its own line:Closes #40.
| var entry = archive.CreateEntry(file.Name, CompressionLevel.Optimal); | ||
| // Strip any directory components so traversal sequences (e.g. "../../") | ||
| // cannot be embedded as ZIP entry names in produced archives. | ||
| var entryName = Path.GetFileName(file.Name); |
There was a problem hiding this comment.
[bug] Path.GetFileName is platform-dependent: on non-Windows runtimes it only treats / as a separator, so Windows-style traversal names like ..\..\etc\passwd pass through UNCHANGED. Verified on this Linux runtime: Path.GetFileName("..\\..\\etc\\passwd") returns the input unchanged. Since produced ZIPs are consumed cross-platform (extracted on Windows, where \ IS a separator), the fix does not actually prevent embedded traversal for backslash sequences when the archive is created on Linux/macOS. Strip BOTH / and \ explicitly (split on both separator chars / take the last segment after any of them) rather than relying on Path.GetFileName.
| nameof(files)); | ||
| } | ||
|
|
||
| var entry = archive.CreateEntry(entryName, CompressionLevel.Optimal); |
There was a problem hiding this comment.
[nit] Flattening directory components can silently collapse distinct inputs (e.g. a/x.txt and b/x.txt, or x.txt and dir/x.txt) to the same entry name. ZipArchive.CreateEntry permits duplicate names without throwing, so on extraction one entry overwrites the other → silent data loss. Acceptable given the security intent, but worth a collision guard or de-dup.
Summary
ZipHelper.CreateZipused the caller-suppliedPgFile.Nameverbatim as the ZIP entry name (src/Zip/ZipHelper.cs:15). This allowed traversal sequences such as../../to be embedded as entry names in produced archives, so a downstream extractor that trusts entry names could write files outside the intended directory (Zip Slip).This PR sanitizes each entry name with
Path.GetFileName(...)before callingCreateEntry, keeping only the bare file component and stripping any directory prefix. If a name resolves to null/empty (no file component, e.g."",dir/,../), it throwsArgumentExceptionrather than producing an unnamed entry.Changes
src/Zip/ZipHelper.cs— sanitize entry name viaPath.GetFileName; throwArgumentExceptionwhen the result is null or empty.tests/E4A.PostGuard.Tests/ZipHelperTests.cs:MultipleFiles_AllPresentWithContent—dir/b.txtis now stored asb.txt(directory components stripped, the intended new behavior).[Theory]covering traversal/absolute/nested paths being flattened to their file component.[Theory]assertingArgumentExceptionfor names with no file component.Testing
dotnet build— succeeds for bothnet8.0andnet10.0.dotnet test --framework net10.0 --filter ZipHelper— all 10 ZipHelper tests pass.CryptifyClientTests.ChunkContentRange_IsFormattedPerChunk) unrelated to this change — it fails identically on the base commit. net8.0 tests deferred to CI (net8.0 runtime not available in the workspace).Refs security advisory GHSA-mr97-hxhp-w3gg. Tracking issue: #40
🤖 Generated with Claude Code