From f5b36dc4ad81e12945f2fda473fd5a196c03ce38 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Mon, 11 Sep 2023 10:51:59 -0600 Subject: [PATCH 01/16] Careful handling when loading files This commit attempts to provide a common API so that the decision of when to follow symlinks is a conscious decision. Because Teleport (particularly the agent) runs in a privilege context, there is risk that following symlinks may allow information disclosure. After review of the cases covered in this commit (and some additional cases where this API was not a natural fit), this does not appear to be a broad problem. This commit however does fix the one known flaw described in the issue https://github.com/gravitational/teleport-private/issues/1009 --- lib/cgroup/cgroup.go | 3 ++- lib/config/configuration.go | 2 +- lib/config/fileconf.go | 2 +- lib/srv/exec.go | 3 ++- lib/tbot/config/config.go | 3 +-- lib/utils/environment.go | 3 +-- lib/utils/fs.go | 8 ++++++-- lib/utils/kernel.go | 4 ++-- tool/tctl/common/edit_command.go | 4 ++-- tool/tctl/common/resource_command.go | 2 +- tool/tctl/sso/tester/command.go | 2 +- 11 files changed, 20 insertions(+), 16 deletions(-) diff --git a/lib/cgroup/cgroup.go b/lib/cgroup/cgroup.go index 41f691222b849..36ec8cfcec7ce 100644 --- a/lib/cgroup/cgroup.go +++ b/lib/cgroup/cgroup.go @@ -41,6 +41,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/defaults" + "github.com/gravitational/teleport/lib/utils" ) var log = logrus.WithFields(logrus.Fields{ @@ -171,7 +172,7 @@ func (s *Service) Place(sessionID string, pid int) error { // readPids returns a slice of PIDs from a file. Used to get list of all PIDs // within a cgroup. func readPids(path string) ([]string, error) { - f, err := os.Open(path) + f, err := utils.OpenFile(path, false) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/configuration.go b/lib/config/configuration.go index b09e2810b7f6f..166b93f18f38b 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -271,7 +271,7 @@ func ReadConfigFile(cliConfigPath string) (*FileConfig, error) { // ReadResources loads a set of resources from a file. func ReadResources(filePath string) ([]types.Resource, error) { - reader, err := utils.OpenFile(filePath) + reader, err := utils.OpenFile(filePath, true) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/fileconf.go b/lib/config/fileconf.go index a362fef25d6c2..1a7aa7b800394 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -104,7 +104,7 @@ type FileConfig struct { // ReadFromFile reads Teleport configuration from a file. Currently only YAML // format is supported func ReadFromFile(filePath string) (*FileConfig, error) { - f, err := os.Open(filePath) + f, err := utils.OpenFile(filePath, true) if err != nil { if errors.Is(err, fs.ErrPermission) { return nil, trace.Wrap(err, "failed to open file for Teleport configuration: %v. Ensure that you are running as a user with appropriate permissions.", filePath) diff --git a/lib/srv/exec.go b/lib/srv/exec.go index d605e9520a725..e7f7a93554f07 100644 --- a/lib/srv/exec.go +++ b/lib/srv/exec.go @@ -41,6 +41,7 @@ import ( apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/utils" ) const ( @@ -526,7 +527,7 @@ func getDefaultEnvPath(uid string, loginDefsPath string) string { envRootPath := defaultEnvRootPath // open file, if it doesn't exist return a default path and move on - f, err := os.Open(loginDefsPath) + f, err := utils.OpenFile(loginDefsPath, true) if err != nil { if uid == "0" { log.Infof("Unable to open %q: %v: returning default su path: %q", loginDefsPath, err, defaultEnvRootPath) diff --git a/lib/tbot/config/config.go b/lib/tbot/config/config.go index 320019967f9f3..0bce57b8530ef 100644 --- a/lib/tbot/config/config.go +++ b/lib/tbot/config/config.go @@ -20,7 +20,6 @@ import ( "fmt" "io" "net/url" - "os" "reflect" "strings" "time" @@ -663,7 +662,7 @@ func FromCLIConf(cf *CLIConf) (*BotConfig, error) { // ReadConfigFromFile reads and parses a YAML config from a file. func ReadConfigFromFile(filePath string, manualMigration bool) (*BotConfig, error) { - f, err := os.Open(filePath) + f, err := utils.OpenFile(filePath, true) if err != nil { return nil, trace.Wrap(err, fmt.Sprintf("failed to open file: %v", filePath)) } diff --git a/lib/utils/environment.go b/lib/utils/environment.go index eaa98bb81d20a..ff1d5383e538e 100644 --- a/lib/utils/environment.go +++ b/lib/utils/environment.go @@ -16,7 +16,6 @@ package utils import ( "bufio" - "os" "strings" log "github.com/sirupsen/logrus" @@ -30,7 +29,7 @@ import ( func ReadEnvironmentFile(filename string) ([]string, error) { // open the users environment file. if we don't find a file, move on as // having this file for the user is optional. - file, err := os.Open(filename) + file, err := OpenFile(filename, false) if err != nil { log.Warnf("Unable to open environment file %v: %v, skipping", filename, err) return []string{}, nil diff --git a/lib/utils/fs.go b/lib/utils/fs.go index a78c66323277c..c203ff6b0a6f9 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -96,8 +96,9 @@ func NormalizePath(path string) (string, error) { return abs, nil } -// OpenFile opens file and returns file handle -func OpenFile(path string) (*os.File, error) { +// OpenFile opens a file and returns file handle. In general symlinks should not be allowed to reduce the risk of +// privilege escalation from Teleports elevated privileges to potentially less privileged users accidentally. +func OpenFile(path string, allowSymlink bool) (*os.File, error) { newPath, err := NormalizePath(path) if err != nil { return nil, trace.Wrap(err) @@ -106,6 +107,9 @@ func OpenFile(path string) (*os.File, error) { if err != nil { return nil, trace.ConvertSystemError(err) } + if !allowSymlink && fi.Mode().Type()&os.ModeSymlink != 0 { + return nil, trace.BadParameter("symlink not allowed") + } if fi.IsDir() { return nil, trace.BadParameter("%v is not a file", path) } diff --git a/lib/utils/kernel.go b/lib/utils/kernel.go index 57f64a03d1170..90b05efdf2112 100644 --- a/lib/utils/kernel.go +++ b/lib/utils/kernel.go @@ -36,7 +36,7 @@ func KernelVersion() (*semver.Version, error) { return nil, trace.BadParameter("requested kernel version on non-Linux host") } - file, err := os.Open("/proc/sys/kernel/osrelease") + file, err := OpenFile("/proc/sys/kernel/osrelease", false) if err != nil { return nil, trace.Wrap(err) } @@ -89,7 +89,7 @@ func HasBTF() error { return trace.BadParameter("requested kernel version on non-Linux host") } - file, err := os.Open(btfFile) + file, err := OpenFile(btfFile, false) if err == nil { file.Close() return nil diff --git a/tool/tctl/common/edit_command.go b/tool/tctl/common/edit_command.go index 3ff27cec8fc7e..0a55f655a2820 100644 --- a/tool/tctl/common/edit_command.go +++ b/tool/tctl/common/edit_command.go @@ -153,7 +153,7 @@ func editor() string { } func checksum(filename string) (string, error) { - f, err := utils.OpenFile(filename) + f, err := utils.OpenFile(filename, true) if err != nil { return "", trace.Wrap(err) } @@ -168,7 +168,7 @@ func checksum(filename string) (string, error) { } func resourceName(filename string) (string, error) { - f, err := utils.OpenFile(filename) + f, err := utils.OpenFile(filename, true) if err != nil { return "", trace.Wrap(err) } diff --git a/tool/tctl/common/resource_command.go b/tool/tctl/common/resource_command.go index 0e5a380dd3379..d973daac7c82a 100644 --- a/tool/tctl/common/resource_command.go +++ b/tool/tctl/common/resource_command.go @@ -270,7 +270,7 @@ func (rc *ResourceCommand) Create(ctx context.Context, client auth.ClientI) (err if rc.filename == "" { reader = os.Stdin } else { - f, err := utils.OpenFile(rc.filename) + f, err := utils.OpenFile(rc.filename, true) if err != nil { return trace.Wrap(err) } diff --git a/tool/tctl/sso/tester/command.go b/tool/tctl/sso/tester/command.go index a726e8f6328f4..7f73030fab062 100644 --- a/tool/tctl/sso/tester/command.go +++ b/tool/tctl/sso/tester/command.go @@ -98,7 +98,7 @@ func (cmd *SSOTestCommand) getSupportedKinds() []string { func (cmd *SSOTestCommand) ssoTestCommand(ctx context.Context, c auth.ClientI) error { reader := os.Stdin if cmd.connectorFileName != "" { - f, err := utils.OpenFile(cmd.connectorFileName) + f, err := utils.OpenFile(cmd.connectorFileName, true) if err != nil { return trace.Wrap(err, "could not open connector spec file %v", cmd.connectorFileName) } From f8de6a35081696664b9b72d08e8b24311d16762b Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Mon, 11 Sep 2023 12:59:41 -0600 Subject: [PATCH 02/16] Apply PR feedback to rename OpenFile --- lib/cgroup/cgroup.go | 2 +- lib/config/configuration.go | 2 +- lib/config/fileconf.go | 2 +- lib/srv/exec.go | 2 +- lib/tbot/config/config.go | 2 +- lib/utils/environment.go | 2 +- lib/utils/fs.go | 15 +++++++++++++-- lib/utils/kernel.go | 4 ++-- tool/tctl/common/edit_command.go | 4 ++-- tool/tctl/common/resource_command.go | 2 +- tool/tctl/sso/tester/command.go | 2 +- 11 files changed, 25 insertions(+), 14 deletions(-) diff --git a/lib/cgroup/cgroup.go b/lib/cgroup/cgroup.go index 36ec8cfcec7ce..4060af4eb256c 100644 --- a/lib/cgroup/cgroup.go +++ b/lib/cgroup/cgroup.go @@ -172,7 +172,7 @@ func (s *Service) Place(sessionID string, pid int) error { // readPids returns a slice of PIDs from a file. Used to get list of all PIDs // within a cgroup. func readPids(path string) ([]string, error) { - f, err := utils.OpenFile(path, false) + f, err := utils.OpenFileNoSymlinks(path) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 166b93f18f38b..9099d23b559f2 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -271,7 +271,7 @@ func ReadConfigFile(cliConfigPath string) (*FileConfig, error) { // ReadResources loads a set of resources from a file. func ReadResources(filePath string) ([]types.Resource, error) { - reader, err := utils.OpenFile(filePath, true) + reader, err := utils.OpenFileAllowingSymlinks(filePath) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/fileconf.go b/lib/config/fileconf.go index 1a7aa7b800394..a83f111abe125 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -104,7 +104,7 @@ type FileConfig struct { // ReadFromFile reads Teleport configuration from a file. Currently only YAML // format is supported func ReadFromFile(filePath string) (*FileConfig, error) { - f, err := utils.OpenFile(filePath, true) + f, err := utils.OpenFileAllowingSymlinks(filePath) if err != nil { if errors.Is(err, fs.ErrPermission) { return nil, trace.Wrap(err, "failed to open file for Teleport configuration: %v. Ensure that you are running as a user with appropriate permissions.", filePath) diff --git a/lib/srv/exec.go b/lib/srv/exec.go index e7f7a93554f07..3c66727c3bd95 100644 --- a/lib/srv/exec.go +++ b/lib/srv/exec.go @@ -527,7 +527,7 @@ func getDefaultEnvPath(uid string, loginDefsPath string) string { envRootPath := defaultEnvRootPath // open file, if it doesn't exist return a default path and move on - f, err := utils.OpenFile(loginDefsPath, true) + f, err := utils.OpenFileAllowingSymlinks(loginDefsPath) if err != nil { if uid == "0" { log.Infof("Unable to open %q: %v: returning default su path: %q", loginDefsPath, err, defaultEnvRootPath) diff --git a/lib/tbot/config/config.go b/lib/tbot/config/config.go index 0bce57b8530ef..23bb435e69bbc 100644 --- a/lib/tbot/config/config.go +++ b/lib/tbot/config/config.go @@ -662,7 +662,7 @@ func FromCLIConf(cf *CLIConf) (*BotConfig, error) { // ReadConfigFromFile reads and parses a YAML config from a file. func ReadConfigFromFile(filePath string, manualMigration bool) (*BotConfig, error) { - f, err := utils.OpenFile(filePath, true) + f, err := utils.OpenFileAllowingSymlinks(filePath) if err != nil { return nil, trace.Wrap(err, fmt.Sprintf("failed to open file: %v", filePath)) } diff --git a/lib/utils/environment.go b/lib/utils/environment.go index ff1d5383e538e..aa7e9b4b33230 100644 --- a/lib/utils/environment.go +++ b/lib/utils/environment.go @@ -29,7 +29,7 @@ import ( func ReadEnvironmentFile(filename string) ([]string, error) { // open the users environment file. if we don't find a file, move on as // having this file for the user is optional. - file, err := OpenFile(filename, false) + file, err := OpenFileNoSymlinks(filename) if err != nil { log.Warnf("Unable to open environment file %v: %v, skipping", filename, err) return []string{}, nil diff --git a/lib/utils/fs.go b/lib/utils/fs.go index c203ff6b0a6f9..45a7224a5eac1 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -96,9 +96,20 @@ func NormalizePath(path string) (string, error) { return abs, nil } -// OpenFile opens a file and returns file handle. In general symlinks should not be allowed to reduce the risk of +// OpenFileAllowingSymlinks will open a file, if it's a symlink the file referenced by the link will be associated to +// the returned os.File. This will return an error if the file is not found or is a directory. +func OpenFileAllowingSymlinks(path string) (*os.File, error) { + return openFile(path, true) +} + +// OpenFileNoSymlinks will open a file, ensuring it's an actual file and not a directory or symlink. +func OpenFileNoSymlinks(path string) (*os.File, error) { + return openFile(path, false) +} + +// openFile opens a file and returns file handle. In general symlinks should not be allowed to reduce the risk of // privilege escalation from Teleports elevated privileges to potentially less privileged users accidentally. -func OpenFile(path string, allowSymlink bool) (*os.File, error) { +func openFile(path string, allowSymlink bool) (*os.File, error) { newPath, err := NormalizePath(path) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/utils/kernel.go b/lib/utils/kernel.go index 90b05efdf2112..1ca6f570a2f4d 100644 --- a/lib/utils/kernel.go +++ b/lib/utils/kernel.go @@ -36,7 +36,7 @@ func KernelVersion() (*semver.Version, error) { return nil, trace.BadParameter("requested kernel version on non-Linux host") } - file, err := OpenFile("/proc/sys/kernel/osrelease", false) + file, err := OpenFileNoSymlinks("/proc/sys/kernel/osrelease") if err != nil { return nil, trace.Wrap(err) } @@ -89,7 +89,7 @@ func HasBTF() error { return trace.BadParameter("requested kernel version on non-Linux host") } - file, err := OpenFile(btfFile, false) + file, err := OpenFileNoSymlinks(btfFile) if err == nil { file.Close() return nil diff --git a/tool/tctl/common/edit_command.go b/tool/tctl/common/edit_command.go index 0a55f655a2820..87c3d375c83ec 100644 --- a/tool/tctl/common/edit_command.go +++ b/tool/tctl/common/edit_command.go @@ -153,7 +153,7 @@ func editor() string { } func checksum(filename string) (string, error) { - f, err := utils.OpenFile(filename, true) + f, err := utils.OpenFileAllowingSymlinks(filename) if err != nil { return "", trace.Wrap(err) } @@ -168,7 +168,7 @@ func checksum(filename string) (string, error) { } func resourceName(filename string) (string, error) { - f, err := utils.OpenFile(filename, true) + f, err := utils.OpenFileAllowingSymlinks(filename) if err != nil { return "", trace.Wrap(err) } diff --git a/tool/tctl/common/resource_command.go b/tool/tctl/common/resource_command.go index d973daac7c82a..801c2d51d0fff 100644 --- a/tool/tctl/common/resource_command.go +++ b/tool/tctl/common/resource_command.go @@ -270,7 +270,7 @@ func (rc *ResourceCommand) Create(ctx context.Context, client auth.ClientI) (err if rc.filename == "" { reader = os.Stdin } else { - f, err := utils.OpenFile(rc.filename, true) + f, err := utils.OpenFileAllowingSymlinks(rc.filename) if err != nil { return trace.Wrap(err) } diff --git a/tool/tctl/sso/tester/command.go b/tool/tctl/sso/tester/command.go index 7f73030fab062..abe4689063358 100644 --- a/tool/tctl/sso/tester/command.go +++ b/tool/tctl/sso/tester/command.go @@ -98,7 +98,7 @@ func (cmd *SSOTestCommand) getSupportedKinds() []string { func (cmd *SSOTestCommand) ssoTestCommand(ctx context.Context, c auth.ClientI) error { reader := os.Stdin if cmd.connectorFileName != "" { - f, err := utils.OpenFile(cmd.connectorFileName, true) + f, err := utils.OpenFileAllowingSymlinks(cmd.connectorFileName) if err != nil { return trace.Wrap(err, "could not open connector spec file %v", cmd.connectorFileName) } From a1212fc03af433ab591de0a54460e21534deadbf Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Mon, 11 Sep 2023 13:38:24 -0600 Subject: [PATCH 03/16] fs.go: Fix symlink evaluation --- lib/utils/fs.go | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 45a7224a5eac1..a827b7b94509a 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -23,6 +23,7 @@ import ( "io" "os" "path/filepath" + "strings" "time" "github.com/gofrs/flock" @@ -84,16 +85,18 @@ func IsDir(path string) bool { // NormalizePath normalises path, evaluating symlinks and converting local // paths to absolute -func NormalizePath(path string) (string, error) { +func NormalizePath(path string, evaluateSymlinks bool) (string, error) { s, err := filepath.Abs(path) if err != nil { return "", trace.ConvertSystemError(err) } - abs, err := filepath.EvalSymlinks(s) - if err != nil { - return "", trace.ConvertSystemError(err) + if evaluateSymlinks { + s, err = filepath.EvalSymlinks(s) + if err != nil { + return "", trace.ConvertSystemError(err) + } } - return abs, nil + return s, nil } // OpenFileAllowingSymlinks will open a file, if it's a symlink the file referenced by the link will be associated to @@ -110,16 +113,27 @@ func OpenFileNoSymlinks(path string) (*os.File, error) { // openFile opens a file and returns file handle. In general symlinks should not be allowed to reduce the risk of // privilege escalation from Teleports elevated privileges to potentially less privileged users accidentally. func openFile(path string, allowSymlink bool) (*os.File, error) { - newPath, err := NormalizePath(path) + newPath, err := NormalizePath(path, allowSymlink) if err != nil { return nil, trace.Wrap(err) } - fi, err := os.Stat(newPath) - if err != nil { - return nil, trace.ConvertSystemError(err) - } - if !allowSymlink && fi.Mode().Type()&os.ModeSymlink != 0 { - return nil, trace.BadParameter("symlink not allowed") + var fi os.FileInfo + if !allowSymlink { + components := strings.Split(newPath, string(os.PathSeparator)) + for i := 1; i <= len(components); i++ { + subPath := filepath.Join(components[:i]...) + fi, err = os.Lstat(subPath) + if err != nil { + return nil, trace.ConvertSystemError(err) + } else if fi.Mode().Type()&os.ModeSymlink != 0 { + return nil, trace.BadParameter("symlink not allowed in path: %v", subPath) + } + } + } else { + fi, err = os.Stat(newPath) + if err != nil { + return nil, trace.ConvertSystemError(err) + } } if fi.IsDir() { return nil, trace.BadParameter("%v is not a file", path) @@ -133,7 +147,7 @@ func openFile(path string, allowSymlink bool) (*os.File, error) { // StatFile stats path, returns error if it exists but a directory. func StatFile(path string) (os.FileInfo, error) { - newPath, err := NormalizePath(path) + newPath, err := NormalizePath(path, true) if err != nil { return nil, trace.Wrap(err) } From 5736c8ae51879b5447f536242b9d05f79b61dd09 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Mon, 11 Sep 2023 14:01:51 -0600 Subject: [PATCH 04/16] fs.go: Fix for rebuilding absolute paths in symlink check --- lib/utils/fs.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index a827b7b94509a..ec725672743e4 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -118,10 +118,19 @@ func openFile(path string, allowSymlink bool) (*os.File, error) { return nil, trace.Wrap(err) } var fi os.FileInfo - if !allowSymlink { + if allowSymlink { + fi, err = os.Stat(newPath) + if err != nil { + return nil, trace.ConvertSystemError(err) + } + } else { + isAbs := filepath.IsAbs(newPath) components := strings.Split(newPath, string(os.PathSeparator)) for i := 1; i <= len(components); i++ { subPath := filepath.Join(components[:i]...) + if isAbs { + subPath = string(os.PathSeparator) + subPath + } fi, err = os.Lstat(subPath) if err != nil { return nil, trace.ConvertSystemError(err) @@ -129,11 +138,6 @@ func openFile(path string, allowSymlink bool) (*os.File, error) { return nil, trace.BadParameter("symlink not allowed in path: %v", subPath) } } - } else { - fi, err = os.Stat(newPath) - if err != nil { - return nil, trace.ConvertSystemError(err) - } } if fi.IsDir() { return nil, trace.BadParameter("%v is not a file", path) From 7a59a793e13e4bb500c4229cb5ebccfd361507e8 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Tue, 12 Sep 2023 08:00:40 -0600 Subject: [PATCH 05/16] fs_test.go: Add symlink testing --- lib/utils/fs_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index b5fd2dce87594..c23b389f441b6 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -26,6 +26,80 @@ import ( "github.com/stretchr/testify/require" ) +func TestOpenFileSymlinks(t *testing.T) { + // symlink structure setup, this will produce the following structure under the temp directory created below: + // dir + // dir-s -> dir + // dir-s-s -> dir-s + // dir/file + // dir/file-s -> dir/file + // circular-s -> circular-s + // broken-s -> nonexistent + rootDir, err := os.MkdirTemp("", "symlink-test") + require.NoError(t, err) + defer os.RemoveAll(rootDir) + + dirPath := filepath.Join(rootDir, "dir") + err = os.Mkdir(dirPath, 0755) + require.NoError(t, err) + + filePath := filepath.Join(dirPath, "file") + _, err = os.Create(filePath) + require.NoError(t, err) + + symlinkToFile := filepath.Join(dirPath, "file-s") + err = os.Symlink(filePath, symlinkToFile) + require.NoError(t, err) + + symlinkDir := filepath.Join(rootDir, "dir-s") + err = os.Symlink(dirPath, symlinkDir) + require.NoError(t, err) + + symlinkToSymlinkDir := filepath.Join(rootDir, "dir-s-s") + err = os.Symlink(symlinkDir, symlinkToSymlinkDir) + require.NoError(t, err) + + circularSymlink := filepath.Join(rootDir, "circular-s") + err = os.Symlink(circularSymlink, circularSymlink) + require.NoError(t, err) + + brokenSymlink := filepath.Join(rootDir, "broken-s") + err = os.Symlink(filepath.Join(rootDir, "nonexistent"), brokenSymlink) + require.NoError(t, err) + + // Tests below can not be done with t.Parallel due to the need for directory cleanup in defer + t.Run("directFileOpenAllowed", func(t *testing.T) { + f, err := openFile(filePath, false) + require.NoError(t, err) + defer f.Close() + }) + t.Run("symlinkFileOpenAllowed", func(t *testing.T) { + f, err := openFile(symlinkToFile, true) + require.NoError(t, err) + defer f.Close() + }) + t.Run("symlinkFileOpenDenied", func(t *testing.T) { + _, err := openFile(symlinkToFile, false) + require.Error(t, err) + }) + t.Run("symlinkDirFileOpenDenied", func(t *testing.T) { + _, err := openFile(filepath.Join(symlinkDir, "file"), false) + require.Error(t, err) + }) + t.Run("symlinkRecursiveDirFileOpenDenied", func(t *testing.T) { + _, err := openFile(filepath.Join(symlinkToSymlinkDir, "file"), false) + require.Error(t, err) + }) + t.Run("circularSymlink", func(t *testing.T) { + _, err := openFile(circularSymlink, false) + require.Error(t, err) + }) + t.Run("brokenSymlink", func(t *testing.T) { + _, err := openFile(brokenSymlink, false) + require.Error(t, err) + }) +} + func TestLocks(t *testing.T) { t.Parallel() From 89ed19843f208d6d00b654101c813491868c8912 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Tue, 12 Sep 2023 08:37:41 -0600 Subject: [PATCH 06/16] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Krzysztof Skrzętnicki --- lib/utils/fs.go | 2 +- lib/utils/fs_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index ec725672743e4..94f3c294468fb 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -135,7 +135,7 @@ func openFile(path string, allowSymlink bool) (*os.File, error) { if err != nil { return nil, trace.ConvertSystemError(err) } else if fi.Mode().Type()&os.ModeSymlink != 0 { - return nil, trace.BadParameter("symlink not allowed in path: %v", subPath) + return nil, trace.BadParameter("failed to open file %v, symlink not allowed in path: %v", path, subPath) } } } diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index c23b389f441b6..b934de97679cb 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -35,9 +35,7 @@ func TestOpenFileSymlinks(t *testing.T) { // dir/file-s -> dir/file // circular-s -> circular-s // broken-s -> nonexistent - rootDir, err := os.MkdirTemp("", "symlink-test") - require.NoError(t, err) - defer os.RemoveAll(rootDir) + rootDir := t.TempDir() dirPath := filepath.Join(rootDir, "dir") err = os.Mkdir(dirPath, 0755) @@ -69,6 +67,8 @@ func TestOpenFileSymlinks(t *testing.T) { // Tests below can not be done with t.Parallel due to the need for directory cleanup in defer t.Run("directFileOpenAllowed", func(t *testing.T) { + filePath, err = filepath.EvalSymlinks(filePath) + require.NoError(t, err) f, err := openFile(filePath, false) require.NoError(t, err) defer f.Close() From 89b6309c6cb5067ba2633229a8e096e319c380df Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Tue, 12 Sep 2023 09:05:07 -0600 Subject: [PATCH 07/16] fs_test.go: Fix build for err reference --- lib/utils/fs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index b934de97679cb..c9c7ea173e145 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -38,7 +38,7 @@ func TestOpenFileSymlinks(t *testing.T) { rootDir := t.TempDir() dirPath := filepath.Join(rootDir, "dir") - err = os.Mkdir(dirPath, 0755) + err := os.Mkdir(dirPath, 0755) require.NoError(t, err) filePath := filepath.Join(dirPath, "file") From d1482dd979e5f688095c77cd655538ceddbfd199 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 13 Sep 2023 09:35:27 -0600 Subject: [PATCH 08/16] fs.go: Apply PR feedback and consider Hardlinks After PR discussion it was highlighted that MacOS does not guard against hardlinks in the same way linux does. For that reason this implementation has been updated with OS conditional logic to validate against hardlinks. --- lib/cgroup/cgroup.go | 2 +- lib/config/configuration.go | 2 +- lib/config/fileconf.go | 2 +- lib/srv/exec.go | 2 +- lib/tbot/config/config.go | 2 +- lib/utils/environment.go | 2 +- lib/utils/fs.go | 32 +++--- lib/utils/fs_test.go | 140 +++++++++++++++++++-------- lib/utils/kernel.go | 4 +- tool/tctl/common/edit_command.go | 4 +- tool/tctl/common/resource_command.go | 2 +- tool/tctl/sso/tester/command.go | 2 +- 12 files changed, 134 insertions(+), 62 deletions(-) diff --git a/lib/cgroup/cgroup.go b/lib/cgroup/cgroup.go index 4060af4eb256c..41967556039a9 100644 --- a/lib/cgroup/cgroup.go +++ b/lib/cgroup/cgroup.go @@ -172,7 +172,7 @@ func (s *Service) Place(sessionID string, pid int) error { // readPids returns a slice of PIDs from a file. Used to get list of all PIDs // within a cgroup. func readPids(path string) ([]string, error) { - f, err := utils.OpenFileNoSymlinks(path) + f, err := utils.OpenFileNoUnsafeLinks(path) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 9099d23b559f2..0cee854be270b 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -271,7 +271,7 @@ func ReadConfigFile(cliConfigPath string) (*FileConfig, error) { // ReadResources loads a set of resources from a file. func ReadResources(filePath string) ([]types.Resource, error) { - reader, err := utils.OpenFileAllowingSymlinks(filePath) + reader, err := utils.OpenFileAllowingUnsafeLinks(filePath) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/fileconf.go b/lib/config/fileconf.go index a83f111abe125..d9329687eb3d0 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -104,7 +104,7 @@ type FileConfig struct { // ReadFromFile reads Teleport configuration from a file. Currently only YAML // format is supported func ReadFromFile(filePath string) (*FileConfig, error) { - f, err := utils.OpenFileAllowingSymlinks(filePath) + f, err := utils.OpenFileAllowingUnsafeLinks(filePath) if err != nil { if errors.Is(err, fs.ErrPermission) { return nil, trace.Wrap(err, "failed to open file for Teleport configuration: %v. Ensure that you are running as a user with appropriate permissions.", filePath) diff --git a/lib/srv/exec.go b/lib/srv/exec.go index 3c66727c3bd95..f21af7be1e769 100644 --- a/lib/srv/exec.go +++ b/lib/srv/exec.go @@ -527,7 +527,7 @@ func getDefaultEnvPath(uid string, loginDefsPath string) string { envRootPath := defaultEnvRootPath // open file, if it doesn't exist return a default path and move on - f, err := utils.OpenFileAllowingSymlinks(loginDefsPath) + f, err := utils.OpenFileAllowingUnsafeLinks(loginDefsPath) if err != nil { if uid == "0" { log.Infof("Unable to open %q: %v: returning default su path: %q", loginDefsPath, err, defaultEnvRootPath) diff --git a/lib/tbot/config/config.go b/lib/tbot/config/config.go index 23bb435e69bbc..287d2d4b9466b 100644 --- a/lib/tbot/config/config.go +++ b/lib/tbot/config/config.go @@ -662,7 +662,7 @@ func FromCLIConf(cf *CLIConf) (*BotConfig, error) { // ReadConfigFromFile reads and parses a YAML config from a file. func ReadConfigFromFile(filePath string, manualMigration bool) (*BotConfig, error) { - f, err := utils.OpenFileAllowingSymlinks(filePath) + f, err := utils.OpenFileAllowingUnsafeLinks(filePath) if err != nil { return nil, trace.Wrap(err, fmt.Sprintf("failed to open file: %v", filePath)) } diff --git a/lib/utils/environment.go b/lib/utils/environment.go index aa7e9b4b33230..f05a31f2405ba 100644 --- a/lib/utils/environment.go +++ b/lib/utils/environment.go @@ -29,7 +29,7 @@ import ( func ReadEnvironmentFile(filename string) ([]string, error) { // open the users environment file. if we don't find a file, move on as // having this file for the user is optional. - file, err := OpenFileNoSymlinks(filename) + file, err := OpenFileNoUnsafeLinks(filename) if err != nil { log.Warnf("Unable to open environment file %v: %v, skipping", filename, err) return []string{}, nil diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 94f3c294468fb..8a0655d375953 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -23,7 +23,9 @@ import ( "io" "os" "path/filepath" + "runtime" "strings" + "syscall" "time" "github.com/gofrs/flock" @@ -99,20 +101,20 @@ func NormalizePath(path string, evaluateSymlinks bool) (string, error) { return s, nil } -// OpenFileAllowingSymlinks will open a file, if it's a symlink the file referenced by the link will be associated to -// the returned os.File. This will return an error if the file is not found or is a directory. -func OpenFileAllowingSymlinks(path string) (*os.File, error) { - return openFile(path, true) +// OpenFileAllowingUnsafeLinks opens a file, if the path includes a symlink, the returned os.File will be resolved to +// the actual file. This will return an error if the file is not found or is a directory. +func OpenFileAllowingUnsafeLinks(path string) (*os.File, error) { + return openFile(path, true, true) } -// OpenFileNoSymlinks will open a file, ensuring it's an actual file and not a directory or symlink. -func OpenFileNoSymlinks(path string) (*os.File, error) { - return openFile(path, false) +// OpenFileNoUnsafeLinks opens a file, ensuring it's an actual file and not a directory or symlink. Depending on +// the os, it may also prevent hardlinks. This is important because MacOS allows hardlinks without validating write +// permissions (similar to a symlink in that regard). +func OpenFileNoUnsafeLinks(path string) (*os.File, error) { + return openFile(path, false, runtime.GOOS != "darwin") } -// openFile opens a file and returns file handle. In general symlinks should not be allowed to reduce the risk of -// privilege escalation from Teleports elevated privileges to potentially less privileged users accidentally. -func openFile(path string, allowSymlink bool) (*os.File, error) { +func openFile(path string, allowSymlink, allowMultipleHardlinks bool) (*os.File, error) { newPath, err := NormalizePath(path, allowSymlink) if err != nil { return nil, trace.Wrap(err) @@ -135,12 +137,18 @@ func openFile(path string, allowSymlink bool) (*os.File, error) { if err != nil { return nil, trace.ConvertSystemError(err) } else if fi.Mode().Type()&os.ModeSymlink != 0 { - return nil, trace.BadParameter("failed to open file %v, symlink not allowed in path: %v", path, subPath) + return nil, trace.BadParameter("opening file %s, symlink not allowed in path: %s", path, subPath) } } } + if !allowMultipleHardlinks { + // hardlinks can only exist at the end file, not for directories within the path + if statT, ok := fi.Sys().(*syscall.Stat_t); ok && statT.Nlink > 1 { + return nil, trace.BadParameter("file has hardlink count greater than 1: %s", path) + } + } if fi.IsDir() { - return nil, trace.BadParameter("%v is not a file", path) + return nil, trace.BadParameter("%s is not a file", path) } f, err := os.Open(newPath) if err != nil { diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index c9c7ea173e145..8ca48a933738a 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -26,7 +26,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestOpenFileSymlinks(t *testing.T) { +func TestOpenFileLinks(t *testing.T) { // symlink structure setup, this will produce the following structure under the temp directory created below: // dir // dir-s -> dir @@ -35,18 +35,21 @@ func TestOpenFileSymlinks(t *testing.T) { // dir/file-s -> dir/file // circular-s -> circular-s // broken-s -> nonexistent + // hardfile + // hardfile-h -> hardfile rootDir := t.TempDir() dirPath := filepath.Join(rootDir, "dir") err := os.Mkdir(dirPath, 0755) require.NoError(t, err) - filePath := filepath.Join(dirPath, "file") - _, err = os.Create(filePath) + dirFilePath := filepath.Join(dirPath, "file") + f, err := os.Create(dirFilePath) require.NoError(t, err) + f.Close() - symlinkToFile := filepath.Join(dirPath, "file-s") - err = os.Symlink(filePath, symlinkToFile) + dirSymlinkToFile := filepath.Join(dirPath, "file-s") + err = os.Symlink(dirFilePath, dirSymlinkToFile) require.NoError(t, err) symlinkDir := filepath.Join(rootDir, "dir-s") @@ -65,39 +68,100 @@ func TestOpenFileSymlinks(t *testing.T) { err = os.Symlink(filepath.Join(rootDir, "nonexistent"), brokenSymlink) require.NoError(t, err) - // Tests below can not be done with t.Parallel due to the need for directory cleanup in defer - t.Run("directFileOpenAllowed", func(t *testing.T) { - filePath, err = filepath.EvalSymlinks(filePath) - require.NoError(t, err) - f, err := openFile(filePath, false) - require.NoError(t, err) - defer f.Close() - }) - t.Run("symlinkFileOpenAllowed", func(t *testing.T) { - f, err := openFile(symlinkToFile, true) - require.NoError(t, err) - defer f.Close() - }) - t.Run("symlinkFileOpenDenied", func(t *testing.T) { - _, err := openFile(symlinkToFile, false) - require.Error(t, err) - }) - t.Run("symlinkDirFileOpenDenied", func(t *testing.T) { - _, err := openFile(filepath.Join(symlinkDir, "file"), false) - require.Error(t, err) - }) - t.Run("symlinkRecursiveDirFileOpenDenied", func(t *testing.T) { - _, err := openFile(filepath.Join(symlinkToSymlinkDir, "file"), false) - require.Error(t, err) - }) - t.Run("circularSymlink", func(t *testing.T) { - _, err := openFile(circularSymlink, false) - require.Error(t, err) - }) - t.Run("brokenSymlink", func(t *testing.T) { - _, err := openFile(brokenSymlink, false) - require.Error(t, err) - }) + dirHardfilePath := filepath.Join(rootDir, "hardfile") + f, err = os.Create(dirHardfilePath) + require.NoError(t, err) + f.Close() + + dirHardLinkToHardfile := filepath.Join(rootDir, "hardfile-h") + err = os.Link(dirHardfilePath, dirHardLinkToHardfile) + require.NoError(t, err) + + // Define and run tests + type testCase struct { + name string + filePath string + allowSymln bool + allowHardln bool + expectErr string + } + testCases := []testCase{ + { + name: "directFileOpenAllowed", + filePath: dirFilePath, + allowSymln: false, + allowHardln: false, + expectErr: "", + }, + { + name: "symlinkFileOpenAllowed", + filePath: dirSymlinkToFile, + allowSymln: true, + allowHardln: false, + expectErr: "", + }, + { + name: "hardlinkFileOpenAllowed", + filePath: dirHardLinkToHardfile, + allowSymln: false, + allowHardln: true, + expectErr: "", + }, + { + name: "symlinkFileOpenDenied", + filePath: dirSymlinkToFile, + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + { + name: "symlinkDirFileOpenDenied", + filePath: filepath.Join(symlinkDir, "file"), + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + { + name: "symlinkRecursiveDirFileOpenDenied", + filePath: filepath.Join(symlinkToSymlinkDir, "file"), + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + { + name: "circularSymlink", + filePath: circularSymlink, + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + { + name: "brokenSymlink", + filePath: brokenSymlink, + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + { + name: "hardlinkFileOpenDenied", + filePath: dirHardLinkToHardfile, + allowSymln: false, + allowHardln: false, + expectErr: "hardlink", + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + f, err := openFile(tt.filePath, tt.allowSymln, tt.allowHardln) + if tt.expectErr == "" { + require.NoError(t, err) + f.Close() + } else { + require.ErrorContains(t, err, tt.expectErr) + } + }) + } } func TestLocks(t *testing.T) { diff --git a/lib/utils/kernel.go b/lib/utils/kernel.go index 1ca6f570a2f4d..f39841bc856be 100644 --- a/lib/utils/kernel.go +++ b/lib/utils/kernel.go @@ -36,7 +36,7 @@ func KernelVersion() (*semver.Version, error) { return nil, trace.BadParameter("requested kernel version on non-Linux host") } - file, err := OpenFileNoSymlinks("/proc/sys/kernel/osrelease") + file, err := OpenFileNoUnsafeLinks("/proc/sys/kernel/osrelease") if err != nil { return nil, trace.Wrap(err) } @@ -89,7 +89,7 @@ func HasBTF() error { return trace.BadParameter("requested kernel version on non-Linux host") } - file, err := OpenFileNoSymlinks(btfFile) + file, err := OpenFileNoUnsafeLinks(btfFile) if err == nil { file.Close() return nil diff --git a/tool/tctl/common/edit_command.go b/tool/tctl/common/edit_command.go index 87c3d375c83ec..e165a8ec7c6e2 100644 --- a/tool/tctl/common/edit_command.go +++ b/tool/tctl/common/edit_command.go @@ -153,7 +153,7 @@ func editor() string { } func checksum(filename string) (string, error) { - f, err := utils.OpenFileAllowingSymlinks(filename) + f, err := utils.OpenFileAllowingUnsafeLinks(filename) if err != nil { return "", trace.Wrap(err) } @@ -168,7 +168,7 @@ func checksum(filename string) (string, error) { } func resourceName(filename string) (string, error) { - f, err := utils.OpenFileAllowingSymlinks(filename) + f, err := utils.OpenFileAllowingUnsafeLinks(filename) if err != nil { return "", trace.Wrap(err) } diff --git a/tool/tctl/common/resource_command.go b/tool/tctl/common/resource_command.go index 801c2d51d0fff..4f6bb617d160d 100644 --- a/tool/tctl/common/resource_command.go +++ b/tool/tctl/common/resource_command.go @@ -270,7 +270,7 @@ func (rc *ResourceCommand) Create(ctx context.Context, client auth.ClientI) (err if rc.filename == "" { reader = os.Stdin } else { - f, err := utils.OpenFileAllowingSymlinks(rc.filename) + f, err := utils.OpenFileAllowingUnsafeLinks(rc.filename) if err != nil { return trace.Wrap(err) } diff --git a/tool/tctl/sso/tester/command.go b/tool/tctl/sso/tester/command.go index abe4689063358..4dd51c571bfe7 100644 --- a/tool/tctl/sso/tester/command.go +++ b/tool/tctl/sso/tester/command.go @@ -98,7 +98,7 @@ func (cmd *SSOTestCommand) getSupportedKinds() []string { func (cmd *SSOTestCommand) ssoTestCommand(ctx context.Context, c auth.ClientI) error { reader := os.Stdin if cmd.connectorFileName != "" { - f, err := utils.OpenFileAllowingSymlinks(cmd.connectorFileName) + f, err := utils.OpenFileAllowingUnsafeLinks(cmd.connectorFileName) if err != nil { return trace.Wrap(err, "could not open connector spec file %v", cmd.connectorFileName) } From 656b1d69a2fde85bfb6b2d9abca8ba1a51a94f86 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 13 Sep 2023 10:58:33 -0600 Subject: [PATCH 09/16] fs.go: Switch loop to range over components instead of index --- lib/utils/fs.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 8a0655d375953..22cc5e59a7981 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -126,13 +126,14 @@ func openFile(path string, allowSymlink, allowMultipleHardlinks bool) (*os.File, return nil, trace.ConvertSystemError(err) } } else { - isAbs := filepath.IsAbs(newPath) components := strings.Split(newPath, string(os.PathSeparator)) - for i := 1; i <= len(components); i++ { - subPath := filepath.Join(components[:i]...) - if isAbs { - subPath = string(os.PathSeparator) + subPath + var subPath string + for _, p := range components { + subPath = filepath.Join(subPath, p) + if subPath == "" { + subPath = string(os.PathSeparator) } + fi, err = os.Lstat(subPath) if err != nil { return nil, trace.ConvertSystemError(err) From d2dc0f9c4af482fe68c933eaede068f397003206 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 13 Sep 2023 11:11:20 -0600 Subject: [PATCH 10/16] Minor improvements from PR feedback --- lib/utils/fs.go | 4 ++-- lib/utils/fs_test.go | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 22cc5e59a7981..74b44a8285f98 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -104,14 +104,14 @@ func NormalizePath(path string, evaluateSymlinks bool) (string, error) { // OpenFileAllowingUnsafeLinks opens a file, if the path includes a symlink, the returned os.File will be resolved to // the actual file. This will return an error if the file is not found or is a directory. func OpenFileAllowingUnsafeLinks(path string) (*os.File, error) { - return openFile(path, true, true) + return openFile(path, true /* allowSymlink */, true /* allowMultipleHardlinks */) } // OpenFileNoUnsafeLinks opens a file, ensuring it's an actual file and not a directory or symlink. Depending on // the os, it may also prevent hardlinks. This is important because MacOS allows hardlinks without validating write // permissions (similar to a symlink in that regard). func OpenFileNoUnsafeLinks(path string) (*os.File, error) { - return openFile(path, false, runtime.GOOS != "darwin") + return openFile(path, false /* allowSymlink */, runtime.GOOS != "darwin" /* allowMultipleHardlinks */) } func openFile(path string, allowSymlink, allowMultipleHardlinks bool) (*os.File, error) { diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index 8ca48a933738a..c424dbfb6b6bb 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -154,9 +154,11 @@ func TestOpenFileLinks(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { f, err := openFile(tt.filePath, tt.allowSymln, tt.allowHardln) + if f != nil { + f.Close() + } if tt.expectErr == "" { require.NoError(t, err) - f.Close() } else { require.ErrorContains(t, err, tt.expectErr) } From acf220b63bf29d163fcb8f2f6d2d13039402e55e Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 13 Sep 2023 11:34:56 -0600 Subject: [PATCH 11/16] fs_test.go: Test public OpenFile API's and include OS specific validation --- lib/utils/fs_test.go | 103 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 12 deletions(-) diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index c424dbfb6b6bb..12359aa58adf4 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -20,6 +20,7 @@ import ( "context" "os" "path/filepath" + "runtime" "testing" "time" @@ -77,15 +78,15 @@ func TestOpenFileLinks(t *testing.T) { err = os.Link(dirHardfilePath, dirHardLinkToHardfile) require.NoError(t, err) - // Define and run tests - type testCase struct { + // Define and run tests against underline openFile function + type openFileTestCase struct { name string filePath string allowSymln bool allowHardln bool expectErr string } - testCases := []testCase{ + commonOpenFileTestCases := []openFileTestCase{ { name: "directFileOpenAllowed", filePath: dirFilePath, @@ -128,6 +129,15 @@ func TestOpenFileLinks(t *testing.T) { allowHardln: false, expectErr: "symlink", }, + { + name: "hardlinkFileOpenDenied", + filePath: dirHardLinkToHardfile, + allowSymln: false, + allowHardln: false, + expectErr: "hardlink", + }, + } + openFileTestCases := append(commonOpenFileTestCases, []openFileTestCase{ { name: "circularSymlink", filePath: circularSymlink, @@ -142,16 +152,9 @@ func TestOpenFileLinks(t *testing.T) { allowHardln: false, expectErr: "symlink", }, - { - name: "hardlinkFileOpenDenied", - filePath: dirHardLinkToHardfile, - allowSymln: false, - allowHardln: false, - expectErr: "hardlink", - }, - } + }...) - for _, tt := range testCases { + for _, tt := range openFileTestCases { t.Run(tt.name, func(t *testing.T) { f, err := openFile(tt.filePath, tt.allowSymln, tt.allowHardln) if f != nil { @@ -164,6 +167,82 @@ func TestOpenFileLinks(t *testing.T) { } }) } + + // Define and run tests against OS specific public functions + // OpenFileAllowingUnsafeLinks should always allow all the common test cases to pass without error + for _, tt := range commonOpenFileTestCases { + t.Run("unsafe-"+tt.name, func(t *testing.T) { + f, err := OpenFileAllowingUnsafeLinks(tt.filePath) + if f != nil { + f.Close() + } + require.NoError(t, err) + }) + } + // OpenFileNoUnsafeLinks has OS conditional logic that necessitates us to define the expected behavior + type safeOpenFileTestCase struct { + name string + filePath string + expectErr string + } + safeOpenFileTestCases := []safeOpenFileTestCase{ + { + name: "directFileOpenAllowed", + filePath: dirFilePath, + expectErr: "", + }, + { + name: "symlinkFileOpenDenied", + filePath: dirSymlinkToFile, + expectErr: "symlink", + }, + { + name: "symlinkDirFileOpenDenied", + filePath: filepath.Join(symlinkDir, "file"), + expectErr: "symlink", + }, + { + name: "symlinkRecursiveDirFileOpenDenied", + filePath: filepath.Join(symlinkToSymlinkDir, "file"), + expectErr: "symlink", + }, + { + name: "circularSymlink", + filePath: circularSymlink, + expectErr: "symlink", + }, + { + name: "brokenSymlink", + filePath: brokenSymlink, + expectErr: "symlink", + }, + } + if runtime.GOOS == "darwin" { + safeOpenFileTestCases = append(safeOpenFileTestCases, safeOpenFileTestCase{ + name: "hardlinkFileOpen", + filePath: dirHardLinkToHardfile, + expectErr: "hardlink", + }) + } else { + safeOpenFileTestCases = append(safeOpenFileTestCases, safeOpenFileTestCase{ + name: "hardlinkFileOpen", + filePath: dirHardLinkToHardfile, + expectErr: "", + }) + } + for _, tt := range safeOpenFileTestCases { + t.Run("safe-"+tt.name, func(t *testing.T) { + f, err := OpenFileNoUnsafeLinks(tt.filePath) + if f != nil { + f.Close() + } + if tt.expectErr == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tt.expectErr) + } + }) + } } func TestLocks(t *testing.T) { From 5f0398a874b3172f0bb019f12815be0a66f4946b Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Thu, 14 Sep 2023 09:28:24 -0600 Subject: [PATCH 12/16] Fix windows build Make hardlink count lookup code build conditional to avoid undefined syscall.Stat_t. --- lib/utils/fs.go | 3 +-- lib/utils/fs_unix.go | 13 +++++++++++++ lib/utils/fs_windows.go | 9 ++++++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 74b44a8285f98..0fcd0be3970af 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -25,7 +25,6 @@ import ( "path/filepath" "runtime" "strings" - "syscall" "time" "github.com/gofrs/flock" @@ -144,7 +143,7 @@ func openFile(path string, allowSymlink, allowMultipleHardlinks bool) (*os.File, } if !allowMultipleHardlinks { // hardlinks can only exist at the end file, not for directories within the path - if statT, ok := fi.Sys().(*syscall.Stat_t); ok && statT.Nlink > 1 { + if ok, linkCount := getHardLinkCount(fi); ok && linkCount > 1 { return nil, trace.BadParameter("file has hardlink count greater than 1: %s", path) } } diff --git a/lib/utils/fs_unix.go b/lib/utils/fs_unix.go index 4a91e84e7f194..ec6526303ca25 100644 --- a/lib/utils/fs_unix.go +++ b/lib/utils/fs_unix.go @@ -19,7 +19,20 @@ limitations under the License. package utils +import ( + "os" + "syscall" +) + // On non-windows we just lock the target file itself. func getPlatformLockFilePath(path string) string { return path } + +func getHardLinkCount(fi os.FileInfo) (bool, uint64) { + if statT, ok := fi.Sys().(*syscall.Stat_t); ok { + return true, statT.Nlink + } else { + return false, 0 + } +} diff --git a/lib/utils/fs_windows.go b/lib/utils/fs_windows.go index f758b47f6bbde..71b6dfccdc519 100644 --- a/lib/utils/fs_windows.go +++ b/lib/utils/fs_windows.go @@ -27,7 +27,10 @@ limitations under the License. // was causing flock.Flock.TryRLock to return either "access denied" or "The process cannot access // the file because it is being used by another process". -import "strings" +import ( + "os" + "strings" +) const lockPostfix = ".lock.tmp" @@ -39,3 +42,7 @@ func getPlatformLockFilePath(path string) string { } return path + lockPostfix } + +func getHardLinkCount(fi os.FileInfo) (bool, uint64) { + return false, 0 +} From adbb0c8a65bb47c8e8b65c96276bf7f007f784de Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Thu, 14 Sep 2023 09:59:54 -0600 Subject: [PATCH 13/16] utils.getHardLinkCount result order update from PR feedback --- lib/utils/fs.go | 2 +- lib/utils/fs_unix.go | 6 +++--- lib/utils/fs_windows.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 0fcd0be3970af..5e8de1ec1727f 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -143,7 +143,7 @@ func openFile(path string, allowSymlink, allowMultipleHardlinks bool) (*os.File, } if !allowMultipleHardlinks { // hardlinks can only exist at the end file, not for directories within the path - if ok, linkCount := getHardLinkCount(fi); ok && linkCount > 1 { + if linkCount, ok := getHardLinkCount(fi); ok && linkCount > 1 { return nil, trace.BadParameter("file has hardlink count greater than 1: %s", path) } } diff --git a/lib/utils/fs_unix.go b/lib/utils/fs_unix.go index ec6526303ca25..9fd69cc72d703 100644 --- a/lib/utils/fs_unix.go +++ b/lib/utils/fs_unix.go @@ -29,10 +29,10 @@ func getPlatformLockFilePath(path string) string { return path } -func getHardLinkCount(fi os.FileInfo) (bool, uint64) { +func getHardLinkCount(fi os.FileInfo) (uint64, bool) { if statT, ok := fi.Sys().(*syscall.Stat_t); ok { - return true, statT.Nlink + return statT.Nlink, true } else { - return false, 0 + return 0, false } } diff --git a/lib/utils/fs_windows.go b/lib/utils/fs_windows.go index 71b6dfccdc519..246d90da94dbf 100644 --- a/lib/utils/fs_windows.go +++ b/lib/utils/fs_windows.go @@ -43,6 +43,6 @@ func getPlatformLockFilePath(path string) string { return path + lockPostfix } -func getHardLinkCount(fi os.FileInfo) (bool, uint64) { - return false, 0 +func getHardLinkCount(fi os.FileInfo) (uint64, bool) { + return 0, false } From a7a6d69698d7d494315ae8103d10625bd3885151 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Thu, 14 Sep 2023 10:19:02 -0600 Subject: [PATCH 14/16] fs_windows.go: Comment improvements from PR feedback --- lib/utils/fs_windows.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/utils/fs_windows.go b/lib/utils/fs_windows.go index 246d90da94dbf..fcb0a6a1a9677 100644 --- a/lib/utils/fs_windows.go +++ b/lib/utils/fs_windows.go @@ -19,14 +19,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -// On Windows we use auxiliary .lock.tmp files to acquire locks, so we can still read/write target -// files themselves. -// -// .lock.tmp files are deliberately not cleaned up. Their presence doesn't matter to the actual -// locking. Repeatedly removing them on unlock when acquiring dozens of locks in a short timespan -// was causing flock.Flock.TryRLock to return either "access denied" or "The process cannot access -// the file because it is being used by another process". - import ( "os" "strings" @@ -34,6 +26,13 @@ import ( const lockPostfix = ".lock.tmp" +// On Windows we use auxiliary .lock.tmp files to acquire locks, so we can still read/write target +// files themselves. +// +// .lock.tmp files are deliberately not cleaned up. Their presence doesn't matter to the actual +// locking. Repeatedly removing them on unlock when acquiring dozens of locks in a short timespan +// was causing flock.Flock.TryRLock to return either "access denied" or "The process cannot access +// the file because it is being used by another process". func getPlatformLockFilePath(path string) string { // If target file is itself dedicated lockfile, we don't create another lockfile, since // we don't intend to read/write the target file itself. @@ -44,5 +43,6 @@ func getPlatformLockFilePath(path string) string { } func getHardLinkCount(fi os.FileInfo) (uint64, bool) { + // Although hardlinks on Windows are possible, Go does not currently expose the hardlinks associated to a file on windows return 0, false } From cc495e961790f5d00d5534865031f749b4b6073a Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Fri, 15 Sep 2023 08:10:11 -0600 Subject: [PATCH 15/16] fs: Fix build for OSX --- lib/utils/fs_unix.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/utils/fs_unix.go b/lib/utils/fs_unix.go index 9fd69cc72d703..4d94e20455034 100644 --- a/lib/utils/fs_unix.go +++ b/lib/utils/fs_unix.go @@ -31,7 +31,8 @@ func getPlatformLockFilePath(path string) string { func getHardLinkCount(fi os.FileInfo) (uint64, bool) { if statT, ok := fi.Sys().(*syscall.Stat_t); ok { - return statT.Nlink, true + // we must do a cast here because this will be uint16 on OSX + return uint64(statT.Nlink), true } else { return 0, false } From 8a2c11fe063d8d4d631464e68438fde2407b02bf Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Fri, 15 Sep 2023 08:24:26 -0600 Subject: [PATCH 16/16] Disable lint error due to unecessary cast on linux --- lib/utils/fs_unix.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/utils/fs_unix.go b/lib/utils/fs_unix.go index 4d94e20455034..b869d277ee0af 100644 --- a/lib/utils/fs_unix.go +++ b/lib/utils/fs_unix.go @@ -32,6 +32,7 @@ func getPlatformLockFilePath(path string) string { func getHardLinkCount(fi os.FileInfo) (uint64, bool) { if statT, ok := fi.Sys().(*syscall.Stat_t); ok { // we must do a cast here because this will be uint16 on OSX + //nolint:unconvert // the cast is only necessary for macOS return uint64(statT.Nlink), true } else { return 0, false