Skip to content
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

Use asyncio to avoid blocking tube.remote when DNS resolution fail #2541

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Feb 9, 2025

I must admit this is the first time I write python code using asyncio. We probaly need some
experts to review this part.

This PR aims to avoid blocking the main process when DNS resolution fail when called via
sock.getaddrinfo in tube.remote().

Closes #2540.

Related links

loop.getaddrinfo

Testing

N/A.

Target Branch

Could we backport this to the stable branch? Main process stuck is kinda sucks.

@tesuji tesuji force-pushed the remote-fail-resolution branch 2 times, most recently from 6db693d to 409f491 Compare February 9, 2025 13:48
@tesuji tesuji changed the title Use asyncio to avoid blocking sock.getaddrinfo when DNS resolution fails. Use asyncio to avoid blocking tube.remote when DNS resolution fails Feb 9, 2025
@tesuji tesuji force-pushed the remote-fail-resolution branch from 409f491 to a7fd893 Compare February 9, 2025 13:52
@tesuji
Copy link
Contributor Author

tesuji commented Feb 9, 2025

Before

Main process is blocked arounds 28 seconds even when one presses Ctrl-C repeatedly

Click to see log with DEBUG

[-] Opening connection to chall.lac.tf on port 31174: Failed
Traceback (most recent call last):
  File "/home/hacker/ctfs/2025/lactf/pwn/library/run.py", line 139, in <module>
    r = connect()
        ^^^^^^^^^
  File "/home/hacker/ctfs/2025/lactf/pwn/library/run.py", line 42, in connect
    r = remote(host, int(port))#, ssl=True)
        ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hacker/venv/lib/python3.12/site-packages/pwnlib/tubes/remote.py", line 85, in __init__
    self.sock   = self._connect(fam, typ)
                  ^^^^^^^^^^^^^^^^^^^^^^^
KeyboardInterrupt
[^] last command took 27.999 s

After

Dump a lot of exception texts if presss Ctrl-C a few times (3 times precisely).
Edit: Much nicer output with the latest commit.

Click to see log with DEBUG

> py run.py DEBUG REMOTE
[-] Opening connection to chall.lac.tf on port 31137: Failed
Traceback (most recent call last):
  File "/home/hacker/ctfs/2025/lactf/pwn/minceraft/run.py", line 106, in <module>
    r = connect()
        ^^^^^^^^^
  File "/home/hacker/ctfs/2025/lactf/pwn/minceraft/run.py", line 42, in connect
    r = remote(host, int(port))#, ssl=True)
        ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/hacker/venv/lib/python3.12/site-packages/pwnlib/tubes/remote.py", line 86, in __init__
    self.sock   = self._connect(fam, typ)
                  ^^^^^^^^^^^^^^^^^^^^^^^
KeyboardInterrupt
^CException ignored in: <module 'threading' from '/usr/lib/python3.12/threading.py'>
Traceback (most recent call last):
  File "/usr/lib/python3.12/concurrent/futures/thread.py", line 31, in _python_exit
    t.join()
  File "/usr/lib/python3.12/threading.py", line 1149, in join
    self._wait_for_tstate_lock()
  File "/usr/lib/python3.12/threading.py", line 1169, in _wait_for_tstate_lock
    if lock.acquire(block, timeout):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyboardInterrupt:

@tesuji tesuji force-pushed the remote-fail-resolution branch from 71c4db2 to b4daf12 Compare February 9, 2025 15:01
@tesuji tesuji changed the title Use asyncio to avoid blocking tube.remote when DNS resolution fails Use asyncio to avoid blocking tube.remote when DNS resolution fail Feb 10, 2025
@peace-maker
Copy link
Member

Good idea. We should add an asyncio.wait_for wrapper to add an actual timeout for DNS lookups.

I'm not sure of the implications of using the asyncio eventloop once. But I think this could break if there already is an event loop and we try to start another one with asyncio.run. I'll test this in different async scenarios and see if using remote breaks anything.

@tesuji
Copy link
Contributor Author

tesuji commented Feb 11, 2025

That's a good critical point about nested asyncio eventloops. I have 2 plans:

  • Call asyncio eventloop on a new thread.
  • Make a async version of tube.remote()) and let people call it with their eventloop.

Both plans will make the code more complicated/harder to maintain.

Could we have a nice way to avoid the blocking getaddrinfo in synchronous code?

@Arusekk
Copy link
Member

Arusekk commented Feb 11, 2025

Can the original problem be solved differently? If you press Ctrl+C and the process still does something, it means the exception handler does too much work, so it might mean there is some except block instead of except IOError. I will try to reproduce your issue.

@tesuji tesuji force-pushed the remote-fail-resolution branch from aba5dc5 to b9976b6 Compare February 11, 2025 10:14
@tesuji
Copy link
Contributor Author

tesuji commented Feb 11, 2025

We should add an asyncio.wait_for wrapper to add an actual timeout for DNS lookups.

I think the default OS resolution timeout (around 30 seconds) is reasonable. Because the main point
is to let users interrupt/quit the blocking calls.

I'll test this in different async scenarios and see if using remote breaks anything.

I've added a new code to handle nested eventloops. Though I haven't tested on my system.
The hostname that I used hasn't failed anymore, perhaps because the DNS server has updated its records.

Edit: Nope, I was wrong. The script deadlocks when having nested eventloops.
Edit 2: Alright, by using a separate thread for eventloop, it's not deadlocks anymore on my simple try.

run.py

from pwn import *
host, port = ('nc chall.lac.tf 31174').split()[-2:]
port = args.PORT or port

r = remote(host, int(port))
r.close()
info("kinda ok")

import asyncio

async def foo():
    return remote(host, int(port))
asyncio.run(foo())

@tesuji tesuji force-pushed the remote-fail-resolution branch from 5f72213 to bebe58f Compare February 11, 2025 11:21
* Require python 3.7+ for `asyncio.get_running_loop()`
* spawn a separate thread to avoid deadlocks
@tesuji tesuji force-pushed the remote-fail-resolution branch from bebe58f to d65ba21 Compare February 12, 2025 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script stuck (cannot ctrl-C to quit) when DNS resolution fails (using remote())
3 participants