From 016442c3dcb1c0f4fdc3fd515eb588b9d1b92951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Fri, 25 Oct 2024 10:51:01 +0200 Subject: [PATCH 1/6] Fix UAF in `_asyncio` --- Lib/test/test_asyncio/test_futures.py | 18 ++++++++++++++++++ Modules/_asynciomodule.c | 8 +++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index c566b28adb2408..540143da2fedc4 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -929,6 +929,24 @@ def __eq__(self, other): fut.remove_done_callback(evil()) + def test_use_after_free_fixed(self): + # Special thanks to Nico-Posada for the original PoC. + # See https://github.com/python/cpython/issues/125789. + + fut = self._new_future() + + class cb_pad: + def __eq__(self, other): + return True + + class evil(cb_pad): + def __eq__(self, other): + fut.remove_done_callback(None) + return NotImplemented + + fut.add_done_callback(cb_pad()) + fut.remove_done_callback(evil()) + @unittest.skipUnless(hasattr(futures, '_CFuture'), 'requires the C _asyncio module') diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 0a769c46b87ac8..7178500db29fc7 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1017,7 +1017,13 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls, ENSURE_FUTURE_ALIVE(state, self) if (self->fut_callback0 != NULL) { - int cmp = PyObject_RichCompareBool(self->fut_callback0, fn, Py_EQ); + // Beware: An evil PyObject_RichCompareBool could change fut_callback0 + // (see https://github.com/python/cpython/issues/125789 for details) + // In addition, the reference to self->fut_callback0 may be cleared, + // so we need to temporarily hold it explicitly. + PyObject *fut_callback0 = Py_NewRef(self->fut_callback0); + int cmp = PyObject_RichCompareBool(fut_callback0, fn, Py_EQ); + Py_DECREF(fut_callback0); if (cmp == -1) { return NULL; } From ca7fe77181ec8d9972a32df0078009a2850af1f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Fri, 25 Oct 2024 11:01:41 +0200 Subject: [PATCH 2/6] fix Python implementation --- Lib/asyncio/futures.py | 3 ++- Lib/test/test_asyncio/test_futures.py | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py index c95fce035cd548..b6a9d383f56030 100644 --- a/Lib/asyncio/futures.py +++ b/Lib/asyncio/futures.py @@ -234,10 +234,11 @@ def remove_done_callback(self, fn): Returns the number of callbacks removed. """ + count = len(self._callbacks) # since filtering may mutate the list filtered_callbacks = [(f, ctx) for (f, ctx) in self._callbacks if f != fn] - removed_count = len(self._callbacks) - len(filtered_callbacks) + removed_count = count - len(filtered_callbacks) if removed_count: self._callbacks[:] = filtered_callbacks return removed_count diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 540143da2fedc4..50db55c1174b2d 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -933,6 +933,7 @@ def test_use_after_free_fixed(self): # Special thanks to Nico-Posada for the original PoC. # See https://github.com/python/cpython/issues/125789. + asserter = self fut = self._new_future() class cb_pad: @@ -941,11 +942,13 @@ def __eq__(self, other): class evil(cb_pad): def __eq__(self, other): - fut.remove_done_callback(None) + removed = fut.remove_done_callback(None) + asserter.assertEqual(removed, 1) return NotImplemented fut.add_done_callback(cb_pad()) - fut.remove_done_callback(evil()) + removed = fut.remove_done_callback(evil()) + self.assertEqual(removed, 1) @unittest.skipUnless(hasattr(futures, '_CFuture'), From f9e21c3a6a77f5a4b5aa24ed2ed82986495322f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Fri, 25 Oct 2024 10:54:00 +0200 Subject: [PATCH 3/6] blurb --- .../next/Library/2024-10-25-10-53-56.gh-issue-125966.eOCYU_.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-25-10-53-56.gh-issue-125966.eOCYU_.rst diff --git a/Misc/NEWS.d/next/Library/2024-10-25-10-53-56.gh-issue-125966.eOCYU_.rst b/Misc/NEWS.d/next/Library/2024-10-25-10-53-56.gh-issue-125966.eOCYU_.rst new file mode 100644 index 00000000000000..9fe8795de18003 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-25-10-53-56.gh-issue-125966.eOCYU_.rst @@ -0,0 +1,2 @@ +Fix a use-after-free crash in :meth:`asyncio.Future.remove_done_callback`. +Patch by Bénédikt Tran. From 181aadfd62c966918a6c6a8ba98df479c5439234 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Fri, 25 Oct 2024 11:23:52 +0200 Subject: [PATCH 4/6] update links --- Lib/test/test_asyncio/test_futures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 50db55c1174b2d..a2e8f47f86f1be 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -931,7 +931,7 @@ def __eq__(self, other): def test_use_after_free_fixed(self): # Special thanks to Nico-Posada for the original PoC. - # See https://github.com/python/cpython/issues/125789. + # See https://github.com/python/cpython/issues/125966. asserter = self fut = self._new_future() From ef878cb1f353fe414f2e82653ff7cadff5ff8fa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Fri, 25 Oct 2024 14:38:06 +0200 Subject: [PATCH 5/6] Revert "fix Python implementation" This reverts commit ca7fe77181ec8d9972a32df0078009a2850af1f4. --- Lib/asyncio/futures.py | 3 +-- Lib/test/test_asyncio/test_futures.py | 7 ++----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py index b6a9d383f56030..c95fce035cd548 100644 --- a/Lib/asyncio/futures.py +++ b/Lib/asyncio/futures.py @@ -234,11 +234,10 @@ def remove_done_callback(self, fn): Returns the number of callbacks removed. """ - count = len(self._callbacks) # since filtering may mutate the list filtered_callbacks = [(f, ctx) for (f, ctx) in self._callbacks if f != fn] - removed_count = count - len(filtered_callbacks) + removed_count = len(self._callbacks) - len(filtered_callbacks) if removed_count: self._callbacks[:] = filtered_callbacks return removed_count diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index a2e8f47f86f1be..d90e9f1d579976 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -933,7 +933,6 @@ def test_use_after_free_fixed(self): # Special thanks to Nico-Posada for the original PoC. # See https://github.com/python/cpython/issues/125966. - asserter = self fut = self._new_future() class cb_pad: @@ -942,13 +941,11 @@ def __eq__(self, other): class evil(cb_pad): def __eq__(self, other): - removed = fut.remove_done_callback(None) - asserter.assertEqual(removed, 1) + fut.remove_done_callback(None) return NotImplemented fut.add_done_callback(cb_pad()) - removed = fut.remove_done_callback(evil()) - self.assertEqual(removed, 1) + fut.remove_done_callback(evil()) @unittest.skipUnless(hasattr(futures, '_CFuture'), From 11870aac2c003443507819c207bf163e13a999bc 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 08:52:28 +0100 Subject: [PATCH 6/6] Clarify comment per Guido's request. --- Modules/_asynciomodule.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 31dcb0fa344956..204cf34b23d789 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1012,10 +1012,9 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls, ENSURE_FUTURE_ALIVE(state, self) if (self->fut_callback0 != NULL) { - // Beware: An evil PyObject_RichCompareBool could change fut_callback0 - // (see https://github.com/python/cpython/issues/125789 for details) - // In addition, the reference to self->fut_callback0 may be cleared, - // so we need to temporarily hold it explicitly. + // Beware: An evil PyObject_RichCompareBool could free fut_callback0 + // before a recursive call is made with that same arg. For details, see + // https://github.com/python/cpython/pull/125967#discussion_r1816593340. PyObject *fut_callback0 = Py_NewRef(self->fut_callback0); int cmp = PyObject_RichCompareBool(fut_callback0, fn, Py_EQ); Py_DECREF(fut_callback0);