Fix TS_ALL_PROCESSES_INFO parsing for RpcWinStationGetAllProcesses#2155
Fix TS_ALL_PROCESSES_INFO parsing for RpcWinStationGetAllProcesses#2155alexisbalbachan wants to merge 2 commits intofortra:masterfrom
Conversation
|
regarding getSid issue, on my tests, pSid has never been populated by the target. not clear to me when this condition changes. This code on the other hand is able to trigger the bug: |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect/heuristic parsing of RpcWinStationGetAllProcesses by modeling the response using the protocol’s NDR structures, and updates the example usage accordingly (addresses #1816 crash).
Changes:
- Implement proper NDR structures for
TS_ALL_PROCESSES_INFOand updateRpcWinStationGetAllProcessesResponseto return parsed process entries. - Replace LDAP3-based SID formatting with local SID canonical formatting helper(s).
- Update
examples/tstool.pyand add unit tests covering SID formatting andTS_ALL_PROCESSES_INFO.getSid().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
impacket/dcerpc/v5/tsts.py |
Reworks RpcWinStationGetAllProcesses response parsing to use protocol-correct NDR structures; updates SID handling helpers. |
examples/tstool.py |
Adapts tasklist/taskkill output to the new parsed structures and corrected image name / SID accessors. |
tests/misc/test_tsts.py |
Adds regression tests for binary SID canonicalization and parsing of populated TS_ALL_PROCESSES_INFO SID pointers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import unittest | ||
|
|
||
| from impacket.dcerpc.v5 import tsts | ||
|
|
There was a problem hiding this comment.
This new test module doesn’t include the standard Impacket test header/shebang seen in other tests/misc/test_*.py files (e.g., tests/misc/test_utils.py:1-10). Consider adding the same header here for consistency and licensing clarity.
|
|
||
| import struct | ||
| from datetime import datetime, timedelta | ||
| from ldap3.protocol.formatters.formatters import format_sid | ||
|
|
||
| from impacket.dcerpc.v5 import transport | ||
| from impacket.uuid import uuidtup_to_bin, bin_to_string, string_to_bin |
There was a problem hiding this comment.
import struct appears to be unused after removing the heuristic parsing logic (no remaining struct. usages in this module). Consider removing this import to avoid lint/packaging issues and keep dependencies minimal.
| ('KernelTime', LARGE_INTEGER), | ||
| ('ImageNameSize', RPC_UNICODE_STRING), | ||
| ('ImageNameSize', TS_UNICODE_STRING), | ||
| ('BasePriority', LONG), |
There was a problem hiding this comment.
The field name ImageNameSize is misleading now that the type is TS_UNICODE_STRING and consumers are expected to call .getValue() to obtain the image name. Per the MS-TSTS / SYSTEM_PROCESS_INFORMATION layout this field represents the image name UNICODE_STRING, so consider renaming it to ImageName (and optionally keeping a backward-compatible alias) to avoid confusion for callers.
This PR fixes #1816 by replacing the heuristic parsing of
RpcWinStationGetAllProcesseswith a proper NDR structure implementation based on the protocol spec. Since the response is now modeled according to the actualTS_ALL_PROCESSES_INFOlayout, the example code was also updated to use the corrected process and SID fields.