Skip to content

UAF in asyncio.Future when removing a callback while fut->fut_callbacks length is 1 #126405

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Nico-Posada opened this issue Nov 4, 2024 · 3 comments · Fixed by #126733
Closed
Assignees
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes extension-modules C modules in the Modules dir topic-asyncio type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@Nico-Posada
Copy link
Contributor

Nico-Posada commented Nov 4, 2024

Crash report

What happened?

This bug boils down to a missing incref on the callback before calling PyObject_RichCompareBool. Although we can't directly control the callbacks list with fut._callbacks ever since it was made to return a copy every time, we can still modify it by removing/adding callbacks in evil __eq__ functions.

The bug is here

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);

PoC

import asyncio

fut = asyncio.Future()

class ConfirmUAF:
    def __eq__(self, other):
        print("!!!! How did we get here !!!!")

class Base:
    def __eq__(self, other):
        print("in tracker eq", self, other)
        return other != pad

    def __del__(self):
        # to see when objects are being deleted
        print("deleting", self)

class Evil(Base):
    def __eq__(self, other):
        global _no_del
        print("in evil eq", self, other)
        fut.remove_done_callback(Base())
        old_id = id(other)
        del other
        _no_del = ConfirmUAF()
        new_id = id(_no_del)

        # if these two are the same, you'll end up in the ConfirmUAF.__eq__ func
        # if not, you'll probably just crash
        print(f"{old_id = :#x} {new_id = :#x}")
        return NotImplemented


pad = ...
fut.add_done_callback(pad)
fut.add_done_callback(Base())
assert fut.remove_done_callback(pad) == 1

print("starting bug")
fut.remove_done_callback(Evil())

Output

in tracker eq <__main__.Base object at 0x7f8cac015d40> Ellipsis
starting bug
in evil eq <__main__.Evil object at 0x7f8cac015ea0> <__main__.Base object at 0x7f8cac015d40>
in tracker eq <__main__.Base object at 0x7f8cac015d40> <__main__.Base object at 0x7f8cac016000>
in tracker eq <__main__.Base object at 0x7f8cac016000> Ellipsis
deleting <__main__.Base object at 0x7f8cac016000>
deleting <__main__.Base object at 0x7f8cac015d40>
old_id = 0x7f8cac015d40 new_id = 0x7f8cac015d40
!!!! How did we get here !!!!
deleting <__main__.Evil object at 0x7f8cac015ea0>

CPython versions tested on:

3.13, 3.14

Operating systems tested on:

Linux, Windows

Output from running 'python -VV' on the command line:

Python 3.14.0a1+ (heads/main:85799f1ffd, Nov 4 2024, 11:28:25) [GCC 13.2.0]

Linked PRs

@Nico-Posada Nico-Posada added the type-crash A hard crash of the interpreter, possibly with a core dump label Nov 4, 2024
@ZeroIntensity ZeroIntensity added topic-asyncio extension-modules C modules in the Modules dir 3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes labels Nov 4, 2024
@github-project-automation github-project-automation bot moved this to Todo in asyncio Nov 4, 2024
@sobolevn sobolevn self-assigned this Nov 5, 2024
@sobolevn
Copy link
Member

sobolevn commented Nov 5, 2024

Thank you! I will take a look.

@sobolevn
Copy link
Member

sobolevn commented Nov 5, 2024

Hm, this code works without any crashes on main:

import asyncio

async def main():
    fut = asyncio.Future()

    class ConfirmUAF:
        def __eq__(self, other):
            print("!!!! How did we get here !!!!")

    class Base:
        def __eq__(self, other):
            print("in tracker eq", self, other)
            return other != pad

        def __del__(self):
            # to see when objects are being deleted
            print("deleting", self)

    class Evil(Base):
        def __eq__(self, other):
            global _no_del
            print("in evil eq", self, other)
            fut.remove_done_callback(Base())
            old_id = id(other)
            del other
            _no_del = ConfirmUAF()
            new_id = id(_no_del)

            # if these two are the same, you'll end up in the ConfirmUAF.__eq__ func
            # if not, you'll probably just crash
            print(f"{old_id = :#x} {new_id = :#x}")
            return NotImplemented


    pad = ...
    fut.add_done_callback(pad)
    fut.add_done_callback(Base())
    assert fut.remove_done_callback(pad) == 1

    print("starting bug")
    assert fut.remove_done_callback(Evil()) == 0

asyncio.run(main())

@Nico-Posada
Copy link
Contributor Author

Nico-Posada commented Nov 5, 2024

The original PoC doesnt crash because I fill the spot where the freed memory is with the ConfirmUAF object. If you comment out the creation of the ConfirmUAF object, then it'll crash.

class Evil(Base):
    def __eq__(self, other):
        global _no_del
        print("in evil eq", self, other)
        fut.remove_done_callback(Base())
        # old_id = id(other)
        del other
        # _no_del = ConfirmUAF()
        # new_id = id(_no_del)

        # if these two are the same, you'll end up in the ConfirmUAF.__eq__ func
        # if not, you'll probably just crash
        # print(f"{old_id = :#x} {new_id = :#x}")
        return NotImplemented

@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Nov 12, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 12, 2024
…allback` (pythonGH-126733)

(cherry picked from commit 37c57df)

Co-authored-by: Kumar Aditya <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 12, 2024
…allback` (pythonGH-126733)

(cherry picked from commit 37c57df)

Co-authored-by: Kumar Aditya <[email protected]>
kumaraditya303 added a commit that referenced this issue Nov 12, 2024
…callback` (GH-126733) (#126737)

gh-126405: fix use-after-free in `_asyncio.Future.remove_done_callback` (GH-126733)
(cherry picked from commit 37c57df)

Co-authored-by: Kumar Aditya <[email protected]>
kumaraditya303 added a commit that referenced this issue Nov 12, 2024
…callback` (GH-126733) (#126736)

gh-126405: fix use-after-free in `_asyncio.Future.remove_done_callback` (GH-126733)
(cherry picked from commit 37c57df)

Co-authored-by: Kumar Aditya <[email protected]>
picnixz pushed a commit to picnixz/cpython that referenced this issue Dec 8, 2024
ebonnal pushed a commit to ebonnal/cpython that referenced this issue Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 bugs and security fixes extension-modules C modules in the Modules dir topic-asyncio type-crash A hard crash of the interpreter, possibly with a core dump
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants