[PR #12091/8a631e74 backport][3.14] Restrict pickle deserialization in 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]>

Author: 3 people

CHANGES/12091.bugfix.rst

@@ -0,0 +1,6 @@
+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`.

aiohttp/cookiejar.py

@@ -4,6 +4,7 @@
 import datetime
 import heapq
 import itertools
+import json
 import os  # noqa
 import pathlib
 import pickle
@@ -38,6 +39,41 @@
 _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."""
 
@@ -112,14 +148,84 @@ def quote_cookie(self) -> bool:
         return self._quote_cookie
 
     def save(self, file_path: PathLike) -> None:
+        """Save cookies to a file using JSON format.
+
+        :param file_path: Path to file where cookies will be serialized,
+            :class:`str` or :class:`pathlib.Path` instance.
+        """
         file_path = pathlib.Path(file_path)
-        with file_path.open(mode="wb") as f:
-            pickle.dump(self._cookies, f, pickle.HIGHEST_PROTOCOL)
+        data: dict[str, dict[str, dict[str, str | bool]]] = {}
+        for (domain, path), cookie in self._cookies.items():
+            key = f"{domain}|{path}"
+            data[key] = {}
+            for name, morsel in cookie.items():
+                morsel_data: dict[str, str | bool] = {
+                    "key": morsel.key,
+                    "value": morsel.value,
+                    "coded_value": morsel.coded_value,
+                }
+                # Save all morsel attributes that have values
+                for attr in morsel._reserved:  # type: ignore[attr-defined]
+                    attr_val = morsel[attr]
+                    if attr_val:
+                        morsel_data[attr] = attr_val
+                data[key][name] = morsel_data
+        with file_path.open(mode="w", encoding="utf-8") as f:
+            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.
+
+        :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)
-        with file_path.open(mode="rb") as f:
-            self._cookies = pickle.load(f)
+        # 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()
+
+    def _load_json_data(
+        self, data: dict[str, dict[str, dict[str, str | bool]]]
+    ) -> defaultdict[tuple[str, str], SimpleCookie]:
+        """Load cookies from parsed JSON data."""
+        cookies: defaultdict[tuple[str, str], SimpleCookie] = defaultdict(SimpleCookie)
+        for compound_key, cookie_data in data.items():
+            domain, path = compound_key.split("|", 1)
+            key = (domain, path)
+            for name, morsel_data in cookie_data.items():
+                morsel: Morsel[str] = Morsel()
+                morsel_key = morsel_data["key"]
+                morsel_value = morsel_data["value"]
+                morsel_coded_value = morsel_data["coded_value"]
+                # Use __setstate__ to bypass validation, same pattern
+                # used in _build_morsel and _cookie_helpers.
+                morsel.__setstate__(  # type: ignore[attr-defined]
+                    {
+                        "key": morsel_key,
+                        "value": morsel_value,
+                        "coded_value": morsel_coded_value,
+                    }
+                )
+                # Restore morsel attributes
+                for attr in morsel._reserved:  # type: ignore[attr-defined]
+                    if attr in morsel_data and attr not in (
+                        "key",
+                        "value",
+                        "coded_value",
+                    ):
+                        morsel[attr] = morsel_data[attr]
+                cookies[key][name] = morsel
+        return cookies
 
     def clear(self, predicate: ClearCookiePredicate | None = None) -> None:
         if predicate is None:

docs/client_reference.rst

@@ -2496,16 +2496,28 @@ Utilities
 
    .. method:: save(file_path)
 
-      Write a pickled representation of cookies into the file
+      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 a pickled representation of cookies from the file
-      at provided 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.
 
       :param file_path: Path to file from where cookies will be
            imported, :class:`str` or :class:`pathlib.Path` instance.

docs/spelling_wordlist.txt

@@ -99,6 +99,7 @@ deduplicate
 defs
 Dependabot
 deprecations
+deserialization
 DER
 dev
 Dev
@@ -213,7 +214,8 @@ Multipart
 musllinux
 mypy
 Nagle
-Nagle’s
+Nagle's
+NFS
 namedtuple
 nameservers
 namespace
@@ -236,6 +238,7 @@ param
 params
 parsers
 pathlib
+payloads
 peername
 performant
 pickleable
@@ -356,6 +359,8 @@ unhandled
 unicode
 unittest
 Unittest
+unpickler
+untrusted
 unix
 unobvious
 unsets

tests/conftest.py

@@ -95,6 +95,11 @@ def blockbuster(request: pytest.FixtureRequest) -> Iterator[None]:
             bb.functions[func].can_block_in(
                 "aiohttp/web_urldispatcher.py", "add_static"
             )
+        # save/load is not async, so we must allow this:
+        for func in ("io.TextIOWrapper.read", "io.BufferedReader.read"):
+            bb.functions[func].can_block_in("aiohttp/cookiejar.py", "load")
+        for func in ("io.TextIOWrapper.write", "io.BufferedWriter.write"):
+            bb.functions[func].can_block_in("aiohttp/cookiejar.py", "save")
         # Note: coverage.py uses locking internally which can cause false positives
         # in blockbuster when it instruments code. This is particularly problematic
         # on Windows where it can lead to flaky test failures.