Encryption-Aware Secure File Deletion #2959
Conversation
This comprehensive refactoring introduces encryption-aware secure deletion, adding infrastructure to detect, track, and optimize file deletion based on volume encryption status across macOS, Linux, and Windows platforms. Key changes include: - Added VolumeEncryption type to domain model with encryption type detection - Implemented platform-specific encryption detection for FileVault, BitLocker, LUKS - Updated SecureDeleteOptions to support configurable, encryption-aware deletion - Added TRIM/hole punching support for secure file deletion on different platforms - Created new volume encryption query API for frontend integration - Enhanced volume listing to include encryption metadata The implementation follows NIST SP 800-88 guidance, recognizing that encrypted volumes require different secure deletion strategies compared to unencrypted media.
|
Requesting review from @jamiepine who has experience with the following files modified in this PR:
|
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| // Alternative: use lsblk to detect crypto devices | ||
| // lsblk -o NAME,FSTYPE,TYPE -J shows TYPE=crypt for encrypted devices | ||
| if let Ok(output) = Command::new("lsblk") | ||
| .args(["-o", "NAME,TYPE", "-J", device]) |
There was a problem hiding this comment.
This lsblk call runs synchronously inside detect_luks_encryption, which is called from async contexts. Since this blocks the async runtime during I/O, consider either making this function async with spawn_blocking, or caching the encryption detection results during initial volume scanning to avoid repeated blocking calls.
| &format!( | ||
| "$vol = Get-BitLockerVolume -MountPoint '{}' -ErrorAction SilentlyContinue; \ | ||
| if ($vol) {{ $vol.ProtectionStatus.ToString() }} else {{ 'NotFound' }}", | ||
| drive |
There was a problem hiding this comment.
The PowerShell command constructs a string with drive interpolated directly. While the input comes from internal drive letter parsing, consider validating that drive contains only expected characters (letters and colon) to prevent any potential command injection if this code path is ever exposed to external input.
| }; | ||
|
|
||
| if ret == 0 { | ||
| debug!("Successfully punched hole in file: {} bytes", size); |
There was a problem hiding this comment.
The file descriptor fd is captured by the closure but the std_file that owns it is moved into the closure. After spawn_blocking completes, the file handle is dropped, which is correct. However, the fcntl call uses the raw fd without ensuring the file stays open - if the blocking task panics, this could leave resources in an inconsistent state. Consider using a guard pattern or ensuring proper cleanup on panic.
| use std::process::Command; | ||
|
|
||
| // Check if file is on an APFS or HFS+ volume with TRIM support | ||
| // Most SSDs on macOS support TRIM natively since macOS 10.10.4 | ||
| let output = tokio::task::spawn_blocking(move || { | ||
| Command::new("diskutil") | ||
| .args(["info", "-plist", "/"]) | ||
| .output() | ||
| }) | ||
| .await; | ||
|
|
There was a problem hiding this comment.
This function spawns a blocking task to check if the filesystem is APFS. The path parameter is unused in this implementation - the check always queries the root volume /. For accurate detection, this should check the actual path's mount point.
|
|
||
| options | ||
| } | ||
|
|
There was a problem hiding this comment.
After a successful TRIM operation, the code still proceeds to do overwrite passes. The comment says "we still do at least one overwrite pass for extra security" but if passes is 1 and TRIM succeeded, you're doing redundant work. Consider making this configurable or skipping the overwrite when TRIM succeeds on encrypted volumes.
| enabled: true, | ||
| encryption_type, | ||
| is_unlocked, | ||
| } |
There was a problem hiding this comment.
This method always returns None, making it a no-op. If the intent is to provide a constructor for the unencrypted case, consider returning Self with enabled: false instead, or removing this method entirely since callers can just use None directly.
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
PR SummaryImplements encryption-aware deletion and surfaces encryption state throughout volumes and file operations.
Written by Cursor Bugbot for commit b329faf. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
core/src/ops/files/delete/trim.rs
Outdated
| &format!( | ||
| "$disk = Get-PhysicalDisk | Where-Object {{ $_.DeviceId -eq (Get-Partition -DriveLetter '{}' -ErrorAction SilentlyContinue).DiskNumber }}; if ($disk) {{ $disk.MediaType }} else {{ 'Unknown' }}", | ||
| drive.chars().next().unwrap_or('C') | ||
| ), |
There was a problem hiding this comment.
Windows TRIM function has copy-paste error with wrong parameter
High Severity
The trim_file_windows function passes a PowerShell command format string as the dwFlagsAndAttributes parameter (6th argument) to CreateFileW, and references an undefined variable drive. This is clearly a copy-paste error from is_trim_supported_windows. The 6th parameter should be a u32 file attributes flag (like FILE_ATTRIBUTE_NORMAL), not a PowerShell script. This renders the Windows TRIM implementation non-functional.
| | EncryptionType::VeraCrypt | ||
| | EncryptionType::Hardware | ||
| ) | ||
| } |
There was a problem hiding this comment.
ECryptfs excluded from at-rest encryption protection check
Medium Severity
The provides_at_rest_protection method does not include EncryptionType::ECryptfs in its match arms, despite ECryptfs being a valid encryption type that's detected on Linux volumes. Since ECryptfs does provide at-rest encryption for files, volumes using it will incorrectly get 3-pass secure delete instead of the optimized 1-pass deletion, causing unnecessary I/O overhead and wear.
| true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
macOS TRIM support check ignores the path parameter
Medium Severity
The is_trim_supported_macos function accepts a path parameter but completely ignores it. The diskutil info -plist / command always queries the root filesystem regardless of what path is provided. If a user has files on an external HDD mounted at a non-root location, this function would incorrectly report TRIM support based on the internal SSD's capabilities rather than the actual volume containing the file.
Co-authored-by: ijamespine <[email protected]>
Summary
Implement encryption-aware secure file deletion system with cross-platform encryption detection and optimized deletion strategies.
Key Changes
VolumeEncryptiontype to track volume encryption statusDeleteModewith configurable secure delete optionsMotivation
Improve file deletion security by:
Testing
Future Work