diff --git a/docs/dev/rest-api/remote-sessions.md b/docs/dev/rest-api/remote-sessions.md index 54c00664..7d10c8f7 100644 --- a/docs/dev/rest-api/remote-sessions.md +++ b/docs/dev/rest-api/remote-sessions.md @@ -398,7 +398,8 @@ serve: cannot read each other's cached tokens. The server deletes the directory when the remote session ends. A remote session that uses a server account (`serve.server_accounts`) gets a persistent per-account cache instead, so the account's tokens outlive the - remote session. + remote session. The server and the client both refuse a forwarded cache file that is not + JSON or that names a type other than a date or time, and the service then authenticates again. - **The server redacts secrets in logs.** It redacts remote session IDs, service tags, and other user values before it writes them to the log. diff --git a/tests/remote/unit/test_cache_payload_safety.py b/tests/remote/unit/test_cache_payload_safety.py new file mode 100644 index 00000000..e4275673 --- /dev/null +++ b/tests/remote/unit/test_cache_payload_safety.py @@ -0,0 +1,74 @@ +import base64 +import datetime as dt +import logging +import zlib +from types import SimpleNamespace + +import pytest + +from unshackle.core.api import handlers +from unshackle.core.api.sanitize import safe_cache_payload +from unshackle.core.cacher import Cacher +from unshackle.core.config import config +from unshackle.core.remote_service import RemoteService + +pytestmark = pytest.mark.unit + +TAG = "TESTSVC" + + +def b64z(text: str) -> str: + return base64.b64encode(zlib.compress(text.encode())).decode("ascii") + + +@pytest.mark.parametrize("text", ['{"a": [1, "b", null]}', '"token"']) +def test_plain_json_is_safe(text) -> None: + assert safe_cache_payload(text) + + +@pytest.mark.parametrize( + "text", + [ + "not json", + '{"py/object": "collections.OrderedDict"}', + '{"py/type": "builtins.dict"}', + '{"py/function": "builtins.print"}', + '{"py/repr": "builtins/1"}', + '{"py/reduce": [{"py/function": "builtins.print"}, {"py/tuple": ["x"]}]}', + '{"py/object": "datetime.datetime", "__reduce__": [{"py/type": "collections.OrderedDict"}, []]}', + '{"py/object": "datetime.datetime", "py/state": {}}', + "[" * 100000 + "]" * 100000, + ], +) +def test_unsafe_payload_is_refused(text) -> None: + assert safe_cache_payload(text) is False + + +def save_returned(cache_map: dict) -> None: + RemoteService.save_returned_cache(SimpleNamespace(service_tag=TAG, log=logging.getLogger("test")), cache_map) + + +@pytest.mark.parametrize( + "write", [lambda m: handlers.write_client_cache(m, TAG), save_returned], ids=["server", "client"] +) +def test_writer_skips_unsafe_entry(tmp_path, monkeypatch, write) -> None: + monkeypatch.setattr(config.directories, "cache", tmp_path) + write({"good": b64z('{"a": 1}'), "bad": b64z('{"py/function": "builtins.print"}')}) + assert (tmp_path / TAG / "good.json").exists() + assert not (tmp_path / TAG / "bad.json").exists() + + +@pytest.mark.parametrize( + "expiration", + [None, dt.datetime(2030, 1, 1, 12, 0, 0), dt.datetime(2030, 1, 1, 12, 0, 0, tzinfo=dt.timezone.utc)], + ids=["none", "naive", "aware"], +) +def test_real_cacher_file_round_trips(tmp_path, monkeypatch, expiration) -> None: + monkeypatch.setattr(config.directories, "cache", tmp_path / "src") + cache = Cacher(TAG, "tok") + cache.set({"token": "abc", "n": [1, 2]}, expiration=expiration) + text = cache.path.read_text(encoding="utf8") + + monkeypatch.setattr(config.directories, "cache", tmp_path / "dst") + save_returned({"tok": b64z(text)}) + assert Cacher(TAG).get("tok").data == {"token": "abc", "n": [1, 2]} diff --git a/tests/remote/unit/test_remote_service_helpers.py b/tests/remote/unit/test_remote_service_helpers.py index 09225241..157eb395 100644 --- a/tests/remote/unit/test_remote_service_helpers.py +++ b/tests/remote/unit/test_remote_service_helpers.py @@ -395,20 +395,20 @@ def test_save_returned_cache_writes_nested_keys_and_rejects_escapes(tmp_path, mo RemoteService.save_returned_cache( stub, { - "tokens": blob("flat"), - "session_web/abc": blob("nested"), - "session_web\\def": blob("windows"), - "MSL/keys.v2": blob("dotted"), - "../escape": blob("bad"), - "/abs": blob("bad"), - "C:/x": blob("bad"), + "tokens": blob('"flat"'), + "session_web/abc": blob('"nested"'), + "session_web\\def": blob('"windows"'), + "MSL/keys.v2": blob('"dotted"'), + "../escape": blob('"bad"'), + "/abs": blob('"bad"'), + "C:/x": blob('"bad"'), }, ) - assert (tmp_path / tag / "tokens.json").read_text() == "flat" - assert (tmp_path / tag / "session_web" / "abc.json").read_text() == "nested" - assert (tmp_path / tag / "session_web" / "def.json").read_text() == "windows" - assert (tmp_path / tag / "MSL" / "keys.v2.json").read_text() == "dotted" + assert (tmp_path / tag / "tokens.json").read_text() == '"flat"' + assert (tmp_path / tag / "session_web" / "abc.json").read_text() == '"nested"' + assert (tmp_path / tag / "session_web" / "def.json").read_text() == '"windows"' + assert (tmp_path / tag / "MSL" / "keys.v2.json").read_text() == '"dotted"' assert not (tmp_path / "escape.json").exists() written = sorted(p.relative_to(tmp_path).as_posix() for p in tmp_path.rglob("*.json")) assert written == [ @@ -433,7 +433,7 @@ def test_save_returned_cache_drops_other_profiles_files(tmp_path, monkeypatch) - monkeypatch.setattr(config.directories, "cache", tmp_path) monkeypatch.setattr(config, "credentials", {tag: {"default": "a@example.com:pw", "work": "b@example.com:pw"}}) stub = SimpleNamespace(service_tag=tag, log=logging.getLogger("test")) - blob = base64.b64encode(zlib.compress(b"x")).decode("ascii") + blob = base64.b64encode(zlib.compress(b'"x"')).decode("ascii") RemoteService.save_returned_cache(stub, {"tokens_work": blob, "tokens_default": blob, "tokens": blob}, "work") @@ -560,12 +560,12 @@ def test_session_create_writes_nested_cache_and_skips_colliding_keys(tmp_path, m ) monkeypatch.setattr("unshackle.core.api.session_store.get_session_store", lambda: (_ for _ in ()).throw(Stop())) - cache = {"x": blob("flat"), "x.json/y": blob("collides"), "MSL/keys.v2": blob("dotted")} + cache = {"x": blob('"flat"'), "x.json/y": blob('"collides"'), "MSL/keys.v2": blob('"dotted"')} response = asyncio.run(handlers.session_create_handler({"service": "TESTSVC", "title_id": "t", "cache": cache})) assert response.status == 500 # the Stop stand-in fired after the cache write written = {p.name: p.read_text() for p in tmp_path.rglob("*.json") if p.is_file()} - assert written == {"x.json": "flat", "keys.v2.json": "dotted"} + assert written == {"x.json": '"flat"', "keys.v2.json": '"dotted"'} class _SentCreate(Exception): diff --git a/unshackle/core/api/handlers.py b/unshackle/core/api/handlers.py index 2f854203..835ced0d 100644 --- a/unshackle/core/api/handlers.py +++ b/unshackle/core/api/handlers.py @@ -23,7 +23,7 @@ from aiohttp import web from unshackle.core.api.compression import safe_inflate from unshackle.core.api.errors import APIError, APIErrorCode, categorize_exception, handle_api_exception from unshackle.core.api.input_bridge import AuthStatus, InputBridge, NoUserBridge -from unshackle.core.api.sanitize import MAX_SESSION_CACHE_KEYS, safe_cache_key, sanitize_log +from unshackle.core.api.sanitize import MAX_SESSION_CACHE_KEYS, safe_cache_key, safe_cache_payload, sanitize_log from unshackle.core.api.session_log import SessionLogBuffer, SessionLogMirror, capture_service_logs from unshackle.core.api.session_store import SessionStore from unshackle.core.cacher import Cacher @@ -316,9 +316,9 @@ def write_client_cache(cache_data: Any, cache_tag: str) -> None: """Write a client-sent ``cache`` map into the cache directory ``cache_tag``. Each cache key is a file path relative to that directory, and each value is the file as - base64 of zlib-compressed bytes. The function skips an unsafe key or a file it cannot write, - and raises INVALID_INPUT for a map that is not an object, has too many entries, or holds a - value that is not a string. + base64 of zlib-compressed bytes. The function skips an unsafe key, a file that names a type + other than a datetime, or a file it cannot write, and raises INVALID_INPUT for a map that is + not an object, has too many entries, or holds a value that is not a string. """ if not isinstance(cache_data, dict) or len(cache_data) > MAX_SESSION_CACHE_KEYS: raise APIError(APIErrorCode.INVALID_INPUT, f"cache must hold at most {MAX_SESSION_CACHE_KEYS} entries") @@ -332,6 +332,9 @@ def write_client_cache(cache_data: Any, cache_tag: str) -> None: if not isinstance(content, str): raise APIError(APIErrorCode.INVALID_INPUT, "cache values must be base64 strings") decompressed = safe_inflate(base64.b64decode(content)).decode("utf-8") + if not safe_cache_payload(decompressed): + log.warning(f"Rejecting session cache file {sanitize_log(key)}: it holds a type a peer may not send") + continue target = cache_dir / f"{safe_name}.json" try: target.parent.mkdir(parents=True, exist_ok=True) @@ -3895,7 +3898,7 @@ async def session_create_handler(data: Dict[str, Any], request: Optional[web.Req cache_data = data.get("cache", {}) if cache_data and session_cache_tag: - write_client_cache(cache_data, session_cache_tag) + await asyncio.to_thread(write_client_cache, cache_data, session_cache_tag) service_instance._input_bridge = bridge diff --git a/unshackle/core/api/sanitize.py b/unshackle/core/api/sanitize.py index 570eb013..e2478001 100644 --- a/unshackle/core/api/sanitize.py +++ b/unshackle/core/api/sanitize.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import re from pathlib import PureWindowsPath from typing import Optional @@ -41,3 +42,38 @@ def safe_cache_key(key: object) -> Optional[str]: if any(not part or part in {".", ".."} or part.endswith((" ", ".")) or ":" in part for part in parts): return None return "/".join(parts) + + +SAFE_PICKLE_TAGS = frozenset({"py/object", "py/type", "py/reduce", "py/tuple", "py/set", "py/b64", "py/id"}) +SAFE_PICKLE_TYPES = frozenset( + {"datetime.datetime", "datetime.date", "datetime.time", "datetime.timedelta", "datetime.timezone"} +) + + +def safe_cache_payload(text: str) -> bool: + """True when a peer-supplied cache file is JSON that jsonpickle decodes to plain data and datetimes only. + + ``Cacher`` decodes cache files with jsonpickle, which imports and calls any type a file names, + so a file from another machine may name the datetime types and nothing else. + """ + try: + root = json.loads(text) + except (ValueError, RecursionError): + return False + stack = [root] + while stack: + node = stack.pop() + if isinstance(node, list): + stack.extend(node) + elif isinstance(node, dict): + for key, value in node.items(): + if key.startswith("py/") and key not in SAFE_PICKLE_TAGS: + return False + if key in ("py/object", "py/type") and not (isinstance(value, str) and value in SAFE_PICKLE_TYPES): + return False + if key in ("py/reduce", "__reduce__") and not ( + isinstance(value, list) and value and isinstance(value[0], dict) and value[0].keys() == {"py/type"} + ): + return False + stack.append(value) + return True diff --git a/unshackle/core/remote_service.py b/unshackle/core/remote_service.py index d87be35a..823a3a0d 100644 --- a/unshackle/core/remote_service.py +++ b/unshackle/core/remote_service.py @@ -1977,12 +1977,13 @@ class RemoteService: can forward them back, skipping interactive auth. The same filter that :meth:`load_cache_files` applies drops a file that belongs to another credential or profile, so it cannot overwrite that profile's local file. + The client also refuses a file that names a type other than a datetime. """ if not cache_data: return from unshackle.core.api.compression import safe_inflate - from unshackle.core.api.sanitize import MAX_SESSION_CACHE_KEYS, safe_cache_key + from unshackle.core.api.sanitize import MAX_SESSION_CACHE_KEYS, safe_cache_key, safe_cache_payload if len(cache_data) > MAX_SESSION_CACHE_KEYS: self.log.warning(f"Ignoring {len(cache_data)} cache files from server: more than {MAX_SESSION_CACHE_KEYS}") @@ -2003,6 +2004,11 @@ class RemoteService: continue try: decompressed = safe_inflate(base64.b64decode(content)) + if not safe_cache_payload(decompressed.decode("utf-8")): + self.log.warning( + f"Rejecting cache file '{safe_name}' from the server: it holds a type a peer may not send" + ) + continue # Not with_suffix: that would turn the key "keys.v2" into "keys.json". target = cache_dir / f"{safe_name}.json" target.parent.mkdir(parents=True, exist_ok=True)