merge #26 into cyphar/filepath-securejoin:main

Kir Kolyshkin (3):
  OpenInRoot: add CVE link to godoc
  Add cross-links to godoc
  Remove osVFS methods documentation

LGTMs: cyphar

join.go

@@ -22,27 +22,27 @@ const maxSymlinkLimit = 255
 
 // IsNotExist tells you if err is an error that implies that either the path
 // accessed does not exist (or path components don't exist). This is
-// effectively a more broad version of os.IsNotExist.
+// effectively a more broad version of [os.IsNotExist].
 func IsNotExist(err error) bool {
 	// Check that it's not actually an ENOTDIR, which in some cases is a more
 	// convoluted case of ENOENT (usually involving weird paths).
 	return errors.Is(err, os.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) || errors.Is(err, syscall.ENOENT)
 }
 
-// SecureJoinVFS joins the two given path components (similar to Join) except
+// SecureJoinVFS joins the two given path components (similar to [filepath.Join]) except
 // that the returned path is guaranteed to be scoped inside the provided root
 // path (when evaluated). Any symbolic links in the path are evaluated with the
 // given root treated as the root of the filesystem, similar to a chroot. The
-// filesystem state is evaluated through the given VFS interface (if nil, the
-// standard os.* family of functions are used).
+// filesystem state is evaluated through the given [VFS] interface (if nil, the
+// standard [os].* family of functions are used).
 //
 // Note that the guarantees provided by this function only apply if the path
 // components in the returned string are not modified (in other words are not
 // replaced with symlinks on the filesystem) after this function has returned.
-// Such a symlink race is necessarily out-of-scope of SecureJoin.
+// Such a symlink race is necessarily out-of-scope of SecureJoinVFS.
 //
 // NOTE: Due to the above limitation, Linux users are strongly encouraged to
-// use OpenInRoot instead, which does safely protect against these kinds of
+// use [OpenInRoot] instead, which does safely protect against these kinds of
 // attacks. There is no way to solve this problem with SecureJoinVFS because
 // the API is fundamentally wrong (you cannot return a "safe" path string and
 // guarantee it won't be modified afterwards).
@@ -123,8 +123,8 @@ func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) {
 	return filepath.Join(root, finalPath), nil
 }
 
-// SecureJoin is a wrapper around SecureJoinVFS that just uses the os.* library
-// of functions as the VFS. If in doubt, use this function over SecureJoinVFS.
+// SecureJoin is a wrapper around [SecureJoinVFS] that just uses the [os].* library
+// of functions as the [VFS]. If in doubt, use this function over [SecureJoinVFS].
 func SecureJoin(root, unsafePath string) (string, error) {
 	return SecureJoinVFS(root, unsafePath, nil)
 }

mkdir_linux.go

@@ -22,23 +22,23 @@ var (
 	errPossibleAttack = errors.New("possible attack detected")
 )
 
-// MkdirAllHandle is equivalent to MkdirAll, except that it is safer to use in
-// two respects:
+// MkdirAllHandle is equivalent to [MkdirAll], except that it is safer to use
+// in two respects:
 //
-//   - The caller provides the root directory as an *os.File (preferably O_PATH)
+//   - The caller provides the root directory as an *[os.File] (preferably O_PATH)
 //     handle. This means that the caller can be sure which root directory is
 //     being used. Note that this can be emulated by using /proc/self/fd/... as
-//     the root path with MkdirAll.
+//     the root path with [os.MkdirAll].
 //
-//   - Once all of the directories have been created, an *os.File (O_PATH) handle
+//   - Once all of the directories have been created, an *[os.File] O_PATH handle
 //     to the directory at unsafePath is returned to the caller. This is done in
 //     an effectively-race-free way (an attacker would only be able to swap the
 //     final directory component), which is not possible to emulate with
-//     MkdirAll.
+//     [MkdirAll].
 //
 // In addition, the returned handle is obtained far more efficiently than doing
-// a brand new lookup of unsafePath (such as with SecureJoin or openat2) after
-// doing MkdirAll. If you intend to open the directory after creating it, you
+// a brand new lookup of unsafePath (such as with [SecureJoin] or openat2) after
+// doing [MkdirAll]. If you intend to open the directory after creating it, you
 // should use MkdirAllHandle.
 func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err error) {
 	// Make sure there are no os.FileMode bits set.
@@ -168,7 +168,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 	return currentDir, nil
 }
 
-// MkdirAll is a race-safe alternative to the Go stdlib's os.MkdirAll function,
+// MkdirAll is a race-safe alternative to the [os.MkdirAll] function,
 // where the new directory is guaranteed to be within the root directory (if an
 // attacker can move directories from inside the root to outside the root, the
 // created directory tree might be outside of the root but the key constraint
@@ -181,16 +181,16 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 //	err := os.MkdirAll(path, mode)
 //
 // But is much safer. The above implementation is unsafe because if an attacker
-// can modify the filesystem tree between SecureJoin and MkdirAll, it is
+// can modify the filesystem tree between [SecureJoin] and [os.MkdirAll], it is
 // possible for MkdirAll to resolve unsafe symlink components and create
 // directories outside of the root.
 //
 // If you plan to open the directory after you have created it or want to use
-// an open directory handle as the root, you should use MkdirAllHandle instead.
-// This function is a wrapper around MkdirAllHandle.
+// an open directory handle as the root, you should use [MkdirAllHandle] instead.
+// This function is a wrapper around [MkdirAllHandle].
 //
 // NOTE: The mode argument must be set the unix mode bits (unix.S_I...), not
-// the Go generic mode bits (os.Mode...).
+// the Go generic mode bits ([os.FileMode]...).
 func MkdirAll(root, unsafePath string, mode int) error {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	if err != nil {

open_linux.go

@@ -14,8 +14,8 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-// OpenatInRoot is equivalent to OpenInRoot, except that the root is provided
-// using an *os.File handle, to ensure that the correct root directory is used.
+// OpenatInRoot is equivalent to [OpenInRoot], except that the root is provided
+// using an *[os.File] handle, to ensure that the correct root directory is used.
 func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) {
 	handle, err := completeLookupInRoot(root, unsafePath)
 	if err != nil {
@@ -31,15 +31,15 @@ func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) {
 //	handle, err := os.OpenFile(path, unix.O_PATH|unix.O_CLOEXEC)
 //
 // But is much safer. The above implementation is unsafe because if an attacker
-// can modify the filesystem tree between SecureJoin and OpenFile, it is
+// can modify the filesystem tree between [SecureJoin] and [os.OpenFile], it is
 // possible for the returned file to be outside of the root.
 //
 // Note that the returned handle is an O_PATH handle, meaning that only a very
 // limited set of operations will work on the handle. This is done to avoid
 // accidentally opening an untrusted file that could cause issues (such as a
 // disconnected TTY that could cause a DoS, or some other issue). In order to
 // use the returned handle, you can "upgrade" it to a proper handle using
-// Reopen.
+// [Reopen].
 func OpenInRoot(root, unsafePath string) (*os.File, error) {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	if err != nil {
@@ -49,7 +49,7 @@ func OpenInRoot(root, unsafePath string) (*os.File, error) {
 	return OpenatInRoot(rootDir, unsafePath)
 }
 
-// Reopen takes an *os.File handle and re-opens it through /proc/self/fd.
+// Reopen takes an *[os.File] handle and re-opens it through /proc/self/fd.
 // Reopen(file, flags) is effectively equivalent to
 //
 //	fdPath := fmt.Sprintf("/proc/self/fd/%d", file.Fd())
@@ -59,7 +59,9 @@ func OpenInRoot(root, unsafePath string) (*os.File, error) {
 // maliciously-configured /proc mount. While this attack scenario is not
 // common, in container runtimes it is possible for higher-level runtimes to be
 // tricked into configuring an unsafe /proc that can be used to attack file
-// operations. See CVE-2019-19921 for more details.
+// operations. See [CVE-2019-19921] for more details.
+//
+// [CVE-2019-19921]: https://github.com/advisories/GHSA-fh74-hm69-rqjw
 func Reopen(handle *os.File, flags int) (*os.File, error) {
 	procRoot, err := getProcRoot()
 	if err != nil {

vfs.go

@@ -10,32 +10,26 @@ import "os"
 // are several projects (umoci and go-mtree) that are using this sort of
 // interface.
 
-// VFS is the minimal interface necessary to use SecureJoinVFS. A nil VFS is
-// equivalent to using the standard os.* family of functions. This is mainly
+// VFS is the minimal interface necessary to use [SecureJoinVFS]. A nil VFS is
+// equivalent to using the standard [os].* family of functions. This is mainly
 // used for the purposes of mock testing, but also can be used to otherwise use
-// SecureJoin with VFS-like system.
+// [SecureJoinVFS] with VFS-like system.
 type VFS interface {
-	// Lstat returns a FileInfo describing the named file. If the file is a
-	// symbolic link, the returned FileInfo describes the symbolic link. Lstat
-	// makes no attempt to follow the link. These semantics are identical to
-	// os.Lstat.
+	// Lstat returns an [os.FileInfo] describing the named file. If the
+	// file is a symbolic link, the returned [os.FileInfo] describes the
+	// symbolic link. Lstat makes no attempt to follow the link.
+	// The semantics are identical to [os.Lstat].
 	Lstat(name string) (os.FileInfo, error)
 
-	// Readlink returns the destination of the named symbolic link. These
-	// semantics are identical to os.Readlink.
+	// Readlink returns the destination of the named symbolic link.
+	// The semantics are identical to [os.Readlink].
 	Readlink(name string) (string, error)
 }
 
 // osVFS is the "nil" VFS, in that it just passes everything through to the os
 // module.
 type osVFS struct{}
 
-// Lstat returns a FileInfo describing the named file. If the file is a
-// symbolic link, the returned FileInfo describes the symbolic link. Lstat
-// makes no attempt to follow the link. These semantics are identical to
-// os.Lstat.
 func (o osVFS) Lstat(name string) (os.FileInfo, error) { return os.Lstat(name) }
 
-// Readlink returns the destination of the named symbolic link. These
-// semantics are identical to os.Readlink.
 func (o osVFS) Readlink(name string) (string, error) { return os.Readlink(name) }