From 90fd5880d4afdb5ba3d360b504a7ba2d587f90a9 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Date: Tue, 15 Sep 2026 11:37:06 -0400 Subject: [PATCH 1/2] Write prewarm secret files under the data root, not Dir.tmpdir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On macOS + colima the VM shares $HOME and /Users/Shared but not /var/folders (Dir.tmpdir), and docker turns a bind mount from an unshared host path into an empty directory — the prewarm container read an empty /run/secrets/WWISE_TOKEN and failed far from the cause. Secret temp files now live under /tmp, which is VM-visible on every supported layout. The dir is sticky world-writable (the data root is shared between the human and agent users); files stay 0600 and are deleted by the caller as before. Co-authored-by: Cursor --- lib/dev/build_container.rb | 18 ++++++++++++++++-- test/dev/build_container_test.rb | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/dev/build_container.rb b/lib/dev/build_container.rb index c099714..d3ceb00 100644 --- a/lib/dev/build_container.rb +++ b/lib/dev/build_container.rb @@ -2,9 +2,9 @@ # frozen_string_literal: true require "digest" +require "fileutils" require "pathname" require "securerandom" -require "tmpdir" require "yaml" require "dev/build_watcher" @@ -541,12 +541,26 @@ def run_watched(argv, container:) # Write each secret value to a private host temp file for bind-mounting into # the prewarm container. Returns {id => path}; caller deletes the files. # + # The files live under the data root, NOT Dir.tmpdir: on macOS + colima the + # VM shares $HOME and /Users/Shared but not /var/folders, and docker turns a + # bind mount from an unshared host path into an empty directory — the + # prewarm then reads an empty secret and fails far from the cause. The tmp + # dir is sticky world-writable (like /tmp) because the data root is shared + # between the human and agent users; each file itself is 0600. + # # @param secrets [Hash{String => String}] # @return [Hash{String => String}] secret id => temp file path sig { params(secrets: T::Hash[String, String]).returns(T::Hash[String, String]) } def write_secret_files(secrets) + dir = File.join(Dev::DataRoot.path, "tmp") + FileUtils.mkdir_p(dir) + begin + File.chmod(0o1777, dir) + rescue Errno::EPERM + # Another user owns the dir; it was created with these bits already. + end secrets.each_with_object({}) do |(id, value), files| - path = File.join(Dir.tmpdir, "dev-secret-#{SecureRandom.hex(8)}") + path = File.join(dir, "dev-secret-#{SecureRandom.hex(8)}") File.open(path, File::WRONLY | File::CREAT | File::EXCL, 0o600) { |f| f.write(value) } files[id] = path end diff --git a/test/dev/build_container_test.rb b/test/dev/build_container_test.rb index 7bb9a4c..6d38cca 100644 --- a/test/dev/build_container_test.rb +++ b/test/dev/build_container_test.rb @@ -1069,6 +1069,23 @@ def build_container(engine: FakeContainerEngine.new) files.each_value { |p| File.delete(p) if File.exist?(p) } end + test "write_secret_files places files under the data root, never Dir.tmpdir" do + Given "a container VM that shares the data root but not the host tmpdir" + # macOS + colima: the VM shares $HOME and /Users/Shared, NOT /var/folders + # (Dir.tmpdir). A bind mount from an unshared path silently mounts an empty + # directory, so the prewarm reads an empty secret and fails downstream. + + When "writing secret files" + files = build_container.write_secret_files({ "TOK" => "s3cr3t" }) + + Then "each file lives under the resolved data root" + files.values.all? { |p| p.start_with?(Dev::DataRoot.path) } + files.values.none? { |p| p.start_with?(Dir.tmpdir) } + + Cleanup + files.each_value { |p| File.delete(p) if File.exist?(p) } + end + test "service_container_name keys the name by image, workspace, and tag" do Given "a full image:tag and the checkout it runs in" root = Pathname("/work/snappy") From 2a0adc1948e1aca6828db41ec236331074ba18bc Mon Sep 17 00:00:00 2001 From: Jean-Philippe Date: Wed, 16 Sep 2026 11:31:05 -0400 Subject: [PATCH 2/2] secrets: verified per-uid dir instead of a shared sticky tmp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit finding on the data-root move: a shared 1777 dir's owner can unlink and replace anyone's secret files between write and bind mount (sticky only stops non-owners), and on an unprovisioned machine /Users/Shared is world-writable, so any local user can pre-own the tree. Replace the shared dir with secrets- directly under the data root (no world-writable parent that could rename it post-check), created 0700 and lstat-verified: a real directory, owned by the current uid, or SecretDirCompromisedError — never a fallback. Also sweep day-old dev-secret-* leftovers (SIGKILL strands them; macOS never purges the data root). Co-authored-by: Cursor --- lib/dev/build_container.rb | 69 ++++++++++++++++++--- test/dev/build_container_test.rb | 102 ++++++++++++++++++++++++++++--- 2 files changed, 154 insertions(+), 17 deletions(-) diff --git a/lib/dev/build_container.rb b/lib/dev/build_container.rb index d3ceb00..fe2996b 100644 --- a/lib/dev/build_container.rb +++ b/lib/dev/build_container.rb @@ -38,6 +38,12 @@ class BuildContainer # remote engine brings its sync strategy instead of reaching this raise. class LocalMountsUnsupportedError < RuntimeError; end + # The per-uid secrets dir under the data root failed verification (symlink, + # non-directory, or owned by another user). Writing 0600 secret files into + # a dir someone else controls lets its owner swap contents between file + # write and container bind mount, so this is a hard stop, never a fallback. + class SecretDirCompromisedError < RuntimeError; end + # Always-hashed inputs. deps.lock (app/test deps, e.g. SML) and build-deps.lock # (build deps, e.g. the engine) join the Dockerfile so a dependency bump # invalidates a prewarmed image. Missing files are skipped (see content_tag). @@ -544,21 +550,15 @@ def run_watched(argv, container:) # The files live under the data root, NOT Dir.tmpdir: on macOS + colima the # VM shares $HOME and /Users/Shared but not /var/folders, and docker turns a # bind mount from an unshared host path into an empty directory — the - # prewarm then reads an empty secret and fails far from the cause. The tmp - # dir is sticky world-writable (like /tmp) because the data root is shared - # between the human and agent users; each file itself is 0600. + # prewarm then reads an empty secret and fails far from the cause. # # @param secrets [Hash{String => String}] # @return [Hash{String => String}] secret id => temp file path + # @raise [SecretDirCompromisedError] when the per-uid dir fails verification sig { params(secrets: T::Hash[String, String]).returns(T::Hash[String, String]) } def write_secret_files(secrets) - dir = File.join(Dev::DataRoot.path, "tmp") - FileUtils.mkdir_p(dir) - begin - File.chmod(0o1777, dir) - rescue Errno::EPERM - # Another user owns the dir; it was created with these bits already. - end + dir = secrets_dir + sweep_stale_secrets(dir) secrets.each_with_object({}) do |(id, value), files| path = File.join(dir, "dev-secret-#{SecureRandom.hex(8)}") File.open(path, File::WRONLY | File::CREAT | File::EXCL, 0o600) { |f| f.write(value) } @@ -566,6 +566,55 @@ def write_secret_files(secrets) end end + # The per-identity secrets dir under the data root: secrets-, 0700, + # verified before use. Per-uid rather than a shared sticky-1777 dir because + # a shared dir's owner can unlink and replace anyone's files (sticky only + # stops non-owners) — a secret-substitution vector between file write and + # bind mount. Verification guards the unprovisioned-machine case: the data + # root's parent (/Users/Shared) ships world-writable, so any local user can + # pre-own the tree before provisioning; a pre-existing entry here is + # attacker-suspect until lstat proves it a real directory we own. The dir + # sits directly under the data root (not a shared tmp/) so no world-writable + # parent can rename a verified dir out from under us post-check. + # + # @return [String] verified dir path + # @raise [SecretDirCompromisedError] + sig { returns(String) } + def secrets_dir + dir = File.join(Dev::DataRoot.path, "secrets-#{Process.uid}") + begin + Dir.mkdir(dir, 0o700) + rescue Errno::EEXIST + # Pre-existing entry: verified below like everything else. + end + st = File.lstat(dir) + unless st.directory? && st.uid == Process.uid + raise SecretDirCompromisedError, + "#{dir} is not a directory owned by uid #{Process.uid} " \ + "(found #{st.directory? ? "dir" : "non-dir"} owned by uid #{st.uid}). " \ + "Refusing to write secrets there — remove it and re-run." + end + # Ours, but normalize the mode (a setgid data root propagates g+s on + # Linux; older dev versions never created this dir, so no legacy modes). + File.chmod(0o700, dir) if (st.mode & 0o7777) != 0o700 + dir + end + + # Delete day-old dev-secret-* leftovers. The caller's ensure covers normal + # failures, but SIGKILL strands 0600 files under the data root, which — + # unlike /var/folders — macOS never purges. A day's grace keeps concurrent + # runs' live files safe (they exist for minutes, not hours). + # + # @param dir [String] the verified per-uid secrets dir + sig { params(dir: String).void } + def sweep_stale_secrets(dir) + Dir.glob(File.join(dir, "dev-secret-*")).each do |path| + File.delete(path) if Time.now - File.mtime(path) > 86_400 + rescue Errno::ENOENT + # A concurrent sweep won the race; the file is gone either way. + end + end + # --- internal helpers ------------------------------------------------ # Unique name for the throwaway prewarm container; pid + random suffix so diff --git a/test/dev/build_container_test.rb b/test/dev/build_container_test.rb index 6d38cca..6d38d43 100644 --- a/test/dev/build_container_test.rb +++ b/test/dev/build_container_test.rb @@ -1058,6 +1058,11 @@ def build_container(engine: FakeContainerEngine.new) end test "write_secret_files writes each secret to a private temp file" do + Given "a scratch data root" + root = Dir.mktmpdir("bc-data-root-") + original = ENV["DEV_DATA_ROOT"] + ENV["DEV_DATA_ROOT"] = root + When "writing secret files" files = build_container.write_secret_files({ "TOK" => "s3cr3t" }) @@ -1066,24 +1071,107 @@ def build_container(engine: FakeContainerEngine.new) (File.stat(files["TOK"]).mode & 0o777) == 0o600 Cleanup - files.each_value { |p| File.delete(p) if File.exist?(p) } + ENV["DEV_DATA_ROOT"] = original + FileUtils.rm_rf(root) end - test "write_secret_files places files under the data root, never Dir.tmpdir" do - Given "a container VM that shares the data root but not the host tmpdir" + test "write_secret_files places files in a private per-uid dir under the data root, never Dir.tmpdir" do + Given "a scratch data root" # macOS + colima: the VM shares $HOME and /Users/Shared, NOT /var/folders # (Dir.tmpdir). A bind mount from an unshared path silently mounts an empty # directory, so the prewarm reads an empty secret and fails downstream. + root = Dir.mktmpdir("bc-data-root-") + original = ENV["DEV_DATA_ROOT"] + ENV["DEV_DATA_ROOT"] = root + + When "writing secret files" + files = build_container.write_secret_files({ "TOK" => "s3cr3t" }) + + Then "each file lives in secrets-, a real dir owned by us, mode 0700" + dir = File.join(root, "secrets-#{Process.uid}") + files.values.all? { |p| File.dirname(p) == dir } + st = File.lstat(dir) + st.directory? == true + st.uid == Process.uid + (st.mode & 0o7777) == 0o700 + + Cleanup + ENV["DEV_DATA_ROOT"] = original + FileUtils.rm_rf(root) + end + + test "write_secret_files refuses a symlinked secrets dir (planted redirect)" do + Given "an attacker-planted symlink where the per-uid dir belongs" + # The data root's parent (/Users/Shared) ships world-writable on macOS: on + # an unprovisioned machine any local user can pre-own the tree and plant a + # symlink so our 0600 files land in a directory they control. + root = Dir.mktmpdir("bc-data-root-") + original = ENV["DEV_DATA_ROOT"] + ENV["DEV_DATA_ROOT"] = root + elsewhere = File.join(root, "attacker-controlled") + FileUtils.mkdir_p(elsewhere) + File.symlink(elsewhere, File.join(root, "secrets-#{Process.uid}")) + + When "writing secret files" + build_container.write_secret_files({ "TOK" => "s3cr3t" }) + + Then "the planted dir is refused, not used" + raises Dev::BuildContainer::SecretDirCompromisedError + + Cleanup + ENV["DEV_DATA_ROOT"] = original + FileUtils.rm_rf(root) + end + + test "write_secret_files refuses a secrets dir owned by another user" do + Given "a per-uid dir whose owner is not us" + root = Dir.mktmpdir("bc-data-root-") + original = ENV["DEV_DATA_ROOT"] + ENV["DEV_DATA_ROOT"] = root + dir = File.join(root, "secrets-#{Process.uid}") + FileUtils.mkdir_p(dir) + foreign = stub(directory?: true, uid: Process.uid + 1, mode: 0o40700) + File.stubs(:lstat).with(dir).returns(foreign) + + When "writing secret files" + build_container.write_secret_files({ "TOK" => "s3cr3t" }) + + Then "the foreign dir is refused — its owner could swap files under us" + raises Dev::BuildContainer::SecretDirCompromisedError + + Cleanup + File.unstub(:lstat) + ENV["DEV_DATA_ROOT"] = original + FileUtils.rm_rf(root) + end + + test "write_secret_files sweeps stale secret files a killed run left behind" do + Given "a leftover secret file from a SIGKILLed run, and a fresh one" + # The ensure-block deletion covers normal failures, but SIGKILL leaves 0600 + # files under the data root, which macOS never purges (unlike /var/folders). + root = Dir.mktmpdir("bc-data-root-") + original = ENV["DEV_DATA_ROOT"] + ENV["DEV_DATA_ROOT"] = root + dir = File.join(root, "secrets-#{Process.uid}") + FileUtils.mkdir_p(dir) + FileUtils.chmod(0o700, dir) + stale = File.join(dir, "dev-secret-stale") + fresh = File.join(dir, "dev-secret-fresh") + File.write(stale, "old") + File.write(fresh, "new") + File.utime(Time.now - 172_800, Time.now - 172_800, stale) When "writing secret files" files = build_container.write_secret_files({ "TOK" => "s3cr3t" }) - Then "each file lives under the resolved data root" - files.values.all? { |p| p.start_with?(Dev::DataRoot.path) } - files.values.none? { |p| p.start_with?(Dir.tmpdir) } + Then "the day-old file is gone; the recent one (a concurrent run's) survives" + !File.exist?(stale) + File.exist?(fresh) + File.read(files["TOK"]) == "s3cr3t" Cleanup - files.each_value { |p| File.delete(p) if File.exist?(p) } + ENV["DEV_DATA_ROOT"] = original + FileUtils.rm_rf(root) end test "service_container_name keys the name by image, workspace, and tag" do