Skip to content

gh-125789: fix side-effects in asyncio 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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
be7cac9
mitigate side-effects in futures scheduling
picnixz Oct 22, 2024
ca61908
blurb
picnixz Oct 22, 2024
640c799
fix UAF when removing a callback
picnixz Oct 22, 2024
a05cde2
Address Guido's and Yury's reviews.
picnixz Oct 23, 2024
22b21cc
Fix UAF (address Nico Posada's review)
picnixz Oct 23, 2024
f7b6730
Enforce conditions on the internal tuple
picnixz Oct 23, 2024
f72c393
Address reviews and improve coverage.
picnixz Oct 24, 2024
378fe09
fix typo
picnixz Oct 24, 2024
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
89 changes: 89 additions & 0 deletions Lib/test/test_asyncio/test_futures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,86 @@ def __eq__(self, other):

fut.remove_done_callback(evil())

def test_schedule_callbacks_list_mutation_3(self):
raise NotImplemented

def _test_schedule_callbacks_list_mutation_3(self, exc_type, exc_text=None):
# see https://github.com/python/cpython/issues/125789 for details
fut = self._new_future()

class evil:
def __eq__(self, other):
global mem
mem = other
return False

cb_pad = lambda: ...
fut.add_done_callback(cb_pad) # sets fut->fut_callback0
fut.add_done_callback(evil()) # sets first item in fut->fut_callbacks
# Consume fut->fut_callback0 callback but checks the remaining callbacks,
# thereby invoking evil.__eq__().
fut.remove_done_callback(cb_pad)
self.assertIs(mem, cb_pad)

fake = (
(0).to_bytes(8, 'little') +
id(bytearray).to_bytes(8, 'little') +
(2 ** 63 - 1).to_bytes(8, 'little') +
(0).to_bytes(24, 'little')
)

i2f = lambda num: 5e-324 * num
fut._callbacks[0] = complex(0, i2f(id(fake) + bytes.__basicsize__ - 1))

# We want to call once again evil.__eq__() to set 'mem' to our
# malicious bytearray. However, since we manually modified the
# callbacks list, we will not be able to by-pass the checks.
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)

# Use-After-Free sanity checks.
# Credits to Nico-Posada for the PoCs.
# See https://github.com/python/cpython/pull/125833.

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]
# removes callback from fut->fut_callback0 setting it to NULL
fut.remove_done_callback(cb_pad)
fut.remove_done_callback(evil())

def test_use_after_free_fixed_2(self):
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')
Expand All @@ -938,6 +1018,9 @@ class CFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
def _new_future(self):
return futures._CFuture(loop=self.loop)

def test_schedule_callbacks_list_mutation_3(self):
super()._test_schedule_callbacks_list_mutation_3(RuntimeError, 'corrupted')


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

def test_schedule_callbacks_list_mutation_3(self):
super()._test_schedule_callbacks_list_mutation_3(RuntimeError, 'corrupted')


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)


class BaseFutureInheritanceTests:

Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst
View file
Open in desktop
Original file line number Diff line number Diff 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.
80 changes: 64 additions & 16 deletions Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,15 +434,33 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
return 0;
}

if (!PyList_CheckExact(fut->fut_callbacks)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test for this? Is there a way to assign a non-list to fut._callbacks?

Copy link
Member Author

@picnixz picnixz Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mmh, actually no. The property is read-only and even with subclassing it's not possible. I'll replace the list checks by assertions.

EDIT: NVM, the macros PyList_GET* macros already take care of the assertion.

PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
return -1;
}

len = PyList_GET_SIZE(fut->fut_callbacks);
if (len == 0) {
/* The list of callbacks was empty; clear it and return. */
Py_CLEAR(fut->fut_callbacks);
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'?

Copy link
Member Author

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 call fut._callbacks.clear() inside the callback that is being called and that would be it but I'd need to confirm tomorrow.

Copy link
Contributor

@graingert graingert Oct 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

import asyncio

def catch(*args, **kwargs):
    fut._callbacks.clear()

loop = lambda: ...
loop.call_soon = catch
loop.get_debug = lambda: False

fut = 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")

Copy link
Member Author

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 evil call_soon.

// (see https://github.com/python/cpython/issues/125789 for details).
for (i = 0; fut->fut_callbacks != NULL; i++) {
if (!PyList_CheckExact(fut->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
return -1;
}
if (i >= PyList_GET_SIZE(fut->fut_callbacks)) {
break; // done
}
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 Down Expand Up @@ -1017,7 +1035,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 @@ -1033,6 +1057,11 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
return PyLong_FromSsize_t(cleared_callback0);
}

if (!PyList_CheckExact(self->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
return NULL;
}

len = PyList_GET_SIZE(self->fut_callbacks);
if (len == 0) {
Py_CLEAR(self->fut_callbacks);
Expand All @@ -1041,8 +1070,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 on cb not cb_tup. We don't necessarily care if the tuple gets deleted in PyObject_RichCompareBool, we care if cb gets deleted. This technically works because I don't think there's a way to delete cb 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_XDECREF(cb_tup);
if (cmp == -1) {
return NULL;
}
Expand All @@ -1060,24 +1098,34 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
return NULL;
}

// Beware: PyObject_RichCompareBool below may change fut_callbacks.
// See GH-97592.
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);
// 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++) {
if (!PyList_CheckExact(self->fut_callbacks)) {
PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
goto fail;
}
if (i >= PyList_GET_SIZE(self->fut_callbacks)) {
break; // done
}
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 delete cb once the tuple has been incref'd, but if it's somehow possible to replace the first item in the tuple then cb 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