fix: verify the working directory rather than trust the window

The last commit moved the race from a file inode to a directory inode.
mkdir creates 0777 against the umask, so under 0002 - ordinary - or 0000
the new directory is briefly writable by others, and nothing checked
that it was still empty when the chmod landed. The child names are
predictable, and create_private/1 used File.write/2, which follows a
symlink: an interloper plants sys.config pointing at a file it can read,
keeps a descriptor on that file, and receives the configuration written
through it. Measured, against the previous commit: the target is
truncated, chmodded to 0600 and filled.

The justification was the flaw, not the mechanism. It said the window
was harmless because it covered an empty directory, and offered that
traversal is checked on every lookup rather than captured in a
descriptor. Both halves are true and both are about reading. An empty
directory is safe to read. It is not safe to write into, and the attack
wants a name inside it, not a handle on it.

So work_dir/1 now creates, narrows, and then verifies: File.ls must come
back empty after the chmod, or the directory is removed and the install
refused with the entry named and the umask pointed at. Verification is
the point rather than the belt to a braces - it does not depend on any
judgement about whether a window is harmless, which is the judgement
that has now been wrong five times running in this module. AGENTS.md
says that as the rule, and preserves the argument it replaced so nobody
reconstructs it from first principles.

create_private/1 creates exclusively. This is a different use of
:exclusive from the one measured two commits ago and the comment keeps
them apart: there it was asked to settle the mode and does not, here it
is asked to refuse a name that is already there and does - eexist for a
regular file, a symlink, and a symlink to nothing, the last without
bringing its target into existence. Exclusivity says nothing about the
permissions an inode arrives with, and a private directory says nothing
about a name already inside it, so neither substitutes for the other.

Which makes write_private/2 single-use per path. Both call sites already
were; the one test that wrote a path twice was testing the mode, and now
uses two.

Audited the rest of the module for predictable names it creates. The
working directory's own name is refused if taken, and File.mkdir/1 is
what refuses it - measured against a directory, a regular file, a
symlink to a directory and a dangling symlink, all eexist, since
{:mode, _} being silently ignored on open was the same category of
surprise. sys.config is replaced by File.rename/2, which replaces a
symlink at that name rather than writing through it, target untouched.
The base is published by File.ln/2, which refuses eexist against all
three shapes and creates nothing from a dangling one. What follows an
eexist there is a read of whatever holds the name, so a symlink planted
at sys.config.pristine would be read through - but planting it needs
write permission on the version directory, and an account with that can
write its own configuration into the file directly. That is the boundary
already documented, not a new hole. All recorded in the module comment.

The test that discriminates stands in the mkdir-to-chmod window, which
is why secure_dir/1 is public: a directory at 0777 with a sys.config
symlink planted in it is refused, removed, and what the symlink pointed
at is neither truncated nor written through - File.rm_rf/1 unlinks a
symlink rather than following it, also measured. Its companion plants a
name inside an already-private directory and asserts write_private/2
refuses rather than writing through it, which is the half the directory
does not cover; against 1f726fc that one returns :ok, follows the link
and hands over the file. Both fail against 1f726fc, the second with the
disclosure visible.

Refs: #13

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

Author: ausimian

AGENTS.md

@@ -93,15 +93,42 @@ Castle's job is configuration and release management on a running node.
   closes it. Do not go looking for an atomic create-with-mode; there isn't one.
 
   What works is `Castle.Peer.work_dir/1`: `mkdir` a directory in the version
-  directory and chmod it 0700 **while it is still empty**. `mkdir` takes no mode
-  either, so the directory is narrowed after the fact exactly as a file would
-  be — but an empty directory has nothing behind the window, and permission to
-  traverse a directory is checked on every lookup rather than captured at open
-  the way permission to read a file is, so a descriptor taken on it during that
-  window grants nothing once the chmod has happened. That asymmetry is the whole
-  reason this works on a directory and could not be made to work on the file.
-  `plan/2` is the one place the two paths are decided, and it puts both inside
-  the working directory under the names they will have when they leave.
+  directory, chmod it 0700, and then **check that it is still empty**, refusing
+  and removing it if it is not. `plan/2` is the one place the two file paths are
+  decided, and it puts both inside that directory under the names they will have
+  when they leave. Each is created **exclusively**, so a name already at the
+  path is refused rather than followed or truncated.
+
+  **The rule is create, narrow, then verify.** The verification is not
+  decoration and it is not defence in depth: it is there because five successive
+  attempts to reason about whether a window was harmless were all wrong, and a
+  sixth judgement of the same kind is worth nothing. Do not remove it on the
+  grounds that you can see why the window is safe. That is precisely the
+  sentence that preceded each of the previous five findings.
+
+  The particular argument it replaced, so nobody reconstructs it: an empty
+  directory has nothing behind the window, and permission to traverse a
+  directory is checked on every lookup rather than captured at open the way
+  permission to read a file is, so a stale directory descriptor grants nothing
+  once the chmod has happened. Both halves are true, and both are about
+  *reading*. A directory the umask left group-writable — 0002 is an ordinary
+  umask and 0000 exists — can be written *into* during that window, and the child
+  names are predictable, so an interloper needs no descriptor at all: it plants
+  `sys.config` as a symlink to a file it can read and waits for the
+  configuration to arrive through it. Empty is safe to read. It is not safe to
+  write into.
+
+  Exclusivity and privacy are separate properties and neither substitutes for
+  the other. `:exclusive` on the open says nothing about the permissions the
+  inode arrives with, which is why it is no answer to the paragraph above — that
+  was measured, and `{:mode, _}` alongside it is silently ignored. A private
+  directory says nothing about what a name already inside it would do, which is
+  why it is no answer to a planted symlink: `File.write/2` follows one, truncates
+  what it points at, chmods *that* to 0600, fills it with the configuration, and
+  creates the target outright if the link dangles. All measured, all refused by
+  `:exclusive`, which returns `:eexist` for a regular file, a symlink and a
+  dangling symlink alike. `File.mkdir/1` refuses all three too, which is what
+  stops the working directory's own name being taken first.
 
   It does not defend a version directory other accounts can write to: whoever
   can create a name there can replace `sys.config` itself, so that release is
@@ -112,11 +139,11 @@ Castle's job is configuration and release management on a running node.
   anything to group or other, rather than trusting the caller to have picked a
   path inside the working directory — the invariant is in the primitive because
   remembering it at the call sites is what failed, four times over. It is a
-  guard against the next call site and not against an attacker (a directory can
-  be chmodded between the check and the create), and it also catches a
-  filesystem that took the `mkdir` and ignored the `chmod`, where none of this
-  can be honoured and the operator's own mode on `sys.config` would not be
-  either.
+  guard against the next call site rather than against an attacker — a directory
+  can be chmodded between the check and the create, which is the gap the
+  exclusive create covers — and it also catches a filesystem that took the
+  `mkdir` and ignored the `chmod`, where none of this can be honoured and the
+  operator's own mode on `sys.config` would not be either.
 
   Inside the directory the file is still created owner-only at 0600, filled
   through that, and given the model's mode **last** — `write_like/3` is the two
@@ -352,16 +379,24 @@ commit — and the other once with the environment as it ended up. The two
 `sys.config` terms have to be equal. That is why `Castle.SyntheticRelease` makes
 its symlinks idempotently: a root has to be able to hold two versions.
 
-`Castle.Peer.work_dir/1`, `write_like/3`, `write_private/2`, `create_private/1`
-and `publish/2` are public for the same kind of reason: what they guarantee is
-about *intermediate* states, and a window nothing can stand in is a window
-nothing can test. One test takes `work_dir/1`, `write_like/3` and `publish/2` one
-at a time and looks at the destination in between — where it finds no file, rather
-than a partial one — then checks that publishing again is refused rather than
-allowed to replace. Another calls `work_dir/1` and finds the directory already at
-0700 **and still empty**, which is the pair of facts that makes its own window
-harmless. A third calls `write_private/2` with a path in a directory the host can
-traverse and gets a refusal.
+`Castle.Peer.work_dir/1`, `secure_dir/1`, `write_like/3`, `write_private/2`,
+`create_private/1` and `publish/2` are public for the same kind of reason: what
+they guarantee is about *intermediate* states, and a window nothing can stand in
+is a window nothing can test. One test takes `work_dir/1`, `write_like/3` and
+`publish/2` one at a time and looks at the destination in between — where it finds
+no file, rather than a partial one — then checks that publishing again is refused
+rather than allowed to replace. Another calls `work_dir/1` and finds the
+directory at 0700 and still empty.
+
+`secure_dir/1` is public so that the `mkdir`-to-`chmod` window can be stood in:
+a test creates a directory at 0777, plants a `sys.config` symlink inside it, and
+asserts that securing it is refused, the directory removed and what the symlink
+pointed at untouched. That is the only way to observe it — nothing about the end
+state distinguishes a directory that was empty when it was narrowed from one that
+was not, which is what made the same mistake possible a sixth time at the
+directory after five at the file. A companion test plants a name inside an
+already-private directory and asserts `write_private/2` refuses it rather than
+writing through it, which is the half a private directory does not cover.
 
 Those have to be written that way. The mode a file *ends up* with is the same
 whether it was set before or after the content, so a test of the end state

lib/castle/peer.ex

@@ -599,23 +599,55 @@ defmodule Castle.Peer do
   # momentary blink. Narrowing after creation is not a window that can be made
   # small enough. It has to be a path no one else can reach.
   #
-  # So `work_dir/1` makes a working directory in the version directory and
-  # chmods it 0700 *while it is still empty*, and both files are created inside
-  # it and moved out. `mkdir` takes no mode either, so the directory is created
-  # against the umask and narrowed just as a file would be - but an empty
-  # directory exposes nothing to see, and permission to traverse a directory is
-  # checked on every lookup rather than captured in a descriptor, so a handle
-  # taken on it during that window grants nothing once the chmod has happened.
-  # That is the whole difference between doing this to a directory and doing it
-  # to a file, and it is why the file-level version of this could not be made
-  # airtight however carefully it was written.
+  # So `work_dir/1` makes a working directory in the version directory, chmods it
+  # 0700, and then checks that it is *still empty*. Both files are created inside
+  # it, exclusively, and moved out.
+  #
+  # The check is the part that matters, and it is here because the reasoning it
+  # replaces was wrong. `mkdir` takes no mode either, so the directory is created
+  # against the umask and narrowed afterwards exactly as a file would be, and the
+  # argument for why that was harmless went: an empty directory has nothing in it
+  # to read, and permission to traverse a directory is checked on every lookup
+  # rather than captured in a descriptor, so a handle taken on it during the
+  # window grants nothing once the chmod has happened. Both halves are true. Both
+  # are about *reading*. A directory the umask left group-writable - 0002 is an
+  # ordinary umask and 0000 exists - can be written *into* during that window,
+  # and the names inside it are predictable, so an interloper needs no descriptor
+  # at all: it plants `sys.config` as a symlink to a file it can read, and waits
+  # for the configuration to be written through it.
+  #
+  # Hence create, narrow, then *verify*: a directory that gained an entry before
+  # it was narrowed is removed and the install refused. Verifying does not depend
+  # on the window being harmless, which is the judgement that has now been wrong
+  # five times running in this module.
+  #
+  # Each child is then created exclusively, which refuses rather than follows or
+  # truncates a name that is already there - a regular file, a symlink, or a
+  # symlink to nothing, all `:eexist`, and the last of those without creating
+  # what it pointed at. That is a different property from the mode, and the two
+  # do not substitute for each other: exclusivity says nothing about the
+  # permissions an inode arrives with, so it is no answer to the paragraph above,
+  # and a private directory says nothing about what a name already inside it
+  # would do, so it is no answer to this one.
   #
   # What it does not defend is a version directory other accounts can write to:
   # whoever can create a name there can replace `sys.config` itself, so such a
   # release is compromised before Castle is asked to configure it. The case being
   # defended is the ordinary one, a version directory anyone may traverse and
   # read.
   #
+  # The other two names this module brings into existence are in the version
+  # directory, and neither can be captured by a name planted at it. `sys.config`
+  # is replaced by `File.rename/2`, which replaces a symlink sitting at that name
+  # with the file rather than writing through it - measured, and the link's target
+  # is left untouched. The base is published by `File.ln/2`, which refuses
+  # `:eexist` against a file, a symlink, or a symlink to nothing, and creates
+  # nothing in the last case. What follows an `:eexist` there is a *read* of
+  # whatever holds the name, so a planted symlink at `sys.config.pristine` would
+  # be read through - but planting it needs write permission on the version
+  # directory, and an account with that could put its own configuration in that
+  # file directly. It is the case above, not a separate one.
+  #
   # `write_private/2` is the one way to bring one of these files into existence,
   # and it refuses to create one in a directory that grants anything to group or
   # other rather than trusting its caller to have picked a path inside the
@@ -663,32 +695,63 @@ defmodule Castle.Peer do
   # after it was filled looks exactly like one secured before, and a window
   # nothing can stand in is a window nothing can test.
 
-  # Created rather than ensured: a name that is already there is not adopted,
-  # whether it is a directory, a file or a symlink to somewhere else - `mkdir`
-  # refuses all three, and the name carries this process's pid and a number no
-  # other call in it will use again. Chmodded straight afterwards, while the only
-  # thing an interloper could reach through it is the fact that it is empty.
-  #
-  # A directory that could not be secured is removed again rather than used: it
-  # is this call's own, made an instant ago and with nothing in it, so removing it
-  # cannot take anybody else's work with it.
+  # Created rather than ensured: a name that is already there is not adopted, and
+  # `File.mkdir/1` is what refuses it - measured against a directory, a regular
+  # file, a symlink to a directory and a symlink to nothing, all `:eexist`. The
+  # name carries this process's pid and a number no other call in it will use
+  # again, so there is nothing to guess in time either way.
   @doc false
   @spec work_dir(Path.t()) :: {:ok, Path.t()} | {:error, String.t()}
   def work_dir(dir) do
     path = Path.join(dir, "castle-#{System.pid()}-#{unique()}.work")
 
-    with :ok <- mkdir(path) do
-      case chmod(path, 0o700) do
-        :ok ->
-          {:ok, path}
+    with :ok <- mkdir(path), :ok <- secure_dir(path), do: {:ok, path}
+  end
 
-        {:error, _reason} = error ->
-          discard(path)
-          error
-      end
+  # Narrowed and then checked, and removed rather than used if either fails. What
+  # the check catches is an entry that appeared between the `mkdir` and the
+  # `chmod`, which is possible for exactly as long as the umask left the new
+  # directory writable by anyone else - and is worth catching whatever put it
+  # there, since nothing of Castle's is written until afterwards.
+  #
+  # Removing it cannot take anyone else's work with it. It is this call's own,
+  # made an instant ago, and `File.rm_rf/1` unlinks a symlink rather than
+  # following it, so a planted name goes and what it pointed at stays.
+  @doc false
+  @spec secure_dir(Path.t()) :: :ok | {:error, String.t()}
+  def secure_dir(path) do
+    case narrowed_and_empty(path) do
+      :ok ->
+        :ok
+
+      {:error, _reason} = error ->
+        discard(path)
+        error
     end
   end
 
+  defp narrowed_and_empty(path) do
+    with :ok <- chmod(path, 0o700), do: empty(path)
+  end
+
+  defp empty(path) do
+    case File.ls(path) do
+      {:ok, []} -> :ok
+      {:ok, entries} -> {:error, occupied(path, entries)}
+      {:error, reason} -> {:error, "Cannot list #{path}. #{format_error(reason)}"}
+    end
+  end
+
+  defp occupied(path, entries) do
+    "Cannot assemble configuration in #{path}. Castle had just created that " <>
+      "directory and written nothing to it, and it already holds " <>
+      "#{Enum.join(entries, ", ")} - so something else can write where this " <>
+      "release's configuration is about to be, and a name planted there is a name " <>
+      "the configuration could be written through. Nothing has been written and " <>
+      "the directory has been removed. Check the umask the release runs under: a " <>
+      "new directory has to be private to the account doing the install."
+  end
+
   @doc false
   @spec write_like(Path.t(), iodata(), Path.t()) :: :ok | {:error, String.t()}
   def write_like(path, bytes, model) do
@@ -704,10 +767,35 @@ defmodule Castle.Peer do
     end
   end
 
+  # Exclusively, so that a name already at this path is refused rather than
+  # followed or truncated. `File.write/2` here would do neither safely: pointed at
+  # a symlink it truncates what the link points at, chmods *that* to 0600, and
+  # fills it with the configuration - and creates the target outright if the link
+  # dangles. All three measured. The mode still goes on afterwards, because there
+  # is still no way to ask for it at creation.
   @doc false
   @spec create_private(Path.t()) :: :ok | {:error, String.t()}
   def create_private(path) do
-    with :ok <- write(path, ""), do: chmod(path, 0o600)
+    with :ok <- create_exclusive(path), do: chmod(path, 0o600)
+  end
+
+  defp create_exclusive(path) do
+    case File.open(path, [:write, :exclusive, :raw]) do
+      {:ok, handle} -> close(path, handle)
+      {:error, reason} -> {:error, "Cannot create #{path}. #{format_error(reason)}"}
+    end
+  end
+
+  # Closed rather than written through, because what is written to these files is
+  # written more than once and by more than this module - the peer's own pipeline
+  # writes the scratch too - so no handle could serve all of it. Nothing can reach
+  # the path between the close and the write: the directory holding it has been
+  # verified private.
+  defp close(path, handle) do
+    case File.close(handle) do
+      :ok -> :ok
+      {:error, reason} -> {:error, "Cannot create #{path}. #{format_error(reason)}"}
+    end
   end
 
   @doc false