From 88c7e1c0cc02ec696dea97a7ba5419576719b497 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 26 Oct 2024 12:48:02 +0200 Subject: [PATCH 1/3] Fix UAFs on an `asyncio.Future` with an evil loop. UAFs (on `fut->fut_callback0` and on `fut->fut_context0`) can be triggered if the future's event loop implements an evil `__getattribute__`. --- Lib/test/test_asyncio/test_futures.py | 73 ++++++++++++++++++++++++++- Modules/_asynciomodule.c | 19 ++++--- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 4e3efa7ad6abc0..7a1024d30adf24 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 @@ def __eq__(self, other): 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 @@ def evil_call_soon(*args, **kwargs): 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 @@ def evil_call_soon(*args, **kwargs): # 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, callback, *args, context=None): + super().call_soon(callback, *args, context=context) + 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, callback, *args, context=None): + super().call_soon(callback, *args, context=context) + 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/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 91ba5cea880a06..a4dab29ee9428d 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); - - Py_CLEAR(fut->fut_callback0); - Py_CLEAR(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; + + 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. */ From 0a09b211797516ac925c3b3c134031415bf354c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 26 Oct 2024 12:50:53 +0200 Subject: [PATCH 2/3] blurb --- .../Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst 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 00000000000000..7a1d7b53b11301 --- /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. From 23123da5541f948f5244b02ab58f62d946933a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 26 Oct 2024 22:28:05 +0200 Subject: [PATCH 3/3] Normalize signature for future compatibility. --- Lib/test/test_asyncio/test_futures.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 7a1024d30adf24..bac76b074981df 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -1003,8 +1003,8 @@ 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, callback, *args, context=None): - super().call_soon(callback, *args, context=context) + def call_soon(self, *args, **kwargs): + super().call_soon(*args, **kwargs) raise ReachableCode def __getattribute__(self, name): @@ -1027,8 +1027,8 @@ 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, callback, *args, context=None): - super().call_soon(callback, *args, context=context) + def call_soon(self, *args, **kwargs): + super().call_soon(*args, **kwargs) raise ReachableCode def __getattribute__(self, name):