security: replace pickle.loads with RestrictedUnpickler (CWE-502)#691
Open
taylorodell wants to merge 2 commits into
Open
security: replace pickle.loads with RestrictedUnpickler (CWE-502)#691taylorodell wants to merge 2 commits into
taylorodell wants to merge 2 commits into
Conversation
Replace all pickle.loads() / pickle.load() calls in the deserialization path with a SafeUnpickler that restricts deserialized types to a known allowlist of snooty, docutils, and builtin types. This prevents arbitrary code execution via __reduce__ gadgets in malicious cache files, whether delivered via: - Local filesystem (e.g., committed in a PR to a docs repo) - HTTP download from the cache_url_prefix S3 bucket The SafeUnpickler blocks types like os.system, subprocess.Popen, and any other module outside snooty/docutils — the standard pickle RCE gadget chains all require importing from os, subprocess, or similar modules. Write-side pickle.dumps() calls are unchanged — they only serialize known snooty types. CWE-502: Deserialization of Untrusted Data Ref: https://docs.python.org/3/library/pickle.html#restricting-globals
- Move safe_unpickler imports to the local imports section (isort compliance) - Add test_safe_unpickler.py with tests for: - Blocking os.system gadgets - Blocking subprocess gadgets - Allowing builtin types - Allowing snooty CacheData types - Gzip-compressed payload handling
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.
Ticket
N/A — External security contribution (CWE-502 remediation)
Notes
Problem: All
pickle.loads()/pickle.load()calls in the deserialization path accept arbitrary types, enabling remote code execution via__reduce__gadgets in crafted cache files.Two input paths reach the vulnerable deserialization:
.snooty-*.cache.gzfiles in the project directorycache_url_prefix(S3 bucket inrstspec.toml) and deserializes the responseFix: Introduces
snooty/safe_unpickler.pywith aSafeUnpicklerthat restricts deserialized types to a known allowlist:snooty.*,docutils.*, and standard builtins (dict,list,tuple,str,int, etc.)os.system,subprocess.Popen,builtins.eval, and all standard pickle RCE gadget chainsAll four
pickle.loads()call sites are replaced withsafe_loads():parse_cache.pyread_from_bytes()(line 140)parse_cache.pyCacheData.get()(line 90)gizaparser/domain.py(line 95)page_database.py(line 199)Write-side
pickle.dumps()calls are unchanged.Tests added in
snooty/test_safe_unpickler.py:os.systemgadgets ✓subprocessgadgets ✓dict,list,str) ✓CacheDatatype ✓References:
README updates