From 56a50cab5f94d1674c31aae27736a6b15de45c7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 29 Oct 2024 10:36:24 +0100 Subject: [PATCH 1/6] Move `ReachableCode` exception to `test.support` --- Lib/test/support/__init__.py | 7 ++++++- Lib/test/test_asyncio/test_futures.py | 8 +------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 7c1ef42a4970d7..e2a5edae3dfa73 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -23,7 +23,7 @@ # globals "PIPE_MAX_SIZE", "verbose", "max_memuse", "use_resources", "failfast", # exceptions - "Error", "TestFailed", "TestDidNotRun", "ResourceDenied", + "ReachableCode", "Error", "TestFailed", "TestDidNotRun", "ResourceDenied", # io "record_original_stdout", "get_original_stdout", "captured_stdout", "captured_stdin", "captured_stderr", "captured_output", @@ -107,6 +107,11 @@ STDLIB_DIR = os.path.dirname(TEST_HOME_DIR) REPO_ROOT = os.path.dirname(STDLIB_DIR) +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 Error(Exception): """Base class for regression test exceptions.""" diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 3a4291e3a68ca6..861b535df0ce2f 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -14,6 +14,7 @@ import warnings from test.test_asyncio import utils as test_utils from test import support +from test.support import ReachableCode def tearDownModule(): @@ -32,13 +33,6 @@ 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.""" From 8329ae6cb903147b5dd1d770a9e131d3500b6f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 29 Oct 2024 10:36:45 +0100 Subject: [PATCH 2/6] Fix UAF in `task_call_step_soon`. --- Lib/test/test_asyncio/test_tasks.py | 87 ++++++++++++++++++++++++++++- Modules/_asynciomodule.c | 4 ++ 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index a1013ab803348d..1212b9bb2506c3 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -19,6 +19,7 @@ from asyncio import tasks from test.test_asyncio import utils as test_utils from test import support +from test.support import ReachableCode from test.support.script_helper import assert_python_ok from test.support.warnings_helper import ignore_warnings @@ -89,8 +90,8 @@ class BaseTaskTests: Future = None all_tasks = None - def new_task(self, loop, coro, name='TestTask', context=None): - return self.__class__.Task(coro, loop=loop, name=name, context=context) + def new_task(self, loop, coro, name='TestTask', context=None, **kwargs): + return self.__class__.Task(coro, loop=loop, name=name, context=context, **kwargs) def new_future(self, loop): return self.__class__.Future(loop=loop) @@ -2688,6 +2689,76 @@ def test_get_context(self): finally: loop.close() + def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): + # Special thanks to Nico-Posada for the original PoC. + # see: https://github.com/python/cpython/issues/126080 + asserter = self + + class Break: + def __str__(self): + # to break recursion errors in Task.__init__ + raise ReachableCode(type(self)) + + class EvilEventLoop: + get_debug = staticmethod(lambda: False) + is_running = staticmethod(lambda: True) + + def call_soon(self, *args, **kwargs): + # raise an exception just to make sure this was called + raise ReachableCode(type(self)) + + def __getattribute__(self, name): + if name == "call_soon": + with asserter.assertRaises(ReachableCode) as cm: + # The context must be set to `None` for it to use + # Py_XSETREF instead of a plain regular assignment. + evil_task.__init__(evil_coro, loop=self, name=Break()) + asserter.assertEqual(len(cm.exception.args), 1) + asserter.assertIs(cm.exception.args[0], Break) + return object.__getattribute__(self, name) + + class TaskWakeupCatch: + _asyncio_future_blocking = True + get_loop = staticmethod(lambda: evil_loop) + task_wakeup_method = None + + def add_done_callback(self, callback, *args, **kwargs): + # Retrieve the 'task_wakeup' method of the Task object + # which is not accessible from pure Python code. + if self.task_wakeup_method is None: + self.task_wakeup_method = callback + + catcher = TaskWakeupCatch() + + # We want a synchronous generator wrapped in a coroutine function + # and not an asynchronous generator defined via 'async def'. + async def evil_coroutine(): + @types.coroutine + def sync_generator(): + # ensure to keep catcher alive after the first send() call + nonlocal catcher + while 1: + yield catcher + await sync_generator() + + evil_coro = evil_coroutine() + evil_loop = EvilEventLoop() + + self.assertIsNone(catcher.task_wakeup_method) + evil_task = self.new_task(evil_loop, evil_coro, eager_start=True) + self.assertIsInstance(catcher.task_wakeup_method, types.BuiltinMethodType) + + with asserter.assertRaises(ReachableCode) as cm: + evil_task.__init__(evil_coro, loop=evil_loop, name=Break()) + self.assertEqual(len(cm.exception.args), 1) + self.assertIs(cm.exception.args[0], Break) + + self.assertIsInstance(catcher.task_wakeup_method, types.BuiltinMethodType) + with self.assertRaises(ReachableCode) as cm: + catcher.task_wakeup_method(mock.Mock()) + self.assertEqual(len(cm.exception.args), 1) + self.assertIs(cm.exception.args[0], EvilEventLoop) + def add_subclass_tests(cls): BaseTask = cls.Task @@ -2863,6 +2934,9 @@ class PyTask_CFutureSubclass_Tests(BaseTaskTests, test_utils.TestCase): Task = tasks._PyTask all_tasks = tasks._py_all_tasks + def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): + self.skipTest("Python implementation is safe") + @unittest.skipUnless(hasattr(tasks, '_CTask'), 'requires the C _asyncio module') @@ -2881,6 +2955,9 @@ class PyTask_CFuture_Tests(BaseTaskTests, test_utils.TestCase): Future = getattr(futures, '_CFuture', None) all_tasks = staticmethod(tasks._py_all_tasks) + def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): + self.skipTest("Python implementation is safe") + class PyTask_PyFuture_Tests(BaseTaskTests, SetMethodsTest, test_utils.TestCase): @@ -2889,12 +2966,18 @@ class PyTask_PyFuture_Tests(BaseTaskTests, SetMethodsTest, Future = futures._PyFuture all_tasks = staticmethod(tasks._py_all_tasks) + def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): + self.skipTest("Python implementation is safe") + @add_subclass_tests class PyTask_PyFuture_SubclassTests(BaseTaskTests, test_utils.TestCase): Task = tasks._PyTask Future = futures._PyFuture + def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): + self.skipTest("Python implementation is safe") + @unittest.skipUnless(hasattr(tasks, '_CTask'), 'requires the C _asyncio module') diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index d4135f04e56575..218b991f4c0467 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2738,7 +2738,11 @@ task_call_step_soon(asyncio_state *state, TaskObj *task, PyObject *arg) return -1; } + // Beware: An evil call_soon could alter task_context. + // See: https://github.com/python/cpython/issues/126080. + PyObject *task_context = Py_NewRef(task->task_context); int ret = call_soon(state, task->task_loop, cb, NULL, task->task_context); + Py_DECREF(task_context); Py_DECREF(cb); return ret; } From 2ee95445c7da9e7ff2cbdcd36edc753c74a76789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 29 Oct 2024 10:38:32 +0100 Subject: [PATCH 3/6] blurb --- .../Library/2024-10-29-10-38-28.gh-issue-126080.qKRBuo.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-29-10-38-28.gh-issue-126080.qKRBuo.rst diff --git a/Misc/NEWS.d/next/Library/2024-10-29-10-38-28.gh-issue-126080.qKRBuo.rst b/Misc/NEWS.d/next/Library/2024-10-29-10-38-28.gh-issue-126080.qKRBuo.rst new file mode 100644 index 00000000000000..e54ac17b217c92 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-29-10-38-28.gh-issue-126080.qKRBuo.rst @@ -0,0 +1,3 @@ +Fix a use-after-free crash on :class:`asyncio.Task` objects for which the +underlying event loop implements an evil :meth:`~object.__getattribute__`. +Reported by Nico-Posada. Patch by Bénédikt Tran. From 52b6505ca552c3339805b21ca4a303f2eae5fb57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Tue, 29 Oct 2024 18:38:54 +0100 Subject: [PATCH 4/6] cosmetics In order to align with the previous UAFs fixes, we use the temporary variable that we created instead of the structure's field (they are the same). --- Modules/_asynciomodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 218b991f4c0467..57fdee5a995e19 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2741,7 +2741,7 @@ task_call_step_soon(asyncio_state *state, TaskObj *task, PyObject *arg) // Beware: An evil call_soon could alter task_context. // See: https://github.com/python/cpython/issues/126080. PyObject *task_context = Py_NewRef(task->task_context); - int ret = call_soon(state, task->task_loop, cb, NULL, task->task_context); + int ret = call_soon(state, task->task_loop, cb, NULL, task_context); Py_DECREF(task_context); Py_DECREF(cb); return ret; From 02415df80aac682454781fd8904c06b02552d4cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Thu, 31 Oct 2024 10:34:55 +0100 Subject: [PATCH 5/6] Remove contrived test --- Lib/test/test_asyncio/test_tasks.py | 87 +---------------------------- 1 file changed, 2 insertions(+), 85 deletions(-) diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index 9ccf34ec413ce1..9d2d356631b42c 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -19,7 +19,6 @@ from asyncio import tasks from test.test_asyncio import utils as test_utils from test import support -from test.support import ReachableCode from test.support.script_helper import assert_python_ok from test.support.warnings_helper import ignore_warnings @@ -90,8 +89,8 @@ class BaseTaskTests: Future = None all_tasks = None - def new_task(self, loop, coro, name='TestTask', context=None, **kwargs): - return self.__class__.Task(coro, loop=loop, name=name, context=context, **kwargs) + def new_task(self, loop, coro, name='TestTask', context=None): + return self.__class__.Task(coro, loop=loop, name=name, context=context) def new_future(self, loop): return self.__class__.Future(loop=loop) @@ -2711,76 +2710,6 @@ def __str__(self): self.assertEqual(sys.getrefcount(obj), initial_refcount) - def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): - # Special thanks to Nico-Posada for the original PoC. - # see: https://github.com/python/cpython/issues/126080 - asserter = self - - class Break: - def __str__(self): - # to break recursion errors in Task.__init__ - raise ReachableCode(type(self)) - - class EvilEventLoop: - get_debug = staticmethod(lambda: False) - is_running = staticmethod(lambda: True) - - def call_soon(self, *args, **kwargs): - # raise an exception just to make sure this was called - raise ReachableCode(type(self)) - - def __getattribute__(self, name): - if name == "call_soon": - with asserter.assertRaises(ReachableCode) as cm: - # The context must be set to `None` for it to use - # Py_XSETREF instead of a plain regular assignment. - evil_task.__init__(evil_coro, loop=self, name=Break()) - asserter.assertEqual(len(cm.exception.args), 1) - asserter.assertIs(cm.exception.args[0], Break) - return object.__getattribute__(self, name) - - class TaskWakeupCatch: - _asyncio_future_blocking = True - get_loop = staticmethod(lambda: evil_loop) - task_wakeup_method = None - - def add_done_callback(self, callback, *args, **kwargs): - # Retrieve the 'task_wakeup' method of the Task object - # which is not accessible from pure Python code. - if self.task_wakeup_method is None: - self.task_wakeup_method = callback - - catcher = TaskWakeupCatch() - - # We want a synchronous generator wrapped in a coroutine function - # and not an asynchronous generator defined via 'async def'. - async def evil_coroutine(): - @types.coroutine - def sync_generator(): - # ensure to keep catcher alive after the first send() call - nonlocal catcher - while 1: - yield catcher - await sync_generator() - - evil_coro = evil_coroutine() - evil_loop = EvilEventLoop() - - self.assertIsNone(catcher.task_wakeup_method) - evil_task = self.new_task(evil_loop, evil_coro, eager_start=True) - self.assertIsInstance(catcher.task_wakeup_method, types.BuiltinMethodType) - - with asserter.assertRaises(ReachableCode) as cm: - evil_task.__init__(evil_coro, loop=evil_loop, name=Break()) - self.assertEqual(len(cm.exception.args), 1) - self.assertIs(cm.exception.args[0], Break) - - self.assertIsInstance(catcher.task_wakeup_method, types.BuiltinMethodType) - with self.assertRaises(ReachableCode) as cm: - catcher.task_wakeup_method(mock.Mock()) - self.assertEqual(len(cm.exception.args), 1) - self.assertIs(cm.exception.args[0], EvilEventLoop) - def add_subclass_tests(cls): BaseTask = cls.Task @@ -2956,9 +2885,6 @@ class PyTask_CFutureSubclass_Tests(BaseTaskTests, test_utils.TestCase): Task = tasks._PyTask all_tasks = tasks._py_all_tasks - def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): - self.skipTest("Python implementation is safe") - @unittest.skipUnless(hasattr(tasks, '_CTask'), 'requires the C _asyncio module') @@ -2977,9 +2903,6 @@ class PyTask_CFuture_Tests(BaseTaskTests, test_utils.TestCase): Future = getattr(futures, '_CFuture', None) all_tasks = staticmethod(tasks._py_all_tasks) - def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): - self.skipTest("Python implementation is safe") - class PyTask_PyFuture_Tests(BaseTaskTests, SetMethodsTest, test_utils.TestCase): @@ -2988,18 +2911,12 @@ class PyTask_PyFuture_Tests(BaseTaskTests, SetMethodsTest, Future = futures._PyFuture all_tasks = staticmethod(tasks._py_all_tasks) - def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): - self.skipTest("Python implementation is safe") - @add_subclass_tests class PyTask_PyFuture_SubclassTests(BaseTaskTests, test_utils.TestCase): Task = tasks._PyTask Future = futures._PyFuture - def test_use_after_free_on_task_call_step_soon_with_ridiculous_setup(self): - self.skipTest("Python implementation is safe") - @unittest.skipUnless(hasattr(tasks, '_CTask'), 'requires the C _asyncio module') From af8614d8618b199135e5811c59a55df259609737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Thu, 31 Oct 2024 10:35:19 +0100 Subject: [PATCH 6/6] Revert "Move `ReachableCode` exception to `test.support`" This reverts commit 56a50cab5f94d1674c31aae27736a6b15de45c7a. --- Lib/test/support/__init__.py | 7 +------ Lib/test/test_asyncio/test_futures.py | 8 +++++++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index e2a5edae3dfa73..7c1ef42a4970d7 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -23,7 +23,7 @@ # globals "PIPE_MAX_SIZE", "verbose", "max_memuse", "use_resources", "failfast", # exceptions - "ReachableCode", "Error", "TestFailed", "TestDidNotRun", "ResourceDenied", + "Error", "TestFailed", "TestDidNotRun", "ResourceDenied", # io "record_original_stdout", "get_original_stdout", "captured_stdout", "captured_stdin", "captured_stderr", "captured_output", @@ -107,11 +107,6 @@ STDLIB_DIR = os.path.dirname(TEST_HOME_DIR) REPO_ROOT = os.path.dirname(STDLIB_DIR) -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 Error(Exception): """Base class for regression test exceptions.""" diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 861b535df0ce2f..3a4291e3a68ca6 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -14,7 +14,6 @@ import warnings from test.test_asyncio import utils as test_utils from test import support -from test.support import ReachableCode def tearDownModule(): @@ -33,6 +32,13 @@ 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."""