diff --git a/CHANGES/12091.bugfix.rst b/CHANGES/12091.bugfix.rst deleted file mode 100644 index 45ffbc3557f..00000000000 --- a/CHANGES/12091.bugfix.rst +++ /dev/null @@ -1,6 +0,0 @@ -Switched :py:meth:`~aiohttp.CookieJar.save` to use JSON format and -:py:meth:`~aiohttp.CookieJar.load` to try JSON first with a fallback to -a restricted pickle unpickler that only allows cookie-related types -(``SimpleCookie``, ``Morsel``, ``defaultdict``, etc.), preventing -arbitrary code execution via malicious pickle payloads -(CWE-502) -- by :user:`YuvalElbar6`. diff --git a/CHANGES/12111.breaking.rst b/CHANGES/12111.breaking.rst new file mode 100644 index 00000000000..469b1d5cd22 --- /dev/null +++ b/CHANGES/12111.breaking.rst @@ -0,0 +1,3 @@ +Removed legacy pickle loading from :py:meth:`~aiohttp.CookieJar.load`. +:py:meth:`~aiohttp.CookieJar.load` now exclusively uses JSON format +-- by :user:`YuvalElbar6`. diff --git a/aiohttp/cookiejar.py b/aiohttp/cookiejar.py index 3ba7de6869b..3d371eeff1e 100644 --- a/aiohttp/cookiejar.py +++ b/aiohttp/cookiejar.py @@ -4,9 +4,7 @@ import heapq import itertools import json -import os # noqa import pathlib -import pickle import re import time import warnings @@ -38,41 +36,6 @@ _SIMPLE_COOKIE = SimpleCookie() -class _RestrictedCookieUnpickler(pickle.Unpickler): - """A restricted unpickler that only allows cookie-related types. - - This prevents arbitrary code execution when loading pickled cookie data - from untrusted sources. Only types that are expected in a serialized - CookieJar are permitted. - - See: https://docs.python.org/3/library/pickle.html#restricting-globals - """ - - _ALLOWED_CLASSES: frozenset[tuple[str, str]] = frozenset( - { - # Core cookie types - ("http.cookies", "SimpleCookie"), - ("http.cookies", "Morsel"), - # Container types used by CookieJar._cookies - ("collections", "defaultdict"), - # builtins that pickle uses for reconstruction - ("builtins", "tuple"), - ("builtins", "set"), - ("builtins", "frozenset"), - ("builtins", "dict"), - } - ) - - def find_class(self, module: str, name: str) -> type: - if (module, name) not in self._ALLOWED_CLASSES: - raise pickle.UnpicklingError( - f"Forbidden class: {module}.{name}. " - "CookieJar.load() only allows cookie-related types for security. " - "See https://docs.python.org/3/library/pickle.html#restricting-globals" - ) - return super().find_class(module, name) # type: ignore[no-any-return] - - class CookieJar(AbstractCookieJar): """Implements cookie storage adhering to RFC 6265.""" @@ -174,25 +137,15 @@ def save(self, file_path: PathLike) -> None: json.dump(data, f, indent=2) def load(self, file_path: PathLike) -> None: - """Load cookies from a file. - - Tries to load JSON format first. Falls back to loading legacy - pickle format (using a restricted unpickler) for backward - compatibility with existing cookie files. + """Load cookies from a JSON file. :param file_path: Path to file from where cookies will be imported, :class:`str` or :class:`pathlib.Path` instance. """ file_path = pathlib.Path(file_path) - # Try JSON format first - try: - with file_path.open(mode="r", encoding="utf-8") as f: - data = json.load(f) - self._cookies = self._load_json_data(data) - except (json.JSONDecodeError, UnicodeDecodeError, ValueError): - # Fall back to legacy pickle format with restricted unpickler - with file_path.open(mode="rb") as f: - self._cookies = _RestrictedCookieUnpickler(f).load() + with file_path.open(mode="r", encoding="utf-8") as f: + data = json.load(f) + self._cookies = self._load_json_data(data) def _load_json_data( self, data: dict[str, dict[str, dict[str, str | bool]]] @@ -495,8 +448,7 @@ def _build_morsel(self, cookie: Morsel[str]) -> Morsel[str]: coded_value = value = cookie.value # We use __setstate__ instead of the public set() API because it allows us to # bypass validation and set already validated state. This is more stable than - # setting protected attributes directly and unlikely to change since it would - # break pickling. + # setting protected attributes directly. morsel.__setstate__({"key": cookie.key, "value": value, "coded_value": coded_value}) # type: ignore[attr-defined] return morsel diff --git a/docs/client_reference.rst b/docs/client_reference.rst index 033dfb53ca7..d731ac9bfd0 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -2498,25 +2498,12 @@ Utilities Write a JSON representation of cookies into the file at provided path. - .. versionchanged:: 3.14 - - Previously used pickle format. Now uses JSON for safe - serialization. - :param file_path: Path to file where cookies will be serialized, :class:`str` or :class:`pathlib.Path` instance. .. method:: load(file_path) - Load cookies from the file at provided path. Tries JSON format - first, then falls back to legacy pickle format (using a restricted - unpickler that only allows cookie-related types) for backward - compatibility with existing cookie files. - - .. versionchanged:: 3.14 - - Now loads JSON format by default. Falls back to restricted - pickle for files saved by older versions. + Load cookies from a JSON file at the provided path. :param file_path: Path to file from where cookies will be imported, :class:`str` or :class:`pathlib.Path` instance. diff --git a/tests/test_cookiejar.py b/tests/test_cookiejar.py index 24c1f73284c..f028f57d7a8 100644 --- a/tests/test_cookiejar.py +++ b/tests/test_cookiejar.py @@ -2,8 +2,6 @@ import heapq import itertools import logging -import pickle -import sys from http.cookies import BaseCookie, Morsel, SimpleCookie from operator import not_ from pathlib import Path @@ -17,13 +15,6 @@ from aiohttp.typedefs import LooseCookies -def dump_cookiejar() -> bytes: # pragma: no cover - """Create pickled data for test_pickle_format().""" - cj = CookieJar() - cj.update_cookies(_cookies_to_send()) - return pickle.dumps(cj._cookies, pickle.HIGHEST_PROTOCOL) - - def _cookies_to_send() -> SimpleCookie: return SimpleCookie( "shared-cookie=first; " @@ -1027,41 +1018,6 @@ async def test_cookie_jar_clear_domain() -> None: next(iterator) -def test_pickle_format(cookies_to_send: SimpleCookie) -> None: - """Test if cookiejar pickle format breaks. - - If this test fails, it may indicate that saved cookiejars will stop working. - If that happens then: - 1. Avoid releasing the change in a bugfix release. - 2. Try to include a migration script in the release notes (example below). - 3. Use dump_cookiejar() at the top of this file to update `pickled`. - - Depending on the changes made, a migration script might look like: - import pickle - with file_path.open("rb") as f: - cookies = pickle.load(f) - - morsels = [(name, m) for c in cookies.values() for name, m in c.items()] - cookies.clear() - for name, m in morsels: - cookies[(m["domain"], m["path"])][name] = m - - with file_path.open("wb") as f: - pickle.dump(cookies, f, pickle.HIGHEST_PROTOCOL) - """ - if sys.version_info < (3, 14): - pickled = b"\x80\x04\x95\xc8\x0b\x00\x00\x00\x00\x00\x00\x8c\x0bcollections\x94\x8c\x0bdefaultdict\x94\x93\x94\x8c\x0chttp.cookies\x94\x8c\x0cSimpleCookie\x94\x93\x94\x85\x94R\x94(\x8c\x00\x94h\x08\x86\x94h\x05)\x81\x94\x8c\rshared-cookie\x94h\x03\x8c\x06Morsel\x94\x93\x94)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94\x8c\x01/\x94\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\x08\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(\x8c\x03key\x94h\x0b\x8c\x05value\x94\x8c\x05first\x94\x8c\x0bcoded_value\x94h\x1cubs\x8c\x0bexample.com\x94h\x08\x86\x94h\x05)\x81\x94(\x8c\rdomain-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\x1e\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah!h\x1b\x8c\x06second\x94h\x1dh-ub\x8c\x14dotted-domain-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94\x8c\x0bexample.com\x94\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah.h\x1b\x8c\x05fifth\x94h\x1dh;ubu\x8c\x11test1.example.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x11subdomain1-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h<\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah?h\x1b\x8c\x05third\x94h\x1dhKubs\x8c\x11test2.example.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x11subdomain2-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94hL\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ahOh\x1b\x8c\x06fourth\x94h\x1dh[ubs\x8c\rdifferent.org\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x17different-domain-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\\\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah_h\x1b\x8c\x05sixth\x94h\x1dhkubs\x8c\nsecure.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\rsecure-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94hl\x8c\x07max-age\x94h\x08\x8c\x06secure\x94\x88\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ahoh\x1b\x8c\x07seventh\x94h\x1dh{ubs\x8c\x0cpathtest.com\x94h\x08\x86\x94h\x05)\x81\x94(\x8c\x0eno-path-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h|\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\x7fh\x1b\x8c\x06eighth\x94h\x1dh\x8bub\x8c\x0cpath1-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94\x8c\x0cpathtest.com\x94\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\x8ch\x1b\x8c\x05ninth\x94h\x1dh\x99ubu\x8c\x0cpathtest.com\x94\x8c\x04/one\x94\x86\x94h\x05)\x81\x94\x8c\x0cpath2-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x9b\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\x9a\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\x9eh\x1b\x8c\x05tenth\x94h\x1dh\xaaubs\x8c\x0cpathtest.com\x94\x8c\x08/one/two\x94\x86\x94h\x05)\x81\x94(\x8c\x0cpath3-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\xac\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\xab\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\xafh\x1b\x8c\x08eleventh\x94h\x1dh\xbbub\x8c\x0cpath4-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94\x8c\t/one/two/\x94\x8c\x07comment\x94h\x08\x8c\x06domain\x94\x8c\x0cpathtest.com\x94\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\xbch\x1b\x8c\x07twelfth\x94h\x1dh\xcaubu\x8c\x0fexpirestest.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x0eexpires-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94\x8c\x1cTue, 1 Jan 2999 12:00:00 GMT\x94\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\xcb\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\xceh\x1b\x8c\nthirteenth\x94h\x1dh\xdbubs\x8c\x0emaxagetest.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x0emax-age-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\xdc\x8c\x07max-age\x94\x8c\x0260\x94\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\xdfh\x1b\x8c\nfourteenth\x94h\x1dh\xecubs\x8c\x12invalid-values.com\x94h\x08\x86\x94h\x05)\x81\x94(\x8c\x16invalid-max-age-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\xed\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\xf0h\x1b\x8c\tfifteenth\x94h\x1dh\xfcub\x8c\x16invalid-expires-cookie\x94h\r)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94h\x11\x8c\x07comment\x94h\x08\x8c\x06domain\x94\x8c\x12invalid-values.com\x94\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08u}\x94(h\x1ah\xfdh\x1b\x8c\tsixteenth\x94h\x1dj\n\x01\x00\x00ubuu." - else: - pickled = b'\x80\x05\x95\x06\x08\x00\x00\x00\x00\x00\x00\x8c\x0bcollections\x94\x8c\x0bdefaultdict\x94\x93\x94\x8c\x0chttp.cookies\x94\x8c\x0cSimpleCookie\x94\x93\x94\x85\x94R\x94(\x8c\x00\x94h\x08\x86\x94h\x05)\x81\x94\x8c\rshared-cookie\x94h\x03\x8c\x06Morsel\x94\x93\x94)\x81\x94(\x8c\x07expires\x94h\x08\x8c\x04path\x94\x8c\x01/\x94\x8c\x07comment\x94h\x08\x8c\x06domain\x94h\x08\x8c\x07max-age\x94h\x08\x8c\x06secure\x94h\x08\x8c\x08httponly\x94h\x08\x8c\x07version\x94h\x08\x8c\x08samesite\x94h\x08\x8c\x0bpartitioned\x94h\x08u}\x94(\x8c\x03key\x94h\x0b\x8c\x05value\x94\x8c\x05first\x94\x8c\x0bcoded_value\x94h\x1dubs\x8c\x0bexample.com\x94h\x08\x86\x94h\x05)\x81\x94(\x8c\rdomain-cookie\x94h\r)\x81\x94(h\x0fh\x08h\x10h\x11h\x12h\x08h\x13h\x1fh\x14h\x08h\x15h\x08h\x16h\x08h\x17h\x08h\x18h\x08h\x19h\x08u}\x94(h\x1bh"h\x1c\x8c\x06second\x94h\x1eh%ub\x8c\x14dotted-domain-cookie\x94h\r)\x81\x94(h\x0fh\x08h\x10h\x11h\x12h\x08h\x13\x8c\x0bexample.com\x94h\x14h\x08h\x15h\x08h\x16h\x08h\x17h\x08h\x18h\x08h\x19h\x08u}\x94(h\x1bh&h\x1c\x8c\x05fifth\x94h\x1eh*ubu\x8c\x11test1.example.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x11subdomain1-cookie\x94h\r)\x81\x94(h\x0fh\x08h\x10h\x11h\x12h\x08h\x13h+h\x14h\x08h\x15h\x08h\x16h\x08h\x17h\x08h\x18h\x08h\x19h\x08u}\x94(h\x1bh.h\x1c\x8c\x05third\x94h\x1eh1ubs\x8c\x11test2.example.com\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x11subdomain2-cookie\x94h\r)\x81\x94(h\x0fh\x08h\x10h\x11h\x12h\x08h\x13h2h\x14h\x08h\x15h\x08h\x16h\x08h\x17h\x08h\x18h\x08h\x19h\x08u}\x94(h\x1bh5h\x1c\x8c\x06fourth\x94h\x1eh8ubs\x8c\rdifferent.org\x94h\x08\x86\x94h\x05)\x81\x94\x8c\x17different-domain-cookie\x94h\r)\x81\x94(h\x0fh\x08h\x10h\x11h\x12h\x08h\x13h9h\x14h\x08h\x15h\x08h\x16h\x08h\x17h\x08h\x18h\x08h\x19h\x08u}\x94(h\x1bh None: assert "universal" in jar._morsel_cache[("", "")] -# === Security tests for restricted unpickler and JSON save/load === - - -def test_load_rejects_malicious_pickle(tmp_path: Path) -> None: - """Verify CookieJar.load() blocks arbitrary code execution via pickle. - - A crafted pickle payload using os.system (or any non-cookie class) - must be rejected by the restricted unpickler. - """ - import os - - file_path = tmp_path / "malicious.pkl" - - class RCEPayload: - def __reduce__(self) -> tuple[object, ...]: - return (os.system, ("echo PWNED",)) - - with open(file_path, "wb") as f: - pickle.dump(RCEPayload(), f, pickle.HIGHEST_PROTOCOL) - - jar = CookieJar() - with pytest.raises(pickle.UnpicklingError, match="Forbidden class"): - jar.load(file_path) - - -def test_load_rejects_eval_payload(tmp_path: Path) -> None: - """Verify CookieJar.load() blocks eval-based pickle payloads.""" - file_path = tmp_path / "eval_payload.pkl" - - class EvalPayload: - def __reduce__(self) -> tuple[object, ...]: - return (eval, ("__import__('os').system('echo PWNED')",)) - - with open(file_path, "wb") as f: - pickle.dump(EvalPayload(), f, pickle.HIGHEST_PROTOCOL) - - jar = CookieJar() - with pytest.raises(pickle.UnpicklingError, match="Forbidden class"): - jar.load(file_path) - - -def test_load_rejects_subprocess_payload(tmp_path: Path) -> None: - """Verify CookieJar.load() blocks subprocess-based pickle payloads.""" - import subprocess - - file_path = tmp_path / "subprocess_payload.pkl" - - class SubprocessPayload: - def __reduce__(self) -> tuple[object, ...]: - return (subprocess.call, (["echo", "PWNED"],)) - - with open(file_path, "wb") as f: - pickle.dump(SubprocessPayload(), f, pickle.HIGHEST_PROTOCOL) - - jar = CookieJar() - with pytest.raises(pickle.UnpicklingError, match="Forbidden class"): - jar.load(file_path) - - -def test_load_falls_back_to_pickle( - tmp_path: Path, - cookies_to_receive: SimpleCookie, -) -> None: - """Verify load() falls back to restricted pickle for legacy cookie files. - - Existing cookie files saved with older versions of aiohttp used pickle. - load() should detect that the file is not JSON and fall back to the - restricted pickle unpickler for backward compatibility. - """ - file_path = tmp_path / "legit.pkl" - - # Write a legacy pickle file directly (as old aiohttp save() would) - jar_save = CookieJar() - jar_save.update_cookies(cookies_to_receive) - with file_path.open(mode="wb") as f: - pickle.dump(jar_save._cookies, f, pickle.HIGHEST_PROTOCOL) - - jar_load = CookieJar() - jar_load.load(file_path=file_path) - - jar_test = SimpleCookie() - for cookie in jar_load: - jar_test[cookie.key] = cookie - - assert jar_test == cookies_to_receive - - def test_save_load_json_roundtrip( tmp_path: Path, cookies_to_receive: SimpleCookie,