Enforce chroot-like security in fdo.upload owner FSIM using os.Root #163
Conversation
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
2 times, most recently
from
acd3205 to
c49848a
Compare
|
@ben-krieger ptal |
|
Is |
The proposed changes always copy the contents from the temp file to the final destination so it will work across filesystems. However, always copying the contents from the temp file to the final destination is sub-optimal for temp files created in the same filesystem. I like the idea of creating temporary files to upload the content and only create the destination file if the upload was successful so I think the better approach here is to maintain the |
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
from
c49848a to
0fb3899
Compare
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
2 times, most recently
from
c1c38c8 to
a7e2026
Compare
|
@mmartinv can you rebase? @ben-krieger can you take a look? |
Replace os.Rename with os.OpenRoot to prevent path traversal attacks in the upload finalize function. The previous implementation was vulnerable to malicious filenames that could escape the intended upload directory using path traversal sequences (e.g., ../../etc/passwd). The refactored implementation: - Uses os.OpenRoot to create a sandboxed filesystem rooted at u.Dir - Detects same filesystem using syscall.Stat device ID comparison - Uses os.Rename when files are on the same filesystem for efficiency - Falls back to copy when files are on different filesystems - Backs up existing destination files with timestamp suffix before overwriting - Copies files through the rooted filesystem, which constrains all operations within the upload directory - Prevents symbolic links from referencing locations outside the root - Ensures u.Rename cannot escape u.Dir even with path traversal attempts Closes fido-device-onboard#69 Signed-off-by: Miguel Martín <[email protected]>
Add unit tests to verify the os.OpenRoot-based security implementation in the fdo.upload owner module: - TestUploadRequestPathTraversalPrevention: Tests multiple path traversal attack vectors including ../, absolute paths, and embedded traversal sequences to ensure os.OpenRoot prevents escaping the upload directory - TestUploadRequestNormalFlow: Validates normal upload operation with proper file placement and content verification - TestUploadRequestDefaultRename: Ensures empty Rename field defaults to basename of Name parameter - TestUploadRequestSHA384Mismatch: Verifies uploads fail when SHA-384 checksum doesn't match - TestUploadRequestBackupExistingFile: Tests that existing files are backed up with timestamp suffixes before being overwritten - TestUploadRequestSameFilesystemRename: Verifies rename optimization when temp file and destination are on the same filesystem - TestUploadRequestBackupWithoutExtension: Tests backup of files without extensions to ensure timestamp formatting works correctly All tests pass and confirm the security fix and optimizations from the previous commit effectively prevent path traversal vulnerabilities while providing efficient file operations. Related to fido-device-onboard#69 Signed-off-by: Miguel Martín <[email protected]>
mmartinv
force-pushed
the
fsim-upload-chroot-security
branch
from
a7e2026 to
904702b
Compare
| func sameFilesystem(path1, path2 string) (bool, error) { | ||
| var stat1, stat2 syscall.Stat_t | ||
|
|
||
| if err := syscall.Stat(path1, &stat1); err != nil { |
There was a problem hiding this comment.
You're using syscall.Stat because you want to compare the Dev (uint64) field. However, this is not portable and will break Windows compatibility, at the least.
GOOS=windows go doc syscall Stat_t
doc: no symbol Stat_t in packages syscallPlease move sameFilesystem to fsim/fs_unix.go with the appropriate build flags for Unix-like OSes that have a syscall.Stat function and then create an implementation that always returns false (I guess?) under fsim/fs_other.go.
| if err != nil { | ||
| // File doesn't exist, nothing to backup |
There was a problem hiding this comment.
Should only return nil if errors.Is(err, fs.ErrNotExist) and return err if err != nil otherwise.
| if err = os.Rename(tempFilePath, destPath); err != nil { | ||
| return false, false, fmt.Errorf("error renaming temp file to %q: %w", u.Rename, err) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Skip the else and make sure the if always returns. This is preferred Go style.
| // Construct the actual destination path | ||
| destPath := filepath.Join(u.Dir, u.Rename) | ||
|
|
||
| // Verify the resolved path is still within u.Dir (security check) |
There was a problem hiding this comment.
Why is this done rather than using rootDir from os.OpenRoot?
| return false, false, fmt.Errorf("path traversal detected in rename: %q", u.Rename) | ||
| } | ||
|
|
||
| // Backup existing file if present |
There was a problem hiding this comment.
This is potentially nice behavior not to clobber the file, but it's also unconfigurable and undocumented.
Following the principle of least surprise, I think you either overwrite or fail, not backup-and-overwrite.
I believe that the best API decision to reduce user error would be fail-if-existing with an optional configurable function for what to do on conflict.
Another clear API win would be to change Rename from a string to a func(string) string so that users can make every upload timestamped by upload time.
| @@ -0,0 +1,576 @@ | |||
| // SPDX-FileCopyrightText: (C) 2024 Intel Corporation | |||
There was a problem hiding this comment.
| // SPDX-FileCopyrightText: (C) 2024 Intel Corporation | |
| // SPDX-FileCopyrightText: (C) 2025 Red Hat, Inc. |
Thanks for adding so many tests! Take credit!
3 participants
Replace os.Rename with os.OpenRoot to prevent path traversal attacks in
the upload finalize function. The previous implementation was vulnerable
to malicious filenames that could escape the intended upload directory
using path traversal sequences (e.g., ../../etc/passwd).
The refactored implementation:
operations within the upload directory
Closes #69