gh-126080: fix UAF on task->task_context in task_call_step_soon due to an evil loop.__getattribute__

[picnixz]

I cleaned up the PoC a bit. The pure Python implementation should not be vulnerable to that kind of shenanigans although the interpreter raises an exception upon interpreter's exit (but it doesn't crash).

• Issue: Crash: UAF in task_call_step_soon in _asynciomodule.c (with admittedly ridiculous setup) #126080

[1st1]

I'm OK to accept the C code part of patch (after additional quick review), but I'm -1 on submitting the test, which looks extremely complicated and nearly impossible to maintain. Also due to how the test is structured it might be a pain to keep it up to date with future asyncio development.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[encukou]

@1st1, would it make sense to

• move the test to a separate file (test_asyncio/test_regressions.py perhaps)?
• add a comment that the test uses internals, and can be removed if the internals change?

[willingc]

@encukou @1st1 I agree that these tests are overly complicated and would be better in its own test file. I also think that the tests would benefit from better naming (ridiculous setup isn't very descriptive for future maintainers). I also think a one or two line docstring of what the test is testing would be helpful too.

[picnixz]

I also think that the tests would benefit from better naming (ridiculous setup isn't very descriptive for future maintainers).

I can find a better name for that :)

I also think a one or two line docstring of what the test is testing would be helpful too.

I assumed that referencing the issue was sufficient but I can add some comments!

I agree that these tests are overly complicated and would be better in its own test file.

Would it be possible to delay that one until we fix all UAFs or make it the priority for now (either way is fine for me). @Nico-Posada has one PR that fixes another UAF and may be working on a second one for an even more contrived crash. If you want it to be now, I can create a PR just to isolate the existing tests (and to move ReachableCode exception to the test.support file because it could be a good place for future usage?).

[1st1]

@encukou @willingc

move the test to a separate file (test_asyncio/test_regressions.py perhaps)?
add a comment that the test uses internals, and can be removed if the internals change?

I'm really not sure this all is worth the hassle. It's only a matter of time until someone stumbles upon this test and will not understand why it's there :)

This PR tests something very abstract and not occurring in real life scenarios, so I personally think it's OK to commit the fix for the sake of having cleaner code, but on the other hand there's no need to overthink it.

[picnixz]

I've removed the contrived test as well as the small refactorization I needed. It's much cleaner and for posterity, people can just look at 02415df to get a test in the future.

[ZeroIntensity]

Ok, LGTM. If any future problems come up with wrapping call_soon, let's move the job of incref'ing there instead of in the caller. For now, this is fine.

[EDITED by @picnixz]: The discussion in question (marked as resolved for visual purposes): #126120 (comment).

[Nico-Posada]

I'm curious if it's viable to do a refcount-based test to make sure that the refcount of our evil object is <num here> in __getattribute__. Can't test for UAF directly but it'll test to make sure the refcount is being increased properly which means the UAF is being prevented. The test case for this should be much shorter and easier to understand/implement.

[picnixz]

I'm curious if it's viable to do a refcount-based test to make sure that the refcount of our evil object is in getattribute

Could be possible and we could transform previous tests like that but it might still require some globals to know when we need to trigger the check and I suspect it'll be ugly to read =/ (haven't tried though)

[Nico-Posada]

Here's something basic I was able to put together. On a build where this bug isn't patched, it says refcount 6, but on this version it says refcount 7. So in the test we would do something to assert the refcount at that spot >= 7`

Obviously there's a lot to clean up in this, but it's MUCH smaller now and we can probably add comments explaining why it's testing for refcounts in that spot.

import asyncio
import types
import sys
from contextvars import Context

async def none_coroutine():
    @types.coroutine
    def gen():
        while True:
            yield None
    await gen()

class TestDoneException(BaseException):
    pass

class TestLoop:
    get_debug = staticmethod(lambda: False)
    is_running = staticmethod(lambda: True)
    
    def __getattribute__(self, name):
        if name == "call_soon":
            print("refcount", sys.getrefcount(my_context))
            raise TestDoneException()
        
        return object.__getattribute__(self, name)

coro = none_coroutine()
my_context = Context()

try:
    task = asyncio.Task(coro, loop=TestLoop(), context=my_context, eager_start=True)
except Exception:
    print("done")

[picnixz]

@1st1 Are you willing to consider this kind of test or do you prefer no test at all? if you want, we can try to remove previous tests that were also using intricate constructions just to trigger the UAF and make them adopt that kind of testing?

[encukou]

Refcount values can change when code is refactored, and if the refactor adds a refcount, a >= 7 assertion would pass even if this regresses. An ineffective test is worse than a failing one, IMO.

[picnixz]

Oh right. Then I think no test is fine in this case.

[1st1]

Approving as is without a test.

[miss-islington-app]

Thanks @picnixz for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-126250 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-126251 is a backport of this pull request to the 3.12 branch.