diff --git a/lib/dev/build_container.rb b/lib/dev/build_container.rb index c099714..fe2996b 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" @@ -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). @@ -541,17 +547,74 @@ 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. + # # @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 = secrets_dir + sweep_stale_secrets(dir) 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 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 7bb9a4c..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,7 +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 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 "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 + ENV["DEV_DATA_ROOT"] = original + FileUtils.rm_rf(root) end test "service_container_name keys the name by image, workspace, and tag" do