Skip to content

Harden sandbox return boundary by replacing host-side pickle deserialization#118

Closed
FatPigeorz with Copilot wants to merge 3 commits into
masterfrom
copilot/fix-unsafe-deserialization
Closed

Harden sandbox return boundary by replacing host-side pickle deserialization#118
FatPigeorz with Copilot wants to merge 3 commits into
masterfrom
copilot/fix-unsafe-deserialization

Conversation

Copilot AI commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Host-side RuntimeClient was deserializing sandbox return payloads with pickle.loads, allowing sandbox-influenced objects to execute code on the host during decode. This change moves return-value transport to msgpack decoding on the host boundary while preserving existing callable/argument behavior.

  • Boundary hardening (host decode path)

    • Replaced host-side return decoding from pickle.loads(...) to unpack(...) (msgpack codec).
    • Added strict payload-type checks and explicit security-oriented decode errors for malformed/non-msgpack return payloads.
  • Return-value wire format change (worker → host)

    • Worker now encodes RemoteResponse.value with shared msgpack codec (pack(result)) instead of pickling return objects.
    • Argument transport remains unchanged (pickle.dumps((args, kwargs))) to keep callable invocation behavior stable.
  • Protocol/docs alignment

    • Updated runtime protocol and architecture docs to reflect: args are pickle, return values are msgpack.
    • Updated wire-model/framing descriptions accordingly.
  • Test updates + security regression

    • Updated runtime tests that previously unpickled return payloads to use msgpack decode.
    • Added a focused regression test proving a malicious pickle payload is rejected by host decode and does not execute host-side effects.
def _decode_result_value(raw: Any) -> Any:
    if raw is None:
        return None
    if isinstance(raw, memoryview):
        raw = raw.tobytes()
    elif isinstance(raw, bytearray):
        raw = bytes(raw)
    if not isinstance(raw, bytes):
        raise RuntimeError("sandbox return value must be msgpack bytes; ...")
    return unpack(raw)

Copilot AI changed the title [WIP] Fix host-side unsafe deserialization of sandbox return values Harden sandbox return boundary by replacing host-side pickle deserialization Jun 2, 2026
Copilot AI requested a review from FatPigeorz June 2, 2026 11:42
@Meirtz

Meirtz commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Review — the concern is real, but this fix breaks the plugin ecosystem as written

#116 is a genuine, present issue on master. The host decodes return bytes that originate inside the sandbox using pickle (agentix/runtime/client/client.py:111, at both the HTTP /call reply :274 and the SIO result :328; the worker packs returns at invoker.py:89). The trust model is what makes it matter: the sandbox runs autonomous agent code while the host holds the real credentials, so hardening the host-side decode of sandbox-produced bytes is a reasonable goal. This shouldn't be closed.

But the fix, as written, breaks essentially every non-dict/bytes remote target. msgpack can't encode a dataclass, and master's codec only has ndarray + pydantic extension hooks — so all seven shipped dataclass-returning targets would fail worker-side with ResultEncodeError:

  • agentix.bash.runBashResult, agentix.bash.run_streamBashEvent union
  • agentix.files.uploadUploadResult
  • swe.prepare_envPrepareEnvResult
  • agentix.agents.claude_code.runClaudeCodeResult
  • qwen_code.runResult
  • abridge TunnelHandle

And even a pydantic return decodes host-side to a plain dict, so every example's attribute access — r.exit_code, prepared.ok, result.returncode, svc.url, diff.stdout — breaks with AttributeError. The added regression test only checks the failure mode is neutralized; it doesn't exercise a real dataclass return, so CI stays green while the plugins/examples would fail at runtime (examples aren't in the pytest run, which is why this isn't caught).

It also contradicts the stated direction and conflicts with #122. ROADMAP is explicit that pickle is the default for "args, kwargs, and return values" and msgpack is a future optional path alongside pickle, not a replacement. Meanwhile #122 keeps returns on pickle and deletes the codec's extension hooks entirely — so #118 and #122 edit the same codec.py / PROTOCOL.md lines in opposite directions and cannot both merge without reconciliation.

Design options for the maintainers (this is a real decision, not a mechanical fix):

  1. Restricted decode path — keep pickle for returns but decode host-side through an allowlist that refuses arbitrary object reconstruction. Preserves the "any Python object round-trips" surface ROADMAP promises; smallest blast radius.
  2. msgpack returns + a dataclass codec hook — go the Harden sandbox return boundary by replacing host-side pickle deserialization #118 direction but first extend codec.py to encode/reconstruct dataclasses, so the plugin ecosystem keeps working. Larger change; must land before or with the switch.
  3. Typed returns — constrain targets to declared return schemas.

I'd lean (1) as the least-disruptive way to actually address #116 while honoring the ROADMAP surface, but it's your call. Flagging rather than merging or closing, since whichever way this goes needs to be reconciled with #122's codec changes.

@Meirtz

Meirtz commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Closing as superseded. The host-side deserialization concern (#116) is being addressed on a different axis: a restricted-unpickler allowlist that fixes the escape vector without changing the return-value wire format — the msgpack-for-returns approach here would break every dataclass-returning shipped target (BashResult, ClaudeCodeResult, PrepareEnvResult, TunnelHandle, …) and conflicts with the codec work landing via the PR #122 split. Thanks for the initial fix; the direction we're taking keeps pickle as the default per the roadmap. Tracking the hardening under #116.

@Meirtz Meirtz closed this Jul 2, 2026
Meirtz added a commit that referenced this pull request Jul 3, 2026
* security: restricted unpickler on the sandbox->host return boundary (#116, PR #122 stage E)

Return values travel host-ward as pickle.dumps(result) and the host
decoded them with plain pickle.loads. pickle reconstructs objects by
invoking whatever callables a stream names, so decoding a value shaped
by a less-trusted sandbox workload (a cloned repo, a generated patch, a
benchmark task) is a trust boundary: the returned object can direct
reconstruction on the host (#116).

Keep pickle as the return codec (the msgpack-returns alternative was
evaluated and closed as #118) and decode host-side through a strict
allowlist (agentix/runtime/shared/safepickle.py). find_class permits
only:
  - a reviewed set of value types whose construction has no external
    effect (stdlib data: datetime/decimal/fractions/uuid/collections/
    pathlib; builtin containers + exceptions; numpy arrays), and
  - a small set of inert reconstruction helpers (copyreg, numpy
    _reconstruct),
and refuses everything else WITHOUT importing it. A denylist was
rejected as unsound: a stream can name a C-accelerator module
(_operator vs operator), a callable produced by one reconstruction step
is invoked by the next without passing find_class (so attribute-access
helpers must never be admitted), and many ordinary constructors have
side effects. A closed allowlist of value types closes all three.

First-party types (agentix.*) are trusted by default: the framework and
its plugins are the trusted computing base that builds the bundle and
runs the sandbox, and their return types (TunnelHandle, BashResult,
agent results) are inert. This keeps the framework's own paths
(Proxy.start, bash.run, agent adapters) working without setup. A
workload's own return types are refused by default; opt in with
safepickle.allow_module(prefix) / allow_callable(module, name), or set
AGENTIX_PICKLE_TRUST=1 to trust the sandbox fully. A refusal raises
agentix.RestrictedUnpickleError.

Only the sandbox->host direction is restricted (client._unpickle_value,
both the SIO and HTTP result paths). The sandbox-side decode of
host-supplied arguments/context stays plain pickle -- the trusted
host->sandbox direction.

Tests: non-allowlisted callables (subprocess.check_output/Popen,
os.system, eval) refused; attribute-access helpers (operator/_operator
attrgetter/itemgetter/methodcaller, getattr) refused; refusal does not
import the named module; object-dtype numpy arrays gate nested globals;
first-party return types (TunnelHandle, BashResult) and permitted value
types + builtin exceptions + workload-via-allow_module round-trip; trust
bypass; end-to-end refusal over a real remote() call. PROTOCOL.md
documents the boundary; RestrictedUnpickleError exported.

Closes #116.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs: design exact-type restricted unpickling

* security: add exact pickle type opt-in

* test: cover exact pickle policy boundary

* security: restrict pickle globals to exact values

* test: cover exact first-party pickle values

* test: tighten safepickle compatibility assertions

* style: format restricted unpickler

* docs: add exact-type unpickling implementation plan

* test: cover warm pickle extension cache bypass

* security: reject pickle extension opcodes

* test: make restricted decode regressions harmless

* docs: list the six exact default return types in PROTOCOL

The unpickling paragraph still described the pre-exact policy: blanket
agentix.* prefix trust and the deleted allow_module()/allow_callable()
surfaces. Name the six individually registered first-party return types
and point extension at allow_type(Class), matching safepickle.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* style: format public export list

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* chore: keep agentix-ray-build skill out of the stage E diff

The Ray-cluster build notes are unrelated to the restricted unpickler;
untrack them so the stage E diff stays on-topic. The file stays on disk
as an untracked local note.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Host-Side Unsafe Deserialization of Sandbox Return Values

3 participants