diff --git a/tools/tht/internal/safeio/claim_windows.go b/tools/tht/internal/safeio/claim_windows.go index e9f1be46..6a524f48 100644 --- a/tools/tht/internal/safeio/claim_windows.go +++ b/tools/tht/internal/safeio/claim_windows.go @@ -31,33 +31,25 @@ func (value *windowsPrivateRegular) Close() { func openWindowsPrivateRegular(path string, links uint32) (*windowsPrivateRegular, error) { parents, target, err := openCanonicalWindowsParent(path) - if err != nil || len(parents.handles) == 0 || validateOwnerOnlyDACL(parents.handles[len(parents.handles)-1]) != nil { + if err != nil || parents == nil || len(parents.handles) == 0 || validateOwnerOnlyDACL(parents.handles[len(parents.handles)-1]) != nil { if parents != nil { parents.Close() } return nil, ErrUnsafeFile } - fullPath := filepath.Join(parents.directory, target) - handle, err := windows.CreateFile( - windows.StringToUTF16Ptr(fullPath), - windows.GENERIC_READ, - windowsRetainedHandleShareMode, - nil, - windows.OPEN_EXISTING, - windows.FILE_FLAG_OPEN_REPARSE_POINT|windows.FILE_ATTRIBUTE_NORMAL, - 0, - ) + value, err := openWindowsPrivateRegularAt(parents.handles[len(parents.handles)-1], target, windows.GENERIC_READ, links) if err != nil { parents.Close() return nil, err } - var info windows.ByHandleFileInformation - if err := windows.GetFileInformationByHandle(handle, &info); err != nil || info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 || info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 || info.NumberOfLinks != links || validateOwnerOnlyDACL(handle) != nil { - _ = windows.CloseHandle(handle) - parents.Close() - return nil, ErrUnsafeFile - } - return &windowsPrivateRegular{parents: parents, handle: handle, path: fullPath, info: info}, nil + handle := value.handle + value.handle = 0 + return &windowsPrivateRegular{ + parents: parents, + handle: handle, + path: filepath.Join(parents.directory, target), + info: value.info, + }, nil } func claimCanonicalPrivateRegular(source, claim string) (bool, error) { diff --git a/tools/tht/internal/safeio/files.go b/tools/tht/internal/safeio/files.go index 36b9d11c..0ea06497 100644 --- a/tools/tht/internal/safeio/files.go +++ b/tools/tht/internal/safeio/files.go @@ -279,7 +279,7 @@ func WriteCanonicalNewPrivateFile(path string, contents []byte, mode os.FileMode // already private parent and returns a read/write handle for streamed contents. Callers must close // the returned handle and remove the file if their stream fails. func CreateCanonicalNewPrivateFile(path string) (*os.File, error) { - if err := validateCanonicalNewFile(path, true); err != nil { + if err := validateCanonicalNewFile(path); err != nil { return nil, err } file, err := createCanonicalNewPrivateParentReadWriteFile(path, 0o600) @@ -290,7 +290,7 @@ func CreateCanonicalNewPrivateFile(path string) (*os.File, error) { } func writeCanonicalNewFile(path string, contents []byte, mode os.FileMode, requirePrivateParent bool) error { - if err := validateCanonicalNewFile(path, requirePrivateParent); err != nil { + if err := validateCanonicalNewFile(path); err != nil { return err } var ( @@ -307,40 +307,28 @@ func writeCanonicalNewFile(path string, contents []byte, mode os.FileMode, requi } if _, err := file.Write(contents); err != nil { _ = file.Close() - _ = os.Remove(path) + _ = RemoveCanonicalPrivateRegular(path) return ErrUnsafeFile } if err := file.Sync(); err != nil { _ = file.Close() - _ = os.Remove(path) + _ = RemoveCanonicalPrivateRegular(path) return ErrUnsafeFile } if err := file.Close(); err != nil { - _ = os.Remove(path) + _ = RemoveCanonicalPrivateRegular(path) return ErrUnsafeFile } return nil } -func validateCanonicalNewFile(path string, requirePrivateParent bool) error { +// validateCanonicalNewFile performs only lexical validation. Platform creators retain and +// inspect the actual parent handle before mutation; pathname existence or parent checks here +// would be stale by the time an exclusive create reaches the filesystem. +func validateCanonicalNewFile(path string) error { if err := ValidateCanonicalPath(path); err != nil { return err } - parent := filepath.Dir(path) - if err := requireCanonicalDirectory(parent); err != nil { - return err - } - if requirePrivateParent && ValidatePrivateDirectory(parent) != nil { - return ErrUnsafeFile - } - if info, err := os.Lstat(path); err == nil { - if !info.Mode().IsRegular() || info.Mode()&os.ModeSymlink != 0 || info.Mode()&os.ModeType != 0 { - return ErrUnsafeFile - } - return ErrUnsafeFile - } else if !errors.Is(err, os.ErrNotExist) { - return ErrUnsafeFile - } return nil } @@ -406,18 +394,3 @@ func randomTemporaryName() (string, error) { } return ".tht-auth-" + hex.EncodeToString(bytes) + ".tmp", nil } - -func requireCanonicalDirectory(path string) error { - if err := ValidateCanonicalPath(path); err != nil { - return err - } - resolved, err := filepath.EvalSymlinks(path) - if err != nil || resolved != path { - return ErrUnsafeFile - } - info, err := os.Stat(path) - if err != nil || !info.IsDir() { - return ErrUnsafeFile - } - return nil -} diff --git a/tools/tht/internal/safeio/files_unix_test.go b/tools/tht/internal/safeio/files_unix_test.go index c644dbbd..657e46ed 100644 --- a/tools/tht/internal/safeio/files_unix_test.go +++ b/tools/tht/internal/safeio/files_unix_test.go @@ -159,3 +159,65 @@ func TestPrivateDirectoryCreationUsesThePinnedParentAfterAncestorSwap(t *testing }) } } + +func TestCreateCanonicalNewPrivateFileUsesPinnedParentAfterAncestorSwap(t *testing.T) { + temporaryRoot, err := filepath.EvalSymlinks(os.TempDir()) + if err != nil { + t.Fatal(err) + } + root, err := os.MkdirTemp(temporaryRoot, "tht-safeio-private-file-openat-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(root) }) + + parent := filepath.Join(root, "parent") + if err := os.Mkdir(parent, 0o700); err != nil { + t.Fatal(err) + } + if err := ProtectPrivateDirectory(parent); err != nil { + t.Fatal(err) + } + outside := filepath.Join(root, "outside") + if err := os.Mkdir(outside, 0o700); err != nil { + t.Fatal(err) + } + path := filepath.Join(parent, "archive.zip") + movedParent := parent + "-original" + swapped := false + restoreHook := SetPrivateDirectoryTestHookForTest(func(stage string) { + if stage != "after-canonical-private-file-parent-open" || swapped { + return + } + if err := os.Rename(parent, movedParent); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, parent); err != nil { + t.Fatal(err) + } + swapped = true + }) + t.Cleanup(restoreHook) + + file, err := CreateCanonicalNewPrivateFile(path) + if err != nil { + t.Fatal(err) + } + if _, err := file.Write([]byte("staged")); err != nil { + _ = file.Close() + t.Fatal(err) + } + if err := file.Close(); err != nil { + t.Fatal(err) + } + if !swapped { + t.Fatal("private file creator did not retain the parent before creation") + } + created := filepath.Join(movedParent, "archive.zip") + if err := ValidatePrivateRegular(created); err != nil { + t.Fatalf("pinned-parent private file validation = %v", err) + } + if _, err := os.Lstat(filepath.Join(outside, "archive.zip")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("outside target was mutated: %v", err) + } +} diff --git a/tools/tht/internal/safeio/files_windows.go b/tools/tht/internal/safeio/files_windows.go index 0040971d..9261d354 100644 --- a/tools/tht/internal/safeio/files_windows.go +++ b/tools/tht/internal/safeio/files_windows.go @@ -4,46 +4,28 @@ package safeio import ( "os" - "path/filepath" - "strings" "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. 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. +// ReadCanonicalRegular resolves every component under the already-opened parent with NT +// RootDirectory-relative opens. Each retained handle rejects reparse points and denies delete +// sharing, so a later ancestor replacement cannot redirect the final regular-file open. func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) { if err := ValidateCanonicalPath(path); err != nil { return nil, err } - volume := filepath.VolumeName(path) - root := volume + string(filepath.Separator) - components := strings.Split(strings.TrimPrefix(path, root), string(filepath.Separator)) - if volume == "" || len(components) == 0 || components[0] == "" { + parents, target, err := openCanonicalWindowsParent(path) + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return nil, ErrUnsafeFile } - - current := root - parents := make([]windows.Handle, 0, len(components)-1) - defer func() { closeWindowsHandles(parents) }() - for _, component := range components[:len(components)-1] { - current = filepath.Join(current, component) - handle, err := openWindowsComponent(current, true) - if err != nil { - return nil, ErrUnsafeFile - } - parents = append(parents, handle) - } - - current = filepath.Join(current, components[len(components)-1]) - handle, err := openWindowsComponent(current, false) + defer parents.Close() + handle, err := openWindowsRelativeComponent(parents.handles[len(parents.handles)-1], target, false, windows.GENERIC_READ) if err != nil { return nil, ErrUnsafeFile } @@ -93,6 +75,34 @@ func openWindowsComponentWithAccess(path string, directory bool, access uint32) return handle, nil } +// openWindowsRelativeComponent is the generic NT equivalent of openat for one canonical leaf. +// Its root handle has already pinned all lexical ancestors; FILE_OPEN_REPARSE_POINT makes a +// reparse point observable so it can be rejected rather than followed. +func openWindowsRelativeComponent(parent windows.Handle, name string, directory bool, access uint32) (windows.Handle, error) { + options := uint32(windows.FILE_SYNCHRONOUS_IO_NONALERT | windows.FILE_OPEN_REPARSE_POINT) + if directory { + options |= windows.FILE_DIRECTORY_FILE + } else { + options |= windows.FILE_NON_DIRECTORY_FILE + } + handle, err := openWindowsRelativeObject(parent, name, access, windows.FILE_OPEN, options, nil) + if err != nil { + return 0, err + } + var information windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(handle, &information); err != nil { + _ = windows.CloseHandle(handle) + return 0, err + } + if information.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 || + (directory && information.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY == 0) || + (!directory && (information.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 || information.NumberOfLinks != 1)) { + _ = windows.CloseHandle(handle) + return 0, ErrUnsafeFile + } + return handle, nil +} + func closeWindowsHandles(handles []windows.Handle) { for _, handle := range handles { windows.CloseHandle(handle) diff --git a/tools/tht/internal/safeio/preflight_windows.go b/tools/tht/internal/safeio/preflight_windows.go index e127721a..57a6989d 100644 --- a/tools/tht/internal/safeio/preflight_windows.go +++ b/tools/tht/internal/safeio/preflight_windows.go @@ -2,22 +2,20 @@ package safeio -import ( - "errors" - "path/filepath" - - "golang.org/x/sys/windows" -) +import "golang.org/x/sys/windows" func preflightPrivateDirectory(path string) (bool, error) { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return false, ErrUnsafeFile } defer parents.Close() - handle, err := openWindowsComponent(filepath.Join(parents.directory, target), true) + handle, err := openWindowsRelativeComponent(parents.handles[len(parents.handles)-1], target, true, windows.GENERIC_READ) if err != nil { - if errors.Is(err, windows.ERROR_FILE_NOT_FOUND) { + if isWindowsRelativeNotFound(err) { writableParent, accessErr := openWindowsComponentWithAccess( parents.directory, true, diff --git a/tools/tht/internal/safeio/private_root.go b/tools/tht/internal/safeio/private_root.go index c1e9459b..aac603f4 100644 --- a/tools/tht/internal/safeio/private_root.go +++ b/tools/tht/internal/safeio/private_root.go @@ -55,7 +55,7 @@ func SetPrivateDirectoryTestHookForTest(hook func(string)) func() { } } -// NotifyPrivateDirectoryTestHookForTest marks an internal retained-root boundary. It is called +// NotifyPrivateDirectoryTestHookForTest marks an internal retained-handle boundary. It is called // only by storage code and lets tests install deterministic directory replacement races. func NotifyPrivateDirectoryTestHookForTest(stage string) { privateDirectoryTestHook.RLock() diff --git a/tools/tht/internal/safeio/private_root_windows.go b/tools/tht/internal/safeio/private_root_windows.go index c86697e3..2d38e11a 100644 --- a/tools/tht/internal/safeio/private_root_windows.go +++ b/tools/tht/internal/safeio/private_root_windows.go @@ -395,6 +395,13 @@ func openWindowsPrivateRegularAtAllowedLinks( } func createWindowsPrivateRegularAt(parent windows.Handle, name string) (*windowsPrivateRegularAt, error) { + return createWindowsPrivateRegularAtWithAccess(parent, name, windows.FILE_GENERIC_READ|windows.FILE_GENERIC_WRITE|windows.DELETE) +} + +// createWindowsPrivateRegularAtWithAccess creates the final leaf beneath an already-retained +// parent with an owner-only DACL in the same NtCreateFile operation. The caller never re-resolves +// an absolute pathname after the parent is pinned. +func createWindowsPrivateRegularAtWithAccess(parent windows.Handle, name string, access uint32) (*windowsPrivateRegularAt, error) { security, err := newOwnerOnlySecurityDescriptor() if err != nil { return nil, ErrUnsafeFile @@ -403,7 +410,7 @@ func createWindowsPrivateRegularAt(parent windows.Handle, name string) (*windows handle, err := openWindowsRelativeObject( parent, name, - windows.FILE_GENERIC_READ|windows.FILE_GENERIC_WRITE|windows.DELETE, + access|windows.DELETE, windows.FILE_CREATE, windows.FILE_NON_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_REPARSE_POINT, security, diff --git a/tools/tht/internal/safeio/private_unix.go b/tools/tht/internal/safeio/private_unix.go index 322f1ce9..f7566c7f 100644 --- a/tools/tht/internal/safeio/private_unix.go +++ b/tools/tht/internal/safeio/private_unix.go @@ -5,7 +5,6 @@ package safeio import ( "errors" "os" - "path/filepath" "golang.org/x/sys/unix" ) @@ -56,22 +55,32 @@ func createPrivateDirectoryAt(parents *unixParentHandles) error { // ProtectPrivateDirectory sets the private directory mode used for local authentication state. func ProtectPrivateDirectory(path string) error { - if err := ValidateCanonicalPath(path); err != nil { - return err - } - if err := os.Chmod(path, 0o700); err != nil { + parents, descriptor, err := openCanonicalUnixPrivateDirectory(path) + if err != nil { return ErrUnsafeFile } - return ValidatePrivateDirectory(path) + defer parents.Close() + defer unix.Close(descriptor) + if unix.Fchmod(descriptor, 0o700) != nil { + return ErrUnsafeFile + } + var stat unix.Stat_t + if unix.Fstat(descriptor, &stat) != nil || !privateUnixDirectoryStat(&stat) { + return ErrUnsafeFile + } + return nil } // ValidatePrivateDirectory requires a canonical, non-symlinked directory with no group or world access. func ValidatePrivateDirectory(path string) error { - if err := requireCanonicalDirectory(path); err != nil { - return err + parents, descriptor, err := openCanonicalUnixPrivateDirectory(path) + if err != nil { + return ErrUnsafeFile } - info, err := os.Lstat(path) - if err != nil || !info.IsDir() || info.Mode()&os.ModeSymlink != 0 || !isExactPrivateMode(info.Mode(), 0o700) { + defer parents.Close() + defer unix.Close(descriptor) + var stat unix.Stat_t + if unix.Fstat(descriptor, &stat) != nil || !privateUnixDirectoryStat(&stat) { return ErrUnsafeFile } return nil @@ -79,61 +88,125 @@ func ValidatePrivateDirectory(path string) error { // ProtectPrivateRegular sets the private file mode used for local authentication files. func ProtectPrivateRegular(path string) error { - if err := ValidateCanonicalPath(path); err != nil { - return err - } - if err := os.Chmod(path, 0o600); err != nil { + parents, descriptor, err := openCanonicalUnixPrivateRegular(path) + if err != nil { return ErrUnsafeFile } - return ValidatePrivateRegular(path) -} - -func createCanonicalNewPrivateFile(path string, mode os.FileMode) (*os.File, error) { - return createCanonicalNewPrivateFileWithFlags(path, mode, os.O_WRONLY) -} - -func createCanonicalNewPrivateFileWithFlags(path string, mode os.FileMode, flags int) (*os.File, error) { - file, err := os.OpenFile(path, flags|os.O_CREATE|os.O_EXCL, mode) - if err != nil { - return nil, err + defer parents.Close() + defer unix.Close(descriptor) + if unix.Fchmod(descriptor, 0o600) != nil { + return ErrUnsafeFile } - if err := ProtectPrivateRegular(path); err != nil { - _ = file.Close() - _ = os.Remove(path) - return nil, ErrUnsafeFile - } - return file, nil -} - -func createCanonicalNewPrivateParentFile(path string, mode os.FileMode) (*os.File, error) { - if err := ValidatePrivateDirectory(filepath.Dir(path)); err != nil { - return nil, ErrUnsafeFile - } - return createCanonicalNewPrivateFile(path, mode) -} - -func createCanonicalNewPrivateParentReadWriteFile(path string, mode os.FileMode) (*os.File, error) { - if err := ValidatePrivateDirectory(filepath.Dir(path)); err != nil { - return nil, ErrUnsafeFile - } - return createCanonicalNewPrivateFileWithFlags(path, mode, os.O_RDWR) -} - -// ValidatePrivateRegular requires a canonical, single-link private regular file. -func ValidatePrivateRegular(path string) error { - if err := ValidateCanonicalPath(path); err != nil { - return err - } - if err := requireCanonicalDirectory(filepath.Dir(path)); err != nil { - return err - } - info, err := os.Lstat(path) - if err != nil || !info.Mode().IsRegular() || info.Mode()&os.ModeSymlink != 0 || !hasSingleLink(info) || !isExactPrivateMode(info.Mode(), 0o600) { + var stat unix.Stat_t + if unix.Fstat(descriptor, &stat) != nil || !privateUnixRegularStat(&stat) { return ErrUnsafeFile } return nil } -func isExactPrivateMode(mode os.FileMode, permissions os.FileMode) bool { - return mode.Perm() == permissions && mode&(os.ModeSetuid|os.ModeSetgid|os.ModeSticky) == 0 +func createCanonicalNewPrivateFile(path string, mode os.FileMode) (*os.File, error) { + return createCanonicalNewPrivateFileWithFlags(path, mode, os.O_WRONLY, false) +} + +func createCanonicalNewPrivateParentFile(path string, mode os.FileMode) (*os.File, error) { + return createCanonicalNewPrivateFileWithFlags(path, mode, os.O_WRONLY, true) +} + +func createCanonicalNewPrivateParentReadWriteFile(path string, mode os.FileMode) (*os.File, error) { + return createCanonicalNewPrivateFileWithFlags(path, mode, os.O_RDWR, true) +} + +// createCanonicalNewPrivateFileWithFlags resolves and validates every parent once, then creates +// the final name through that retained parent descriptor. No later lexical ancestor replacement +// can redirect the create, mode repair, metadata check, or failure cleanup. +func createCanonicalNewPrivateFileWithFlags(path string, mode os.FileMode, flags int, requirePrivateParent bool) (*os.File, error) { + parents, err := openCanonicalUnixParent(path) + if err != nil { + return nil, ErrUnsafeFile + } + defer parents.Close() + if requirePrivateParent { + var parentStat unix.Stat_t + if unix.Fstat(parents.parent, &parentStat) != nil || !privateUnixDirectoryStat(&parentStat) { + return nil, ErrUnsafeFile + } + } + NotifyPrivateDirectoryTestHookForTest("after-canonical-private-file-parent-open") + + // The final 0600 is applied with fchmod rather than trusting process umask or a path lookup. + // mode remains accepted for the existing helper contract; private files are always exactly 0600. + _ = mode + descriptor, err := unix.Openat(parents.parent, parents.target, + flags|unix.O_CREAT|unix.O_EXCL|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0o600) + if err != nil { + return nil, err + } + failed := true + defer func() { + if failed { + _ = unix.Close(descriptor) + _ = unix.Unlinkat(parents.parent, parents.target, 0) + } + }() + if unix.Fchmod(descriptor, 0o600) != nil { + return nil, ErrUnsafeFile + } + var stat unix.Stat_t + if unix.Fstat(descriptor, &stat) != nil || !privateUnixRegularStat(&stat) { + return nil, ErrUnsafeFile + } + file := os.NewFile(uintptr(descriptor), "tht-safeio-private") + if file == nil { + return nil, ErrUnsafeFile + } + failed = false + return file, nil +} + +// ValidatePrivateRegular requires a canonical, single-link private regular file. +func ValidatePrivateRegular(path string) error { + parents, descriptor, err := openCanonicalUnixPrivateRegular(path) + if err != nil { + return ErrUnsafeFile + } + defer parents.Close() + defer unix.Close(descriptor) + var stat unix.Stat_t + if unix.Fstat(descriptor, &stat) != nil || !privateUnixRegularStat(&stat) { + return ErrUnsafeFile + } + return nil +} + +func openCanonicalUnixPrivateDirectory(path string) (*unixParentHandles, int, error) { + parents, err := openCanonicalUnixParent(path) + if err != nil { + return nil, -1, ErrUnsafeFile + } + descriptor, err := unix.Openat(parents.parent, parents.target, + unix.O_RDONLY|unix.O_CLOEXEC|unix.O_DIRECTORY|unix.O_NOFOLLOW, 0) + if err != nil { + parents.Close() + return nil, -1, ErrUnsafeFile + } + return parents, descriptor, nil +} + +func openCanonicalUnixPrivateRegular(path string) (*unixParentHandles, int, error) { + parents, err := openCanonicalUnixParent(path) + if err != nil { + return nil, -1, ErrUnsafeFile + } + descriptor, err := unix.Openat(parents.parent, parents.target, + unix.O_RDONLY|unix.O_CLOEXEC|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) + if err != nil { + parents.Close() + return nil, -1, ErrUnsafeFile + } + return parents, descriptor, nil +} + +func privateUnixRegularStat(stat *unix.Stat_t) bool { + return stat != nil && stat.Mode&unix.S_IFMT == unix.S_IFREG && stat.Nlink == 1 && + stat.Uid == uint32(os.Geteuid()) && stat.Mode&0o7777 == 0o600 } diff --git a/tools/tht/internal/safeio/private_windows.go b/tools/tht/internal/safeio/private_windows.go index 18a30744..5376aeda 100644 --- a/tools/tht/internal/safeio/private_windows.go +++ b/tools/tht/internal/safeio/private_windows.go @@ -3,7 +3,6 @@ package safeio import ( - "errors" "os" "path/filepath" "runtime" @@ -15,38 +14,37 @@ import ( func createPrivateDirectory(path string) error { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return ErrUnsafeFile } defer parents.Close() - security, err := newOwnerOnlySecurityDescriptor() - if err != nil { - return ErrUnsafeFile - } - defer security.Close() - attributes := &windows.SecurityAttributes{ - Length: uint32(unsafe.Sizeof(windows.SecurityAttributes{})), - SecurityDescriptor: security.descriptor, - } - err = windows.CreateDirectory(windows.StringToUTF16Ptr(filepath.Join(parents.directory, target)), attributes) - runtime.KeepAlive(security) - if errors.Is(err, windows.ERROR_ALREADY_EXISTS) { + handle, err := createWindowsRelativePrivateDirectory(parents.handles[len(parents.handles)-1], target) + if isWindowsRelativeCollision(err) { return os.ErrExist } if err != nil { return ErrUnsafeFile } - return ValidatePrivateDirectory(path) + if err := windows.CloseHandle(handle); err != nil { + return ErrUnsafeFile + } + return nil } // ProtectPrivateDirectory sets a protected DACL containing only the current owner. func ProtectPrivateDirectory(path string) error { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return ErrUnsafeFile } defer parents.Close() - handle, err := openWindowsComponentWithAccess(filepath.Join(parents.directory, target), true, windows.GENERIC_READ|windows.WRITE_DAC|windows.WRITE_OWNER) + handle, err := openWindowsRelativeComponent(parents.handles[len(parents.handles)-1], target, true, windows.GENERIC_READ|windows.WRITE_DAC|windows.WRITE_OWNER) if err != nil { return ErrUnsafeFile } @@ -60,11 +58,14 @@ func ProtectPrivateDirectory(path string) error { // ValidatePrivateDirectory requires a canonical directory protected for its current owner only. func ValidatePrivateDirectory(path string) error { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return ErrUnsafeFile } defer parents.Close() - handle, err := openWindowsComponent(filepath.Join(parents.directory, target), true) + handle, err := openWindowsRelativeComponent(parents.handles[len(parents.handles)-1], target, true, windows.GENERIC_READ) if err != nil { return ErrUnsafeFile } @@ -78,11 +79,14 @@ func ValidatePrivateDirectory(path string) error { // ProtectPrivateRegular sets a protected DACL containing only the current owner. func ProtectPrivateRegular(path string) error { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return ErrUnsafeFile } defer parents.Close() - handle, err := openWindowsComponentWithAccess(filepath.Join(parents.directory, target), false, windows.GENERIC_READ|windows.WRITE_DAC|windows.WRITE_OWNER) + handle, err := openWindowsRelativeComponent(parents.handles[len(parents.handles)-1], target, false, windows.GENERIC_READ|windows.WRITE_DAC|windows.WRITE_OWNER) if err != nil { return ErrUnsafeFile } @@ -109,62 +113,45 @@ func createCanonicalNewPrivateParentReadWriteFile(path string, mode os.FileMode) func createCanonicalNewFile(path string, mode os.FileMode, requirePrivateParent bool, access uint32) (*os.File, error) { parents, target, err := openCanonicalWindowsParent(path) - if err != nil || len(parents.handles) == 0 || (requirePrivateParent && validateOwnerOnlyDACL(parents.handles[len(parents.handles)-1]) != nil) { + if err != nil || parents == nil || len(parents.handles) == 0 || (requirePrivateParent && validateOwnerOnlyDACL(parents.handles[len(parents.handles)-1]) != nil) { if parents != nil { parents.Close() } return nil, ErrUnsafeFile } defer parents.Close() - security, err := newOwnerOnlySecurityDescriptor() - if err != nil { - return nil, ErrUnsafeFile - } - defer security.Close() - attributes := &windows.SecurityAttributes{ - Length: uint32(unsafe.Sizeof(windows.SecurityAttributes{})), - SecurityDescriptor: security.descriptor, - } - handle, err := windows.CreateFile( - windows.StringToUTF16Ptr(filepath.Join(parents.directory, target)), - access, - windowsRetainedHandleShareMode, - attributes, - windows.CREATE_NEW, - windows.FILE_ATTRIBUTE_NORMAL, - 0, - ) - runtime.KeepAlive(security) + NotifyPrivateDirectoryTestHookForTest("after-canonical-private-file-parent-open") + // mode remains accepted for the existing helper contract; Windows installs the owner-only + // DACL in the NtCreateFile call below rather than relying on inherited file attributes. + _ = mode + value, err := createWindowsPrivateRegularAtWithAccess(parents.handles[len(parents.handles)-1], target, access) if err != nil { return nil, err } - if err := validateOwnerOnlyDACL(handle); err != nil { - _ = windows.CloseHandle(handle) - _ = os.Remove(path) - return nil, ErrUnsafeFile - } - file := os.NewFile(uintptr(handle), "tht-safeio-private") + file := os.NewFile(uintptr(value.handle), "tht-safeio-private") if file == nil { - _ = windows.CloseHandle(handle) - _ = os.Remove(path) + _ = closeAndDeleteWindowsPrivateRegular(value) return nil, ErrUnsafeFile } + value.handle = 0 return file, nil } // ValidatePrivateRegular requires a canonical, single-link file protected for its current owner only. func ValidatePrivateRegular(path string) error { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return ErrUnsafeFile } defer parents.Close() - handle, err := openWindowsComponent(filepath.Join(parents.directory, target), false) + value, err := openWindowsPrivateRegularAt(parents.handles[len(parents.handles)-1], target, windows.GENERIC_READ, 1) if err != nil { return ErrUnsafeFile } - defer windows.CloseHandle(handle) - if err := validateOwnerOnlyDACL(handle); err != nil { + if err := value.Close(); err != nil { return ErrUnsafeFile } return nil @@ -182,8 +169,8 @@ func (parents *windowsParentHandles) Close() { } // openCanonicalWindowsParent retains every directory handle from the volume root through the -// target parent without FILE_SHARE_DELETE. The resulting parent cannot be renamed or replaced by -// a reparse point while an operation uses its absolute child paths. +// target parent without FILE_SHARE_DELETE. Every component after the volume root is resolved +// through the prior retained handle's NT RootDirectory, never by re-opening an absolute prefix. func openCanonicalWindowsParent(path string) (*windowsParentHandles, string, error) { if err := ValidateCanonicalPath(path); err != nil { return nil, "", err @@ -201,12 +188,12 @@ func openCanonicalWindowsParent(path string) (*windowsParentHandles, string, err } parents.handles = append(parents.handles, rootHandle) for _, component := range components[:len(components)-1] { - parents.directory = filepath.Join(parents.directory, component) - handle, err := openWindowsComponent(parents.directory, true) + handle, err := openWindowsRelativeComponent(parents.handles[len(parents.handles)-1], component, true, windows.GENERIC_READ) if err != nil { parents.Close() return nil, "", err } + parents.directory = filepath.Join(parents.directory, component) parents.handles = append(parents.handles, handle) } return parents, components[len(components)-1], nil diff --git a/tools/tht/internal/safeio/private_windows_test.go b/tools/tht/internal/safeio/private_windows_test.go index 6d70b590..742c2bd6 100644 --- a/tools/tht/internal/safeio/private_windows_test.go +++ b/tools/tht/internal/safeio/private_windows_test.go @@ -216,6 +216,47 @@ func TestOpenCanonicalWindowsParentBlocksParentRename(t *testing.T) { } } +func TestCreateCanonicalNewPrivateFilePinsWindowsParentBeforeCreate(t *testing.T) { + root := t.TempDir() + parent := filepath.Join(root, "auth") + if err := os.Mkdir(parent, 0o700); err != nil { + t.Fatal(err) + } + if err := ProtectPrivateDirectory(parent); err != nil { + t.Fatal(err) + } + path := filepath.Join(parent, "archive.zip") + attemptedSwap := false + restoreHook := SetPrivateDirectoryTestHookForTest(func(stage string) { + if stage != "after-canonical-private-file-parent-open" || attemptedSwap { + return + } + attemptedSwap = true + if err := os.Rename(parent, parent+"-renamed"); err == nil { + t.Fatal("parent rename succeeded while private file creation retained its handle") + } + }) + t.Cleanup(restoreHook) + + file, err := CreateCanonicalNewPrivateFile(path) + if err != nil { + t.Fatal(err) + } + if _, err := file.Write([]byte("staged")); err != nil { + _ = file.Close() + t.Fatal(err) + } + if err := file.Close(); err != nil { + t.Fatal(err) + } + if !attemptedSwap { + t.Fatal("private file creator did not retain the parent before creation") + } + if err := ValidatePrivateRegular(path); err != nil { + t.Fatalf("ValidatePrivateRegular() = %v, want owner-private staged file", err) + } +} + func setPermissiveDACL(path string) error { world, err := windows.StringToSid("S-1-1-0") if err != nil { diff --git a/tools/tht/internal/safeio/replace_windows.go b/tools/tht/internal/safeio/replace_windows.go index 838ccca9..76345883 100644 --- a/tools/tht/internal/safeio/replace_windows.go +++ b/tools/tht/internal/safeio/replace_windows.go @@ -49,14 +49,18 @@ func replaceCanonicalRegular(path string, contents []byte) error { func removeCanonicalPrivateRegular(path string) error { parents, target, err := openCanonicalWindowsParent(path) - if err != nil { + if err != nil || parents == nil || len(parents.handles) == 0 { + if parents != nil { + parents.Close() + } return ErrUnsafeFile } defer parents.Close() - if err := ValidatePrivateRegular(path); err != nil { + value, err := openWindowsPrivateRegularAt(parents.handles[len(parents.handles)-1], target, windows.GENERIC_READ|windows.DELETE, 1) + if err != nil { return ErrUnsafeFile } - if err := windows.DeleteFile(windows.StringToUTF16Ptr(filepath.Join(parents.directory, target))); err != nil { + if err := closeAndDeleteWindowsPrivateRegular(value); err != nil { return ErrUnsafeFile } return nil