Improve injection error reporting

[hakril]

After discussing injections error with MsStore python (#72 #73). I have improved the error reporting when the injection fail with a InjectionFailedError().

The new message contains the error code and error message of the GetLastError() in case of LoadLibrary fail.

Exemple:

OSError: [WinError 126] The specified module could not be found.

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

Traceback (most recent call last):
  File "C:\Users\[XXX]\tstinjecterror.py", line 9, in <module>
    mod = p.load_library(r"C:\Users\[XXX]\NOFILE.dll")
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\users\[XXX]\pythonforwindows\windows\winobject\process.py", line 1126, in load_library
    dllbase = windows.injection.load_dll_in_remote_process(self, dll_path)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\users\[XXX]\pythonforwindows\windows\injection.py", line 236, in load_dll_in_remote_process
    raise myexc
windows.injection.InjectionFailedError: Injection of <C:\Users\[XXX]\NOFILE.dll> failed due to error <[WinError 126] The specified module could not be found.> in injected process

Return value of injection code on suspended process have also been fixed. (perform_manual_getproc_loadlib_X).
True was only returned when the module address was expected.

Related tests have been added.

[github-actions]

PyTest Results for 3.6-32

469 tests  +14   441 ✅ +14   3m 33s ⏱️ +25s
  1 suites ± 0    28 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 98abd73. ± Comparison against base commit 16242d3.

♻️ This comment has been updated with latest results.

[hakril]

@dariushoule, if you have any feedback, I would be glad to ear them !

[github-actions]

PyTest Results for 3.6-64

469 tests  +14   462 ✅ +14   3m 2s ⏱️ +22s
  1 suites ± 0     7 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 98abd73. ± Comparison against base commit 16242d3.

♻️ This comment has been updated with latest results.

[github-actions]

PyTest Results for 2.7-32

469 tests  +14   441 ✅ +15   3m 0s ⏱️ +17s
  1 suites ± 0    28 💤 ± 0 
  1 files   ± 0     0 ❌  -  1 

Results for commit 98abd73. ± Comparison against base commit 16242d3.

♻️ This comment has been updated with latest results.

[github-actions]

PyTest Results for 3.11-64

469 tests  +14   449 ✅ +14   2m 37s ⏱️ +3s
  1 suites ± 0    20 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 98abd73. ± Comparison against base commit 16242d3.

♻️ This comment has been updated with latest results.

[github-actions]

PyTest Results for 3.11-32

469 tests  +14   428 ✅ +14   2m 53s ⏱️ +9s
  1 suites ± 0    41 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 98abd73. ± Comparison against base commit 16242d3.

♻️ This comment has been updated with latest results.

[github-actions]

PyTest Results for 2.7-64

469 tests  +14   462 ✅ +14   3m 15s ⏱️ +40s
  1 suites ± 0     7 💤 ± 0 
  1 files   ± 0     0 ❌ ± 0 

Results for commit 98abd73. ± Comparison against base commit 16242d3.

♻️ This comment has been updated with latest results.

[dariushoule]

@hakril I tried this on the original machine I was having issues with and it dramatically cuts down ambiguity in troubleshooting. I think its a great improvement.

[dariushoule]

FYI: I have a work-in-progress pitch for you (branched off this MR)

Before I get too far I wanted to get your thoughts on potentially expanding support for mspython using the mechanism I've demonstrated in that branch. I have it closer to fully functional with a "dll-copy-before-load" approach (working well in manual tests, automated tests still have issues but a lot closer to target state).

High-level:

1. Copy the DLLs from the mspython interpreter to a temp dir (vcruntime.dll, python3x.dll, dlls/*)
   • NB: Restrictive ACLs no longer cascade to the DLLs in temp after copy
2. Load vcruntime and python dlls from the temp dir using shellcode as before
3. Set PYTHONHOME and PYTHONPATH using a SetEnvironmentVariableA shellcode so that python module DLLs get loaded from the temp dir, and builtins get loaded from the original interpreter dir
4. Run the injected python as normal 🎉

Note we only apply these steps as a fallback if the original shellcode fails with an access denied, and the current process token indicates its running in a windows app package.

Obviously some drawbacks here as well, namely some noticeable latency and noisiness. Weighed against not working at all I'd still say its worth considering? 🧐

The branch also fixes more hard failures in the test suite than the PR I previously opened, accounting for the fact CoInitializeSecurity seems to start already initialized in mspython builds.

FWIW this unblocks my use case, which is using mspython and PFW in environments with restrictive GPOs / AppLocker.

[hakril]

In my opinion, this approach is too intrusive to make it happen under the hood when someone only wants to execute Python.
Currently, PythonForWindows code does not create new files, write in ones or copy files (at the obvious exception of tests).
For this reason, I won't add mspython_acl_workaround in execute_python_code.

But I like that you could make it work and I am sure that your code may help other people with the same problem.
If that's okay with you, my proposition would be adding mspython_acl_workaround as a code sample in both samples/process and the documentation (https://hakril.github.io/PythonForWindows/build/html/sample.html#processes) and add a link to this sample in the documentation for WinProcess.execute_python where we will describe the problem with mspython.

Your change of initsecurity is good for me, as it's the same logic used in cpython itself :D https://github.com/python/cpython/blob/0ef8d470b79889de065e94cecd0ee01e45037d3a/PC/_wmimodule.cpp#L90

I hope this solution will suit you.

[dariushoule]

That's a reasonable take - it is not exactly a subtle approach and I agree there's something of a "consent" factor when making operations like that. I'd very much appreciate being able to add this as an example and documentation note.

One benefit of showcasing it as a code sample is I could drop the SetEnvironmentVariabe shellcode. In a sample we'd know what the environment needs to look like at create time so lpEnvironment could be used.

If I'm following correctly then my current plan is:

1. Wait for this PR to get merged
2. Update Improved behavior when running under Microsoft Store python interpreters #73 to reflect the 3 tests as discussed
3. Update Improved behavior when running under Microsoft Store python interpreters #73 to reference the mspython_acl_workaround in the process documentation and a code sample

Does this sounds good to you?

[hakril]

Good for me !