Skip to content

Commit 8ee366e

Browse files
author
White-Mouse
committed
fix(migration): restrict unpickling of v0 actions blobs
1 parent ad8b6c7 commit 8ee366e

2 files changed

Lines changed: 105 additions & 1 deletion

File tree

src/google/adk/sessions/migration/migrate_from_sqlalchemy_pickle.py

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import argparse
1919
from datetime import datetime
2020
from datetime import timezone
21+
import io
2122
import json
2223
import logging
2324
import pickle
@@ -37,6 +38,44 @@
3738

3839
logger = logging.getLogger("google_adk." + __name__)
3940

41+
_ALLOWED_PICKLE_GLOBALS: set[tuple[str, str]] = {
42+
# Builtin containers/primitives.
43+
("builtins", "dict"),
44+
("builtins", "list"),
45+
("builtins", "set"),
46+
("builtins", "tuple"),
47+
("builtins", "str"),
48+
("builtins", "bytes"),
49+
("builtins", "bytearray"),
50+
("builtins", "int"),
51+
("builtins", "float"),
52+
("builtins", "bool"),
53+
# Expected pickled payload for v0 session schema events.
54+
("google.adk.events.event_actions", "EventActions"),
55+
}
56+
57+
58+
class _RestrictedUnpickler(pickle.Unpickler):
59+
"""Restricted unpickler for migrating legacy v0 schema actions.
60+
61+
The v0 session schema stored `EventActions` as a pickled blob. During
62+
migration we treat the raw bytes read from the source DB as untrusted input
63+
and only allow the minimum set of safe globals needed to reconstruct
64+
`EventActions`.
65+
"""
66+
67+
def find_class(self, module: str, name: str): # noqa: ANN001
68+
if (module, name) in _ALLOWED_PICKLE_GLOBALS:
69+
return super().find_class(module, name)
70+
raise pickle.UnpicklingError(
71+
f"Blocked global during migration unpickle: {module}.{name}"
72+
)
73+
74+
75+
def _restricted_pickle_loads(data: bytes) -> Any:
76+
"""Load a pickle payload using the restricted unpickler."""
77+
return _RestrictedUnpickler(io.BytesIO(data)).load()
78+
4079

4180
def _to_datetime_obj(val: Any) -> datetime | Any:
4281
"""Converts string to datetime if needed."""
@@ -59,7 +98,7 @@ def _row_to_event(row: dict) -> Event:
5998
if actions_val is not None:
6099
try:
61100
if isinstance(actions_val, bytes):
62-
actions = pickle.loads(actions_val)
101+
actions = _restricted_pickle_loads(actions_val)
63102
else: # for spanner - it might return object directly
64103
actions = actions_val
65104
except Exception as e:

tests/unittests/sessions/migration/test_migration.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717

1818
from datetime import datetime
1919
from datetime import timezone
20+
import os
21+
import pickle
2022

2123
from google.adk.events.event_actions import EventActions
2224
from google.adk.sessions.migration import _schema_check_utils
@@ -25,6 +27,7 @@
2527
from google.adk.sessions.schemas import v1
2628
import pytest
2729
from sqlalchemy import create_engine
30+
from sqlalchemy import text
2831
from sqlalchemy.orm import sessionmaker
2932

3033

@@ -184,6 +187,68 @@ def test_migrate_from_sqlalchemy_pickle(tmp_path):
184187
dest_session.close()
185188

186189

190+
def test_migrate_from_sqlalchemy_pickle_blocks_unsafe_actions_pickle(
191+
tmp_path, monkeypatch
192+
):
193+
"""Migration should not execute arbitrary globals from a pickled actions blob."""
194+
monkeypatch.delenv("ADK_MIGRATION_PICKLE_RCE", raising=False)
195+
196+
source_db_path = tmp_path / "source_pickle_unsafe_actions.db"
197+
dest_db_path = tmp_path / "dest_json_unsafe_actions.db"
198+
source_db_url = f"sqlite:///{source_db_path}"
199+
dest_db_url = f"sqlite:///{dest_db_path}"
200+
201+
source_engine = create_engine(source_db_url)
202+
v0.Base.metadata.create_all(source_engine)
203+
SourceSession = sessionmaker(bind=source_engine)
204+
205+
# Populate source DB with a valid session row to satisfy the FK constraint,
206+
# then insert a malicious pickled actions blob directly as raw bytes.
207+
now = datetime.now(timezone.utc)
208+
with SourceSession() as source_session:
209+
source_session.add(
210+
v0.StorageSession(
211+
app_name="app1",
212+
user_id="user1",
213+
id="session1",
214+
state={},
215+
create_time=now,
216+
update_time=now,
217+
)
218+
)
219+
source_session.commit()
220+
221+
class Evil:
222+
def __reduce__(self):
223+
# This is intentionally non-destructive: it only sets an env var.
224+
return (
225+
exec,
226+
("import os; os.environ['ADK_MIGRATION_PICKLE_RCE']='1'",),
227+
)
228+
229+
source_session.execute(
230+
text(
231+
"INSERT INTO events (id, app_name, user_id, session_id, invocation_id, author, actions, timestamp) "
232+
"VALUES (:id, :app_name, :user_id, :session_id, :invocation_id, :author, :actions, :timestamp)"
233+
),
234+
{
235+
"id": "event1",
236+
"app_name": "app1",
237+
"user_id": "user1",
238+
"session_id": "session1",
239+
"invocation_id": "invoke1",
240+
"author": "user",
241+
"actions": pickle.dumps(Evil()),
242+
"timestamp": now,
243+
},
244+
)
245+
source_session.commit()
246+
247+
mfsp.migrate(source_db_url, dest_db_url)
248+
249+
assert os.environ.get("ADK_MIGRATION_PICKLE_RCE") is None
250+
251+
187252
def test_migrate_from_sqlalchemy_pickle_with_async_driver_urls(tmp_path):
188253
"""Tests that migration works with async driver URLs (fixes issue #4176).
189254

0 commit comments

Comments
 (0)