From f819d4301d7c75f02be1187fda017f0e7b608816 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sun, 27 Oct 2024 16:04:43 +0100 Subject: [PATCH] gh-125984: fix use-after-free on `fut->fut_{callback,context}0` due to an evil `loop.__getattribute__` (#126003) --- Lib/test/test_asyncio/test_futures.py | 73 ++++++++++++++++++- ...-10-26-12-50-48.gh-issue-125984.d4vp5_.rst | 3 + Modules/_asynciomodule.c | 17 +++-- 3 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 4e3efa7ad6a..bac76b07498 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -32,6 +32,25 @@ def last_cb(): pass +class ReachableCode(Exception): + """Exception to raise to indicate that some code was reached. + + Use this exception if using mocks is not a good alternative. + """ + + +class SimpleEvilEventLoop(asyncio.base_events.BaseEventLoop): + """Base class for UAF and other evil stuff requiring an evil event loop.""" + + def get_debug(self): # to suppress tracebacks + return False + + def __del__(self): + # Automatically close the evil event loop to avoid warnings. + if not self.is_closed() and not self.is_running(): + self.close() + + class DuckFuture: # Class that does not inherit from Future but aims to be duck-type # compatible with it. @@ -948,6 +967,7 @@ class BaseFutureDoneCallbackTests(): fut.remove_done_callback(evil()) def test_evil_call_soon_list_mutation(self): + # see: https://github.com/python/cpython/issues/125969 called_on_fut_callback0 = False pad = lambda: ... @@ -962,9 +982,8 @@ class BaseFutureDoneCallbackTests(): else: called_on_fut_callback0 = True - fake_event_loop = lambda: ... + fake_event_loop = SimpleEvilEventLoop() fake_event_loop.call_soon = evil_call_soon - fake_event_loop.get_debug = lambda: False # suppress traceback with mock.patch.object(self, 'loop', fake_event_loop): fut = self._new_future() @@ -980,6 +999,56 @@ class BaseFutureDoneCallbackTests(): # returns an empty list but the C implementation returns None. self.assertIn(fut._callbacks, (None, [])) + def test_use_after_free_on_fut_callback_0_with_evil__getattribute__(self): + # see: https://github.com/python/cpython/issues/125984 + + class EvilEventLoop(SimpleEvilEventLoop): + def call_soon(self, *args, **kwargs): + super().call_soon(*args, **kwargs) + raise ReachableCode + + def __getattribute__(self, name): + nonlocal fut_callback_0 + if name == 'call_soon': + fut.remove_done_callback(fut_callback_0) + del fut_callback_0 + return object.__getattribute__(self, name) + + evil_loop = EvilEventLoop() + with mock.patch.object(self, 'loop', evil_loop): + fut = self._new_future() + self.assertIs(fut.get_loop(), evil_loop) + + fut_callback_0 = lambda: ... + fut.add_done_callback(fut_callback_0) + self.assertRaises(ReachableCode, fut.set_result, "boom") + + def test_use_after_free_on_fut_context_0_with_evil__getattribute__(self): + # see: https://github.com/python/cpython/issues/125984 + + class EvilEventLoop(SimpleEvilEventLoop): + def call_soon(self, *args, **kwargs): + super().call_soon(*args, **kwargs) + raise ReachableCode + + def __getattribute__(self, name): + if name == 'call_soon': + # resets the future's event loop + fut.__init__(loop=SimpleEvilEventLoop()) + return object.__getattribute__(self, name) + + evil_loop = EvilEventLoop() + with mock.patch.object(self, 'loop', evil_loop): + fut = self._new_future() + self.assertIs(fut.get_loop(), evil_loop) + + fut_callback_0 = mock.Mock() + fut_context_0 = mock.Mock() + fut.add_done_callback(fut_callback_0, context=fut_context_0) + del fut_context_0 + del fut_callback_0 + self.assertRaises(ReachableCode, fut.set_result, "boom") + @unittest.skipUnless(hasattr(futures, '_CFuture'), 'requires the C _asyncio module') diff --git a/Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst b/Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst new file mode 100644 index 00000000000..7a1d7b53b11 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst @@ -0,0 +1,3 @@ +Fix use-after-free crashes on :class:`asyncio.Future` objects for which the +underlying event loop implements an evil :meth:`~object.__getattribute__`. +Reported by Nico-Posada. Patch by Bénédikt Tran. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 91ba5cea880..a4dab29ee94 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -409,12 +409,19 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) if (fut->fut_callback0 != NULL) { /* There's a 1st callback */ - int ret = call_soon(state, - fut->fut_loop, fut->fut_callback0, - (PyObject *)fut, fut->fut_context0); + // Beware: An evil call_soon could alter fut_callback0 or fut_context0. + // Since we are anyway clearing them after the call, whether call_soon + // succeeds or not, the idea is to transfer ownership so that external + // code is not able to alter them during the call. + PyObject *fut_callback0 = fut->fut_callback0; + fut->fut_callback0 = NULL; + PyObject *fut_context0 = fut->fut_context0; + fut->fut_context0 = NULL; - Py_CLEAR(fut->fut_callback0); - Py_CLEAR(fut->fut_context0); + int ret = call_soon(state, fut->fut_loop, fut_callback0, + (PyObject *)fut, fut_context0); + Py_CLEAR(fut_callback0); + Py_CLEAR(fut_context0); if (ret) { /* If an error occurs in pure-Python implementation, all callbacks are cleared. */