mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-06 06:52:20 +02:00
Léo
2a540f2acc
"Is this path inside that root" is asked in twenty places in this tree and answered twenty times by a locally written realpath/commonpath pair. Nine test files exist because nine call sites each needed their own proof. Each one is defensible alone; together they are the defect, because the boundary has no single definition and a site that gets a detail wrong is wrong by itself. src/path_confinement.py is that definition, and it settles the details the copies disagreed on. Both sides get canonicalized: comparing a realpath-ed candidate against a root that was only abspath-ed is the macOS /tmp -> /private/tmp mismatch that has already produced a false failure here, and canonicalizing one side is worse than canonicalizing neither. commonpath rather than startswith, because /a/bc begins with /a/b and is not inside it. A relative candidate joins the root rather than os.getcwd(), which is whatever directory the server happens to be running in. NUL and newline are refused with a reason instead of caught by a bare `except Exception` and reported as an ordinary escape. Eighteen call sites go through it now. It deliberately does not decide whether a path is sensitive -- that deny list answers "allowed" rather than "inside", and it stays with src/tool_execution, which owns it. The one commonpath left in the tree, in src/workspace_paths.py, stays: that function translates a host path into a container path, so canonicalizing either side would change the relative path it computes and break the mapping. It is not a confinement check. Two of those sites were weaker than the rest and are fixed rather than moved. The email attachment check used abspath, which folds `..` but does not resolve symlinks, so a symlink written into the extraction directory passed it and was then read through. The skill-reference guard compared a realpath-ed target against a raw dirname, so on a host where the skills tree is reached through a symlink the two sides never matched and the guard could not fire. The execution boundary had two separate holes. The workspace namespace bound /home and /mnt read-write. On the one platform where that namespace engages at all, a command inside it reaches outside the workspace and writes to the user's home directory -- measured by running this argv on a Linux host with working bubblewrap, not inferred from the source. Binding the user's whole home directory into a workspace-confinement namespace gives back most of what the namespace was for. Both are read-only now. The workspace is also bound writable at its real host path, not only at /workspace: BashTool's own /tmp redirect rewrites `/tmp/` to `<agent_cwd()>/.tmp/` before the namespace is built, so the command bwrap receives already names the real path, and those writes previously landed only because the workspace happened to sit under the writable /home. `namespaced or _replace_workspace_alias(...)` chose between a mount namespace and a regex with nothing in the result saying which one ran. The fallback rewrites the literal token /workspace in the command string, so a command that never mentions /workspace is untouched by it and runs on the host unrestricted -- which is every agent shell command on macOS. Both tools now ask containment.probe() instead of each deciding for itself, and every bash and python result carries a containment block naming the mechanism and stating whether the filesystem dimension actually held. Under enforcing mode the command is not run and the result says so. That block reports the filesystem dimension only, and says so in a reported_dimensions field. The probe knows this host could also give a process group and a real wall clock, but these two tools still assemble their own create_subprocess_* call and pass neither, so listing those dimensions would be exactly the false claim src/containment.py calls worse than an honest absence. probe() is new on src/containment.py: the same mechanism table and the same arithmetic as acquire(), stopping before the side effects. acquire() is the wrong shape for a decision -- it writes a durable grant record, and a record whose pid is never filled in and whose release() never runs is an entry a restart reaper keeps finding. CONTAINMENT_MODE stays report_only. Flipping it refuses every agent shell command on macOS and on any Linux host without bubblewrap, which is a product decision rather than a code one. Smaller things in the same area: the /tmp redirect's makedirs was unguarded, so a read-only workspace turned a command that merely mentioned `/tmp/` into an OSError traceback instead of a tool error; it degrades now. WORKSPACE_MOUNT moved to src/constants.py so the namespace and the path resolvers read one definition of the contract rather than two. The ".tmp" dirname got a constant, since it appeared in both tool paths. One generated artifact moved with it: website/configuration-reference.md pins the source line where each ODYSSEUS_* variable is read, and three of those shifted. Regenerated with scripts/generate_env_reference.py; the diff is line numbers only. Three existing tests changed. test_workspace_artifact_tool_floor asserted that an unsafe interpreter prefix produces no `--ro-bind <prefix> <prefix>`, which now fires on /home because /home is legitimately a read-only base mount. Asserting the absence of a literal flag string cannot distinguish "the prefix was rejected" from "the argv mounted that root itself", so it compares the argv against the no-prefix baseline instead: an unsafe prefix must add nothing. The Windows bash test asserted dict equality on the whole result, which makes adding a field to every bash result impossible without touching a test about tmux; it asserts the shape now. The personal-dir symlink test grepped the resolver's source for the literal "os.path.realpath", which is gone because the resolution moved into the shared boundary -- it keeps the negative assertion that the closure must not grow its own abspath check again, and the behavioural half now runs against the boundary, where it covers every call site instead of one closure. Not verified: the bubblewrap argv is asserted, not executed. There is no bwrap on macOS, and in Docker it needs --privileged to work at all -- default and seccomp=unconfined both fail with "Creating new namespace failed", and --cap-add=SYS_ADMIN fails at pivot_root. The Python tool's needs_virtual_namespace gate means ordinary Python code gets no namespace even on a Linux host that could provide one; that is reported now but deliberately not changed, because it alters the Linux Python path on every call and cannot be checked from here.
94 lines
3.9 KiB
Python
94 lines
3.9 KiB
Python
"""Regression: _resolve_allowed_personal_dir must resolve symlinks (realpath)
|
|
when confining a path to PERSONAL_DIR.
|
|
|
|
It used os.path.abspath, which normalises ``..`` but does NOT resolve symlinks,
|
|
so a symlink placed inside PERSONAL_DIR pointing outside it passes the
|
|
os.path.commonpath confinement check and lets index_personal_documents read
|
|
files outside the root. os.path.realpath resolves the symlink before the check.
|
|
|
|
_resolve_allowed_personal_dir is a closure inside setup_personal_routes, so it
|
|
cannot be imported and called directly. The resolution now happens in
|
|
src.path_confinement, so the behavioural test runs against that boundary and
|
|
the source-level test is reduced to the one thing still worth pinning here:
|
|
this closure must not grow its own abspath-based check again.
|
|
"""
|
|
import ast
|
|
import os
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
from src.path_confinement import PathEscape, confine
|
|
|
|
SRC = Path(__file__).resolve().parent.parent / "routes" / "personal_routes.py"
|
|
|
|
|
|
def _function_source(src_text, name):
|
|
tree = ast.parse(src_text)
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
|
|
return ast.get_source_segment(src_text, node)
|
|
raise AssertionError(f"{name} not found in {SRC}")
|
|
|
|
|
|
def test_confinement_does_not_rely_on_abspath():
|
|
"""The resolver must not reach a confinement verdict through abspath.
|
|
|
|
Originally this asserted the presence of the literal ``os.path.realpath``.
|
|
The resolution now happens inside ``src.path_confinement.confine``, which
|
|
is the point — one boundary instead of a copy per call site — so the
|
|
literal is gone while the behaviour is unchanged. What is still worth
|
|
pinning at the source level is the negative: this closure must not grow its
|
|
own abspath-based check again.
|
|
"""
|
|
body = _function_source(SRC.read_text(), "_resolve_allowed_personal_dir")
|
|
assert "os.path.abspath" not in body, (
|
|
"os.path.abspath does not resolve symlinks; the confinement check must "
|
|
"not rely on it"
|
|
)
|
|
assert "confine(" in body, (
|
|
"the resolver must go through the shared confinement boundary rather "
|
|
"than reimplementing one"
|
|
)
|
|
|
|
|
|
def test_shared_boundary_refuses_a_symlink_out_of_the_base(tmp_path):
|
|
"""The behaviour the source assertion used to stand in for.
|
|
|
|
A symlink inside the base pointing outside it is refused, and the file it
|
|
points at is not reachable through it. This is asserted against the
|
|
boundary the resolver now calls, so it covers every call site that shares
|
|
it rather than this one closure.
|
|
"""
|
|
base = tmp_path / "personal"
|
|
base.mkdir()
|
|
outside = tmp_path / "outside"
|
|
outside.mkdir()
|
|
(outside / "secret.txt").write_text("nope", encoding="utf-8")
|
|
os.symlink(outside, base / "escape")
|
|
|
|
with pytest.raises(PathEscape):
|
|
confine(base, "escape")
|
|
with pytest.raises(PathEscape):
|
|
confine(base, "escape/secret.txt")
|
|
# A real directory inside the base is still reachable.
|
|
(base / "real").mkdir()
|
|
assert confine(base, "real") == os.path.join(os.path.realpath(base), "real")
|
|
|
|
|
|
def test_realpath_catches_symlink_escape(tmp_path):
|
|
# The principle the fix relies on: abspath keeps the symlink path inside the
|
|
# base (confinement fooled); realpath resolves it outside (confinement holds).
|
|
base = tmp_path / "personal"
|
|
base.mkdir()
|
|
outside = tmp_path / "outside"
|
|
outside.mkdir()
|
|
link = base / "escape"
|
|
os.symlink(outside, link)
|
|
|
|
base_abs = os.path.realpath(base) # base itself may live under a symlinked tmp
|
|
# abspath: the symlink still looks inside base -> escape not detected
|
|
assert os.path.commonpath([os.path.abspath(base / "escape"), os.path.abspath(base)]) == os.path.abspath(base)
|
|
# realpath: the symlink resolves to `outside` -> escape detected
|
|
assert os.path.commonpath([os.path.realpath(link), base_abs]) != base_abs
|