gh-120289: Do not free memory in disable() to prevent use-after-free #120297

gaogaotiantian · 2024-06-09T19:51:18Z

Issue: Use After Free in initContext(_lsprof.c) #120289

markshannon · 2024-06-10T08:55:22Z

I'm not familiar with this code, so I might be mistaken, but...

Needing a flag to determine if something is in a freelist seems risky. What if it is both in the freelist and in use, can't it end up being used twice?

The safer (if maybe a tad slower) approach would be to NULL out the context whenever it is freed and check before use.
It looks like you are already adding a NULL check here anyway.

gaogaotiantian · 2024-06-11T01:10:47Z

Yeah that's one possible way to go. I thought of that but I also thought I can avoid the NULL checking with the free list solution. I was wrong and the ultimate solution was not as clean as I expected. I changed the solution to NULL the context when something goes wrong. Do you think it's better?

markshannon

The news entry needs to be fixed. Should be good to merge once that's done.

markshannon · 2024-06-11T09:29:05Z

Misc/NEWS.d/next/Library/2024-06-09-19-53-11.gh-issue-120289.s4HXR0.rst

@@ -0,0 +1 @@
+Generate an unraisable exception if the external timer :mod:`cProfile` uses changes the profile context.


I don't think this is correct any more.

I changed that. I did not put implementation detail in it because I remember that we are not supposed to do that. So I just said I fixed the issue and mentioned the context.

bedevere-app · 2024-06-11T09:30:19Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner

This approach sounds complicated. Can't we modify timer.disable() to raise an exception if the timer is "not supposed to be disabled"?

gaogaotiantian · 2024-06-11T19:46:49Z

I just realized when I tried to click "quote reply", I accidentally clicked "edit" ..

So here's my reply, and I'll try to restore your reply..

This approach sounds complicated. Can't we modify timer.disable() to raise an exception if the timer is "not supposed to be disabled"?

That's not the issue. The issue is that the profiler is disabled in the timer, which frees the context memory and caused the UAF. It's possible to enforce that the profiler should not be disabled in the timer, but the code of the timer does not propagate exceptions.

static inline PyTime_t
call_timer(ProfilerObject *pObj)
{
    if (pObj->externalTimer != NULL) {
        return CallExternalTimer(pObj);
    }
    else {
        PyTime_t t;
        (void)PyTime_PerfCounterRaw(&t);
        return t;
    }
}

More than that, initContext in ptrace_enter_call is also not friendly with exceptions. It's not possible to do that, but it's definitely not trivial. _lsprof.c is old and not quite well-maintained. To be honest cProfile is too old for a modern profiler.

vstinner · 2024-06-12T09:17:12Z

It's possible to enforce that the profiler should not be disabled in the timer, but the code of the timer does not propagate exceptions.

You can emit a warning and/or log an "unraisable exception". Or simply ignore the attempt to disable the profiler silently.

gaogaotiantian · 2024-06-12T17:08:17Z

Attempting to ignore the disable is very difficult - it requires the code to know whether it's in a timer which would make the code even more complicated. Yes emitting a warning/unraisable exception is an options, but do you mean in addition to the current code? Because this is a UAF, you need to deal with it. Simply sending a warning does not fix anything.

gaogaotiantian · 2024-06-25T01:23:19Z

Hey @markshannon , do you still have comments on this PR? Thanks!

markshannon

LGTM.

I have one question.

markshannon · 2024-06-25T16:45:37Z

Lib/test/test_cprofile.py


            self.assertEqual(cm.unraisable.exc_type, TypeError)

+    @unittest.skipUnless(support.check_sanitizer(address=True), "only used to fail with ASAN")


Is this particularly slow?
I was wondering why not test it for all builds?

Not really. I added this just because that was the place where it failed. I can remove this so we can test it in all environments for regression.

sobolevn

Can you copy the second's issue reproducer to the tests please?

gaogaotiantian · 2024-07-03T21:52:30Z

The original reproducer in the second issue won't be quite applicable because the actual trigger (clear()) won't be executed - disable() will raise an exception and stop clear() from running. What we can do is to do a pure clear() - I'm not sure if that would repro before the patch.

Modules/_lsprof.c

serhiy-storchaka

LGTM.

miss-islington-app · 2024-07-18T19:47:52Z

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

…prevent use-after-free (pythonGH-120297) (cherry picked from commit 1ab1778) Co-authored-by: Tian Gao <[email protected]>

bedevere-app · 2024-07-18T19:48:19Z

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

… prevent use-after-free (GH-120297) (#121984) gh-120289: Disallow disable() and clear() in external timer to prevent use-after-free (GH-120297) (cherry picked from commit 1ab1778) Co-authored-by: Tian Gao <[email protected]>

miss-islington-app · 2024-07-18T21:44:53Z

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

…prevent use-after-free (pythonGH-120297) (cherry picked from commit 1ab1778) Co-authored-by: Tian Gao <[email protected]>

bedevere-app · 2024-07-18T21:45:04Z

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

… prevent use-after-free (GH-120297) (#121989) gh-120289: Disallow disable() and clear() in external timer to prevent use-after-free (GH-120297) (cherry picked from commit 1ab1778) Co-authored-by: Tian Gao <[email protected]>

colesbury · 2024-07-19T01:55:02Z

@gaogaotiantian, it looks like the refleaks buildbots are failing after this PR:

https://buildbot.python.org/#/builders/259/builds/1190/steps/6/logs/stdio
https://buildbot.python.org/#/builders/1287/builds/289/steps/6/logs/stdio

gaogaotiantian · 2024-07-19T01:58:41Z

I'll take a look at it soon, thanks for the notice!

gaogaotiantian · 2024-07-19T03:16:10Z

It turned out that the new code did not cause the issue, it triggered the issue. The root cause is that the profiler did not check the external timer, which could be a container, in traverse. I had the fix in #121998 and ./python -m test -R3:3 test_cprofile passed after the fix.

Warn if the external timer changes the cprofile context

59538ad

bedevere-app bot mentioned this pull request Jun 9, 2024

Use After Free in initContext(_lsprof.c) #120289

Closed

bedevere-app bot added the awaiting core review label Jun 9, 2024

blurb-it bot and others added 2 commits June 9, 2024 19:53

📜🤖 Added by blurb_it.

811b74c

Do not free memory when disable

df99b2e

gaogaotiantian changed the title ~~gh-120289: Warn if the external timer changes the cprofile context~~ gh-120289: Do not free memory in disable() to prevent use-after-free Jun 10, 2024

We need to check if the context is already put in the free list

7675efb

nullify the context pointer instead of use an extra field

e2d68e6

markshannon requested changes Jun 11, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting core review labels Jun 11, 2024

vstinner reviewed Jun 11, 2024

View reviewed changes

Update 2024-06-09-19-53-11.gh-issue-120289.s4HXR0.rst

b317c67

gaogaotiantian requested a review from markshannon June 14, 2024 04:10

Update 2024-06-09-19-53-11.gh-issue-120289.s4HXR0.rst

aa5efcd

markshannon approved these changes Jun 25, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Jun 25, 2024

Remove restriction to limit the test on ASAN

92cb052

kcatss mentioned this pull request Jul 1, 2024

Use After Free in Stop(_lsprof.c) #121225

Closed

gaogaotiantian added 3 commits July 2, 2024 17:51

Revert changes to lsprof

c5f89b5

Revert blank line as well

98e27a9

Disallow disable/clear in external timer

faa05b1

gaogaotiantian added 2 commits July 2, 2024 18:06

Add tests

1a44820

Update 2024-06-09-19-53-11.gh-issue-120289.s4HXR0.rst

d59f4c4

sobolevn reviewed Jul 3, 2024

View reviewed changes

Merge branch 'main' into cprofile-uaf-warning

e76613f

serhiy-storchaka reviewed Jul 17, 2024

View reviewed changes

Modules/_lsprof.c Show resolved Hide resolved

serhiy-storchaka approved these changes Jul 17, 2024

View reviewed changes

gaogaotiantian added the 3.13 bugs and security fixes label Jul 18, 2024

gaogaotiantian merged commit 1ab1778 into python:main Jul 18, 2024

bedevere-app bot removed the awaiting merge label Jul 18, 2024

gaogaotiantian deleted the cprofile-uaf-warning branch July 18, 2024 19:47

gaogaotiantian added needs backport to 3.13 bugs and security fixes and removed 3.13 bugs and security fixes labels Jul 18, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 18, 2024

gaogaotiantian added the needs backport to 3.12 only security fixes label Jul 18, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label Jul 18, 2024

		@@ -0,0 +1 @@
		Generate an unraisable exception if the external timer :mod:`cProfile` uses changes the profile context.


		self.assertEqual(cm.unraisable.exc_type, TypeError)

		@unittest.skipUnless(support.check_sanitizer(address=True), "only used to fail with ASAN")

Uh oh!

gh-120289: Do not free memory in disable() to prevent use-after-free #120297

gh-120289: Do not free memory in disable() to prevent use-after-free #120297

Uh oh!

Conversation

gaogaotiantian commented Jun 9, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon commented Jun 10, 2024

Uh oh!

gaogaotiantian commented Jun 11, 2024

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

gaogaotiantian Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Jun 11, 2024

Uh oh!

vstinner left a comment • edited by gaogaotiantian Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaogaotiantian commented Jun 11, 2024

Uh oh!

vstinner commented Jun 12, 2024

Uh oh!

gaogaotiantian commented Jun 12, 2024

Uh oh!

gaogaotiantian commented Jun 25, 2024

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

gaogaotiantian Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

sobolevn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaogaotiantian commented Jul 3, 2024

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app bot commented Jul 18, 2024

Uh oh!

bedevere-app bot commented Jul 18, 2024

Uh oh!

miss-islington-app bot commented Jul 18, 2024

Uh oh!

bedevere-app bot commented Jul 18, 2024

Uh oh!

colesbury commented Jul 19, 2024

Uh oh!

gaogaotiantian commented Jul 19, 2024

Uh oh!

gaogaotiantian commented Jul 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gaogaotiantian commented Jun 9, 2024 •

edited by bedevere-app bot

Loading

vstinner left a comment •

edited by gaogaotiantian

Loading

sobolevn left a comment •

edited

Loading