From 419d04d05416c1de98d773d43e4aca20581586bf Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Tue, 3 Jun 2025 17:18:02 -0600 Subject: [PATCH 1/4] MWI: Fix missing O_CLOEXEC in `botfs.openSecure()` and other issues This fixes various issues in botfs's `openSecure` implementation for Linux, particularly: - Added missing `O_CLOEXEC` flag to `openat2()` flags - Added EINTR retry loop to follow the stdlib's `OpenFile()` implementation. - Remove `O_CREATE` from `ReadFlags`, and convert it to a pseudo-enum so we can accurately predict intent (see next item) - Pass `0` mode to `openat2()` when reading (see above) - Renamed confusingly named `OpenMode` to `OpenFlags`, because it corresponds to flags, not file modes. - Fixed coincidentally correct misuse of `unix.O_RDONLY` Note that while removing `O_CREATE` from read flags is technically a breaking change, we still automatically files downstream as needed. This results in a "new" debug-level log, but not any actual behavior change. --- lib/tbot/botfs/botfs.go | 37 ++++++++++++++-------- lib/tbot/botfs/fs_linux.go | 59 ++++++++++++++++++++++-------------- lib/tbot/botfs/fs_unix.go | 4 +-- lib/tbot/botfs/fs_windows.go | 4 +-- 4 files changed, 64 insertions(+), 40 deletions(-) diff --git a/lib/tbot/botfs/botfs.go b/lib/tbot/botfs/botfs.go index dd834200836bd..0e38d2d915766 100644 --- a/lib/tbot/botfs/botfs.go +++ b/lib/tbot/botfs/botfs.go @@ -66,12 +66,7 @@ const ( // ACLRequired enables ACL support and fails if ACLs are unavailable. ACLRequired ACLMode = "required" -) - -// OpenMode is a mode for opening files. -type OpenMode int -const ( // DefaultMode is the preferred permissions mode for bot files. DefaultMode fs.FileMode = 0600 @@ -79,16 +74,32 @@ const ( // Directories need the execute bit set for most operations on their // contents to succeed. DefaultDirMode fs.FileMode = 0700 +) - // ReadMode is the mode with which files should be opened for reading and - // writing. - ReadMode OpenMode = OpenMode(os.O_CREATE | os.O_RDONLY) +// OpenFlags is a bitmask containing flags passed to `open()` +type OpenFlags int - // WriteMode is the mode with which files should be opened specifically +const ( + // ReadFlags contains `open()` flags to be used when opening files for + // reading. The file will be created if it does not exist, and reads should + // return an empty byte array. + ReadFlags = iota + + // WriteFlags is the mode with which files should be opened specifically // for writing. - WriteMode OpenMode = OpenMode(os.O_CREATE | os.O_WRONLY | os.O_TRUNC) + WriteFlags ) +// Flags returns opening flags for this OpenFlags variant. +func (f OpenFlags) Flags() int { + switch f { + case WriteFlags: + return os.O_CREATE | os.O_WRONLY | os.O_TRUNC + default: + return os.O_RDONLY + } +} + // ACLOptions contains parameters needed to configure ACLs type ACLOptions struct { // BotUser is the bot user that should have write access to this entry @@ -126,8 +137,8 @@ func (s *ACLSelector) CheckAndSetDefaults() error { // openStandard attempts to open the given path for reading and writing with // O_CREATE set. -func openStandard(path string, mode OpenMode) (*os.File, error) { - file, err := os.OpenFile(path, int(mode), DefaultMode) +func openStandard(path string, flags OpenFlags) (*os.File, error) { + file, err := os.OpenFile(path, flags.Flags(), DefaultMode) if err != nil { return nil, trace.ConvertSystemError(err) } @@ -146,7 +157,7 @@ func createStandard(path string, isDir bool) error { return nil } - f, err := openStandard(path, WriteMode) + f, err := openStandard(path, WriteFlags) if err != nil { return trace.Wrap(err) } diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index e7d1097d3bfba..45bd388a1d2fc 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -63,45 +63,58 @@ const modeACLNone fs.FileMode = 0 // missingSyscallWarning is used to reduce log spam when a syscall is missing. var missingSyscallWarning sync.Once -// openSecure opens the given path for writing (with O_CREAT, mode 0600) -// with the RESOLVE_NO_SYMLINKS flag set. -func openSecure(path string, mode OpenMode) (*os.File, error) { +// openSecure opens the given path for either reading or writing with the +// RESOLVE_NO_SYMLINKS flag set. +func openSecure(path string, flags OpenFlags) (*os.File, error) { + var mode uint64 + if flags == ReadFlags { + // openat2() with nonzero mode will raise EINVAL unless O_CREATE or + // O_TMPFILE is set, so zero it out. + mode = 0 + } else { + mode = uint64(DefaultMode.Perm()) + } + how := unix.OpenHow{ - // Equivalent to 0600. Unfortunately it's not worth reusing our - // default file mode constant here. - Mode: unix.O_RDONLY | unix.S_IRUSR | unix.S_IWUSR, - Flags: uint64(mode), + Mode: mode, + Flags: uint64(flags.Flags()) | unix.O_CLOEXEC, Resolve: unix.RESOLVE_NO_SYMLINKS, } - fd, err := unix.Openat2(unix.AT_FDCWD, path, &how) - if err != nil { - // note: returning the original error here for comparison purposes - return nil, err - } + for { + fd, err := unix.Openat2(unix.AT_FDCWD, path, &how) + if err == syscall.EINTR { + // Per the stdlib's implementation, EINTR errors should be ignored + // and retried. + continue + } else if err != nil { + // note: returning the original error here for comparison purposes + return nil, err + } - // os.File.Close() appears to close wrapped files sanely, so rely on that - // rather than relying on callers to use unix.Close(fd) - return os.NewFile(uintptr(fd), filepath.Base(path)), nil + // os.File.Close() appears to close wrapped files sanely, so rely on that + // rather than relying on callers to use unix.Close(fd) + return os.NewFile(uintptr(fd), filepath.Base(path)), nil + } } // openSymlinks mode opens the file for read or write using the given symlink // mode, potentially failing or logging a warning if symlinks can't be // secured. -func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*os.File, error) { +func openSymlinksMode(path string, flags OpenFlags, symlinksMode SymlinksMode) (*os.File, error) { var file *os.File var err error switch symlinksMode { case SymlinksSecure: - file, err = openSecure(path, mode) + file, err = openSecure(path, flags) if errors.Is(err, unix.ENOSYS) { return nil, trace.Errorf("openSecure failed due to missing syscall; configure `symlinks: insecure` for %q", path) } else if err != nil { return nil, trace.Wrap(err) } case SymlinksTrySecure: - file, err = openSecure(path, mode) + file, err = openSecure(path, flags) if errors.Is(err, unix.ENOSYS) { missingSyscallWarning.Do(func() { log.DebugContext( @@ -111,7 +124,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o ) }) - file, err = openStandard(path, mode) + file, err = openStandard(path, flags) if err != nil { return nil, trace.Wrap(err) } @@ -119,7 +132,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o return nil, trace.Wrap(err) } case SymlinksInsecure: - file, err = openStandard(path, mode) + file, err = openStandard(path, flags) if err != nil { return nil, trace.Wrap(err) } @@ -145,7 +158,7 @@ func createSecure(path string, isDir bool) error { return nil } - f, err := openSecure(path, WriteMode) + f, err := openSecure(path, WriteFlags) if errors.Is(err, unix.ENOSYS) { // bubble up the original error for comparison return err @@ -218,7 +231,7 @@ func Create(path string, isDir bool, symlinksMode SymlinksMode) error { // Read reads the contents of the given file into memory. func Read(path string, symlinksMode SymlinksMode) ([]byte, error) { - file, err := openSymlinksMode(path, ReadMode, symlinksMode) + file, err := openSymlinksMode(path, ReadFlags, symlinksMode) if err != nil { return nil, trace.Wrap(err) } @@ -235,7 +248,7 @@ func Read(path string, symlinksMode SymlinksMode) ([]byte, error) { // Write stores the given data to the file at the given path. func Write(path string, data []byte, symlinksMode SymlinksMode) error { - file, err := openSymlinksMode(path, WriteMode, symlinksMode) + file, err := openSymlinksMode(path, WriteFlags, symlinksMode) if err != nil { return trace.Wrap(err) } diff --git a/lib/tbot/botfs/fs_unix.go b/lib/tbot/botfs/fs_unix.go index ad6d417e7836f..880b2d5e9d1c9 100644 --- a/lib/tbot/botfs/fs_unix.go +++ b/lib/tbot/botfs/fs_unix.go @@ -61,7 +61,7 @@ func Read(path string, symlinksMode SymlinksMode) ([]byte, error) { }) } - file, err := openStandard(path, ReadMode) + file, err := openStandard(path, ReadFlags) if err != nil { return nil, trace.Wrap(err) } @@ -91,7 +91,7 @@ func Write(path string, data []byte, symlinksMode SymlinksMode) error { }) } - file, err := openStandard(path, WriteMode) + file, err := openStandard(path, WriteFlags) if err != nil { return trace.Wrap(err) } diff --git a/lib/tbot/botfs/fs_windows.go b/lib/tbot/botfs/fs_windows.go index 41ef28ab8e66d..5f90fd4c2ca40 100644 --- a/lib/tbot/botfs/fs_windows.go +++ b/lib/tbot/botfs/fs_windows.go @@ -62,7 +62,7 @@ func Read(path string, symlinksMode SymlinksMode) ([]byte, error) { }) } - file, err := openStandard(path, ReadMode) + file, err := openStandard(path, ReadFlags) if err != nil { return nil, trace.Wrap(err) } @@ -92,7 +92,7 @@ func Write(path string, data []byte, symlinksMode SymlinksMode) error { }) } - file, err := openStandard(path, WriteMode) + file, err := openStandard(path, WriteFlags) if err != nil { return trace.Wrap(err) } From 918d54ed685472fd63bb8d499d61b58aef0b192e Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Tue, 3 Jun 2025 17:54:00 -0600 Subject: [PATCH 2/4] Fix lint --- lib/tbot/botfs/fs_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index 45bd388a1d2fc..49924f99ce379 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -83,7 +83,7 @@ func openSecure(path string, flags OpenFlags) (*os.File, error) { for { fd, err := unix.Openat2(unix.AT_FDCWD, path, &how) - if err == syscall.EINTR { + if errors.Is(err, syscall.EINTR) { // Per the stdlib's implementation, EINTR errors should be ignored // and retried. continue From 17a15ff88991e551d8931c6537f70616e63e4fae Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Wed, 4 Jun 2025 10:34:58 -0600 Subject: [PATCH 3/4] Simplify mode conditional, fix comments --- lib/tbot/botfs/botfs.go | 4 ++-- lib/tbot/botfs/fs_linux.go | 8 +++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/tbot/botfs/botfs.go b/lib/tbot/botfs/botfs.go index 0e38d2d915766..6007f04859dda 100644 --- a/lib/tbot/botfs/botfs.go +++ b/lib/tbot/botfs/botfs.go @@ -135,8 +135,8 @@ func (s *ACLSelector) CheckAndSetDefaults() error { return nil } -// openStandard attempts to open the given path for reading and writing with -// O_CREATE set. +// openStandard attempts to open the given path. The file may be writable +// depending on the provided `OpenFlags` value. func openStandard(path string, flags OpenFlags) (*os.File, error) { file, err := os.OpenFile(path, flags.Flags(), DefaultMode) if err != nil { diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index 49924f99ce379..5525b00c1a2a0 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -67,11 +67,9 @@ var missingSyscallWarning sync.Once // RESOLVE_NO_SYMLINKS flag set. func openSecure(path string, flags OpenFlags) (*os.File, error) { var mode uint64 - if flags == ReadFlags { - // openat2() with nonzero mode will raise EINVAL unless O_CREATE or - // O_TMPFILE is set, so zero it out. - mode = 0 - } else { + if flags != ReadFlags { + // openat2() with a nonzero mode will raise EINVAL unless O_CREATE or + // O_TMPFILE is set, so only set this in non-read mode. mode = uint64(DefaultMode.Perm()) } From 34b65be26ed1edacc373711147fd7c9c5289663c Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Wed, 4 Jun 2025 10:50:21 -0600 Subject: [PATCH 4/4] Handle potentially nil file returns from `os.OpenFile` --- lib/tbot/botfs/fs_linux.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index 5525b00c1a2a0..75e1ea9c4a8d8 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -29,7 +29,6 @@ import ( "io/fs" "os" "os/user" - "path/filepath" "strconv" "sync" "syscall" @@ -90,9 +89,15 @@ func openSecure(path string, flags OpenFlags) (*os.File, error) { return nil, err } - // os.File.Close() appears to close wrapped files sanely, so rely on that - // rather than relying on callers to use unix.Close(fd) - return os.NewFile(uintptr(fd), filepath.Base(path)), nil + file := os.NewFile(uintptr(fd), path) + if file == nil { + // Probably useless since this implies the fd itself is invalid, but + // attempt to close the fd anyway. + _ = unix.Close(fd) + return nil, os.ErrInvalid + } + + return file, nil } }