Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-125789: fix side-effects inasyncio callback scheduling methods#125833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletionLib/asyncio/futures.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -234,10 +234,11 @@ def remove_done_callback(self, fn):

Returns the number of callbacks removed.
"""
count = len(self._callbacks)
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
Expand Down
156 changes: 156 additions & 0 deletionsLib/test/test_asyncio/test_futures.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -929,6 +929,136 @@ def __eq__(self, other):

fut.remove_done_callback(evil())

# Sanity checks for callback tuples corruption and Use-After-Free.
# Special thanks to Nico-Posada for the original PoCs and ideas.
# See https://github.com/python/cpython/issues/125789.

def test_schedule_callbacks_list_mutation_3(self):
raise NotImplementedError

def _test_schedule_callbacks_list_mutation_3(self, exc_type, exc_text=None):
class evil:
def __eq__(self, other):
global mem
mem = other
return False

cb_pad = lambda: ...

fut = self._new_future()
fut.add_done_callback(cb_pad) # sets fut->fut_callback0
fut.add_done_callback(evil()) # sets fut->fut_callbacks[0]
# Consume fut->fut_callback0 callback and set it to NULL before
# checking fut->fut_callbacks, thereby invoking evil.__eq__().
removed = fut.remove_done_callback(cb_pad)
self.assertEqual(removed, 1)
self.assertIs(mem, cb_pad)
# Set the malicious callback tuple and trigger a type check on it
# by re-invoking evil.__eq__() through remove_done_callback().
fut._callbacks[0] = complex(0, 0)
if exc_text is None:
self.assertRaises(exc_type, fut.remove_done_callback, evil())
else:
self.assertRaisesRegex(exc_type, exc_text,
fut.remove_done_callback, evil())
self.assertIs(mem, cb_pad)

def _evil_call_soon_event_loop(self, evil_call_soon):
fake_event_loop = lambda: ...
fake_event_loop.call_soon = evil_call_soon
fake_event_loop.get_debug = lambda: False # suppress traceback
return fake_event_loop

def test_evil_call_soon_list_mutation_1(self):
def evil_call_soon(*args, **kwargs):
fut._callbacks.clear()

loop = self._evil_call_soon_event_loop(evil_call_soon)
with mock.patch.object(self, 'loop', loop):
fut = self._new_future()
self.assertIs(fut.get_loop(), loop)

cb_pad = lambda: ...
fut.add_done_callback(cb_pad)
fut.add_done_callback(None)
fut.add_done_callback(None)

removed = fut.remove_done_callback(cb_pad)
self.assertEqual(removed, 1)
self.assertEqual(len(fut._callbacks), 2)
fut.set_result("boom")
# When there are no more callbacks, the Python implementation
# returns an empty list but the C implementation returns None.
self.assertIn(fut._callbacks, (None, []))

def test_evil_call_soon_list_mutation_2(self):
raise NotImplementedError

def _test_evil_call_soon_list_mutation_2(self, exc_type, exc_text=None):
def evil_call_soon(*args, **kwargs):
fut._callbacks[1] = complex(0, 0)

loop = self._evil_call_soon_event_loop(evil_call_soon)
with mock.patch.object(self, 'loop', loop):
fut = self._new_future()
self.assertIs(fut.get_loop(), loop)

cb_pad = lambda: ...
fut.add_done_callback(cb_pad)
fut.add_done_callback(None)
fut.add_done_callback(None)
removed = fut.remove_done_callback(cb_pad)
self.assertEqual(removed, 1)
self.assertEqual(len(fut._callbacks), 2)
# The evil 'call_soon' is executed by calling set_result().
if exc_text is None:
self.assertRaises(exc_type, fut.set_result, "boom")
else:
self.assertRaisesRegex(exc_type, exc_text, fut.set_result, "boom")

def test_use_after_free_fixed_1(self):
fut = self._new_future()

class setup:
def __eq__(self, other):
return False

# evil MUST be a subclass of setup for __eq__ to get called first
class evil(setup):
def __eq__(self, value):
fut._callbacks.clear()
return NotImplemented

cb_pad = lambda: ...
fut.add_done_callback(cb_pad) # sets fut->fut_callback0
fut.add_done_callback(setup()) # sets fut->fut_callbacks[0]
removed = fut.remove_done_callback(cb_pad)
self.assertEqual(removed, 1)

# This triggers evil.__eq__(), thereby clearing fut->fut_callbacks
# but we will still hold a reference to fut->fut_callbacks[0] until
# it is no more needed.
removed = fut.remove_done_callback(evil())
self.assertEqual(removed, 0)

def test_use_after_free_fixed_2(self):
asserter = self
fut = self._new_future()

class cb_pad:
def __eq__(self, other):
return True

class evil(cb_pad):
def __eq__(self, other):
removed = fut.remove_done_callback(None)
asserter.assertEqual(removed, 1)
return NotImplemented

fut.add_done_callback(cb_pad())
removed = fut.remove_done_callback(evil())
self.assertEqual(removed, 1)


@unittest.skipUnless(hasattr(futures, '_CFuture'),
'requires the C _asyncio module')
Expand All@@ -938,6 +1068,14 @@ class CFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
def _new_future(self):
return futures._CFuture(loop=self.loop)

def test_schedule_callbacks_list_mutation_3(self):
errmsg = 'corrupted callback tuple'
super()._test_schedule_callbacks_list_mutation_3(RuntimeError, errmsg)

def test_evil_call_soon_list_mutation_2(self):
errmsg = 'corrupted callback tuple'
super()._test_evil_call_soon_list_mutation_2(RuntimeError, errmsg)


@unittest.skipUnless(hasattr(futures, '_CFuture'),
'requires the C _asyncio module')
Expand All@@ -949,13 +1087,31 @@ class CSubFuture(futures._CFuture):
pass
return CSubFuture(loop=self.loop)

def test_schedule_callbacks_list_mutation_3(self):
errmsg = 'corrupted callback tuple'
super()._test_schedule_callbacks_list_mutation_3(RuntimeError, errmsg)

def test_evil_call_soon_list_mutation_2(self):
errmsg = 'corrupted callback tuple'
super()._test_evil_call_soon_list_mutation_2(RuntimeError, errmsg)


class PyFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
test_utils.TestCase):

def _new_future(self):
return futures._PyFuture(loop=self.loop)

def test_schedule_callbacks_list_mutation_3(self):
super()._test_schedule_callbacks_list_mutation_3(TypeError)

def test_evil_call_soon_list_mutation_2(self):
# For this test, the Python implementation raises an IndexError
# because the attribute fut._callbacks is set to an empty list
# *before* invoking the callbacks, while the C implementation
# does not make a temporary copy of the list of callbacks.
super()._test_evil_call_soon_list_mutation_2(IndexError)


class BaseFutureInheritanceTests:

Expand Down
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
Mitigate interpreter crashes and state corruption due to side-effects in
:meth:`asyncio.Future.remove_done_callback` or other callback scheduling
methods. Patch by Bénédikt Tran.
54 changes: 41 additions & 13 deletionsModules/_asynciomodule.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -441,8 +441,16 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
return 0;
}

for (i = 0; i < len; i++) {
// Beware: An evil 'call_soon' could change fut_callbacks or its items
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you have a test case for an evil 'call_soon'?

willingc reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No, you're right. I didn't test but I think we can just callfut._callbacks.clear() inside the callback that is being called and that would be it but I'd need to confirm tomorrow.

willingc reacted with thumbs up emoji
Copy link
Contributor

@graingertgraingertOct 23, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The callback being called won't run until the loop here is finished (and whatever is running future_schedule_callbacks returns to _run_once)

You do need an evil call_soon that either calls the callback immediately or mutates fut._callbacks itself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oops, I mentioned it can be abused in my initial report but never actually followed up on it. Here's a simple crash POC

importasynciodefcatch(*args,**kwargs):fut._callbacks.clear()loop=lambda: ...loop.call_soon=catchloop.get_debug=lambda:Falsefut=asyncio.Future(loop=loop)pad=lambda: ...fut.add_done_callback(pad)fut.add_done_callback(None)fut.add_done_callback(None)fut.remove_done_callback(pad)fut.set_result("bonk")

picnixz reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for that Nico; I've also added a test for when the callback tuple is changed in the evilcall_soon.

// (see https://github.com/python/cpython/issues/125789 for details).
for (i = 0;
fut->fut_callbacks != NULL && i < PyList_GET_SIZE(fut->fut_callbacks);
i++) {
PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
return -1;
}
PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
PyObject *ctx = PyTuple_GET_ITEM(cb_tup, 1);

Expand DownExpand Up@@ -1017,7 +1025,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;
}
Expand All@@ -1041,8 +1055,17 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,

if (len == 1) {
PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, 0);
int cmp = PyObject_RichCompareBool(
PyTuple_GET_ITEM(cb_tup, 0), fn, Py_EQ);
// Beware: An evil PyObject_RichCompareBool could change fut_callbacks
// or its items (see https://github.com/python/cpython/issues/97592 or
// https://github.com/python/cpython/issues/125789 for details).
if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
return NULL;
}
Py_INCREF(cb_tup);
PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A little nitpicky, but the incref should be oncb notcb_tup. We don't necessarily care if the tuple gets deleted in PyObject_RichCompareBool, we care ifcb gets deleted. This technically works because I don't think there's a way to deletecb at this point if the tuple can't be deleted, but it's still worth noting.

int cmp = PyObject_RichCompareBool(cb, fn, Py_EQ);
Py_DECREF(cb_tup);
if (cmp == -1) {
return NULL;
}
Expand All@@ -1060,24 +1083,29 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
return NULL;
}

// Beware: PyObject_RichCompareBool below may change fut_callbacks.
// See GH-97592.
// Beware: An evil PyObject_RichCompareBool could change fut_callbacks
// or its items (see https://github.com/python/cpython/issues/97592 or
// https://github.com/python/cpython/issues/125789 for details).
for (i = 0;
self->fut_callbacks != NULL && i < PyList_GET_SIZE(self->fut_callbacks);
i++) {
int ret;
PyObject *item = PyList_GET_ITEM(self->fut_callbacks, i);
Py_INCREF(item);
ret = PyObject_RichCompareBool(PyTuple_GET_ITEM(item, 0), fn, Py_EQ);
PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i);
if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
goto fail;
}
Py_INCREF(cb_tup);
PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same thing here, I don't think it's possible to deletecb once the tuple has been incref'd, but if it's somehow possible to replace the first item in the tuple thencb will be freed possibly causing a UAF.

int ret = PyObject_RichCompareBool(cb, fn, Py_EQ);
if (ret == 0) {
if (j < len) {
PyList_SET_ITEM(newlist, j,item);
PyList_SET_ITEM(newlist, j,cb_tup);
j++;
continue;
}
ret = PyList_Append(newlist,item);
ret = PyList_Append(newlist,cb_tup);
}
Py_DECREF(item);
Py_DECREF(cb_tup);
if (ret < 0) {
goto fail;
}
Expand Down
Loading
[8]ページ先頭
©2009-2025 Movatter.jp