Merged
Conversation
The export-attachment command's --output flag passes user-supplied paths directly to os.OpenFile without validation. The command's
own documentation shows a scripted workflow where email-derived attachment filenames are piped into -o:
jq -r '.attachments[] | "\(.content_hash)\t\(.filename)"' | \
while IFS=$'\t' read -r hash name; do
msgvault export-attachment "$hash" -o "$name"
done
An email attachment named ../../.ssh/authorized_keys would write outside the working directory.
The Fix (3 lines of logic)
Added ValidateOutputPath() to internal/export/attachments.go — it cleans the path with filepath.Clean() and rejects relative paths
that start with .. (meaning they escape the working directory after normalization). Absolute paths are allowed since they represent
an explicit user choice.
The validation is called early in runExportAttachment before any file I/O.
Changes Made
- internal/export/attachments.go: Added ValidateOutputPath() function (7 lines)
- internal/export/attachments_test.go: Added TestValidateOutputPath with 8 test cases
- cmd/msgvault/cmd/export_attachment.go: Added validation call before file operations (5 lines)
ValidateOutputPath previously allowed absolute paths (e.g. /etc/cron.d/evil) which could be exploited via email-supplied attachment names in the documented jq pipeline workflow. Also, strings.HasPrefix(cleaned, "..") falsely rejected legitimate filenames like "..backup" that aren't path traversal. Fix: reject absolute paths entirely, and only match ".." as a path component (cleaned == ".." or starts with "../") rather than a string prefix. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- saveToken: atomic write via temp file + os.Rename to prevent TOCTOU symlink races where an attacker could create a symlink between tokenPath() returning and the write - hasPathPrefix: rewrite using filepath.Rel to correctly handle filesystem roots (/, C:\) — the old cleanDir+separator approach produced "//" when dir was "/" - isPathWithinDir: delegate to hasPathPrefix to avoid duplication - Add TestHasPathPrefix with Windows drive-root coverage Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
filepath.IsAbs misses drive-relative paths (C:foo, C:..\evil) and UNC paths (\\server\share) on Windows. Add a filepath.VolumeName check to catch these. Add Windows-only test cases for C:\, C:relative, C:..\, and UNC paths. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wesm
force-pushed
the
bug-path-traversal-attachment
branch
from
8a9c3de to
7ddbdfa
Compare
Owner
|
I'm pushing some fixes from reviews on the other path traversal PR, and I rebased. Bear with me |
hasPathPrefix used strings.HasPrefix(rel, "..") which falsely rejected child paths like /a/b/..backup (rel="..backup"). Changed to match ".." only as a path component: rel == ".." || HasPrefix(rel, "../"). Added TestSaveToken_OverwriteExisting to verify atomic temp+rename overwrites an existing token file (os.Rename uses MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows since Go 1.5). Added "dotdot prefix child" test case to TestHasPathPrefix. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
On Windows, filepath.IsAbs returns false for paths like /tmp/file.pdf (rooted but without a drive letter). Add explicit check for leading / or \ to catch drive-relative rooted paths that escape the working directory. Add backslash rooted path test case. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The export-attachment command's --output flag passes user-supplied paths directly to os.OpenFile without validation. The command's own documentation shows a scripted workflow where email-derived attachment filenames are piped into -o:
jq -r '.attachments[] | "(.content_hash)\t(.filename)"' |
while IFS=$'\t' read -r hash name; do
msgvault export-attachment "$hash" -o "$name"
done
An email attachment named ../../.ssh/authorized_keys would write outside the working directory.
The Fix (3 lines of logic)
Added ValidateOutputPath() to internal/export/attachments.go — it cleans the path with filepath.Clean() and rejects relative paths that start with .. (meaning they escape the working directory after normalization). Absolute paths are allowed since they represent an explicit user choice.
The validation is called early in runExportAttachment before any file I/O.
Changes Made