Closed peace-maker closed 9 months ago
@masthoon do you have thoughts on this? I like your way of a non-blocking timeout read better using PeekNamedPipe
instead of a busy loop in a separate thread, but I don't want to add too much custom windows api interaction here. Maybe os.pipe()
works too, but isn't that what the stdout = PIPE
option to subprocess.Popen
does already internally?
Also os.set_blocking
says it is supported for pipes on windows since Python 3.12 now? Something to consider once we drop Python 2 support.
Hey, nice work!
Since you are already adding the PythonForWindows
dependency for process management, it might be better to use the appropriate Windows API rather than using the Python abstractions (CreatePipe/CreateProcess vs os.pipe/subprocess.Popen) if that doesn't require too much work. I don't see any major issues with the async thread aside from lack of cleanup (thread stuck on blocking read).
Yes, it is very likely that os.pipe
and subprocess.PIPE
use Windows Pipe internally but it is not recommended to rely on CPython internals to retrieve Pipe handles.
Also, feel free to ask the maintainer of PythonForWindows you want to expose Windows API in windows.winproxy
like PeekNamedPipe
.
PythonForWindows
already implement process management, memory reading/writing, parsing mapped PE executables, process module list enumeration, ... Some are hard to implement, like reading 64-bit process memory with 32-bit Python.
Note: I don't think you should cache the _libs
since it may change during runtime.
Since you are already adding the
PythonForWindows
dependency for process management, it might be better to use the appropriate Windows API rather than using the Python abstractions (CreatePipe/CreateProcess vs os.pipe/subprocess.Popen) if that doesn't require too much work.
I'd rather not add the dependency to be honest, but I liked the process memory read/write and cwd lookup possibility. Right now windows support isn't tested in CI and we're not clear on if and how we want to add support throughout pwntools. Maybe we need a major version bump to start working on some host os abstractions more cleanly. So keeping the changes required for very basic Windows support at a minimum leads to using the available abstractions in subprocess etc. instead of reimplementing them in Python. If we actually decide to fully support Windows and Mac native exploitation, we can of course go deeper and use the Windows API to give more control over the process creation if need be.
I don't see any major issues with the async thread aside from lack of cleanup (thread stuck on blocking read). Yes, it is very likely that
os.pipe
andsubprocess.PIPE
use Windows Pipe internally but it is not recommended to rely on CPython internals to retrieve Pipe handles.
From my tests the thread is terminated when the process exits since the stdout pipe is closed and the read
call returns an empty bytes object breaking the endless loop. But the recv implementation is in reverse this way. tube.can_recv_raw
should wait for timeout
seconds until any bytes are available. Then tube.recv_raw
should just read all available bytes.
Using that queue
to sync reading from another thread forces this to be the other way around breaking that contract.
I don't see an easy way to call PeekNamedPipe
on the default os.pipe
pipes and yes, poking at CPython internals would be ugly and non-portable, so this is better than nothing right now 🤷
@Arusekk What is your stance on such Windows support? I know it's not a supported host os - but it could be?
Okay now I'm almost sure that most of this is already part of psutil
we already use. I am strongly against windows-specific code in pwntools (sure sometimes it maybe has to be there, but why have lots of unmaintainable code vs small amounts of unmaintainable code), and would prefer to have 'non-posix'-specific code in general that hopefully works on posix as well, but maybe worse (for example a separate thread). Using py3.12 setblocking is also an option, with a fallback of threads. But I am not sure I like the general direction; pwntools was always focused on ELFs and Linux (then *BSD as well, but it was free of tons of extra labor).
And here is a bit of justification for my reluctance. Windows is different everywhere. It does not have file descriptors, it does not have standard process management, it does not have standard memory management, it does not have a built-in ANSI terminal, it uses explicit line endings never mind the strange path separators and environment variable names, it uses 16-bit unicode strings (not opaque 8-bit) as the canonical form in some places, but 8-bit byte arrays in other places (thank goodness for UTF-8 ubiquity, but Windows still has not adapted fully); it even treats processes with stdio and without stdio as different organisms (python.exe vs pythonw.exe for instance). It is also great that Python tries so hard to unify the OS abstractions even though they are so different even down to the core principles. It is a miracle that so much software is portable to Windows. But Pwntools is low-level in the very ugliest sense 😄 — and uses every single one of those features I listed above.
Anyway, thanks for the great proof of concept, @peace-maker , you are making miracles happen and this sets some level of number of windows-specific lines to beat. Great job.
I think of Python as being OS agnostic, so having basic support on all platforms in pwntools seems intuitive to me. It doesn't have to support all features everywhere, but stuff that usually works in plain Python should work in pwntools' wrappers too. We don't have to support all the neat low-level features there are for Linux. All the other differences can be handled by the exploit author.
The previously mentioned poor timeout handling in tube.can_recv_raw()
and tube.recv_raw()
was improved by manually waiting in tube.can_recv_raw
for timeout seconds for a byte to be available instead of using the timeout of the queue.read()
function. This fixes a busy loop when using tube.interactive()
, so it's actually usable.
I've removed PythonForWindows again while replacing parts of it by psutil and think this is fine for now. One can use it manually in exploits if needed like
io = process('chall.exe')
winp = windows.winobject.process.WinProcess(pid=io.pid)
winp.read_memory(addr, 8)
Would this still be useful to you @masthoon? I've a patch ready to add windbg.debug()
and windbg.attach()
as well to allow easy debugging from within an exploit script too.
Yes, I would like to see more Windows features in pwntools (reading memory, attaching windbg/cdb, crashdump generation, injecting shellcode, PE from shellcode).
Regarding this PR, I agree that for basic process support, 'psutil' is sufficient.
There is one issue with the busy _read_in_thread
daemon thread that cause Python to fastfail:
# Tested on Python 3.12.0
from pwn import *
io = process(["cmd.exe", "/C", "pause"])
exit()
'''
Exception in thread Thread-1 (_read_in_thread):
Traceback (most recent call last):
File "C:\Users\WDAGUtilityAccount\AppData\Local\Programs\Python\Python312\Lib\threading.py", line 1052, in _bootstrap_inner
[*] Stopped process 'C:\\Windows\\system32\\cmd.exe' (pid 9836)
self.run()
Fatal Python error: _enter_buffered_busy: could not acquire lock for <_io.BufferedWriter name='<stderr>'> at interpreter shutdown, possibly due to daemon threads
Python runtime state: finalizing (tstate=0x00007ffa822cbfb0)
Current thread 0x000024e4 (most recent call first):
<no Python frame>
Extension modules: _wmi, psutil._psutil_windows (total: 2)
'''
The issue doesn't reproduce consistently, so I suspect it's a race between the cleanup of the thread and the process. I can take a look this weekend if you want.
Additionally, if you call io.kill()
, it might throw a ValueError which should probably be caught and ignored.
// Edit: Actually, crash of Python is caused by the deamon thread printing an exception to stderr, causing a locking issue at shutdown. One solution is to catch all exceptions in _read_in_thread
to prevent Python fatal error.
Thank you, I've changed the reading thread to just ignore all errors.
Additionally, if you call
io.kill()
, it might throw a ValueError which should probably be caught and ignored.
Can you elaborate please?
Your last commit fixes both issues. The ValueError
exception was triggered by proc_stdout.read(1) -> ValueError: PyMemoryView_FromBuffer(): info->buf must not be NULL
on process kill.
This is heavily inspired by https://github.com/masthoon/pwintools and #996
Allows to use the
process
tube on a Windows host running local processes. I've tried to keep the changes at a minimum. Windows doesn't support non-blocking reads, so I've opted to handle the reading in a separate thread to simulate the non-blocking check.Fixes #1930