security: validate msgpack ext-type decoding#142
Merged
Conversation
unpack() runs on peer-shaped payloads — including host-side unpacks of sandbox-emitted side-channel events — so ext decoding is now validated and bounded (#140): a malformed ext payload raises ExtDecodeError instead of an arbitrary numpy/msgpack error from mid-decode, the ndarray header is checked before the buffer is interpreted (dtype must parse, carry no objects, and agree with the buffer size), and pydantic ext nesting is depth-bounded so a payload cannot recurse the unpacker. Also fixes a latent round-trip bug: dtype.str for single-byte-order dtypes leads with the header separator ('|b1', '|i1', '|S5'), so bool/int8/bytes arrays failed to decode — the shape now splits on the LAST separator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adversarial review found two payload classes that passed every check
and then raised a raw numpy ValueError from frombuffer/reshape (ndim
above numpy's cap; subarray dtypes that expand extra elements),
breaking the only-ExtDecodeError guarantee. Refuse subarray/void
dtypes explicitly, wrap the final construction as belt-and-braces,
and refuse object/structured arrays at encode — dtype.str collapses
a structured dtype to '|V<size>', so decoding it would return
silently field-stripped data. Wrapped messages use {exc!r} because
msgpack's FormatError stringifies to ''.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
unpack()runs on payloads the peer shaped — including host-side unpacks of sandbox-emitted side-channel events (/trace,/log, plugin namespaces) — but the two msgpack extension types decoded with no validation. This PR makes ext decoding validated and bounded; the #116 story then covers both sandbox→host decode surfaces (pickle returns via #137's restricted unpickler, msgpack side channels here).ExtDecodeError(aValueError): a malformed ext payload surfaces as this one typed error, never an arbitrary numpy/msgpack exception from mid-decode.prod(shape) * itemsizemust equal the buffer length; final construction is wrapped as belt-and-braces (e.g. ndim above numpy's cap)._MAX_EXT_DEPTH = 16; legitimate nesting — a model payload carrying an array or another dumped model — is shallow); inner msgpack errors are wrapped with the cause class visible.TypeError:dtype.strcollapses a structured dtype to'|V<size>'(fields stripped), so such a payload could only ever decode to corrupted data.Bug fix
Latent round-trip breakage:
dtype.strfor single-byte-order dtypes leads with the header separator ('|b1','|i1','|S5'), and the decoder split on the FIRST|— sobool/int8/bytes/unicode arrays failed to decode on master (ValueError: too many values to unpack). The shape now splits on the LAST separator; round-trips are covered per dtype class.Review
Three-lens adversarial review (legit-traffic regressions / validation completeness / decode-state reentrancy) with two refuters per finding: 3 confirmed findings all fixed in the second commit (unwrapped
frombuffer/reshapeescapes, silent void decode, empty wrapped message); thread-safety concerns refuted (everyunpack()call site is synchronous on the asyncio loop).Testing
tests/runtime/shared/test_codec.py(round-trip fidelity per dtype class incl. scalars and model-embedded arrays, plus refusal cases for each validation)pytest -q tests plugins: 603 passed, 6 deselectedruff check .clean; pyright 0 errors at--pythonversion 3.11and3.13Closes #140.
🤖 Generated with Claude Code