Harden read path against RCE via unconditional pickle deserialization by devin-ai-integration[bot] · Pull Request #9 · COG-GTM/ArcticDB

devin-ai-integration · 2026-07-03T02:49:04Z

Reference Issues/PRs

Fixes a security finding: RCE via unconditional pickle deserialization of stored symbols/metadata on read in python/arcticdb/version_store/_normalization.py.

What does this implement or fix?

ArcticDB stores non-normalizable Python objects and user-defined metadata by pickling them and wrapping the bytes in a msgpack ExtType (PY_PICKLE_2/PY_PICKLE_3). On read, MsgPackNormalizer._ext_hook unconditionally called Pickler.read → pd.read_pickle → pickle.load, executing any __reduce__ payload embedded in storage. Anyone able to write a symbol or its metadata to a library a victim later reads could therefore run arbitrary code in the victim's process. strict_mode only blocked writing new pickles — there was no opt-out on the read side.

This PR gates the read-side pickle path behind an explicit, default-off opt-in (finding recommendation option 1).

_ext_hook now calls _check_pickle_read_allowed() before unpickling a PY_PICKLE_* payload; when pickle reads are not enabled it raises the new UnsafePickleReadError instead of executing the payload. This gates every read path that flows through _msgpack_unpackb — whole-symbol denormalize (MsgPackNormalizer.denormalize) and denormalize_user_metadata.

Opt-in (only when every writer to the library is trusted), highest precedence first:

Mechanism	Scope
`MsgPackNormalizer.allow_pickle_reads = True`	per-instance override
`arcticdb.set_allow_pickle_reads(True)` (`None` defers to env)	process-wide
`ARCTICDB_ALLOW_PICKLE_READS=1` env var	process-wide default

Pseudocode of the gate:

def _ext_hook(self, code, data):
    ...
    if code in (PY_PICKLE_2, PY_PICKLE_3):
        self._check_pickle_read_allowed()   # raises UnsafePickleReadError unless opted in
        return Pickler.read(...)

def _pickle_reads_allowed(self):
    return self.allow_pickle_reads if self.allow_pickle_reads is not None else allow_pickle_reads()

Reading natively-stored (non-pickled) data is unaffected.

Any other comments?

Behaviour change (intentional): reading a pickled symbol or pickled user-defined metadata now raises UnsafePickleReadError by default. Readers who trust their storage must opt in via one of the mechanisms above. Non-pickled reads are unchanged.
The test suite reads back its own trusted pickled data, so an autouse fixture in python/tests/conftest.py enables pickle reads for existing tests. The new default-off behaviour and each opt-in mechanism are covered by dedicated tests in test_normalization.py.
Verified end-to-end against a real LMDB library (write_pickle + pickled metadata): reads are blocked by default with UnsafePickleReadError and succeed after opting in via the setter, env var, and per-instance flag; plain DataFrame reads keep working with pickle reads disabled.
Docs updated: FAQ pickling section (user-facing warning) and docs/claude/python/NORMALIZATION.md.

Checklist

Checklist for code changes...

Have you updated the relevant docstrings, documentation and copyright notice?
Is this contribution tested against all ArcticDB's features?
Do all exceptions introduced raise appropriate error messages?
Are API changes highlighted in the PR description?
Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

Link to Devin session: https://app.devin.ai/sessions/73396622f67e4ef3b196970477e6a643
Requested by: @kevinlrd

Devin Review

Status	Commit
⚪ Not started	—

Run Devin Review

ArcticDB stores non-normalizable objects and user-defined metadata as pickle bytes wrapped in a msgpack ExtType. On read, MsgPackNormalizer._ext_hook unconditionally unpickled them via pd.read_pickle, executing any __reduce__ payload embedded in storage. A writer to shared storage could therefore run arbitrary code in a reader's process. Gate the read-side pickle path behind an explicit, default-off opt-in: _ext_hook now raises UnsafePickleReadError for PY_PICKLE_* codes unless pickle reads have been enabled via arcticdb.set_allow_pickle_reads(True), the ARCTICDB_ALLOW_PICKLE_READS environment variable, or a per-instance override. This covers both whole-symbol reads and denormalize_user_metadata. Co-Authored-By: Kevin Lee <kevin.lee@cognition.ai>

devin-ai-integration · 2026-07-03T02:49:07Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment, CI, and merge conflict monitoring

devin-ai-integration

Devin Review found 4 potential issues.

devin-ai-integration · 2026-07-07T03:28:24Z

📝 Info: Module-level _msgpack_metadata singleton correctly participates in the read gate

The _msgpack_metadata singleton (_normalization.py:1778) is created at module load with allow_pickle_reads = None. When denormalize_user_metadata calls _msgpack_metadata._msgpack_unpackb(...), the _ext_hook callback on this singleton will call _check_pickle_read_allowed(), which defers to the global allow_pickle_reads() function. This means user-metadata deserialization is correctly gated without needing to thread a per-call parameter through the existing denormalize_user_metadata API.

(Refers to lines 1835-1839)

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-07-07T03:28:25Z

+    def _pickle_reads_allowed(self):
+        # Instance override takes priority, otherwise defer to the global setting.
+        return self.allow_pickle_reads if self.allow_pickle_reads is not None else allow_pickle_reads()


📝 Info: Instance attribute shadows module-level function name

The MsgPackNormalizer instance attribute self.allow_pickle_reads (_normalization.py:1415) has the same name as the module-level function allow_pickle_reads() (_normalization.py:1400). In _pickle_reads_allowed (_normalization.py:1499-1501), both are referenced: self.allow_pickle_reads for the instance attribute and bare allow_pickle_reads() for the function. This works correctly due to Python's scoping rules (attribute access vs name lookup), but the naming overlap could confuse future maintainers reading the code.

Was this helpful? React with 👍 or 👎 to provide feedback.

Acknowledged — this is intentional and works correctly (attribute access vs. name lookup). The per-instance allow_pickle_reads attribute and the module-level allow_pickle_reads() function are deliberately named to read as the same concept at different scopes (instance override vs. global default). I'll leave the names as-is to keep the public per-instance override and the documented helper consistent, rather than introduce churn for a purely cosmetic overlap.

devin-ai-integration · 2026-07-07T03:28:26Z

+def _pack_pickle_payload():
+    norm = MsgPackNormalizer()
+    norm.allow_pickle_reads = True
+    return norm._msgpack_packb(errors.BoundaryError("bananas"))


📝 Info: Test helper sets allow_pickle_reads on a write-only normalizer instance

In _pack_pickle_payload(), norm.allow_pickle_reads = True is set on the normalizer used only for _msgpack_packb (writing). The allow_pickle_reads attribute is only consulted during unpacking (reading), so this assignment has no effect. It doesn't cause a test failure since the write path is gated by strict_mode/disallow_pickle (both False here), but it's misleading — it suggests the write path needs this flag.

Was this helpful? React with 👍 or 👎 to provide feedback.

Good catch — removed the misleading no-op assignment (the packer only consults strict_mode/disallow_pickle, both off here) and added a comment. Fixed in 7fb170d.

devin-ai-integration · 2026-07-07T03:28:27Z

+@pytest.fixture(autouse=True)
+def _allow_pickle_reads_in_tests():
+    from arcticdb.version_store import _normalization
+
+    _normalization.set_allow_pickle_reads(True)
+    try:
+        yield
+    finally:
+        _normalization.set_allow_pickle_reads(None)


📝 Info: Autouse fixture is function-scoped — consider session scope for performance

The _allow_pickle_reads_in_tests fixture is autouse=True with default function scope, meaning it runs setup/teardown for every single test function. Since it only sets/resets a module-level Python variable, the overhead is negligible, but a session-scoped fixture would be slightly more efficient. However, function scope is safer here because it ensures clean state between tests and allows the _pickle_reads_reset fixture to override it correctly via fixture ordering.

Was this helpful? React with 👍 or 👎 to provide feedback.

…est helper Co-Authored-By: Kevin Lee <kevin.lee@cognition.ai>

devin-ai-integration Bot commented Jul 7, 2026

View reviewed changes

test: drop misleading no-op allow_pickle_reads assignment in pickle t…

7fb170d

…est helper Co-Authored-By: Kevin Lee <kevin.lee@cognition.ai>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Harden read path against RCE via unconditional pickle deserialization#9

Harden read path against RCE via unconditional pickle deserialization#9
devin-ai-integration[bot] wants to merge 2 commits into
masterfrom
devin/1783046926-fix-pickle-read-rce

devin-ai-integration Bot commented Jul 3, 2026 •

edited by staging-devin-ai-integration Bot

Loading

Uh oh!

devin-ai-integration Bot commented Jul 3, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

Uh oh!

Conversation

devin-ai-integration Bot commented Jul 3, 2026 • edited by staging-devin-ai-integration Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

Devin Review

Uh oh!

devin-ai-integration Bot commented Jul 3, 2026

🤖 Devin AI Engineer

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

devin-ai-integration Bot commented Jul 3, 2026 •

edited by staging-devin-ai-integration Bot

Loading