fix: harden thothctl file access

tools/thothctl/cmd/thothctl/main_test.go

@@ -3,8 +3,12 @@ package main
 import (
 	"bytes"
 	"context"
+	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -151,9 +155,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) {
 				t.Fatal(err)
 			}
 			linkDirectory := filepath.Join(fixture.root, "linked")
-			if err := os.Symlink(realDirectory, linkDirectory); err != nil {
-				t.Fatal(err)
-			}
+			symlinkOrSkip(t, realDirectory, linkDirectory)
 			return filepath.Join(linkDirectory, "secret")
 		},
 		"final symlink": func(t *testing.T, fixture cliFixture) string {
@@ -162,9 +164,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) {
 				t.Fatal(err)
 			}
 			linkSecret := filepath.Join(fixture.root, "linked-secret")
-			if err := os.Symlink(realSecret, linkSecret); err != nil {
-				t.Fatal(err)
-			}
+			symlinkOrSkip(t, realSecret, linkSecret)
 			return linkSecret
 		},
 	} {
@@ -185,6 +185,7 @@ func TestRunFailsClosedForTraversalAndParentSymlinkSecretSources(t *testing.T) {
 			if strings.Contains(stderr.String(), unsafeSource) {
 				t.Errorf("stderr revealed unsafe source path: %q", stderr.String())
 			}
+			assertDockerNotInvoked(t, fixture)
 		})
 	}
 }
@@ -199,6 +200,7 @@ func TestRunFailsClosedForOversizedEnvAndSecretFiles(t *testing.T) {
 		if exitCode != 2 || !strings.Contains(stderr.String(), "installation secret declarations could not be read") {
 			t.Errorf("exit=%d stderr=%q, want sanitized oversized-env failure", exitCode, stderr.String())
 		}
+		assertDockerNotInvoked(t, fixture)
 	})
 	t.Run("secret", func(t *testing.T) {
 		fixture := newCLIFixture(t, "")
@@ -213,6 +215,49 @@ func TestRunFailsClosedForOversizedEnvAndSecretFiles(t *testing.T) {
 		if exitCode != 2 || !strings.Contains(stderr.String(), "declared secret file could not be read") {
 			t.Errorf("exit=%d stderr=%q, want sanitized oversized-secret failure", exitCode, stderr.String())
 		}
+		assertDockerNotInvoked(t, fixture)
+	})
+}
+
+func TestRunFailsClosedForTooManyOrTooLargeSecretSources(t *testing.T) {
+	t.Run("too many sources", func(t *testing.T) {
+		fixture := newCLIFixture(t, "")
+		var declarations strings.Builder
+		for index := range 33 {
+			secretPath := filepath.Join(fixture.root, "secret-count-"+strconv.Itoa(index))
+			if err := os.WriteFile(secretPath, []byte("secret"), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			fmt.Fprintf(&declarations, "SECRET_%d_FILE=%s\n", index, secretPath)
+		}
+		fixture.setEnvContents(t, declarations.String())
+
+		var stdout, stderr bytes.Buffer
+		exitCode := run(context.Background(), []string{"--installation", fixture.installationPath, "logs"}, &stdout, &stderr)
+		if exitCode != 2 || !strings.Contains(stderr.String(), "installation secret declarations could not be read") {
+			t.Errorf("exit=%d stderr=%q, want sanitized source-count failure", exitCode, stderr.String())
+		}
+		assertDockerNotInvoked(t, fixture)
+	})
+
+	t.Run("total source bytes", func(t *testing.T) {
+		fixture := newCLIFixture(t, "")
+		var declarations strings.Builder
+		for index := range 5 {
+			secretPath := filepath.Join(fixture.root, "secret-total-"+strconv.Itoa(index))
+			if err := os.WriteFile(secretPath, bytes.Repeat([]byte("x"), 60*1024), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			fmt.Fprintf(&declarations, "SECRET_%d_SOURCE=%s\n", index, secretPath)
+		}
+		fixture.setEnvContents(t, declarations.String())
+
+		var stdout, stderr bytes.Buffer
+		exitCode := run(context.Background(), []string{"--installation", fixture.installationPath, "logs"}, &stdout, &stderr)
+		if exitCode != 2 || !strings.Contains(stderr.String(), "declared secret file could not be read") {
+			t.Errorf("exit=%d stderr=%q, want sanitized total-size failure", exitCode, stderr.String())
+		}
+		assertDockerNotInvoked(t, fixture)
 	})
 }
 
@@ -406,3 +451,20 @@ func (f cliFixture) invocations(t *testing.T) [][]string {
 	}
 	return invocations
 }
+
+func assertDockerNotInvoked(t *testing.T, fixture cliFixture) {
+	t.Helper()
+	if _, err := os.Stat(fixture.argsFile); !os.IsNotExist(err) {
+		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)
+	}
+}

tools/thothctl/go.mod

@@ -7,5 +7,5 @@ require gopkg.in/yaml.v3 v3.0.1
 require (
 	github.com/compose-spec/compose-go/v2 v2.14.0
 	github.com/sirupsen/logrus v1.9.0
-	golang.org/x/sys v0.5.0 // indirect
+	golang.org/x/sys v0.5.0
 )

tools/thothctl/internal/config/installation.go

@@ -9,6 +9,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"sync"
 
@@ -22,6 +23,8 @@ const installationFileName = "thothii-installation.yaml"
 
 const maxEnvironmentFileBytes = 1 << 20
 
+const maxSecretSources = 32
+
 var dotenvParseMu sync.Mutex
 
 type descriptor struct {
@@ -152,8 +155,12 @@ func (i Installation) SecretFiles() ([]string, error) {
 		if _, exists := seen[value]; !exists {
 			files = append(files, value)
 			seen[value] = struct{}{}
+			if len(files) > maxSecretSources {
+				return nil, errors.New("installation secret declarations could not be read")
+			}
 		}
 	}
+	sort.Strings(files)
 	return files, nil
 }
 

tools/thothctl/internal/output/sanitize.go

@@ -14,6 +14,10 @@ var credentialField = regexp.MustCompile(`(?im)(\b[\w.-]*(?:password|token|key)[
 
 const maxSecretFileBytes = 64 * 1024
 
+const maxSecretSourceFiles = 32
+
+const maxSecretSourceBytes = 256 * 1024
+
 // Sanitize redacts common credential fields and every supplied secret value.
 func Sanitize(text string, secretValues []string) string {
 	text = credentialField.ReplaceAllString(text, "${1}[REDACTED]")
@@ -29,13 +33,21 @@ func Sanitize(text string, secretValues []string) string {
 
 // SecretValuesFromFiles reads non-empty secret-file contents without exposing them to callers.
 func SecretValuesFromFiles(paths []string) ([]string, error) {
+	if len(paths) > maxSecretSourceFiles {
+		return nil, errors.New("declared secret file could not be read")
+	}
 	values := make([]string, 0, len(paths))
 	seen := make(map[string]struct{})
+	var totalBytes int64
 	for _, path := range paths {
-		value, err := readSecretFile(path)
+		value, size, err := readSecretFile(path)
 		if err != nil {
 			return nil, err
 		}
+		totalBytes += size
+		if totalBytes > maxSecretSourceBytes {
+			return nil, errors.New("declared secret file could not be read")
+		}
 		if value != "" {
 			if _, exists := seen[value]; exists {
 				continue
@@ -47,10 +59,10 @@ func SecretValuesFromFiles(paths []string) ([]string, error) {
 	return values, nil
 }
 
-func readSecretFile(path string) (string, error) {
+func readSecretFile(path string) (string, int64, error) {
 	contents, err := safeio.ReadCanonicalRegular(path, maxSecretFileBytes)
 	if err != nil {
-		return "", errors.New("declared secret file could not be read")
+		return "", 0, errors.New("declared secret file could not be read")
 	}
-	return strings.TrimRight(string(contents), "\r\n"), nil
+	return strings.TrimRight(string(contents), "\r\n"), int64(len(contents)), nil
 }

tools/thothctl/internal/safeio/files.go

@@ -19,21 +19,10 @@ func ValidateCanonicalPath(path string) error {
 	return nil
 }
 
-// ReadCanonicalRegular opens a canonical regular file after rejecting symlinked parents, then
-// bounds reads against the opened handle rather than a pre-open size check.
-func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) {
-	if err := ValidateCanonicalPath(path); err != nil {
-		return nil, err
-	}
-	if err := rejectSymlinkComponents(path); err != nil {
-		return nil, err
-	}
-
-	file, err := os.Open(path)
-	if err != nil {
+func readBoundedRegularFile(file *os.File, maximum int64) ([]byte, error) {
+	if maximum < 0 || maximum == int64(^uint64(0)>>1) {
 		return nil, ErrUnsafeFile
 	}
-	defer file.Close()
 	info, err := file.Stat()
 	if err != nil || !info.Mode().IsRegular() {
 		return nil, ErrUnsafeFile
@@ -44,20 +33,3 @@ func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) {
 	}
 	return contents, nil
 }
-
-func rejectSymlinkComponents(path string) error {
-	volume := filepath.VolumeName(path)
-	current := volume + string(filepath.Separator)
-	relative := strings.TrimPrefix(path, current)
-	for _, component := range strings.Split(relative, string(filepath.Separator)) {
-		if component == "" {
-			continue
-		}
-		current = filepath.Join(current, component)
-		info, err := os.Lstat(current)
-		if err != nil || info.Mode()&os.ModeSymlink != 0 {
-			return ErrUnsafeFile
-		}
-	}
-	return nil
-}

tools/thothctl/internal/safeio/files_test.go

@@ -0,0 +1,52 @@
+package safeio
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+)
+
+func TestReadCanonicalRegularRejectsFinalAndParentSymlinks(t *testing.T) {
+	temporaryRoot, err := filepath.EvalSymlinks(os.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	root, err := os.MkdirTemp(temporaryRoot, "thothctl-safeio-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(root) })
+
+	realDirectory := filepath.Join(root, "real")
+	if err := os.Mkdir(realDirectory, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	realFile := filepath.Join(realDirectory, "secret")
+	if err := os.WriteFile(realFile, []byte("secret"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	parentLink := filepath.Join(root, "parent-link")
+	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)
+	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)
+	}
+}

tools/thothctl/internal/safeio/files_unix.go

@@ -0,0 +1,57 @@
+//go:build !windows
+
+package safeio
+
+import (
+	"os"
+	"strings"
+
+	"golang.org/x/sys/unix"
+)
+
+// ReadCanonicalRegular opens an absolute canonical path component by component from the root
+// descriptor. O_NOFOLLOW rejects symlinks at every component, and the open directory descriptors
+// prevent later parent replacement from redirecting the final open.
+func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) {
+	if err := ValidateCanonicalPath(path); err != nil {
+		return nil, err
+	}
+	components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator))
+	if len(components) == 0 || components[0] == "" {
+		return nil, ErrUnsafeFile
+	}
+
+	directory, err := unix.Open(string(os.PathSeparator), unix.O_RDONLY|unix.O_CLOEXEC|unix.O_DIRECTORY, 0)
+	if err != nil {
+		return nil, ErrUnsafeFile
+	}
+	directories := []int{directory}
+	defer func() { closeUnixDescriptors(directories) }()
+
+	for _, component := range components[:len(components)-1] {
+		nextDirectory, err := unix.Openat(directory, component, unix.O_RDONLY|unix.O_CLOEXEC|unix.O_DIRECTORY|unix.O_NOFOLLOW, 0)
+		if err != nil {
+			return nil, ErrUnsafeFile
+		}
+		directory = nextDirectory
+		directories = append(directories, directory)
+	}
+
+	descriptor, err := unix.Openat(directory, components[len(components)-1], unix.O_RDONLY|unix.O_CLOEXEC|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, ErrUnsafeFile
+	}
+	file := os.NewFile(uintptr(descriptor), "thothctl-safeio")
+	if file == nil {
+		unix.Close(descriptor)
+		return nil, ErrUnsafeFile
+	}
+	defer file.Close()
+	return readBoundedRegularFile(file, maximum)
+}
+
+func closeUnixDescriptors(descriptors []int) {
+	for _, descriptor := range descriptors {
+		unix.Close(descriptor)
+	}
+}

tools/thothctl/internal/safeio/files_unix_test.go

@@ -0,0 +1,32 @@
+//go:build !windows
+
+package safeio
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/sys/unix"
+)
+
+func TestReadCanonicalRegularRejectsNamedPipeWithoutBlocking(t *testing.T) {
+	temporaryRoot, err := filepath.EvalSymlinks(os.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	root, err := os.MkdirTemp(temporaryRoot, "thothctl-safeio-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(root) })
+	pipe := filepath.Join(root, "secret-pipe")
+	if err := unix.Mkfifo(pipe, 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := ReadCanonicalRegular(pipe, 1024); !errors.Is(err, ErrUnsafeFile) {
+		t.Fatalf("named pipe error = %v, want ErrUnsafeFile", err)
+	}
+}

tools/thothctl/internal/safeio/files_windows.go

@@ -0,0 +1,92 @@
+//go:build windows
+
+package safeio
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+
+	"golang.org/x/sys/windows"
+)
+
+// 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.
+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] == "" {
+		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)
+	if err != nil {
+		return nil, ErrUnsafeFile
+	}
+	file := os.NewFile(uintptr(handle), "thothctl-safeio")
+	if file == nil {
+		windows.CloseHandle(handle)
+		return nil, ErrUnsafeFile
+	}
+	defer file.Close()
+	return readBoundedRegularFile(file, maximum)
+}
+
+func openWindowsComponent(path string, directory bool) (windows.Handle, error) {
+	flags := uint32(windows.FILE_FLAG_OPEN_REPARSE_POINT)
+	if directory {
+		flags |= windows.FILE_FLAG_BACKUP_SEMANTICS
+	} else {
+		flags |= windows.FILE_ATTRIBUTE_NORMAL
+	}
+	handle, err := windows.CreateFile(
+		windows.StringToUTF16Ptr(path),
+		windows.GENERIC_READ,
+		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
+		nil,
+		windows.OPEN_EXISTING,
+		flags,
+		0,
+	)
+	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) {
+		windows.CloseHandle(handle)
+		return 0, ErrUnsafeFile
+	}
+	return handle, nil
+}
+
+func closeWindowsHandles(handles []windows.Handle) {
+	for _, handle := range handles {
+		windows.CloseHandle(handle)
+	}
+}