diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dd7a8e..d79ee37 100644 --- a/CHANGELOG.md +++ b/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//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. diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 4037d29..ed17250 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -112,6 +112,10 @@ same-UID reads. Measured, not assumed — items 7 and 8 below: | `/proc//environ` | **Readable** | Needs only `PTRACE_MODE_READ`, which Yama does not restrict. | | `/proc//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//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//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 diff --git a/docs/LOGS.md b/docs/LOGS.md index fe1f2bd..14e7e91 100644 --- a/docs/LOGS.md +++ b/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//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//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 diff --git a/hotcell-server/lib/hot_cell/supervisor.rb b/hotcell-server/lib/hot_cell/supervisor.rb index 11c2164..a6cf8cd 100644 --- a/hotcell-server/lib/hot_cell/supervisor.rb +++ b/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//fd/2 and write there too. + # holding fd 2 writes whenever it likes, and a sibling can open a tool's /proc//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//fd/N and reopen the files and pipes + # a worker holds. A non-dumpable process's /proc/ 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 diff --git a/hotcell-server/test/worker_isolation_test.rb b/hotcell-server/test/worker_isolation_test.rb new file mode 100644 index 0000000..5528bc9 --- /dev/null +++ b/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