diff --git a/README.md b/README.md index 5ee85c8..de7f056 100644 --- a/README.md +++ b/README.md @@ -123,15 +123,18 @@ is what makes editing a large file possible without transferring it. `fs_write` and `fs_replace` commit through a temp file in the same directory followed by `rename`. Mode, extended attributes and ACL are preserved exactly; `umask` does not eat mode bits. If they cannot be preserved, the write is -refused and the file left unchanged. +refused and the file left unchanged. A file the server's user **cannot write** +(mode, owner or ACL) is refused and left unchanged, as a plain write would be. -Two consequences, both deliberate: +Three consequences, all deliberate: - A replace **breaks a hard link** to the target (new inode). The alternative is in-place truncation, which is not crash-safe. - A file whose ACL **denies delete** cannot be written. That entry exists to stop the inode being replaced, so stripping it to force the write through would defeat the protection it enforces. The refusal says so. +- A replaced file is **owned by the server's user**. A writable file owned by + someone else changes owner, because the replace creates a new inode. ## Integrity across a hop diff --git a/docs/ACCEPTANCE.md b/docs/ACCEPTANCE.md index dd69203..a6fcdc0 100644 --- a/docs/ACCEPTANCE.md +++ b/docs/ACCEPTANCE.md @@ -80,6 +80,8 @@ from a self-move. | C9 | a target protected by a **file flag** (`uchg`/`uappnd`) | refused, file unchanged, and the refusal names `chflags` — never a deny-delete ACL entry the file does not have | | C10 | BSD file flags across a successful replace | **not preserved** — a stated non-guarantee, since `cp -pN`'s `-N` is what stops a copied `uchg` stranding fsMCP's own temp file | | C11 | `if_sha256` on a very large file | verified by streaming for `fs_write`/`fs_delete`; memory does not scale with the file | +| C12 | a target the server's user cannot write (mode, owner or ACL) | refused naming "not writable", file unchanged, no temp file left | +| C13 | owner of a writable target owned by another user | **not preserved** — the replacement is owned by the server's user; a stated non-guarantee of a rename-based replace | **Known and deliberate:** a replace breaks a hard link to the target (new inode). Assert it rather than fix it — the alternative is in-place truncation, diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 63b3c59..633b38e 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -383,6 +383,17 @@ write is refused with the flag named. This is review issue #20; prove it with a test that asserts `ls -le` and `xattr -l` output is identical before and after. +A target the server's user cannot write is refused before any temp file exists. +`rename(2)` consults only the directory, so the check opens the target for +writing and so gets the answer a plain write would get (mode, owner, ACL). It +is not a security boundary: a change between the check and the rename is a +non-goal. + +**Ownership is not preserved.** A rename-based replace creates a new inode, so a +replaced file comes back owned by the server's user. A writable file owned by +someone else changes owner. This is a stated non-guarantee, alongside the +hard-link break. + Clean up the temp file on every failure path. ## Integrity across the hop: `_meta.args_sha256` diff --git a/docs/TRACEABILITY.md b/docs/TRACEABILITY.md index 2022c81..53c688a 100644 --- a/docs/TRACEABILITY.md +++ b/docs/TRACEABILITY.md @@ -55,6 +55,8 @@ nothing covers it. Every row below is the state *after* this pass; the | C9 | file-flag target refused, naming chflags | `TestAtomicReplaceDistinguishesAFileFlagFromAnACL`, `TestWriteNamesTheRightRemedyForEachCause` | **missing** — only the ACL cause was planted, so both shared one diagnosis | covered | | C10 | file flags not preserved across a replace | `TestAtomicReplaceDoesNotPreserveFileFlags` | **missing** | covered | | C11 | precondition streamed, not read | `TestCheckPreconditionDoesNotAllocateTheWholeFile`, `TestCheckPrecondition` | **missing** | covered | +| C12 | unwritable target refused, file unchanged, no temp left | `TestAtomicReplaceRefusesTargetOwnedByAnotherUser`, `TestWriteRefusesUnwritableTarget`, `TestReplaceRefusesTargetOwnedByAnotherUser` | **missing** | covered | +| C13 | owner not preserved for a writable foreign-owned target | documented, no test | n/a | n/a | Hard-link-breaks-on-replace ("known and deliberate", not independently numbered) is pinned by `TestAtomicReplaceBreaksHardLink`. diff --git a/internal/fsapi/atomic.go b/internal/fsapi/atomic.go index bdaa004..df65558 100644 --- a/internal/fsapi/atomic.go +++ b/internal/fsapi/atomic.go @@ -33,6 +33,11 @@ var errAttrsUnpreservable = errors.New("attributes could not be preserved") // inode, not the bytes — but fsMCP commits through a rename, which it forbids. var errTargetUndeletable = errors.New("target cannot be replaced") +// errTargetNotWritable marks a replace refused because the target's mode, +// owner or ACL denies the server's user write access. rename(2) never asks +// that question, so without the refusal a read-only file would be replaced. +var errTargetNotWritable = errors.New("target is not writable") + // errTargetFlagged marks a replace refused because the target carries a BSD // file flag that forbids being replaced. Kept distinct from // errTargetUndeletable even though rename(2) reports both as EPERM: the @@ -88,6 +93,28 @@ func AtomicReplace(root *Root, targetPath string, data []byte) error { if exists && !fi.Mode().IsRegular() { return errNotRegularFile } + if exists { + // Deliberate: rename(2) only consults the directory, so a file the + // server's user cannot write would be replaced anyway. Opening it for + // write asks the kernel the same question a plain write would (mode, + // owner, ACL). This is an expectation check, not a security boundary; + // a change between it and the rename is a non-goal. + f, err := root.OpenFile(targetPath, os.O_WRONLY, 0) + if err != nil { + switch { + case errors.Is(err, syscall.EACCES): + return errTargetNotWritable + case errors.Is(err, syscall.EPERM): + // A file flag (uchg, uappnd) refuses the open with EPERM, not + // EACCES. Leave it to the rename, whose EPERM is classified + // as a flag further down so the remedy named is chflags. + default: + return err + } + } else { + f.Close() + } + } tmpPath, err := reserveTempName(root, dir, !exists) if err != nil { diff --git a/internal/fsapi/atomic_test.go b/internal/fsapi/atomic_test.go index e693fe0..bacc3c1 100644 --- a/internal/fsapi/atomic_test.go +++ b/internal/fsapi/atomic_test.go @@ -103,7 +103,10 @@ func TestAtomicReplaceNoTempFileLeftAfterAttrFailure(t *testing.T) { if err := os.WriteFile(target, []byte("secret"), 0o644); err != nil { t.Fatal(err) } - if err := os.Chmod(target, 0o000); err != nil { + // Write-only, not 0o000: an unwritable target is refused before any + // temp file exists, so only a writable-but-unreadable one reaches the + // attribute copy this test is about. + if err := os.Chmod(target, 0o200); err != nil { t.Fatal(err) } t.Cleanup(func() { os.Chmod(target, 0o644) }) @@ -701,3 +704,78 @@ func TestAtomicReplaceDoesNotPreserveFileFlags(t *testing.T) { t.Errorf("st_flags = 0x%08x, want 0 — DESIGN.md states flags are NOT preserved", st.Flags) } } + +// plantForeignOwnedFile creates name in rootDir as a 0664 file owned by uid 0, +// a file the test's user can see but not write, inside a directory the user +// can. It skips when the machine cannot give a file to another user. +func plantForeignOwnedFile(t *testing.T, rootDir, name string, content []byte) { + t.Helper() + if os.Getuid() == 0 { + t.Skip("needs a non-root test user to plant a file it cannot write") + } + if err := exec.Command("sudo", "-n", "true").Run(); err != nil { + t.Skip("needs passwordless sudo to plant a file owned by another user") + } + p := filepath.Join(rootDir, name) + if err := os.WriteFile(p, content, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(p, 0o644); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("sudo", "-n", "chown", "0:0", p).CombinedOutput(); err != nil { + t.Fatalf("sudo chown: %v: %s", err, out) + } + t.Cleanup(func() { exec.Command("sudo", "-n", "rm", "-f", p).Run() }) +} + +// diskState is what a refused replace must leave exactly as it found it. +type diskState struct { + content string + uid uint32 + mode os.FileMode +} + +func snapshotDisk(t *testing.T, p string) diskState { + t.Helper() + fi, err := os.Lstat(p) + if err != nil { + t.Fatal(err) + } + b, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + return diskState{content: string(b), uid: fi.Sys().(*syscall.Stat_t).Uid, mode: fi.Mode()} +} + +// assertDiskUnchanged compares against before and checks no temp file remains. +// Callers run it before checking the returned error, so a replace that went +// through fails here first. +func assertDiskUnchanged(t *testing.T, rootDir, name string, before diskState) { + t.Helper() + after := snapshotDisk(t, filepath.Join(rootDir, name)) + if after.content != before.content { + t.Errorf("bytes changed: %q -> %q", before.content, after.content) + } + if after.uid != before.uid { + t.Errorf("owner uid changed: %d -> %d", before.uid, after.uid) + } + if after.mode != before.mode { + t.Errorf("mode changed: %v -> %v", before.mode, after.mode) + } + assertNoTempFiles(t, rootDir) +} + +func TestAtomicReplaceRefusesTargetOwnedByAnotherUser(t *testing.T) { + root, rootDir := newTestRoot(t) + plantForeignOwnedFile(t, rootDir, "foreign.txt", []byte("original")) + before := snapshotDisk(t, filepath.Join(rootDir, "foreign.txt")) + + err := AtomicReplace(root, "foreign.txt", []byte("replacement")) + + assertDiskUnchanged(t, rootDir, "foreign.txt", before) + if !errors.Is(err, errTargetNotWritable) { + t.Errorf("AtomicReplace = %v, want errTargetNotWritable", err) + } +} diff --git a/internal/fsapi/replace_test.go b/internal/fsapi/replace_test.go index d56a529..9158dd5 100644 --- a/internal/fsapi/replace_test.go +++ b/internal/fsapi/replace_test.go @@ -324,3 +324,18 @@ func TestReplaceDirectoryTargetRefused(t *testing.T) { } func strptr(s string) *string { return &s } + +func TestReplaceRefusesTargetOwnedByAnotherUser(t *testing.T) { + root, rootDir := newTestRoot(t) + plantForeignOwnedFile(t, rootDir, "locked.txt", []byte("original")) + before := snapshotDisk(t, filepath.Join(rootDir, "locked.txt")) + edits := []editArg{{Find: "original", Replace: strptr("changed")}} + + hash := sha256Hex(t, []byte("original")) + result := handleReplace(root, replaceArgsJSON(t, "locked.txt", &hash, edits)) + + assertDiskUnchanged(t, rootDir, "locked.txt", before) + if msg := errorMessage(t, result); !strings.Contains(msg, "not writable") { + t.Errorf("message does not name write permission: %q", msg) + } +} diff --git a/internal/fsapi/write.go b/internal/fsapi/write.go index 1fe5017..0072f50 100644 --- a/internal/fsapi/write.go +++ b/internal/fsapi/write.go @@ -139,6 +139,8 @@ func mapAtomicReplaceError(err error, p string) *proto.CallToolResult { return proto.NewErrorResult(proto.ErrNotAFile, "not a regular file", p) case errors.Is(err, errAttrsUnpreservable): return proto.NewErrorResult(proto.ErrIOError, "the file's mode, extended attributes or ACL could not be preserved; nothing was written", p) + case errors.Is(err, errTargetNotWritable): + return proto.NewErrorResult(proto.ErrIOError, "this file is not writable by the user fsMCP runs as (its mode, owner or an ACL entry denies write), so it was not replaced; nothing was changed. Granting write permission (chmod u+w, or adjusting the ACL) makes it writable through fsMCP", p) case errors.Is(err, errTargetFlagged): return proto.NewErrorResult(proto.ErrIOError, "this file carries a BSD file flag that forbids replacing it (uchg/schg immutable, or uappnd/sappnd append-only), and fsMCP replaces a file by renaming over it, so it cannot be written; nothing was changed. Clearing the flag with chflags makes it writable through fsMCP", p) case errors.Is(err, errTargetUndeletable): diff --git a/internal/fsapi/write_test.go b/internal/fsapi/write_test.go index 21188eb..c795f73 100644 --- a/internal/fsapi/write_test.go +++ b/internal/fsapi/write_test.go @@ -4,8 +4,12 @@ import ( "encoding/base64" "encoding/json" "os" + "os/exec" "path/filepath" + "strings" "testing" + + "fsmcp/internal/proto" ) // writeArgsJSON builds fs_write arguments with an explicit if_sha256 shape, @@ -176,3 +180,63 @@ func TestWriteResultSHA256MatchesContent(t *testing.T) { t.Errorf("sha256 = %v, want %v", decoded["sha256"], want) } } + +// errorMessage returns the message of an error result, failing the test if the +// result is not an io_error. +func errorMessage(t *testing.T, result *proto.CallToolResult) string { + t.Helper() + assertErrorCode(t, result, "io_error") + var decoded struct { + Error struct { + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal([]byte(result.Content[0].Text), &decoded); err != nil { + t.Fatal(err) + } + return decoded.Error.Message +} + +func TestWriteRefusesUnwritableTarget(t *testing.T) { + cases := []struct { + name string + plant func(t *testing.T, rootDir string) + }{ + {"owned by another user", func(t *testing.T, rootDir string) { + plantForeignOwnedFile(t, rootDir, "locked.txt", []byte("original")) + }}, + {"read-only file the user owns", func(t *testing.T, rootDir string) { + p := filepath.Join(rootDir, "locked.txt") + if err := os.WriteFile(p, []byte("original"), 0o444); err != nil { + t.Fatal(err) + } + if err := os.Chmod(p, 0o444); err != nil { + t.Fatal(err) + } + }}, + {"ACL entry denies write", func(t *testing.T, rootDir string) { + p := filepath.Join(rootDir, "locked.txt") + if err := os.WriteFile(p, []byte("original"), 0o644); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("/bin/chmod", "+a", "everyone deny write", p).CombinedOutput(); err != nil { + t.Fatalf("chmod +a: %v: %s", err, out) + } + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + root, rootDir := newTestRoot(t) + tc.plant(t, rootDir) + before := snapshotDisk(t, filepath.Join(rootDir, "locked.txt")) + + hash := sha256Hex(t, []byte("original")) + result := handleWrite(root, writeArgsJSON(t, "locked.txt", "replacement", "", &hash)) + + assertDiskUnchanged(t, rootDir, "locked.txt", before) + if msg := errorMessage(t, result); !strings.Contains(msg, "not writable") { + t.Errorf("message does not name write permission: %q", msg) + } + }) + } +}