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.

Sign up for GitHub

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

Starting from windows 8.1, get commandline content using NtQueryInformationProcess (see #1384) #1398

Merged
merged 14 commits into from
Feb 3, 2019
Merged

Starting from windows 8.1, get commandline content using NtQueryInformationProcess (see #1384) #1398

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
463f9b8
modify se_debug enable (this way, no access denied when opening prote…
Jan 26, 2019
c09a63c
starting from windows 8.1, get command line using alternate method (w…
Jan 26, 2019
b53fe97
forgot a free in case of error
Jan 26, 2019
6aba45c
typo whoops
Jan 26, 2019
7065a98
windows >= 8.1 : default to reading PEB, in case of error use NtQuery…
Jan 29, 2019
af24702
spaces instead of tabs..
Jan 29, 2019
875ab8e
check for failures instead of success
Jan 29, 2019
d81b610
giampaolo comments
Jan 29, 2019
05e74a8
comment on static variable
Jan 30, 2019
b817434
requested fixes
Feb 1, 2019
1c2a06e
logic error
Feb 1, 2019
a827a98
bugfix : bad variable name
Feb 1, 2019
db068de
cleanup logic at the end (and fix memleak in case of error..)
Feb 3, 2019
1042b15
comments
Feb 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 134 additions & 4 deletions psutil/arch/windows/process_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,72 @@ const int STATUS_INFO_LENGTH_MISMATCH = 0xC0000004;
const int STATUS_BUFFER_TOO_SMALL = 0xC0000023L;



#define WINDOWS_UNITIALIZED 0
Copy link
Owner

Choose a reason for hiding this comment

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

Typo? WINDOWS_UNITIALIZED -> WINDOWS_UNINITIALIZED

#define WINDOWS_XP 51
#define WINDOWS_VISTA 60
#define WINDOWS_7 61
#define WINDOWS_8 62
#define WINDOWS_81 63
#define WINDOWS_10 100


int get_windows_version() {
OSVERSIONINFO ver_info;
BOOL result;
DWORD dwMajorVersion;
DWORD dwMinorVersion;
DWORD dwBuildNumber;
static int windows_version = WINDOWS_UNITIALIZED;
// didn't get version yet
if (windows_version == WINDOWS_UNITIALIZED) {
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this is unnecessary as windows_version is already set as WINDOWS_UNITIALIZED in the previous line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

windows_version is a static var, this check is taken only the first time the function is called

Copy link
Owner

Choose a reason for hiding this comment

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

Gotcha. I'd add a comment then ('cause I will forget for sure =)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

memset(&ver_info, 0, sizeof(ver_info));
ver_info.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
result = GetVersionEx(&ver_info);
if (result != FALSE) {
dwMajorVersion = ver_info.dwMajorVersion;
dwMinorVersion = ver_info.dwMinorVersion;
dwBuildNumber = ver_info.dwBuildNumber;
// Windows XP, Windows server 2003
if (dwMajorVersion == 5 && dwMinorVersion == 1) {
windows_version = WINDOWS_XP;
}
// Windows Vista
else if (dwMajorVersion == 6 && dwMinorVersion == 0) {
windows_version = WINDOWS_VISTA;
}
// Windows 7, Windows Server 2008 R2
else if (dwMajorVersion == 6 && dwMinorVersion == 1) {
windows_version = WINDOWS_7;
}
// Windows 8, Windows Server 2012
else if (dwMajorVersion == 6 && dwMinorVersion == 2) {
windows_version = WINDOWS_8;
}
// Windows 8.1, Windows Server 2012 R2
else if (dwMajorVersion == 6 && dwMinorVersion == 3)
{
windows_version = WINDOWS_81;
}
// Windows 10, Windows Server 2016
else if (dwMajorVersion == 10) {
windows_version = WINDOWS_10;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: how about using a switch/case/default block instead? Not terribly important... up to you.

}
}
return windows_version;
}

_NtQueryInformationProcess get_NtQueryInformationProcess() {
static _NtQueryInformationProcess NtQueryInformationProcess = NULL;
if (NtQueryInformationProcess == NULL) {
NtQueryInformationProcess = (_NtQueryInformationProcess)GetProcAddress(
GetModuleHandleA("ntdll.dll"), "NtQueryInformationProcess");
}
return NtQueryInformationProcess;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I like putting this in a utility function because it can be reused. Would you mind using this utility function also in other parts of the code? Also, for consistency I think it's better to rename this function to psutil_NtQueryInformationProcess.



// ====================================================================
// Process and PIDs utiilties.
// ====================================================================
Expand Down Expand Up @@ -804,10 +870,72 @@ psutil_get_cmdline(long pid) {
PyObject *py_unicode = NULL;
LPWSTR *szArglist = NULL;
int nArgs, i;
HANDLE hProcess;
int windows_version;
ULONG ret_length = 4096;
NTSTATUS status;
char * cmdline_buffer = NULL;
WCHAR * cmdline_buffer_wchar = NULL;
PUNICODE_STRING tmp = NULL;
DWORD string_size;
_NtQueryInformationProcess NtQueryInformationProcess = NULL;

windows_version = get_windows_version();
if (windows_version >= WINDOWS_81) {
// by defaut, still use PEB (if command line params have been patched in PEB, we will get the actual ones)
if (psutil_get_process_data(pid, KIND_CMDLINE, &data, &size) != 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

Mmmm... I'm a bit confused about this. It appears regardless of the Windows version you first attempt to use the PEB method first, and fallback on using NtQueryInformationProcess in case of failure on Win >= 8.1. Wouldn't it make sense to always use NtQueryInformationProcess on Win >= 8.1? I a bit against trying 2 different methods and suppressing error in case the first one fails. Unless there's a good reason I think it's better to always use 1 method only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading the PEB to get the command line parameters still seem to be the best method if somebody has tampered with the parameters after creating the process.
For instance, create a process as suspended, patch the command line in its PEB and unfreeze it.
The process will use the "new" parameters whereas the system (with NtQueryInformationProcess) will give you the "old" ones (see here : https://blog.xpnsec.com/how-to-argue-like-cobalt-strike/)
By default, reading from PEB is still allowed if one has sufficient privileges except for protected processes where one must use NtQueryInformationProcess.
So the PEB method will work for almost all processes on windows 8.1+ except the few (4-5) protected processes lying around (where we can fallback on NtQIP), and it will accurately report the actual command line parameters used

Copy link
Owner

Choose a reason for hiding this comment

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

OK, I see. In this case I would recommend the following:

  • use psutil_handle_from_pid(pid, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ)
  • if it succeeds use the PEB method (and only that)
  • if it doesn't (for any reason), use NtQueryInformationProcess if Win >= 8.1, else fail
    In both cases you should simply close the returned handle: the only reason to get the handle is to see if you have permissions to do so.
    Makes sense?

Also, please add a full explanation of why one method is preferred over the other one (you can include the information you wrote in the comment above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think using psutil_handle_from_pid(pid, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ) is redundant as reading from PEB will also open process with those rights, so for every process, we would open/close it twice

Copy link
Owner

@giampaolo giampaolo Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Mmm.. true. Another possibility is to use the PEB method and check for GetLastError() == ERROR_ACCESS_DENIED in case it fails, and only in that case use NtQueryInformationProcess. Also, for PID 0 I suspect you will always get ERROR_ACCESS_DENIED.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is, the code works. What I could do is add an explanation as why the PEB method is used first for windows >= 8.1 as you propose, but go with this code anyway, no?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes but as-is it tries the other method for any error. I would like to limit the check to ERROR_ACCESS_DENIED. If it fails for any other reason I think it's saner to just raise exception.

Copy link
Owner

@giampaolo giampaolo Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So something like this (not tested):

    int ret;

    ret = psutil_get_process_data(pid, KIND_CMDLINE, &data, &size);
    if ((ret != 0) && 
            (GetLastError() == ERROR_ACCESS_DENIED) && 
            (windows_version >= WINDOWS_81)) 
    {
        // try NtQueryInformationProcess()
        PyErr_Clear();
        ...
    }
    else {
        return NULL;
    }

Copy link
Owner

@giampaolo giampaolo Feb 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

...also, I suggest to put the whole NtQueryInformationProcess() block in a separate function, similarly to what it has been done for psutil_get_process_data

Copy link
Owner

Choose a reason for hiding this comment

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

hint: function name could be psutil_get_cmdline_data

PyErr_Clear(); // reset that we had an error, and retry with NtQueryInformationProcess
// if it fails, fallback to NtQueryInformationProcess (for protected processes)
NtQueryInformationProcess = get_NtQueryInformationProcess();
if (NtQueryInformationProcess == NULL) {
PyErr_SetFromWindowsErr(0);
goto out;
}

if (psutil_get_process_data(pid, KIND_CMDLINE, &data, &size) != 0)
goto out;
cmdline_buffer = calloc(4096, 1);
if (cmdline_buffer == NULL) {
PyErr_NoMemory();
goto out;
}

hProcess = psutil_handle_from_pid(pid, PROCESS_QUERY_LIMITED_INFORMATION);
if (hProcess == NULL) {
PyErr_SetFromWindowsErr(0);
Copy link
Owner

Choose a reason for hiding this comment

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

psutil_handle_from_pid already sets the Python errorcode/exception so this is unnecessary

goto out;
}
status = NtQueryInformationProcess(
hProcess,
60, // ProcessCommandLineInformation
cmdline_buffer,
ret_length,
&ret_length
);
CloseHandle(hProcess);
if (!NT_SUCCESS(status)) {
PyErr_SetFromWindowsErr(0);
goto out;
}
giampaolo marked this conversation as resolved.
Show resolved Hide resolved
tmp = (PUNICODE_STRING)cmdline_buffer;
string_size = wcslen(tmp->Buffer) + 1;
cmdline_buffer_wchar = (WCHAR *)calloc(string_size, sizeof(WCHAR));

if (cmdline_buffer_wchar == NULL) {
free(cmdline_buffer);
PyErr_NoMemory();
goto out;
}

wcscpy_s(cmdline_buffer_wchar, string_size, tmp->Buffer);
data = cmdline_buffer_wchar;
size = string_size * sizeof(WCHAR);
free(cmdline_buffer);
}
}
// legacy version that reads data from PEB
else {
if (psutil_get_process_data(pid, KIND_CMDLINE, &data, &size) != 0)
goto out;
}
// attempt to parse the command line using Win32 API
szArglist = CommandLineToArgvW(data, &nArgs);
if (szArglist == NULL) {
Expand All @@ -822,7 +950,7 @@ psutil_get_cmdline(long pid) {
goto out;
for (i = 0; i < nArgs; i++) {
py_unicode = PyUnicode_FromWideChar(szArglist[i],
wcslen(szArglist[i]));
wcslen(szArglist[i]));
if (py_unicode == NULL)
goto out;
PyList_SET_ITEM(py_retlist, i, py_unicode);
Expand All @@ -833,7 +961,8 @@ psutil_get_cmdline(long pid) {
py_retlist = NULL;

out:
LocalFree(szArglist);
if (szArglist != NULL)
LocalFree(szArglist);
if (data != NULL)
free(data);
Py_XDECREF(py_unicode);
Expand Down Expand Up @@ -960,3 +1089,4 @@ psutil_get_proc_info(DWORD pid, PSYSTEM_PROCESS_INFORMATION *retProcess,
free(buffer);
return 0;
}

16 changes: 6 additions & 10 deletions psutil/arch/windows/security.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ psutil_token_from_handle(HANDLE hProcess) {
* constant, we pass through the TOKEN_PRIVILEGES constant. This value returns
* an array of privileges that the account has in the environment. Iterating
* through the array, we call the function LookupPrivilegeName looking for the
* string �SeTcbPrivilege. If the function returns this string, then this
* string “SeTcbPrivilege. If the function returns this string, then this
* account has Local System privileges
*/
int
Expand Down Expand Up @@ -131,7 +131,6 @@ psutil_set_privilege(HANDLE hToken, LPCTSTR Privilege, BOOL bEnablePrivilege) {
);

if (GetLastError() != ERROR_SUCCESS) return FALSE;

// second pass. set privilege based on previous setting
tpPrevious.PrivilegeCount = 1;
tpPrevious.Privileges[0].Luid = luid;
Expand Down Expand Up @@ -160,19 +159,17 @@ psutil_set_privilege(HANDLE hToken, LPCTSTR Privilege, BOOL bEnablePrivilege) {
int
psutil_set_se_debug() {
HANDLE hToken;
if (! OpenThreadToken(GetCurrentThread(),
if (!OpenProcessToken(GetCurrentProcess(),
TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
FALSE,
Copy link
Owner

Choose a reason for hiding this comment

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

mmm this looks wrong; aren't you now passing 3 args instead of 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using GetProcessToken instead of GetThreadToken and it needs 1 less argument
In the case where the actual privileged call is performed from another thread than the one getting th SE_DEBUG privilege, it might be best to have the whole process enable the privilege

Copy link
Owner

Choose a reason for hiding this comment

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

I see (didn't notice you changed syscall). I checked ProcessHacker source code and it also uses OpenProcessToken so I guess it makes sense to do the same. Good one.

&hToken)
) {
if (GetLastError() == ERROR_NO_TOKEN) {
if (!ImpersonateSelf(SecurityImpersonation)) {
CloseHandle(hToken);
return 0;
}
if (!OpenThreadToken(GetCurrentThread(),
if (!OpenProcessToken(GetCurrentProcess(),
TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
FALSE,
&hToken)
) {
RevertToSelf();
Expand All @@ -198,17 +195,15 @@ psutil_set_se_debug() {
int
psutil_unset_se_debug() {
HANDLE hToken;
if (! OpenThreadToken(GetCurrentThread(),
if (!OpenProcessToken(GetCurrentProcess(),
TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
FALSE,
&hToken)
) {
if (GetLastError() == ERROR_NO_TOKEN) {
if (! ImpersonateSelf(SecurityImpersonation))
return 0;
if (!OpenThreadToken(GetCurrentThread(),
if (!OpenProcessToken(GetCurrentProcess(),
TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
FALSE,
&hToken))
{
return 0;
Expand All @@ -223,3 +218,4 @@ psutil_unset_se_debug() {
CloseHandle(hToken);
return 1;
}