diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index 41973010..0f3027b6 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/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) + } +} diff --git a/tools/thothctl/go.mod b/tools/thothctl/go.mod index 72c13da5..72316e51 100644 --- a/tools/thothctl/go.mod +++ b/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 ) diff --git a/tools/thothctl/internal/config/installation.go b/tools/thothctl/internal/config/installation.go index 8da5f6cb..7b296f6e 100644 --- a/tools/thothctl/internal/config/installation.go +++ b/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 } diff --git a/tools/thothctl/internal/output/sanitize.go b/tools/thothctl/internal/output/sanitize.go index 80c60fbf..6295715e 100644 --- a/tools/thothctl/internal/output/sanitize.go +++ b/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 } diff --git a/tools/thothctl/internal/safeio/files.go b/tools/thothctl/internal/safeio/files.go index 22cbc9c0..b83339cd 100644 --- a/tools/thothctl/internal/safeio/files.go +++ b/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 -} diff --git a/tools/thothctl/internal/safeio/files_test.go b/tools/thothctl/internal/safeio/files_test.go new file mode 100644 index 00000000..77c381f5 --- /dev/null +++ b/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) + } +} diff --git a/tools/thothctl/internal/safeio/files_unix.go b/tools/thothctl/internal/safeio/files_unix.go new file mode 100644 index 00000000..c3a745b9 --- /dev/null +++ b/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) + } +} diff --git a/tools/thothctl/internal/safeio/files_unix_test.go b/tools/thothctl/internal/safeio/files_unix_test.go new file mode 100644 index 00000000..30aebf53 --- /dev/null +++ b/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) + } +} diff --git a/tools/thothctl/internal/safeio/files_windows.go b/tools/thothctl/internal/safeio/files_windows.go new file mode 100644 index 00000000..ee5d065a --- /dev/null +++ b/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) + } +}