diff --git a/free/ci/internal/artifacts/records/read.go b/free/ci/internal/artifacts/records/read.go index 2cb083f12..191f9c8da 100644 --- a/free/ci/internal/artifacts/records/read.go +++ b/free/ci/internal/artifacts/records/read.go @@ -7,7 +7,7 @@ package records // Constraints: "not found" and "not visible" are the same error value; a // record whose blob is gone is reported missing (and remembered) and Get // refuses it (E712 missing_blob); Get writes only inside a directory that -// authz.ArtifactGetDest accepted, creates every file exclusively and +// authz.OpenArtifactGetDest pinned, creates every file exclusively and // re-verifies size and digest of what it wrote. import ( @@ -99,29 +99,15 @@ func (r Reader) Get(ctx context.Context, v authz.Viewer, id, dest, cwd string) ( if err := verifyBlob(r.Blobs, rec); err != nil { return "", rec, err } - if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { - return "", rec, failf(ReasonUnreadable, "destination: %v", err) - } - // re-check after the create: the directory must still be empty and ours - if dir, err = authz.ArtifactGetDest(dir, cwd); err != nil { + root, dir, err := authz.OpenArtifactGetDest(dir, cwd) + if err != nil { return "", rec, err } - return r.write(dir, rel, rec) + defer func() { _ = root.Close() }() + return r.write(root, dir, rel, rec) } -func (r Reader) write(dir, rel string, rec Record) (string, Record, error) { - before, err := os.Lstat(dir) - if err != nil || !before.IsDir() { - return "", rec, failf(ReasonUnreadable, "destination %q is not a plain directory", dir) - } - root, err := os.OpenRoot(dir) - if err != nil { - return "", rec, failf(ReasonUnreadable, "destination: %v", err) - } - defer func() { _ = root.Close() }() - if opened, serr := root.Stat("."); serr != nil || !os.SameFile(before, opened) { - return "", rec, failf(ReasonUnreadable, "destination %q changed while it was opened", dir) - } +func (r Reader) write(root *os.Root, dir, rel string, rec Record) (string, Record, error) { if d := filepath.Dir(rel); d != "." { if err := root.MkdirAll(d, 0o700); err != nil { return "", rec, failf(ReasonUnreadable, "destination: %v", err) diff --git a/free/ci/internal/authz/dest.go b/free/ci/internal/authz/dest.go index df016a7b5..f29c40ef9 100644 --- a/free/ci/internal/authz/dest.go +++ b/free/ci/internal/authz/dest.go @@ -4,11 +4,9 @@ package authz // directory that is not the working directory and not under a .git directory. // Inputs: the requested destination and the caller's working directory. // Outputs: the absolute, symlink-resolved destination, or a *PathError (E683). -// Constraints: the only I/O in this package (stat, read the directory, resolve -// symlinks). It creates nothing: the caller creates a missing destination with -// os.Mkdir (not MkdirAll) mode 0700, which fails if anything appeared in the -// meantime, and passes the returned path on. A leaf that is a symlink is -// refused, so the check and the write name the same directory. +// Constraints: this function only validates a pathname. Extraction callers +// must use OpenArtifactGetDest and write through its pinned os.Root; a returned +// pathname alone cannot protect against concurrent parent replacement. import ( "errors" diff --git a/free/ci/internal/authz/dest_open.go b/free/ci/internal/authz/dest_open.go new file mode 100644 index 000000000..4027f429d --- /dev/null +++ b/free/ci/internal/authz/dest_open.go @@ -0,0 +1,76 @@ +package authz + +import ( + "errors" + "io" + "os" + "path/filepath" +) + +// OpenArtifactGetDest validates and opens an empty destination for extraction. +// A missing leaf is created with mode 0700 through its pinned parent. Callers +// must write through the returned root and close it; the path is for display +// only. Renaming a parent after this call cannot redirect extraction. +func OpenArtifactGetDest(dest, cwd string) (*os.Root, string, error) { + if dest == "" || !filepath.IsAbs(cwd) { + return nil, "", reject(DestEmpty, dest) + } + requested := dest + if !filepath.IsAbs(requested) { + requested = filepath.Join(cwd, requested) + } + requested = filepath.Clean(requested) + // Capture identities before validation, so replacement between validation + // and opening cannot turn an approved pathname into a different directory. + parentInfo, err := os.Stat(filepath.Dir(requested)) + if err != nil { + return nil, "", reject(DestNoParent, dest) + } + leafInfo, leafErr := os.Lstat(requested) + if leafErr != nil && !errors.Is(leafErr, os.ErrNotExist) { + return nil, "", reject(DestUnreachable, dest) + } + resolved, err := ArtifactGetDest(dest, cwd) + if err != nil { + return nil, "", err + } + parent, err := os.OpenRoot(filepath.Dir(resolved)) + if err != nil { + return nil, "", reject(DestUnreachable, dest) + } + defer parent.Close() + if opened, err := parent.Stat("."); err != nil || !os.SameFile(parentInfo, opened) { + return nil, "", reject(DestUnreachable, dest) + } + leaf := filepath.Base(resolved) + if leafErr != nil { + if err := parent.Mkdir(leaf, 0o700); err != nil { + return nil, "", reject(DestUnreachable, dest) + } + leafInfo, err = parent.Lstat(leaf) + if err != nil { + return nil, "", reject(DestUnreachable, dest) + } + } + if !leafInfo.IsDir() { + return nil, "", reject(DestNotDir, dest) + } + root, err := parent.OpenRoot(leaf) + if err != nil { + return nil, "", reject(DestUnreachable, dest) + } + fail := func(reason PathReason) (*os.Root, string, error) { root.Close(); return nil, "", reject(reason, dest) } + if opened, err := root.Stat("."); err != nil || !os.SameFile(leafInfo, opened) { + return fail(DestUnreachable) + } + dir, err := root.Open(".") + if err != nil { + return fail(DestUnreachable) + } + names, err := dir.Readdirnames(1) + dir.Close() + if len(names) != 0 || (err != nil && !errors.Is(err, io.EOF)) { + return fail(DestNotEmpty) + } + return root, resolved, nil +} diff --git a/free/ci/internal/authz/dest_open_test.go b/free/ci/internal/authz/dest_open_test.go new file mode 100644 index 000000000..0cff02b3f --- /dev/null +++ b/free/ci/internal/authz/dest_open_test.go @@ -0,0 +1,73 @@ +package authz_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/nself-org/plugins/free/ci/internal/authz" +) + +func TestOpenArtifactGetDestPinsParentAndLeaf(t *testing.T) { + for _, existing := range []bool{false, true} { + t.Run(map[bool]string{false: "new", true: "existing"}[existing], func(t *testing.T) { + base := scratch(t) + cwd := filepath.Join(base, "work") + parent := filepath.Join(base, "parent") + outside := filepath.Join(base, "outside") + for _, p := range []string{cwd, parent, outside} { + mustMkdir(t, p) + } + dest := filepath.Join(parent, "out") + if existing { + mustMkdir(t, dest) + } + root, _, err := authz.OpenArtifactGetDest(dest, cwd) + if err != nil { + t.Fatal(err) + } + defer root.Close() + moved := filepath.Join(base, "moved") + if err := os.Rename(parent, moved); err != nil { + t.Fatal(err) + } + mustMkdir(t, filepath.Join(outside, "out")) + if err := os.Symlink(outside, parent); err != nil { + t.Fatal(err) + } + file, err := root.OpenFile("artifact", os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + t.Fatal(err) + } + file.Close() + if _, err := os.Stat(filepath.Join(outside, "out", "artifact")); !os.IsNotExist(err) { + t.Fatalf("write escaped to replacement parent: %v", err) + } + if _, err := os.Stat(filepath.Join(moved, "out", "artifact")); err != nil { + t.Fatal(err) + } + }) + } +} + +func TestOpenArtifactGetDestRefusesNonemptyAndSymlink(t *testing.T) { + base := scratch(t) + cwd := filepath.Join(base, "work") + mustMkdir(t, cwd) + full := filepath.Join(base, "full") + mustMkdir(t, full) + mustWrite(t, filepath.Join(full, "artifact")) + link := filepath.Join(base, "link") + if err := os.Symlink(full, link); err != nil { + t.Fatal(err) + } + for _, dest := range []string{full, link, cwd, filepath.Join(base, ".git", "out")} { + root, _, err := authz.OpenArtifactGetDest(dest, cwd) + if root != nil { + root.Close() + } + if err == nil { + t.Fatalf("accepted %s", dest) + } + } +}