Repository navigation
os: File.Stat on Windows doesn't use file handle for directories #52747
Description
On Windows File.Stat doesn't use the file handle to retrieve attributes when the file is a directory, due to the lack of a ...ByHandle Windows API which works on directories (GetFileInformationByHandle only works on files), using the path instead.
This means there may be a TOCTOU issue, as the results returned by Stat represent the attributes for the file at the path passed to Open, which may no longer be the file represented by the file handle if the file has been moved/replaced.
Activity
gopherbot commentedon May 10, 2022
Change https://go.dev/cl/405275 mentions this issue:
os,syscall: File.Stat to use file handle for directories on Windowsqmuntal commentedon May 10, 2022
I've been investigating this issue lately, and I've found that
GetFileInformationByHandledoes support directory handles. We can make that work by setting theFILE_FLAG_BACKUP_SEMANTICSattribute when creating the handle viasyscall.CreateFile. These are the relevant docs: CreateFile.This change breaks directory traversal, as readdir is expecting a handle created via
FindFirstFile, not theCreateFileone. It can be solved by creating a new traversal-enabled handle the first timereaddiris called, which has the nice benefit of only allocating adirInfoif it is really needed, instead of every time a directory is opened.I've done a working prototype of this idea in CL 405275.
I'm wondering if changing
syscall.Opento support directories (only in read mode) would be a breaking behavior change.I'm also pretty sure that there is no need to use the path-based
Find*API for traversing directories, and that we can instead use NtQueryDirectoryFile. These should also be far more efficient, e.g. git-for-windows/git@b69c08c#diff-4b6d4f2af4b31f0bc3ff43eac9a2a437.qmuntal commentedon May 10, 2022
It is likely that CL 405275 also fixes #36019 and #43322, at least it does in my local computer.
I'll reenable
TestDirSeekand updateFile.Seekin a separate CL just in case we have to revert it.qmuntal commentedon May 10, 2022
Another side effect of CL 405275: The directory handle created by
syscall.Opendoes not have the share modeFILE_SHARE_DELETEset (see #32088), but the handle created bysyscall.FindFirstFiledid.This means it wouldn't be possible to move or remove an opened directory, just as it is already happening with normal opened files. @alexbrainman @mattn thoughts?
mattn commentedon May 10, 2022
How does
os.File.Fd()work? It should always return the handle of the file. Because the user passes that handle to the function that expect the file descriptor. I think this might possibly makes another side-effect with this change. To put it simply, I thinkFileshould return ErrNotSupported or whatever for listing files.qmuntal commentedon May 11, 2022
os.File.Fd()returns whatever handler is stored infile.pfd.Sysfd.This is the current behavior:
syscall.Open, which internally callssyscall.CreateFile. This handle works in most win32 apis expecting file handles.syscall.FindFirstFile. This handle mostly only works insyscall.Find*apis, i.e., it doesn't work insyscall.GetFileInformationByHandle, which is used byos.File.Stat.So one could say that the current behavior of
os.File.Fd()for directories is only good if you need to traverse it, but not for the general case.CL 405275 changes how directory handles are constructed, instead of using
syscall.FindFirstFileit usessyscall.Open, unifying regular file handles with directory handles. This meansos.File.Fd()will return the same type of handle regardless if it is pointing to a directory, but it won't be directly usable insyscall.FindNextFile.qmuntal commentedon Oct 10, 2022
Summary of behavior changes in CL 405275:
File.Stat()TOCTOU is fixed because it uses a file handle instead of a pathos.Open()opens, when in read-only mode, files and directories with theFILE_FLAG_BACKUP_SEMANTICSflag. This means that the returned file handler will have additional restore and backup permissions. See docs. I don't expect this to pose any security concern, as the process still needSE_BACKUP_NAMEand/orSE_RESTORE_NAMEprivileges to perform this operate with these additional capabilities.os.Open()will not be removable nor renamable untilFile.Close()is called, as they won't have theFILE_SHARE_DELETEpermission. Note that this is already the behavior of files opened viaos.Open(), but it might break some fragile tests.I don't see how we can avoid these behavior changes if we want to fix the TOCTOU issue, using
FILE_FLAG_BACKUP_SEMANTICSis required to get a directory handle.@alexbrainman @rsc @ianlancetaylor
alexbrainman commentedon Oct 16, 2022
I am not against these changes. Mainly because it is @bcmills and others will be dealing with new bugs introduced by this change.
I don't see TOCTOU as a problem. But I am not security expert. It is hard for me to judge if these changes actually help any users or not.
On the hand @qmuntal mentioned that CL 405275 also fixes #36019 and #43322 .
Up to others if we should submit CL 405275 or not.
Alex
bcmills commentedon Oct 17, 2022
At the risk of reopening a can of worms, we've had a lot of discussion in the past about
FILE_SHARE_DELETE(see #35150, #34681, #32088). I'm not sure that this would really make things on that front any worse, but it is already something of a source of friction.bcmills commentedon Oct 17, 2022
The documentation for that flag says that it means “[t]he file is being opened or created for a backup or restore operation.” In general
os.Openis not performing a backup or restore operation, so that flag seems a little odd, and I can't find much other documentation on it beyond that it allows opening directory handles. (See previously #23312.I wonder, though: when we call
FindFirstFileWwe get a search handle for the directory, and theWIN32_FIND_DATAWstructure that we use to iterate over that handle lists files by basename (in thecFileNamefield), not by returning handles to the individual files.So, what I'm wondering is: does that search handle itself not prevent the directory from being renamed, akin to opening it without the
FILE_SHARE_DELETEflag? Otherwise, how is one intended to open sequential files in a directory without a similar TOCTOU problem?qmuntal commentedon Oct 18, 2022
It would make things more homogeneous. The current
os.Openbehavior is to open files withoutFILE_SHARE_DELETE(see #32088) and directories withFILE_SHARE_DELETE. The latter is probably not a conscious decision but just a side effect ofFindFirstFileWimplicitly settingFILE_SHARE_DELETE. We can decideqmuntal commentedon Oct 18, 2022
It is unfortunate that
FILE_FLAG_BACKUP_SEMANTICSconflates backup operations and directory handling, don't know the reasoning. For what I understand, the main reason #23312 was closed was due do to this argument from @alexbrainman:IMO this argument no longer holds, this discussion has some examples of operations that can only be applied to directory handlers opened via
FILE_FLAG_BACKUP_SEMANTICS, e.g., #36019.Having said this, I agree with @alexbrainman that CL 405275 will cause troubles to some, mainly to the lack of
FILE_SHARE_DELETEflag. Yet, I think it will fix more latent issues than it will cause, as this change makes files and directories behavior more consistent, andos.Openeasier to reason about.A
FindFirstFileWhandle does not prevent the directory from being renamed or deleted. TOCTOU problems can be avoided by holding a proper file handle created usingCreateFileWwithout theFILE_SHARE_DELETEflag while traversing the directory, which is in fact what CL 405275 does.alexbrainman commentedon Oct 23, 2022
I agree with @qmuntal . Back in #23312 there was nothing to fix - the proposal was to make code less "inconsistent". But CL 405275 will actually fix some real issues.
Alex
os,syscall: File.Stat to use file handle for directories on Windows