fix: create the peer's configuration files where nothing else can reach them by ausimian · Pull Request #19 · ausimian/castle

ausimian · 2026-08-22T05:30:08Z

Refs #13 — the carried finding from step 1, and the first task of step 2. Targets
release/1.0.0.

Step 1 merged with one known [high] outstanding, deliberately and with the owner's
agreement, recorded in
this comment and on
PR #18. This is it, landing before
forecastle#6 — the change that makes the affected path reachable for the first time.

The defect

create_private/1 called File.write(path, "") and then chmodded to 0600.
File.write/2 creates the inode with the process umask, so the file briefly existed at
0644. Since chmod does not revoke an already-open descriptor, a local process
that opened it in that window kept a readable one and saw everything written afterwards
— configuration and serialised provider state. A persistent read channel, not a
momentary blink.

Demonstrated rather than argued: File.write! on a planted symlink writes through to
the target — a file holding "important" became "leaked" — and the following chmod
set the target's mode.

Three mechanisms, in the order they were needed

OTP has no create-with-mode, and this is worth stating precisely because the obvious
remedy looks available and is not: :file.open/2's mode list says nothing about
permissions, and {:mode, 0o600} is silently accepted and ignored — it returns
{:ok, pid} and produces 0644. So:

1. A working directory. Both files are created inside
releases/<vsn>/castle-<pid>-<n>.work, mkdir'd and chmodded 0700. A directory works
where a file did not because traversal permission is re-checked on every lookup rather
than captured at open, so a descriptor taken during its own window grants nothing
afterwards.

2. Verify, don't reason. The first version of that argued the window was harmless
because it covered an empty directory. Empty is safe to read; it is not safe to
write into — under a umask of 0002 the directory is briefly group-writable, and a
symlink planted at the predictable child path would be followed. So after the chmod the
directory must still be empty, or it is discarded and refused. And every child is
created with an exclusive open, so a planted name is refused rather than followed.

3. Write through the handle. The exclusive open establishes that the name was free —
and closing it to reopen by name throws that away. Content is now written through the
handle that proved the name free, with create_private/1 removed entirely so there is no
longer a way to bring one of these files into existence without also placing its content.

chmod still goes by path, because OTP has no fchmod — measured: change_mode with a
handle is :badarg, write_file_info is :function_clause. That asymmetry is
deliberate and documented: by then the content is committed to the inode this call
created, so a swapped name gets narrowed, not filled. A nuisance inside a directory
already verified empty and made 0700, and the test pins that cost rather than only
claiming it.

Why the scratch keeps its mode until last

It is written twice more after Castle creates it — by Elixir's own
Config.Provider.write_config! in the peer's VM, then by this module again — and both
reopen by name. That is where the 0440 case actually bites: a read-only mode applied
early leaves a file that cannot be reopened for writing.

Holding the creation handle open across the peer's run would not extend the
guarantee, and the comment says why it must not be tried: the peer writes by name in a
VM of its own, so a retained handle would point at whichever inode the name had at open
time. Elixir truncates the same inode today; one that wrote a temp file and renamed would
leave the handle on an orphan, and the resolved configuration would be written to nobody.
Those two writes rest on the directory instead.

Tests

84, up from 77. The discriminating ones observe intermediate states, because every
finding in this class turned on an end-state assertion being unable to see a transient
one — the mode always ended correct.

the working directory is refused when something is written into it between the mkdir
and the chmod, with the planted file's target left intact;
a planted name is refused rather than written through — against the previous commit
this fails with the disclosure visible;
content goes through the handle: the name is swapped for a symlink to a decoy after the
exclusive open, and the decoy still holds the interloper's own content afterwards;
File.mkdir/1 refuses an existing name in all four shapes, including a dangling
symlink;
a 0440 sys.config still materialises, twice over.

Where a test passes against the previous commit too, or discriminates only because a
function did not exist there, it is labelled as such rather than presented as proof.

No `RELEASE.md` entry

Deliberate. The path this protects has never been reachable by any release — every
release Forecastle assembles today has build.config and takes the old generate/1
path — so there is nothing an operator can observe. The unreleased "Added" entry already
states that the copies carry sys.config's permissions.

Review

Three rounds. The first found the directory had inherited the same create-then-narrow
race it was introduced to fix; the second found the exclusive open's guarantee being
discarded by a reopen. An ACL-based residual was raised and, on the owner's instruction,
is not addressed and not documented.

create_private/1 wrote the file and then chmodded it, so with a typical 0022 umask it existed as 0644 for the window in between - and chmod does not revoke a descriptor somebody already holds. A local process that opened the path in that window kept reading everything written to it afterwards: the release's configuration and its serialised provider state, credentials included. A standing read channel, not a blink, on both files this path creates. It cannot be fixed at the file. OTP has no open that takes a creation mode: kernel's own mode() type in file.erl is the whole list of what :file.open/2 accepts, none of it about permissions, and an unrecognised option is ignored rather than refused - measured, {:mode, 0o600} is accepted and the inode still arrives 0644 under umask 0022, raw and cooked, exclusive or not. So a mode can only be applied to a file that already exists, and applying one is exactly what does not help. So the protection is a directory. work_dir/1 mkdirs one 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 narrowed after the fact just as a file was - 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. That asymmetry is the whole reason this works on a directory and could not be made to work on the file, and the comment says so, because the difference is not obvious and the next person will otherwise read mkdir-then-chmod as the same mistake. write_private/2 now refuses to create a file in a directory that grants anything to group or other, rather than trusting its caller to have picked a path inside the working directory. Four rounds of this class went wrong by reasoning about one call site at a time, so the invariant is in the primitive; plan/2 is the one place the two paths are decided, and it puts both inside the directory under the names they will have when they leave. The check also catches a filesystem that took the mkdir and ignored the chmod, where nothing here can be honoured and the operator's own mode on sys.config would not be honoured either. 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 that release is compromised before Castle is asked to configure it. The three things that had to keep working, do. The scratch is still filled three times - here, by the peer's pipeline, here again - so it holds 0600 for its working life and takes sys.config's mode immediately before the rename; Elixir writes it with File.write/2 over the same inode, which is what makes that ordering enough. A sys.config at 0440 still materialises, twice over, and both files still end at 0440. And an orphaned working directory is left exactly where it is, for the reason an orphaned staging file is: an install cannot tell its own leftovers from another install's work in progress. What it does remove is the directory it made itself, on every way out, which is now the one cleanup point - the two per-file removals are gone with it. The file's own 0600 stays as belt to the directory's braces. It is no longer what closes the window, and the comment says that rather than leaving it looking load-bearing. The test that discriminates walks the version directory from inside the peer, while the staged base and the file being resolved into both exist and both hold configuration - the only moment at which any of this can be observed, since by the time materialisation returns both have been moved to their final names or removed, and the end state is identical either way. It finds one thing of Castle's there: a directory at 0700, with both files inside it. Against 37ae759 there is no such directory and the files are loose beside sys.config, so it fails there, as do the refusal-to-create test and the in-peer mode test whose glob now insists the scratch is one level down. The tests that make the primitives write inside a working directory fail against 37ae759 only because work_dir/1 does not exist there; the orphaned-directory test and the read-only release pass against it too, and are regression guards rather than discriminators. No RELEASE.md entry. The path is unreachable by any release - every release Forecastle assembles today has build.config and is materialised the old way - so nothing an operator can observe changes, and the unreleased entry describing this mechanism already says the copies carry sys.config's permissions. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

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

write_private/2 opened the file exclusively, closed the handle, and then reopened the same name with File.write/2 to put the content in. The exclusive open is what establishes that the name did not exist, and closing it to reopen by path gives that proof back: anything able to create the name in between is handed the configuration, which is the disclosure the exclusive open was added to prevent. So the bytes now go through the handle the open returned, and the name is never reopened to place content. The content can only reach the inode this call created, whatever becomes of the name afterwards. Creation and filling are one movement for the same reason - there is no longer a way here to bring one of these files into existence without also placing its content, because separating the two is what let the reopen back in, so create_private/1 is gone and write_private/2 is what call sites use. chmod is now the only step that goes by path, and it has to: OTP has nothing that sets a mode on an open file. :file.change_mode/2 rejects a handle with badarg and :file.write_file_info/2 with function_clause, and those, with raw_write_file_info/2, are the whole of what :file offers. The asymmetry is accepted rather than overlooked. By the time the chmod runs the content is already committed to this call's inode, so a name swapped underneath it does not receive the configuration - it gets narrowed, which is Castle setting some other file to 0600 inside a directory it verified empty and made 0700. A nuisance, not a disclosure. The test asserts both halves of that, so the cost is pinned rather than merely asserted in a comment. The mode still goes on after the content rather than before, and the reason is now stated properly: it is about the writes that come later. The scratch is written twice more after Castle creates it - the peer's pipeline writes the resolved configuration over it, then this module writes it again - and both reopen the name, so a 0440 model applied any earlier would leave a file that cannot be reopened for writing. One of those two is Config.Provider.write_config!, which is File.write/2 in Elixir's own code, running in the peer's VM. Driving Elixir's pipeline is what this module exists for, so that is not ours to change and is not worked around. Holding the creation handle open across the peer's run would not cover it either, and the comment says why it must not be tried: the peer writes by name in a VM of its own, and a handle kept here would point at whichever inode the name had when it was opened. Elixir truncates the same inode today; one that wrote a temporary file and renamed it would leave the handle on an orphan and the configuration written through it would silently be nobody's. Those two writes rest on the directory - verified empty, 0700, holding only names this call created - and that is what the directory is for. The test stands between the exclusive open and the write, which is why create_exclusive/1 and fill/3 are public: the name is swapped for a symlink to a decoy in between, and the configuration is found in neither the decoy nor anywhere the name leads. With the name left alone the two behaviours are identical, so there is no other way to tell content that went to the inode from content that went to the name. It fails against ad68d0e because the handle it writes through does not exist there - write_private/2 had no seam to stand in, which is the same shape of discrimination as secure_dir/1 last time and worth saying plainly rather than dressing up. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

ausimian · 2026-08-22T05:30:20Z

@codex review

chatgpt-codex-connector · 2026-08-22T05:33:58Z

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: a8157d6c29

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Elixir 1.20's type checker refused the error clause in written/3 as unreachable, failing mix compile --warnings-as-errors on all three 1.20 cells while 1.18 and 1.19 stayed green. It was right, and deleting the clause would have been the wrong way to agree with it. IO.binwrite/2 is specified to return :ok, and it earns that spec: its body calls :file.write/2 and raises whatever error comes back (:erlang.error/1, io.ex:308). So the clause really was dead, and what would have happened on a failed write was not an unreported error but an ErlangError leaving materialise/2 - past every caller written to expect {:error, message}, in a module whose whole discipline is that it reports rather than raises because it runs ahead of install_release/1 where an exception is a silent abort. The checker found a real defect, not a style violation. The right call was there all along. The handle is opened :raw, which makes it a :file_descriptor record rather than an io device - the IO functions accept one, but they are not for it - and :file.write/2 is what a raw handle takes. It returns the error instead of raising it: {:error, :ebadf} writing to a handle that cannot be written, {:error, :einval} to one already closed. Measured under both 1.19.5 and 1.20.0 rather than assumed, since an error branch that cannot be reached is what caused this. Audited the rest for the same shape. Two IO references in the project, both on standard output or standard error where an io device is what is wanted and nothing matches on the return - Castle.report!/1 and one test - and one raw handle in the codebase, the one this changes. Nothing else. Verified against the version that failed rather than only the one installed here: mise had 1.20.0-otp-28, so the failing cell's steps were run locally against a separate build root. The old call reproduces CI's warning exactly, at Castle.Peer.written/3; with this change mix compile --warnings-as-errors is clean and mix test passes 84 with no warnings from the test tree either. The other two red cells are 1.20 on OTP 27 and 29, which are not installed here, but the checker's judgement is Elixir's and not OTP's. Refs: #13 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

ausimian and others added 3 commits August 22, 2026 14:54

ausimian merged commit 3ec5979 into release/1.0.0 Aug 22, 2026
7 checks passed

ausimian deleted the issue/13-private-dir branch August 22, 2026 05:47

Navigation Menu

fix: create the peer's configuration files where nothing else can reach them - #19

fix: create the peer's configuration files where nothing else can reach them#19
ausimian merged 4 commits into
release/1.0.0from
issue/13-private-dir

ausimian commented Aug 22, 2026

Uh oh!

ausimian commented Aug 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ausimian commented Aug 22, 2026

The defect

Three mechanisms, in the order they were needed

Why the scratch keeps its mode until last

Tests

No RELEASE.md entry

Review

Uh oh!

ausimian commented Aug 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

No `RELEASE.md` entry