From daead99c02019c22d73e06180e2ecadc17f2e999 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 23 Sep 2024 13:07:15 -0700 Subject: [PATCH 1/3] Remove osVFS methods documentation These methods are not public, and their docstrings are copies of those from the VFS interface methods. Signed-off-by: Kir Kolyshkin --- vfs.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/vfs.go b/vfs.go index 6e27c7d..1932eb3 100644 --- a/vfs.go +++ b/vfs.go @@ -30,12 +30,6 @@ type VFS interface { // 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) } From 5b5a7a4e1986087025ec622968f14ac953fc6aef Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 23 Sep 2024 13:30:05 -0700 Subject: [PATCH 2/3] Add cross-links to godoc Since Go 1.19 it is possible to link to other identifiers. Let's use it. Signed-off-by: Kir Kolyshkin --- join.go | 16 ++++++++-------- mkdir_linux.go | 26 +++++++++++++------------- open_linux.go | 10 +++++----- vfs.go | 18 +++++++++--------- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/join.go b/join.go index bd86a48..ca4ce1f 100644 --- a/join.go +++ b/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) } diff --git a/mkdir_linux.go b/mkdir_linux.go index e9801bb..b5f6745 100644 --- a/mkdir_linux.go +++ b/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 { diff --git a/open_linux.go b/open_linux.go index 52dce76..46287c9 100644 --- a/open_linux.go +++ b/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,7 +31,7 @@ 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 @@ -39,7 +39,7 @@ func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) { // 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()) diff --git a/vfs.go b/vfs.go index 1932eb3..36373f8 100644 --- a/vfs.go +++ b/vfs.go @@ -10,19 +10,19 @@ 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) } From 09afcf2d145829620dbf964bfba926f1098986cd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 23 Sep 2024 13:36:50 -0700 Subject: [PATCH 3/3] OpenInRoot: add CVE link to godoc Signed-off-by: Kir Kolyshkin --- open_linux.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/open_linux.go b/open_linux.go index 46287c9..230be73 100644 --- a/open_linux.go +++ b/open_linux.go @@ -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 {