Restrict pickle deserialization in CookieJar.load()#12091
Restrict pickle deserialization in CookieJar.load()#12091Dreamsorcerer merged 15 commits intoaio-libs:masterfrom
Conversation
CookieJar.load() previously used pickle.load() without restrictions, allowing arbitrary code execution if an attacker could write to the cookie jar file (e.g. shared volumes, NFS, multi-tenant environments). This commit: - Adds _RestrictedCookieUnpickler that only allows cookie-related types (SimpleCookie, Morsel, defaultdict, and safe builtins) - Replaces pickle.load() with the restricted unpickler in load() - Adds safe JSON-based save_json()/load_json() alternatives - Adds security warnings to the documentation for save()/load() - Adds tests verifying malicious payloads are blocked while legitimate cookies continue to load correctly Existing pickle files with legitimate cookies are fully backward compatible — only payloads containing non-cookie types (os.system, eval, exec, subprocess, etc.) are rejected. Co-Authored-By: Claude Opus 4.6 <[email protected]>
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12091 +/- ##
==========================================
+ Coverage 98.71% 98.77% +0.05%
==========================================
Files 128 128
Lines 44907 45045 +138
Branches 2383 2396 +13
==========================================
+ Hits 44332 44495 +163
+ Misses 408 390 -18
+ Partials 167 160 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Add "partitioned" to the boolean attribute check in load_json() so it is restored as True (bool) rather than "true" (string) on Python 3.14+ where Morsel._reserved includes partitioned. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Add type: ignore[no-any-return] on _RestrictedCookieUnpickler.find_class - Add type: ignore[attr-defined] on Morsel._reserved access - Remove unused type: ignore[assignment] in load_json - Fix __reduce__ return type annotations in test payloads - Fix partitioned cookie test to compare attributes directly instead of via SimpleCookie equality (avoids Morsel re-encoding) Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add deserialization, NFS, payloads, unpickler to the spelling wordlist for the new CookieJar security documentation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
save() now writes JSON format directly. load() tries JSON first and falls back to restricted pickle for backward compatibility with existing cookie files. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-authored-by: Sam Bull <[email protected]>
- Remove str() conversion for morsel attributes (already strings) - _load_json_data() returns cookies instead of assigning internally - Remove defensive else/defaults in JSON parsing (let invalid data fail) - Use tuple unpacking for domain|path split Co-Authored-By: Claude Opus 4.6 <[email protected]>
Instead of converting bool to "true" string for JSON storage and back on load, just store the native bool values directly. JSON handles booleans natively, so no conversion is needed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…om/YuvalElbar6/aiohttp into security/cookiejar-restrict-pickle
Backport to 3.14: 💚 backport PR created✅ Backport PR branch: Backported as #12105 🤖 @patchback |
--------- Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit 8a631e7)
|
@YuvalElbar6 Could you submit a followup PR to master that removes the pickle loading, and any references to pickling in the docs? |
|
@Dreamsorcerer yes I will |
Follow-up to aio-libs#12091. Remove mentions of pickle format from the CookieJar.load() docstring and the client_reference docs. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…n CookieJar.load() (#12105) **This is a backport of PR #12091 as merged into master (8a631e7).** --------- Co-authored-by: Yuval Elbar <[email protected]> Co-authored-by: Sam Bull <[email protected]>
Changes
_RestrictedCookieUnpickler— Apickle.Unpicklersubclass that only allows cookie-related types (SimpleCookie,Morsel,defaultdict, and safe builtins). All other types (e.g.os.system,eval,exec,subprocess) raiseUnpicklingError.CookieJar.load()— Now uses_RestrictedCookieUnpicklerinstead of barepickle.load(). Fully backward compatible with existing pickle files containing legitimate cookies.CookieJar.save_json()/load_json()— New safe JSON-based alternatives for cookie persistence, immune to deserialization attacks by design.Documentation — Added
.. warning::directives tosave()/load()and documented the newsave_json()/load_json()methods.Tests — Added tests verifying:
os.system,eval,subprocess.callpayloads are blockedTest plan
os.system,eval,exec,subprocess.call,os.popen) are all rejected withUnpicklingErrortest_pickle_formatregression test data loads through restricted unpicklersave_json/load_jsonroundtrip preserves all cookie attributes (domain, path, secure, httponly, expires, max-age, samesite, partitioned)🤖 Generated with Claude Code