Repository navigation
fix: shred only on this PC's drives, and remove only the file overwritten - #3231
Merged
Merged
Conversation
…tten The refusal of Windows' own folders compared paths in their drive-letter form, and a path through a network share kept its share form at every resolution step, so \\localhost\C$\Program Files\..., this PC's own folders under another name, passed it. A file on a network share is refused now before the share is reached, and again when the canonical path, or the file the overwrite holds open, leads to one through a link or a mapped drive. An overwrite sent over the network is not a secure one anyway: the computer holding the share decides where it lands and can keep copies. The overwrite holds one exclusive handle from the check of what it opened to the truncate, and the file was deleted by its name after that handle had closed, so the delete acted on whatever the path led to by then. The named streams are overwritten while the handle is still held, and the file is removed through a handle opened for deletion, a link at its name opened as the link, and checked to be open on the file the overwrite held. When it is not, nothing is removed and the caller hears that the contents were overwritten and the name stayed.
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
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.
Two changes to how File Shredder decides where it may write and what it removes.
What changes
ThrowIfProtectedcompares against the drive-letter forms of Windows, System32 and Program Files, and a path through a share stayed in its UNC form throughPath.GetFullPath,ResolveLinkTargetandGetFinalPathNameByHandle, so\\localhost\C$\Program Files\…(or\\127.0.0.1\C$\…, or a drive letter mapped to one) passed the check itsC:\form is refused by.ValidatePathnow refuses a network path first, from the text alone, before anything reaches the share; it refuses again when the canonical path turns out to be on one, which a directory link in the middle of a local path can do; and the held-handle check refuses a handle that is open on a share.IsOnANetworkSharetakes the UNC spellings (\\server\share,//server/share,\\?\UNC\,\\.\UNC\) and a drive letter Windows reports as a network drive, and leaves a device path to a local volume to the canonical check. Shredding over SMB is not a secure overwrite in any case, since the server decides where the writes land and can keep copies in its caches and snapshots.FileShare.Nonehandle from the canonical check to the truncate, but the named streams were overwritten and the file deleted by path after that handle closed, so the one step that removes the file was not bound to the object that was checked and written. The named streams are now overwritten while the handle is still held (a named stream opens withFileShare.Nonewhile the unnamed one is held, as the method's remarks record), the file's identity is read from the held handle, andRemoveShreddedFileopens the name again withDELETEandFILE_FLAG_OPEN_REPARSE_POINT, checks that handle is open on the same file, and sets the delete disposition on it. A mismatch removes nothing and reports that the contents were overwritten and the name stayed; a refused disposition keeps the existing "overwritten but not removed" and "removal was denied" wording.FileIdentity.OfAnyFileSystemfalls back to the 64-bit file index where a file system has no 128-bit ID, a FAT or exFAT USB stick say, so the comparison works there too.How it was checked
ValidatePath, all four spellings ofShredFileAsync_APathOnANetworkShare_IsRefusedfailed. The files named do not exist, so nothing could be written had the refusal failed; the folder variants stayed refused by the canonical check, since those folders exist through the share.ShredFileAsync_AFileReachedThroughALinkToAShare_IsRefused_AndLeftAsItWasgoes through a directory link to this PC's temp folder by way of its administrative share, and skips when that share cannot be reached. With the canonical and the held-handle checks both taken out it failed, its file overwritten; with either one taken out alone it still passed, as it should, since each refuses the file by itself.File.Delete(filePath),ShredFileAsync_RemovesOnlyTheFileItOverwrote_WhenItsNameLeadsElsewhereByThenfailed: its write handle moves the file away on close and puts another at the name, and the path delete removed that other file.\\, three rows ofIsOnANetworkShare_SaysSoForAShare_InEverySpellingfailed;IsOnANetworkShare_IsFalseForThisPCsOwnDrive_AlsoAsADevicePathcovers the other side.