Skip to content

SubprocessTransport .close() can fail with PermissionError with setuid programs #112800

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

Jump to bottom
Closed
allisonkarlitskaya opened this issue Dec 6, 2023 · 0 comments · Fixed by #112803
Closed

SubprocessTransport .close() can fail with PermissionError with setuid programs #112800

allisonkarlitskaya opened this issue Dec 6, 2023 · 0 comments · Fixed by #112803
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@allisonkarlitskaya
Copy link
Contributor

allisonkarlitskaya commented Dec 6, 2023 •
edited by bedevere-app bot
Loading

Bug report

Bug description:

Python 3.12.0 (main, Oct  2 2023, 00:00:00) [GCC 13.2.1 20230918 (Red Hat 13.2.1-3)] on linux

aka python3-3.12.0-1.fc39.x86_64.

Run this code on your Linux system with pkexec setup. That should work on any normalish GNOME system, I'd guess. I'm using Fedora 39.

import asyncio


class MyProtocol(asyncio.SubprocessProtocol):
    pass


async def run():
    loop = asyncio.get_running_loop()
    transport, protocol = await loop.subprocess_exec(MyProtocol, 'pkexec', 'cat')
    await asyncio.sleep(10)
    transport.close()


asyncio.run(run())

You should get a popup to enter your admin password. Do that within 10 seconds. Then cat (which now has the same PID as we spawned pkexec with) will be running as root.

transport.close() attempts to kill() that PID, which fails:

Traceback (most recent call last):
  File "/var/home/lis/src/cockpit/ferny-transport/break.py", line 15, in <module>
    asyncio.run(run())
  File "/usr/lib64/python3.12/asyncio/runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/var/home/lis/src/cockpit/ferny-transport/break.py", line 12, in run
    transport.close()
  File "/usr/lib64/python3.12/asyncio/base_subprocess.py", line 117, in close
    self._proc.kill()
  File "/usr/lib64/python3.12/subprocess.py", line 2209, in kill
    self.send_signal(signal.SIGKILL)
  File "/usr/lib64/python3.12/subprocess.py", line 2196, in send_signal
    os.kill(self.pid, sig)
PermissionError: [Errno 1] Operation not permitted

Probably the call to self._proc.kill() in .close() should be guarded to ignore PermissionError. It already ignores ProcessLookupError:

            try:
                self._proc.kill()
            except ProcessLookupError:
                pass

There are many other setuid utilities that this doesn't seem to be a problem with. The shadow-utils tools like passwd seem to remain killable, as does sudo (which keeps a process running around and forks off to spawn the desired command as root). In fact, pkexec was the only tool I could find that causes this issue, but as viewed from the Python side, we clearly cannot necessarily rely on being able to .kill() a PID that we created.

Thanks!

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

Linux

Linked PRs
@allisonkarlitskaya allisonkarlitskaya added the type-bug An unexpected behavior, bug, or error label Dec 6, 2023
@github-project-automation github-project-automation bot moved this to Todo in asyncio Dec 6, 2023
allisonkarlitskaya added a commit to allisonkarlitskaya/cpython that referenced this issue Dec 6, 2023
@allisonkarlitskaya
pythongh-112800: Ignore PermissionError on SubprocessTransport.close()
3eae8fb 
In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
This was referenced Dec 6, 2023
Merged
Closed
allisonkarlitskaya added a commit to allisonkarlitskaya/cpython that referenced this issue Dec 6, 2023
@allisonkarlitskaya
pythongh-112800: Ignore PermissionError on SubprocessTransport.close()
9f1b302 
In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
allisonkarlitskaya added a commit to allisonkarlitskaya/cpython that referenced this issue Dec 6, 2023
@allisonkarlitskaya
pythongh-112800: Ignore PermissionError on SubprocessTransport.close()
f79b680 
In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
allisonkarlitskaya added a commit to allisonkarlitskaya/ferny that referenced this issue Dec 6, 2023
@allisonkarlitskaya
transport: suppress PermissionError from SubprocessTransport.close()
5457cf0 
See python/cpython#112800
@allisonkarlitskaya allisonkarlitskaya mentioned this issue Dec 6, 2023
Merged
allisonkarlitskaya added a commit to allisonkarlitskaya/ferny that referenced this issue Dec 6, 2023
@allisonkarlitskaya
transport: suppress PermissionError from SubprocessTransport.close()
8495022 
See python/cpython#112800
@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Dec 24, 2023
gvanrossum pushed a commit that referenced this issue Dec 24, 2023
@allisonkarlitskaya
gh-112800: Ignore PermissionError on SubprocessTransport.close() in a…
0187a7e 
…syncio (#112803)

In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
@allisonkarlitskaya @ryan-duve
pythongh-112800: Ignore PermissionError on SubprocessTransport.close(…
dd97147 
…) in asyncio (python#112803)

In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
kulikjak pushed a commit to kulikjak/cpython that referenced this issue Jan 22, 2024
@allisonkarlitskaya @kulikjak
pythongh-112800: Ignore PermissionError on SubprocessTransport.close(…
8b8cb7b 
…) in asyncio (python#112803)

In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
@allisonkarlitskaya @aisk
pythongh-112800: Ignore PermissionError on SubprocessTransport.close(…
bf46a25 
…) in asyncio (python#112803)

In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
@allisonkarlitskaya @Glyphack
pythongh-112800: Ignore PermissionError on SubprocessTransport.close(…
badb1e1 
…) in asyncio (python#112803)

In case the spawned process is setuid, we may not be able to send
signals to it, in which case our .kill() call will raise
PermissionError.

Ignore that in order to avoid .close() raising an exception.  Hopefully
the process will exit as a result of receiving EOF on its stdin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
asyncio
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gh-112800: Ignore PermissionError on SubprocessTransport.close() allisonkarlitskaya/cpython
2 participants
@allisonkarlitskaya @AlexWaygood