From c7f7a6e1b07329b560aa38e408d48152e7c741de Mon Sep 17 00:00:00 2001 From: mptyl Date: Tue, 11 Aug 2026 04:05:40 +0200 Subject: [PATCH] fix Task1 publication and secret preflight --- docs/contracts/workspace-preprocessing-cli.md | 2 +- tools/thothctl/cmd/thothctl/main.go | 87 ++++++++--- tools/thothctl/cmd/thothctl/main_test.go | 27 ++-- tools/thothctl/internal/safeio/files_linux.go | 52 +++---- tools/thothctl/internal/safeio/files_unix.go | 91 +++++------- .../thothctl/internal/safeio/files_windows.go | 138 +++++++++--------- .../internal/safeio/files_windows_test.go | 47 +++++- .../internal/workspaceops/operations.go | 95 ++++++++++-- 8 files changed, 324 insertions(+), 215 deletions(-) diff --git a/docs/contracts/workspace-preprocessing-cli.md b/docs/contracts/workspace-preprocessing-cli.md index 01eea76a..c710df66 100644 --- a/docs/contracts/workspace-preprocessing-cli.md +++ b/docs/contracts/workspace-preprocessing-cli.md @@ -24,6 +24,6 @@ The bounded schema-v1 stdin envelope is exact: omitted fields are not equivalent * `check`: `resume` is required; `annotations` and `reviewedCandidates` are either both present or both absent. `annotations` is exactly `{basename, contentBase64, sha256}`. * `index-schema`: no additional fields. -No other fields, duplicate JSON value, host path, raw SQL, or raw annotation content are accepted. SQL and annotations use a logical basename, base64 bytes, and a declared `sha256:` digest. The request envelope is independently capped at 24 MiB (after JSON/base64 encoding), enough to carry the frozen 1 MiB-per-file/16 MiB aggregate raw ingress bounds; every supplied field/value must exactly match the command-derived envelope. Candidate responses may carry the internal `hostExport` (`mediaType`, `sha256`, `contentBase64`) only for `suggest-fks`; its object is strict (unknown fields rejected) and is always verified, even without `--output`: YAML media type (`application/yaml` or `text/yaml`), UTF-8, digest, and decoded size at most 700 KiB. It is removed from the public result and written exclusively only after result/run/identity validation. Output publication uses restrictive mode `0600` and refuses existing leaves, links, hardlinks, directories, replacement races, and reparse points. +No other fields, duplicate JSON value, host path, raw SQL, or raw annotation content are accepted. SQL and annotations use a logical basename, base64 bytes, and a declared `sha256:` digest. The request envelope is independently capped at 24 MiB (after JSON/base64 encoding), enough to carry the frozen 1 MiB-per-file/16 MiB aggregate raw ingress bounds; every supplied field/value must exactly match the command-derived envelope. Candidate responses may carry the internal `hostExport` (`mediaType`, `sha256`, `contentBase64`) only for `suggest-fks`; its object is strict (unknown fields rejected) and is always verified, even without `--output`: YAML media type (`application/yaml` or `text/yaml`), UTF-8, digest, and decoded size at most 700 KiB. It is removed from the public result and written exclusively only after result/run/identity validation. Output publication uses restrictive mode `0600` and refuses existing leaves, links, hardlinks, directories, replacement races, and reparse points. On Linux, publication uses an anonymous `O_TMPFILE` inode and `linkat(..., AT_EMPTY_PATH)`; the link operation is the final commit, and no post-commit check can turn success into a not-published error. On Darwin, the named-stage implementation is a trusted-parent mode: the target parent and ancestors must remain namespace-stable and same-UID stage mutation is explicitly outside the threat model. It verifies the stage identity/link count before using `renameatx_np(..., RENAME_EXCL)` as the final no-replace commit. It does not claim protection against a same-UID hostile hard-linker. On Windows, the stage is held open with `DELETE|WRITE` and zero sharing, then renamed atomically with `SetFileInformationByHandle(FileRenameInfo)` rooted at the retained parent handle; replacement is disabled and the rename is final. The public result has schema version 1 and only these fields: `status`, `code`, workspace/revision/descriptor/operation identities, optional run and child run IDs, completed stages, counts, artifact identities, and warnings. Revisions/descriptors are 40-hex; run IDs are 32-hex; artifact digests are `sha256:`. Allowed statuses are `succeeded`, `unchanged`, `dry_run`, `blocked`, and `failed`. Allowed codes are `ok`, `workspace_not_found`, `workspace_not_activatable`, `binding_missing`, `preprocessing_conflict`, `preprocessing_resume_mismatch`, `manual_review_required`, `evidence_materialization_required`, `effective_config_mismatch`, `semantic_index_incompatible`, `annotation_invalid`, `egress_policy_refused`, and `registry_bootstrap_recovery_conflict`. `succeeded`, `unchanged`, and `dry_run` require `ok` and child exit 0. `blocked` requires one of `manual_review_required`, `evidence_materialization_required`, `preprocessing_conflict`, `preprocessing_resume_mismatch`, or `registry_bootstrap_recovery_conflict`, and child exit 3. `failed` requires a non-`ok` operational code other than those blocked-only codes, and child exit 1. A nonzero child exit is never accepted for another status/code combination. The public `thothctl` exit mapping is fixed independently of child details: `0` for succeeded/unchanged/dry-run, `3` for an expected blocked result, `2` for command grammar or unsafe host-file failures, and `1` for operational failures (including invalid child envelopes, output-limit failures, and child execution failures). Stdout is capped at 1 MiB after final human/JSON encoding and stderr at 64 KiB after sanitization; output is never allowed to exceed those bounds. In human mode, a `registry_bootstrap_recovery_conflict` result prints exactly `Bootstrap recovery is ambiguous or corrupt; inspect the installation registry jobs.` and prints neither a candidate export nor any run/candidate ID. Compose is invoked only as `compose run --rm --no-deps --no-TTY workspace-maintenance ...`; output never includes child stderr or secrets. diff --git a/tools/thothctl/cmd/thothctl/main.go b/tools/thothctl/cmd/thothctl/main.go index 728197cf..b7ec8883 100644 --- a/tools/thothctl/cmd/thothctl/main.go +++ b/tools/thothctl/cmd/thothctl/main.go @@ -107,16 +107,6 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) int { if parseErr != nil { return commandUsageError(stderr, parseErr.Error()) } - 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()) - } - fmt.Fprintf(stderr, "thothctl: %s\n", output.Sanitize(operationErr.Error(), secretValues)) - return 1 - } jsonMode := true switch c := workspaceCommand.(type) { case workspaceops.InspectCommand: @@ -134,17 +124,29 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) int { case workspaceops.RunRequest: jsonMode = c.JSON } - publicResult, projectionErr := projectWorkspaceResult(result, secretValues) - if projectionErr != nil { - fmt.Fprintln(stderr, "thothctl: invalid workspace result") + var finalOutput []byte + result, operationErr := workspaceops.RunWithProjectorAndSecrets(ctx, installation, runner, workspaceCommand, nil, func(result workspaceops.Result) (workspaceops.Result, error) { + return projectWorkspaceResult(result, secretValues) + }, secretValues, func(result workspaceops.Result) error { + var err error + if jsonMode { + finalOutput, err = encodeWorkspaceJSON(result) + } else { + finalOutput, err = encodeWorkspaceHuman(result) + } + if err != nil || len(finalOutput) > maxPublicStdoutBytes { + return errors.New("workspace result exceeds output limit") + } + return nil + }) + if operationErr != nil { + if workspaceUsageError(operationErr) { + return commandUsageError(stderr, operationErr.Error()) + } + fmt.Fprintf(stderr, "thothctl: %s\n", output.Sanitize(operationErr.Error(), secretValues)) return 1 } - if jsonMode { - if err := writeWorkspaceJSON(stdout, publicResult); err != nil { - fmt.Fprintln(stderr, "thothctl: workspace result exceeds output limit") - return 1 - } - } else if err := renderWorkspaceHuman(stdout, publicResult); err != nil { + if _, err := stdout.Write(finalOutput); err != nil { fmt.Fprintln(stderr, "thothctl: workspace result exceeds output limit") return 1 } @@ -268,21 +270,34 @@ func (w *boundedWriter) Write(p []byte) (int, error) { return n, err } -func writeWorkspaceJSON(w io.Writer, result workspaceops.Result) error { +func encodeWorkspaceJSON(result workspaceops.Result) ([]byte, error) { var encoded bytes.Buffer encoder := json.NewEncoder(&encoded) // Keep public output compact and avoid HTML-escape amplification of warnings. encoder.SetEscapeHTML(false) if err := encoder.Encode(result); err != nil { - return err + return nil, err } - if encoded.Len() > maxPublicStdoutBytes { + return encoded.Bytes(), nil +} + +func writeWorkspaceJSON(w io.Writer, result workspaceops.Result) error { + encoded, err := encodeWorkspaceJSON(result) + if err != nil || len(encoded) > maxPublicStdoutBytes { return io.ErrShortWrite } - _, err := w.Write(encoded.Bytes()) + _, err = w.Write(encoded) return err } +func encodeWorkspaceHuman(result workspaceops.Result) ([]byte, error) { + var encoded bytes.Buffer + if err := renderWorkspaceHuman(&encoded, result); err != nil { + return nil, err + } + return encoded.Bytes(), nil +} + 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). @@ -361,9 +376,35 @@ func projectWorkspaceResult(result workspaceops.Result, secretValues []string) ( return workspaceops.Result{}, err } } + if publicResultContainsSecret(result, secretValues) { + return workspaceops.Result{}, errors.New("workspace result contains a declared secret") + } return result, nil } +func publicResultContainsSecret(result workspaceops.Result, secrets []string) bool { + values := []string{result.Status, result.Code, result.WorkspaceID, result.WorkspaceRevision, result.DescriptorBlob, result.Operation, result.RunID} + values = append(values, result.CompletedStages...) + values = append(values, result.Warnings...) + for key, value := range result.ChildRuns { + values = append(values, key, value) + } + for key := range result.Counts { + values = append(values, key) + } + for _, artifact := range result.ArtifactIdentities { + values = append(values, artifact.Kind, artifact.Digest) + } + for _, value := range values { + for _, secret := range secrets { + if secret != "" && strings.Contains(value, secret) { + return true + } + } + } + return false +} + func renderWorkspaceHuman(w io.Writer, result workspaceops.Result) error { if result.Code == workspaceops.CodeRegistryBootstrapRecoveryConflict { _, err := fmt.Fprintln(w, "Bootstrap recovery is ambiguous or corrupt; inspect the installation registry jobs.") diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index f6517504..a8513432 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/tools/thothctl/cmd/thothctl/main_test.go @@ -134,20 +134,19 @@ func TestRunWorkspacePublicDispatchExitMatrix(t *testing.T) { } func TestRunWorkspaceBoundsFinalJSONEncoding(t *testing.T) { - fixture := newCLIFixture(t, "") - fixture.setEnvironment(t) - 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()) + for _, tc := range []struct{ name string; final int; wantCode int }{{"exact", 1 << 20, 0}, {"one-over", (1 << 20) + 1, 1}} { + t.Run(tc.name, func(t *testing.T) { + fixture := newCLIFixture(t, "") + fixture.setEnvironment(t) + payload, encodedLength := boundedWorkspaceResultPayload(t, tc.final) + if encodedLength != tc.final { t.Fatalf("final encoded length = %d, want %d", encodedLength, tc.final) } + 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 || (tc.wantCode == 0 && stdout.Len() != tc.final) || (tc.wantCode != 0 && stdout.Len() != 0) { + t.Fatalf("bounded output = exit %d stdout %d stderr %q", code, stdout.Len(), stderr.String()) + } + }) } } diff --git a/tools/thothctl/internal/safeio/files_linux.go b/tools/thothctl/internal/safeio/files_linux.go index e5f64be8..31dde734 100644 --- a/tools/thothctl/internal/safeio/files_linux.go +++ b/tools/thothctl/internal/safeio/files_linux.go @@ -92,8 +92,6 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err 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) @@ -103,14 +101,12 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err _ = unix.Close(dir) dir = next } + // This is a capability publication API: the retained parent is the namespace + // anchor. The lexical check is only a pre-commit diagnostic and cannot close an + // ancestor rename race atomically. 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 @@ -128,30 +124,18 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err 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 } + var expected unix.Stat_t + if err := unix.Fstat(fd, &expected); err != nil || expected.Nlink != 0 || expected.Mode&unix.S_IFMT != unix.S_IFREG { + return fail() + } if err := file.Chmod(mode); err != nil { return fail() } - n, err := file.Write(contents) - if err != nil || n != len(contents) { + if n, err := file.Write(contents); err != nil || n != len(contents) { return fail() } if err := file.Sync(); err != nil { @@ -164,24 +148,24 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err 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 { + // All fallible preparation, checks, and syncs happen before the linearization + // point. A directory sync here is best effort durability for the retained + // parent; it cannot be used to report failure after Linkat. + if err := unix.Fsync(dir); 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 { + if !recheckUnixParentPath(components[:len(components)-1], dir) { return fail() } - if err := unix.Fsync(dir); err != nil || !recheckUnixParentPath(components[:len(components)-1], dir) { + if err := unix.Linkat(fd, "", dir, components[len(components)-1], unix.AT_EMPTY_PATH); err != nil { return fail() } - if err := closeFile(); err != nil { - cleanup() - return ErrUnsafeFile - } + // Linkat is the final commit. Close errors and any post-commit observations + // are deliberately ignored: returning ErrUnsafeFile here would lie about a + // candidate that is already committed, and pathname cleanup would be racy. + _ = closeFile() return nil } - func privateStageName() (string, error) { var random [16]byte if _, err := rand.Read(random[:]); err != nil { diff --git a/tools/thothctl/internal/safeio/files_unix.go b/tools/thothctl/internal/safeio/files_unix.go index 10cbf2d2..66533930 100644 --- a/tools/thothctl/internal/safeio/files_unix.go +++ b/tools/thothctl/internal/safeio/files_unix.go @@ -80,6 +80,11 @@ func closeUnixDescriptors(descriptors []int) { } } +// Darwin uses a named stage because it has no relinkable O_TMPFILE equivalent. +// The caller must provide a trusted parent namespace: same-UID namespace mutation +// (including stage hard-link/replacement races and ancestor replacement) is outside +// this mode's threat model. Under that precondition renameatx_np(RENAME_EXCL) is the +// final fallible no-replace commit and removes the stage atomically. 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 @@ -92,24 +97,18 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err if err != nil { return ErrUnsafeFile } - // 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 { return ErrUnsafeFile } - unix.Close(dir) + _ = unix.Close(dir) dir = next } if !recheckUnixParentPath(components[:len(components)-1], dir) { return ErrUnsafeFile } - - // Build the candidate under a private, same-parent name. Only after it is fully - // written, synced, and identity-checked do we link it into the requested leaf. stage, err := privateStageName() if err != nil { return ErrUnsafeFile @@ -120,90 +119,72 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err } stageFile := os.NewFile(uintptr(stageFD), "thothctl-safeio-stage") if stageFile == nil { - unix.Close(stageFD) + _ = unix.Close(stageFD) _ = unix.Unlinkat(dir, stage, 0) return ErrUnsafeFile } - stageCreated := true + closed := false + closeStage := func() error { + if closed { + return nil + } + closed = true + return stageFile.Close() + } + defer func() { _ = closeStage() }() var staged unix.Stat_t if err := unix.Fstat(stageFD, &staged); err != nil || staged.Nlink != 1 || staged.Mode&unix.S_IFMT != unix.S_IFREG { - _ = stageFile.Close() + _ = closeStage() _ = unix.Unlinkat(dir, stage, 0) return ErrUnsafeFile } - published := false - // Keep the inode identity immutable across every check and cleanup path. - var publishedIdentity = staged cleanup := func() { - if published { - var current unix.Stat_t - if unix.Fstatat(dir, components[len(components)-1], ¤t, unix.AT_SYMLINK_NOFOLLOW) == nil && - current.Ino == publishedIdentity.Ino && current.Dev == publishedIdentity.Dev { - _ = unix.Unlinkat(dir, components[len(components)-1], 0) - } - } - if stageCreated { - var current unix.Stat_t - if unix.Fstatat(dir, stage, ¤t, unix.AT_SYMLINK_NOFOLLOW) == nil && - current.Ino == staged.Ino && current.Dev == staged.Dev { - _ = unix.Unlinkat(dir, stage, 0) - } + // Trusted-parent mode makes this identity check + unlink pre-commit safe; + // never unlink a replacement observed at the stage name. + var current unix.Stat_t + if unix.Fstatat(dir, stage, ¤t, unix.AT_SYMLINK_NOFOLLOW) == nil && current.Ino == staged.Ino && current.Dev == staged.Dev { + _ = unix.Unlinkat(dir, stage, 0) } } - fail := func() error { - _ = stageFile.Close() - cleanup() - return ErrUnsafeFile - } + fail := func() error { _ = closeStage(); cleanup(); return ErrUnsafeFile } if err := stageFile.Chmod(mode); err != nil { return fail() } - n, err := stageFile.Write(contents) - if err != nil || n != len(contents) { + if n, err := stageFile.Write(contents); err != nil || n != len(contents) { return fail() } 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)) { + var after unix.Stat_t + if err := unix.Fstat(stageFD, &after); err != nil || after.Nlink != 1 || after.Mode&unix.S_IFMT != unix.S_IFREG || after.Size != int64(len(contents)) { return fail() } - if err := stageFile.Close(); err != nil { - cleanup() - return ErrUnsafeFile - } - stageCreated = true if !recheckUnixParentPath(components[:len(components)-1], dir) { return fail() } - if err := unix.Linkat(dir, stage, dir, components[len(components)-1], 0); err != nil { + // Verify the named stage and its retained handle immediately before commit. + var named unix.Stat_t + if err := unix.Fstatat(dir, stage, &named, unix.AT_SYMLINK_NOFOLLOW); err != nil || named.Ino != staged.Ino || named.Dev != staged.Dev || named.Nlink != 1 { return fail() } - published = true - 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 { + if err := unix.Fstat(stageFD, &after); err != nil || after.Ino != staged.Ino || after.Dev != staged.Dev || after.Nlink != 1 { return fail() } - if err := unix.Unlinkat(dir, stage, 0); err != nil { + if err := unix.Fsync(dir); err != nil { return fail() } - stageCreated = false - 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 { + if !recheckUnixParentPath(components[:len(components)-1], dir) { return fail() } - if err := unix.Fsync(dir); err != nil || !recheckUnixParentPath(components[:len(components)-1], dir) { + if err := unix.RenameatxNp(dir, stage, dir, components[len(components)-1], unix.RENAME_EXCL); err != nil { return fail() } + // renameatx_np is the final commit. Closing the still-open handle is ignored + // after success and never triggers pathname cleanup or a false failure. + _ = closeStage() return nil } - func privateStageName() (string, error) { var random [16]byte if _, err := rand.Read(random[:]); err != nil { diff --git a/tools/thothctl/internal/safeio/files_windows.go b/tools/thothctl/internal/safeio/files_windows.go index 2038b081..d65fbfb6 100644 --- a/tools/thothctl/internal/safeio/files_windows.go +++ b/tools/thothctl/internal/safeio/files_windows.go @@ -20,9 +20,10 @@ const ( // Retained input handles deny delete sharing while permitting ordinary reads // and writes by trusted callers. windowsRetainedHandleShareMode uint32 = windows.FILE_SHARE_READ | windows.FILE_SHARE_WRITE - // Outputs are opened for exclusive publication: deny write/delete sharing, - // but permit the exact-identity read recheck below. - windowsOutputHandleShareMode uint32 = windows.FILE_SHARE_READ + // A discoverable stage is protected by mandatory zero-share semantics for its + // entire lifetime. This denies reads, writes, rename, delete, and hard-link + // acquisition by another handle until our final handle-relative rename. + windowsOutputHandleShareMode uint32 = 0 ) // ReadCanonicalRegular opens each component with FILE_FLAG_OPEN_REPARSE_POINT and rejects a @@ -139,6 +140,10 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err return ErrUnsafeFile } defer closeWindowsHandles(retainedParents) + if len(retainedParents) == 0 { + return ErrUnsafeFile + } + parentHandle := retainedParents[len(retainedParents)-1] securityDescriptor, securityAttributes, err := ownerOnlySecurityAttributes() if err != nil { return ErrUnsafeFile @@ -149,77 +154,86 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err return ErrUnsafeFile } stagePath := filepath.Join(parent, stageName) - stageHandle, err := windows.CreateFile(windows.StringToUTF16Ptr(stagePath), windows.GENERIC_WRITE, windowsOutputHandleShareMode, securityAttributes, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) + stageHandle, err := windows.CreateFile(windows.StringToUTF16Ptr(stagePath), windows.GENERIC_WRITE|windows.DELETE, windowsOutputHandleShareMode, securityAttributes, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) if err != nil { return ErrUnsafeFile } stageFile := os.NewFile(uintptr(stageHandle), "thothctl-safeio-stage") if stageFile == nil { - windows.CloseHandle(stageHandle) - _ = windows.DeleteFile(windows.StringToUTF16Ptr(stagePath)) + _ = deleteWindowsHandle(stageHandle) + _ = windows.CloseHandle(stageHandle) return ErrUnsafeFile } - stageCreated := true - published := false - var staged, publishedIdentity windows.ByHandleFileInformation - if err := windows.GetFileInformationByHandle(stageHandle, &staged); err != nil || staged.NumberOfLinks != 1 { - _ = stageFile.Close() - _ = windows.DeleteFile(windows.StringToUTF16Ptr(stagePath)) - return ErrUnsafeFile - } - cleanup := func() { - if published { - removeWindowsIfIdentity(filepath.Join(parent, filepath.Base(path)), publishedIdentity) - } - if stageCreated { - removeWindowsIfIdentity(stagePath, staged) + closed := false + closeStage := func() error { + if closed { + return nil } + closed = true + return stageFile.Close() } - fail := func() error { - _ = stageFile.Close() - cleanup() - return ErrUnsafeFile + defer func() { _ = closeStage() }() + var staged windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(stageHandle, &staged); err != nil || staged.NumberOfLinks != 1 || staged.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 || staged.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 { + return failWindowsStage(stageHandle, closeStage) } + fail := func() error { _ = deleteWindowsHandle(stageHandle); _ = closeStage(); return ErrUnsafeFile } if n, err := stageFile.Write(contents); err != nil || n != len(contents) { return fail() } 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 == 0 || afterWrite.FileSizeHigh != uint32(uint64(len(contents))>>32) || afterWrite.FileSizeLow != uint32(len(contents)) { + var after windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(stageHandle, &after); err != nil || after.NumberOfLinks != 1 || after.FileSizeHigh != uint32(uint64(len(contents))>>32) || after.FileSizeLow != uint32(len(contents)) { return fail() } - if err := stageFile.Close(); err != nil { - cleanup() - return ErrUnsafeFile - } - // CreateHardLink is an atomic, same-volume, no-replace publication. The final - // pathname can never refer to a partially written candidate. - finalPath := filepath.Join(parent, filepath.Base(path)) - if err := windows.CreateHardLink(windows.StringToUTF16Ptr(finalPath), windows.StringToUTF16Ptr(stagePath), 0); err != nil { - return fail() - } - published = true - publishedIdentity = staged - check, identityErr := windowsFileIdentity(finalPath) - if identityErr != nil || check.NumberOfLinks < 2 || !sameWindowsFile(staged, check) { - return fail() - } - if err := windows.DeleteFile(windows.StringToUTF16Ptr(stagePath)); err != nil { - return fail() - } - stageCreated = false - finalIdentity, identityErr := windowsFileIdentity(finalPath) - if identityErr != nil || finalIdentity.NumberOfLinks == 0 || !sameWindowsFile(staged, finalIdentity) { + // Keep the exact stage handle open with zero sharing through this final check + // and atomic no-replace rename. No pathname reopen or cleanup is needed. + if err := renameWindowsHandle(stageHandle, parentHandle, filepath.Base(path)); err != nil { return fail() } + // Handle-relative rename is the final commit. Handle close is intentionally + // ignored after success; no fallible observation or pathname cleanup follows. + _ = closeStage() return nil } +// failWindowsStage disposes an exact handle when initial identity inspection fails. +func failWindowsStage(handle windows.Handle, closeStage func() error) error { + _ = deleteWindowsHandle(handle) + _ = closeStage() + return ErrUnsafeFile +} + +func deleteWindowsHandle(handle windows.Handle) error { + var disposition byte = 1 + return windows.SetFileInformationByHandle(handle, windows.FileDispositionInfo, &disposition, uint32(unsafe.Sizeof(disposition))) +} + +type windowsFileRenameInfo struct { + ReplaceIfExists uint8 + RootDirectory windows.Handle + FileNameLength uint32 + FileName [1]uint16 +} + +func renameWindowsHandle(handle, parent windows.Handle, leaf string) error { + name, err := windows.UTF16FromString(leaf) + if err != nil { + return err + } + name = name[:len(name)-1] + base := unsafe.Offsetof(windowsFileRenameInfo{}.FileName) + buffer := make([]byte, int(base)+len(name)*2) + info := (*windowsFileRenameInfo)(unsafe.Pointer(&buffer[0])) + info.ReplaceIfExists = 0 + info.RootDirectory = parent + info.FileNameLength = uint32(len(name) * 2) + nameBytes := unsafe.Slice((*uint16)(unsafe.Pointer(&buffer[base])), len(name)) + copy(nameBytes, name) + return windows.SetFileInformationByHandle(handle, windows.FileRenameInfo, &buffer[0], uint32(len(buffer))) +} func privateWindowsStageName() (string, error) { var random [16]byte if _, err := rand.Read(random[:]); err != nil { @@ -228,26 +242,6 @@ func privateWindowsStageName() (string, error) { return ".thothctl-candidate-" + hex.EncodeToString(random[:]), nil } -func windowsFileIdentity(path string) (windows.ByHandleFileInformation, error) { - h, err := windows.CreateFile(windows.StringToUTF16Ptr(path), windows.GENERIC_READ, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) - if err != nil { - return windows.ByHandleFileInformation{}, err - } - defer windows.CloseHandle(h) - var information windows.ByHandleFileInformation - if err := windows.GetFileInformationByHandle(h, &information); err != nil || information.NumberOfLinks == 0 || information.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 || information.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 { - return windows.ByHandleFileInformation{}, ErrUnsafeFile - } - return information, nil -} - -func removeWindowsIfIdentity(path string, expected windows.ByHandleFileInformation) { - got, err := windowsFileIdentity(path) - if err == nil && sameWindowsFile(got, expected) { - _ = windows.DeleteFile(windows.StringToUTF16Ptr(path)) - } -} - func openWindowsParents(path string) (string, []windows.Handle, error) { volume := filepath.VolumeName(path) root := volume + string(filepath.Separator) @@ -348,7 +342,7 @@ func ownerOnlySecurityAttributes() (*windows.SECURITY_DESCRIPTOR, *windows.Secur TrusteeValue: windows.TrusteeValueFromSID(user.User.Sid), } entries := []windows.EXPLICIT_ACCESS{{ - AccessPermissions: windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE, + AccessPermissions: windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE | windows.DELETE, AccessMode: windows.SET_ACCESS, Inheritance: windows.NO_INHERITANCE, Trustee: trustee, diff --git a/tools/thothctl/internal/safeio/files_windows_test.go b/tools/thothctl/internal/safeio/files_windows_test.go index c2bbb2dc..ae4325f1 100644 --- a/tools/thothctl/internal/safeio/files_windows_test.go +++ b/tools/thothctl/internal/safeio/files_windows_test.go @@ -13,7 +13,7 @@ import ( ) const expectedWindowsRetainedHandleShareMode = windows.FILE_SHARE_READ | windows.FILE_SHARE_WRITE -const expectedWindowsOutputHandleShareMode = windows.FILE_SHARE_READ +const expectedWindowsOutputHandleShareMode = 0 // Keep this contract compile-enforced so Windows cross-test compilation catches a future // FILE_SHARE_DELETE regression even when the tests are compiled on a non-Windows host. @@ -80,6 +80,51 @@ func TestOpenWindowsComponentBlocksMutationWhileHandleIsRetained(t *testing.T) { }) } +func TestWindowsStageHandleDeniesReadRenameDeleteAndHardlink(t *testing.T) { + root := filepath.Join(t.TempDir(), "parent") + if err := os.Mkdir(root, 0o700); err != nil { + t.Fatal(err) + } + securityDescriptor, securityAttributes, err := ownerOnlySecurityAttributes() + if err != nil { + t.Fatal(err) + } + _ = securityDescriptor + stagePath := filepath.Join(root, ".thothctl-candidate-test") + h, err := windows.CreateFile(windows.StringToUTF16Ptr(stagePath), windows.GENERIC_WRITE|windows.DELETE, 0, securityAttributes, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) + if err != nil { + t.Fatal(err) + } + closed := false + defer func() { + if !closed { + _ = windows.CloseHandle(h) + } + }() + if _, err := windows.CreateFile(windows.StringToUTF16Ptr(stagePath), windows.GENERIC_READ, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0); err == nil { + t.Fatal("stage read succeeded while zero-share handle was open") + } + if err := os.Rename(stagePath, stagePath+"-renamed"); err == nil { + t.Fatal("stage rename succeeded while handle was open") + } + if err := os.Link(stagePath, filepath.Join(root, "stolen")); err == nil { + t.Fatal("stage hardlink succeeded while handle was open") + } + if err := os.Remove(stagePath); err == nil { + t.Fatal("stage delete succeeded while handle was open") + } + if err := deleteWindowsHandle(h); err != nil { + t.Fatal(err) + } + if err := windows.CloseHandle(h); err != nil { + t.Fatal(err) + } + closed = true + if _, err := os.Stat(stagePath); !os.IsNotExist(err) { + t.Fatalf("disposed stage remains: %v", err) + } +} + func TestWriteCanonicalExclusiveRequiresRestrictiveMode(t *testing.T) { if err := writeCanonicalExclusive(`C:\\tmp\\thothctl-output.yaml`, []byte("x"), 0o640); err == nil { t.Fatal("accepted non-restrictive output mode") diff --git a/tools/thothctl/internal/workspaceops/operations.go b/tools/thothctl/internal/workspaceops/operations.go index 79f43a8d..d5ffd4c7 100644 --- a/tools/thothctl/internal/workspaceops/operations.go +++ b/tools/thothctl/internal/workspaceops/operations.go @@ -471,6 +471,14 @@ func Run(ctx context.Context, installation config.Installation, runner compose.R // 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) { + return RunWithProjectorAndSecrets(ctx, installation, runner, command, stdin, projector, nil, nil) +} + +// RunWithProjectorAndSecrets adds the host's universal no-secret boundary and a +// final-public-encoding preflight. prePublish runs after projection but before +// WriteCanonicalExclusive, so deterministic output rejection cannot consume the +// exclusive candidate name. +func RunWithProjectorAndSecrets(ctx context.Context, installation config.Installation, runner compose.Runner, command Command, stdin io.Reader, projector ResultProjector, secrets []string, prePublish func(Result) error) (Result, error) { env, generated, e := makeInput(command) if e != nil { return Result{}, e @@ -555,6 +563,9 @@ func RunWithProjector(ctx context.Context, installation config.Installation, run } result = projected } + if err := rejectDeclaredSecrets(result, secrets); err != nil { + return Result{}, err + } if hasExport { if export == nil { return Result{}, errors.New("invalid candidate export") @@ -562,11 +573,24 @@ func RunWithProjector(ctx context.Context, installation config.Installation, run if !candidateBoundToResult(*export, result) { return Result{}, errors.New("invalid candidate identity") } - if err := publishCandidate(export, result, outPath(command)); err != nil { + candidate, err := decodeCandidateExport(export) + if err != nil || containsDeclaredSecret(candidate, secrets) { + return Result{}, errors.New("invalid candidate export") + } + if prePublish != nil { + if err := prePublish(result); err != nil { + return Result{}, err + } + } + if err := publishCandidateBytes(export, result, outPath(command), candidate); err != nil { return Result{}, err } } else if outPath(command) != "" { return Result{}, errors.New("candidate export is required") + } else if prePublish != nil { + if err := prePublish(result); err != nil { + return Result{}, err + } } return result, nil } @@ -595,39 +619,80 @@ func candidateBoundToResult(x hostExport, result Result) bool { } func publishCandidate(x *hostExport, result Result, path string) error { - if x.MediaType != "application/yaml" && x.MediaType != "text/yaml" { - return errors.New("invalid candidate export") + b, err := decodeCandidateExport(x) + if err != nil { + return err } - if !digestPattern.MatchString(x.SHA256) { - return errors.New("invalid candidate export") + return publishCandidateBytes(x, result, path, b) +} + +func decodeCandidateExport(x *hostExport) ([]byte, error) { + if x.MediaType != "application/yaml" && x.MediaType != "text/yaml" || !digestPattern.MatchString(x.SHA256) { + return nil, errors.New("invalid candidate export") } - b, e := base64.StdEncoding.DecodeString(x.ContentBase64) - if e != nil || len(b) > maxCandidate || !utf8.Valid(b) { - return errors.New("invalid candidate export") + b, err := base64.StdEncoding.DecodeString(x.ContentBase64) + if err != nil || len(b) > maxCandidate || !utf8.Valid(b) || DigestBytes(b) != x.SHA256 { + return nil, errors.New("invalid candidate export") } - if DigestBytes(b) != x.SHA256 { - return errors.New("invalid candidate export") - } - var doc any decoder := yaml.NewDecoder(bytes.NewReader(b)) + var doc any if decoder.Decode(&doc) != nil { - return errors.New("invalid candidate export") + return nil, errors.New("invalid candidate export") } var trailing any if err := decoder.Decode(&trailing); err != io.EOF { - return errors.New("invalid candidate export") + return nil, errors.New("invalid candidate export") } + return b, nil +} + +func publishCandidateBytes(x *hostExport, result Result, path string, b []byte) error { if result.RunID == "" || !runIDPattern.MatchString(result.RunID) { return errors.New("invalid candidate identity") } if path == "" { return nil } - if e = safeio.WriteCanonicalExclusive(path, b, 0o600); e != nil { + if err := safeio.WriteCanonicalExclusive(path, b, 0o600); err != nil { return errors.New("unsafe output file") } return nil } + +func containsDeclaredSecret(contents []byte, secrets []string) bool { + for _, secret := range secrets { + if secret != "" && bytes.Contains(contents, []byte(secret)) { + return true + } + } + return false +} + +// rejectDeclaredSecrets traverses the typed public envelope rather than its JSON +// encoding: JSON escaping must not turn a declared value into an apparent non-match. +func rejectDeclaredSecrets(result Result, secrets []string) error { + values := make([]string, 0, 16) + values = append(values, result.Status, result.Code, result.WorkspaceID, result.WorkspaceRevision, result.DescriptorBlob, result.Operation, result.RunID) + values = append(values, result.CompletedStages...) + values = append(values, result.Warnings...) + for key, value := range result.ChildRuns { + values = append(values, key, value) + } + for key := range result.Counts { + values = append(values, key) + } + for _, artifact := range result.ArtifactIdentities { + values = append(values, artifact.Kind, artifact.Digest) + } + for _, value := range values { + for _, secret := range secrets { + if secret != "" && strings.Contains(value, secret) { + return errors.New("workspace result contains a declared secret") + } + } + } + return nil +} func validateResult(r Result, workspace, operation string) error { if r.SchemaVersion != 1 || r.WorkspaceID != workspace || r.Operation != operation || !validStatus(r.Status) || !validCode(r.Code) || !revisionPattern.MatchString(r.WorkspaceRevision) || !revisionPattern.MatchString(r.DescriptorBlob) { return errors.New("invalid workspace result")