From b5f02000d6df6e215f76700c21108069a65161b4 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 29 Jun 2026 08:11:37 +0000 Subject: [PATCH] fix(host/security): scope Windows shared-section SDDL to SYSTEM+LocalService (close #5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gamepad host<->UMDF-driver shared sections (Global\pfds-shm-*, pfxusb-shm-*) and the IDD-push frame ring/event (Global\pfvd-*) were created with `D:(A;;GA;;;WD)` — GENERIC_ALL to **Everyone** — on the assumption the driver's WUDFHost ran under a restricted token needing broad access. So any local unprivileged user could OpenFileMapping the section to inject controller input, tamper the trusted HID channel, or read captured screen frames (security-review 2026-06-28 #5). On-box validation (RTX box, 2026-06-29) disproved the restricted-token premise: the WUDFHost token is NT AUTHORITY\LocalService (S-1-5-19), SYSTEM integrity, with ZERO restricted SIDs. So the section only needs SYSTEM (the host creates + writes it) and LocalService (the driver opens it). Scope both SDDL sites to `D:(A;;GA;;;SY)(A;;GA;;;LS)`; rename the now-misnamed `permissive_sa` -> `shared_object_sa`; correct the stale "restricted-token / Everyone" docs. Validated live: a full DualSense + 1280x720x60 session — 6943 frames received, HID output round-tripped, device status OK (pf_dualsense + pf_vdisplay WUDFHosts both LocalService open the scoped sections fine), while OpenFileMapping from a non-SYSTEM admin session now returns ACCESS_DENIED (was a granted handle under WD). Host-only change (the SDDL is set when the host CREATES the section); drivers unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/capture/windows/idd_push.rs | 30 +++++++++++-------- .../src/inject/windows/gamepad_raii.rs | 18 +++++++---- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/crates/punktfunk-host/src/capture/windows/idd_push.rs b/crates/punktfunk-host/src/capture/windows/idd_push.rs index a36436c..e2c1712 100644 --- a/crates/punktfunk-host/src/capture/windows/idd_push.rs +++ b/crates/punktfunk-host/src/capture/windows/idd_push.rs @@ -1,8 +1,8 @@ -//! P2 direct frame push (kill DDA) — HOST side. The pf-vdisplay driver runs in a restricted WUDFHost -//! token that canNOT create named kernel objects, so — exactly like the gamepad UMDF drivers -//! (`inject/dualsense_windows.rs`) — the HOST (privileged) CREATES the shared header + frame-ready -//! event + ring of keyed-mutex textures (`Global\` names, permissive `D:(A;;GA;;;WD)` SDDL) on the -//! discrete render GPU, and the driver only OPENS them and copies frames in. We then consume the ring +//! P2 direct frame push (kill DDA) — HOST side. The pf-vdisplay driver's WUDFHost canNOT create named +//! kernel objects, so — exactly like the gamepad UMDF drivers (`inject/dualsense_windows.rs`) — the +//! HOST (privileged) CREATES the shared header + frame-ready event + ring of keyed-mutex textures +//! (`Global\` names, scoped `D:(A;;GA;;;SY)(A;;GA;;;LS)` to SYSTEM + the driver's LocalService host — +//! see `shared_object_sa`) on the discrete render GPU, and the driver only OPENS them and copies frames in. We then consume the ring //! straight into the zero-copy NVENC path — no DXGI Desktop Duplication, no `win32u` hook. Gated by //! `PUNKTFUNK_IDD_PUSH`. Driver counterpart: `packaging/windows/drivers/pf-vdisplay/src/ //! frame_transport.rs`. The shared `SharedHeader` layout, `MAGIC`/`VERSION`/`RING_LEN`, the @@ -236,13 +236,17 @@ pub struct IddPushCapturer { // ownership to one thread at a time with NO concurrent access; we do not (and must not) claim `Sync`. unsafe impl Send for IddPushCapturer {} -/// Build a permissive (Everyone:GenericAll) `SECURITY_ATTRIBUTES` so the restricted WUDFHost driver -/// can OPEN the host-created objects — the same `D:(A;;GA;;;WD)` SDDL the gamepad shared section uses. -/// The returned `psd` backing must outlive `sa`; both are dropped when the process exits. -unsafe fn permissive_sa() -> Result<(SECURITY_ATTRIBUTES, PSECURITY_DESCRIPTOR)> { +/// Build a `SECURITY_ATTRIBUTES` granting GENERIC_ALL to **SYSTEM** (the host creates+publishes the +/// shared event + texture ring) and **LocalService** (the account the pf_vdisplay WUDFHost runs under, +/// which consumes them) — `D:(A;;GA;;;SY)(A;;GA;;;LS)`, the same scoping as the gamepad section. The +/// old SDDL granted **Everyone** (`WD`), which let any local user open the `Global\pfvd-*` objects and +/// read captured screen frames (security-review 2026-06-28 #5). Verified on the RTX box (2026-06-29): +/// the WUDFHost token is `S-1-5-19` (LocalService), SYSTEM integrity, zero restricted SIDs — so SY+LS +/// suffices for the driver and excludes normal user processes. `psd` must outlive `sa`. +unsafe fn shared_object_sa() -> Result<(SECURITY_ATTRIBUTES, PSECURITY_DESCRIPTOR)> { let mut psd = PSECURITY_DESCRIPTOR::default(); ConvertStringSecurityDescriptorToSecurityDescriptorW( - w!("D:(A;;GA;;;WD)"), + w!("D:(A;;GA;;;SY)(A;;GA;;;LS)"), SDDL_REVISION_1, &mut psd, None, @@ -269,7 +273,7 @@ impl IddPushCapturer { h: u32, format: DXGI_FORMAT, ) -> Result> { - let (sa, _psd) = permissive_sa()?; + let (sa, _psd) = shared_object_sa()?; let mut slots = Vec::new(); for k in 0..RING_LEN { let desc = D3D11_TEXTURE2D_DESC { @@ -375,7 +379,7 @@ impl IddPushCapturer { // SAFETY: one block over the whole ring setup; every operation in it is sound: // - `set_advanced_color`/`advanced_color_enabled` are `unsafe fn`s taking only a copy of the plain // `u32` target id; they read/flip CCD display config and return owned values, borrowing nothing. - // - `CreateDXGIFactory1`, `EnumAdapterByLuid`, `make_device`, `permissive_sa`, `CreateFileMappingW`, + // - `CreateDXGIFactory1`, `EnumAdapterByLuid`, `make_device`, `shared_object_sa`, `CreateFileMappingW`, // `MapViewOfFile`, `CreateEventW`, and `create_ring_slots` are all `?`-checked, so every returned // interface/handle/view is non-error before use; `&sa`/`&adapter`/`&device`/the `&HSTRING` names // are live borrows that outlive each synchronous call, and `sa.lpSecurityDescriptor` stays valid @@ -421,7 +425,7 @@ impl IddPushCapturer { .context("EnumAdapterByLuid(render adapter) for IDD push")?; let (device, context) = make_device(&adapter).context("make_device for IDD push")?; - let (sa, _psd) = permissive_sa()?; + let (sa, _psd) = shared_object_sa()?; let bytes = std::mem::size_of::().max(64); // Header. diff --git a/crates/punktfunk-host/src/inject/windows/gamepad_raii.rs b/crates/punktfunk-host/src/inject/windows/gamepad_raii.rs index b460d8e..8f05ffb 100644 --- a/crates/punktfunk-host/src/inject/windows/gamepad_raii.rs +++ b/crates/punktfunk-host/src/inject/windows/gamepad_raii.rs @@ -21,10 +21,18 @@ use windows::Win32::System::Memory::{ MEMORY_MAPPED_VIEW_ADDRESS, PAGE_READWRITE, }; -/// A named, anonymous (pagefile-backed) shared section + its mapped read/write view, created with the -/// permissive `D:(A;;GA;;;WD)` SDDL the restricted-token driver needs to open it. RAII: drop unmaps the -/// view, then the [`OwnedHandle`] closes the section handle (in that order). Replaces the three backends' -/// hand-duplicated `CreateFileMappingW` + `MapViewOfFile` + manual `Drop`. +/// A named, anonymous (pagefile-backed) shared section + its mapped read/write view. RAII: drop unmaps +/// the view, then the [`OwnedHandle`] closes the section handle (in that order). Replaces the three +/// backends' hand-duplicated `CreateFileMappingW` + `MapViewOfFile` + manual `Drop`. +/// +/// SDDL `D:(A;;GA;;;SY)(A;;GA;;;LS)`: GENERIC_ALL to **SYSTEM** (the host creates the section and +/// writes the live HID input report into it) and **LocalService** (the account the UMDF driver's +/// WUDFHost runs under, which reads it). The old SDDL granted **Everyone** (`WD`) — on the (mistaken) +/// assumption the driver needed a restricted token's broad access — letting any local user +/// `OpenFileMapping` the section to inject controller input or tamper the trusted channel +/// (security-review 2026-06-28 #5). Verified on the RTX box (2026-06-29): the WUDFHost token is +/// `S-1-5-19` (LocalService), SYSTEM integrity, with **zero restricted SIDs** — so scoping to SY+LS is +/// sufficient for the driver and excludes normal (medium-IL, non-service) user processes. pub(super) struct Shm { /// Owns the section handle (closed on drop). Held only for ownership — never read after construction. _handle: OwnedHandle, @@ -40,7 +48,7 @@ impl Shm { // exit — acceptable for a host-lifetime object). unsafe { ConvertStringSecurityDescriptorToSecurityDescriptorW( - w!("D:(A;;GA;;;WD)"), + w!("D:(A;;GA;;;SY)(A;;GA;;;LS)"), SDDL_REVISION_1, &mut psd, None,