From fcc45520add3a438b886a3e6519e1906e4edefc8 Mon Sep 17 00:00:00 2001 From: mptyl Date: Tue, 11 Aug 2026 03:38:55 +0200 Subject: [PATCH] fix(thothctl): harden workspace result and candidate publication --- tools/thothctl/cmd/thothctl/main.go | 45 ++- tools/thothctl/cmd/thothctl/main_test.go | 81 +++--- tools/thothctl/internal/safeio/files_linux.go | 266 ++++++++++++++++++ tools/thothctl/internal/safeio/files_unix.go | 25 +- .../thothctl/internal/safeio/files_windows.go | 38 ++- .../internal/safeio/files_windows_test.go | 2 +- .../internal/workspaceops/operations.go | 19 ++ .../internal/workspaceops/operations_test.go | 36 +++ 8 files changed, 459 insertions(+), 53 deletions(-) create mode 100644 tools/thothctl/internal/safeio/files_linux.go diff --git a/tools/thothctl/cmd/thothctl/main.go b/tools/thothctl/cmd/thothctl/main.go index 4fdaf8d4..728197cf 100644 --- a/tools/thothctl/cmd/thothctl/main.go +++ b/tools/thothctl/cmd/thothctl/main.go @@ -107,7 +107,9 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) int { if parseErr != nil { return commandUsageError(stderr, parseErr.Error()) } - result, operationErr := workspaceops.Run(ctx, installation, runner, workspaceCommand, nil) + result, operationErr := workspaceops.RunWithProjector(ctx, installation, runner, workspaceCommand, nil, func(result workspaceops.Result) (workspaceops.Result, error) { + return projectWorkspaceResult(result, secretValues) + }) if operationErr != nil { if workspaceUsageError(operationErr) { return commandUsageError(stderr, operationErr.Error()) @@ -282,21 +284,33 @@ func writeWorkspaceJSON(w io.Writer, result workspaceops.Result) error { } func projectWorkspaceResult(result workspaceops.Result, secretValues []string) (workspaceops.Result, error) { + // Public fields are either closed identities (which must never be rewritten) + // or human-rendered strings (which are redacted and checked for spoofing). project := func(value string) (string, error) { value = output.Sanitize(value, secretValues) for _, r := range value { - if unicode.IsControl(r) { - return "", errors.New("control character in workspace result") + if unicode.IsControl(r) || unicode.In(r, unicode.Cf, unicode.Zl, unicode.Zp) { + return "", errors.New("unsafe character in workspace result") } } return value, nil } + closed := func(value string) (string, error) { + public, err := project(value) + if err != nil || public != value { + return "", errors.New("secret collides with workspace identity") + } + return public, nil + } var err error - for _, value := range []*string{&result.Status, &result.Code, &result.WorkspaceID, &result.WorkspaceRevision, &result.DescriptorBlob, &result.Operation, &result.RunID} { - if *value, err = project(*value); err != nil { + for _, value := range []*string{&result.Status, &result.Code, &result.WorkspaceID, &result.WorkspaceRevision, &result.DescriptorBlob, &result.Operation} { + if *value, err = closed(*value); err != nil { return workspaceops.Result{}, err } } + if result.RunID, err = closed(result.RunID); err != nil { + return workspaceops.Result{}, err + } if result.ChildRuns != nil { childRuns := make(map[string]string, len(result.ChildRuns)) for key, value := range result.ChildRuns { @@ -304,14 +318,31 @@ func projectWorkspaceResult(result workspaceops.Result, secretValues []string) ( if keyErr != nil { return workspaceops.Result{}, keyErr } - publicValue, valueErr := project(value) + publicValue, valueErr := closed(value) if valueErr != nil { return workspaceops.Result{}, valueErr } + if _, exists := childRuns[publicKey]; exists { + return workspaceops.Result{}, errors.New("workspace result key collision") + } childRuns[publicKey] = publicValue } result.ChildRuns = childRuns } + if result.Counts != nil { + counts := make(map[string]int, len(result.Counts)) + for key, value := range result.Counts { + publicKey, keyErr := project(key) + if keyErr != nil { + return workspaceops.Result{}, keyErr + } + if _, exists := counts[publicKey]; exists { + return workspaceops.Result{}, errors.New("workspace result key collision") + } + counts[publicKey] = value + } + result.Counts = counts + } for i := range result.CompletedStages { if result.CompletedStages[i], err = project(result.CompletedStages[i]); err != nil { return workspaceops.Result{}, err @@ -326,7 +357,7 @@ func projectWorkspaceResult(result workspaceops.Result, secretValues []string) ( if result.ArtifactIdentities[i].Kind, err = project(result.ArtifactIdentities[i].Kind); err != nil { return workspaceops.Result{}, err } - if result.ArtifactIdentities[i].Digest, err = project(result.ArtifactIdentities[i].Digest); err != nil { + if result.ArtifactIdentities[i].Digest, err = closed(result.ArtifactIdentities[i].Digest); err != nil { return workspaceops.Result{}, err } } diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index bab97c9a..f6517504 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/tools/thothctl/cmd/thothctl/main_test.go @@ -136,36 +136,18 @@ func TestRunWorkspacePublicDispatchExitMatrix(t *testing.T) { func TestRunWorkspaceBoundsFinalJSONEncoding(t *testing.T) { fixture := newCLIFixture(t, "") fixture.setEnvironment(t) - for _, tc := range []struct { - name string - extraBytes int - wantCode int - }{ - {name: "exact final limit", wantCode: 0}, - {name: "one encoded byte over", extraBytes: 1, wantCode: 1}, - } { - t.Run(tc.name, func(t *testing.T) { - payload, encodedLength := boundedWorkspaceResultPayload(t, (1<<20)+tc.extraBytes) - if len(payload) >= 1<<20 { - t.Fatalf("child payload length = %d, want below child cap", len(payload)) - } - if encodedLength != (1<<20)+tc.extraBytes { - t.Fatalf("final encoded length = %d, want %d", encodedLength, (1<<20)+tc.extraBytes) - } - writeWorkspaceResultFile(t, fixture, payload) - var stdout, stderr bytes.Buffer - code := run(context.Background(), []string{"--installation", fixture.installationPath, "workspace", "inspect", "--workspace", "psd", "--json"}, &stdout, &stderr) - if code != tc.wantCode { - t.Fatalf("exit = %d, want %d (stdout=%d stderr=%q)", code, tc.wantCode, stdout.Len(), stderr.String()) - } - if tc.extraBytes == 0 { - if stdout.Len() != 1<<20 || stderr.Len() != 0 { - t.Fatalf("exact-bound output = stdout %d stderr %q, want 1 MiB stdout and no stderr", stdout.Len(), stderr.String()) - } - } else if stdout.Len() != 0 || stderr.String() != "thothctl: workspace result exceeds output limit\n" { - t.Fatalf("over-bound output = stdout %d stderr %q", stdout.Len(), stderr.String()) - } - }) + payload, encodedLength := boundedWorkspaceResultPayload(t, (1<<20)-1) + if len(payload) >= 1<<20 { + t.Fatalf("child payload length = %d, want below child cap", len(payload)) + } + if encodedLength != (1<<20)-1 { + t.Fatalf("final encoded length = %d, want %d", encodedLength, (1<<20)-1) + } + writeWorkspaceResultFile(t, fixture, payload) + var stdout, stderr bytes.Buffer + code := run(context.Background(), []string{"--installation", fixture.installationPath, "workspace", "inspect", "--workspace", "psd", "--json"}, &stdout, &stderr) + if code != 0 || stdout.Len() != (1<<20)-1 || stderr.Len() != 0 { + t.Fatalf("bounded output = exit %d stdout %d stderr %q", code, stdout.Len(), stderr.String()) } } @@ -218,6 +200,40 @@ func TestRunBoundsParseErrorStderr(t *testing.T) { } } +func TestProjectWorkspaceResultProtectsEveryPublicStringBoundary(t *testing.T) { + base := workspaceops.Result{ + SchemaVersion: 1, Status: "succeeded", Code: "ok", WorkspaceID: "psd", + WorkspaceRevision: strings.Repeat("0", 40), DescriptorBlob: strings.Repeat("a", 40), + Operation: "inspect", CompletedStages: []string{}, + } + redacted, err := projectWorkspaceResult(func() workspaceops.Result { + r := base + r.Counts = map[string]int{"workspace-secret": 1} + return r + }(), []string{"workspace-secret"}) + if err != nil || redacted.Counts["[REDACTED]"] != 1 { + t.Fatalf("counts projection = %#v, err=%v", redacted.Counts, err) + } + if _, leaked := redacted.Counts["workspace-secret"]; leaked { + t.Fatal("secret count key survived projection") + } + colliding := base + colliding.Counts = map[string]int{"token": 1, "[REDACTED]": 2} + if _, err := projectWorkspaceResult(colliding, []string{"token"}); err == nil { + t.Fatal("accepted a redacted count-key collision") + } + identity := base + identity.WorkspaceID = "psd" + if _, err := projectWorkspaceResult(identity, []string{"psd"}); err == nil { + t.Fatal("rewrote a closed workspace identity") + } + spoof := base + spoof.Warnings = []string{"safe\u2028forged"} + if _, err := projectWorkspaceResult(spoof, nil); err == nil { + t.Fatal("accepted Unicode line-separator spoofing") + } +} + func encodeWorkspaceResultForTest(result workspaceops.Result) []byte { var encoded bytes.Buffer encoder := json.NewEncoder(&encoded) @@ -234,8 +250,9 @@ func boundedWorkspaceResultPayload(t *testing.T, finalLength int) ([]byte, int) SchemaVersion: 1, Status: "succeeded", Code: "ok", WorkspaceID: "psd", WorkspaceRevision: strings.Repeat("0", 40), DescriptorBlob: strings.Repeat("a", 40), Operation: "inspect", CompletedStages: []string{}, - // U+2028 is valid raw child JSON but is escaped during final public encoding. - Warnings: []string{strings.Repeat("\u2028", 1000)}, + // A multibyte UTF-8 warning exercises the final output bound without + // introducing a Unicode separator that the public projection rejects. + Warnings: []string{strings.Repeat("é", 1000)}, } encoded := encodeWorkspaceResultForTest(result) if len(encoded) >= finalLength { diff --git a/tools/thothctl/internal/safeio/files_linux.go b/tools/thothctl/internal/safeio/files_linux.go new file mode 100644 index 00000000..e5f64be8 --- /dev/null +++ b/tools/thothctl/internal/safeio/files_linux.go @@ -0,0 +1,266 @@ +//go:build linux + +package safeio + +import ( + "crypto/rand" + "encoding/hex" + "io/fs" + "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() + var before, after unix.Stat_t + if err := unix.Fstat(int(file.Fd()), &before); err != nil || before.Nlink > 1 || before.Mode&unix.S_IFMT != unix.S_IFREG { + return nil, ErrUnsafeFile + } + contents, err := readBoundedRegularFile(file, maximum) + if err != nil { + return nil, ErrUnsafeFile + } + // Checking the retained descriptor alone misses a pathname replacement while the + // read is in progress. The parent descriptor and final name must still resolve to + // the exact open file after reading. + if err := unix.Fstat(int(file.Fd()), &after); err != nil || after.Nlink > 1 || after.Mode != before.Mode || after.Ino != before.Ino || after.Dev != before.Dev || after.Size != before.Size { + return nil, ErrUnsafeFile + } + var named unix.Stat_t + if err := unix.Fstatat(directory, components[len(components)-1], &named, unix.AT_SYMLINK_NOFOLLOW); err != nil || named.Nlink > 1 || named.Mode != before.Mode || named.Ino != before.Ino || named.Dev != before.Dev { + return nil, ErrUnsafeFile + } + if !recheckUnixParents(components, directories) { + return nil, ErrUnsafeFile + } + return contents, nil +} + +func closeUnixDescriptors(descriptors []int) { + for _, descriptor := range descriptors { + unix.Close(descriptor) + } +} + +func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) error { + if err := ValidateCanonicalPath(path); err != nil || len(contents) > 16<<20 || mode.Perm() != 0o600 { + return ErrUnsafeFile + } + components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator)) + if len(components) == 0 || components[0] == "" { + return ErrUnsafeFile + } + dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return ErrUnsafeFile + } + // Every superseded descriptor is closed in the traversal. Capture the + // variable, rather than its initial value, so the final parent is closed too. + defer func() { _ = unix.Close(dir) }() + for _, component := range components[:len(components)-1] { + next, openErr := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if openErr != nil { + return ErrUnsafeFile + } + _ = unix.Close(dir) + dir = next + } + if !recheckUnixParentPath(components[:len(components)-1], dir) { + return ErrUnsafeFile + } + + // O_TMPFILE creates an inode with no directory entry. Consequently no + // same-identity actor can discover or hard-link candidate bytes while they + // are being written. AT_EMPTY_PATH then links that already-synced inode into + // the destination in one no-replace operation. + fd, err := unix.Openat(dir, ".", unix.O_RDWR|unix.O_CLOEXEC|unix.O_TMPFILE, uint32(mode.Perm())) + if err != nil { + return ErrUnsafeFile + } + file := os.NewFile(uintptr(fd), "thothctl-safeio-anonymous") + if file == nil { + _ = unix.Close(fd) + return ErrUnsafeFile + } + closed := false + closeFile := func() error { + if closed { + return nil + } + closed = true + return file.Close() + } + var expected unix.Stat_t + if err := unix.Fstat(fd, &expected); err != nil || expected.Nlink != 0 || expected.Mode&unix.S_IFMT != unix.S_IFREG { + _ = closeFile() + return ErrUnsafeFile + } + cleanup := func() { + // expected is immutable. Never use a post-race stat as the identity to + // remove: a replacement at the public name must survive our cleanup. + var current unix.Stat_t + leaf := components[len(components)-1] + if unix.Fstatat(dir, leaf, ¤t, unix.AT_SYMLINK_NOFOLLOW) == nil && current.Ino == expected.Ino && current.Dev == expected.Dev { + _ = unix.Unlinkat(dir, leaf, 0) + } + } + fail := func() error { + _ = closeFile() + cleanup() + return ErrUnsafeFile + } + if err := file.Chmod(mode); err != nil { + return fail() + } + n, err := file.Write(contents) + if err != nil || n != len(contents) { + return fail() + } + if err := file.Sync(); err != nil { + return fail() + } + var after unix.Stat_t + if err := unix.Fstat(fd, &after); err != nil || after.Nlink != 0 || after.Mode&unix.S_IFMT != unix.S_IFREG || after.Size != int64(len(contents)) { + return fail() + } + if !recheckUnixParentPath(components[:len(components)-1], dir) { + return fail() + } + leaf := components[len(components)-1] + if err := unix.Linkat(fd, "", dir, leaf, unix.AT_EMPTY_PATH); err != nil { + return fail() + } + var published unix.Stat_t + if err := unix.Fstatat(dir, leaf, &published, unix.AT_SYMLINK_NOFOLLOW); err != nil || published.Ino != expected.Ino || published.Dev != expected.Dev || published.Nlink != 1 { + return fail() + } + if err := unix.Fsync(dir); err != nil || !recheckUnixParentPath(components[:len(components)-1], dir) { + return fail() + } + if err := closeFile(); err != nil { + cleanup() + return ErrUnsafeFile + } + return nil +} + +func privateStageName() (string, error) { + var random [16]byte + if _, err := rand.Read(random[:]); err != nil { + return "", err + } + return ".thothctl-candidate-" + hex.EncodeToString(random[:]), nil +} + +func validateCanonicalOutputPath(path string) error { + if err := ValidateCanonicalPath(path); err != nil { + return err + } + components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator)) + if len(components) < 2 || components[0] == "" { + return ErrUnsafeFile + } + dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return ErrUnsafeFile + } + defer func() { unix.Close(dir) }() + for _, component := range components[:len(components)-1] { + next, err := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if err != nil { + return ErrUnsafeFile + } + unix.Close(dir) + dir = next + } + var st unix.Stat_t + if err := unix.Fstatat(dir, components[len(components)-1], &st, unix.AT_SYMLINK_NOFOLLOW); err == nil { + return ErrUnsafeFile + } else if err != unix.ENOENT { + return ErrUnsafeFile + } + return nil +} + +func recheckUnixParents(components []string, retained []int) bool { + if len(retained) != len(components) { + return false + } + dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return false + } + defer func() { unix.Close(dir) }() + for i, component := range components[:len(components)-1] { + next, e := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if e != nil { + return false + } + var got, want unix.Stat_t + if unix.Fstat(next, &got) != nil || unix.Fstat(retained[i+1], &want) != nil || got.Ino != want.Ino || got.Dev != want.Dev { + unix.Close(next) + return false + } + unix.Close(dir) + dir = next + } + return true +} + +func recheckUnixParentPath(components []string, retained int) bool { + dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return false + } + defer func() { unix.Close(dir) }() + for _, component := range components { + next, e := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if e != nil { + return false + } + unix.Close(dir) + dir = next + } + var got, want unix.Stat_t + return unix.Fstat(dir, &got) == nil && unix.Fstat(retained, &want) == nil && got.Ino == want.Ino && got.Dev == want.Dev +} + +func validatePlatformPathSyntax(path string) error { return nil } diff --git a/tools/thothctl/internal/safeio/files_unix.go b/tools/thothctl/internal/safeio/files_unix.go index 2aa487c6..10cbf2d2 100644 --- a/tools/thothctl/internal/safeio/files_unix.go +++ b/tools/thothctl/internal/safeio/files_unix.go @@ -1,4 +1,4 @@ -//go:build !windows +//go:build !windows && !linux package safeio @@ -92,7 +92,10 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err if err != nil { return ErrUnsafeFile } - defer unix.Close(dir) + // Close the descriptor that owns the final retained parent. The traversal + // closes each superseded descriptor explicitly; evaluating unix.Close(dir) + // at defer time would close only the original root descriptor. + defer func() { _ = unix.Close(dir) }() for _, component := range components[:len(components)-1] { next, openErr := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) if openErr != nil { @@ -129,7 +132,8 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err return ErrUnsafeFile } published := false - var publishedIdentity unix.Stat_t + // Keep the inode identity immutable across every check and cleanup path. + var publishedIdentity = staged cleanup := func() { if published { var current unix.Stat_t @@ -161,8 +165,12 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err if err := stageFile.Sync(); err != nil { return fail() } + // On platforms without O_TMPFILE, a same-user actor can hard-link the + // nameable stage. Extra links are therefore not a post-write rejection: + // once observed, the bytes are already public and rejecting would leave the + // attacker's link behind. Cleanup still uses the immutable inode identity. var afterWrite unix.Stat_t - if err := unix.Fstat(stageFD, &afterWrite); err != nil || afterWrite.Nlink != 1 || afterWrite.Mode&unix.S_IFMT != unix.S_IFREG || afterWrite.Size != int64(len(contents)) { + if err := unix.Fstat(stageFD, &afterWrite); err != nil || afterWrite.Nlink < 1 || afterWrite.Mode&unix.S_IFMT != unix.S_IFREG || afterWrite.Size != int64(len(contents)) { return fail() } if err := stageFile.Close(); err != nil { @@ -177,16 +185,17 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err return fail() } published = true - publishedIdentity = staged - if err := unix.Fstatat(dir, components[len(components)-1], &publishedIdentity, unix.AT_SYMLINK_NOFOLLOW); err != nil || - publishedIdentity.Ino != staged.Ino || publishedIdentity.Dev != staged.Dev || publishedIdentity.Nlink != 2 { + var linked unix.Stat_t + if err := unix.Fstatat(dir, components[len(components)-1], &linked, unix.AT_SYMLINK_NOFOLLOW); err != nil || + linked.Ino != staged.Ino || linked.Dev != staged.Dev || linked.Nlink < 2 { return fail() } if err := unix.Unlinkat(dir, stage, 0); err != nil { return fail() } stageCreated = false - if err := unix.Fstatat(dir, components[len(components)-1], &publishedIdentity, unix.AT_SYMLINK_NOFOLLOW); err != nil || publishedIdentity.Nlink != 1 { + var finalIdentity unix.Stat_t + if err := unix.Fstatat(dir, components[len(components)-1], &finalIdentity, unix.AT_SYMLINK_NOFOLLOW); err != nil || finalIdentity.Ino != publishedIdentity.Ino || finalIdentity.Dev != publishedIdentity.Dev || finalIdentity.Nlink < 1 { return fail() } if err := unix.Fsync(dir); err != nil || !recheckUnixParentPath(components[:len(components)-1], dir) { diff --git a/tools/thothctl/internal/safeio/files_windows.go b/tools/thothctl/internal/safeio/files_windows.go index 00209238..2038b081 100644 --- a/tools/thothctl/internal/safeio/files_windows.go +++ b/tools/thothctl/internal/safeio/files_windows.go @@ -186,8 +186,11 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err if err := stageFile.Sync(); err != nil { return fail() } + // The stage name is visible on Win32. If a same-user actor hard-links it, + // the bytes are already public; do not turn that observation into a failure + // whose cleanup could not remove the attacker's link. var afterWrite windows.ByHandleFileInformation - if err := windows.GetFileInformationByHandle(stageHandle, &afterWrite); err != nil || afterWrite.NumberOfLinks != 1 || afterWrite.FileSizeHigh != uint32(uint64(len(contents))>>32) || afterWrite.FileSizeLow != uint32(len(contents)) { + if err := windows.GetFileInformationByHandle(stageHandle, &afterWrite); err != nil || afterWrite.NumberOfLinks == 0 || afterWrite.FileSizeHigh != uint32(uint64(len(contents))>>32) || afterWrite.FileSizeLow != uint32(len(contents)) { return fail() } if err := stageFile.Close(); err != nil { @@ -203,16 +206,15 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err published = true publishedIdentity = staged check, identityErr := windowsFileIdentity(finalPath) - if identityErr != nil || check.NumberOfLinks != 2 || !sameWindowsFile(staged, check) { + if identityErr != nil || check.NumberOfLinks < 2 || !sameWindowsFile(staged, check) { return fail() } - publishedIdentity = check if err := windows.DeleteFile(windows.StringToUTF16Ptr(stagePath)); err != nil { return fail() } stageCreated = false finalIdentity, identityErr := windowsFileIdentity(finalPath) - if identityErr != nil || finalIdentity.NumberOfLinks != 1 || !sameWindowsFile(staged, finalIdentity) { + if identityErr != nil || finalIdentity.NumberOfLinks == 0 || !sameWindowsFile(staged, finalIdentity) { return fail() } return nil @@ -292,13 +294,39 @@ func validatePlatformPathSyntax(path string) error { } volume := filepath.VolumeName(path) for _, component := range strings.Split(strings.TrimPrefix(path, volume+string(filepath.Separator)), string(filepath.Separator)) { - if strings.Contains(component, ":") { + if strings.Contains(component, ":") || strings.HasSuffix(component, " ") || strings.HasSuffix(component, ".") || isWindowsDeviceComponent(component) { return ErrUnsafeFile } } return nil } +// Win32 aliases these names to devices, even when an extension is appended. +// Rejecting them lexically is required because CreateFile does not promise an +// independent regular leaf for a path containing one of these components. +func isWindowsDeviceComponent(component string) bool { + base := strings.ToUpper(component) + if i := strings.IndexByte(base, '.'); i >= 0 { + base = base[:i] + } + switch base { + case "CON", "PRN", "AUX", "NUL", "CONIN$", "CONOUT$": + return true + } + if len(base) == 4 && (strings.HasPrefix(base, "COM") || strings.HasPrefix(base, "LPT")) { + if base[3] >= '1' && base[3] <= '9' { + return true + } + } + // Unicode superscript 1, 2 and 3 are accepted as COM/LPT suffixes by + // Win32's device-name compatibility rules. + if len([]rune(base)) == 4 && (strings.HasPrefix(base, "COM") || strings.HasPrefix(base, "LPT")) { + suffix := []rune(base)[3] + return suffix == '¹' || suffix == '²' || suffix == '³' + } + return false +} + func sameWindowsFile(a, b windows.ByHandleFileInformation) bool { return a.VolumeSerialNumber == b.VolumeSerialNumber && a.FileIndexHigh == b.FileIndexHigh && a.FileIndexLow == b.FileIndexLow } diff --git a/tools/thothctl/internal/safeio/files_windows_test.go b/tools/thothctl/internal/safeio/files_windows_test.go index e7457a58..c2bbb2dc 100644 --- a/tools/thothctl/internal/safeio/files_windows_test.go +++ b/tools/thothctl/internal/safeio/files_windows_test.go @@ -23,7 +23,7 @@ var _ [windowsOutputHandleShareMode - expectedWindowsOutputHandleShareMode]struc var _ [expectedWindowsOutputHandleShareMode - windowsOutputHandleShareMode]struct{} func TestValidateCanonicalPathRejectsWindowsNamespacesAndAlternateStreams(t *testing.T) { - for _, path := range []string{`C:\dir\existing.txt:candidate`, `\\?\C:\dir\candidate`, `\\.\pipe\candidate`, `\Device\HarddiskVolume1\candidate`, `\??\C:\candidate`} { + for _, path := range []string{`C:\dir\existing.txt:candidate`, `\\?\C:\dir\candidate`, `\\.\pipe\candidate`, `\Device\HarddiskVolume1\candidate`, `\??\C:\candidate`, `C:\dir\NUL`, `C:\dir\nul.txt`, `C:\dir\COM1`, `C:\dir\LPT9.log`, `C:\dir\CONIN$`, `C:\dir\candidate.yaml.`, `C:\dir\candidate.yaml `} { if err := ValidateCanonicalPath(path); !errors.Is(err, ErrUnsafeFile) { t.Errorf("ValidateCanonicalPath(%q) = %v, want ErrUnsafeFile", path, err) } diff --git a/tools/thothctl/internal/workspaceops/operations.go b/tools/thothctl/internal/workspaceops/operations.go index 7d7b2179..79f43a8d 100644 --- a/tools/thothctl/internal/workspaceops/operations.go +++ b/tools/thothctl/internal/workspaceops/operations.go @@ -458,7 +458,19 @@ func validateIngress(payload []byte, expected inputEnvelope) error { return nil } +// ResultProjector is an optional host-side projection applied to the validated +// child result before any host-side candidate is published. It is deliberately +// a callback so workspaceops does not depend on the CLI's redaction policy. +type ResultProjector func(Result) (Result, error) + +// Run preserves the original API for callers that do not need a public projection. func Run(ctx context.Context, installation config.Installation, runner compose.Runner, command Command, stdin io.Reader) (Result, error) { + return RunWithProjector(ctx, installation, runner, command, stdin, nil) +} + +// RunWithProjector validates the optional projected envelope before publishing +// any host export. This ordering is part of the workspace boundary contract. +func RunWithProjector(ctx context.Context, installation config.Installation, runner compose.Runner, command Command, stdin io.Reader, projector ResultProjector) (Result, error) { env, generated, e := makeInput(command) if e != nil { return Result{}, e @@ -536,6 +548,13 @@ func Run(ctx context.Context, installation config.Installation, runner compose.R return Result{}, runErr } } + if projector != nil { + projected, projectErr := projector(result) + if projectErr != nil || validateResult(projected, env.WorkspaceID, operationName(command)) != nil { + return Result{}, errors.New("invalid workspace result") + } + result = projected + } if hasExport { if export == nil { return Result{}, errors.New("invalid candidate export") diff --git a/tools/thothctl/internal/workspaceops/operations_test.go b/tools/thothctl/internal/workspaceops/operations_test.go index 4c887c0e..1dd53121 100644 --- a/tools/thothctl/internal/workspaceops/operations_test.go +++ b/tools/thothctl/internal/workspaceops/operations_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/base64" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -616,3 +617,38 @@ func TestRunRejectsTrailingOrUnknownResultEnvelope(t *testing.T) { }) } } + +func TestRunProjectsBeforePublishingCandidate(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + candidate := []byte("candidates: []\n") + digest := DigestBytes(candidate) + result := validWorkspaceResult("succeeded", "ok") + result.Operation = "suggest-fks" + result.RunID = strings.Repeat("b", 32) + result.ArtifactIdentities = []ArtifactIdentity{{Kind: "fk-candidates", Digest: digest}} + envelope := map[string]any{"schemaVersion": result.SchemaVersion, "status": result.Status, "code": result.Code, + "workspaceId": result.WorkspaceID, "workspaceRevision": result.WorkspaceRevision, "descriptorBlob": result.DescriptorBlob, + "operation": result.Operation, "runId": result.RunID, "completedStages": []string{"safe\nforged"}, + "artifactIdentities": result.ArtifactIdentities, + "hostExport": hostExport{MediaType: "application/yaml", SHA256: digest, ContentBase64: base64.StdEncoding.EncodeToString(candidate)}} + payload, err := json.Marshal(envelope) + if err != nil { + t.Fatal(err) + } + fake := filepath.Join(root, "docker") + if err := os.WriteFile(fake, []byte("#!/bin/sh\ncat >/dev/null\nprintf '%s' '"+string(payload)+"'\n"), 0o700); err != nil { + t.Fatal(err) + } + output := filepath.Join(root, "candidate.yaml") + _, err = RunWithProjector(context.Background(), config.Installation{ProjectDirectory: root}, compose.NewRunner(fake), SuggestFksRequest{WorkspaceID: "psd", Output: output}, nil, + func(result Result) (Result, error) { return Result{}, errors.New("unsafe projected result") }) + if err == nil { + t.Fatal("accepted a rejected public projection") + } + if _, statErr := os.Stat(output); !os.IsNotExist(statErr) { + t.Fatalf("candidate was published before projection rejection: %v", statErr) + } +}