diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index 0f3027b6..c3c3a2fb 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/tools/thothctl/cmd/thothctl/main_test.go @@ -3,14 +3,14 @@ package main import ( "bytes" "context" - "errors" "fmt" "os" "path/filepath" - "runtime" "strconv" "strings" "testing" + + "github.com/aritmolab/thothii/tools/thothctl/internal/testsupport" ) func TestRunLogsRedactsAnUnlabelledDeclaredSecret(t *testing.T) { @@ -155,7 +155,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) { t.Fatal(err) } linkDirectory := filepath.Join(fixture.root, "linked") - symlinkOrSkip(t, realDirectory, linkDirectory) + testsupport.SymlinkOrSkip(t, realDirectory, linkDirectory) return filepath.Join(linkDirectory, "secret") }, "final symlink": func(t *testing.T, fixture cliFixture) string { @@ -164,7 +164,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) { t.Fatal(err) } linkSecret := filepath.Join(fixture.root, "linked-secret") - symlinkOrSkip(t, realSecret, linkSecret) + testsupport.SymlinkOrSkip(t, realSecret, linkSecret) return linkSecret }, } { @@ -458,13 +458,3 @@ func assertDockerNotInvoked(t *testing.T, fixture cliFixture) { t.Errorf("Docker was invoked: stat error = %v", err) } } - -func symlinkOrSkip(t *testing.T, target, link string) { - t.Helper() - if err := os.Symlink(target, link); err != nil { - if runtime.GOOS == "windows" && errors.Is(err, os.ErrPermission) { - t.Skip("Windows symlink privilege is unavailable") - } - t.Fatal(err) - } -} diff --git a/tools/thothctl/internal/safeio/files_test.go b/tools/thothctl/internal/safeio/files_test.go index 77c381f5..67d4d0fa 100644 --- a/tools/thothctl/internal/safeio/files_test.go +++ b/tools/thothctl/internal/safeio/files_test.go @@ -4,8 +4,9 @@ import ( "errors" "os" "path/filepath" - "runtime" "testing" + + "github.com/aritmolab/thothii/tools/thothctl/internal/testsupport" ) func TestReadCanonicalRegularRejectsFinalAndParentSymlinks(t *testing.T) { @@ -29,24 +30,14 @@ func TestReadCanonicalRegularRejectsFinalAndParentSymlinks(t *testing.T) { } parentLink := filepath.Join(root, "parent-link") - symlinkOrSkip(t, realDirectory, parentLink) + testsupport.SymlinkOrSkip(t, realDirectory, parentLink) if _, err := ReadCanonicalRegular(filepath.Join(parentLink, "secret"), 1024); !errors.Is(err, ErrUnsafeFile) { t.Fatalf("parent symlink error = %v, want ErrUnsafeFile", err) } finalLink := filepath.Join(root, "final-link") - symlinkOrSkip(t, realFile, finalLink) + testsupport.SymlinkOrSkip(t, realFile, finalLink) if _, err := ReadCanonicalRegular(finalLink, 1024); !errors.Is(err, ErrUnsafeFile) { t.Fatalf("final symlink error = %v, want ErrUnsafeFile", err) } } - -func symlinkOrSkip(t *testing.T, target, link string) { - t.Helper() - if err := os.Symlink(target, link); err != nil { - if runtime.GOOS == "windows" && errors.Is(err, os.ErrPermission) { - t.Skip("Windows symlink privilege is unavailable") - } - t.Fatal(err) - } -} diff --git a/tools/thothctl/internal/safeio/files_windows.go b/tools/thothctl/internal/safeio/files_windows.go index ee5d065a..ce40ae03 100644 --- a/tools/thothctl/internal/safeio/files_windows.go +++ b/tools/thothctl/internal/safeio/files_windows.go @@ -10,11 +10,15 @@ import ( "golang.org/x/sys/windows" ) +const windowsRetainedHandleShareMode uint32 = windows.FILE_SHARE_READ | windows.FILE_SHARE_WRITE + // ReadCanonicalRegular opens each component with FILE_FLAG_OPEN_REPARSE_POINT and rejects a -// reparse point on the opened handle before opening the next component. Windows' Win32 API does -// not expose a portable descriptor-relative equivalent of POSIX openat, so a hostile local actor -// with permission to rename a normal parent between these opens remains outside this guarantee. -// Installation directories therefore need trusted local filesystem/ACL ownership on Windows. +// reparse point on the opened handle before opening the next component. Retained handles allow +// ordinary read/write sharing but deny delete sharing, which blocks rename or deletion after a +// component is opened and throughout the final read. Windows' Win32 API does not expose a +// portable descriptor-relative equivalent of POSIX openat, so a hostile local actor can still +// replace a not-yet-opened normal component between absolute-path opens. Installation directories +// therefore need trusted local filesystem/ACL ownership on Windows. func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { if err := ValidateCanonicalPath(path); err != nil { return nil, err @@ -62,7 +66,7 @@ func openWindowsComponent(path string, directory bool) (windows.Handle, error) { handle, err := windows.CreateFile( windows.StringToUTF16Ptr(path), windows.GENERIC_READ, - windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + windowsRetainedHandleShareMode, nil, windows.OPEN_EXISTING, flags, diff --git a/tools/thothctl/internal/safeio/files_windows_test.go b/tools/thothctl/internal/safeio/files_windows_test.go new file mode 100644 index 00000000..745ea672 --- /dev/null +++ b/tools/thothctl/internal/safeio/files_windows_test.go @@ -0,0 +1,68 @@ +//go:build windows + +package safeio + +import ( + "os" + "path/filepath" + "testing" + + "golang.org/x/sys/windows" +) + +const expectedWindowsRetainedHandleShareMode = windows.FILE_SHARE_READ | windows.FILE_SHARE_WRITE + +// Keep this contract compile-enforced so Windows cross-test compilation catches a future +// FILE_SHARE_DELETE regression even when the tests are compiled on a non-Windows host. +var _ [windowsRetainedHandleShareMode - expectedWindowsRetainedHandleShareMode]struct{} +var _ [expectedWindowsRetainedHandleShareMode - windowsRetainedHandleShareMode]struct{} + +func TestOpenWindowsComponentBlocksMutationWhileHandleIsRetained(t *testing.T) { + t.Run("parent rename", func(t *testing.T) { + parent := filepath.Join(t.TempDir(), "parent") + if err := os.Mkdir(parent, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(parent, "secret"), []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + + handle, err := openWindowsComponent(parent, true) + if err != nil { + t.Fatal(err) + } + renamed := parent + "-renamed" + if err := os.Rename(parent, renamed); err == nil { + windows.CloseHandle(handle) + t.Fatal("parent rename succeeded while its safe-I/O handle was retained") + } + if err := windows.CloseHandle(handle); err != nil { + t.Fatal(err) + } + if err := os.Rename(parent, renamed); err != nil { + t.Fatalf("parent rename after closing its safe-I/O handle: %v", err) + } + }) + + t.Run("final delete", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "secret") + if err := os.WriteFile(path, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + + handle, err := openWindowsComponent(path, false) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(path); err == nil { + windows.CloseHandle(handle) + t.Fatal("final-file deletion succeeded while its safe-I/O handle was retained") + } + if err := windows.CloseHandle(handle); err != nil { + t.Fatal(err) + } + if err := os.Remove(path); err != nil { + t.Fatalf("final-file deletion after closing its safe-I/O handle: %v", err) + } + }) +} diff --git a/tools/thothctl/internal/testsupport/symlink.go b/tools/thothctl/internal/testsupport/symlink.go new file mode 100644 index 00000000..ad499ad1 --- /dev/null +++ b/tools/thothctl/internal/testsupport/symlink.go @@ -0,0 +1,19 @@ +// Package testsupport provides portable helpers shared by thothctl tests. +package testsupport + +import ( + "os" + "testing" +) + +// SymlinkOrSkip creates a symlink or skips only when Windows reports that symlink privilege is +// unavailable. All other failures remain test failures. +func SymlinkOrSkip(t testing.TB, target, link string) { + t.Helper() + if err := os.Symlink(target, link); err != nil { + if isSymlinkPrivilegeUnavailable(err) { + t.Skip("Windows symlink privilege is unavailable") + } + t.Fatal(err) + } +} diff --git a/tools/thothctl/internal/testsupport/symlink_nonwindows.go b/tools/thothctl/internal/testsupport/symlink_nonwindows.go new file mode 100644 index 00000000..e171b1d1 --- /dev/null +++ b/tools/thothctl/internal/testsupport/symlink_nonwindows.go @@ -0,0 +1,7 @@ +//go:build !windows + +package testsupport + +func isSymlinkPrivilegeUnavailable(_ error) bool { + return false +} diff --git a/tools/thothctl/internal/testsupport/symlink_test.go b/tools/thothctl/internal/testsupport/symlink_test.go new file mode 100644 index 00000000..b65c2738 --- /dev/null +++ b/tools/thothctl/internal/testsupport/symlink_test.go @@ -0,0 +1,12 @@ +package testsupport + +import ( + "errors" + "testing" +) + +func TestSymlinkPrivilegeUnavailableDoesNotMatchUnrelatedErrors(t *testing.T) { + if isSymlinkPrivilegeUnavailable(errors.New("unrelated symlink failure")) { + t.Fatal("unrelated symlink failure was classified as a missing Windows privilege") + } +} diff --git a/tools/thothctl/internal/testsupport/symlink_windows.go b/tools/thothctl/internal/testsupport/symlink_windows.go new file mode 100644 index 00000000..194709a5 --- /dev/null +++ b/tools/thothctl/internal/testsupport/symlink_windows.go @@ -0,0 +1,14 @@ +//go:build windows + +package testsupport + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +func isSymlinkPrivilegeUnavailable(err error) bool { + return errors.Is(err, os.ErrPermission) || errors.Is(err, windows.ERROR_PRIVILEGE_NOT_HELD) +} diff --git a/tools/thothctl/internal/testsupport/symlink_windows_test.go b/tools/thothctl/internal/testsupport/symlink_windows_test.go new file mode 100644 index 00000000..485653ce --- /dev/null +++ b/tools/thothctl/internal/testsupport/symlink_windows_test.go @@ -0,0 +1,38 @@ +//go:build windows + +package testsupport + +import ( + "os" + "testing" + + "golang.org/x/sys/windows" +) + +func TestSymlinkPrivilegeUnavailableRecognizesOnlyWindowsPrivilegeErrors(t *testing.T) { + for name, err := range map[string]error{ + "permission": os.ErrPermission, + "privilege not held": &os.LinkError{ + Op: "symlink", + Old: "target", + New: "link", + Err: windows.ERROR_PRIVILEGE_NOT_HELD, + }, + } { + t.Run(name, func(t *testing.T) { + if !isSymlinkPrivilegeUnavailable(err) { + t.Fatalf("isSymlinkPrivilegeUnavailable(%v) = false, want true", err) + } + }) + } + + unrelated := &os.LinkError{ + Op: "symlink", + Old: "target", + New: "link", + Err: windows.ERROR_FILENAME_EXCED_RANGE, + } + if isSymlinkPrivilegeUnavailable(unrelated) { + t.Fatal("unrelated Windows symlink failure was classified as a missing privilege") + } +}