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

Add support to start a process on Windows #2310

Merged
merged 11 commits into from Jan 2, 2024

Conversation

peace-maker
Copy link
Member

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

This is heavily inspired by https://github.com/masthoon/pwintools

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.
`os.getuid()` is only supported on unix.
@peace-maker
Copy link
Member Author

@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?

https://github.com/masthoon/pwintools/blob/ffb00174d758ce3886301d1fbfd5183ac307c5ba/pwintools.py#L479-L486

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.

@masthoon
Copy link

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.

@peace-maker
Copy link
Member Author

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 and subprocess.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?

@Arusekk
Copy link
Member

Arusekk commented Dec 1, 2023

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.

Instead of handling the timeout for reading poorly in `process.recv_raw`,
wait for any bytes to be available in `process.can_recv_raw` like on linux.

This prevents a 100% spin of one core in `tube.interactive()` since it
expects the thread to block for `timeout` seconds while we would return
immediately previously.
This loses the `readmem` and `writemem` feature, but that's not
necessary for this basic process startup support.
@peace-maker
Copy link
Member Author

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.

@masthoon
Copy link

masthoon commented Dec 5, 2023

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.

This could cause a deadlock / fastfail on shutdown when stdout is closed, thus read(1) fails
and the exception is tried to be printed to the console while stderr is in the process of getting
closed.
@peace-maker
Copy link
Member Author

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?

@masthoon
Copy link

masthoon commented Jan 2, 2024

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.

Copy link
Member

@Arusekk Arusekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, even though it is quite much code:)

@@ -751,7 +804,7 @@ def close(self):
try:
fd.close()
except IOError as e:
if e.errno != errno.EPIPE:
if e.errno != errno.EPIPE and e.errno != errno.EINVAL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EINVAL? how? :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's returned when the fd was already closed somewhere.

@peace-maker peace-maker merged commit c4a3d34 into Gallopsled:dev Jan 2, 2024
12 of 13 checks passed
@peace-maker peace-maker deleted the windows_processes branch January 2, 2024 20:12
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.

launch processes on windows
3 participants