Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: File.Stat on Windows doesn't use file handle for directories #52747

Closed
rolandshoemaker opened this issue May 6, 2022 · 13 comments
Closed
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@rolandshoemaker
Copy link
Member

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.

@rolandshoemaker rolandshoemaker added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 6, 2022
@rolandshoemaker rolandshoemaker added this to the Backlog milestone May 6, 2022
@gopherbot
Copy link

Change https://go.dev/cl/405275 mentions this issue: os,syscall: File.Stat to use file handle for directories on Windows

@qmuntal
Copy link
Contributor

qmuntal commented May 10, 2022

I've been investigating this issue lately, and I've found that GetFileInformationByHandle does support directory handles. We can make that work by setting the FILE_FLAG_BACKUP_SEMANTICS attribute when creating the handle via syscall.CreateFile. These are the relevant docs: CreateFile.

This change breaks directory traversal, as readdir is expecting a handle created via FindFirstFile, not the CreateFile one. It can be solved by creating a new traversal-enabled handle the first time readdir is called, which has the nice benefit of only allocating a dirInfo if 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.Open to 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
Copy link
Contributor

qmuntal commented 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 TestDirSeek and update File.Seek in a separate CL just in case we have to revert it.

@qmuntal
Copy link
Contributor

qmuntal commented May 10, 2022

Another side effect of CL 405275: The directory handle created by syscall.Open does not have the share mode FILE_SHARE_DELETE set (see #32088), but the handle created by syscall.FindFirstFile did.

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
Copy link
Member

mattn commented 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 think File should return ErrNotSupported or whatever for listing files.

@qmuntal
Copy link
Contributor

qmuntal commented May 11, 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 think File should return ErrNotSupported or whatever for listing files.

os.File.Fd() returns whatever handler is stored in file.pfd.Sysfd.

This is the current behavior:

  • If the opened file is not a directory, the handle is created using syscall.Open, which internally calls syscall.CreateFile. This handle works in most win32 apis expecting file handles.
  • If the opened file is a directory, the handle is created using syscall.FindFirstFile. This handle mostly only works in syscall.Find* apis, i.e., it doesn't work in syscall.GetFileInformationByHandle, which is used by os.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.FindFirstFile it uses syscall.Open, unifying regular file handles with directory handles. This means os.File.Fd() will return the same type of handle regardless if it is pointing to a directory, but it won't be directly usable in syscall.FindNextFile.

@julieqiu julieqiu modified the milestones: Backlog, Go1.20 Jul 18, 2022
@qmuntal
Copy link
Contributor

qmuntal commented Oct 10, 2022

Summary of behavior changes in CL 405275:

  • File.Stat() TOCTOU is fixed because it uses a file handle instead of a path
  • os.Open() opens, when in read-only mode, files and directories with the FILE_FLAG_BACKUP_SEMANTICS flag. 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 need SE_BACKUP_NAME and/or SE_RESTORE_NAME privileges to perform this operate with these additional capabilities.
  • Directories opened via os.Open() will not be removable nor renamable until File.Close() is called, as they won't have the FILE_SHARE_DELETE permission. Note that this is already the behavior of files opened via os.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_SEMANTICS is required to get a directory handle.

@alexbrainman @rsc @ianlancetaylor

@alexbrainman
Copy link
Member

Summary of behavior changes in CL 405275:

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
Copy link
Contributor

bcmills commented Oct 17, 2022

Directories opened via os.Open() will not be removable nor renamable until File.Close() is called, as they won't have the FILE_SHARE_DELETE permission.

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
Copy link
Contributor

bcmills commented Oct 17, 2022

os.Open() opens, when in read-only mode, files and directories with the FILE_FLAG_BACKUP_SEMANTICS flag.

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.Open is 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 FindFirstFileW we get a search handle for the directory, and the WIN32_FIND_DATAW structure that we use to iterate over that handle lists files by basename (in the cFileName field), 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_DELETE flag? Otherwise, how is one intended to open sequential files in a directory without a similar TOCTOU problem?

@qmuntal
Copy link
Contributor

qmuntal commented Oct 18, 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.

It would make things more homogeneous. The current os.Open behavior is to open files without FILE_SHARE_DELETE (see #32088) and directories with FILE_SHARE_DELETE. The latter is probably not a conscious decision but just a side effect of FindFirstFileW implicitly setting FILE_SHARE_DELETE. We can decide

@qmuntal
Copy link
Contributor

qmuntal commented Oct 18, 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.Open is 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.

It is unfortunate that FILE_FLAG_BACKUP_SEMANTICS conflates 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:

We only use FILE_FLAG_BACKUP_SEMANTICS to open directories and symlinks. There is no way to open them without FILE_FLAG_BACKUP_SEMANTICS flag. We do not use FILE_FLAG_BACKUP_SEMANTICS to open files, because everything we do with files work just fine without FILE_FLAG_BACKUP_SEMANTICS.

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_DELETE flag. Yet, I think it will fix more latent issues than it will cause, as this change makes files and directories behavior more consistent, and os.Open easier to reason about.

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_DELETE flag? Otherwise, how is one intended to open sequential files in a directory without a similar TOCTOU problem?

A FindFirstFileW handle does not prevent the directory from being renamed or deleted. TOCTOU problems can be avoided by holding a proper file handle created using CreateFileW without the FILE_SHARE_DELETE flag while traversing the directory, which is in fact what CL 405275 does.

@rolandshoemaker rolandshoemaker removed their assignment Oct 18, 2022
@alexbrainman
Copy link
Member

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.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

7 participants