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: safer file open functions #67002

Open
Tracked by #13091
neild opened this issue Apr 23, 2024 · 98 comments
Open
Tracked by #13091

os: safer file open functions #67002

neild opened this issue Apr 23, 2024 · 98 comments

Comments

@neild
Copy link
Contributor

neild commented Apr 23, 2024

Please see the updated proposal in #67002 (comment)


Directory traversal vulnerabilities are a common class of vulnerability, in which an attacker tricks a program into opening a file that it did not intend. These attacks often take the form of providing a relative pathname such as "../../../etc/passwd", which results in access outside an intended location. CVE-2024-3400 is a recent, real-world example of directory traversal leading to an actively exploited remote code execution vulnerability.

A related, but less commonly exploited, class of vulnerability involves unintended symlink traversal, in which an attacker creates a symbolic link in the filesystem and manipulates the target into following it.

I propose adding several new functions to the os package to aid in safely opening files with untrusted filename components and defending against symlink traversal.


It is very common for programs to open a file in a known location using an untrusted filename. Programs can avoid directory traversal attacks by first validating the filename with a function like filepath.IsLocal. Defending against symlink traversal is harder.

I propose adding functions to open a file in a location:

package os

// OpenFileIn opens the named file in the named directory.
//
// If the file contains relative path components (..), no component may
// refer to a location outside the parent directory. The file may not be
// "", an absolute path, or (on Windows) a reserved device name such as "NUL".
// The file may refer to the directory itself (.).
//
// If any component of the named file references a symbolic link
// referencing a location out of the parent directory,
// OpenFileIn returns an error.
//
// OpenFileIn otherwise behaves like OpenFile.
func OpenFileIn(parent, name string, flag int, perm FileMode) (*File, error)

// CreateIn creates or truncates the named file in the named parent directory.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Create].
func CreateIn(parent, name string) (*File, error)

// Open opens the named file in the named parent directory for reading.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Open].
func OpenIn(parent, name string) (*File, error)

The OpenFileIn, OpenIn, and CreateIn family of functions safely open a file within a given location, defending against directory traversal, symlinks to unexpected locations, and unexpected access to Windows device files.


All modern Unix systems that I know of provide an openat call, to open a file relative to an existing directory handle (FD). Windows provides an equivalent (NtCreateFile with ObjectAttributes including a RootDirectory). Of the supported Go ports, I believe only js and plan9 do not support openat or an equivalent.

I propose adding support for openat-like behavior to os.File:

package os

// OpenFile opens the named file in the directory associated with the file f.
//
// If the file contains relative path components (..), no component may
// refer to a location outside the parent directory. The file may not be
// "", an absolute path, or (on Windows) a reserved device name such as "NUL".
//
// If any component of the named file references a symbolic link
// referencing a location out of the parent directory,
// OpenFile returns an error.
func (f *File) OpenFile(name string, flag int, perm FileMode) (*File, error)

// Create creates or truncates the named file in
// the directory associated with the file f.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) Create(name string) (*File, error)

// Open opens the named file in the directory associated with the file f for reading.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) OpenIn(name string) (*File, error)

Like the top-level CreateIn, OpenIn, and OpenFileIn, the methods defend against accessing files outside the given directory. This is unlike the default behavior of openat, which permits absolute paths, relative paths outside the root, and symlink traversal outside the root. (It corresponds to Linux's openat2 with the RESOLVE_BENEATH flag.)

A property of openat is that it follows a file across renames: If you open a directory, rename the directory, and use openat on the still-open FD, access is relative to the directory's new location. We cannot support this behavior on platforms which don't have openat or an equivalent (plan9 and js). We could fall back to operating purely on filenames, such that f.OpenIn(x) is equivalent to os.OpenIn(f.Name(), x). However, this seems potentially hazardous. I propose, therefore, that File.CreateIn, File.OpenIn, and File.OpenFileIn return an errors.ErrUnsupported error on these platforms.


The above functions defend against symlink traversal that leads outside of the designated root directory. Some users may wish to defend against symlink traversal entirely. Many modern operating systems provide an easy way to disable symlink following: Linux has RESOLVE_NO_SYMLINKS, Darwin has O_NOFOLLOW_ANY, and some other platforms have equivalents.

I propose adding support for disabling symlink traversal to the os package:

const (
	// O_NOFOLLOW_ANY, when included in the flags passed to [OpenFile], [OpenFileIn],
	// or [File.OpenFile], disallows resolution of symbolic links anywhere in the
	// named file.
	//
	// O_NOFOLLOW_ANY affects the handling of symbolic links in all components
	// of the filename. (In contrast, the O_NOFOLLOW flag supported by many
	// platforms only affects resolution of the last path component.) 
	//
	// O_NOFOLLOW_ANY does not disallow symbolic links in the parent directory name
	// parameter of [OpenFileIn].
	//
	// O_NOFOLLOW_ANY does not affect traversal of hard links, Windows junctions,
	// or Plan 9 bind mounts.
	//
	// On platforms which support symbolic links but do not provide a way to
	// disable symbolic link traversal (GOOS=js), open functions return an error
	// if O_NOFOLLOW_ANY is provided.
	O_NOFOLLOW_ANY int = (some value)
)

O_NOFOLLOW_ANY may be passed to OpenFile, OpenFIleIn, or File.OpenFIle to disable symlink traversal in any component of the file name. For OpenFileIn, symlinks would still be permitted in the directory component.

On platforms which do not support the equivalent of O_NOFOLLOW_ANY/RESOLVE_NO_SYMLINKS natively, the os package will use successive openat calls with O_NOFOLLOW to emulate it. On platforms with no openat (plan9 and js), open operations will return an error when O_NOFOLLOW_ANY is specified.

@neild neild added the Proposal label Apr 23, 2024
@gopherbot gopherbot added this to the Proposal milestone Apr 23, 2024
@seankhliao
Copy link
Member

seankhliao commented Apr 23, 2024

is this essentially https://pkg.go.dev/github.com/google/safeopen with Beneath -> In ?

that also has a ReadFile / WriteFile variant which I'd use more then the create version.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Apr 23, 2024
@neild
Copy link
Contributor Author

neild commented Apr 23, 2024

The design of this proposal is influenced by github.com/google/safeopen, but differs in a few areas. (Sorry, I really should have mentioned safeopen as prior art.)

Of the three parts of this proposal:

  • os.OpenIn is essentially safeopen.OpenBeneath.
  • File.Open is a slightly more limited but safer version of openat, and has no equivalent in safeopen.
  • O_NOFOLLOW_ANY has no equivalent in safeopen.

ReadFileIn and WriteFileIn seem like a useful and logical extension of this proposal.

@dsnet
Copy link
Member

dsnet commented Apr 24, 2024

Yes, please. When I was working on safe file operations and it turned out to be hard to do correctly without OS support.

Without O_NOFOLLOW, you have to slowly check every segment for symlinks before traversing into it. For the naive implementation, how do you protect against TOCTOU bugs? At the moment that you check some path segment and verify that it's not a symlink (or a safe one) and then proceed to descend into it, some other process (or goroutine) could have asynchronously changed the target.

@dsnet
Copy link
Member

dsnet commented Apr 24, 2024

What, if any, changes would be made to "io/fs"? Ideally, there is a mirror of these APIs in that package.

@neild
Copy link
Contributor Author

neild commented Apr 24, 2024

If we wanted to extend this proposal to io.FS, I believe the one addition would be:

package fs

// An OpenFile is a directory file whose entries may be opened with the Open method.
type OpenFile interface {
  File

  // Open opens the named file in the directory.
  //
  // When Open returns an error, it should be of type *PathError
  // with the Op field set to "openat", the Path field set to name,
  // and the Err field describing the problem.
  //
  // Open should reject attempts to open names that do not
  // satisfy ValidPath(name), returning a *PathError with Err set to
  // ErrInvalid or ErrNotExist.
  Open(name string) (File, error)
}

A more interesting question is os.DirFS. Currently, DirFS has two documented limitations: It follows symlinks out of the directory tree, and if the FS root is a relative path then it will be affected by later Chdir calls.

I don't think we can change DirFS's symlink-following behavior: It's documented, and it's a behavior that a user could reasonably depend on.

The interaction between DirFS and Chdir seems less likely to be something a user would depend on, but it is documented. I'm not sure if we can change it at this point, but perhaps.

Perhaps we should add a version of DirFS that opens the directory root at creation time (retaining a handle to it even if the current working directory changes or the root is renamed), and refuses to follow symlinks out of the root. I'm not sure if that should be part of this proposal or a separate one.

@adonovan
Copy link
Member

Perhaps the new names should include an At suffix to make clear to casual readers of the API that these are not the usual open system calls. Either way, the three new methods should probably reference some shared section of documentation on the concept of the at-suffixed operations.

@neild
Copy link
Contributor Author

neild commented Apr 26, 2024

I presume you mean the new method names? The functions have an "In" suffix.

We could also include the In suffix on the methods; I waffled on whether it belongs there or not:

func (f *File) OpenFileIn(name string, flag int, perm FileMode) (*File, error)
func (f *File) CreateIn(name string) (*File, error)
func (f *File) OpenIn(name string) (*File, error)

I'm trying to avoid the suffix "At" to make it clear that none of these calls are precisely openat. openat(2) permits escaping from the root directory via absolute or relative paths, and doesn't do anything about symlink traversal. (Linux has openat2(2), which is quite configurable. The proposed *In functions are essentially openat2 with the RESOLVE_BENEATH flag.)

@adonovan
Copy link
Member

Fair enough. Should the fs.OpenFile.Open method also be named OpenIn?

@neild
Copy link
Contributor Author

neild commented Apr 26, 2024

Should the fs.OpenFile.Open method also be named OpenIn?

Probably, for consistency.

@rsc
Copy link
Contributor

rsc commented May 29, 2024

Are we missing RemoveIn?

@neild
Copy link
Contributor Author

neild commented May 29, 2024

We should probably have RemoveIn as well:

// RemoveIn removes the named file or (empty) directory.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Remove].
func RemoveIn(parent, name string) error

// Remove removes the named file or (empty) directory
// in the directory associated with the file f.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) RemoveIn(name string) error

Perhaps also RemoveAllIn?

@rsc
Copy link
Contributor

rsc commented May 30, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals May 30, 2024
@bjorndm
Copy link

bjorndm commented May 30, 2024

Maybe it would be better if the parent was an fs.FS? Seems more widely applicable, if somewhat more complex.

@qmuntal
Copy link
Contributor

qmuntal commented May 31, 2024

func RemoveIn(parent, name string) error

Note that Windows does not provide (AFAIK) an unlinkat counterpart. It will have to be emulated doing something like:

func RemoveIn(parent, name string) error
  f, err := os.OpenIn(parent, name)
  if err != nil {
    return err
  }
  return syscall.SetFileInformationByHandle(f.Fd(), syscall.FileDispositionInfo, ...)
}

@hherman1
Copy link

hherman1 commented May 31, 2024

As a user, when should I use os.Open vs os.OpenIn? Should I continue to default to os.Open, and only use OpenIn when I am actively avoiding a security issue, or should my default be OpenIn now?

@neild
Copy link
Contributor Author

neild commented Jun 5, 2024

Maybe it would be better if the parent was an fs.FS?

The os package file functions operate on the local filesystem. fs.FS is an abstraction over a filesystem; it sits atop the os package functions, not under them.

If we want to add support for OpenIn on fs.FS filesystems, we would want something like #67002 (comment). We could add that to this proposal if we want, but for now I'm keeping this proposal focused on the os package.

Note that Windows does not provide (AFAIK) an unlinkat counterpart.

I think that's fine. This proposal requires varying degrees of implementation depending on platform already. (Linux has the very nice openat2 with RESOLVE_BENEATH, platforms without an equivalent are going to require us to do more work to produce equivalent behavior.)

If it's not possible to emulate unlinkat on Windows, that might be a problem, but it sounds like it should be possible.

As a user, when should I use os.Open vs os.OpenIn?

You should use OpenIn when you want to open a file within a directory.

I don't know how to give comprehensive guidance on when to use one vs. the other; the two functions behave differently and you should use the one that suits your specific purposes. If you're writing a command-line tool that accepts an input filename from the user, you probably want to use os.Open. If you're writing a tool that decompresses an archive, you probably want to use os.OpenIn to ensure that the output doesn't escape from the destination directory.

@bjorndm
Copy link

bjorndm commented Jun 5, 2024

The FS OpenFile looks good, yes. Somehow I skipped that comment, sorry.

@magical
Copy link
Contributor

magical commented Jun 7, 2024

A property of openat is that it follows a file across renames: If you open a directory, rename the directory, and use openat on the still-open FD, access is relative to the directory's new location. We cannot support this behavior on platforms which don't have openat or an equivalent (plan9 and js). We could fall back to operating purely on filenames, such that f.OpenIn(x) is equivalent to os.OpenIn(f.Name(), x). However, this seems potentially hazardous. I propose, therefore, that File.CreateIn, File.OpenIn, and File.OpenFileIn return an errors.ErrUnsupported error on these platforms.

I don't really understand this. What is a program supposed to do if File.Open returns ErrUnsupported? Either it can give up and report an error to the user, meaning that the program simply doesn't work on plan9 or js, Or it can implement the fallback manually,

f, err := parent.Open(filename) 
if err == os.ErrUnsupported {
   f, err = os.OpenIn(parent.Name(), filename)
   //or even: os.Open(path.Join(parent.Name(), filename))
}

which is exactly the "hazardous" behaviour you say you're trying to avoid. If the underlying platform truly has no equivalent to openat, though, then there's no other reasonable fallback. Returning ErrUnsupported is just creating more work for developers for no tangible benefit.

I think this could be addressed perfectly well in the docs by saying that some platforms (linux, windows, etc) provide extra guarantees around renamed files, and that others (plan9 and js) do not.

@neild
Copy link
Contributor Author

neild commented Jun 13, 2024

If the underlying platform truly has no equivalent to openat, though, then there's no other reasonable fallback.

The question is whether this is a reasonable fallback or not.

In the case of os.OpenIn, I think it's reasonable to fall back to a less-secure implementation. Lacking openat, we can statically validate the untrusted filename component for unintended traversal (os.OpenIn(dir, "../escapes")), and we can test for symlinks on the path, but we remain vulnerable to TOCTOU attacks. TOCTOU symlink attacks, where an attacker creates a symlink on the path while we're in the process of validating it, are an edge case and I think it's okay for us to support os.OpenIn on platforms where we can't defend against them (plan9 and js).

In the case of os.File.OpenIn, however, there are valid operations that we simply can't support without openat or the equivalent. With openat, you can open a directory, rename or even delete it, and then continue to access files in that directory. There's no way to simulate this with operations on the directory's filename.

Perhaps it's okay to say that os.File.OpenIn behaves differently on plan9 and js, and that users who need the ability to follow a directory across renames/deletes are responsible for not trying to do so on those platforms. Returning an error is the more conservative choice.

I note also that if you don't need the openat behavior of following a directory across renames, you don't need to use os.File.OpenIn at all--you can just always use os.OpenIn.

@CAFxX
Copy link
Contributor

CAFxX commented Jun 24, 2024

I don't know how to give comprehensive guidance on when to use one vs. the other; the two functions behave differently and you should use the one that suits your specific purposes. If you're writing a command-line tool that accepts an input filename from the user, you probably want to use os.Open. If you're writing a tool that decompresses an archive, you probably want to use os.OpenIn to ensure that the output doesn't escape from the destination directory.

I would recommend adding guidance in the documentation of the not *In variants calling out that the *In variants exist and recommended for cases when directory escape is not desirable.

@neild
Copy link
Contributor Author

neild commented Jul 22, 2024

Updated proposal, with comments on various changes arising from above discussion and working on implementation.

The OpenFileIn, CreateIn, and OpenIn functions are unchanged from the original proposal:

package os

// OpenFileIn opens the named file in the named directory.
//
// If the file contains relative path components (..), no component may
// refer to a location outside the parent directory. The file may not be
// "", an absolute path, or (on Windows) a reserved device name such as "NUL".
// The file may refer to the directory itself (.).
//
// If any component of the named file references a symbolic link
// referencing a location out of the parent directory,
// OpenFileIn returns an error.
//
// OpenFileIn otherwise behaves like OpenFile.
func OpenFileIn(parent, name string, flag int, perm FileMode) (*File, error)

// CreateIn creates or truncates the named file in the named parent directory.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Create].
func CreateIn(parent, name string) (*File, error)

// Open opens the named file in the named parent directory for reading.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Open].
func OpenIn(parent, name string) (*File, error)

The File methods now all have an In suffix: File.OpenFileIn, File.CreateIn, File.OpenIn. This is clearer overall: For example, f.CreateIn creates a file in the directory f, it doesn't create f. This also resolves an ambiguity between File.Stat and File.StatIn (see below).

package os

// OpenFileIn opens the named file in the directory associated with the file f.
//
// If the file contains relative path components (..), no component may
// refer to a location outside the parent directory. The file may not be
// "", an absolute path, or (on Windows) a reserved device name such as "NUL".
//
// If any component of the named file references a symbolic link
// referencing a location out of the parent directory,
// OpenFileIn returns an error.
func (f *File) OpenFileIn(name string, flag int, perm FileMode) (*File, error)

// CreateIn creates or truncates the named file in
// the directory associated with the file f.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) CreateIn(name string) (*File, error)

// OpenIn opens the named file in the directory associated with the file f for reading.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) OpenIn(name string) (*File, error)

To the above, we add MkdirIn, RemoveIn, and StatIn functions and methods. Creating directories and removing files are fundamental operations, and there's no reason to leave them out. DirFSIn (see below) provides a traversal-resistant Stat, so StatIn is included here as well.

Open question: Should we add LstatIn as well? How about SymlinkIn? RenameIn? ReadFileIn and WriteFileIn? On one hand, I don't want to let this proposal get out of hand with an endless array of new functions; on the other hand, some of these do seem useful. I'd appreciate proposal committee's thoughts on where we should draw the line with this proposal.

package os

// MkdirIn creates a new directory in the named parent directory
// with the specified name and permission bits (before umask).
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Mkdir].
func MkdirIn(parent, name string, perm FileMode) error

// MkdirIn creates a new directory in the directory associated with the file f.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) MkdirIn(name string, perm FileMode) error

// RemoveIn removes the named file or (empty) directory.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Remove].
func RemoveIn(parent, name string) error

// RemoveIn removes the named file or (empty) directory
// in the directory associated with the file f.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) RemoveIn(name string) error

// StatIn returns a FileInfo describing the named file in the named parent directory.
// It applies the same constraints on files as [OpenFileIn].
// It otherwise behaves like [Stat].
func StatIn(parent, name string) (FileInfo, error)

// StatIn returns a FileInfo describing the named file in the directory associated with  the file f.
// It applies the same constraints on files as [File.OpenFile].
func (f *File) StatIn(name string) (FileInfo, error)

We add os.DirFSIn, a traversal-safe version of os.DirFS.

Open question: DirFSIn or DirInFS? I prefer DirFSIn--"a directory filesystem in (root)", but internal discussion suggested DirInFS might be better.

For the moment, we do not add any new optional interfaces to io/fs, such as fs.OpenFile (see #67002 (comment)).

There are many existing APIs, both in and out of the standard library, that operate on an io/fs.FS. Providing a traversal-resistant FS implementation is a simpler and more effective approach to hardening programs than requiring every API which operates on an FS to check for and use an OpenFile method.

Open question: It seems likely to me that we're going to want more variations on DirFS in the future. For example, it seems reasonable to want an FS that disallows symlink traversal entirely (essentially passing O_NOFOLLOW_ANY to every file open). Therefore, I think DirFSIn should either accept an options struct to allow for future customization, or should return a concrete type with customization methods. ( For example, fs := os.DirFSIn("root"); fs.SetFollowSymlinks(false)). The following returns a concrete type.

package os

// DirFSIn returns a filesystem for the tree of files rooted at the directory dir.
// The directory dir must not be "".
//
// Open calls will resolve symbolic links, but return an error if any link points outside the directory dir.
//
// The returned filesystem implements [io/fs.FS], [io/fs.StatFS], [io/fs.ReadFileFS], and [io/fs.ReadDirFS].
func DirFSIn(dir string) *FS

type FS struct{}
func (fs *FS) Open(name string) (File, error)
func (fs *FS) Stat(name string) (FileInfo, error)
func (fs *FS) ReadFile(name string) ([]byte, error)
func (fs *FS) ReadDir(name string) ([]fs.DirEntry, error)

The O_NOFOLLOW_ANY open flag remains unchanged.

Open question: Should we add os.O_NOFOLLOW? I only realized while implementing this proposal that it doesn't exist already. (Existing code which uses the flag uses syscall.O_NOFOLLOW.) On one hand, if we're supporting a portable O_NOFOLLOW_ANY, perhaps we should support a portable O_NOFOLLOW as well. On the other hand, O_NOFOLLOW can be dangerously surprising, since it only prevents symlink resolution in the final filename component, so perhaps we should stick to the more robust form.

const (
	// O_NOFOLLOW_ANY, when included in the flags passed to [OpenFile], [OpenFileIn],
	// or [File.OpenFile], disallows resolution of symbolic links anywhere in the
	// named file.
	//
	// O_NOFOLLOW_ANY affects the handling of symbolic links in all components
	// of the filename. (In contrast, the O_NOFOLLOW flag supported by many
	// platforms only affects resolution of the last path component.) 
	//
	// O_NOFOLLOW_ANY does not disallow symbolic links in the parent directory name
	// parameter of [OpenFileIn].
	//
	// O_NOFOLLOW_ANY does not affect traversal of hard links, Windows junctions,
	// or Plan 9 bind mounts.
	//
	// On platforms which support symbolic links but do not provide a way to
	// disable symbolic link traversal (GOOS=js), open functions return an error
	// if O_NOFOLLOW_ANY is provided.
	O_NOFOLLOW_ANY int = (some value)
)

Open question: How should we handle .. relative path components in filenames?

Consider the following directory tree:

  • a/b is a directory.
  • s is a symlink to a/b.
  • f, a/f, and a/b/f are files.

On the Unix command line, if we cat s/../f, we print the contents of the file a/f.

If we open the current directory and openat(curfd, "s/../f"), we also open a/f.

The safeopen package cleans filenames prior to opening a file, so safeopen.OpenBeneath(".", "s/../f") opens the file f. The safeopen package also forbids symlink traversal entirely, so safeopen.OpenBeneath(".", "s/f") returns an error rather than opening a/b/f.

On Windows, things are confusing (and I'm still trying to understand what's going on under the hood): Using NtCreateFile to open a file in "." (the rough equivalent of Unix's openat):

  • s/../f opens a/f.
  • a/b/../f is an error.

It appears that NtCreateFile will resolve .. path components only if a symlink appears somewhere in the path. This is weird enough that I feel like I must be be missing something.

The question is: What should os.OpenIn(".", "s/../f") do in this case? Options I see include:

  • Resolve the symlink s and the relative path component .., and open a/f. This matches Unix openat behavior.
  • Clean the path prior to opening, performing lexical resolution of .. components, and open f. This matches the safeopen package's behavior. I don't like this option, as it defines a new set of nonstandard filesystem semantics (pathnames are lexically resolved prior to opening).
  • Disallow relative path components and return an error.
  • Disallow symlink resolution and return an error when attempting to open s.
  • Disallow both relative path components and symlink resolution.

My current inclination is the first option above: Permit both symlinks and .. path components, and resolve each step of the path in sequence. (So s/../f opens a/f in the above example.) This may be a bit tricky to implement on Windows, but it should be possible.

I can, however, see a good argument for disallowing . and .. relative path components. This simplifies the implementation, there are few if any real-world cases where resolving paths like s/../f is necessary, and users can lexically clean paths with filepath.Clean if desired.


Open question: How should we handle platforms without openat or an equivalent, namely GOOS=plan9 and GOOS=js?

GOOS=js does not permit implementing OpenIn in a fashion free of TOCTOU races (swapping a directory component with a symlink elsewhere on the filesystem). I believe Plan 9 doesn't have symlinks; if that's the case, TOCTOU races are not a concern on it.

GOOS=js and GOOS=plan9 do not permit implementing File.OpenIn correctly. Opening a directory as f, renaming or deleting that directory, and then using f.OpenIn should act on the original directory. Without openat or an equivalent, we have no way to follow the directory handle and the best we can do is act on the original directory path.

I've argued above for supporting OpenIn on these platforms and not supporting File.OpenIn. I think that I've been convinced by arguments above that it's better to support as much of the API as possible, even if platform limitations prevent supporting all of it. I therefore propose that on js and plan9, f.OpenIn("path") behaves equivalently to os.OpenIn(f.Name(), "path").

@bjorndm
Copy link

bjorndm commented Jul 22, 2024

This is rather extensive API. Perhaps a separate package from os would be better? Maybe os/in?

@neild neild mentioned this issue Jul 23, 2024
@rsc
Copy link
Contributor

rsc commented Jul 24, 2024

On a very minor note, Plan 9 can be considered to implement O_NOFOLLOW_ANY because there are no symlinks on Plan 9 at all.

@rsc
Copy link
Contributor

rsc commented Jul 24, 2024

More generally, I understand the motivation here, but the amount of new API is a bit daunting. I think we need to keep thinking about reducing the total amount of API. It seems like there needs to be some type representing the constrained file system. For this message, let's call it a Dir. It would be defined like:

// A Dir represents a root directory in the file system.
// Methods on a Dir can only access files and directories inside that root directory.
// Methods on Dir are safe to be used from multiple goroutines simultaneously.
// After Close is called, methods on Dir return errors.
type Dir struct {
   ...
}

func OpenDir(name string) (*Dir, error)

func (*Dir) FS() fs.FS
func (*Dir) OpenFile
func (*Dir) Create
func (*Dir) Open
func (*Dir) OpenDir
func (*Dir) Mkdir
func (*Dir) Remove
func (*Dir) MkdirAll
func (*Dir) RemoveAll
func (*Dir) Close

All the top-level convenience things like os.OpenIn can be left out. Code can use OpenDir followed by the operation it wants.

That at least feels like a more manageable amount of API.

I have been thinking for a while and have not come up with a name like more than Dir. It's certainly not perfect, and OpenDir would need a doc comment explaining that it's not opendir(3), but it's not bad.

@cyphar
Copy link

cyphar commented Nov 2, 2024

The concern I had about Root.OpenRoot hasn't really been addressed (the history of chroot-style attacks indicates that this API could result in security issues), but I understand that @neild doesn't seem to consider it an issue so 🤷. Just leaving this here for the record.

@aclements
Copy link
Member

@cyphar, can you point to an explanation of the chroot breakout you're referring to? I'm not familiar with it.

@aclements aclements moved this from Likely Accept to Accepted in Proposals Nov 6, 2024
@aclements
Copy link
Member

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.

The proposal is:

package os

// Root represents a directory.
//
// Methods on Root can only access files and directories within that directory.
// If any component of a file name passed to a method of Root references a location
// outside the root, the method returns an error.
// File names may reference the directory itself (.).
//
// File names may contain symbolic links, but symbolic links may not
// reference a location outside the root.
// Symbolic links must not be absolute.
//
// Methods on Root do not prohibit traversal of filesystem boundaries,
// Linux bind mounts, /proc special files, or access to Unix device files.
//
// Methods on Root are safe to be used from multiple goroutines simultaneously.
//
// On most platforms, creating a Root opens a file descriptor or handle referencing
// the directory. If the directory is moved, methods on Root reference the original
// directory.
//
// Root's behavior differs on some platforms:
//
//   - When GOOS=windows, file names may not reference Windows reserved device names
//     such as NUL and COM1.
//   - When GOOS=js, Root is vulnerable to TOCTOU (time-of-check-time-of-use)
//     attacks in symlink validation, and cannot ensure that operations will not
//     escape the root.
//   - When GOOS=plan9 or GOOS=js, Root does not track directories across renames.
//     On these platforms, a Root references a directory name, not a file descriptor
type Root struct { ... }

func OpenRoot(dir string) (*Root, error)
func (*Root) FS() fs.FS
func (*Root) OpenFile
func (*Root) Create
func (*Root) Open
func (*Root) OpenRoot
func (*Root) Close
func (*Root) Mkdir
func (*Root) Remove
func (*Root) MkdirAll
func (*Root) RemoveAll
func (*Root) Chmod
func (*Root) Chown
func (*Root) Chtimes
func (*Root) Lchown
func (*Root) Lstat
func (*Root) Readlink
func (*Root) Rename
func (*Root) Stat
func (*Root) Symlink
func (*Root) Link
func (*Root) Truncate

func OpenInRoot(dir, name string) (*File, error) {
   r, err := OpenRoot(dir)
   if err != nil { return nil }
   return r.Open(name)
}

@aclements aclements changed the title proposal: os: safer file open functions os: safer file open functions Nov 6, 2024
@aclements aclements modified the milestones: Proposal, Backlog Nov 6, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/627076 mentions this issue: os: add Root.Remove

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/627475 mentions this issue: os: add Root.Stat

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629518 mentions this issue: os: add Root.FS

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629555 mentions this issue: os: add OpenInRoot

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629519 mentions this issue: os: add Root.Readlink

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629698 mentions this issue: os: add Root.RemoveAll, avoid symlink race in RemoveAll on Windows

@neild
Copy link
Contributor Author

neild commented Nov 20, 2024

My current intent is to submit a subset of this proposal for 1.24, with the remainder following in 1.25. 1.24 will contain:

type Root struct { ... }

func OpenRoot(dir string) (*Root, error)
func (*Root) OpenFile
func (*Root) Create
func (*Root) Open
func (*Root) OpenRoot
func (*Root) Close
func (*Root) Mkdir
func (*Root) Remove
func (*Root) Lstat
func (*Root) Stat

func OpenInRoot(dir, name string) (*File, error)

That will leave the following functions for 1.25:

func (*Root) MkdirAll
func (*Root) RemoveAll
func (*Root) Chmod
func (*Root) Chown
func (*Root) Chtimes
func (*Root) Lchown
func (*Root) Readlink
func (*Root) Rename
func (*Root) Symlink
func (*Root) Link
func (*Root) Truncate

The implementation in 1.24 will support all our ports (with the caveats mentioned above for GOOS=js and GOOS=plan9), but not does not take advantage of platform-specific features such as Linux's RESOLVE_BENEATH and Darwin's O_NOFOLLOW_ANY which allow for a more efficient implementation. That will also be a task for 1.25.

gopherbot pushed a commit that referenced this issue Nov 20, 2024
Add os.Root, a type which represents a directory and permits performing
file operations within that directory.

For #67002

Change-Id: I863f4f1bc320a89b1125ae4237761f3e9320a901
Reviewed-on: https://go-review.googlesource.com/c/go/+/612136
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Nov 20, 2024
For #67002

Change-Id: Ibbf44c0bf62f53695a7399ba0dae5b84d5efd374
Reviewed-on: https://go-review.googlesource.com/c/go/+/627076
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Nov 20, 2024
For #67002

Change-Id: I0903f45dbb4c44ea0280c340c96c5f3c3c0781be
Reviewed-on: https://go-review.googlesource.com/c/go/+/627475
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
gopherbot pushed a commit that referenced this issue Nov 20, 2024
For #67002

Change-Id: Ib687c92d645b9172677e5781a3e51ef1a0427c30
Reviewed-on: https://go-review.googlesource.com/c/go/+/629518
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Nov 20, 2024
For #67002

Change-Id: If919ee8a5e3d90e91c7848330762e3254245fba1
Reviewed-on: https://go-review.googlesource.com/c/go/+/629555
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests