fix(deserialization): replace pickle.loads with JSON across all sinks - #2709
fix(deserialization): replace pickle.loads with JSON across all sinks#2709amabito wants to merge 3 commits into
Conversation
Five deserialization sinks consumed untrusted bytes via pickle.loads or importlib.import_module on attacker-controlled strings. Each path allowed remote code execution for any actor with write access to the respective backend (Redis pub/sub, Redis GET, Cosmos DB items, local teachability store). This change replaces pickle with JSON across all five sites and replaces the dynamic class resolver with a registry lookup. Pickle read paths remain accessible behind explicit opt-in environment variables with deprecation warnings for smooth migration. Addresses: R1R3-B1, R1R3-B2, R3R2-B1, R3R2-B2, R4R1-B1
Codecov Report❌ Patch coverage is
... and 393 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
CI is red on |
|
This is a high-quality, important security fix — the design is well-thought-out across all three attack surfaces (Redis cache, CosmosDB cache, teachability). The version-prefix byte approach for forward/backward compatibility is clean, and the migration script + security docs are a nice touch. One issue to address before merging: unversioned legacy cache data Existing Redis/CosmosDB users who upgrade will have cache entries written by the old code as raw The current
This means:
Suggested fix — add a fallback case in # Unversioned payload: legacy pickle written by old code (no prefix byte).
# Only honoured with AG2_ALLOW_PICKLE_CACHE_READ=1.
if _PICKLE_CACHE_READ_ENABLED:
warnings.warn(
"Reading an unversioned (pre-migration) pickle cache entry. ...",
DeprecationWarning, stacklevel=4,
)
return pickle.loads(raw) # noqa: S301
raise ValueError(f"Unknown cache payload format prefix: {prefix!r}")Or alternatively, update Option B in the migration doc to say it only works for data re-written by the new code, and recommend Option A (flush) for existing deployments. The same logic likely applies to CosmosDB. The beta Redis serializer (pub/sub) is transient so no migration concern there. Everything else looks great — happy to approve once this edge case is addressed. |
marklysze
left a comment
There was a problem hiding this comment.
Strong security fix for a real RCE vector. The version-prefix approach (\x01 = JSON, \x00 = legacy pickle) is clean — it's backward compatible for existing caches while defaulting all new writes to JSON.
The design is correct:
- New writes: always JSON regardless of env var
- Legacy pickle reads: blocked by default, opt-in via
AG2_ALLOW_PICKLE_CACHE_READ=1with aDeprecationWarning - Migration path:
teachability_migrate_pickle_to_json.py+docs/cache-migration.mdgive operators a clear upgrade story - All five sinks (Redis cache, Cosmos DB cache, Redis pub/sub serializer, teachability) covered consistently
CI passes on the latest run. ✓
Addresses marklysze's review feedback on ag2ai#2709: existing Redis/CosmosDB cache entries from old code lack the \x01 (JSON) or \x00 (legacy) prefix. AG2_ALLOW_PICKLE_CACHE_READ=1 now reads such payloads via pickle.loads(raw) with a DeprecationWarning. Without the env var, ValueError still raised. Migration doc Option B now actually works for unversioned data.
|
@marklysze Thanks for the review. Pushed the fallback in
Ready for merge if no other concerns. |
|
Thanks @amabito! @Lancetnik can you please check over? |
|
Hi @amabito — this PR now has merge conflicts with Please rebase onto the current ⏳ If there's no update within 30 days, this PR will be marked stale and closed 7 days after that. |
Why are these changes needed?
autogen/cache/redis_cache.py:77ispickle.loads(cache.get(...)). That is thedefault production cache path -- anything that can write to your Redis instance
gets RCE on every AG2 process using it. Four sibling sinks had the same shape:
pickle.loadsin the Cosmos DB cache,pickle.loadson inbound Redis pub/submessages,
pickle.loadfrom a caller-suppliedpath_to_db_dirin teachability(RCE on the next agent startup if you can drop a file there), and
importlib.import_moduleon the__type__field arriving over Redis pub/subJSON -- a channel writer can trigger arbitrary import-time side effects with any
importable path. All five are fixed here because splitting the rollout would just
extend the exposure window.
JSON replaces pickle everywhere. Cache stores now write a one-byte version prefix
(
\x01+ JSON). Legacy pickle reads still work, but only whenAG2_ALLOW_PICKLE_CACHE_READ=1is set -- they log a DeprecationWarning on everyhit, noisy by design. Without the flag, the reader raises. The importlib resolver
is gone; a module-level
_EVENT_REGISTRYdict takes its place. CustomBaseEventsubclasses published over Redis pub/sub need@register_event_class;the rejection test in
test_serializer_security.pydemonstrates what happenswhen an unregistered
__type__arrives.Breaking changes: existing
.pklcache entries and teachability stores needmigration. A migration helper ships with this PR
(
python -m autogen.agentchat.contrib.capabilities.teachability_migrate_pickle_to_json).Stub docs at
docs/security/deserialization.mdanddocs/cache-migration.md;website integration is a follow-up.
Related issue number
Addresses: N/A (proactive security audit of deserialization paths)
Checks
(Stub docs added to
docs/security/deserialization.mdanddocs/cache-migration.md; website integration is a follow-up.)