diff --git a/lib/cgroup/cgroup.go b/lib/cgroup/cgroup.go index 41f691222b849..41967556039a9 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.OpenFileNoUnsafeLinks(path) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/config/configuration.go b/lib/config/configuration.go index b09e2810b7f6f..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.OpenFile(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 a362fef25d6c2..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 := os.Open(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 d605e9520a725..f21af7be1e769 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.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 320019967f9f3..287d2d4b9466b 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.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 eaa98bb81d20a..f05a31f2405ba 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 := 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 a78c66323277c..5e8de1ec1727f 100644 --- a/lib/utils/fs.go +++ b/lib/utils/fs.go @@ -23,6 +23,8 @@ import ( "io" "os" "path/filepath" + "runtime" + "strings" "time" "github.com/gofrs/flock" @@ -84,30 +86,69 @@ 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 +} + +// 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 /* allowSymlink */, true /* allowMultipleHardlinks */) } -// OpenFile opens file and returns file handle -func OpenFile(path string) (*os.File, error) { - newPath, err := NormalizePath(path) +// 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 /* allowSymlink */, runtime.GOOS != "darwin" /* allowMultipleHardlinks */) +} + +func openFile(path string, allowSymlink, allowMultipleHardlinks bool) (*os.File, error) { + 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) + var fi os.FileInfo + if allowSymlink { + fi, err = os.Stat(newPath) + if err != nil { + return nil, trace.ConvertSystemError(err) + } + } else { + components := strings.Split(newPath, string(os.PathSeparator)) + 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) + } else if fi.Mode().Type()&os.ModeSymlink != 0 { + 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 linkCount, ok := getHardLinkCount(fi); ok && linkCount > 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 { @@ -118,7 +159,7 @@ func OpenFile(path string) (*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) } diff --git a/lib/utils/fs_test.go b/lib/utils/fs_test.go index b5fd2dce87594..12359aa58adf4 100644 --- a/lib/utils/fs_test.go +++ b/lib/utils/fs_test.go @@ -20,12 +20,231 @@ import ( "context" "os" "path/filepath" + "runtime" "testing" "time" "github.com/stretchr/testify/require" ) +func TestOpenFileLinks(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 + // hardfile + // hardfile-h -> hardfile + rootDir := t.TempDir() + + dirPath := filepath.Join(rootDir, "dir") + err := os.Mkdir(dirPath, 0755) + require.NoError(t, err) + + dirFilePath := filepath.Join(dirPath, "file") + f, err := os.Create(dirFilePath) + require.NoError(t, err) + f.Close() + + dirSymlinkToFile := filepath.Join(dirPath, "file-s") + err = os.Symlink(dirFilePath, dirSymlinkToFile) + 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) + + 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 against underline openFile function + type openFileTestCase struct { + name string + filePath string + allowSymln bool + allowHardln bool + expectErr string + } + commonOpenFileTestCases := []openFileTestCase{ + { + 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: "hardlinkFileOpenDenied", + filePath: dirHardLinkToHardfile, + allowSymln: false, + allowHardln: false, + expectErr: "hardlink", + }, + } + openFileTestCases := append(commonOpenFileTestCases, []openFileTestCase{ + { + name: "circularSymlink", + filePath: circularSymlink, + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + { + name: "brokenSymlink", + filePath: brokenSymlink, + allowSymln: false, + allowHardln: false, + expectErr: "symlink", + }, + }...) + + for _, tt := range openFileTestCases { + 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) + } else { + require.ErrorContains(t, err, tt.expectErr) + } + }) + } + + // 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) { t.Parallel() diff --git a/lib/utils/fs_unix.go b/lib/utils/fs_unix.go index 4a91e84e7f194..b869d277ee0af 100644 --- a/lib/utils/fs_unix.go +++ b/lib/utils/fs_unix.go @@ -19,7 +19,22 @@ 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) (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 + } +} diff --git a/lib/utils/fs_windows.go b/lib/utils/fs_windows.go index f758b47f6bbde..fcb0a6a1a9677 100644 --- a/lib/utils/fs_windows.go +++ b/lib/utils/fs_windows.go @@ -19,6 +19,13 @@ See the License for the specific language governing permissions and limitations under the License. */ +import ( + "os" + "strings" +) + +const lockPostfix = ".lock.tmp" + // On Windows we use auxiliary .lock.tmp files to acquire locks, so we can still read/write target // files themselves. // @@ -26,11 +33,6 @@ limitations under the License. // 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 "strings" - -const lockPostfix = ".lock.tmp" - 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. @@ -39,3 +41,8 @@ func getPlatformLockFilePath(path string) string { } return path + lockPostfix } + +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 +} diff --git a/lib/utils/kernel.go b/lib/utils/kernel.go index 57f64a03d1170..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 := os.Open("/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 := os.Open(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 3ff27cec8fc7e..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.OpenFile(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.OpenFile(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 0e5a380dd3379..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.OpenFile(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 a726e8f6328f4..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.OpenFile(cmd.connectorFileName) + f, err := utils.OpenFileAllowingUnsafeLinks(cmd.connectorFileName) if err != nil { return trace.Wrap(err, "could not open connector spec file %v", cmd.connectorFileName) }