Conversation
There was a problem hiding this comment.
Pull request overview
Adds primary token SID data to the process event context surfaced from ntosebpfext → eBPF program (process_monitor) → user-mode library/events, and updates tests/docs accordingly.
Changes:
- Extend
process_md_t/process_info_twithtoken_sid_sizeandtoken_sidand populate it in the kernel process notify path. - Decode SID in
ProcessMonitorBPFLoaderand expose it viaProcessCreatedEventArgs+ logging. - Update unit/integration tests and documentation for the new fields.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/process_monitor_bpf/process_monitor.c | Copies SID fields from hook context into ringbuf event payload. |
| tools/process_monitor.Library/ProcessMonitorBPFLoader.cs | Extends native event struct, decodes SID to string, plumbs into created-event args. |
| tools/process_monitor.Library/ProcessMonitor.cs | Includes TokenSid in structured debug log. |
| tools/process_monitor.Library/ProcessCreatedEventArgs.cs | Adds TokenSid to the public created-event args type. |
| tests/process_monitor.Tests/ProcessMonitorTests.cs | Asserts TokenSid is non-empty on create events. |
| tests/ntosebpfext/ntosebpfext_unit/ntos_ebpfext_unit.cpp | Adds SID fields to test structs and validates copy/size behavior. |
| include/ebpf_ntos_hooks.h | Defines TOKEN_SID_MAX_SIZE and adds SID fields to process_md_t. |
| ebpf_extensions/ntosebpfext/ntos_ebpf_ext_process.c | Queries primary token SID and writes it into process_md_t on create. |
| docs/ntosebpfext.md | Documents new token SID fields in the process context. |
| .gitmodules | Changes external/usersim submodule URL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (evt->operation == 0 /* 0 == PROCESS_OPERATION_COMPLETE */) | ||
| { |
| [submodule "external/usersim"] | ||
| path = external/usersim | ||
| url = https://github.com/microsoft/usersim.git | ||
| url = https://github.com/LakshK98/usersim.git |
There was a problem hiding this comment.
this will be resolved when usersim pr gets merged.
| process_operation_t operation : 8; ///< Operation to do. | ||
| uint32_t token_sid_size; ///< Size of the token SID in bytes. Set only for PROCESS_OPERATION_CREATE. | ||
| uint8_t token_sid[TOKEN_SID_MAX_SIZE]; ///< Primary token SID. Set only for PROCESS_OPERATION_CREATE. | ||
| } process_md_t; |
| var sid = new SecurityIdentifier(sidBytes, 0); | ||
| tokenSidStr = sid.Value; | ||
| } | ||
| catch { /* SID conversion failure is non-fatal */ } |
There was a problem hiding this comment.
Pull request overview
This PR extends the process monitor pipeline to surface the primary token SID and resolved account identity (domain\name) for process creation events, flowing from the kernel hook context through the eBPF program into the .NET library and tests.
Changes:
- Added token SID fields to process context/event payloads and converted SID bytes to string in the .NET loader.
- Introduced new process helpers and LRU maps to capture account name and account domain.
- Updated tests and documentation to reflect the additional token-related data.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/process_monitor_bpf/process_monitor.c | Adds SID fields to ringbuf payload and stores account name/domain into new LRU maps. |
| tools/process_monitor.Library/ProcessMonitorBPFLoader.cs | Loads new maps, reads SID bytes, converts to SecurityIdentifier, and populates new event fields. |
| tools/process_monitor.Library/ProcessMonitor.cs | Extends debug logging to include SID and account identity. |
| tools/process_monitor.Library/ProcessCreatedEventArgs.cs | Adds TokenSid/AccountName/AccountDomain to the creation event contract. |
| tests/process_monitor.Tests/ProcessMonitorTests.cs | Asserts TokenSid is present on creation events. |
| tests/ntosebpfext/ntosebpfext_unit/ntos_ebpfext_unit.cpp | Adds coverage for SID passthrough and validates new account maps. |
| include/ebpf_ntos_hooks.h | Adds SID fields to process_md_t and declares new helper APIs. |
| ebpf_extensions/ntosebpfext/sys/ntosebpfext.vcxproj | Links ksecdd.lib to support new security/SID lookup usage. |
| ebpf_extensions/ntosebpfext/ntos_ebpf_ext_program_info.h | Registers the two new helper prototypes. |
| ebpf_extensions/ntosebpfext/ntos_ebpf_ext_process.c | Implements SID + account resolution and exposes account helper functions + context packing. |
| docs/ntosebpfext.md | Documents new token SID fields (but not the new helpers). |
| .gitmodules | Points external/usersim submodule URL to a fork. |
| external/usersim | Updates the submodule commit pointer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (NT_SUCCESS(lookup_status)) { | ||
| process_notify_context.account_name.Length = (USHORT)name_size; | ||
| process_notify_context.account_domain.Length = (USHORT)domain_size; | ||
| } else { | ||
| // Lookup failed; free buffers. | ||
| if (process_notify_context.account_name.Buffer != NULL) { | ||
| ExFreePool(process_notify_context.account_name.Buffer); | ||
| process_notify_context.account_name.Buffer = NULL; | ||
| } | ||
| if (process_notify_context.account_domain.Buffer != NULL) { | ||
| ExFreePool(process_notify_context.account_domain.Buffer); | ||
| process_notify_context.account_domain.Buffer = NULL; | ||
| } | ||
| } |
| // Copy image path into the LRU hash. Note we use IMAGE_PATH_SIZE - 1 to leave a guaranteed null terminator | ||
| bpf_process_get_image_path(ctx, buffer, IMAGE_PATH_SIZE - 1); | ||
| bpf_map_update_elem(&process_map, &process_info.process_id, buffer, BPF_ANY); | ||
|
|
||
| // Copy account name into the LRU hash. | ||
| memset(buffer, 0, MAX_ACCOUNT_NAME_SIZE); | ||
| bpf_process_get_account_name(ctx, buffer, MAX_ACCOUNT_NAME_SIZE - 1); | ||
| bpf_map_update_elem(&account_name_map, &process_info.process_id, buffer, BPF_ANY); | ||
|
|
||
| // Copy account domain into the LRU hash. | ||
| memset(buffer, 0, MAX_ACCOUNT_DOMAIN_SIZE); | ||
| bpf_process_get_account_domain(ctx, buffer, MAX_ACCOUNT_DOMAIN_SIZE - 1); |
| process_context->account_domain.MaximumLength = 0; | ||
| } | ||
|
|
||
| // Set command_startand command_end to point to the copied command_line buffer |
| - **Token Information:** | ||
| - `token_sid_size` - Size of the primary token SID in bytes (only valid for `PROCESS_OPERATION_CREATE`) | ||
| - `token_sid` - The raw SID bytes of the new process's primary token (only valid for `PROCESS_OPERATION_CREATE`). Maximum size is `TOKEN_SID_MAX_SIZE` (68 bytes). |
| [submodule "external/usersim"] | ||
| path = external/usersim | ||
| url = https://github.com/microsoft/usersim.git | ||
| url = https://github.com/LakshK98/usersim.git |
| var sid = new SecurityIdentifier(sidBytes, 0); | ||
| tokenSidStr = sid.Value; | ||
| } | ||
| catch { /* SID conversion failure is non-fatal */ } |
There was a problem hiding this comment.
Pull request overview
This PR extends the process monitor pipeline to surface the primary token SID and account identity (domain/name) alongside existing creation event metadata.
Changes:
- Add token SID bytes + size to the native process context and propagate them through the ring buffer into the C# event args.
- Add new eBPF helpers and per-PID LRU maps to capture account name and account domain.
- Update tests, docs, and the ntosebpfext driver to support the new fields/helpers.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/process_monitor_bpf/process_monitor.c | Adds token SID fields to event + stores account name/domain in new LRU maps. |
| tools/process_monitor.Library/ProcessMonitorBPFLoader.cs | Loads new maps and converts SID bytes to a string for managed event args. |
| tools/process_monitor.Library/ProcessMonitor.cs | Extends debug logging to include SID and account identity. |
| tools/process_monitor.Library/ProcessCreatedEventArgs.cs | Extends public event args with TokenSid/AccountName/AccountDomain. |
| tests/process_monitor.Tests/ProcessMonitorTests.cs | Adds assertions that TokenSid is non-empty on creation events. |
| tests/ntosebpfext/ntosebpfext_unit/ntos_ebpfext_unit.cpp | Extends unit tests to validate SID propagation and new maps. |
| include/ebpf_ntos_hooks.h | Extends process_md_t with SID fields and declares new helpers. |
| ebpf_extensions/ntosebpfext/sys/ntosebpfext.vcxproj | Adds ksecdd.lib dependency (needed for SID/account lookup). |
| ebpf_extensions/ntosebpfext/ntos_ebpf_ext_program_info.h | Registers new helper prototypes for account name/domain. |
| ebpf_extensions/ntosebpfext/ntos_ebpf_ext_process.c | Implements SID extraction + account lookup and helper functions; extends context packing. |
| docs/ntosebpfext.md | Documents token SID fields on process_md_t. |
| .gitmodules | Changes usersim submodule URL. |
| external/usersim | Updates usersim submodule commit pointer. |
Comments suppressed due to low confidence (1)
tools/process_monitor.Library/ProcessCreatedEventArgs.cs:1
- This changes the public
ProcessCreatedEventArgspositional signature by adding new required constructor parameters, which is a breaking change for any external consumers constructing it positionally. If backwards compatibility matters, consider adding these as optional properties with defaults (or add an additional constructor) rather than extending the primary constructor parameter list.
// Copyright (c) Microsoft Corporation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [submodule "external/usersim"] | ||
| path = external/usersim | ||
| url = https://github.com/microsoft/usersim.git | ||
| url = https://github.com/LakshK98/usersim.git |
There was a problem hiding this comment.
Changing a dependency submodule URL from the official microsoft/usersim to a personal fork introduces a supply-chain risk and can break reproducible builds. Prefer keeping the URL pointing at the upstream repository and only update the pinned commit SHA (or, if a fork is required, document the trust/ownership rationale and consider pinning via an internal mirror).
| url = https://github.com/LakshK98/usersim.git | |
| url = https://github.com/microsoft/usersim.git |
| try | ||
| { | ||
| var sidBytes = new byte[evt->token_sid_size]; | ||
| Marshal.Copy((IntPtr)evt->token_sid, sidBytes, 0, (int)evt->token_sid_size); |
There was a problem hiding this comment.
Casting the fixed buffer evt->token_sid directly to IntPtr is not valid in C# in many cases (fixed buffers typically require taking the address explicitly, e.g. using &evt->token_sid[0] or a fixed statement). As written, this risks a compile-time failure or incorrect pointer conversion; update this to obtain a stable byte* pointer to the first element before Marshal.Copy.
| Marshal.Copy((IntPtr)evt->token_sid, sidBytes, 0, (int)evt->token_sid_size); | |
| byte* tokenSidPtr = &evt->token_sid[0]; | |
| Marshal.Copy((IntPtr)tokenSidPtr, sidBytes, 0, (int)evt->token_sid_size); |
| } | ||
| } | ||
| } | ||
| #pragma warning(pop) |
There was a problem hiding this comment.
#pragma warning(pop) should be paired with a preceding #pragma warning(push); otherwise the warning state stack may be unbalanced and can trigger compiler diagnostics or unexpected warning state changes. Use #pragma warning(push) before disabling 6387 (and then pop), or explicitly restore the specific warning with #pragma warning(default: 6387).
Description
Describe the purpose of and changes within this Pull Request.
Testing
Do any existing tests cover this change? Are new tests needed?
Documentation
Is there any documentation impact for this change?
Installation
Is there any installer impact for this change?