diff --git a/docs/sandbox-docker-backend.md b/docs/sandbox-docker-backend.md index e00b5e1a1..a20422870 100644 --- a/docs/sandbox-docker-backend.md +++ b/docs/sandbox-docker-backend.md @@ -74,11 +74,19 @@ ENTRYPOINT 拼到 Cmd 前面，所以只设 Cmd 时，任何声明了 ENTRYPOINT 执行命令的东西——包括普通技能脚本，不需要 root——都可以起一个后台循环持续 `touch` 标记， 把自己维持成「一直活跃」。当前没有硬寿命上限，需要的话应由部署方在 daemon 侧限制。 -**所有 exec 都以 `user`(uid 1000) 运行。** 脚本执行、`shell_exec`、以及全部文件操作都显式 -传 `DefaultSandboxExecUser`。唯一的例外是 manager 自己的 bootstrap：它要把产物目录 `chown` -**给**沙箱账号，因此不能以该账号执行，这也是 `RemoteExecRequest.User` 留空的唯一用途。 +**所有 exec 都以 `user`(uid 1000) 运行，没有例外。** 脚本执行、`shell_exec`、全部文件操作、 +以及 manager 自己的产物目录 bootstrap 都跑在沙箱账号下；`RemoteExecRequest.User` 留空时 +适配器解析成 `DefaultSandboxExecUser` 而不是 root，漏传账号只会失去权限、不会拿到权限。 + +bootstrap 尤其不能以 root 跑：产物目录位于会话自己可写的 `/workspace` 下，而 `chown`/`chmod` +会跟随符号链接。会话只要把产物目录换成指向 `/etc` 的链接，一次 root bootstrap 就会把 `/etc` +的属主交给沙箱账号，接着改写 `passwd` 即可让该账号在下一次 exec 时变成 uid 0（真机验证过）。 +以沙箱账号执行时这条链直接断在内核：`chown` 对不属于自己的目标一律失败。 + 容器 `CapDrop: ALL` 之后额外补回 CHOWN/DAC_OVERRIDE/FOWNER/FSETID/SETGID/SETUID/KILL， -这是 root 装包和修属主需要的最小集合；Docker 默认给的 NET_RAW、MKNOD、SYS_CHROOT 等一律不给。 +Docker 默认给的 NET_RAW、MKNOD、SYS_CHROOT 等一律不给。注意这批 capability 是给容器内 +**root** 用的（装包、修属主），而目前没有任何 exec 以 root 运行，因此它们对现有路径是冗余的； +保留是为了自定义镜像里用 `sudo` 装包的场景，收紧它们是可以独立推进的加固项。 **文件操作走 exec，不走 archive 接口。** archive 接口（`PUT`/`GET`/`HEAD /archive`）由 daemon 执行，这意味着两件事同时成立：它忽略 exec user 一律以 root 操作，并且会在路径解析时跟随符号 @@ -86,8 +94,14 @@ ENTRYPOINT 拼到 Cmd 前面，所以只设 Cmd 时，任何声明了 ENTRYPOINT `ln -s /root /workspace/output/esc` 之后，`/workspace/output/esc/secret.txt` 既能通过守卫， 又会被 daemon 以 root 读出来（真机验证过，不是推演）。改成以沙箱账号 exec 之后，能不能读写 由内核判定，符号链接指向哪里都不再重要，也不存在「先校验后使用」之间被换掉链接的窗口。 -`Stat` 用 `find`，它不跟随符号链接，因此链接会如实报告为 `other` 类型，要求正规文件的调用方 -在尝试读取之前就会拒绝。archive 接口里只剩 `HEAD` 还在用，且仅用于读固定路径的活跃标记。 +`Stat` 用 `find`，它不跟随**最后一段**路径，因此路径本身是链接时会如实报告为 `other` 类型， +要求正规文件的调用方在尝试读取之前就会拒绝。 + +这个保证到最后一段为止：中间层的链接由内核在路径解析时展开，`/workspace/output/链接/passwd` +仍会 stat 成普通文件（真机验证过）。因此「只读产物目录」是一个约定而非权限边界——绕过它读到的 +东西，沙箱账号本来就能用 `shell_exec` 读到，真正的边界始终是内核的权限检查。 + +archive 接口里只剩 `HEAD` 还在用，且仅用于读固定路径的活跃标记。 **PID 1 开 tini（`HostConfig.Init`）。** 容器入口是 `sleep`，它从不调用 `wait()`。长会话里 后台进程一旦活得比启动它的 exec 久，退出后就会变成没人回收的僵尸，堆到 `pids_limit` 之后所有 diff --git a/internal/agent/tools/sandbox_read.go b/internal/agent/tools/sandbox_read.go index d95f38e5d..73c82df63 100644 --- a/internal/agent/tools/sandbox_read.go +++ b/internal/agent/tools/sandbox_read.go @@ -190,8 +190,14 @@ func (t *ReadSandboxFileTool) Execute(ctx context.Context, args json.RawMessage) } // The directory guard above is a string prefix test, so it cannot tell that // a symlink under the output directory points somewhere else entirely. The - // backends report a link as-is rather than as its target, which makes this - // the check that keeps such a path from being read at all. + // backends stat the final component without following it, so this refuses a + // path that names a link. + // + // A link in the MIDDLE of the path is still resolved by the kernel and is + // not caught here. That leaves the artifact-directory convention evadable, + // but not the privilege boundary: the read runs as the sandbox account, so + // it can only return what that account could already have read via + // shell_exec. if stat.Type != sandbox.RemoteEntryFile { return &types.ToolResult{ Success: false, diff --git a/internal/agent/tools/sandbox_read_test.go b/internal/agent/tools/sandbox_read_test.go index 2c160fe62..8d60924e7 100644 --- a/internal/agent/tools/sandbox_read_test.go +++ b/internal/agent/tools/sandbox_read_test.go @@ -81,9 +81,14 @@ func TestReadSandboxFileReturnsSmallTextOnlyInOutput(t *testing.T) { } // The output-directory guard is a string prefix test, so a symlink planted -// under that directory satisfies it while pointing anywhere. The backends -// report a link as-is instead of following it, and this is the check that -// turns that into a refusal before any read is attempted. +// under that directory satisfies it while pointing anywhere. The backends stat +// the final component without following it, and this is the check that turns +// that into a refusal before any read is attempted. +// +// The path here names the link itself, which is the case this actually covers. +// A link used as an intermediate component is resolved by the kernel and still +// stats as a regular file; see the note in Execute for why that is a convention +// leak rather than a privilege one. func TestReadSandboxFileRefusesNonRegularFile(t *testing.T) { source := &fakeSandboxFileSource{ stat: &sandbox.RemoteStatEntry{ @@ -96,7 +101,7 @@ func TestReadSandboxFileRefusesNonRegularFile(t *testing.T) { result, err := NewReadSandboxFileTool(source).Execute( sandboxFileTestContext(), - json.RawMessage(`{"path":"/workspace/output/esc/secret.txt"}`), + json.RawMessage(`{"path":"/workspace/output/esc"}`), ) require.NoError(t, err) diff --git a/internal/sandbox/docker_integration_test.go b/internal/sandbox/docker_integration_test.go index 2b10d357b..3f47a3b61 100644 --- a/internal/sandbox/docker_integration_test.go +++ b/internal/sandbox/docker_integration_test.go @@ -321,11 +321,10 @@ func TestDockerBackendScriptExecutionRefreshesActivityMarkerIntegration(t *testi } before := dockerActivityMarkerMTime(t, ctx, summaries[0].ID) - // Exec directly as the unprivileged account, with no root-run step in - // between: WeKnora happens to prepare the artifact directory as root - // before each script today, and leaning on that would let the marker - // silently stop tracking the account that actually runs user code. - // The marker has one-second resolution on most filesystems. + // Exec directly as the unprivileged account: this asserts that the account + // running user code refreshes the marker itself, rather than relying on + // some other step happening to touch it first. The marker has one-second + // resolution on most filesystems. time.Sleep(2 * time.Second) result, err := client.Exec(ctx, handle, RemoteExecRequest{ Command: "echo", @@ -427,6 +426,97 @@ with open(%q) as handle: } } +// A session owns /workspace, so it can replace its own artifact directory with +// a symlink pointing anywhere in the container. chown and chmod follow +// symlinks, so if the pre-execution bootstrap ran as root it would hand the +// session ownership of the link's target — /etc here, which is enough to +// rewrite passwd and give the sandbox account uid 0 on the next exec. +// +// Session-scoped containers are what make this reachable: the link planted by +// one execution is still there when the next one runs the bootstrap. Under the +// old one-shot `docker run --rm` the two never shared a filesystem. +func TestDockerBackendArtifactBootstrapDoesNotFollowSymlinkIntegration(t *testing.T) { + cfg := dockerIntegrationConfig(t) + manager := newDockerIntegrationManager(t, cfg) + + ctx, cancel := context.WithTimeout( + types.WithSandboxTenantID(context.Background(), dockerIntegrationTenantID), + 5*time.Minute, + ) + defer cancel() + + sessionID := fmt.Sprintf("docker-chown-escape-%d", time.Now().UnixNano()) + t.Cleanup(func() { + cleanupCtx, cleanupCancel := context.WithTimeout( + types.WithSandboxTenantID(context.Background(), dockerIntegrationTenantID), + time.Minute, + ) + defer cleanupCancel() + _ = manager.DestroySession(cleanupCtx, sessionID) + }) + + if first := runDockerScript(t, ctx, manager, sessionID, `print('seed')`); !first.IsSuccess() { + t.Fatalf("seed execution failed: %#v", first) + } + + client, err := NewDockerRemoteClient(cfg) + if err != nil { + t.Fatalf("build docker client: %v", err) + } + summaries, err := client.List(ctx, RemoteListFilter{ + Metadata: map[string]string{remoteMetadataSessionID: sessionID}, + }) + if err != nil || len(summaries) != 1 { + t.Fatalf("expected exactly one container for the session: %v %#v", err, summaries) + } + handle, err := client.Connect(ctx, summaries[0].ID) + if err != nil { + t.Fatalf("Connect: %v", err) + } + + ownerOfEtc := func() string { + t.Helper() + result, err := client.Exec(ctx, handle, RemoteExecRequest{ + Command: "stat", + Args: []string{"-c", "%U:%G", "/etc"}, + User: DefaultSandboxExecUser, + Timeout: 30 * time.Second, + }) + if err != nil || result.ExitCode != 0 { + t.Fatalf("stat /etc failed: %v %#v", err, result) + } + return strings.TrimSpace(result.Stdout) + } + + before := ownerOfEtc() + if before != "root:root" { + t.Fatalf("/etc should start out root-owned, got %q", before) + } + + // Exactly what a model with shell_exec can do: it owns /workspace. + plant, err := client.Exec(ctx, handle, RemoteExecRequest{ + Shell: true, + Command: fmt.Sprintf( + "rm -rf %s && ln -s /etc %s", SessionOutputRoot, SessionOutputRoot, + ), + User: DefaultSandboxExecUser, + Timeout: 30 * time.Second, + }) + if err != nil || plant.ExitCode != 0 { + t.Fatalf("planting the symlink failed: %v %#v", err, plant) + } + + // Any further execution runs the bootstrap against the planted path. + if second := runDockerScript(t, ctx, manager, sessionID, `print('after')`); second == nil { + t.Fatal("second execution returned no result") + } + + if after := ownerOfEtc(); after != "root:root" { + t.Fatalf("the artifact bootstrap chowned through the symlink: /etc is now %q, want root:root", + after) + } +} + // stopDockerContainerForTest stops a container behind the adapter's back, the // way a host reboot or an operator with a shell would. func stopDockerContainerForTest(ctx context.Context, id string) error { diff --git a/internal/sandbox/docker_remote_client.go b/internal/sandbox/docker_remote_client.go index f1e8816ec..fc7c21ba3 100644 --- a/internal/sandbox/docker_remote_client.go +++ b/internal/sandbox/docker_remote_client.go @@ -14,11 +14,16 @@ // Get/List → GET /containers/json?filters=label=… // Delete → DELETE /containers/{id}?force=1 // Exec → POST /containers/{id}/exec → /exec/{id}/start (hijack) -// Files → PUT/GET/HEAD /containers/{id}/archive // -// Three operations have no Engine API and are implemented as exec: -// MakeDir, Remove and ListDir. ListDir uses `find -printf`, which needs GNU -// findutils in the image; the standard WeKnora sandbox image provides it. +// Every file operation — WriteFile, ReadFile, Stat, MakeDir, Remove, ListDir — +// is an exec running as the sandbox account, NOT a call to /archive. The +// archive endpoints run as root and resolve symlinks, so a session that plants +// a link inside its own workspace could read or overwrite anything in the +// container through them. Going through exec puts the kernel back in charge of +// who may touch what. Do not "simplify" these back onto /archive. +// +// ListDir and Stat use `find -printf`, which needs GNU findutils in the image; +// the standard WeKnora sandbox image provides it. // // Two Docker facts shape the rest of this file: // @@ -666,18 +671,24 @@ func dockerExecCommand(req RemoteExecRequest, timeout time.Duration) []string { // dockerExecUser resolves which account a command runs as. // -// An empty user is reserved for the manager's own bootstrap, which chowns the -// artifact directory TO the sandbox account and therefore cannot run as it. It -// is not a general default: the envd-backed backends resolve a blank user to -// DefaultSandboxExecUser (E2B authenticates the data plane as that account, -// Cube hands the field to envd, which defaults the same way), so a path that -// left this to the adapter would run as root here and unprivileged there. -// Every caller-reachable path names the account explicitly. +// A blank user resolves to the sandbox account. It must never resolve to root: +// this function is the single choke point for every exec the daemon runs, so a +// caller that forgets to name an account has to lose privileges here, not +// silently gain them. It also makes the backends agree — E2B authenticates its +// data plane as DefaultSandboxExecUser and Cube hands a blank field to envd, +// which defaults the same way. +// +// This used to fall back to root for the manager's artifact-directory +// bootstrap. That was a container-escape primitive: chown follows symlinks, so +// a session that replaced its own artifact directory with a link to /etc got +// the root-run bootstrap to hand it ownership of /etc, and from there uid 0 by +// rewriting passwd. The bootstrap now names the account like everyone else and +// simply fails when it is aimed at something the account does not own. func dockerExecUser(user string) string { - if strings.TrimSpace(user) == "" { - return "root" + if trimmed := strings.TrimSpace(user); trimmed != "" { + return trimmed } - return user + return DefaultSandboxExecUser } // dockerExecWasKilled reports whether an exit code means the wrapper killed @@ -776,9 +787,17 @@ func (c *DockerRemoteClient) ReadFile( // Stat returns metadata for one path. // -// find does not follow symlinks, so a link reports as RemoteEntryOther rather -// than as whatever it points at. Callers that only accept regular files -// therefore refuse it before any read is attempted. +// find reports the FINAL component without following it, so a path that names a +// link reports RemoteEntryOther rather than whatever it points at, and callers +// that only accept regular files refuse it before any read is attempted. +// +// That guarantee stops at the final component. Intermediate components are +// resolved by the kernel during path lookup, exactly as they are for any other +// process, so `/workspace/output/link-to-etc/passwd` stats as a regular file — +// verified against a real daemon. Reads through such a path are not a +// privilege boundary being crossed, only the "stay inside the artifact +// directory" convention: ReadFile still runs as the sandbox account, so it +// returns what that account could have read anyway with shell_exec. func (c *DockerRemoteClient) Stat( ctx context.Context, handle RemoteSandboxHandle, diff --git a/internal/sandbox/docker_remote_client_test.go b/internal/sandbox/docker_remote_client_test.go index 0bb982f42..6e73d9b3c 100644 --- a/internal/sandbox/docker_remote_client_test.go +++ b/internal/sandbox/docker_remote_client_test.go @@ -550,14 +550,17 @@ func TestDockerClientExecWrapsCommandWithTimeoutAndActivityMarker(t *testing.T) opts.Cmd[3:], "the command must reach the shell as positional args, never interpolated") } -// A blank user is reserved for the manager's own privileged bootstrap. Every -// other adapter resolves it to the sandbox account, so a caller that forgets to -// name one here would silently gain root instead of losing privileges. -func TestDockerExecUserOnlyFallsBackToRootWhenUnnamed(t *testing.T) { +// Every exec the daemon runs passes through dockerExecUser, so a caller that +// forgets to name an account has to lose privileges here rather than gain them. +// Falling back to root used to be a container-escape primitive: the artifact +// bootstrap chowns a path inside the session's own workspace, and chown follows +// symlinks, so root + a planted link meant the session could take ownership of +// /etc and rewrite passwd to give itself uid 0. +func TestDockerExecUserNeverFallsBackToRoot(t *testing.T) { require.Equal(t, DefaultSandboxExecUser, dockerExecUser(DefaultSandboxExecUser)) require.Equal(t, "1000:1000", dockerExecUser("1000:1000")) - require.Equal(t, "root", dockerExecUser("")) - require.Equal(t, "root", dockerExecUser(" ")) + require.Equal(t, DefaultSandboxExecUser, dockerExecUser("")) + require.Equal(t, DefaultSandboxExecUser, dockerExecUser(" ")) } // Cancelling an exec leaves the copy goroutine writing into the output buffers. @@ -611,8 +614,8 @@ func TestDockerClientExecShellPassesCommandAsPositionalArgument(t *testing.T) { require.NoError(t, err) opts := engine.execOptions[0] require.Equal(t, []string{"weknora-exec", `echo "a b"; rm -rf /nope`}, opts.Cmd[3:]) - require.Equal(t, "root", opts.User, - "a blank user is the manager's bootstrap escape hatch; callers name the account") + require.Equal(t, DefaultSandboxExecUser, opts.User, + "an unnamed account must resolve to the sandbox user, never to root") } func TestDockerClientExecRejectsShellWithArgs(t *testing.T) { diff --git a/internal/sandbox/session_manager.go b/internal/sandbox/session_manager.go index 02a16b7b7..d15bd28ed 100644 --- a/internal/sandbox/session_manager.go +++ b/internal/sandbox/session_manager.go @@ -304,11 +304,18 @@ func (m *SessionBoundManager) Execute(ctx context.Context, cfg *ExecuteConfig) ( return m.ephemeral.ExecuteOnHandle(ctx, handle, cfg) } -// ensureExecutionOutputDir creates the skill artifact directory and grants -// DefaultSandboxExecUser write access before script execution. envd MakeDir -// often leaves the path root-owned on Cube; the follow-up chown/chmod runs as -// root via envd (empty User). Best-effort: failures are logged and do not -// abort the upcoming script execution. +// ensureExecutionOutputDir creates the skill artifact directory and makes sure +// DefaultSandboxExecUser can write to it before script execution. +// +// This runs AS that account, never as root. The directory sits inside the +// session's own writable workspace, and chown/chmod follow symlinks, so a +// root-run bootstrap can be aimed at any directory in the container: a session +// that swaps its artifact directory for a link to /etc gets handed ownership of +// /etc, and from there uid 0 by rewriting passwd. Running as the sandbox +// account makes that a no-op — chown succeeds on the directory MakeDir just +// created for it and is refused by the kernel on anything else. +// +// Best-effort: failures are logged and do not abort the upcoming execution. func (m *SessionBoundManager) ensureExecutionOutputDir( ctx context.Context, handle RemoteSandboxHandle, @@ -334,6 +341,7 @@ func (m *SessionBoundManager) ensureExecutionOutputDir( result, err := m.client.Exec(ctx, handle, RemoteExecRequest{ Shell: true, Command: line, + User: execUser, Timeout: sessionArtifactDirBootstrapTimeout, }) if err != nil { diff --git a/internal/sandbox/session_manager_test.go b/internal/sandbox/session_manager_test.go index a5c383276..b8c7bd5a4 100644 --- a/internal/sandbox/session_manager_test.go +++ b/internal/sandbox/session_manager_test.go @@ -89,16 +89,17 @@ func TestSessionBoundManagerExecuteEnsuresOutputDir(t *testing.T) { require.True(t, execs[0].Shell) require.Contains(t, execs[0].Command, "chown user:user") require.Contains(t, execs[0].Command, SessionOutputRoot) - require.Empty(t, execs[0].User, - "the bootstrap chowns the directory TO the sandbox account, so it is the "+ - "one exec that must keep the adapter's privileged default") + require.Equal(t, DefaultSandboxExecUser, execs[0].User, + "chown follows symlinks, so a root-run bootstrap can be aimed at /etc by "+ + "a session that swaps its artifact directory for a link; running as the "+ + "sandbox account is what makes that attempt fail") } // shell_exec carries a command line the model wrote, which makes it the exec -// path an injected prompt reaches most directly. The account it runs as must -// therefore be pinned here rather than left to each adapter: Docker resolves a -// blank user to root, while the envd-backed backends resolve it to the sandbox -// account, so omitting it would hand out different privileges per backend. +// path an injected prompt reaches most directly. The account it runs as is +// pinned here rather than left to each adapter, so that reading this call site +// answers "as whom does model-authored input run" without having to trust that +// all three adapters agree on what a blank user means. func TestSessionBoundManagerShellExecRunsAsSandboxUser(t *testing.T) { client := newFakeRemoteClient(SandboxTypeCube) cfg := DefaultConfig()