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 for PPID spoofing #374

Merged
merged 1 commit into from Jan 22, 2020
Merged

Add support for PPID spoofing #374

merged 1 commit into from Jan 22, 2020

Conversation

phra
Copy link
Contributor

@phra phra commented Dec 16, 2019

will fix #373

@@ -407,7 +480,7 @@ DWORD request_sys_process_execute(Remote *remote, Packet *packet)

if (session_id(GetCurrentProcessId()) == session || !hWtsapi32)
{
if (!CreateProcess(NULL, commandLine, NULL, NULL, inherit, createFlags, NULL, NULL, &si, &pi))
if (!CreateProcess(NULL, commandLine, NULL, NULL, inherit, createFlags, NULL, NULL, (STARTUPINFOA*)&si, &pi))
Copy link
Contributor

@bwatters-r7 bwatters-r7 Dec 27, 2019

Choose a reason for hiding this comment

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

Why cast this larger object to a smaller contained object rather than pass the smaller contained object in itself? To be more specific, why use this:
(STARTUPINFOA*)&si
When you're effectively passing in this:
si.StartupInfo
?
It will work because the first object in the STARTUPINFOEXA object is a STARTUPINFOA object, but it would seem to be better and more clear to just pass in the STARTUPINFOA object itself? Or am I missing something?

Copy link
Contributor Author

@phra phra Jan 2, 2020

Choose a reason for hiding this comment

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

the manually declared _STARTUPINFOEXA extends the existing STARTUPINFOEXA by adding LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList since it's not available on win xp.
when i wrote the code, i thought to make explicit the fact that the struct is effectively casted down to STARTUPINFOA, but it gets interpreted as STARTUPINFOEXA when the flag EXTENDED_STARTUPINFO_PRESENT is specified.
it will work in both ways, maybe si.StartupInfo is more clear, but since the CreateProcess function can potentially access data outside si.StartupInfo i preferred to use an explicit cast. feel free to send a PR with the proposed change. 👍

Copy link
Contributor

@bwatters-r7 bwatters-r7 Jan 16, 2020

Choose a reason for hiding this comment

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

Apologies for the long delay...
Wow.... I did not know about the EXTENDED_STARTUPINFO_PRESENT attribute bit. That's.... uh... certainly one way to overload a method, and I'm a little sad that's the route MS took on it. In light of that, what you have makes perfect sense, though it's necessity makes me sad.

bwatters-r7 added a commit that referenced this issue Jan 22, 2020
Merge branch 'land-374' into upstream-master
@bwatters-r7 bwatters-r7 merged commit b35cc0a into rapid7:master Jan 22, 2020
1 check passed
@bwatters-r7
Copy link
Contributor

@bwatters-r7 bwatters-r7 commented Jan 22, 2020

Release Notes:

This adds the ability to spoof parent process ID when creating a new process.

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.

2 participants