diff --git a/internal/platform/ops.go b/internal/platform/ops.go index 1c8da8c..85bcb8f 100644 --- a/internal/platform/ops.go +++ b/internal/platform/ops.go @@ -2,6 +2,7 @@ package platform import ( "errors" + "io/fs" "os" "os/exec" "path/filepath" @@ -19,6 +20,13 @@ func startDetached(name string, args ...string) error { return nil } +// Expect is what the scan recorded for an item about to be trashed. Trash +// refuses an item whose owner or file type no longer matches. +type Expect struct { + UID uint32 + Mode fs.FileMode // only the type bits are compared +} + // checkAbs guards helper invocations: an absolute path can never be parsed // as an option by the helper program. func checkAbs(path string) error { diff --git a/internal/platform/ops_darwin.go b/internal/platform/ops_darwin.go index 6e4a481..b038a47 100644 --- a/internal/platform/ops_darwin.go +++ b/internal/platform/ops_darwin.go @@ -21,7 +21,7 @@ func Reveal(path string) error { // Trash moves path into ~/.Trash (same volume) or the volume's // .Trashes/ directory. It never copies and never deletes. -func Trash(path string) error { +func Trash(path string, want Expect) error { if err := checkAbs(path); err != nil { return err } @@ -34,8 +34,13 @@ func Trash(path string) error { dirs = append(dirs, filepath.Join(vol, ".Trashes", fmt.Sprint(os.Getuid()))) } base := filepath.Base(path) - for _, dir := range dirs { - if err := os.MkdirAll(dir, 0o700); err != nil { + for vi, dir := range dirs { + if err := os.MkdirAll(filepath.Dir(dir), 0o700); err != nil { + continue + } + // A volume trash is on a shared filesystem: another user could plant + // a symlink or an open directory there, so insist on a private one. + if err := ensurePrivateDir(dir, vi > 0); err != nil { continue } for i := 1; i < 10000; i++ { @@ -47,7 +52,7 @@ func Trash(path string) error { if _, err := os.Lstat(dst); err == nil { continue } - err := os.Rename(path, dst) + err := renameChecked(path, dst, want) if errors.Is(err, syscall.EXDEV) { break // try the next trash location } diff --git a/internal/platform/ops_linux.go b/internal/platform/ops_linux.go index 9f75f57..03eb026 100644 --- a/internal/platform/ops_linux.go +++ b/internal/platform/ops_linux.go @@ -29,7 +29,7 @@ func Reveal(path string) error { // Trash moves path to the freedesktop.org trash: the home trash when on the // same filesystem, otherwise $topdir/.Trash-$uid on the item's filesystem. // It never copies across filesystems and never deletes. -func Trash(path string) error { +func Trash(path string, want Expect) error { if err := checkAbs(path); err != nil { return err } @@ -99,7 +99,7 @@ func Trash(path string) error { os.Remove(ip) return errors.Join(werr, cerr) } - if err := os.Rename(path, dst); err != nil { + if err := renameChecked(path, dst, want); err != nil { os.Remove(ip) return err } @@ -108,31 +108,6 @@ func Trash(path string) error { return errors.New("trash: too many items with the same name") } -// ensurePrivateDir creates dir (mode 0700) if missing; an existing entry must -// be a real directory (not a symlink) owned by us. With strict (trash on a -// shared filesystem) it must also be inaccessible to others, as the -// freedesktop.org spec requires; home trash directories made by desktop -// environments are sometimes 0755, which is harmless there. -func ensurePrivateDir(dir string, strict bool) error { - if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { - return err - } - fi, err := os.Lstat(dir) - if err != nil { - return err - } - st := fi.Sys().(*syscall.Stat_t) - switch { - case !fi.IsDir(): - return errors.New("not a directory (symlink?)") - case int(st.Uid) != os.Getuid(): - return errors.New("owned by another user") - case strict && fi.Mode().Perm()&0o077 != 0: - return errors.New("accessible to other users") - } - return nil -} - // mountTop returns the top directory of the filesystem containing path. func mountTop(path string, dev uint64) string { cur := filepath.Dir(path) diff --git a/internal/platform/ops_linux_test.go b/internal/platform/ops_linux_test.go index 3f32da0..9980d49 100644 --- a/internal/platform/ops_linux_test.go +++ b/internal/platform/ops_linux_test.go @@ -16,7 +16,7 @@ func TestTrashFreedesktop(t *testing.T) { if err := os.WriteFile(victim, []byte("x"), 0o644); err != nil { t.Fatal(err) } - if err := Trash(victim); err != nil { + if err := Trash(victim, want(t, victim)); err != nil { t.Fatal(err) } if _, err := os.Lstat(victim); !os.IsNotExist(err) { @@ -32,13 +32,13 @@ func TestTrashFreedesktop(t *testing.T) { } // A second item with the same name gets a distinct trash name. os.WriteFile(victim, []byte("y"), 0o644) - if err := Trash(victim); err != nil { + if err := Trash(victim, want(t, victim)); err != nil { t.Fatal(err) } if _, err := os.Lstat(trashed + ".2"); err != nil { t.Fatal("collision not handled") } - if err := Trash("relative/path"); err == nil { + if err := Trash("relative/path", Expect{}); err == nil { t.Fatal("relative paths must be refused") } } @@ -62,3 +62,54 @@ func TestEnsurePrivateDirRejectsSymlink(t *testing.T) { t.Fatal(err) } } + +func want(t *testing.T, path string) Expect { + t.Helper() + m, err := Lstat(path) + if err != nil { + t.Fatal(err) + } + return Expect{UID: m.UID, Mode: m.Mode} +} + +// A parent swapped for a symlink after the scan must not redirect the move +// to whatever the symlink points at. +func TestTrashRefusesSwappedItem(t *testing.T) { + data := t.TempDir() + t.Setenv("XDG_DATA_HOME", data) + dir := filepath.Join(data, "work") + os.Mkdir(dir, 0o755) + victim := filepath.Join(dir, "big.iso") + os.WriteFile(victim, []byte("x"), 0o644) + scanned := want(t, victim) + + // Item replaced by a symlink to a file elsewhere: type differs. + other := filepath.Join(data, "precious") + os.WriteFile(other, []byte("keep"), 0o644) + os.Remove(victim) + os.Symlink(other, victim) + scannedDir := Expect{UID: scanned.UID, Mode: os.ModeDir} + if err := Trash(victim, scanned); err == nil { + t.Fatal("symlink accepted where a regular file was scanned") + } + if err := Trash(victim, scannedDir); err == nil { + t.Fatal("type mismatch accepted") + } + if _, err := os.Lstat(victim); err != nil { + t.Fatal("refused item was moved") + } + + // Owner differs from the scan. + os.Remove(victim) + os.WriteFile(victim, []byte("x"), 0o644) + if err := Trash(victim, Expect{UID: scanned.UID + 1, Mode: scanned.Mode}); err == nil { + t.Fatal("owner mismatch accepted") + } + entries, _ := os.ReadDir(filepath.Join(data, "Trash", "info")) + if len(entries) != 0 { + t.Fatalf("refused trash left %d .trashinfo files", len(entries)) + } + if err := Trash(victim, scanned); err != nil { + t.Fatal(err) + } +} diff --git a/internal/platform/ops_other.go b/internal/platform/ops_other.go index 9794878..ee3d9d5 100644 --- a/internal/platform/ops_other.go +++ b/internal/platform/ops_other.go @@ -11,4 +11,4 @@ const TrashName = "trash" func Reveal(string) error { return errors.New("not supported on this platform") } // Trash is unsupported on this platform. -func Trash(string) error { return errors.New("trash not supported on this platform") } +func Trash(string, Expect) error { return errors.New("trash not supported on this platform") } diff --git a/internal/platform/ops_windows.go b/internal/platform/ops_windows.go index 028677d..e1c7214 100644 --- a/internal/platform/ops_windows.go +++ b/internal/platform/ops_windows.go @@ -45,8 +45,9 @@ const ( ) // Trash moves path to the Recycle Bin via SHFileOperationW with -// FOF_ALLOWUNDO. It never deletes permanently. -func Trash(path string) error { +// FOF_ALLOWUNDO. It never deletes permanently. want is not checked here: +// Windows exposes no owner or inode identity through the scan. +func Trash(path string, _ Expect) error { if err := checkAbs(path); err != nil { return err } diff --git a/internal/platform/trash_unix.go b/internal/platform/trash_unix.go new file mode 100644 index 0000000..078e815 --- /dev/null +++ b/internal/platform/trash_unix.go @@ -0,0 +1,59 @@ +//go:build linux || darwin + +package platform + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "syscall" + + "golang.org/x/sys/unix" +) + +// renameChecked moves path to dst if it still matches want. The parent +// directory is opened once and both the check and the rename are relative to +// that descriptor, so a parent swapped for a symlink since the scan (or +// between check and rename) cannot redirect the move to another user's file. +func renameChecked(path, dst string, want Expect) error { + dir, base := filepath.Dir(path), filepath.Base(path) + dfd, err := unix.Open(dir, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return &os.PathError{Op: "open", Path: dir, Err: err} + } + defer unix.Close(dfd) + var st unix.Stat_t + if err := unix.Fstatat(dfd, base, &st, unix.AT_SYMLINK_NOFOLLOW); err != nil { + return &os.PathError{Op: "stat", Path: path, Err: err} + } + if st.Uid != want.UID || modeFromUnix(uint32(st.Mode))&fs.ModeType != want.Mode&fs.ModeType { + return errors.New("item changed since the scan (owner or type differs); rescan first") + } + return unix.Renameat(dfd, base, unix.AT_FDCWD, dst) +} + +// ensurePrivateDir creates dir (mode 0700) if missing; an existing entry must +// be a real directory (not a symlink) owned by us. With strict (trash on a +// shared filesystem) it must also be inaccessible to others, as the +// freedesktop.org spec requires; home trash directories made by desktop +// environments are sometimes 0755, which is harmless there. +func ensurePrivateDir(dir string, strict bool) error { + if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { + return err + } + fi, err := os.Lstat(dir) + if err != nil { + return err + } + st := fi.Sys().(*syscall.Stat_t) + switch { + case !fi.IsDir(): + return errors.New("not a directory (symlink?)") + case int(st.Uid) != os.Getuid(): + return errors.New("owned by another user") + case strict && fi.Mode().Perm()&0o077 != 0: + return errors.New("accessible to other users") + } + return nil +} diff --git a/internal/tui/actions.go b/internal/tui/actions.go index cf7af22..55fe3dc 100644 --- a/internal/tui/actions.go +++ b/internal/tui/actions.go @@ -3,6 +3,7 @@ package tui import ( "context" "fmt" + "io/fs" "time" tea "charm.land/bubbletea/v2" @@ -39,6 +40,7 @@ func (m *Model) requestTrash() tea.Cmd { } n := m.tree.Node(id) path := m.tree.Path(id) + want := platform.Expect{UID: n.UID, Mode: fs.FileMode(n.Mode)} what := "file" if n.IsDir() { what = fmt.Sprintf("directory with %s files", textutil.Count(int64(n.Files))) @@ -51,7 +53,7 @@ func (m *Model) requestTrash() tea.Cmd { yes: "move to " + platform.TrashName, onYes: func(m *Model) tea.Cmd { t := m.tree - return func() tea.Msg { return trashDoneMsg{t, id, platform.Trash(path)} } + return func() tea.Msg { return trashDoneMsg{t, id, platform.Trash(path, want)} } }, }) return nil