From 7bad6072ce535439f6109c68950ed6cfeb901c70 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 95a54ff81c9d3..3af9bcf8fd92e 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -272,7 +272,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 7a0eb21c9d7eb..4255f57a04ff8 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -103,7 +103,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 ba538ad48ce33a5ec2a689d658d866a900db3ece 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 3af9bcf8fd92e..cc818c56a6d7b 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -272,7 +272,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 4255f57a04ff8..d322571b35693 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -103,7 +103,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 a9d28ce48efada202ea9551da7e188f8524b4f74 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 0eaf14e8cf08115fbefd3635c2e0cc24c39d333b 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 d6e90e84660b9cc222ce8b6becb8651ff0d350f4 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 b2ee1181cb5861465d43ab606db39765710da44a 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 3b1c010f9e35dd73466891c96e4557cc335fb60c 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 630b04e1e14affc2b80e2d5270c3101f70fdf937 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 cc818c56a6d7b..436fdf2eaf1be 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -272,7 +272,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 d322571b35693..a3f15c5e1c065 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -103,7 +103,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 6a99ae4307ab0aa746b06d008b3e24a8cfad0ef9 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 a6cadeec519ff5b713f265e1a723458c10778eab 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 2164c25ccd0ec1a13d20fbb47331119bf5771d08 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 124a4927089e617f1b69fec9c0fe19936a86dec0 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 59f671f77be1650f0fe720d1e216659ea3d6f896 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 8cf53394daa0f9682c8b258eae4321276ab4f5e3 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 9580e5657059d2e6b2bb87d636f5dd1b3cc53067 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 b10eebdcc22d3ea9178e010e278746e73931df23 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