Harden workers so a sibling cannot open their descriptors

CHANGELOG.md

@@ -21,6 +21,10 @@ Some actions that application developers should consider taking when upgrading f
 
 ### HotCell::Server
 
+#### Security
+
+* A worker is now non-dumpable. Another worker cannot open the worker's descriptors through `/proc/<pid>/fd`. A tool that the worker runs does not get this protection.
+
 #### Added
 
 * `hot_cell/health_operations` defines `health.echo` and `health.reopen`, the round trips an application calls to prove it can use a cell's work socket. A cell serves them only if it requires the file.

docs/DESIGN.md

@@ -112,6 +112,10 @@ same-UID reads. Measured, not assumed — items 7 and 8 below:
 | `/proc/<sibling>/environ` | **Readable** | Needs only `PTRACE_MODE_READ`, which Yama does not restrict. |
 | `/proc/<sibling>/fd/N` | **Readable** | Same `PTRACE_MODE_READ`. So are the scratch files themselves, by directory listing. |
 
+A worker is forked non-dumpable, so its `/proc` entries are no longer owned by its uid, and both readable
+rows are `EACCES` for a worker. They stay readable for the supervisor, whose environment every
+worker inherits, and for a tool, because an `exec` resets the flag.
+
 So invariant 8 has three parts and they have different answers. Only the first is about `ptrace_scope`.
 
 **Request memory** is protected by `kernel.yama.ptrace_scope >= 1`. That is a host sysctl a container
@@ -153,11 +157,11 @@ closes the offline route, where nothing of the attacker's is still running.
 
 **Files are not isolated between concurrent workers, and cannot be.** Every worker runs as the same uid in
 one mount namespace, so a worker that reads another worker's scratch directory — by listing it, or through
-`/proc/<sibling>/fd/N` — gets that request's input and output bytes. Unlinking the scratch file does not
-close it, because the descriptor is still reachable through the sibling's `/proc`. The two fixes that
-would work need `CAP_SETUID` for a per-worker uid or `CAP_SYS_ADMIN` for a per-request mount namespace,
-and `cap-drop ALL` removes both. This is the same shape as the environment: a real residual, stated rather
-than papered over.
+`/proc/<pid>/fd/N` of a tool that worker runs — gets that request's input and output bytes. Unlinking the
+scratch file does not close it, because the descriptor is still reachable through that tool's `/proc`. The
+two fixes that would work need `CAP_SETUID` for a per-worker uid or `CAP_SYS_ADMIN` for a per-request mount
+namespace, and `cap-drop ALL` removes both. This is the same shape as the environment: a real residual,
+stated rather than papered over.
 
 What bounds it is the size of the window and the value of the contents. Only requests actually in flight
 have bytes inside a cell, a cell holds no credentials, and a cell carries one toolchain. So the exposure

docs/LOGS.md

@@ -101,10 +101,10 @@ libgomp: Thread creation failed: Resource temporarily unavailable
 **The text is not evidence — it establishes neither who wrote it nor which request it belongs to**, the
 caveat `hotcell.signal` and `hotcell.cause` already carry. It comes from the one process in a cell that
 runs untrusted code, over an unauthenticated channel: everything a worker spawned inherits fd 2, so a tool
-can write long after the request it belongs to finished, and a sibling worker can open `/proc/<pid>/fd/2`
-and write whatever it likes, since workers share a uid and `kernel.yama.ptrace_scope` protects memory
-rather than descriptors. The supervisor clears the buffer at each dispatch, which keeps an old warning off
-an unrelated death in the ordinary case. It is not a boundary.
+can write long after the request it belongs to finished, and a sibling worker can open a tool's
+`/proc/<pid>/fd/2` and write whatever it likes, since workers share a uid and an `exec`ed tool is
+dumpable. The supervisor clears the buffer at each dispatch, which keeps an old warning off an unrelated
+death in the ordinary case. It is not a boundary.
 
 **The capture is best effort, because fd 2 is non-blocking.** A C library writing to a full pipe gets `EAGAIN`
 and loses the line, and a fatal handler cannot retry — it writes once and calls `exit()`. That costs nothing

hotcell-server/lib/hot_cell/supervisor.rb

@@ -6,6 +6,7 @@
 require "tmpdir"
 require "pathname"
 require "etc"
+require "rbconfig"
 
 module HotCell
   # Accepts, queues, dispatches, times, kills, reaps, and cleans up. It never evaluates image data.
@@ -65,7 +66,8 @@ def stderr_field
 
       def dispatched(connection, deadline, at:)
         # An earlier request's warning is not this request's death. Not a boundary, though: a descendant
-        # holding fd 2 writes whenever it likes, and a sibling can open /proc/<pid>/fd/2 and write there too.
+        # holding fd 2 writes whenever it likes, and a sibling can open a tool's /proc/<pid>/fd/2 and write
+        # there too.
         captured.clear
         self.connection = connection
         self.dispatched_at = at
@@ -150,6 +152,11 @@ def timed?
     # host sysctl no container flag can supply.
     PTRACE_SCOPE = "/proc/sys/kernel/yama/ptrace_scope"
 
+    # prctl(2) by architecture, for `fork_nondumpable`. An architecture missing here forks dumpable workers.
+    PRCTL = { "x86_64" => 157, "aarch64" => 167 }.freeze
+    PR_GET_DUMPABLE = 3
+    PR_SET_DUMPABLE = 4
+
     # A control connection that has not sent its request yet. Reading it non-blockingly is what stops a
     # client that connects and then says nothing from stalling the loop every conversion depends on.
     Pending = Struct.new(:connection, :accepted_at, :buffer)
@@ -487,7 +494,7 @@ def spawn
         # A fork that fails is a host under pressure, not a reason to stop serving. The request stays queued
         # and is either dispatched on a later pass or answered `capacity` when its wait runs out.
         pid = begin
-          fork do
+          fork_nondumpable do
             become_worker supervisor_side, stderr_reader, stderr_writer
             Worker.new(slot: slot, configuration: configuration, control: Connection.new(worker_side),
                        log: log).run
@@ -554,6 +561,37 @@ def become_worker(supervisor_side, stderr_reader, stderr_writer)
         stderr_writer.close
       end
 
+      # Every worker runs as one uid, so a sibling could open /proc/<pid>/fd/N and reopen the files and pipes
+      # a worker holds. A non-dumpable process's /proc/<pid> is no longer owned by its uid, which
+      # `cap-drop ALL` leaves nothing to override, and setting it needs no capability. It also stops a core
+      # dump, which RLIMIT_CORE 0 in `Limits#apply` already did.
+      #
+      # Set around the fork rather than by the worker first thing, because the child is in /proc before it
+      # runs any Ruby: a sibling watching for new pids listed a child's descriptors before its own prctl in
+      # nearly every fork.
+      #
+      # An exec resets the flag, so a tool a worker runs is reachable again for as long as it runs. See
+      # "Worker isolation" in docs/DESIGN.md.
+      def fork_nondumpable(&block)
+        dumpable = prctl(PR_GET_DUMPABLE)
+        prctl PR_SET_DUMPABLE, 0
+        fork(&block)
+      ensure
+        prctl PR_SET_DUMPABLE, dumpable if dumpable
+      end
+
+      # `syscall` rather than Fiddle, which is a bundled gem since Ruby 4.0 and so a dependency this gem does
+      # not carry. It warns on every call under -W:deprecated.
+      def prctl(option, argument = 0)
+        number = PRCTL[RbConfig::CONFIG["host_cpu"]]
+        return unless number && RUBY_PLATFORM.include?("linux")
+
+        deprecated, Warning[:deprecated] = Warning[:deprecated], false
+        syscall number, option, argument
+      ensure
+        Warning[:deprecated] = deprecated unless deprecated.nil?
+      end
+
       # Everything the supervisor holds and a child of its must not: the listeners, the signal pipe, the
       # children's control sockets, and every connection the supervisor is still holding for somebody else.
       def leave_supervisor

hotcell-server/test/worker_isolation_test.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+# Every worker runs as the same uid, so without more a sibling reads a worker's /proc entries the way it reads
+# its own. The test process stands in for the sibling: it has the worker's uid and is not root.
+class WorkerIsolationTest < HotCellServerTest
+  # A second allowed request keeps the worker alive, past `become_worker`, to be looked at.
+  def test_a_process_with_the_workers_uid_cannot_reach_its_descriptors
+    skip "reading another process's descriptors needs procfs" unless File.directory?("/proc/self/fd")
+    skip "root reads every process's descriptors" if Process.uid.zero?
+
+    TestCell.boot(concurrency: 1, max_requests_per_worker: 2) do |cell|
+      worker = assert_ok(cell.call("test.whoami")).result[:pid]
+      supervisor = cell.log_events("cell.boot").first.dig(:process, :pid)
+
+      # The premise first: the same uid reaches a process that is not a worker, so the refusal means something.
+      Dir.children "/proc/#{supervisor}/fd"
+
+      assert_raises(Errno::EACCES) { Dir.children "/proc/#{worker}/fd" }
+    end
+  end
+end