Improved behavior when running under Microsoft Store python interpreters

[dariushoule]

Proposing a small changeset as a result of investigating #72

What I've discovered is the windows store build of python is sandboxed - meaning other processes cannot access its runtime. This is the security configuration of windows store apps by default. So pythonXX.dll and others are set as such they're not executable in the remote process and LoadLibraryA yields an ERROR_ACCESS_DENIED.

I toyed with the idea of automatically copying the interpreter outside of the sandbox to work around, as well as prompting for UAC elevation to apply more open ACLs as a convenience. Ultimately I think the best option here is probably just going to be telling the user that sandboxed python installations are incompatible with the remote python execution feature. The other options I explored are filled with edge cases and thorny-bits. Open to thoughts 🧠

As it stands you'll find three changes in this MR:

1. Fix test_get_current_process_modules, which failed on Microsoft Store builds of python because the app execution alias filename doesn't match the PEB. If we lean on GetModuleFileNameA we avoid the confusion caused by the NTFS reparse points.

2. Fix test_current_process_pe_imports which failed on Microsoft Store builds of python because the store builds have no direct dependency on kernel32.dll. I am proposing that we instead look for the python DLL dependency which we know will always be present, and rely on the first imported method.
   

3. Lastly, if attempting remote python execution on a sandboxed python interpreter the library now provides a helpful error message explaining why the operation would ultimately fail.

• Tested this on python 2/3 to make sure I did my best to avoid regressing compatibility.

[hakril]

Hi !

Thank you for the PR, I will try to have a in depth look in the next few days. I am curious about the inner working of "Microsoft Store python" and would like to explore it before giving my feedback on the PR.

I will come back to it soon !

[dariushoule]

Thanks for the consideration @hakril, look forward to your findings. I am open to reworking / feedback.

[hakril]

I toyed a little bit with the MsStore python.exe (I will call it mspython.exe for simplicity). Thank you for the mention of NTFS reparse points, it saved me some time.

Concerning the tests: I am open to change but from what I understand other parts of PFW will still not work with mspython.exe. I tried loading the full test suite and it exploded at windows.system.task_scheduler 😅.
What tests did you run ? Only test_process.py ?

For test_get_current_process_modules I think we can prevent using GetModuleFileNameA and directly go via the peb with assert os.path.basename(windows.current_process.peb.ProcessParameters[0].ImagePathName.str) == windows.current_process.peb.modules[0].name.

For test_current_process_pe_imports there is really no requirement to test the PE imports on python.exe. I just supposed python.exe importing kernel32.dll would be an invariant. In this case we can tests on peb.modules[2] that should be kernel32.dll or kernelbase.dll and test for ntdll.dll import with function NtCreateFile.

Lastly, concerning the main point of injection.py. I would not say that it's really sandboxed just that the C:\Program Files*\WindowsApps\ subdirectories have non-permissive ACL concerning execution.
I am not in favor of preemptively raising a ValueError based on the path when there are many scenarios in which the injection would work. For example, a user could have modified the ACL as you said, but injection in a nt authority\system process would also work.
I am considering modifying the code of the injector to be aware of the error in the injected process and returns comprehensive exception allowing us to know what was wrong.
In our case the error would look like:

PermissionError: [WinError 5] Access is denied.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:\Users\[XXX[\injection\tstinjecterror.py", line 8, in <module>
    mod = p.load_library(r"C:\Users\[XXX]\tstinjection\noexec_wintrust.dll")
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\users\[XXX]\pythonforwindows\windows\winobject\process.py", line 1164, in load_library
    dllbase = windows.injection.load_dll_in_remote_process(self, dll_path)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\users\[XXX]\pythonforwindows\windows\injection.py", line 175, in load_dll_in_remote_process
    raise myexc
windows.injection.InjectionFailedError: Injection of <C:\Users\[XXX]\tstinjection\noexec_wintrust.dll> failed due to error <[WinError 5] Access is denied.> in injected process

This would also allow to have a better understanding of other errors like:

windows.injection.InjectionFailedError: Injection of <C:\Users\[XXX]\tstinjection\NOFILE.dll> failed due to error <[WinError 126] The specified module could not be found.> in injected process

What are your thoughts in this solution ?

[dariushoule]

@hakril appreciate the thoughtful response. Noted re: sandbox - I am probably using the term incorrectly here.

In the hopes of keeping the MR small/focused on my specific issue I only worked on test_process.py. There are assuredly mspython related failures elsewhere to work on.

The test feedback makes sense, no notes. I greatly prefer your solution to the hardcoded path check. Is it possibly worth a footnote on the process docs to help guide users who run into that ACL footgun?

Would you like me to implement your feedback in this request, or would you like to take it from here? I will not be put off either way 🙂

[dariushoule]

Related: should we skip certain tests when we know they can't be run on mspython?

[hakril]

Yeah, the testing on test_process.py makes sense. For the implementation, I prefer to handle the changes in injection.py as I will explore the best way to adapt each shellcode. But I will gladly let the update of the tests to you.

I completely agree for the documentation update, putting it in the InjectionFailedError would make it tedious to read.
Do you have any feedback on where to put it ? Did you check some specific parts of the documentation when searching for a solution to your bug ?

I that is OK for you, Il will work on the update of injection.py in a different PR and @ you on it for your feedback when it’s ready ;)

For now I don't think implementing skip of certain tests for mspython is revelant as starting the full test-suite crash before executing a single test.
(python -m pytest .\tests)

[dariushoule]

I'm going to have time to circle back to this soon -- keeping this open for a little longer if thats alright. I'm finding myself exploring a couple more workarounds that could lean either on "copy-before-load", or reflective DLL injection. I'll propose it to you if I find something with good success. With your improved your error reporting PR in place I think it could be implemented decently as a fallback.

Re: documentation -- I looked @ https://hakril.github.io/PythonForWindows/build/html/process.html#windows.winobject.process.WinProcess.execute_python
originally to see if there were some notes on why this could fail.

I'll propose a small verbiage update.

[dariushoule]

@hakril This is ready for your 👀 again. Feedback should be addressed.

Let me know what you think, maintainer edits are allowed if you'd like to tweak anything.

[hakril]

Thank you for the update ! It was almost perfect for me.

I took the liberty to make some changes and push it to your branch:

• Added some print for better comprehension when executing the sample
• Reimplemented retrieve_last_exception_data with your logic, as when testing the sample I add some exception I could not debug
• Added you sample to https://hakril.github.io/PythonForWindows/build/html/sample.html
  -- Included an output of execution
• Changed your reference to the raw sample .py in WinProcess.execute_python to a link to the html/sample.html section

And that's it !
I did not commit the updated build/html directory as your PR is a little behind and did not take a new version number into account. I will regenerate + push the build/html once I merged the PR.

If these changes are OK for you, I I’m good to go and merge it !
Thank you again for your interest in this project and for the PR !

[dariushoule]

Thanks @hakril, I appreciate you!

Merge away whenever it makes sense on your side 🚀