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

EccoTheFlintstone · 2019-01-26T19:52:02Z

had to fix enable_se_debug that seemed not to work (access to protected processes info does not work with old method, works with enabling se_debug on the whole process token (and not only 1 thread))
starting from windows 8.1, get commandline value using NtQueryInformationProcess + ProcessCommandLineInformation (see Windows >= 8.1 : get commandline through NtQueryInformationProcess + ProcessCommandLineInformation #1384), this way we can get commandline for protected processes (smss.exe, msmpeng.exe, ...) instead of getting AccessDenied
Do this only if PEB read fails (by default, still use PEB read to be able to get actual process arguments, see https://blog.xpnsec.com/how-to-argue-like-cobalt-strike/)

…cted processes with QUERY_LIMITED_INFORMATION)

…orks for protected processes this way)

giampaolo · 2019-01-26T19:53:52Z

Beautiful! I will try take a look at this soon.

…InformationProcess (this way, we get the real params used even if they've been patched in the PEB)

giampaolo · 2019-01-29T16:30:48Z

psutil/arch/windows/process_info.c

+            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) {


In order to stay consistent with the rest of the code I suggest to check for failure instead of success. This way we will avoid long if blocks and indentation. Also avoid else at all (you save one indented block). In practice:

if (NtQueryInformationProcess == NULL) { PyErr_SetFromWindowsErr(0); goto out; } ...

giampaolo · 2019-01-29T16:31:16Z

psutil/arch/windows/process_info.c

+                }
+
+                hProcess = psutil_handle_from_pid(pid, PROCESS_QUERY_LIMITED_INFORMATION);
+                if (hProcess != NULL) {


Same here (check for failure, not success)

giampaolo · 2019-01-29T16:31:28Z

psutil/arch/windows/process_info.c

+                        &ret_length
+                    );
+                    CloseHandle(hProcess);
+                    if (NT_SUCCESS(status)) {


giampaolo · 2019-01-29T16:32:08Z

psutil/arch/windows/security.c

@@ -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(),


looks like this was accidental/unnecessary

giampaolo · 2019-01-29T16:32:17Z

psutil/arch/windows/security.c

                          &hToken)
       ) {
        if (GetLastError() == ERROR_NO_TOKEN) {
            if (!ImpersonateSelf(SecurityImpersonation)) {
                CloseHandle(hToken);
                return 0;
            }
-            if (!OpenThreadToken(GetCurrentThread(),
+			if (!OpenProcessToken(GetCurrentProcess(),


looks like this was accidental/unnecessary

giampaolo · 2019-01-29T16:32:27Z

psutil/arch/windows/security.c

@@ -198,17 +195,15 @@ psutil_set_se_debug() {
 int
 psutil_unset_se_debug() {
    HANDLE hToken;
-    if (! OpenThreadToken(GetCurrentThread(),
+	if (!OpenProcessToken(GetCurrentProcess(),


looks like this was accidental/unnecessary

giampaolo · 2019-01-29T16:32:33Z

psutil/arch/windows/security.c

                          &hToken)
       ) {
        if (GetLastError() == ERROR_NO_TOKEN) {
            if (! ImpersonateSelf(SecurityImpersonation))
                return 0;
-            if (!OpenThreadToken(GetCurrentThread(),
+			if (!OpenProcessToken(GetCurrentProcess(),


looks like this was accidental/unnecessary

giampaolo · 2019-01-29T16:56:34Z

psutil/arch/windows/process_info.c

+    DWORD dwBuildNumber;
+    static int windows_version = WINDOWS_UNITIALIZED;
+    // didn't get version yet
+    if (windows_version == WINDOWS_UNITIALIZED) {


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

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

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

giampaolo · 2019-01-29T16:59:40Z

psutil/arch/windows/process_info.c

+
+            hProcess = psutil_handle_from_pid(pid, PROCESS_QUERY_LIMITED_INFORMATION);
+            if (hProcess == NULL) {
+                PyErr_SetFromWindowsErr(0);


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

psutil/arch/windows/process_info.c

giampaolo · 2019-01-29T17:03:25Z

psutil/arch/windows/process_info.c

+            // Windows 10, Windows Server 2016
+            else if (dwMajorVersion == 10) {
+                windows_version = WINDOWS_10;
+            }


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

giampaolo · 2019-01-29T17:06:53Z

psutil/arch/windows/security.c

                          TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
-                          FALSE,


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

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

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.

giampaolo · 2019-01-29T17:25:09Z

psutil/arch/windows/process_info.c

+    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) {


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.

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

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).

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

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.

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?

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.

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; }

...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

hint: function name could be psutil_get_cmdline_data

giampaolo · 2019-01-29T17:30:15Z

psutil/arch/windows/process_info.c

+            GetModuleHandleA("ntdll.dll"), "NtQueryInformationProcess");
+    }
+    return NtQueryInformationProcess;
+}


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.

giampaolo · 2019-01-29T17:30:56Z

psutil/arch/windows/process_info.c

@@ -162,6 +162,72 @@ const int STATUS_INFO_LENGTH_MISMATCH = 0xC0000004;
 const int STATUS_BUFFER_TOO_SMALL = 0xC0000023L;


+
+#define WINDOWS_UNITIALIZED 0


Typo? WINDOWS_UNITIALIZED -> WINDOWS_UNINITIALIZED

giampaolo · 2019-02-01T16:09:18Z

psutil/arch/windows/security.c

                          TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY,
-                          FALSE,


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.

giampaolo · 2019-02-01T16:14:55Z

psutil/arch/windows/process_info.c

+    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) {


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).

giampaolo

hopefully the last change

giampaolo · 2019-02-01T21:32:46Z

psutil/arch/windows/process_info.c

+
+    hProcess = psutil_handle_from_pid(pid, PROCESS_QUERY_LIMITED_INFORMATION);
+    if (hProcess == NULL) {
+        // psutil_handle_from_pid sets errorcode/exception, don't need to do it it


I think you need to free(cmdline_buffer); here - you can use a goto error and put all the cleanup logic in there

giampaolo · 2019-02-01T21:33:02Z

psutil/arch/windows/process_info.c

+        // set error before closing handle to keep original error
+        // CloseHandle might fail and set a new errno/GetLastError
+        PyErr_SetFromWindowsErr(0);
+        CloseHandle(hProcess);


Same here cmdline_buffer has to be freed (as goto error)

giampaolo · 2019-02-01T22:01:35Z

psutil/arch/windows/process_info.c

+    cmdline_buffer_wchar = (WCHAR *)calloc(string_size, sizeof(WCHAR));
+
+    if (cmdline_buffer_wchar == NULL) {
+        free(cmdline_buffer);


...and here (use goto instead)

giampaolo · 2019-02-03T12:56:58Z

Merged. Thanks for the hard work. ;)

EccoTheFlintstone · 2019-02-03T17:34:02Z

Hi, thanks for the merge There's a slight problem with your latest commit in psutil_get_cmdline_data: return 0; error: if (cmdline_buffer != NULL) free(cmdline_buffer); if (hProcess != NULL) CloseHandle(hProcess); return -1; the free() and CloseHandle() are always necessary Before, ret was set by default to -1 (error) and was set to 0 (success) only at the last moment to distinguish between failure and success All the cleanup was performed at the end for clarity as it is now, hProcess is not closed and cmdline_buffer is not freed (but it should be)

…

-- Securely sent with Tutanota. Get your own encrypted, ad-free mailbox: https://tutanota.com Feb 3, 2019, 7:57 AM by [email protected]:

Merged. Thanks for the hard work. ;) — You are receiving this because you authored the thread. Reply to this email directly, > view it on GitHub <#1398 (comment)>> , or > mute the thread <https://github.com/notifications/unsubscribe-auth/AfRyOG6SojzHdQRKELYpfgLbYhV1LVM9ks5vJtyegaJpZM4aUWYD>> .

…plementations can be called separately (and tested separately)

…t time to get the right buf size (ProcessHacker does this)

* origin/master: (150 commits) Linux / CPU freq, fixes giampaolo#1481 improve pmap.py script reuse ps.py script in psutil.test() move get_terminal_size() in _compat.py improve ps.py script improve ps.py script move bytes2human() into psutil._common and reused it from scripts dir fix windows failure re. py 2 vs. 3 fix linux tests fix giampaolo#1474: fix formatting of psutil.tests() which mimicks 'ps aux' output give CREDITS for giampaolo#1480 remove outdated tests Fix read access violation in psutil.cpu_count(logical=False) (giampaolo#1480) update doc update doc [Win] Process IO priority constants + high priority (giampaolo#1479 / giampaolo#1476) don't fail if there are not prev failed tests fix giampaolo#1478: add make command to re-run tests failed on last run [Win] return value is not properly handled for undocumented NT* Windows APIs. (giampaolo#1477) fix error on py 2.7 where OSError doesn't always have winerror attribute update HISTORY for giampaolo#1475 properly check OSError.winerror refactor ionice() on Linux refactor ionice() on Linux ionice test refactoring giampaolo#1404: fix regression not returning CPUs > 9 give CREDITS to Daniel Beer for giampaolo#1471 Fix spurious exception when iterating processes on Solaris (giampaolo#1471) give CREDITS for giampaolo#1470 Fix corner case when /etc/mtab doesn't exist and procfs=/proc (giampaolo#1470) update DEVNOTES Typo fixed (giampaolo#1469) giampaolo#1458: implement colors on Windows update HISTORY / CREDITS, fix some C warnings Make uptime type consitent to fix boot time error. (giampaolo#1225) fix giampaolo#1463: cpu_distribution.py script is broken update issue template issue giampaolo#1404 / linux / phys CPUs count Big docfix (giampaolo#1464) Big docfix (giampaolo#1464) Make tests invariant to LANG setting (giampaolo#1462) test runner: show errors on KeyboardInterrupt test runner refactoring (avoid code duplication) Coloured tests (giampaolo#1459) pre-release [Windows] calculate USS memory by using NtQueryVirtualMemory (giampaolo#1453) fix version highlighting in docs/index (giampaolo#1455) fix version highlighting in README (giampaolo#1454) run win specific tests twice as fast test refactoring test: avoid failing at import time mention how to run tests in INSTALL guide giampaolo#1448: fix Wine support due to missing rtlIpv6AddressToStringA update HISTORY Fix giampaolo#1329: [AIX] disable some functions based on availability in libperfstat (giampaolo#1349) bump up version, fix some doc issues fix ResourceWarning pre-release fix giampaolo#1447: we weren't use @wrap_exceptions around oneshot() (doh\!) update doc + change git hook location update doc make pre-release checks/install src dist in a venv add new make command to check tar.gz sanity move doc; rephrase it a bit add issue templates for 'bug' and 'enhancement' types remove issue template commited by accident Update issue templates giampaolo#1291: (BACKWARD-INCOMPATIBLE) remove memory_maps() on OSX Restore Win-7 support on GIT master (5.5.1 was OK) (giampaolo#1446) try to fix ntext.h restore previous def fix compiler warning fix compiler warning fix compiler warning fix compiler warning take defs from PH set proper SYSTEM_PROCESS_INFORMATION struct from PH fix compilation warnings giampaolo#1398 / win / cmdline: call NtQueryInformationProcess twice, the first time to get the right buf size (ProcessHacker does this) update doc better print formatting for print scripts fix giampaolo#1442: use python3 as Makefile default appveyor: run print scripts after tests highlight top 6 slowest calls add printerr() and exit() to shared utils module add arg parser for ad script introduce a new scriptsutils.py private module shared by all internal utils + refactor print_access_speed.py script giampaolo#1291 / OSX: mark memory_maps() as deprecated and make it alwats raise AccessDenied OSX memory_maps() - add error handling add script for to benchmark API calls move access_denied script _assert_alive() refactor (linux) refactor fix NetBSD: Process.connections() may return incomplete results if using oneshot() giampaolo#1439 add win tests related to send_signal(CTRL_C_EVENT) giampaolo#1227 fix win test fix win tests fix backslash warnings refactor README a bit #fix 1438: do not return any parent() for PID 0 + update doc ...

ecco added 2 commits January 26, 2019 14:24

modify se_debug enable (this way, no access denied when opening prote…

463f9b8

…cted processes with QUERY_LIMITED_INFORMATION)

starting from windows 8.1, get command line using alternate method (w…

c09a63c

…orks for protected processes this way)

ecco added 3 commits January 26, 2019 15:04

forgot a free in case of error

b53fe97

typo whoops

6aba45c

windows >= 8.1 : default to reading PEB, in case of error use NtQuery…

7065a98

…InformationProcess (this way, we get the real params used even if they've been patched in the PEB)

giampaolo requested changes Jan 29, 2019

View reviewed changes

ecco added 2 commits January 29, 2019 11:41

spaces instead of tabs..

af24702

check for failures instead of success

875ab8e

giampaolo requested changes Jan 29, 2019

View reviewed changes

ecco added 2 commits January 29, 2019 14:22

giampaolo comments

d81b610

comment on static variable

05e74a8

giampaolo requested changes Feb 1, 2019

View reviewed changes

ecco added 3 commits February 1, 2019 12:16

requested fixes

b817434

logic error

1c2a06e

bugfix : bad variable name

a827a98

giampaolo requested changes Feb 1, 2019

View reviewed changes

ecco added 2 commits February 3, 2019 03:57

cleanup logic at the end (and fix memleak in case of error..)

db068de

comments

1042b15

giampaolo merged commit a453659 into giampaolo:master Feb 3, 2019

giampaolo added a commit that referenced this pull request Feb 15, 2019

#1398 #1348 / win / cmdline: refactor code so that the 2 cmdline() im…

f1374c3

…plementations can be called separately (and tested separately)

amerry mentioned this pull request Feb 27, 2019

cmdline() can now throw OSError instead of AccessDenied #1443

Closed

giampaolo added a commit that referenced this pull request Feb 28, 2019

#1398 / win / cmdline: call NtQueryInformationProcess twice, the firs…

59e3c5e

…t time to get the right buf size (ProcessHacker does this)

giampaolo mentioned this pull request Feb 28, 2019

Consider using NtQuerySystemInformation to retrieve process exe() #1445

Closed

giampaolo added the windows label Nov 15, 2020

aresstokrat mentioned this pull request May 15, 2021

[BUG] Access denied in 12hours to local drive? what is it ? swar/Swar-Chia-Plot-Manager#319

Closed

		@@ -162,6 +162,72 @@ const int STATUS_INFO_LENGTH_MISMATCH = 0xC0000004;
		const int STATUS_BUFFER_TOO_SMALL = 0xC0000023L;



		#define WINDOWS_UNITIALIZED 0

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

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

Conversation

EccoTheFlintstone commented Jan 26, 2019 • edited Loading

giampaolo commented Jan 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

giampaolo Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo commented Feb 3, 2019

EccoTheFlintstone commented Feb 3, 2019 via email

EccoTheFlintstone commented Jan 26, 2019 •

edited

Loading

giampaolo Feb 1, 2019 •

edited

Loading

giampaolo Feb 1, 2019 •

edited

Loading

giampaolo Feb 1, 2019 •

edited

Loading