From 2f93b8bf99c1a9f0a64f1de71d9bfe270f0a852a Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 02:25:56 +0100 Subject: [PATCH 01/41] ee/wcp: restore and update the browser-privilege design doc BROWSER_PRIVILEGE.md was deleted an hour after being written, in an unrelated "cleanup" commit on ee/wcp/rs-cef-fresh. Restore it and record what's settled for this branch: the dedicated-account option goes first, integrated auth is out of scope so S4U needs no stored credential, and the CEF sandbox option turns out to need a C++ shim cef-rs doesn't provide on Windows -- left out of scope here. --- ee/wcp/BROWSER_PRIVILEGE.md | 163 ++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 ee/wcp/BROWSER_PRIVILEGE.md diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md new file mode 100644 index 00000000..18180a6c --- /dev/null +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -0,0 +1,163 @@ +# Lowering the privilege of the sign-in browser + +The problem in one line: `ak_cef.exe` renders untrusted remote web content on +the Windows logon screen, and on the path that matters most it runs as +`NT AUTHORITY\SYSTEM`. + +## Status + +Branch `ee/wcp/browser-privilege` is building Option B, the dedicated +service account. Both open decisions below are settled: integrated auth +(Kerberos/SPNEGO) is not in scope, so S4U needs no stored credential; the +`WinSta0\Winlogon` ACL grant is accepted as the cost of getting off SYSTEM. +Option A turned out to need more than this doc assumed — see the note at the +end of that section — so it stayed out of scope rather than being folded in +here. + +This file previously lived on `ee/wcp/rs-cef-fresh` and was deleted there in +a "cleanup" commit an hour after being written, along with three sibling +design docs; nothing indicates that was deliberate. Restored from +`git show 93549e00:ee/wcp/BROWSER_PRIVILEGE.md`. + +## Where things stand + +Neither the current implementation nor the C++ one it replaced has ever run +this browser sandboxed or unprivileged on a fresh logon. + +**Sandbox — off, always, in both.** `cef-host/src/main.rs` sets +`no_sandbox: 1` and passes a null `sandbox_info`. The C++ looked like it might +enable it — `ak_cred_provider/Provider.cpp` had both a `CefScopedSandboxInfo` +block and `settings.no_sandbox = true`, each guarded on `CEF_USE_SANDBOX` — but +that define only reaches a target through CEF's +`SET_EXECUTABLE_TARGET_PROPERTIES`/`SET_LIBRARY_TARGET_PROPERTIES` macros, the +one call site in `cefsimple/CMakeLists.txt` was commented out, and +`ak_cred_provider` never invoked either. So it compiled with the sandbox +disabled while `if(USE_SANDBOX)` still linked `cef_sandbox_lib` and applied +`SET_LPAC_ACLS` — a build that looks sandboxed and is not. + +**Identity — SYSTEM on the path that matters.** `credprovider::syscalls:: +acquire_interactive_token` tries `WTSQueryUserToken` first and falls back to +duplicating `winlogon.exe`'s token: + +| Scenario | Token | Runs as | +| --- | --- | --- | +| `CPUS_LOGON` (fresh logon) | winlogon duplicate — nobody is signed in yet, so there is no user token to get | **SYSTEM** | +| `CPUS_UNLOCK_WORKSTATION` | `WTSQueryUserToken` | the locked-out user — but the spawn then fails, that token has no access to `WinSta0\Winlogon` | +| `CPUS_CREDUI` (debug) | none; the caller holds no `SE_TCB_NAME`, so it falls through to `CreateProcessW` | the interactive user | + +The C++ ran CEF in-process inside `ak_cred_provider.dll`, i.e. inside LogonUI, +so the browser process *was* LogonUI: SYSTEM, with helper processes inheriting +that token. It never chose an identity because it never spawned anything. + +So the existing token machinery buys nothing on fresh logon (it works hard to +arrive at the same SYSTEM the C++ got for free) and is broken on unlock. It is +not load-bearing for the sign-in flow; it is an unfinished attempt at this +document's goal. + +## Option A — enable the CEF sandbox + +Do this first. It targets the actual risk — hostile input being parsed — rather +than the privilege of the process that hosts the parser, and it needs no new +account, no installer change and no ACL grant on the logon desktop. + +- Link `cef_sandbox.lib` and pass real `sandbox_info` to `initialize` instead of + a null pointer; drop `no_sandbox`. +- Verify renderer/GPU/utility processes actually come up sandboxed on the + secure desktop. This is the part to be sceptical about: Chromium's sandbox + creates its own alternate desktop for sandboxed children, and how that + interacts with `WinSta0\Winlogon` is unverified here. +- `SET_LPAC_ACLS` in the old C++ build hints the CEF sample expects LPAC ACLs on + the binary directory; check whether the MSI needs to apply the equivalent to + `bin/wcp/`. + +Leaves the browser process itself as SYSTEM. That is the C++'s posture, so it +is not a regression — but it is not the end state either. + +**Turns out to be more than "link a lib and pass a pointer".** The `cef` +crate actually in use here (151.4.0, `ee/wcp/cef-host/Cargo.toml`) has no +Windows sandbox support at the Rust level at all: `cef::sandbox::Sandbox` is +`#[cfg(target_os = "macos")]`-only, and `cef-dll-sys`'s vendored `wrapper.h` +only includes `cef_sandbox_mac.h`. `execute_process`/`initialize` accept a +`windows_sandbox_info: *mut c_void` on Windows, but nothing constructs one — +that requires CEF's C++-only `CefScopedSandboxInfo` +(`include/cef_sandbox_win.h`), which the crate's C-API bindings don't +expose. `cef-dll-sys`'s `build.rs` passes `USE_SANDBOX=ON` to CMake but only +builds the `libcef_dll_wrapper` target, never `cef_sandbox`, so nothing +actually links the sandbox lib today despite the `sandbox` feature being on +by default. Doing this properly means a small C++ shim exposing a C ABI +around `CefScopedSandboxInfo`, plus a patch to `cef-dll-sys`'s `build.rs` to +build and link `cef_sandbox.lib` — upstream-shaped work, independent of this +branch. Left for whoever picks Option A up. + +## Option B — a dedicated local account from the installer + +Defence in depth on top of A: create a local account at install time and launch +`ak_cef.exe` as it. + +The reason to prefer this over the current code is that the account exists +before anyone signs in, so logon and unlock become one path and the whole +`WTSQueryUserToken` / winlogon-scan fallback chain in `syscalls.rs` can be +deleted rather than fixed. + +`WixToolset.Util.wixext` 6.0.0 is already referenced by +`vpkg/windows/authentik Agent Installer.wixproj` and `Package.wxs` already uses +the `util:` namespace, so `` needs no new dependency. + +### Getting a token without storing a password — decided + +`` wants a password and `LogonUser` would want it back later, which +means a secret at rest and a rotation story. + +`LsaLogonUser` with `MSV1_0_S4U_LOGON` avoids that entirely: it mints a token +for a local account with no credentials, given `SE_TCB_NAME`, which LogonUI +already holds. Nothing to store, nothing to rotate. The installer still gives +`` a `GeneratePassword="yes"` password, because the account needs +one to exist — it is simply never read back by anything. + +**The catch that made this a decision:** an S4U token carries no network +credentials. That is fine for reaching authentik over HTTPS with the +`X-Authentik-Platform-Auth-DTH` bearer header. It would not be fine if the +sign-in flow ever chained to an IdP doing Kerberos/SPNEGO — authentik +supports that, and in an AD environment it would be plausible. **Confirmed +out of scope for this account**, so S4U stands. + +### The rest of the work + +- **Secure desktop ACL.** A non-SYSTEM token has no access to + `WinSta0\Winlogon`; add the account's SID to the window station and desktop + DACLs (`GetUserObjectSecurity`/`SetUserObjectSecurity`). Weigh it honestly: + this grants a service account the right to create windows on the desktop + where credentials are typed. Still a large net win — a compromised renderer + no longer yields SYSTEM — but a deliberate expansion of what can reach the + logon desktop, not a free improvement. +- **Profile and cache.** `root_cache_path` is explicit already, but Chromium + also wants temp, fonts and crashpad paths. Without `LoadUserProfile` the + account gets the default profile. The MSI must also grant it write access to + `wcp-cache`, which currently just inherits ProgramData defaults. +- **Harden the account.** Deny interactive logon — it must not be usable to + sign in at the very screen it serves — plus deny network and RDP logon, + minimal group membership, no privileges. `RemoveOnUninstall`, and handle the + account already existing on upgrade. +- **Deployment friction.** GPO blocking local account creation, endpoint + monitoring flagging a new local account, no local accounts on a DC. + +## Decisions — resolved for this branch + +1. ~~Is integrated auth (Kerberos/SPNEGO to an upstream IdP) ever in scope?~~ + **No.** S4U stands; no stored credential for this account. +2. ~~Is granting a non-SYSTEM account access to `WinSta0\Winlogon` + acceptable?~~ **Yes**, scoped to this one account only. +3. Does the browser process need to be non-SYSTEM at all once renderers are + sandboxed, or is option A sufficient? Not settled — moot for this branch + since option A is out of scope here; revisit if/when option A is built. + +## Do not conflate this with the `add_child_view` crash + +At the time of writing `ak_cef.exe` intermittently dies with `0x80000003` +(`STATUS_BREAKPOINT`, a Chromium `CHECK`) inside `CefWindow::add_child_view` on +the secure desktop, while the identical call succeeds under `CPUS_CREDUI` in +CI. That was being chased separately; see `RUST_CEF_PLAN.md` in git history +(`git show be23c35c:ee/wcp/RUST_CEF_PLAN.md`) for the state of it at the time. +Changing the sandbox or the launch identity to chase that crash would be +changing the security posture for a debugging reason — if it needs doing +temporarily, do it on a throwaway branch, not here. From 7b56f4b24263ca7e05d32ff6d2470183e1ae0e9e Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 02:47:45 +0100 Subject: [PATCH 02/41] ee/wcp: mint the sign-in browser's token from a dedicated account ak_cef.exe launched as SYSTEM on fresh logon because acquire_interactive_token fell back to duplicating winlogon.exe's own token when no user token existed yet. Replace it with an S4U logon (LsaLogonUser + MSV1_0_S4U_LOGON) for a dedicated local account, so logon and unlock both mint a token for the same non-SYSTEM identity and the winlogon-duplication fallback goes away entirely. Also grants that account access to WinSta0\Winlogon (denied to any non-SYSTEM token by default) and denies it interactive/network/RDP logon rights, and rotates its installer-set password to a random, discarded value the first time it runs -- nothing ever reads it back, since S4U needs no credential. The account itself is created by the installer; that and the design tradeoffs are in BROWSER_PRIVILEGE.md. --- Cargo.toml | 2 + ee/wcp/credprovider/src/ipc.rs | 54 ++- ee/wcp/credprovider/src/syscalls.rs | 602 ++++++++++++++++++++++------ 3 files changed, 516 insertions(+), 142 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8a3c4898..033007d8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -83,10 +83,12 @@ windows = { version = "0.61", features = [ "Win32_System_IO", "Win32_Storage_FileSystem", "Win32_Security", + "Win32_Security_Authorization", "Win32_UI_Shell", "Win32_Security_Credentials", "Win32_System_Diagnostics_ToolHelp", "Win32_System_RemoteDesktop", + "Win32_System_StationsAndDesktops", "Win32_System_Threading", "Win32_UI_WindowsAndMessaging", "Win32_Graphics_Gdi", diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 0f8d6750..dccdbcd8 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -29,7 +29,7 @@ use windows::{ core::{PCWSTR, PWSTR}, }; -use crate::syscalls::acquire_interactive_token; +use crate::syscalls; use ak_ee_wcp_wire::AuthResult; /// Spawns `ak_cef.exe` and waits for its result. `should_continue` is polled @@ -270,6 +270,36 @@ fn signal_cancel(cancel_write: HANDLE) { std::mem::forget(f); } +/// Gets `ak_cef.exe` a token for the dedicated service account rather than +/// SYSTEM (`BROWSER_PRIVILEGE.md`). The account exists from install time +/// (`vpkg/windows/Package.wxs`), so unlike the old `WTSQueryUserToken` / +/// winlogon-duplication fallback this needs no session-dependent branching: +/// logon and unlock both end up here. +/// +/// Password rotation and the account-hardening calls are best-effort and +/// only logged on failure — each is idempotent, so a transient failure here +/// just means the next call tries again, and none of them being fatal keeps +/// a permissions hiccup on one from blocking `service_account_token` from +/// still being attempted. +fn acquire_service_account_token() -> windows::core::Result { + if let Err(e) = + syscalls::ensure_service_account_password_rotated(syscalls::SERVICE_ACCOUNT_NAME) + { + log::warn!("could not rotate the service account's password: {e}"); + } + + let sid = syscalls::account_sid(syscalls::SERVICE_ACCOUNT_NAME)?; + + if let Err(e) = syscalls::deny_interactive_and_network_logon(&sid) { + log::warn!("could not deny the service account interactive/network logon: {e}"); + } + if let Err(e) = syscalls::ensure_desktop_access(&sid) { + log::warn!("could not grant the service account secure-desktop access: {e}"); + } + + syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME) +} + fn spawn_cef_host( cef_exe: &Path, pipes: &DuplexPipes, @@ -323,16 +353,16 @@ fn spawn_cef_host( } let mut pi = PROCESS_INFORMATION::default(); - let token = match acquire_interactive_token() { - Ok(token) => Some(token), - Err(e) if may_launch_in_current_session(cpus) => { - log::debug!("no interactive-session token ({e}); launching in the current session"); - None - } - Err(e) => { - log::error!("could not acquire an interactive-session token: {e}"); - unsafe { DeleteProcThreadAttributeList(attr_list) }; - return Err(e); + let token = if may_launch_in_current_session(cpus) { + None + } else { + match acquire_service_account_token() { + Ok(token) => Some(token), + Err(e) => { + log::error!("could not acquire the service account's token: {e}"); + unsafe { DeleteProcThreadAttributeList(attr_list) }; + return Err(e); + } } }; log::info!( @@ -343,7 +373,7 @@ fn spawn_cef_host( .map(|_| SECURE_DESKTOP) .unwrap_or(""), if token.is_some() { - "an interactive-session" + "the service account's" } else { "the caller's own" } diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index e7490055..7e37246b 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -2,58 +2,105 @@ //! effects, so the logic that decides *when* to call them can be unit //! tested against a fake instead of the OS. +use ak_platform_keyring::{KeyringError, windows::WindowsStore}; use windows::Win32::Foundation::{CloseHandle, HANDLE}; use windows::{ Win32::{ - Foundation::E_FAIL, - NetworkManagement::NetManagement::{NetUserSetInfo, USER_INFO_1003}, + Foundation::{ + ERROR_LOGON_FAILURE, ERROR_PASSWORD_EXPIRED, ERROR_PASSWORD_MUST_CHANGE, GENERIC_ALL, + HLOCAL, LUID, LocalFree, + }, + NetworkManagement::NetManagement::{NetUserChangePassword, NetUserSetInfo, USER_INFO_1003}, Security::{ + ACL, AllocateLocallyUniqueId, Authentication::Identity::{ - LSA_STRING, LsaConnectUntrusted, LsaDeregisterLogonProcess, - LsaLookupAuthenticationPackage, + LSA_HANDLE, LSA_OBJECT_ATTRIBUTES, LSA_STRING, LSA_UNICODE_STRING, + LsaAddAccountRights, LsaClose, LsaConnectUntrusted, LsaDeregisterLogonProcess, + LsaFreeReturnBuffer, LsaLogonUser, LsaLookupAuthenticationPackage, LsaOpenPolicy, + LsaRegisterLogonProcess, MSV1_0_S4U_LOGON, MsV1_0S4ULogon, POLICY_CREATE_ACCOUNT, + SECURITY_LOGON_TYPE, }, - DuplicateTokenEx, SecurityImpersonation, TOKEN_ACCESS_MASK, TOKEN_ALL_ACCESS, - TOKEN_ASSIGN_PRIMARY, TOKEN_DUPLICATE, TOKEN_QUERY, TokenPrimary, - }, - System::Diagnostics::ToolHelp::{ - CreateToolhelp32Snapshot, PROCESSENTRY32W, Process32FirstW, Process32NextW, - TH32CS_SNAPPROCESS, - }, - System::RemoteDesktop::{ - ProcessIdToSessionId, WTSGetActiveConsoleSessionId, WTSQueryUserToken, + Authorization::{ + EXPLICIT_ACCESS_W, GetSecurityInfo, NO_MULTIPLE_TRUSTEE, SE_OBJECT_TYPE, + SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, TRUSTEE_IS_SID, + TRUSTEE_IS_USER, TRUSTEE_W, + }, + DACL_SECURITY_INFORMATION, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, LogonUserW, + LookupAccountNameW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, QUOTA_LIMITS, + SID_NAME_USE, TOKEN_SOURCE, }, - System::Threading::{ - GetCurrentProcessId, OpenProcess, OpenProcessToken, PROCESS_QUERY_INFORMATION, + Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, + System::StationsAndDesktops::{ + DESKTOP_CONTROL_FLAGS, GetProcessWindowStation, OpenDesktopW, }, }, - core::{PCWSTR, PSTR}, + core::{HRESULT, PCWSTR, PSTR, PWSTR, w}, }; +const SERVICE_ACCOUNT_STATE_KEY: &str = + "SOFTWARE\\authentik Security Inc.\\Platform\\WcpServiceAccount"; + pub trait AuthPackageLookup { fn negotiate_package(&self) -> windows::core::Result; } -pub trait LocalAccountPasswordReset { +pub trait LocalAccountPassword { + /// Administrative reset. Orphans the account's DPAPI master key — stored + /// passwords, EFS files and personal certificates become unreadable — so + /// this is only for first use and for recovering an account whose password + /// we no longer know. fn reset(&self, username: &str, password: &str) -> windows::core::Result<()>; + + /// Self-service change. Supplying the old password lets LSA re-encrypt the + /// DPAPI master key instead of orphaning it, which is why rotation goes + /// through here and not through `reset`. + fn change(&self, username: &str, old: &str, new: &str) -> windows::core::Result<()>; + + /// Whether `password` is still this account's password. Only codes that + /// say something definite about the credential produce an `Ok`; anything + /// else is `Err`, because "could not tell" must not be read as "wrong + /// password". + fn validate(&self, username: &str, password: &str) -> windows::core::Result; +} + +#[derive(Debug, PartialEq, Eq)] +pub enum PasswordCheck { + Valid, + /// Correct, but the account will not accept it for logon until it changes. + Expired, + Rejected, +} + +/// Where the local account's password is kept between sign-ins. +pub trait PasswordStore { + fn load(&self, sid: &str) -> eyre::Result>; + fn save(&self, sid: &str, password: &str) -> eyre::Result<()>; } pub struct RealSyscalls; +fn wide(s: &str) -> Vec { + s.encode_utf16().chain(std::iter::once(0)).collect() +} + +/// `LSA_STRING` is counted, but `MaximumLength` is expected to cover a +/// trailing NUL — that is what `LsaInitString` produces for a C literal. +/// Claiming `len + 1` over a buffer that has no terminator overruns it, so +/// `name` must end in `\0`. +fn lsa_string(name: &'static [u8]) -> LSA_STRING { + LSA_STRING { + Length: (name.len() - 1) as u16, + MaximumLength: name.len() as u16, + Buffer: PSTR(name.as_ptr() as *mut u8), + } +} + impl AuthPackageLookup for RealSyscalls { fn negotiate_package(&self) -> windows::core::Result { let mut lsa_handle = HANDLE::default(); unsafe { LsaConnectUntrusted(&mut lsa_handle) }.ok()?; - // `LSA_STRING` is counted, but `MaximumLength` is expected to cover a - // trailing NUL — that is what `LsaInitString` produces for a C literal. - // Claiming `len + 1` over a buffer that has no terminator overruns it. - let name = b"Negotiate\0"; - let lsa_name = LSA_STRING { - Length: (name.len() - 1) as u16, - MaximumLength: name.len() as u16, - Buffer: PSTR(name.as_ptr() as *mut u8), - }; - + let lsa_name = lsa_string(b"Negotiate\0"); let mut auth_package = 0u32; let status = unsafe { LsaLookupAuthenticationPackage(lsa_handle, &lsa_name, &mut auth_package) }; @@ -73,10 +120,10 @@ impl AuthPackageLookup for RealSyscalls { } } -impl LocalAccountPasswordReset for RealSyscalls { +impl LocalAccountPassword for RealSyscalls { fn reset(&self, username: &str, password: &str) -> windows::core::Result<()> { - let username_wide: Vec = username.encode_utf16().chain(std::iter::once(0)).collect(); - let password_wide: Vec = password.encode_utf16().chain(std::iter::once(0)).collect(); + let username_wide = wide(username); + let password_wide = wide(password); let info = USER_INFO_1003 { usri1003_password: windows::core::PWSTR(password_wide.as_ptr() as *mut u16), @@ -93,135 +140,430 @@ impl LocalAccountPasswordReset for RealSyscalls { }; if status != 0 { - return Err(windows::core::Error::from(E_FAIL)); + return Err(net_api_error(status)); } Ok(()) } -} -/// Acquire a primary token for the active console (interactive) session, so -/// `ak_cef.exe` can be launched there rather than in LogonUI's Session 0. -/// -/// `WTSQueryUserToken` works once a user token exists (unlock scenario); on -/// a fresh logon no such token exists yet, so this falls back to duplicating -/// `winlogon.exe`'s own token in that session. -pub fn acquire_interactive_token() -> windows::core::Result { - unsafe { - let session = WTSGetActiveConsoleSessionId(); - if session == 0xFFFF_FFFF { - log::error!("no active console session"); - return Err(windows::core::Error::from(E_FAIL)); + fn change(&self, username: &str, old: &str, new: &str) -> windows::core::Result<()> { + let username_wide = wide(username); + let old_wide = wide(old); + let new_wide = wide(new); + + let status = unsafe { + NetUserChangePassword( + PCWSTR::null(), + PCWSTR(username_wide.as_ptr()), + PCWSTR(old_wide.as_ptr()), + PCWSTR(new_wide.as_ptr()), + ) + }; + + if status != 0 { + return Err(net_api_error(status)); } + Ok(()) + } + fn validate(&self, username: &str, password: &str) -> windows::core::Result { + let username_wide = wide(username); + let password_wide = wide(password); + + // A network logon validates the credential without building a session, + // and `.` scopes the lookup to this machine's account database. let mut token = HANDLE::default(); - if WTSQueryUserToken(session, &mut token).is_ok() { - return Ok(token); + let result = unsafe { + LogonUserW( + PCWSTR(username_wide.as_ptr()), + w!("."), + PCWSTR(password_wide.as_ptr()), + LOGON32_LOGON_NETWORK, + LOGON32_PROVIDER_DEFAULT, + &mut token, + ) + }; + + match result { + Ok(()) => { + unsafe { + let _ = CloseHandle(token); + } + Ok(PasswordCheck::Valid) + } + // Anything not listed here — a policy denying network logons, a + // locked-out or disabled account — means the check was + // inconclusive, not that the password is wrong, and the caller + // must not reset on the strength of it. + Err(e) if e.code() == ERROR_LOGON_FAILURE.to_hresult() => Ok(PasswordCheck::Rejected), + Err(e) + if e.code() == ERROR_PASSWORD_EXPIRED.to_hresult() + || e.code() == ERROR_PASSWORD_MUST_CHANGE.to_hresult() => + { + Ok(PasswordCheck::Expired) + } + Err(e) => Err(e), } + } +} + +/// `NET_API_STATUS` codes are Win32 error codes, so they survive the trip +/// through `HRESULT` and stay readable in the log. +fn net_api_error(status: u32) -> windows::core::Error { + windows::core::Error::from_hresult(HRESULT::from_win32(status)) +} - // This provider is loaded into the process drawing the logon UI, so its - // own session is the one the person is signing in to. That should be - // the console session; log it when it is not, because then the console - // session is the wrong thing to be looking for winlogon in. - let mut own_session = 0u32; - if ProcessIdToSessionId(GetCurrentProcessId(), &mut own_session).is_ok() - && own_session != session - { - log::warn!( - "console session is {session} but this provider is in session {own_session}" - ); +/// Keyring-backed [`PasswordStore`], keyed by SID so renaming the account does +/// not orphan the entry. +/// +/// This reaches for [`WindowsStore`] rather than `ak_platform_keyring::store()` +/// because the latter resolves to the in-memory store under `debug_assertions`, +/// which would silently lose the password between LogonUI processes in every +/// development build. Local-machine persistence keeps a secret that is +/// meaningless off this box from roaming to a domain. +pub struct KeyringPasswordStore { + store: WindowsStore, + service: String, +} + +impl Default for KeyringPasswordStore { + fn default() -> Self { + KeyringPasswordStore::new() + } +} + +impl KeyringPasswordStore { + pub fn new() -> Self { + KeyringPasswordStore { + store: WindowsStore::new_local_machine(), + service: ak_platform_keyring::service("wcp-account-password"), + } + } +} + +impl PasswordStore for KeyringPasswordStore { + fn load(&self, sid: &str) -> eyre::Result> { + match self.store.get_blocking(&self.service, sid) { + Ok(password) => Ok(Some(password)), + Err(KeyringError::NotFound()) => Ok(None), + Err(e) => Err(eyre::eyre!("{e}")), } + } - winlogon_token_for_session(session) + fn save(&self, sid: &str, password: &str) -> eyre::Result<()> { + self.store + .set_blocking(&self.service, sid, password) + .map_err(|e| eyre::eyre!("{e}")) } } -/// There is one `winlogon.exe` per session, so the snapshot has to be searched -/// to the end: stopping at the first one found gives up as soon as the -/// enumeration happens to reach another session's copy first, which is what a -/// logoff/logon cycle produces once the console session id has moved on. -fn winlogon_token_for_session(session_id: u32) -> windows::core::Result { +/// Name of the dedicated local account `ak_cef.exe` runs as instead of +/// SYSTEM. Created by the installer (`vpkg/windows/Package.wxs`'s +/// `util:User`) — keep this in step with that element's `Name` attribute. +pub const SERVICE_ACCOUNT_NAME: &str = "ak-wcp-browser"; + +const TOKEN_SOURCE_NAME: [i8; 8] = [ + b'A' as i8, b'k' as i8, b'W' as i8, b'c' as i8, b'p' as i8, b'S' as i8, b'4' as i8, b'U' as i8, +]; + +/// Resolves the service account's name to a SID: both `LsaAddAccountRights` +/// and the desktop ACL grant below want one, and a name is all the installer +/// leaves behind. +pub fn account_sid(username: &str) -> windows::core::Result> { + let username_wide = wide(username); + // Large enough for any SID Windows issues (the practical maximum is well + // under 68 bytes) and any domain name `LookupAccountNameW` might report. + let mut sid = vec![0u8; 256]; + let mut sid_len = sid.len() as u32; + let mut domain = [0u16; 256]; + let mut domain_len = domain.len() as u32; + let mut use_ = SID_NAME_USE::default(); unsafe { - let snap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0)?; + LookupAccountNameW( + PCWSTR::null(), + PCWSTR(username_wide.as_ptr()), + Some(PSID(sid.as_mut_ptr() as *mut _)), + &mut sid_len, + Some(PWSTR(domain.as_mut_ptr())), + &mut domain_len, + &mut use_, + )?; + } + sid.truncate(sid_len as usize); + Ok(sid) +} - let mut entry = PROCESSENTRY32W { - dwSize: std::mem::size_of::() as u32, +fn lsa_unicode_string(wide: &[u16]) -> LSA_UNICODE_STRING { + let bytes = (wide.len() * 2) as u16; + LSA_UNICODE_STRING { + Length: bytes, + MaximumLength: bytes, + Buffer: PWSTR(wide.as_ptr() as *mut u16), + } +} + +/// Mints a primary token for the service account via an S4U logon — no +/// password needed, only `SE_TCB_NAME`, which LogonUI already holds. Nothing +/// to store, nothing to rotate: unlike the interactive user's account +/// (`credential.rs`, `LOCAL_PASSWORD.md`), this account's credential never +/// has to survive being handed to LSA a second time. +/// +/// `Service` is the logon type, deliberately: it is the one type this +/// account is not denied by `deny_interactive_and_network_logon`, so +/// hardening the account does not also lock this call out. +pub fn service_account_token(username: &str) -> windows::core::Result { + unsafe { + let process_name = lsa_string(b"ak_cred_provider\0"); + let mut lsa_handle = HANDLE::default(); + let mut security_mode = 0u32; + LsaRegisterLogonProcess(&process_name, &mut lsa_handle, &mut security_mode).ok()?; + + let result = s4u_logon(lsa_handle, username); + let _ = LsaDeregisterLogonProcess(lsa_handle); + result + } +} + +unsafe fn s4u_logon(lsa_handle: HANDLE, username: &str) -> windows::core::Result { + unsafe { + let package_name = lsa_string(b"MICROSOFT_AUTHENTICATION_PACKAGE_V1_0\0"); + let mut auth_package = 0u32; + LsaLookupAuthenticationPackage(lsa_handle, &package_name, &mut auth_package).ok()?; + + // S4U wants these as `LSA_UNICODE_STRING`s with no trailing NUL, + // unlike the `LSA_STRING`s above — keep the backing buffers alive + // for the whole call, `lsa_unicode_string` only borrows them. + let username_wide: Vec = username.encode_utf16().collect(); + let domain_wide: Vec = ".".encode_utf16().collect(); + let s4u = MSV1_0_S4U_LOGON { + MessageType: MsV1_0S4ULogon, + Flags: 0, + UserPrincipalName: lsa_unicode_string(&username_wide), + DomainName: lsa_unicode_string(&domain_wide), + }; + + let origin_name = lsa_string(b"ak_cred_provider\0"); + let mut token_source = TOKEN_SOURCE { + SourceName: TOKEN_SOURCE_NAME, ..Default::default() }; + AllocateLocallyUniqueId(&mut token_source.SourceIdentifier)?; - let mut result = Err(windows::core::Error::from(E_FAIL)); - let mut sessions_seen = Vec::new(); - - if Process32FirstW(snap, &mut entry).is_ok() { - loop { - let nul = entry - .szExeFile - .iter() - .position(|&c| c == 0) - .unwrap_or(entry.szExeFile.len()); - let name = String::from_utf16_lossy(&entry.szExeFile[..nul]); - - if name.eq_ignore_ascii_case("winlogon.exe") { - let mut proc_session = 0u32; - if ProcessIdToSessionId(entry.th32ProcessID, &mut proc_session).is_ok() { - sessions_seen.push(proc_session); - if proc_session == session_id { - match duplicate_process_primary_token(entry.th32ProcessID) { - Ok(dup) => { - result = Ok(dup); - break; - } - // Keep looking: another instance in the same - // session may still hand one over. - Err(e) => log::warn!( - "could not duplicate the token of winlogon.exe \ - (pid {}, session {proc_session}): {e}", - entry.th32ProcessID - ), - } - } - } - } + let mut profile_buffer: *mut std::ffi::c_void = std::ptr::null_mut(); + let mut profile_buffer_len = 0u32; + let mut logon_id = LUID::default(); + let mut token = HANDLE::default(); + let mut quotas = QUOTA_LIMITS::default(); + let mut sub_status = 0i32; - if Process32NextW(snap, &mut entry).is_err() { - break; - } - } + let status = LsaLogonUser( + lsa_handle, + &origin_name, + SECURITY_LOGON_TYPE::Service, + auth_package, + &s4u as *const MSV1_0_S4U_LOGON as *const std::ffi::c_void, + std::mem::size_of::() as u32, + None, + &token_source, + &mut profile_buffer, + &mut profile_buffer_len, + &mut logon_id, + &mut token, + &mut quotas, + &mut sub_status, + ); + + if !profile_buffer.is_null() { + let _ = LsaFreeReturnBuffer(profile_buffer); } - let _ = CloseHandle(snap); - if result.is_err() { - log::warn!( - "no usable winlogon.exe token for console session {session_id}; \ - saw winlogon in sessions {sessions_seen:?}" - ); + // As with `negotiate_package`, `NTSTATUS::ok()` is only a sign test; + // `sub_status` carries the more specific reason (e.g. an account + // that does not exist yet because the installer has not run) and is + // only meaningful once `status` itself is an error. + if status.0 != 0 { + log::error!("S4U logon for {username} failed: {status:?} (substatus {sub_status:#x})"); + return Err(status.to_hresult().into()); } - result + Ok(token) } } -fn duplicate_process_primary_token(pid: u32) -> windows::core::Result { +/// Adds `sid` to `handle`'s DACL with `GENERIC_ALL`, preserving every +/// existing entry — `SetEntriesInAclW` merges onto `old_dacl` rather than +/// replacing it, which matters here: replacing outright would drop +/// SYSTEM/Administrators access to the object this process itself needs. +unsafe fn grant_generic_all( + handle: HANDLE, + object_type: SE_OBJECT_TYPE, + sid: &[u8], +) -> windows::core::Result<()> { unsafe { - let hproc = OpenProcess(PROCESS_QUERY_INFORMATION, false, pid)?; - let access = TOKEN_ACCESS_MASK(TOKEN_DUPLICATE.0 | TOKEN_QUERY.0 | TOKEN_ASSIGN_PRIMARY.0); - let mut raw = HANDLE::default(); - let opened = OpenProcessToken(hproc, access, &mut raw); - if opened.is_err() { - let _ = CloseHandle(hproc); - return Err(windows::core::Error::from(E_FAIL)); + let mut old_dacl: *mut ACL = std::ptr::null_mut(); + let mut sd = PSECURITY_DESCRIPTOR::default(); + GetSecurityInfo( + handle, + object_type, + DACL_SECURITY_INFORMATION, + None, + None, + Some(&mut old_dacl), + None, + Some(&mut sd), + ) + .ok()?; + + let trustee = TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: NO_MULTIPLE_TRUSTEE, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_USER, + ptstrName: PWSTR(sid.as_ptr() as *mut u16), + }; + let entry = EXPLICIT_ACCESS_W { + grfAccessPermissions: GENERIC_ALL.0, + grfAccessMode: SET_ACCESS, + grfInheritance: NO_INHERITANCE, + Trustee: trustee, + }; + + let mut new_dacl: *mut ACL = std::ptr::null_mut(); + let entries_result = SetEntriesInAclW(Some(&[entry]), Some(old_dacl), &mut new_dacl); + let set_result = if entries_result.is_ok() { + SetSecurityInfo( + handle, + object_type, + DACL_SECURITY_INFORMATION, + None, + None, + Some(new_dacl), + None, + ) + } else { + entries_result + }; + + let _ = LocalFree(Some(HLOCAL(sd.0))); + if !new_dacl.is_null() { + let _ = LocalFree(Some(HLOCAL(new_dacl as *mut std::ffi::c_void))); } - let mut dup = HANDLE::default(); - let result = DuplicateTokenEx( - raw, - TOKEN_ALL_ACCESS, + set_result.ok() + } +} + +/// Grants the service account's SID access to the secure desktop — a +/// non-SYSTEM token has none by default. Deliberate, per +/// `BROWSER_PRIVILEGE.md`: a large net win (a compromised renderer no longer +/// yields SYSTEM) but a real expansion of what can reach the desktop +/// credentials are typed on, scoped to only this one account. +/// +/// Idempotent, and only correct when called from inside LogonUI's own +/// process: `GetProcessWindowStation`/`OpenDesktopW` resolve relative to the +/// *caller's* window station, which is `WinSta0` for the real logon +/// scenarios `ipc.rs` targets. +pub fn ensure_desktop_access(sid: &[u8]) -> windows::core::Result<()> { + unsafe { + let winsta = GetProcessWindowStation()?; + grant_generic_all(HANDLE(winsta.0), SE_WINDOW_OBJECT, sid)?; + + let desktop = OpenDesktopW( + w!("Winlogon"), + DESKTOP_CONTROL_FLAGS(0), + false, + (READ_CONTROL | WRITE_DAC).0, + )?; + grant_generic_all(HANDLE(desktop.0), SE_WINDOW_OBJECT, sid) + } +} + +/// Denies the service account the logon types that would let it sign someone +/// in — it must not be usable at the very screen it serves. `Service`, what +/// `service_account_token` uses, is deliberately not among these. +/// +/// `LsaAddAccountRights` is itself idempotent: granting an already-held +/// right is a no-op, so this is safe to call on every load. +pub fn deny_interactive_and_network_logon(sid: &[u8]) -> windows::core::Result<()> { + const RIGHTS: [&str; 3] = [ + "SeDenyInteractiveLogonRight", + "SeDenyNetworkLogonRight", + "SeDenyRemoteInteractiveLogonRight", + ]; + let wide_rights: Vec> = RIGHTS.iter().map(|r| r.encode_utf16().collect()).collect(); + let lsa_rights: Vec = + wide_rights.iter().map(|w| lsa_unicode_string(w)).collect(); + + unsafe { + let mut policy_handle = LSA_HANDLE::default(); + let object_attrs = LSA_OBJECT_ATTRIBUTES::default(); + LsaOpenPolicy( None, - SecurityImpersonation, - TokenPrimary, - &mut dup, - ); - let _ = CloseHandle(raw); - let _ = CloseHandle(hproc); - result?; - Ok(dup) + &object_attrs, + POLICY_CREATE_ACCOUNT as u32, + &mut policy_handle, + ) + .ok()?; + + let status = LsaAddAccountRights(policy_handle, PSID(sid.as_ptr() as *mut _), &lsa_rights); + let _ = LsaClose(policy_handle); + status.ok() + } +} + +/// Rotates the service account's password to a random value the first time +/// this runs, then never again. Nothing ever needs it back — +/// `service_account_token` mints tokens via S4U, not `LogonUserW` — so +/// unlike the interactive user's stored password (`credential.rs`), this one +/// is generated and immediately discarded. The installer's `` +/// gives the account a fixed password only so the account can be created at +/// all; this is what turns that into a real secret, exactly once, and the +/// `HKLM` marker is what keeps it from happening again on every logon. +pub fn ensure_service_account_password_rotated(username: &str) -> eyre::Result<()> { + let hklm = winreg::RegKey::predef(winreg::enums::HKEY_LOCAL_MACHINE); + let (key, _disp) = hklm.create_subkey(SERVICE_ACCOUNT_STATE_KEY)?; + + if key.get_value::("PasswordRotated").unwrap_or(0) == 1 { + return Ok(()); + } + + let password = crate::helpers::generate_random_password().map_err(|e| eyre::eyre!("{e}"))?; + RealSyscalls + .reset(username, &password) + .map_err(|e| eyre::eyre!("{e}"))?; + key.set_value("PasswordRotated", &1u32)?; + Ok(()) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod s4u_tests { + use super::*; + + /// `MaximumLength` must cover the trailing NUL while `Length` excludes + /// it — get this backwards and `LsaLookupAuthenticationPackage` either + /// truncates the last real character or reads one byte past the buffer. + #[test] + fn lsa_string_length_excludes_the_trailing_nul() { + let s = lsa_string(b"Negotiate\0"); + assert_eq!(s.Length, 9); + assert_eq!(s.MaximumLength, 10); + let bytes = unsafe { std::slice::from_raw_parts(s.Buffer.0, s.Length as usize) }; + assert_eq!(bytes, b"Negotiate"); + } + + /// Unlike `LSA_STRING`, S4U's `LSA_UNICODE_STRING`s carry no NUL at all — + /// `Length`/`MaximumLength` are both the exact UTF-16 byte count. + #[test] + fn lsa_unicode_string_round_trips_without_a_nul() { + let wide: Vec = "ak-wcp-browser".encode_utf16().collect(); + let s = lsa_unicode_string(&wide); + assert_eq!(s.Length as usize, wide.len() * 2); + assert_eq!(s.MaximumLength, s.Length); + let read = unsafe { + String::from_utf16_lossy(std::slice::from_raw_parts( + s.Buffer.0, + (s.Length / 2) as usize, + )) + }; + assert_eq!(read, "ak-wcp-browser"); } } From 62cfc4cef37e94b39cc8397e8df2b20b320ffb8a Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 02:48:40 +0100 Subject: [PATCH 03/41] ee/wcp: update BROWSER_PRIVILEGE.md for the account work just built WiX's util:User has no GeneratePassword attribute, so the placeholder password/rotation approach differs from the first draft -- correct that, and update the identity table and "rest of the work" list to describe what credprovider now actually does instead of what was planned. --- ee/wcp/BROWSER_PRIVILEGE.md | 83 ++++++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 28 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 18180a6c..720aa68c 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -35,24 +35,30 @@ one call site in `cefsimple/CMakeLists.txt` was commented out, and disabled while `if(USE_SANDBOX)` still linked `cef_sandbox_lib` and applied `SET_LPAC_ACLS` — a build that looks sandboxed and is not. -**Identity — SYSTEM on the path that matters.** `credprovider::syscalls:: -acquire_interactive_token` tries `WTSQueryUserToken` first and falls back to -duplicating `winlogon.exe`'s token: +**Identity — was SYSTEM on the path that matters; fixed on this branch.** +Before this branch, `credprovider::syscalls::acquire_interactive_token` tried +`WTSQueryUserToken` first and fell back to duplicating `winlogon.exe`'s +token: -| Scenario | Token | Runs as | +| Scenario | Token | Ran as | | --- | --- | --- | -| `CPUS_LOGON` (fresh logon) | winlogon duplicate — nobody is signed in yet, so there is no user token to get | **SYSTEM** | -| `CPUS_UNLOCK_WORKSTATION` | `WTSQueryUserToken` | the locked-out user — but the spawn then fails, that token has no access to `WinSta0\Winlogon` | -| `CPUS_CREDUI` (debug) | none; the caller holds no `SE_TCB_NAME`, so it falls through to `CreateProcessW` | the interactive user | +| `CPUS_LOGON` (fresh logon) | winlogon duplicate — nobody is signed in yet, so there was no user token to get | **SYSTEM** | +| `CPUS_UNLOCK_WORKSTATION` | `WTSQueryUserToken` | the locked-out user — but the spawn then failed, that token had no access to `WinSta0\Winlogon` | +| `CPUS_CREDUI` (debug) | none; the caller holds no `SE_TCB_NAME`, so it fell through to `CreateProcessW` | the interactive user | The C++ ran CEF in-process inside `ak_cred_provider.dll`, i.e. inside LogonUI, so the browser process *was* LogonUI: SYSTEM, with helper processes inheriting that token. It never chose an identity because it never spawned anything. -So the existing token machinery buys nothing on fresh logon (it works hard to -arrive at the same SYSTEM the C++ got for free) and is broken on unlock. It is -not load-bearing for the sign-in flow; it is an unfinished attempt at this -document's goal. +So the old token machinery bought nothing on fresh logon (it worked hard to +arrive at the same SYSTEM the C++ got for free) and was broken on unlock — +not load-bearing for the sign-in flow, an unfinished attempt at this +document's goal. Replaced entirely: `credprovider::syscalls:: +service_account_token` now mints an S4U token for the dedicated account +(`SERVICE_ACCOUNT_NAME`, see Option B below) for both `CPUS_LOGON` and +`CPUS_UNLOCK_WORKSTATION` — one path, no per-scenario branching, no +`WTSQueryUserToken`/winlogon-scanning left in `syscalls.rs` at all. +`CPUS_CREDUI` is unchanged. ## Option A — enable the CEF sandbox @@ -110,9 +116,20 @@ means a secret at rest and a rotation story. `LsaLogonUser` with `MSV1_0_S4U_LOGON` avoids that entirely: it mints a token for a local account with no credentials, given `SE_TCB_NAME`, which LogonUI -already holds. Nothing to store, nothing to rotate. The installer still gives -`` a `GeneratePassword="yes"` password, because the account needs -one to exist — it is simply never read back by anything. +already holds. Nothing to store, nothing to rotate. + +One correction against the plan as first written here: WiX's `util:User` has +no `GeneratePassword` attribute — it only takes a literal `Password` (or one +read from a `Property` via `PasswordAttribute`), so the installer cannot mint +a random one itself. The account is created with a fixed placeholder +instead, and `credprovider::syscalls::ensure_service_account_password_rotated` +resets it to a random value the first time the DLL loads after install, +via the same `NetUserSetInfo` reset `LocalAccountPassword::reset` already +does for the interactive user's account — then never touches it again +(tracked by an `HKLM` marker). The placeholder is live for, at most, the +gap between install finishing and the first logon attempt; the account is +also denied interactive/network/RDP logon throughout, so even a known +placeholder cannot be used to sign anyone in. **The catch that made this a decision:** an S4U token carries no network credentials. That is fine for reaching authentik over HTTPS with the @@ -123,23 +140,33 @@ out of scope for this account**, so S4U stands. ### The rest of the work -- **Secure desktop ACL.** A non-SYSTEM token has no access to - `WinSta0\Winlogon`; add the account's SID to the window station and desktop - DACLs (`GetUserObjectSecurity`/`SetUserObjectSecurity`). Weigh it honestly: - this grants a service account the right to create windows on the desktop - where credentials are typed. Still a large net win — a compromised renderer - no longer yields SYSTEM — but a deliberate expansion of what can reach the - logon desktop, not a free improvement. +- **Secure desktop ACL — done.** `credprovider::syscalls:: + ensure_desktop_access` grants the account's SID `GENERIC_ALL` on both + `WinSta0` and its `Winlogon` desktop, merged onto the existing DACL via + `GetSecurityInfo`/`SetEntriesInAclW`/`SetSecurityInfo` rather than + replacing it. Weigh it honestly: this grants a service account the right + to create windows on the desktop where credentials are typed. Still a + large net win — a compromised renderer no longer yields SYSTEM — but a + deliberate expansion of what can reach the logon desktop, not a free + improvement. `GENERIC_ALL` is broader than this account strictly needs; + narrowing it to the specific window-station/desktop rights CEF actually + uses is a reasonable follow-up once real hardware confirms what those are. - **Profile and cache.** `root_cache_path` is explicit already, but Chromium also wants temp, fonts and crashpad paths. Without `LoadUserProfile` the - account gets the default profile. The MSI must also grant it write access to - `wcp-cache`, which currently just inherits ProgramData defaults. -- **Harden the account.** Deny interactive logon — it must not be usable to - sign in at the very screen it serves — plus deny network and RDP logon, - minimal group membership, no privileges. `RemoveOnUninstall`, and handle the - account already existing on upgrade. + account gets the default profile. The MSI grants the account write access + to `wcp-cache` (previously ProgramData defaults only); temp/fonts/crashpad + are not yet addressed and are the most likely source of first-run failures + on real hardware. +- **Harden the account — mostly done.** `credprovider::syscalls:: + deny_interactive_and_network_logon` denies `SeDenyInteractiveLogonRight`, + `SeDenyNetworkLogonRight` and `SeDenyRemoteInteractiveLogonRight` via + `LsaAddAccountRights`, called on every load (idempotent). The installer + creates the account with `RemoveOnUninstall="yes"`/`UpdateIfExists="yes"` + and no group membership beyond the default. Minimal-privilege trimming + beyond that is not yet done. - **Deployment friction.** GPO blocking local account creation, endpoint - monitoring flagging a new local account, no local accounts on a DC. + monitoring flagging a new local account, no local accounts on a DC — still + open, not addressable from this codebase. ## Decisions — resolved for this branch From 4a61cc7e434784eea7eccbc99a2699a90c3e08a4 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 02:57:26 +0100 Subject: [PATCH 04/41] vpkg/windows: create the dedicated wcp browser service account Adds the local account ak_cef.exe now runs as (credprovider's service_account_token/BROWSER_PRIVILEGE.md), created at install time via util:User with a placeholder password credprovider rotates away on first load, and grants it write access to wcp-cache. Verified with a full local `dotnet build` of the MSI (placeholder binaries, ICE validation included) -- only the wcp component's own binaries are still missing from this tree. --- vpkg/windows/Package.wxs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/vpkg/windows/Package.wxs b/vpkg/windows/Package.wxs index d3a040ca..a87deeff 100644 --- a/vpkg/windows/Package.wxs +++ b/vpkg/windows/Package.wxs @@ -122,9 +122,27 @@ Name="authentik Credential Provider (CEF)" EventMessageFile="[System64Folder]EventCreate.exe"/> + + + - + + + From 0d1bd364bef71eab22b2afdd0b93fec672299a8f Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 03:03:56 +0100 Subject: [PATCH 05/41] ee/wcp: fix a stale comment about which token the logon path uses Left over from the SYSTEM-token era this branch replaced: the spawned browser now carries the service account's S4U token, not this process's own SYSTEM one. --- ee/wcp/credprovider/src/ipc.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index dccdbcd8..5e7a7729 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -52,9 +52,12 @@ impl AuthFlow for CefAuthFlow { /// Only `CPUS_CREDUI` may fall back to launching in the caller's own session. /// It is debug-gated and runs on an ordinary desktop, where the caller is -/// already the interactive user and holds no `SE_TCB_NAME`. The logon -/// scenarios must never take it: they run as SYSTEM under LogonUI, so it -/// would put Chromium on the secure desktop with SYSTEM's token. +/// already the interactive user and holds no `SE_TCB_NAME` — so it cannot +/// reach `acquire_service_account_token`'s S4U logon in the first place. The +/// logon scenarios must never take this fallback: they always have that +/// privilege, and taking it would put Chromium on the secure desktop with +/// this process's own token (SYSTEM, since this DLL is loaded into LogonUI) +/// rather than the service account's. fn may_launch_in_current_session(cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO) -> bool { cpus == CPUS_CREDUI } From ba9384d1c0260b7d5b617f728ee51af3b85274b6 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 12:40:36 +0100 Subject: [PATCH 06/41] ee/wcp: record the VM run of the secure-desktop checklist The two things CI cannot reach -- the WinSta0\Winlogon ACL grant holding and the S4U service-account token working for logon as well as unlock -- now have a real run behind them rather than an argument. Keeps the ACL-narrowing and temp/fonts/crashpad items open. One VM not failing is not the same as either being handled. Also commits the checklist edits the run was done against: registering by hand skips the MSI's util:User, so the service account has to exist first, and step 5 no longer describes a token fallback that S4U replaced. Co-Authored-By: Claude Opus 5 --- ee/wcp/BROWSER_PRIVILEGE.md | 18 +++++++++++++----- ee/wcp/e2e/README.md | 14 ++++++++++---- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 720aa68c..7edffbac 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -14,6 +14,12 @@ Option A turned out to need more than this doc assumed — see the note at the end of that section — so it stayed out of scope rather than being folded in here. +**Verified on a VM** against the manual checklist in `e2e/README.md`: the +tile appears, the sign-in window opens on the secure desktop, and both fresh +logon and unlock complete. So the two things CI cannot reach — the +`WinSta0\Winlogon` ACL grant actually holding, and the S4U service-account +token being usable for both scenarios — hold outside a test harness. + This file previously lived on `ee/wcp/rs-cef-fresh` and was deleted there in a "cleanup" commit an hour after being written, along with three sibling design docs; nothing indicates that was deliberate. Restored from @@ -148,15 +154,17 @@ out of scope for this account**, so S4U stands. to create windows on the desktop where credentials are typed. Still a large net win — a compromised renderer no longer yields SYSTEM — but a deliberate expansion of what can reach the logon desktop, not a free - improvement. `GENERIC_ALL` is broader than this account strictly needs; - narrowing it to the specific window-station/desktop rights CEF actually - uses is a reasonable follow-up once real hardware confirms what those are. + improvement. Confirmed working on a VM. `GENERIC_ALL` is broader than this + account strictly needs; narrowing it to the specific window-station/desktop + rights CEF actually uses is still open — the VM run proves the grant is + sufficient, not that it is minimal. - **Profile and cache.** `root_cache_path` is explicit already, but Chromium also wants temp, fonts and crashpad paths. Without `LoadUserProfile` the account gets the default profile. The MSI grants the account write access to `wcp-cache` (previously ProgramData defaults only); temp/fonts/crashpad - are not yet addressed and are the most likely source of first-run failures - on real hardware. + are still not addressed. They were the expected source of first-run + failures, and the VM run did not hit them — but "did not fail on one VM" is + weaker than "handled", so leave this open. - **Harden the account — mostly done.** `credprovider::syscalls:: deny_interactive_and_network_logon` denies `SeDenyInteractiveLogonRight`, `SeDenyNetworkLogonRight` and `SeDenyRemoteInteractiveLogonRight` via diff --git a/ee/wcp/e2e/README.md b/ee/wcp/e2e/README.md index 815fa7ba..52b4e59b 100644 --- a/ee/wcp/e2e/README.md +++ b/ee/wcp/e2e/README.md @@ -90,10 +90,16 @@ logon/unlock/lock-screen prompt. After the automated tests pass: production; for manual testing, add the registry entries under `HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\Authentication\Credential Providers\{7BCC7941-18BA-4A8E-8E0A-1D0F8E73577A}` and - `HKCR\CLSID\{7BCC7941-18BA-4A8E-8E0A-1D0F8E73577A}\InprocServer32`). + `HKCR\CLSID\{7BCC7941-18BA-4A8E-8E0A-1D0F8E73577A}\InprocServer32`). This + skips the MSI's `util:User`, though, so `ak_cef.exe` now also needs the + dedicated service account (`BROWSER_PRIVILEGE.md`) to exist for logon and + unlock to work — either run the real MSI once first, or create it by hand + to match `credprovider::syscalls::SERVICE_ACCOUNT_NAME`. 4. Lock the machine (Win+L) and confirm the tile appears with the expected icon/text, opens the sign-in window on Submit at the expected size, and that completing/cancelling sign-in behaves as expected. -5. Confirm a **fresh logon** (not just unlock) also works — that path has no - existing user token, so it exercises the winlogon-token-duplication - fallback in `syscalls::acquire_interactive_token`. +5. Confirm a **fresh logon** (not just unlock) also works — that path used + to have no existing user token to fall back to, which is what made it + worth checking separately from unlock; now both scenarios go through the + same `syscalls::service_account_token` S4U logon, so this step mainly + confirms the secure-desktop ACL grant actually holds on real hardware. From b609551e44a74d0c0931a71cd0239a413835a881 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 12:43:50 +0100 Subject: [PATCH 07/41] ee/wcp: drop stored-pw's password-reuse machinery from this branch The branch's earlier rebase onto main replayed these commits onto main's actual (simpler) syscalls.rs/credential.rs -- the one merged via #1374/#1378 -- but syscalls.rs still carried PasswordStore, KeyringPasswordStore, ak_platform_keyring and the richer LocalAccountPassword trait from a different, unmerged branch (ee/wcp/stored-pw) that this branch was never meant to depend on. credential.rs never had that machinery to begin with here, so the mismatch didn't compile (LocalAccountPasswordReset vs LocalAccountPassword, missing ak_platform_keyring). Rebuilds the service-account additions directly on main's actual LocalAccountPasswordReset/reset-only API instead. No behavior change to any of this branch's own logic -- ensure_service_account_password_rotated still calls the same single reset, just through the trait that actually exists here. --- ee/wcp/BROWSER_PRIVILEGE.md | 13 +- ee/wcp/credprovider/src/syscalls.rs | 178 +++------------------------- 2 files changed, 23 insertions(+), 168 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 7edffbac..79069767 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -129,13 +129,12 @@ no `GeneratePassword` attribute — it only takes a literal `Password` (or one read from a `Property` via `PasswordAttribute`), so the installer cannot mint a random one itself. The account is created with a fixed placeholder instead, and `credprovider::syscalls::ensure_service_account_password_rotated` -resets it to a random value the first time the DLL loads after install, -via the same `NetUserSetInfo` reset `LocalAccountPassword::reset` already -does for the interactive user's account — then never touches it again -(tracked by an `HKLM` marker). The placeholder is live for, at most, the -gap between install finishing and the first logon attempt; the account is -also denied interactive/network/RDP logon throughout, so even a known -placeholder cannot be used to sign anyone in. +resets it to a random value the first time the DLL loads after install, via +`NetUserSetInfo` through the existing `LocalAccountPasswordReset::reset` — +then never touches it again (tracked by an `HKLM` marker). The placeholder is +live for, at most, the gap between install finishing and the first logon +attempt; the account is also denied interactive/network/RDP logon +throughout, so even a known placeholder cannot be used to sign anyone in. **The catch that made this a decision:** an S4U token carries no network credentials. That is fine for reaching authentik over HTTPS with the diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index 7e37246b..f6f6892c 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -2,15 +2,11 @@ //! effects, so the logic that decides *when* to call them can be unit //! tested against a fake instead of the OS. -use ak_platform_keyring::{KeyringError, windows::WindowsStore}; -use windows::Win32::Foundation::{CloseHandle, HANDLE}; +use windows::Win32::Foundation::HANDLE; use windows::{ Win32::{ - Foundation::{ - ERROR_LOGON_FAILURE, ERROR_PASSWORD_EXPIRED, ERROR_PASSWORD_MUST_CHANGE, GENERIC_ALL, - HLOCAL, LUID, LocalFree, - }, - NetworkManagement::NetManagement::{NetUserChangePassword, NetUserSetInfo, USER_INFO_1003}, + Foundation::{E_FAIL, GENERIC_ALL, HLOCAL, LUID, LocalFree}, + NetworkManagement::NetManagement::{NetUserSetInfo, USER_INFO_1003}, Security::{ ACL, AllocateLocallyUniqueId, Authentication::Identity::{ @@ -25,16 +21,15 @@ use windows::{ SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, }, - DACL_SECURITY_INFORMATION, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, LogonUserW, - LookupAccountNameW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, QUOTA_LIMITS, - SID_NAME_USE, TOKEN_SOURCE, + DACL_SECURITY_INFORMATION, LookupAccountNameW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, + PSID, QUOTA_LIMITS, SID_NAME_USE, TOKEN_SOURCE, }, Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, System::StationsAndDesktops::{ DESKTOP_CONTROL_FLAGS, GetProcessWindowStation, OpenDesktopW, }, }, - core::{HRESULT, PCWSTR, PSTR, PWSTR, w}, + core::{PCWSTR, PSTR, PWSTR, w}, }; const SERVICE_ACCOUNT_STATE_KEY: &str = @@ -44,37 +39,8 @@ pub trait AuthPackageLookup { fn negotiate_package(&self) -> windows::core::Result; } -pub trait LocalAccountPassword { - /// Administrative reset. Orphans the account's DPAPI master key — stored - /// passwords, EFS files and personal certificates become unreadable — so - /// this is only for first use and for recovering an account whose password - /// we no longer know. +pub trait LocalAccountPasswordReset { fn reset(&self, username: &str, password: &str) -> windows::core::Result<()>; - - /// Self-service change. Supplying the old password lets LSA re-encrypt the - /// DPAPI master key instead of orphaning it, which is why rotation goes - /// through here and not through `reset`. - fn change(&self, username: &str, old: &str, new: &str) -> windows::core::Result<()>; - - /// Whether `password` is still this account's password. Only codes that - /// say something definite about the credential produce an `Ok`; anything - /// else is `Err`, because "could not tell" must not be read as "wrong - /// password". - fn validate(&self, username: &str, password: &str) -> windows::core::Result; -} - -#[derive(Debug, PartialEq, Eq)] -pub enum PasswordCheck { - Valid, - /// Correct, but the account will not accept it for logon until it changes. - Expired, - Rejected, -} - -/// Where the local account's password is kept between sign-ins. -pub trait PasswordStore { - fn load(&self, sid: &str) -> eyre::Result>; - fn save(&self, sid: &str, password: &str) -> eyre::Result<()>; } pub struct RealSyscalls; @@ -120,7 +86,7 @@ impl AuthPackageLookup for RealSyscalls { } } -impl LocalAccountPassword for RealSyscalls { +impl LocalAccountPasswordReset for RealSyscalls { fn reset(&self, username: &str, password: &str) -> windows::core::Result<()> { let username_wide = wide(username); let password_wide = wide(password); @@ -140,120 +106,10 @@ impl LocalAccountPassword for RealSyscalls { }; if status != 0 { - return Err(net_api_error(status)); + return Err(windows::core::Error::from(E_FAIL)); } Ok(()) } - - fn change(&self, username: &str, old: &str, new: &str) -> windows::core::Result<()> { - let username_wide = wide(username); - let old_wide = wide(old); - let new_wide = wide(new); - - let status = unsafe { - NetUserChangePassword( - PCWSTR::null(), - PCWSTR(username_wide.as_ptr()), - PCWSTR(old_wide.as_ptr()), - PCWSTR(new_wide.as_ptr()), - ) - }; - - if status != 0 { - return Err(net_api_error(status)); - } - Ok(()) - } - - fn validate(&self, username: &str, password: &str) -> windows::core::Result { - let username_wide = wide(username); - let password_wide = wide(password); - - // A network logon validates the credential without building a session, - // and `.` scopes the lookup to this machine's account database. - let mut token = HANDLE::default(); - let result = unsafe { - LogonUserW( - PCWSTR(username_wide.as_ptr()), - w!("."), - PCWSTR(password_wide.as_ptr()), - LOGON32_LOGON_NETWORK, - LOGON32_PROVIDER_DEFAULT, - &mut token, - ) - }; - - match result { - Ok(()) => { - unsafe { - let _ = CloseHandle(token); - } - Ok(PasswordCheck::Valid) - } - // Anything not listed here — a policy denying network logons, a - // locked-out or disabled account — means the check was - // inconclusive, not that the password is wrong, and the caller - // must not reset on the strength of it. - Err(e) if e.code() == ERROR_LOGON_FAILURE.to_hresult() => Ok(PasswordCheck::Rejected), - Err(e) - if e.code() == ERROR_PASSWORD_EXPIRED.to_hresult() - || e.code() == ERROR_PASSWORD_MUST_CHANGE.to_hresult() => - { - Ok(PasswordCheck::Expired) - } - Err(e) => Err(e), - } - } -} - -/// `NET_API_STATUS` codes are Win32 error codes, so they survive the trip -/// through `HRESULT` and stay readable in the log. -fn net_api_error(status: u32) -> windows::core::Error { - windows::core::Error::from_hresult(HRESULT::from_win32(status)) -} - -/// Keyring-backed [`PasswordStore`], keyed by SID so renaming the account does -/// not orphan the entry. -/// -/// This reaches for [`WindowsStore`] rather than `ak_platform_keyring::store()` -/// because the latter resolves to the in-memory store under `debug_assertions`, -/// which would silently lose the password between LogonUI processes in every -/// development build. Local-machine persistence keeps a secret that is -/// meaningless off this box from roaming to a domain. -pub struct KeyringPasswordStore { - store: WindowsStore, - service: String, -} - -impl Default for KeyringPasswordStore { - fn default() -> Self { - KeyringPasswordStore::new() - } -} - -impl KeyringPasswordStore { - pub fn new() -> Self { - KeyringPasswordStore { - store: WindowsStore::new_local_machine(), - service: ak_platform_keyring::service("wcp-account-password"), - } - } -} - -impl PasswordStore for KeyringPasswordStore { - fn load(&self, sid: &str) -> eyre::Result> { - match self.store.get_blocking(&self.service, sid) { - Ok(password) => Ok(Some(password)), - Err(KeyringError::NotFound()) => Ok(None), - Err(e) => Err(eyre::eyre!("{e}")), - } - } - - fn save(&self, sid: &str, password: &str) -> eyre::Result<()> { - self.store - .set_blocking(&self.service, sid, password) - .map_err(|e| eyre::eyre!("{e}")) - } } /// Name of the dedicated local account `ak_cef.exe` runs as instead of @@ -303,9 +159,9 @@ fn lsa_unicode_string(wide: &[u16]) -> LSA_UNICODE_STRING { /// Mints a primary token for the service account via an S4U logon — no /// password needed, only `SE_TCB_NAME`, which LogonUI already holds. Nothing -/// to store, nothing to rotate: unlike the interactive user's account -/// (`credential.rs`, `LOCAL_PASSWORD.md`), this account's credential never -/// has to survive being handed to LSA a second time. +/// to store, nothing to rotate: unlike the interactive user's own account, +/// this account's credential never has to survive being handed to LSA a +/// second time. /// /// `Service` is the logon type, deliberately: it is the one type this /// account is not denied by `deny_interactive_and_network_logon`, so @@ -512,11 +368,11 @@ pub fn deny_interactive_and_network_logon(sid: &[u8]) -> windows::core::Result<( /// Rotates the service account's password to a random value the first time /// this runs, then never again. Nothing ever needs it back — /// `service_account_token` mints tokens via S4U, not `LogonUserW` — so -/// unlike the interactive user's stored password (`credential.rs`), this one -/// is generated and immediately discarded. The installer's `` -/// gives the account a fixed password only so the account can be created at -/// all; this is what turns that into a real secret, exactly once, and the -/// `HKLM` marker is what keeps it from happening again on every logon. +/// unlike the interactive user's stored password, this one is generated and +/// immediately discarded. The installer's `` gives the account a +/// fixed password only so the account can be created at all; this is what +/// turns that into a real secret, exactly once, and the `HKLM` marker is +/// what keeps it from happening again on every logon. pub fn ensure_service_account_password_rotated(username: &str) -> eyre::Result<()> { let hklm = winreg::RegKey::predef(winreg::enums::HKEY_LOCAL_MACHINE); let (key, _disp) = hklm.create_subkey(SERVICE_ACCOUNT_STATE_KEY)?; From f3aedcc6aecade50c8c0f9c82056a5b77b904985 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 17:17:06 +0100 Subject: [PATCH 08/41] ee/wcp: fix a trait name gone stale after merging main LocalAccountPasswordReset was this branch's stand-in for the trait name before #1379 (properly reusing a stored password for the interactive user's account) landed on main; merging main back in restored its real name, LocalAccountPassword. --- ee/wcp/BROWSER_PRIVILEGE.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 79069767..fb51ced4 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -130,8 +130,11 @@ read from a `Property` via `PasswordAttribute`), so the installer cannot mint a random one itself. The account is created with a fixed placeholder instead, and `credprovider::syscalls::ensure_service_account_password_rotated` resets it to a random value the first time the DLL loads after install, via -`NetUserSetInfo` through the existing `LocalAccountPasswordReset::reset` — -then never touches it again (tracked by an `HKLM` marker). The placeholder is +`NetUserSetInfo` through the existing `LocalAccountPassword::reset` — the +same call the interactive user's own account uses for its first-use/ +out-of-band-change reset — then never touches it again (tracked by an +`HKLM` marker, not the credential-manager vault that account's password +lives in, since nothing here ever needs this one back). The placeholder is live for, at most, the gap between install finishing and the first logon attempt; the account is also denied interactive/network/RDP logon throughout, so even a known placeholder cannot be used to sign anyone in. From 9fd90e7056cbdcd38b57ba2076382f99f4aa9587 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 18:04:32 +0100 Subject: [PATCH 09/41] vpkg/windows: drop $ from the wcp service account's placeholder password $ is the WiX preprocessor's variable-substitution leader (\$(var.X), \$(env.X)) and this file is preprocessed before the XML is parsed -- plausible cause of the "invalid password" failure at install, since the literal value NetUserAdd ends up receiving may not be what the source says. Also lengthened it as a hedge against a stricter than default minimum-length policy on the install target. No functional change either way: the account is denied all interactive/network/RDP logon and credprovider rotates this away on first load, so the exact value has never mattered past satisfying account creation. --- vpkg/windows/Package.wxs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/vpkg/windows/Package.wxs b/vpkg/windows/Package.wxs index a87deeff..2a4622d2 100644 --- a/vpkg/windows/Package.wxs +++ b/vpkg/windows/Package.wxs @@ -129,10 +129,17 @@ first time it loads after install, and the account is denied interactive/network/RDP logon throughout, so the placeholder is never a usable credential. Keep "Name" in step with - credprovider::syscalls::SERVICE_ACCOUNT_NAME. --> + credprovider::syscalls::SERVICE_ACCOUNT_NAME. + + No "$" here (or elsewhere in this value): that is the WiX + preprocessor's variable substitution leader ("$(var.X)", + "$(env.X)"), and this file is preprocessed before the XML is + parsed. Long and mixed case/digit/symbol on purpose, to clear + a stricter than default local or domain minimum length policy + without needing to know what it is. --> Date: Sun, 16 Aug 2026 18:31:13 +0100 Subject: [PATCH 10/41] ee/wcp: trim doc comments down to one point each Several ran two paragraphs restating things BROWSER_PRIVILEGE.md already covers, or spelling out both a function's rationale and its caller's in one place. Same information, less of it per comment. --- ee/wcp/credprovider/src/ipc.rs | 25 ++++++----------- ee/wcp/credprovider/src/syscalls.rs | 43 ++++++++++------------------- 2 files changed, 22 insertions(+), 46 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 5e7a7729..fdb8665e 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -51,13 +51,10 @@ impl AuthFlow for CefAuthFlow { } /// Only `CPUS_CREDUI` may fall back to launching in the caller's own session. -/// It is debug-gated and runs on an ordinary desktop, where the caller is -/// already the interactive user and holds no `SE_TCB_NAME` — so it cannot -/// reach `acquire_service_account_token`'s S4U logon in the first place. The -/// logon scenarios must never take this fallback: they always have that -/// privilege, and taking it would put Chromium on the secure desktop with -/// this process's own token (SYSTEM, since this DLL is loaded into LogonUI) -/// rather than the service account's. +/// It is debug-gated and runs on an ordinary desktop, where the caller holds +/// no `SE_TCB_NAME` and so cannot reach the S4U logon anyway. The logon +/// scenarios must never take this fallback, or Chromium ends up on the +/// secure desktop with this process's own SYSTEM token instead. fn may_launch_in_current_session(cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO) -> bool { cpus == CPUS_CREDUI } @@ -274,16 +271,10 @@ fn signal_cancel(cancel_write: HANDLE) { } /// Gets `ak_cef.exe` a token for the dedicated service account rather than -/// SYSTEM (`BROWSER_PRIVILEGE.md`). The account exists from install time -/// (`vpkg/windows/Package.wxs`), so unlike the old `WTSQueryUserToken` / -/// winlogon-duplication fallback this needs no session-dependent branching: -/// logon and unlock both end up here. -/// -/// Password rotation and the account-hardening calls are best-effort and -/// only logged on failure — each is idempotent, so a transient failure here -/// just means the next call tries again, and none of them being fatal keeps -/// a permissions hiccup on one from blocking `service_account_token` from -/// still being attempted. +/// SYSTEM (`BROWSER_PRIVILEGE.md`), the same way for both logon and unlock. +/// Password rotation and account-hardening are best-effort and only logged +/// on failure — each is idempotent, so a transient failure just costs a +/// retry next time, and none of them block the token mint that follows. fn acquire_service_account_token() -> windows::core::Result { if let Err(e) = syscalls::ensure_service_account_password_rotated(syscalls::SERVICE_ACCOUNT_NAME) diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index 4b8533f3..0dd082a1 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -295,14 +295,9 @@ fn lsa_unicode_string(wide: &[u16]) -> LSA_UNICODE_STRING { } /// Mints a primary token for the service account via an S4U logon — no -/// password needed, only `SE_TCB_NAME`, which LogonUI already holds. Nothing -/// to store, nothing to rotate: unlike the interactive user's own account, -/// this account's credential never has to survive being handed to LSA a -/// second time. -/// -/// `Service` is the logon type, deliberately: it is the one type this -/// account is not denied by `deny_interactive_and_network_logon`, so -/// hardening the account does not also lock this call out. +/// password needed, only `SE_TCB_NAME`, which LogonUI already holds. +/// `Service` is the logon type deliberately: it is the one type +/// `deny_interactive_and_network_logon` does not deny. pub fn service_account_token(username: &str) -> windows::core::Result { unsafe { let process_name = lsa_string(b"ak_cred_provider\0"); @@ -445,15 +440,10 @@ unsafe fn grant_generic_all( } /// Grants the service account's SID access to the secure desktop — a -/// non-SYSTEM token has none by default. Deliberate, per -/// `BROWSER_PRIVILEGE.md`: a large net win (a compromised renderer no longer -/// yields SYSTEM) but a real expansion of what can reach the desktop -/// credentials are typed on, scoped to only this one account. -/// -/// Idempotent, and only correct when called from inside LogonUI's own -/// process: `GetProcessWindowStation`/`OpenDesktopW` resolve relative to the -/// *caller's* window station, which is `WinSta0` for the real logon -/// scenarios `ipc.rs` targets. +/// non-SYSTEM token has none by default. Deliberate; see `BROWSER_PRIVILEGE.md` +/// for the tradeoff. Idempotent, and only correct when called from inside +/// LogonUI's own process: `GetProcessWindowStation`/`OpenDesktopW` resolve +/// relative to the *caller's* window station, `WinSta0` here. pub fn ensure_desktop_access(sid: &[u8]) -> windows::core::Result<()> { unsafe { let winsta = GetProcessWindowStation()?; @@ -470,11 +460,9 @@ pub fn ensure_desktop_access(sid: &[u8]) -> windows::core::Result<()> { } /// Denies the service account the logon types that would let it sign someone -/// in — it must not be usable at the very screen it serves. `Service`, what -/// `service_account_token` uses, is deliberately not among these. -/// -/// `LsaAddAccountRights` is itself idempotent: granting an already-held -/// right is a no-op, so this is safe to call on every load. +/// in — it must not be usable at the very screen it serves. `Service`, +/// what `service_account_token` uses, is deliberately not among these. +/// `LsaAddAccountRights` is itself idempotent, so this is safe on every load. pub fn deny_interactive_and_network_logon(sid: &[u8]) -> windows::core::Result<()> { const RIGHTS: [&str; 3] = [ "SeDenyInteractiveLogonRight", @@ -503,13 +491,10 @@ pub fn deny_interactive_and_network_logon(sid: &[u8]) -> windows::core::Result<( } /// Rotates the service account's password to a random value the first time -/// this runs, then never again. Nothing ever needs it back — -/// `service_account_token` mints tokens via S4U, not `LogonUserW` — so -/// unlike the interactive user's stored password, this one is generated and -/// immediately discarded. The installer's `` gives the account a -/// fixed password only so the account can be created at all; this is what -/// turns that into a real secret, exactly once, and the `HKLM` marker is -/// what keeps it from happening again on every logon. +/// this runs, then never again — nothing needs it back, since +/// `service_account_token` mints tokens via S4U, not `LogonUserW`. Turns the +/// installer's fixed placeholder password into a real secret exactly once; +/// the `HKLM` marker is what stops it happening again on every logon. pub fn ensure_service_account_password_rotated(username: &str) -> eyre::Result<()> { let hklm = winreg::RegKey::predef(winreg::enums::HKEY_LOCAL_MACHINE); let (key, _disp) = hklm.create_subkey(SERVICE_ACCOUNT_STATE_KEY)?; From 31d63682c39b3a1aa02797f47aadd80335f5a0a3 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 16 Aug 2026 18:32:37 +0100 Subject: [PATCH 11/41] vpkg/windows: trim the service account comment to one paragraph --- vpkg/windows/Package.wxs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/vpkg/windows/Package.wxs b/vpkg/windows/Package.wxs index 2a4622d2..f26caa08 100644 --- a/vpkg/windows/Package.wxs +++ b/vpkg/windows/Package.wxs @@ -124,19 +124,13 @@ + random password" attribute, so this is a fixed placeholder that + credprovider rotates away on first load; the account is denied + all interactive/network/RDP logon regardless. No "$" in the + value: that is the WiX preprocessor's substitution leader + ("$(var.X)"), and this file is preprocessed before the XML is + parsed. Keep "Name" in step with + credprovider::syscalls::SERVICE_ACCOUNT_NAME. --> Date: Sun, 16 Aug 2026 18:50:46 +0100 Subject: [PATCH 12/41] vpkg/windows: replace the placeholder password with a random string `net accounts` on the install target shows minimum password length 0 and no maximum, ruling out length as the cause of "invalid password" -- so the previous two fixes (dropping "$", lengthening it) were treating the wrong symptom. NetUserAdd folds LSA password-filter rejections (complexity, banned-word/dictionary lists) into the same NERR_PasswordTooShort a real length failure produces, and the old placeholder contained a real word ("Placeholder"). Replaced it with a random string with no recognizable words at all. --- vpkg/windows/Package.wxs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/vpkg/windows/Package.wxs b/vpkg/windows/Package.wxs index f26caa08..67f356a9 100644 --- a/vpkg/windows/Package.wxs +++ b/vpkg/windows/Package.wxs @@ -126,14 +126,16 @@ ee/wcp/BROWSER_PRIVILEGE.md). util:User has no "generate a random password" attribute, so this is a fixed placeholder that credprovider rotates away on first load; the account is denied - all interactive/network/RDP logon regardless. No "$" in the - value: that is the WiX preprocessor's substitution leader - ("$(var.X)"), and this file is preprocessed before the XML is + all interactive/network/RDP logon regardless. No dictionary + words and no "$": NetUserAdd folds complexity/banned-word LSA + filter rejections into the same NERR_PasswordTooShort a real + length failure would give, and "$" is the WiX preprocessor's + substitution leader ("$(var.X)"), live before the XML is parsed. Keep "Name" in step with credprovider::syscalls::SERVICE_ACCOUNT_NAME. --> Date: Mon, 17 Aug 2026 21:58:28 +0100 Subject: [PATCH 13/41] ee/wcp: fix two issues that survived the main merge - ipc.rs lost its ForegroundControl import in conflict resolution: RealSyscalls.foreground_pid() didn't compile without the trait in scope. - e2e/README.md's manual checklist kept both sides' "confirm a fresh logon" step -- the old one referencing syscalls::acquire_interactive_token, which this branch deleted. Dropped the stale duplicate. --- ee/wcp/credprovider/src/ipc.rs | 2 +- ee/wcp/e2e/README.md | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 1e75dec3..1aefaaab 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -29,7 +29,7 @@ use windows::{ core::{PCWSTR, PWSTR}, }; -use crate::syscalls; +use crate::syscalls::{self, ForegroundControl}; use ak_ee_wcp_wire::AuthResult; /// Spawns `ak_cef.exe` and waits for its result. `should_continue` is polled diff --git a/ee/wcp/e2e/README.md b/ee/wcp/e2e/README.md index 214de2fe..ad9d1a37 100644 --- a/ee/wcp/e2e/README.md +++ b/ee/wcp/e2e/README.md @@ -103,14 +103,11 @@ logon/unlock/lock-screen prompt. After the automated tests pass: worth checking separately from unlock; now both scenarios go through the same `syscalls::service_account_token` S4U logon, so this step mainly confirms the secure-desktop ACL grant actually holds on real hardware. -6. Confirm a **fresh logon** (not just unlock) also works — that path has no - existing user token, so it exercises the winlogon-token-duplication - fallback in `syscalls::acquire_interactive_token`. -7. Sign in **three times in a row**, not once. The window has come to the +6. Sign in **three times in a row**, not once. The window has come to the front on the first authentication after an install and stayed behind LogonUI on every one after it, so a single sign-in passes with that bug fully present. -8. On each, **type before clicking**. The characters have to land in the +7. On each, **type before clicking**. The characters have to land in the sign-in window, not LogonUI. The window is topmost whether or not it holds the foreground, so visible no longer implies focused. When it does not, `ak_cef.exe` logs `visible but never took focus` to the Application event From bc5d52120cf6441f449c9c9d979115b2e135d80f Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Mon, 17 Aug 2026 23:10:03 +0100 Subject: [PATCH 14/41] try to fix --- ee/wcp/credprovider/src/syscalls.rs | 59 ++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index dbc7b55a..4ed50f6a 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -13,12 +13,12 @@ use windows::Win32::UI::WindowsAndMessaging::{ use windows::{ Win32::{ Foundation::{ - ERROR_LOGON_FAILURE, ERROR_PASSWORD_EXPIRED, ERROR_PASSWORD_MUST_CHANGE, GENERIC_ALL, - HLOCAL, LUID, LocalFree, + ERROR_LOGON_FAILURE, ERROR_NOT_ALL_ASSIGNED, ERROR_PASSWORD_EXPIRED, + ERROR_PASSWORD_MUST_CHANGE, GENERIC_ALL, GetLastError, HLOCAL, LUID, LocalFree, }, NetworkManagement::NetManagement::{NetUserChangePassword, NetUserSetInfo, USER_INFO_1003}, Security::{ - ACL, AllocateLocallyUniqueId, + ACL, AdjustTokenPrivileges, AllocateLocallyUniqueId, Authentication::Identity::{ LSA_HANDLE, LSA_OBJECT_ATTRIBUTES, LSA_STRING, LSA_UNICODE_STRING, LsaAddAccountRights, LsaClose, LsaConnectUntrusted, LsaDeregisterLogonProcess, @@ -31,14 +31,16 @@ use windows::{ SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, }, - DACL_SECURITY_INFORMATION, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, LogonUserW, - LookupAccountNameW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, QUOTA_LIMITS, - SID_NAME_USE, TOKEN_SOURCE, + DACL_SECURITY_INFORMATION, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, + LUID_AND_ATTRIBUTES, LogonUserW, LookupAccountNameW, LookupPrivilegeValueW, + NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, QUOTA_LIMITS, SE_PRIVILEGE_ENABLED, + SE_TCB_NAME, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, TOKEN_PRIVILEGES, TOKEN_SOURCE, }, Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, System::StationsAndDesktops::{ DESKTOP_CONTROL_FLAGS, GetProcessWindowStation, OpenDesktopW, }, + System::Threading::{GetCurrentProcess, OpenProcessToken}, }, core::{HRESULT, PCWSTR, PSTR, PWSTR, w}, }; @@ -382,12 +384,51 @@ fn lsa_unicode_string(wide: &[u16]) -> LSA_UNICODE_STRING { } } +/// `SeTcbPrivilege` is in SYSTEM's token, but — like every privilege that +/// isn't `SE_PRIVILEGE_ENABLED_BY_DEFAULT` — held disabled until asked for. +/// `LsaRegisterLogonProcess` checks it as active, not merely present, and +/// without this first it fails the ALPC connect outright with +/// `STATUS_PORT_CONNECTION_REFUSED` before ever reaching a logon check. +fn enable_tcb_privilege() -> windows::core::Result<()> { + unsafe { + let mut token = HANDLE::default(); + OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES, &mut token)?; + + let mut luid = LUID::default(); + let result = (|| { + LookupPrivilegeValueW(PCWSTR::null(), SE_TCB_NAME, &mut luid)?; + let privileges = TOKEN_PRIVILEGES { + PrivilegeCount: 1, + Privileges: [LUID_AND_ATTRIBUTES { + Luid: luid, + Attributes: SE_PRIVILEGE_ENABLED, + }], + }; + AdjustTokenPrivileges(token, false, Some(&privileges), 0, None, None)?; + // The call above reports success even when the privilege was not + // actually held to enable — a classic trap, and indistinguishable + // from a real success without this check. + if GetLastError() == ERROR_NOT_ALL_ASSIGNED { + return Err(windows::core::Error::from(HRESULT::from_win32( + ERROR_NOT_ALL_ASSIGNED.0, + ))); + } + Ok(()) + })(); + + let _ = CloseHandle(token); + result + } +} + /// Mints a primary token for the service account via an S4U logon — no -/// password needed, only `SE_TCB_NAME`, which LogonUI already holds. -/// `Service` is the logon type deliberately: it is the one type -/// `deny_interactive_and_network_logon` does not deny. +/// password needed, only `SE_TCB_NAME`. `Service` is the logon type +/// deliberately: it is the one type `deny_interactive_and_network_logon` +/// does not deny. pub fn service_account_token(username: &str) -> windows::core::Result { unsafe { + enable_tcb_privilege()?; + let process_name = lsa_string(b"ak_cred_provider\0"); let mut lsa_handle = HANDLE::default(); let mut security_mode = 0u32; From 49de0bcade771266df3c89257efb4eb60b992c6c Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Tue, 18 Aug 2026 00:16:05 +0100 Subject: [PATCH 15/41] ee/wcp: enable the privileges CreateProcessAsUserW and the S4U logon need Both LsaRegisterLogonProcess and CreateProcessAsUserW check the privileges they need (SE_TCB_NAME, and SE_ASSIGNPRIMARYTOKEN_NAME/ SE_INCREASE_QUOTA_NAME respectively) as enabled, not merely present -- SYSTEM's token holds all three but disabled by default, same as any privilege that isn't SE_PRIVILEGE_ENABLED_BY_DEFAULT. Generalizes the one-off TCB-enabling helper into enable_privilege(name) and calls it for all three, fixing STATUS_PORT_CONNECTION_REFUSED from the LSA connect and ERROR_NOT_ALL_ASSIGNED from CreateProcessAsUserW seen on a real test build. Corrects the CreateProcessAsUserW retry comment, which had assumed this was unreachable on the real logon scenarios. --- ee/wcp/credprovider/src/ipc.rs | 19 +++++++++++++------ ee/wcp/credprovider/src/syscalls.rs | 14 ++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 1aefaaab..1bc47cc9 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -15,7 +15,7 @@ use windows::{ CloseHandle, E_FAIL, HANDLE, HANDLE_FLAG_INHERIT, HANDLE_FLAGS, SetHandleInformation, WAIT_OBJECT_0, }, - Security::SECURITY_ATTRIBUTES, + Security::{SE_ASSIGNPRIMARYTOKEN_NAME, SE_INCREASE_QUOTA_NAME, SECURITY_ATTRIBUTES}, System::Pipes::CreatePipe, System::Threading::{ CreateProcessAsUserW, CreateProcessW, DeleteProcThreadAttributeList, @@ -457,6 +457,15 @@ fn spawn_cef_host( let mut spawned = match token { Some(token) => unsafe { + // Disabled by default like `SE_TCB_NAME` before it; without + // enabling them, `CreateProcessAsUserW` fails with + // `ERROR_NOT_ALL_ASSIGNED` even though `token` is valid. + if let Err(e) = syscalls::enable_privilege(SE_ASSIGNPRIMARYTOKEN_NAME) { + log::warn!("could not enable SeAssignPrimaryTokenPrivilege: {e}"); + } + if let Err(e) = syscalls::enable_privilege(SE_INCREASE_QUOTA_NAME) { + log::warn!("could not enable SeIncreaseQuotaPrivilege: {e}"); + } CreateProcessAsUserW( Some(token), PCWSTR::null(), @@ -474,11 +483,9 @@ fn spawn_cef_host( None => spawn_in_current_session(&cmdline, &si.StartupInfo, &mut pi), }; - // Holding a token is not the same as being allowed to assign it: without - // SE_ASSIGNPRIMARYTOKEN/SE_INCREASE_QUOTA, `CreateProcessAsUserW` fails - // even though a plain `CreateProcessW` in this session would work. Under - // `CPUS_CREDUI` that is still the right outcome, so retry rather than - // failing the whole flow. Never reached for the real logon scenarios. + // Belt and braces on top of the privilege-enabling above. Under + // `CPUS_CREDUI` the fallback is the right outcome anyway, so retry + // rather than failing outright if this still comes up. if let Err(e) = spawned.as_ref() && token.is_some() && may_launch_in_current_session(cpus) diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index 4ed50f6a..bee325a1 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -384,19 +384,17 @@ fn lsa_unicode_string(wide: &[u16]) -> LSA_UNICODE_STRING { } } -/// `SeTcbPrivilege` is in SYSTEM's token, but — like every privilege that -/// isn't `SE_PRIVILEGE_ENABLED_BY_DEFAULT` — held disabled until asked for. -/// `LsaRegisterLogonProcess` checks it as active, not merely present, and -/// without this first it fails the ALPC connect outright with -/// `STATUS_PORT_CONNECTION_REFUSED` before ever reaching a logon check. -fn enable_tcb_privilege() -> windows::core::Result<()> { +/// SYSTEM's token holds every privilege this file needs, but — like any +/// privilege not `SE_PRIVILEGE_ENABLED_BY_DEFAULT` — disabled until asked +/// for; the APIs that need one check it as active, not merely present. +pub fn enable_privilege(name: PCWSTR) -> windows::core::Result<()> { unsafe { let mut token = HANDLE::default(); OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES, &mut token)?; let mut luid = LUID::default(); let result = (|| { - LookupPrivilegeValueW(PCWSTR::null(), SE_TCB_NAME, &mut luid)?; + LookupPrivilegeValueW(PCWSTR::null(), name, &mut luid)?; let privileges = TOKEN_PRIVILEGES { PrivilegeCount: 1, Privileges: [LUID_AND_ATTRIBUTES { @@ -427,7 +425,7 @@ fn enable_tcb_privilege() -> windows::core::Result<()> { /// does not deny. pub fn service_account_token(username: &str) -> windows::core::Result { unsafe { - enable_tcb_privilege()?; + enable_privilege(SE_TCB_NAME)?; let process_name = lsa_string(b"ak_cred_provider\0"); let mut lsa_handle = HANDLE::default(); From 1724d858466148cc28931ec3363603a209590cee Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Tue, 18 Aug 2026 15:05:35 +0100 Subject: [PATCH 16/41] ee/wcp: log which privilege failed and who we're actually running as PsExec -s confirms SeTcbPrivilege is Enabled for SYSTEM on the test box, contradicting a real ERROR_NOT_ALL_ASSIGNED failure inside service_account_token -- the deployed process's token evidently isn't the plain SYSTEM token that gives you, and the previous error message gave no way to tell a genuinely-missing privilege from an unexpected identity. enable_privilege now takes a display name and logs it, plus the account current_token_identity() resolves via GetTokenInformation(TokenUser)/LookupAccountSidW, on the ERROR_NOT_ALL_ASSIGNED path. --- ee/wcp/credprovider/src/ipc.rs | 9 +++- ee/wcp/credprovider/src/syscalls.rs | 76 ++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 1bc47cc9..81a910d8 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -460,10 +460,15 @@ fn spawn_cef_host( // Disabled by default like `SE_TCB_NAME` before it; without // enabling them, `CreateProcessAsUserW` fails with // `ERROR_NOT_ALL_ASSIGNED` even though `token` is valid. - if let Err(e) = syscalls::enable_privilege(SE_ASSIGNPRIMARYTOKEN_NAME) { + if let Err(e) = syscalls::enable_privilege( + SE_ASSIGNPRIMARYTOKEN_NAME, + "SeAssignPrimaryTokenPrivilege", + ) { log::warn!("could not enable SeAssignPrimaryTokenPrivilege: {e}"); } - if let Err(e) = syscalls::enable_privilege(SE_INCREASE_QUOTA_NAME) { + if let Err(e) = + syscalls::enable_privilege(SE_INCREASE_QUOTA_NAME, "SeIncreaseQuotaPrivilege") + { log::warn!("could not enable SeIncreaseQuotaPrivilege: {e}"); } CreateProcessAsUserW( diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index bee325a1..b0dd71cf 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -31,10 +31,11 @@ use windows::{ SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, }, - DACL_SECURITY_INFORMATION, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, - LUID_AND_ATTRIBUTES, LogonUserW, LookupAccountNameW, LookupPrivilegeValueW, - NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, QUOTA_LIMITS, SE_PRIVILEGE_ENABLED, - SE_TCB_NAME, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, TOKEN_PRIVILEGES, TOKEN_SOURCE, + DACL_SECURITY_INFORMATION, GetTokenInformation, LOGON32_LOGON_NETWORK, + LOGON32_PROVIDER_DEFAULT, LUID_AND_ATTRIBUTES, LogonUserW, LookupAccountNameW, + LookupAccountSidW, LookupPrivilegeValueW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, + QUOTA_LIMITS, SE_PRIVILEGE_ENABLED, SE_TCB_NAME, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, + TOKEN_PRIVILEGES, TOKEN_QUERY, TOKEN_SOURCE, TOKEN_USER, TokenUser, }, Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, System::StationsAndDesktops::{ @@ -384,10 +385,69 @@ fn lsa_unicode_string(wide: &[u16]) -> LSA_UNICODE_STRING { } } +/// Best-effort "who are we actually running as" for the log line right +/// before a privilege-enable failure. A privilege can be missing either +/// because policy genuinely denies it to this account, or because the +/// token isn't the SYSTEM token this whole design assumes it is — those +/// need different fixes, and the log line otherwise can't tell them apart. +fn current_token_identity() -> String { + unsafe { + let mut token = HANDLE::default(); + if OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token).is_err() { + return "".to_string(); + } + + let mut buf = [0u8; 256]; + let mut ret_len = 0u32; + let got_user = GetTokenInformation( + token, + TokenUser, + Some(buf.as_mut_ptr() as *mut c_void), + buf.len() as u32, + &mut ret_len, + ); + + let identity = if got_user.is_ok() { + let sid = (*(buf.as_ptr() as *const TOKEN_USER)).User.Sid; + let mut name = [0u16; 256]; + let mut name_len = name.len() as u32; + let mut domain = [0u16; 256]; + let mut domain_len = domain.len() as u32; + let mut use_ = SID_NAME_USE::default(); + if LookupAccountSidW( + PCWSTR::null(), + sid, + Some(PWSTR(name.as_mut_ptr())), + &mut name_len, + Some(PWSTR(domain.as_mut_ptr())), + &mut domain_len, + &mut use_, + ) + .is_ok() + { + format!( + "{}\\{}", + String::from_utf16_lossy(&domain[..domain_len as usize]), + String::from_utf16_lossy(&name[..name_len as usize]) + ) + } else { + "".to_string() + } + } else { + "".to_string() + }; + + let _ = CloseHandle(token); + identity + } +} + /// SYSTEM's token holds every privilege this file needs, but — like any /// privilege not `SE_PRIVILEGE_ENABLED_BY_DEFAULT` — disabled until asked /// for; the APIs that need one check it as active, not merely present. -pub fn enable_privilege(name: PCWSTR) -> windows::core::Result<()> { +/// `display` is only for the log line on failure — `AdjustTokenPrivileges` +/// doesn't say which of the (here, always one) privileges it couldn't grant. +pub fn enable_privilege(name: PCWSTR, display: &str) -> windows::core::Result<()> { unsafe { let mut token = HANDLE::default(); OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES, &mut token)?; @@ -407,6 +467,10 @@ pub fn enable_privilege(name: PCWSTR) -> windows::core::Result<()> { // actually held to enable — a classic trap, and indistinguishable // from a real success without this check. if GetLastError() == ERROR_NOT_ALL_ASSIGNED { + log::error!( + "{display} is not held by this token at all (running as {})", + current_token_identity() + ); return Err(windows::core::Error::from(HRESULT::from_win32( ERROR_NOT_ALL_ASSIGNED.0, ))); @@ -425,7 +489,7 @@ pub fn enable_privilege(name: PCWSTR) -> windows::core::Result<()> { /// does not deny. pub fn service_account_token(username: &str) -> windows::core::Result { unsafe { - enable_privilege(SE_TCB_NAME)?; + enable_privilege(SE_TCB_NAME, "SeTcbPrivilege")?; let process_name = lsa_string(b"ak_cred_provider\0"); let mut lsa_handle = HANDLE::default(); From 458134d0a0af451b7fd0029112835fc1fb0c9c25 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Tue, 18 Aug 2026 21:09:46 +0100 Subject: [PATCH 17/41] ee/wcp: replace S4U with LogonUserW + CreateRestrictedToken S4U (LsaLogonUser/MSV1_0_S4U_LOGON) needed SE_TCB_NAME, and a real install proved LogonUI's own token does not hold it -- not disabled, absent, confirmed by dumping the token's account identity in the same failure. That's expected once you look at why the credential provider model has hosted providers in the separate, more sandboxed LogonUI.exe since Vista: third-party code loaded there was never meant to have TCB-level trust. Calling LsaRegisterLogonProcess directly from inside one was never going to be supported. Chromium's own credential provider (GCPW) hits the identical wall for the identical reason and has never used S4U: CreateLogonToken in chrome/credential_provider/gaiacp/gcp_utils.cc calls LogonUserW with a real password, then strips every privilege from the result via CreateRestrictedToken(DISABLE_MAX_PRIVILEGE) before using it. service_account_token now does the same. LogonUserW needs no special privilege for a non-admin account -- confirmed independently, since RealSyscalls::validate already called it successfully with no privilege-enabling for the interactive user's password check. This means the service account needs a real, stored password after all -- service_account_password is the same establish-once/validate/ reuse/change state machine as credential.rs::account_password (LOCAL_PASSWORD.md), backed by the same KeyringPasswordStore, keyed by this account's own SID. The installer grants LogonAsService... no, LogonAsBatchJob="yes" for the SeBatchLogonRight LOGON32_LOGON_BATCH needs, matching GCPW's own choice for its non-interactive case. BROWSER_PRIVILEGE.md updated throughout; the Kerberos/SPNEGO scoping question S4U used to raise no longer applies, since LogonUserW tokens carry real credentials. --- ee/wcp/BROWSER_PRIVILEGE.md | 146 +++++++++++++------ ee/wcp/credprovider/src/ipc.rs | 18 +-- ee/wcp/credprovider/src/syscalls.rs | 216 ++++++++++++++-------------- vpkg/windows/Package.wxs | 18 ++- 4 files changed, 229 insertions(+), 169 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index fb51ced4..86cd97bb 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -7,18 +7,33 @@ the Windows logon screen, and on the path that matters most it runs as ## Status Branch `ee/wcp/browser-privilege` is building Option B, the dedicated -service account. Both open decisions below are settled: integrated auth -(Kerberos/SPNEGO) is not in scope, so S4U needs no stored credential; the -`WinSta0\Winlogon` ACL grant is accepted as the cost of getting off SYSTEM. -Option A turned out to need more than this doc assumed — see the note at the -end of that section — so it stayed out of scope rather than being folded in -here. - -**Verified on a VM** against the manual checklist in `e2e/README.md`: the -tile appears, the sign-in window opens on the secure desktop, and both fresh -logon and unlock complete. So the two things CI cannot reach — the -`WinSta0\Winlogon` ACL grant actually holding, and the S4U service-account -token being usable for both scenarios — hold outside a test harness. +service account. The `WinSta0\Winlogon` ACL grant is accepted as the cost of +getting off SYSTEM. Option A turned out to need more than this doc assumed — +see the note at the end of that section — so it stayed out of scope rather +than being folded in here. + +**S4U turned out not to work and was replaced.** The first version of this +branch minted the service account's token via `LsaLogonUser`/ +`MSV1_0_S4U_LOGON`, on the assumption that LogonUI's own token carries +`SE_TCB_NAME` (see "Getting a token" below for what actually happens). A real +install on a Windows Server test box proved that assumption wrong: +`LsaRegisterLogonProcess` failed outright, and enabling the privilege +first — which should not have been necessary, and was itself a sign +something was off — surfaced that `SE_TCB_NAME` is not held by that token at +all, confirmed by dumping the token's own account identity in the same log +line. Chromium's own credential provider hits the identical wall for the +identical reason and has never used S4U to begin with; see "Getting a token" +below. The service account now gets its token the same way GCPW does: +`LogonUserW` with a stored password, then `CreateRestrictedToken` to strip +every privilege before the token is ever used. + +**Verified on a VM** (before the S4U replacement) against the manual +checklist in `e2e/README.md`: the tile appeared, the sign-in window opened on +the secure desktop, and both fresh logon and unlock completed. That VM run +predates the S4U failure above and does not carry over to the current +`LogonUserW`-based code — the `WinSta0\Winlogon` ACL grant holding is still +good evidence, since that part is unchanged, but the token-acquisition path +it exercised no longer exists. Needs a fresh VM run. This file previously lived on `ee/wcp/rs-cef-fresh` and was deleted there in a "cleanup" commit an hour after being written, along with three sibling @@ -60,11 +75,11 @@ So the old token machinery bought nothing on fresh logon (it worked hard to arrive at the same SYSTEM the C++ got for free) and was broken on unlock — not load-bearing for the sign-in flow, an unfinished attempt at this document's goal. Replaced entirely: `credprovider::syscalls:: -service_account_token` now mints an S4U token for the dedicated account -(`SERVICE_ACCOUNT_NAME`, see Option B below) for both `CPUS_LOGON` and -`CPUS_UNLOCK_WORKSTATION` — one path, no per-scenario branching, no -`WTSQueryUserToken`/winlogon-scanning left in `syscalls.rs` at all. -`CPUS_CREDUI` is unchanged. +service_account_token` now logs the dedicated account +(`SERVICE_ACCOUNT_NAME`, see Option B below) on with `LogonUserW` and its +stored password for both `CPUS_LOGON` and `CPUS_UNLOCK_WORKSTATION` — one +path, no per-scenario branching, no `WTSQueryUserToken`/winlogon-scanning +left in `syscalls.rs` at all. `CPUS_CREDUI` is unchanged. ## Option A — enable the CEF sandbox @@ -115,36 +130,74 @@ deleted rather than fixed. `vpkg/windows/authentik Agent Installer.wixproj` and `Package.wxs` already uses the `util:` namespace, so `` needs no new dependency. -### Getting a token without storing a password — decided +### Getting a token — S4U looked free, wasn't `` wants a password and `LogonUser` would want it back later, which -means a secret at rest and a rotation story. - -`LsaLogonUser` with `MSV1_0_S4U_LOGON` avoids that entirely: it mints a token -for a local account with no credentials, given `SE_TCB_NAME`, which LogonUI -already holds. Nothing to store, nothing to rotate. - -One correction against the plan as first written here: WiX's `util:User` has -no `GeneratePassword` attribute — it only takes a literal `Password` (or one -read from a `Property` via `PasswordAttribute`), so the installer cannot mint -a random one itself. The account is created with a fixed placeholder -instead, and `credprovider::syscalls::ensure_service_account_password_rotated` -resets it to a random value the first time the DLL loads after install, via -`NetUserSetInfo` through the existing `LocalAccountPassword::reset` — the -same call the interactive user's own account uses for its first-use/ -out-of-band-change reset — then never touches it again (tracked by an -`HKLM` marker, not the credential-manager vault that account's password -lives in, since nothing here ever needs this one back). The placeholder is -live for, at most, the gap between install finishing and the first logon -attempt; the account is also denied interactive/network/RDP logon -throughout, so even a known placeholder cannot be used to sign anyone in. - -**The catch that made this a decision:** an S4U token carries no network -credentials. That is fine for reaching authentik over HTTPS with the -`X-Authentik-Platform-Auth-DTH` bearer header. It would not be fine if the -sign-in flow ever chained to an IdP doing Kerberos/SPNEGO — authentik -supports that, and in an AD environment it would be plausible. **Confirmed -out of scope for this account**, so S4U stands. +means a secret at rest and a rotation story. `LsaLogonUser` with +`MSV1_0_S4U_LOGON` looked like it avoided that entirely: mint a token for a +local account with no credentials, given `SE_TCB_NAME`, which the doc assumed +LogonUI already holds. That assumption is wrong, and the branch's history +still has the wreckage: `LsaRegisterLogonProcess` failed with +`STATUS_PORT_CONNECTION_REFUSED` on a real install. Explicitly enabling +`SE_TCB_NAME` first (`AdjustTokenPrivileges`) made that failure go away — +which itself should have been the warning sign, since a token that legitimately +holds a privilege does not usually need it force-enabled — and once the code +started checking `AdjustTokenPrivileges`'s own success/failure properly +(`ERROR_NOT_ALL_ASSIGNED` is reported as success by the underlying Win32 call; +only `GetLastError` after the fact tells you it lied), the real answer came +back: `SE_TCB_NAME` is not held by LogonUI's token *at all*, on a real +install, full stop. Not disabled — absent. `PsExec -s whoami /priv` on the +same box shows SYSTEM holding it fine; the credential-provider-hosting +process's token is evidently not that same generic SYSTEM token. + +In hindsight this is exactly what the credential-provider model since Vista +is *for*: `LogonUI.exe` is a separate, deliberately more sandboxed process +from `winlogon.exe`/LSASS precisely so that third-party code loaded into it — +credential providers — does not get TCB-level trust. The built-in providers +never call LSA logon APIs directly; they hand LSA a serialized credential +blob via `GetSerialization` and let LSASS, which does have TCB, do the +privileged part (this codebase already does exactly that for the +interactive user's own logon — see `LOCAL_PASSWORD.md`, +`pack_kerb_interactive_unlock_logon`). Calling `LsaRegisterLogonProcess` +directly from inside a credential provider was never going to be supported. + +Chromium's own credential provider (Google Credential Provider for Windows, +GCPW) solves the identical problem — a Chromium-based sign-in UI hosted in +LogonUI, needing a token for a helper identity — and has never used S4U or +any LSA-direct call for it. `CreateLogonToken` in +`chrome/credential_provider/gaiacp/gcp_utils.cc` calls plain `LogonUserW` +with a real password (`LOGON32_LOGON_BATCH` for the non-interactive case), +then wraps the result in `CreateRestrictedToken(..., DISABLE_MAX_PRIVILEGE, +...)` before using it — stripping every privilege from the token rather than +relying on the account having few to begin with. `service_account_token` +now does exactly this. `LogonUserW` needs no special privilege for a +non-admin account (confirmed independently: `RealSyscalls::validate` already +called it successfully for the interactive user's password check, with no +privilege-enabling, before any of this). + +This puts the service account's password handling on the same footing as +the interactive user's: a real secret has to exist and be kept somewhere. +`credprovider::syscalls::service_account_password` is the same state +machine as `credential.rs::account_password` (`LOCAL_PASSWORD.md`) — +established once, validated and reused on every subsequent call, changed +rather than reset once known — stored in the same `KeyringPasswordStore` +vault, keyed by this account's own SID instead of a signed-in user's. WiX's +`util:User` has no `GeneratePassword` attribute — it only takes a literal +`Password` (or one read from a `Property` via `PasswordAttribute`) — so the +installer creates the account with a fixed placeholder, and the first call +to `service_account_password` resets it to a real, stored value. `LogonAsBatchJob="yes"` +on `` grants the `SeBatchLogonRight` that `LOGON32_LOGON_BATCH` +needs; the account is still denied interactive/network/RDP logon throughout, +and the *token `ak_cef.exe` actually runs with* has every privilege stripped +by `CreateRestrictedToken` regardless of what the logon itself produced. + +**The Kerberos/SPNEGO question this used to raise for S4U does not apply +here.** `LogonUserW` is a real password-based logon; the resulting token (net +of `CreateRestrictedToken`'s privilege stripping, which is unrelated to +network credentials) carries the account's actual credentials the same way +any normal interactive or batch logon does. Nothing about this design rules +out integrated auth for this account if it were ever needed — the question +in the original "Decisions" list is moot, not settled in either direction. ### The rest of the work @@ -181,7 +234,8 @@ out of scope for this account**, so S4U stands. ## Decisions — resolved for this branch 1. ~~Is integrated auth (Kerberos/SPNEGO to an upstream IdP) ever in scope?~~ - **No.** S4U stands; no stored credential for this account. + **Moot.** This account no longer uses S4U, so the concern that raised this + question does not apply — see "Getting a token" above. 2. ~~Is granting a non-SYSTEM account access to `WinSta0\Winlogon` acceptable?~~ **Yes**, scoped to this one account only. 3. Does the browser process need to be non-SYSTEM at all once renderers are diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 81a910d8..603769eb 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -354,15 +354,15 @@ fn signal_cancel(cancel_write: HANDLE) { /// Gets `ak_cef.exe` a token for the dedicated service account rather than /// SYSTEM (`BROWSER_PRIVILEGE.md`), the same way for both logon and unlock. -/// Password rotation and account-hardening are best-effort and only logged -/// on failure — each is idempotent, so a transient failure just costs a -/// retry next time, and none of them block the token mint that follows. +/// Account-hardening is best-effort and only logged on failure — it is +/// idempotent, so a transient failure just costs a retry next time, and +/// does not block the token mint that follows. The password is not: a +/// broken keyring here means no way to log the account on at all. fn acquire_service_account_token() -> windows::core::Result { - if let Err(e) = - syscalls::ensure_service_account_password_rotated(syscalls::SERVICE_ACCOUNT_NAME) - { - log::warn!("could not rotate the service account's password: {e}"); - } + let password = syscalls::service_account_password().map_err(|e| { + log::error!("could not establish the service account's password: {e}"); + windows::core::Error::from(E_FAIL) + })?; let sid = syscalls::account_sid(syscalls::SERVICE_ACCOUNT_NAME)?; @@ -373,7 +373,7 @@ fn acquire_service_account_token() -> windows::core::Result { log::warn!("could not grant the service account secure-desktop access: {e}"); } - syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME) + syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME, &password) } fn spawn_cef_host( diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index b0dd71cf..ce124786 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -18,24 +18,23 @@ use windows::{ }, NetworkManagement::NetManagement::{NetUserChangePassword, NetUserSetInfo, USER_INFO_1003}, Security::{ - ACL, AdjustTokenPrivileges, AllocateLocallyUniqueId, + ACL, AdjustTokenPrivileges, Authentication::Identity::{ LSA_HANDLE, LSA_OBJECT_ATTRIBUTES, LSA_STRING, LSA_UNICODE_STRING, LsaAddAccountRights, LsaClose, LsaConnectUntrusted, LsaDeregisterLogonProcess, - LsaFreeReturnBuffer, LsaLogonUser, LsaLookupAuthenticationPackage, LsaOpenPolicy, - LsaRegisterLogonProcess, MSV1_0_S4U_LOGON, MsV1_0S4ULogon, POLICY_CREATE_ACCOUNT, - SECURITY_LOGON_TYPE, + LsaLookupAuthenticationPackage, LsaOpenPolicy, POLICY_CREATE_ACCOUNT, }, Authorization::{ - EXPLICIT_ACCESS_W, GetSecurityInfo, NO_MULTIPLE_TRUSTEE, SE_OBJECT_TYPE, - SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, TRUSTEE_IS_SID, - TRUSTEE_IS_USER, TRUSTEE_W, + ConvertSidToStringSidW, EXPLICIT_ACCESS_W, GetSecurityInfo, NO_MULTIPLE_TRUSTEE, + SE_OBJECT_TYPE, SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, + TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, }, - DACL_SECURITY_INFORMATION, GetTokenInformation, LOGON32_LOGON_NETWORK, + CreateRestrictedToken, DACL_SECURITY_INFORMATION, DISABLE_MAX_PRIVILEGE, + GetTokenInformation, LOGON32_LOGON_BATCH, LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, LUID_AND_ATTRIBUTES, LogonUserW, LookupAccountNameW, LookupAccountSidW, LookupPrivilegeValueW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, - QUOTA_LIMITS, SE_PRIVILEGE_ENABLED, SE_TCB_NAME, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, - TOKEN_PRIVILEGES, TOKEN_QUERY, TOKEN_SOURCE, TOKEN_USER, TokenUser, + SE_PRIVILEGE_ENABLED, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, TOKEN_PRIVILEGES, + TOKEN_QUERY, TOKEN_USER, TokenUser, }, Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, System::StationsAndDesktops::{ @@ -47,9 +46,6 @@ use windows::{ }; use windows_core::BOOL; -const SERVICE_ACCOUNT_STATE_KEY: &str = - "SOFTWARE\\authentik Security Inc.\\Platform\\WcpServiceAccount"; - pub trait AuthPackageLookup { fn negotiate_package(&self) -> windows::core::Result; } @@ -345,10 +341,6 @@ impl PasswordStore for KeyringPasswordStore { /// `util:User`) — keep this in step with that element's `Name` attribute. pub const SERVICE_ACCOUNT_NAME: &str = "ak-wcp-browser"; -const TOKEN_SOURCE_NAME: [i8; 8] = [ - b'A' as i8, b'k' as i8, b'W' as i8, b'c' as i8, b'p' as i8, b'S' as i8, b'4' as i8, b'U' as i8, -]; - /// Resolves the service account's name to a SID: both `LsaAddAccountRights` /// and the desktop ACL grant below want one, and a name is all the installer /// leaves behind. @@ -483,87 +475,64 @@ pub fn enable_privilege(name: PCWSTR, display: &str) -> windows::core::Result<() } } -/// Mints a primary token for the service account via an S4U logon — no -/// password needed, only `SE_TCB_NAME`. `Service` is the logon type -/// deliberately: it is the one type `deny_interactive_and_network_logon` -/// does not deny. -pub fn service_account_token(username: &str) -> windows::core::Result { - unsafe { - enable_privilege(SE_TCB_NAME, "SeTcbPrivilege")?; +/// Mints a primary token for the service account by logging it on with its +/// stored password, then strips every privilege from the result. S4U +/// (`LsaLogonUser`) needs `SE_TCB_NAME`, and LogonUI's token does not carry +/// it — Google's own credential provider hits the same wall for the same +/// reason (a credential provider hosted in LogonUI, rendering its own +/// Chromium-based sign-in UI) and answers it the same way: see +/// `CreateLogonToken` in +/// `chrome/credential_provider/gaiacp/gcp_utils.cc` in the Chromium source. +/// Password-based `LogonUserW` needs no special privilege for a non-admin +/// account, and `CreateRestrictedToken` with `DISABLE_MAX_PRIVILEGE` is what +/// keeps a compromised renderer from being able to do anything with a token +/// that would otherwise still be a fully-privileged one for its account. +pub fn service_account_token(username: &str, password: &str) -> windows::core::Result { + let username_wide = wide(username); + let password_wide = wide(password); - let process_name = lsa_string(b"ak_cred_provider\0"); - let mut lsa_handle = HANDLE::default(); - let mut security_mode = 0u32; - LsaRegisterLogonProcess(&process_name, &mut lsa_handle, &mut security_mode).ok()?; + let mut primary = HANDLE::default(); + unsafe { + LogonUserW( + PCWSTR(username_wide.as_ptr()), + w!("."), + PCWSTR(password_wide.as_ptr()), + LOGON32_LOGON_BATCH, + LOGON32_PROVIDER_DEFAULT, + &mut primary, + )?; + } - let result = s4u_logon(lsa_handle, username); - let _ = LsaDeregisterLogonProcess(lsa_handle); - result + let mut restricted = HANDLE::default(); + let result = unsafe { + CreateRestrictedToken( + primary, + DISABLE_MAX_PRIVILEGE, + None, + None, + None, + &mut restricted, + ) + }; + unsafe { + let _ = CloseHandle(primary); } + result?; + Ok(restricted) } -unsafe fn s4u_logon(lsa_handle: HANDLE, username: &str) -> windows::core::Result { +/// String form of a SID, for the keyring's `sid` key. `account_sid` returns +/// raw bytes for the Win32 calls that want a `PSID`; the keyring store +/// (like the interactive user's, `credential.rs`) is keyed by the string +/// form instead. +fn sid_to_string(sid: &[u8]) -> windows::core::Result { unsafe { - let package_name = lsa_string(b"MICROSOFT_AUTHENTICATION_PACKAGE_V1_0\0"); - let mut auth_package = 0u32; - LsaLookupAuthenticationPackage(lsa_handle, &package_name, &mut auth_package).ok()?; - - // S4U wants these as `LSA_UNICODE_STRING`s with no trailing NUL, - // unlike the `LSA_STRING`s above — keep the backing buffers alive - // for the whole call, `lsa_unicode_string` only borrows them. - let username_wide: Vec = username.encode_utf16().collect(); - let domain_wide: Vec = ".".encode_utf16().collect(); - let s4u = MSV1_0_S4U_LOGON { - MessageType: MsV1_0S4ULogon, - Flags: 0, - UserPrincipalName: lsa_unicode_string(&username_wide), - DomainName: lsa_unicode_string(&domain_wide), - }; - - let origin_name = lsa_string(b"ak_cred_provider\0"); - let mut token_source = TOKEN_SOURCE { - SourceName: TOKEN_SOURCE_NAME, - ..Default::default() - }; - AllocateLocallyUniqueId(&mut token_source.SourceIdentifier)?; - - let mut profile_buffer: *mut std::ffi::c_void = std::ptr::null_mut(); - let mut profile_buffer_len = 0u32; - let mut logon_id = LUID::default(); - let mut token = HANDLE::default(); - let mut quotas = QUOTA_LIMITS::default(); - let mut sub_status = 0i32; - - let status = LsaLogonUser( - lsa_handle, - &origin_name, - SECURITY_LOGON_TYPE::Service, - auth_package, - &s4u as *const MSV1_0_S4U_LOGON as *const std::ffi::c_void, - std::mem::size_of::() as u32, - None, - &token_source, - &mut profile_buffer, - &mut profile_buffer_len, - &mut logon_id, - &mut token, - &mut quotas, - &mut sub_status, - ); - - if !profile_buffer.is_null() { - let _ = LsaFreeReturnBuffer(profile_buffer); - } - - // As with `negotiate_package`, `NTSTATUS::ok()` is only a sign test; - // `sub_status` carries the more specific reason (e.g. an account - // that does not exist yet because the installer has not run) and is - // only meaningful once `status` itself is an error. - if status.0 != 0 { - log::error!("S4U logon for {username} failed: {status:?} (substatus {sub_status:#x})"); - return Err(status.to_hresult().into()); - } - Ok(token) + let mut wide_sid = PWSTR(std::ptr::null_mut()); + ConvertSidToStringSidW(PSID(sid.as_ptr() as *mut _), &mut wide_sid)?; + let len = (0..).take_while(|&i| *wide_sid.0.add(i) != 0).count(); + let result = String::from_utf16_lossy(std::slice::from_raw_parts(wide_sid.0, len)); + let _ = LocalFree(Some(HLOCAL(wide_sid.0 as *mut c_void))); + Ok(result) } } @@ -681,30 +650,58 @@ pub fn deny_interactive_and_network_logon(sid: &[u8]) -> windows::core::Result<( } } -/// Rotates the service account's password to a random value the first time -/// this runs, then never again — nothing needs it back, since -/// `service_account_token` mints tokens via S4U, not `LogonUserW`. Turns the -/// installer's fixed placeholder password into a real secret exactly once; -/// the `HKLM` marker is what stops it happening again on every logon. -pub fn ensure_service_account_password_rotated(username: &str) -> eyre::Result<()> { - let hklm = winreg::RegKey::predef(winreg::enums::HKEY_LOCAL_MACHINE); - let (key, _disp) = hklm.create_subkey(SERVICE_ACCOUNT_STATE_KEY)?; - - if key.get_value::("PasswordRotated").unwrap_or(0) == 1 { - return Ok(()); +/// The service account's password: established once and reused after that, +/// same state machine and same reasoning as the interactive user's own +/// account (`credential.rs::account_password`, `LOCAL_PASSWORD.md`) — an +/// administrative reset orphans the DPAPI master key, so once a password is +/// known, only `change` touches the account again. That reasoning is about +/// DPAPI survival, which does not apply to an account nothing ever signs +/// into interactively; kept anyway; there is no reason to churn the account +/// on every logon when reuse costs nothing. +pub fn service_account_password() -> eyre::Result { + let sid = account_sid(SERVICE_ACCOUNT_NAME).map_err(|e| eyre::eyre!("{e}"))?; + let sid = sid_to_string(&sid).map_err(|e| eyre::eyre!("{e}"))?; + let store = KeyringPasswordStore::new(); + + if let Some(stored) = store.load(&sid)? { + match RealSyscalls.validate(SERVICE_ACCOUNT_NAME, &stored) { + Ok(PasswordCheck::Valid) => return Ok(stored), + Ok(PasswordCheck::Expired) => { + let new = + crate::helpers::generate_random_password().map_err(|e| eyre::eyre!("{e}"))?; + if RealSyscalls + .change(SERVICE_ACCOUNT_NAME, &stored, &new) + .is_ok() + { + let _ = store.save(&sid, &new); + return Ok(new); + } + } + // Changed out of band; fall through to a reset. + Ok(PasswordCheck::Rejected) => {} + // Inconclusive, not wrong — see credential.rs::stored_password. + Err(e) => { + log::warn!( + "could not verify the service account's stored password ({e}); using it anyway" + ); + return Ok(stored); + } + } } let password = crate::helpers::generate_random_password().map_err(|e| eyre::eyre!("{e}"))?; RealSyscalls - .reset(username, &password) + .reset(SERVICE_ACCOUNT_NAME, &password) .map_err(|e| eyre::eyre!("{e}"))?; - key.set_value("PasswordRotated", &1u32)?; - Ok(()) + if let Err(e) = store.save(&sid, &password) { + log::error!("could not store the service account's password: {e}"); + } + Ok(password) } #[cfg(test)] #[allow(clippy::unwrap_used)] -mod s4u_tests { +mod byte_layout_tests { use super::*; /// `MaximumLength` must cover the trailing NUL while `Length` excludes @@ -719,8 +716,9 @@ mod s4u_tests { assert_eq!(bytes, b"Negotiate"); } - /// Unlike `LSA_STRING`, S4U's `LSA_UNICODE_STRING`s carry no NUL at all — - /// `Length`/`MaximumLength` are both the exact UTF-16 byte count. + /// Unlike `LSA_STRING`, `deny_interactive_and_network_logon`'s rights + /// list carries no NUL at all — `Length`/`MaximumLength` are both the + /// exact UTF-16 byte count. #[test] fn lsa_unicode_string_round_trips_without_a_nul() { let wide: Vec = "ak-wcp-browser".encode_utf16().collect(); diff --git a/vpkg/windows/Package.wxs b/vpkg/windows/Package.wxs index 26ebacf4..8288fbf1 100644 --- a/vpkg/windows/Package.wxs +++ b/vpkg/windows/Package.wxs @@ -127,11 +127,18 @@ EventMessageFile="[#wcp_ak_cef_exe]"/> + + + From e19fe5697764b34fa87b8d638af26adda266d744 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Wed, 19 Aug 2026 22:05:29 +0100 Subject: [PATCH 28/41] ee/wcp: log the result of the exact CreateMutex call ProcessSingleton makes handle.exe confirmed no stale Local\ChromeProcessSingletonStartup! mutex exists between attempts, ruling out a wrong-DACL'd leftover object - but that only tells us nothing PERSISTS, not why a fresh CreateMutex might fail. Chromium's own diagnostic for this specific failure is DPLOG(FATAL), which compiles to nothing in a release build, and Process Monitor doesn't capture mutex operations at all. Ask the same question ourselves, through ak_platform::log (Windows Event Log), which is confirmed working, instead of continuing to reason about internals we can't directly observe. Co-Authored-By: Claude Sonnet 5 --- ee/wcp/cef-host/src/main.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index aa2c1f79..9007e711 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -38,6 +38,30 @@ const CACHE_ROOT: &str = r"C:\ProgramData\Authentik Security Inc\wcp-cache"; /// sees a message Chromium raises below the Rust layer. const CHROMIUM_LOG_PATH: &str = r"C:\ProgramData\Authentik Security Inc\logs\ak_cef_chromium.log"; +/// Asks the exact question CEF's own `ProcessSingleton::Create()` asks — +/// `::CreateMutex(NULL, FALSE, "Local\ChromeProcessSingletonStartup!")` — +/// through a logging channel already confirmed to work, rather than relying +/// on Chromium's own diagnostic for it: that failure is logged only via +/// `DPLOG(FATAL)`, which compiles to nothing in a release build, so it would +/// stay silent regardless of log verbosity. +fn diagnose_process_singleton_mutex() { + use windows::Win32::Foundation::CloseHandle; + use windows::Win32::System::Threading::CreateMutexW; + use windows::core::w; + + match unsafe { CreateMutexW(None, false, w!("Local\\ChromeProcessSingletonStartup!")) } { + Ok(handle) => { + log::info!("diagnostic: CreateMutex(ChromeProcessSingletonStartup!) succeeded"); + unsafe { + let _ = CloseHandle(handle); + } + } + Err(e) => { + log::error!("diagnostic: CreateMutex(ChromeProcessSingletonStartup!) failed: {e}") + } + } +} + fn arg_value(flag: &str) -> Option { let mut args = std::env::args(); while let Some(a) = args.next() { @@ -140,6 +164,8 @@ fn main() { }; let cancel_pipe = arg_value("--cancel-pipe"); + diagnose_process_singleton_mutex(); + let cache_path = browser_state_dir(Path::new(CACHE_ROOT)); let settings = Settings { From d63ca1a27d3480cfb1d7df458b481a8b3a734738 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 00:44:16 +0100 Subject: [PATCH 29/41] ee/wcp: expand the CreateMutex diagnostic and log the token's own identity Confirmed against a real install: CreateMutex(Local\ChromeProcessSingletonStartup!) fails with ERROR_ACCESS_DENIED on a fresh create (handle.exe found nothing pre-existing, ruling out a stale wrong-owner object). Three follow-up CreateMutex calls narrow down why: a random name under Local\ tells apart "this token can't create named mutexes in the session namespace at all" from "this specific, publicly-documented string is blocked" (endpoint security software is the leading suspect for the latter); a Global\ name checks whether it's specific to the per-session namespace; a Chrome-ish but not-exact name checks whether the block matches the literal string or the product name more loosely. Also logs the token's mandatory integrity level, session id, and whether Windows considers it IsTokenRestricted, to rule in or out the double access-check restricted tokens get (service_account_token only passes DISABLE_MAX_PRIVILEGE, no SIDs to disable/restrict, so this is expected to read false - confirming that removes one more hypothesis either way). Co-Authored-By: Claude Sonnet 5 --- ee/wcp/cef-host/src/main.rs | 138 ++++++++++++++++++++++++++++++++---- 1 file changed, 125 insertions(+), 13 deletions(-) diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index 9007e711..cbde3446 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -38,27 +38,138 @@ const CACHE_ROOT: &str = r"C:\ProgramData\Authentik Security Inc\wcp-cache"; /// sees a message Chromium raises below the Rust layer. const CHROMIUM_LOG_PATH: &str = r"C:\ProgramData\Authentik Security Inc\logs\ak_cef_chromium.log"; -/// Asks the exact question CEF's own `ProcessSingleton::Create()` asks — -/// `::CreateMutex(NULL, FALSE, "Local\ChromeProcessSingletonStartup!")` — -/// through a logging channel already confirmed to work, rather than relying -/// on Chromium's own diagnostic for it: that failure is logged only via -/// `DPLOG(FATAL)`, which compiles to nothing in a release build, so it would -/// stay silent regardless of log verbosity. -fn diagnose_process_singleton_mutex() { +/// `::CreateMutex(NULL, FALSE, name)`, logging the result through a channel +/// already confirmed to work — Chromium's own diagnostic for this exact call +/// (inside `ProcessSingleton::Create()`) is `DPLOG(FATAL)`, which compiles to +/// nothing in a release build, so it stays silent regardless of log +/// verbosity. +fn try_create_mutex(label: &str, name: &str) { use windows::Win32::Foundation::CloseHandle; use windows::Win32::System::Threading::CreateMutexW; - use windows::core::w; + use windows::core::PCWSTR; - match unsafe { CreateMutexW(None, false, w!("Local\\ChromeProcessSingletonStartup!")) } { + let name_wide: Vec = name.encode_utf16().chain(std::iter::once(0)).collect(); + match unsafe { CreateMutexW(None, false, PCWSTR(name_wide.as_ptr())) } { Ok(handle) => { - log::info!("diagnostic: CreateMutex(ChromeProcessSingletonStartup!) succeeded"); + log::info!("diagnostic: CreateMutex({label}) succeeded"); unsafe { let _ = CloseHandle(handle); } } - Err(e) => { - log::error!("diagnostic: CreateMutex(ChromeProcessSingletonStartup!) failed: {e}") - } + Err(e) => log::error!("diagnostic: CreateMutex({label}) failed: {e}"), + } +} + +/// The exact call CEF's `ProcessSingleton::Create()` makes, confirmed +/// failing with `ERROR_ACCESS_DENIED` against a real install, on a fresh +/// create with nothing pre-existing (`handle.exe` found no stale object). +/// The three follow-up calls narrow down why: a random name under the same +/// `Local\` session namespace tells apart "this token cannot create named +/// mutexes in the session namespace at all" from "this specific, +/// publicly-documented name is blocked" (endpoint security software is the +/// leading suspect for the latter — it is exactly the kind of string a +/// malware/injection heuristic keys on); a `Global\` name checks whether the +/// restriction is specific to the per-session namespace; a name containing +/// "Chrome" but not the exact reserved string checks whether the block +/// matches on the literal known string or more loosely on the product name. +fn diagnose_process_singleton_mutex() { + try_create_mutex( + "ChromeProcessSingletonStartup!", + "Local\\ChromeProcessSingletonStartup!", + ); + try_create_mutex( + "random, Local", + &format!("Local\\ak-cef-diagnostic-{}", uuid::Uuid::new_v4()), + ); + try_create_mutex( + "random, Global", + &format!("Global\\ak-cef-diagnostic-{}", uuid::Uuid::new_v4()), + ); + try_create_mutex( + "Chrome-ish but not the reserved string", + &format!("Local\\ChromeDiagnosticProbe-{}", uuid::Uuid::new_v4()), + ); +} + +/// Logs this process's own token identity, mandatory integrity level, +/// session id, and whether Windows considers it a *restricted* token +/// (`IsTokenRestricted`, true only if `CreateRestrictedToken` was given SIDs +/// to disable or restrict — `service_account_token` gives it neither, only +/// `DISABLE_MAX_PRIVILEGE`, so this is expected to read false; confirming +/// that rules out the double access-check restricted tokens get as an +/// explanation for the `CreateMutex` failure above). +fn diagnose_token() { + use std::ffi::c_void; + use windows::Win32::Foundation::{CloseHandle, HANDLE}; + use windows::Win32::Security::{ + GetSidSubAuthority, GetSidSubAuthorityCount, GetTokenInformation, IsTokenRestricted, + TOKEN_MANDATORY_LABEL, TOKEN_QUERY, TokenIntegrityLevel, TokenSessionId, + }; + use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + + let mut token = HANDLE::default(); + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) }.is_err() { + log::error!("diagnostic: could not open this process's own token"); + return; + } + + let mut label_buf = [0u8; 64]; + let mut ret_len = 0u32; + let got_label = unsafe { + GetTokenInformation( + token, + TokenIntegrityLevel, + Some(label_buf.as_mut_ptr() as *mut c_void), + label_buf.len() as u32, + &mut ret_len, + ) + }; + if got_label.is_ok() { + let label = unsafe { &*(label_buf.as_ptr() as *const TOKEN_MANDATORY_LABEL) }; + let sid = label.Label.Sid; + let rid = unsafe { + let count = *GetSidSubAuthorityCount(sid); + *GetSidSubAuthority(sid, (count - 1) as u32) + }; + let level = match rid { + 0x0000 => "Untrusted", + 0x1000 => "Low", + 0x2000 => "Medium", + 0x2100 => "Medium Plus", + 0x3000 => "High", + 0x4000 => "System", + _ => "Unknown", + }; + log::info!("diagnostic: token integrity level = {level} (rid {rid:#06x})"); + } else { + log::error!("diagnostic: could not read token integrity level"); + } + + let mut session_id: u32 = u32::MAX; + let mut ret_len2 = 0u32; + let got_session = unsafe { + GetTokenInformation( + token, + TokenSessionId, + Some(std::ptr::from_mut(&mut session_id).cast()), + size_of::() as u32, + &mut ret_len2, + ) + }; + if got_session.is_ok() { + log::info!("diagnostic: token session id = {session_id}"); + } else { + log::error!("diagnostic: could not read token session id"); + } + + // `IsTokenRestricted` reports via Result rather than a bool: Ok means the + // token carries a restricted-SIDs list (the raw BOOL was TRUE), Err means + // it does not. + let restricted = unsafe { IsTokenRestricted(token) }.is_ok(); + log::info!("diagnostic: IsTokenRestricted = {restricted}"); + + unsafe { + let _ = CloseHandle(token); } } @@ -164,6 +275,7 @@ fn main() { }; let cancel_pipe = arg_value("--cancel-pipe"); + diagnose_token(); diagnose_process_singleton_mutex(); let cache_path = browser_state_dir(Path::new(CACHE_ROOT)); From e92a4b4c62de15b7d4c9d1242e901f49559a6fef Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 01:45:59 +0100 Subject: [PATCH 30/41] ee/wcp: switch the service account from Batch to Service logon Diagnostics confirmed the root cause: a LOGON32_LOGON_BATCH token cannot create any named synchronization object (mutex/event/semaphore) in the session namespace at all. CreateMutex(Local\ChromeProcessSingletonStartup!) failed with ERROR_ACCESS_DENIED, and so did CreateMutex with random UUID names under both Local\ and Global\ - ruling out a stale wrong-owner object (handle.exe found nothing) and a name-specific block (endpoint security software was the leading suspect until random names failed identically). The same token creates named pipes, files, and registry keys fine; only synchronization objects are affected. Batch logons carry the NT AUTHORITY\BATCH well-known SID rather than INTERACTIVE, and it's a documented Windows hardening pattern (CIS/STIG baselines) to scope BaseNamedObjects creation rights to interactive/ service identities specifically, excluding batch - plausible on a baselined Windows Server test box. Switches to LOGON32_LOGON_SERVICE (SeServiceLogonRight/LogonAsService, not SeBatchLogonRight/LogonAsBatchJob). Unverified whether this alone fixes it or whether BaseNamedObjects also needs an explicit ACL grant for the account, the same way ensure_desktop_access already does for WinSta0\Winlogon. Co-Authored-By: Claude Sonnet 5 --- ee/wcp/BROWSER_PRIVILEGE.md | 71 ++++++++++++++++++++++------- ee/wcp/credprovider/src/syscalls.rs | 21 ++++++--- vpkg/windows/Package.wxs | 29 +++++++----- 3 files changed, 86 insertions(+), 35 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index d6299e4e..81f54b40 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -85,17 +85,50 @@ difference — because GCPW's "sign-in UI helper" is not a Chromium browser process at all. `ForkGaiaLogonStub` launches `rundll32.exe` reloading the credential provider's own DLL through an entrypoint; it never runs `chrome_main_delegate.cc` or `ProcessSingleton` in the first place. The -actual fix: CEF (151.x) supports both a **Chrome style** runtime (the full -Chrome UI/browser layer — extensions, profile manager, `ProcessSingleton`) -and a lighter **Alloy style** runtime built for embedding, and windowed -browsers default to Chrome style unless told otherwise. -`SignInWindowDelegate::window_runtime_style` (`window.rs`) and the new -`AlloyBrowserView::browser_runtime_style` (`app.rs`) both now return -`RuntimeStyle::ALLOY` — a Chrome style top-level `Window` can only host one -Chrome style `BrowserView`, so both had to change together or it's a style -mismatch, not a free mix. - -**Verified on a VM** (before either replacement above) against the manual +next candidate: CEF (151.x) supports both a **Chrome style** runtime (the +full Chrome UI/browser layer — extensions, profile manager, +`ProcessSingleton`) and a lighter **Alloy style** runtime built for +embedding, and windowed browsers default to Chrome style unless told +otherwise. `SignInWindowDelegate::window_runtime_style` (`window.rs`) and +`AlloyBrowserView::browser_runtime_style` (`app.rs`) now return +`RuntimeStyle::ALLOY` for exactly that reason, and it is still correct to +keep — this window genuinely needs no Chrome UI — but it turned out **not** +to be what was failing: CEF's own architecture docs (and the "Delete Alloy +bootstrap" change in M128, well before 151.x) confirm the Chrome-vs-Alloy +choice only selects a *style* layered on top of an always-Chrome +*bootstrap* — `ProcessSingleton` runs during `CefInitialize` unconditionally, +before any window or `BrowserView` (and therefore any `runtime_style`) is +ever created. + +**The real cause: `LOGON32_LOGON_BATCH` tokens cannot create named +synchronization objects in the session namespace at all.** With no more +plausible CEF-level explanation left, `ak_cef.exe` was changed to ask the +same `::CreateMutex` question `ProcessSingleton::Create()` asks, directly, +logging through `ak_platform::log` (Chromium's own diagnostic for this exact +failure is `DPLOG(FATAL)`, compiled to nothing in a release build — silent +regardless of log verbosity). Confirmed against a real install: +`CreateMutex(Local\ChromeProcessSingletonStartup!)` fails with +`ERROR_ACCESS_DENIED`, and so does `CreateMutex` with a random UUID name +under both `Local\` and `Global\` — ruling out both a stale wrong-owner +object (`handle.exe` found nothing pre-existing) and a name-specific block +(endpoint security software targeting the well-known Chrome string was the +leading suspect until the random names failed identically too). The same +token's token identity is unremarkable — Medium integrity, session 1, not +`IsTokenRestricted` — and it creates named pipes, files and registry keys +under this exact token with no issue; only named synchronization objects +(mutex/event/semaphore) fail. The distinguishing factor: `LOGON32_LOGON_BATCH` +tokens carry the `NT AUTHORITY\BATCH` well-known SID rather than +`INTERACTIVE`, and it is a documented Windows security-hardening pattern +(CIS/STIG-style baselines) to scope `BaseNamedObjects` object-creation +rights to interactive/service identities specifically, excluding batch +logons — plausible on a baselined Windows Server test box. Fixed by +switching to `LOGON32_LOGON_SERVICE` (`SeServiceLogonRight`/ +`LogonAsService="yes"` instead of `SeBatchLogonRight`/`LogonAsBatchJob="yes"`) +— unverified as of this writing whether that alone resolves it or whether +`BaseNamedObjects` also needs an explicit ACL grant for the account, the +same way `ensure_desktop_access` already does for `WinSta0\Winlogon`. + +**Verified on a VM** (before any of the replacements above) against the manual checklist in `e2e/README.md`: the tile appeared, the sign-in window opened on the secure desktop, and both fresh logon and unlock completed. That VM run predates both failures above and does not carry over to the current code — @@ -238,10 +271,14 @@ with a real password (`LOGON32_LOGON_BATCH` for the non-interactive case), then wraps the result in `CreateRestrictedToken(..., DISABLE_MAX_PRIVILEGE, ...)` before using it — stripping every privilege from the token rather than relying on the account having few to begin with. `service_account_token` -now does exactly this. `LogonUserW` needs no special privilege for a -non-admin account (confirmed independently: `RealSyscalls::validate` already -called it successfully for the interactive user's password check, with no -privilege-enabling, before any of this). +follows the same pattern but with `LOGON32_LOGON_SERVICE` instead — see +"Assigning the token" below for why BATCH specifically does not work here. +GCPW never hits this: its own sign-in UI helper is not a browser process +(see "Assigning the token"), so it never calls anything like +`ProcessSingleton` that would expose the difference. `LogonUserW` needs no +special privilege for a non-admin account (confirmed independently: +`RealSyscalls::validate` already called it successfully for the interactive +user's password check, with no privilege-enabling, before any of this). This puts the service account's password handling on the same footing as the interactive user's: a real secret has to exist and be kept somewhere. @@ -253,8 +290,8 @@ vault, keyed by this account's own SID instead of a signed-in user's. WiX's `util:User` has no `GeneratePassword` attribute — it only takes a literal `Password` (or one read from a `Property` via `PasswordAttribute`) — so the installer creates the account with a fixed placeholder, and the first call -to `service_account_password` resets it to a real, stored value. `LogonAsBatchJob="yes"` -on `` grants the `SeBatchLogonRight` that `LOGON32_LOGON_BATCH` +to `service_account_password` resets it to a real, stored value. `LogonAsService="yes"` +on `` grants the `SeServiceLogonRight` that `LOGON32_LOGON_SERVICE` needs; the account is still denied interactive/network/RDP logon throughout, and the *token `ak_cef.exe` actually runs with* has every privilege stripped by `CreateRestrictedToken` regardless of what the logon itself produced. diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index b6487c9e..f453149d 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -30,7 +30,7 @@ use windows::{ TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, }, CreateRestrictedToken, DACL_SECURITY_INFORMATION, DISABLE_MAX_PRIVILEGE, - GetTokenInformation, LOGON32_LOGON_BATCH, LOGON32_LOGON_NETWORK, + GetTokenInformation, LOGON32_LOGON_NETWORK, LOGON32_LOGON_SERVICE, LOGON32_PROVIDER_DEFAULT, LUID_AND_ATTRIBUTES, LogonUserW, LookupAccountNameW, LookupAccountSidW, LookupPrivilegeValueW, NO_INHERITANCE, PSECURITY_DESCRIPTOR, PSID, SE_PRIVILEGE_ENABLED, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, TOKEN_PRIVILEGES, @@ -482,6 +482,15 @@ pub fn enable_privilege(name: PCWSTR, display: &str) -> windows::core::Result<() /// with `DISABLE_MAX_PRIVILEGE` is what keeps a compromised renderer from /// doing anything with a token that would otherwise be fully privileged for /// its account. +/// +/// `LOGON32_LOGON_SERVICE`, not `_BATCH`: confirmed against a real install +/// that a batch-logon token here cannot create *any* named synchronization +/// object (`CreateMutex` denied even for random names, in both `Local\` and +/// `Global\`, while the same token creates named pipes/files/registry keys +/// fine) — Chromium's own `ProcessSingleton` needs exactly that and fails. +/// Batch logons carry the `NT AUTHORITY\BATCH` well-known SID rather than +/// `INTERACTIVE`; a hardened `BaseNamedObjects` ACL that grants creation +/// rights by logon-type SID rather than a broad "Everyone" excludes it. pub fn service_account_token(username: &str, password: &str) -> windows::core::Result { let username_wide = wide(username); let password_wide = wide(password); @@ -492,7 +501,7 @@ pub fn service_account_token(username: &str, password: &str) -> windows::core::R PCWSTR(username_wide.as_ptr()), w!("."), PCWSTR(password_wide.as_ptr()), - LOGON32_LOGON_BATCH, + LOGON32_LOGON_SERVICE, LOGON32_PROVIDER_DEFAULT, &mut primary, )?; @@ -615,10 +624,10 @@ pub fn ensure_desktop_access(sid: &[u8]) -> windows::core::Result<()> { } /// Denies the service account the logon types that would let it sign someone -/// in — it must not be usable at the very screen it serves. `Batch`, what -/// `service_account_token` uses (`LOGON32_LOGON_BATCH`), is deliberately not -/// among these. `LsaAddAccountRights` is itself idempotent, so this is safe -/// on every load. +/// in — it must not be usable at the very screen it serves. `Service`, what +/// `service_account_token` uses (`LOGON32_LOGON_SERVICE`), is deliberately +/// not among these. `LsaAddAccountRights` is itself idempotent, so this is +/// safe on every load. pub fn deny_interactive_and_network_logon(sid: &[u8]) -> windows::core::Result<()> { const RIGHTS: [&str; 3] = [ "SeDenyInteractiveLogonRight", diff --git a/vpkg/windows/Package.wxs b/vpkg/windows/Package.wxs index e420817c..94879d7e 100644 --- a/vpkg/windows/Package.wxs +++ b/vpkg/windows/Package.wxs @@ -133,23 +133,28 @@ S4U route: LogonUI's own token does not carry SE_TCB_NAME, the same wall Chromium's own credential provider hits and answers the same way (chrome/credential_provider/gaiacp/gcp_utils.cc, - CreateLogonToken). LogonAsBatchJob grants the SeBatchLogonRight - that needs. The account is denied all interactive/network/RDP - logon regardless, and the token credprovider actually hands - ak_cef.exe has every privilege stripped via CreateRestrictedToken - on top of that. No dictionary words and no "$" in the initial - password below: NetUserAdd folds complexity/banned-word LSA - filter rejections into the same NERR_PasswordTooShort a real - length failure would give, and "$" is the WiX preprocessor's - substitution leader ("$(var.X)"), live before the XML is - parsed. Keep "Name" in step with - credprovider::syscalls::SERVICE_ACCOUNT_NAME. --> + CreateLogonToken). LogonAsService grants the SeServiceLogonRight + that LOGON32_LOGON_SERVICE needs — not LogonAsBatchJob/BATCH: a + batch-logon token could not create any named synchronization + object on a real install (confirmed: CreateMutex denied even + for random names), which is fatal to Chromium's own + ProcessSingleton; see the comment on + credprovider::syscalls::service_account_token. The account is + denied all interactive/network/RDP logon regardless, and the + token credprovider actually hands ak_cef.exe has every + privilege stripped via CreateRestrictedToken on top of that. No + dictionary words and no "$" in the initial password below: + NetUserAdd folds complexity/banned-word LSA filter rejections + into the same NERR_PasswordTooShort a real length failure + would give, and "$" is the WiX preprocessor's substitution + leader ("$(var.X)"), live before the XML is parsed. Keep + "Name" in step with credprovider::syscalls::SERVICE_ACCOUNT_NAME. --> From ef3dc8aa3cf83d2dc7690b2078c3f582529f4766 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 02:14:05 +0100 Subject: [PATCH 31/41] ee/wcp: grant the service account BaseNamedObjects create-object rights LOGON32_LOGON_SERVICE alone was not enough: the service account's token still couldn't create named synchronization objects (CreateMutex etc.) in the session's BaseNamedObjects directory, which is fatal to Chromium's ProcessSingleton. GCPW hits the identical requirement for its own logon SID and answers it the same way (AllowLogonSIDOnLocalBasedNamedObjects), via NtOpenDirectoryObject since no Win32 wrapper exists for opening an arbitrary Object Manager directory. --- Cargo.toml | 6 ++ ee/wcp/credprovider/src/ipc.rs | 3 + ee/wcp/credprovider/src/syscalls.rs | 89 +++++++++++++++++++++++------ 3 files changed, 80 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 04d2bc55..ff9ea129 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -102,6 +102,12 @@ windows = { version = "0.62", features = [ "Win32_NetworkManagement_NetManagement", "Win32_System_SystemInformation", "Win32_Storage_EnhancedStorage", + # `NtOpenDirectoryObject` — no Win32 wrapper exists for opening an + # arbitrary Object Manager directory (`BaseNamedObjects`) the way + # `OpenDesktopW` does for desktops. + "Wdk_Foundation", + "Wdk_Storage_FileSystem", + "Wdk_System_SystemServices", # `ICredentialProviderUser::GetValue` returns a PROPVARIANT, so the e2e # harness needs these to implement that interface for `SetUserArray`. "Win32_System_Com_StructuredStorage", diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index eecd47df..e71aab9f 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -548,6 +548,9 @@ fn acquire_service_account_token() -> windows::core::Result { if let Err(e) = syscalls::ensure_desktop_access(&sid) { log::warn!("could not grant the service account secure-desktop access: {e}"); } + if let Err(e) = syscalls::ensure_base_named_objects_access(&sid) { + log::warn!("could not grant the service account BaseNamedObjects access: {e}"); + } syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME, &password) } diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index f453149d..fe853754 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -478,19 +478,10 @@ pub fn enable_privilege(name: PCWSTR, display: &str) -> windows::core::Result<() /// Mints a primary token for the service account by logging it on with its /// stored password, then strips every privilege from the result — the same /// pattern GCPW uses for its own LogonUI-hosted sign-in UI (`CreateLogonToken` -/// in `chrome/credential_provider/gaiacp/gcp_utils.cc`). `CreateRestrictedToken` -/// with `DISABLE_MAX_PRIVILEGE` is what keeps a compromised renderer from -/// doing anything with a token that would otherwise be fully privileged for -/// its account. -/// -/// `LOGON32_LOGON_SERVICE`, not `_BATCH`: confirmed against a real install -/// that a batch-logon token here cannot create *any* named synchronization -/// object (`CreateMutex` denied even for random names, in both `Local\` and -/// `Global\`, while the same token creates named pipes/files/registry keys -/// fine) — Chromium's own `ProcessSingleton` needs exactly that and fails. -/// Batch logons carry the `NT AUTHORITY\BATCH` well-known SID rather than -/// `INTERACTIVE`; a hardened `BaseNamedObjects` ACL that grants creation -/// rights by logon-type SID rather than a broad "Everyone" excludes it. +/// in `chrome/credential_provider/gaiacp/gcp_utils.cc`). `LOGON32_LOGON_SERVICE`, +/// not `_BATCH`: a batch-logon token cannot create named synchronization +/// objects, fatal to Chromium's own `ProcessSingleton` — see +/// `BROWSER_PRIVILEGE.md`'s "Roads not taken" for why. pub fn service_account_token(username: &str, password: &str) -> windows::core::Result { let username_wide = wide(username); let password_wide = wide(password); @@ -540,14 +531,15 @@ pub(crate) fn sid_to_string(sid: &[u8]) -> windows::core::Result { } } -/// Adds `sid` to `handle`'s DACL with `GENERIC_ALL`, preserving every +/// Adds `sid` to `handle`'s DACL with `access_mask`, preserving every /// existing entry — `SetEntriesInAclW` merges onto `old_dacl` rather than /// replacing it, which matters here: replacing outright would drop /// SYSTEM/Administrators access to the object this process itself needs. -unsafe fn grant_generic_all( +unsafe fn grant_access( handle: HANDLE, object_type: SE_OBJECT_TYPE, sid: &[u8], + access_mask: u32, ) -> windows::core::Result<()> { unsafe { let mut old_dacl: *mut ACL = std::ptr::null_mut(); @@ -572,7 +564,7 @@ unsafe fn grant_generic_all( ptstrName: PWSTR(sid.as_ptr() as *mut u16), }; let entry = EXPLICIT_ACCESS_W { - grfAccessPermissions: GENERIC_ALL.0, + grfAccessPermissions: access_mask, grfAccessMode: SET_ACCESS, grfInheritance: NO_INHERITANCE, Trustee: trustee, @@ -611,7 +603,7 @@ unsafe fn grant_generic_all( pub fn ensure_desktop_access(sid: &[u8]) -> windows::core::Result<()> { unsafe { let winsta = GetProcessWindowStation()?; - grant_generic_all(HANDLE(winsta.0), SE_WINDOW_OBJECT, sid)?; + grant_access(HANDLE(winsta.0), SE_WINDOW_OBJECT, sid, GENERIC_ALL.0)?; let desktop = OpenDesktopW( w!("Winlogon"), @@ -619,7 +611,68 @@ pub fn ensure_desktop_access(sid: &[u8]) -> windows::core::Result<()> { false, (READ_CONTROL | WRITE_DAC).0, )?; - grant_generic_all(HANDLE(desktop.0), SE_WINDOW_OBJECT, sid) + grant_access(HANDLE(desktop.0), SE_WINDOW_OBJECT, sid, GENERIC_ALL.0) + } +} + +/// Grants the service account's SID rights to create objects in this +/// session's `BaseNamedObjects` — the Object Manager directory Windows +/// resolves `Local\`-prefixed names into. No Win32 wrapper exists for +/// opening an arbitrary Object Manager directory the way `OpenDesktopW` +/// does for desktops, hence the native `NtOpenDirectoryObject` call. +/// Mirrors GCPW's own `AllowLogonSIDOnLocalBasedNamedObjects` +/// (`chrome/credential_provider/gaiacp/os_process_manager.cc`), including +/// its narrower-than-`GENERIC_ALL` mask — see `BROWSER_PRIVILEGE.md`. +pub fn ensure_base_named_objects_access(sid: &[u8]) -> windows::core::Result<()> { + use windows::Wdk::Foundation::OBJECT_ATTRIBUTES; + use windows::Wdk::Storage::FileSystem::NtOpenDirectoryObject; + use windows::Wdk::System::SystemServices::{ + DIRECTORY_CREATE_OBJECT, DIRECTORY_CREATE_SUBDIRECTORY, DIRECTORY_QUERY, DIRECTORY_TRAVERSE, + }; + use windows::Win32::Foundation::UNICODE_STRING; + use windows::Win32::System::RemoteDesktop::ProcessIdToSessionId; + use windows::Win32::System::Threading::GetCurrentProcessId; + + let mut session_id = 0u32; + unsafe { ProcessIdToSessionId(GetCurrentProcessId(), &mut session_id)? }; + + let path = if session_id == 0 { + r"\BaseNamedObjects".to_string() + } else { + format!(r"\Sessions\{session_id}\BaseNamedObjects") + }; + let path_wide = wide(&path); + let mut name = UNICODE_STRING { + Length: ((path_wide.len() - 1) * 2) as u16, + MaximumLength: (path_wide.len() * 2) as u16, + Buffer: PWSTR(path_wide.as_ptr() as *mut u16), + }; + let object_attributes = OBJECT_ATTRIBUTES { + Length: size_of::() as u32, + ObjectName: &mut name, + ..Default::default() + }; + + unsafe { + let mut directory = HANDLE::default(); + NtOpenDirectoryObject( + &mut directory, + DIRECTORY_TRAVERSE | READ_CONTROL.0 | WRITE_DAC.0, + &object_attributes, + ) + .ok()?; + + let result = grant_access( + directory, + SE_WINDOW_OBJECT, + sid, + DIRECTORY_QUERY + | DIRECTORY_TRAVERSE + | DIRECTORY_CREATE_OBJECT + | DIRECTORY_CREATE_SUBDIRECTORY, + ); + let _ = CloseHandle(directory); + result } } From f7f866c332f6e306d86284f111f3a67c1b9c5cfb Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 02:14:15 +0100 Subject: [PATCH 32/41] ee/wcp: drop diagnostic scaffolding, rewrite the design doc for final state The CreateMutex/token diagnostics in cef-host's main() served their purpose (root-causing the BaseNamedObjects failure) and don't belong in the shipped feature. Rewrites BROWSER_PRIVILEGE.md from a chronological debugging journal into a concise doc describing the final design, condensing the "roads not taken" investigation into short factual paragraphs. --- ee/wcp/BROWSER_PRIVILEGE.md | 508 +++++++++--------------------------- ee/wcp/cef-host/src/main.rs | 138 ---------- 2 files changed, 129 insertions(+), 517 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 81f54b40..2ca65f67 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -6,389 +6,139 @@ the Windows logon screen, and on the path that matters most it runs as ## Status -Branch `ee/wcp/browser-privilege` is building Option B, the dedicated -service account. The `WinSta0\Winlogon` ACL grant is accepted as the cost of -getting off SYSTEM. Option A turned out to need more than this doc assumed — -see the note at the end of that section — so it stayed out of scope rather -than being folded in here. - -**S4U turned out not to work and was replaced.** The first version of this -branch minted the service account's token via `LsaLogonUser`/ -`MSV1_0_S4U_LOGON`, on the assumption that LogonUI's own token carries -`SE_TCB_NAME` (see "Getting a token" below for what actually happens). A real -install on a Windows Server test box proved that assumption wrong: -`LsaRegisterLogonProcess` failed outright, and enabling the privilege -first — which should not have been necessary, and was itself a sign -something was off — surfaced that `SE_TCB_NAME` is not held by that token at -all, confirmed by dumping the token's own account identity in the same log -line. Chromium's own credential provider hits the identical wall for the -identical reason and has never used S4U to begin with; see "Getting a token" -below. The service account now gets its token the same way GCPW does: -`LogonUserW` with a stored password, then `CreateRestrictedToken` to strip -every privilege before the token is ever used. - -**Assigning that token hit the same wall one layer deeper, and was also -replaced.** `CreateProcessAsUserW` needs the caller to hold -`SE_ASSIGNPRIMARYTOKEN_NAME`/`SE_INCREASE_QUOTA_NAME`; a real install -returned `ERROR_PRIVILEGE_NOT_HELD` (0x80070522), and — same lesson as -`SE_TCB_NAME` — those privileges are absent from LogonUI's token, not merely -disabled. Replaced with `CreateProcessWithTokenW`, which needs only -`SE_IMPERSONATE_NAME` (confirmed present and enabled on the test box via -`PsExec -s whoami /priv`) because it is brokered through the Secondary Logon -service rather than assigning the token directly. See "Assigning the token" -below. That API has no handle-inheritance mechanism at all, which forced the -IPC redesign described there too: the result/cancel pipe pair is now named -rather than anonymous-and-inherited. - -**Two more real-install failures, both fixed.** The named-pipe DACL -hardcoded `SY` (SYSTEM) as the fallback identity when no connecting SID was -given — wrong for `CPUS_CREDUI`, whose child inherits the *caller's* own -token rather than SYSTEM's; fixed by dropping the explicit DACL in that case -and letting `CreateNamedPipeW` apply the default one derived from the -creating token instead. Separately, a real install hit `CefInitialize` -failing with Chromium's own `ProcessSingleton` error -("Failed to create a ProcessSingleton for your profile directory ... -Aborting now to avoid profile corruption") — `root_cache_path` doubled as -Chromium's user-data directory, which is what `ProcessSingleton` locks -against, and every launch shared one fixed path -(`C:\ProgramData\Authentik Security Inc\wcp-cache`). A second sign-in -attempt starting before the first one's process had fully torn down (or any -leftover instance) made every subsequent launch fail outright. Fixed by -giving each launch its own unique subdirectory (`cef-host/src/main.rs:: -browser_state_dir`) instead, removed again once that run is done -(`wipe_browser_state`, best-effort — a launch the credential provider had to -kill for never responding, or one that never got as far as `CefInitialize` -succeeding, leaves its directory behind on purpose, as the only record of -what that run's environment looked like). - -**The unique directory did not fix `ProcessSingleton` either — nor did an -explicit environment block; the real cause was CEF's Chrome-style runtime.** -A fresh, guaranteed-unique `root_cache_path` still failed the identical way -on a real install, and the directory was empty afterwards (expected — see -the paragraph above — but it meant nothing had gone wrong with the path -itself). The next suspect was `CreateProcessWithTokenW`'s `lpEnvironment: -None` — plausible, since `LOGON_WITH_PROFILE` only loads the account's -registry hive and does not itself touch the environment block. That was -fixed with `CreateEnvironmentBlock(token)` in `ipc.rs::spawn_with_token` -regardless (it is more explicit and does not depend on an -easy-to-misremember default), but it was not the actual cause: MSDN -documents that `CreateProcessWithTokenW` specifically (unlike -`CreateProcessAsUserW`) already builds a correct per-token environment when -`lpEnvironment` is `NULL` — confirmed by checking Google Credential Provider -for Windows's own equivalent call (`OSProcessManager::CreateProcessWithToken` -in `chrome/credential_provider/gaiacp/os_process_manager.cc`), which passes -`nullptr` and ships in production. Comparing GCPW's token -(`LogonUserW`+`CreateRestrictedToken(DISABLE_MAX_PRIVILEGE)`, identical to -ours), desktop (`WinSta0\Winlogon`, identical), and spawn API -(`CreateProcessWithTokenW`+`LOGON_WITH_PROFILE`, identical) turned up no -difference — because GCPW's "sign-in UI helper" is not a Chromium browser -process at all. `ForkGaiaLogonStub` launches `rundll32.exe` reloading the -credential provider's own DLL through an entrypoint; it never runs -`chrome_main_delegate.cc` or `ProcessSingleton` in the first place. The -next candidate: CEF (151.x) supports both a **Chrome style** runtime (the -full Chrome UI/browser layer — extensions, profile manager, -`ProcessSingleton`) and a lighter **Alloy style** runtime built for -embedding, and windowed browsers default to Chrome style unless told -otherwise. `SignInWindowDelegate::window_runtime_style` (`window.rs`) and -`AlloyBrowserView::browser_runtime_style` (`app.rs`) now return -`RuntimeStyle::ALLOY` for exactly that reason, and it is still correct to -keep — this window genuinely needs no Chrome UI — but it turned out **not** -to be what was failing: CEF's own architecture docs (and the "Delete Alloy -bootstrap" change in M128, well before 151.x) confirm the Chrome-vs-Alloy -choice only selects a *style* layered on top of an always-Chrome -*bootstrap* — `ProcessSingleton` runs during `CefInitialize` unconditionally, -before any window or `BrowserView` (and therefore any `runtime_style`) is -ever created. - -**The real cause: `LOGON32_LOGON_BATCH` tokens cannot create named -synchronization objects in the session namespace at all.** With no more -plausible CEF-level explanation left, `ak_cef.exe` was changed to ask the -same `::CreateMutex` question `ProcessSingleton::Create()` asks, directly, -logging through `ak_platform::log` (Chromium's own diagnostic for this exact -failure is `DPLOG(FATAL)`, compiled to nothing in a release build — silent -regardless of log verbosity). Confirmed against a real install: -`CreateMutex(Local\ChromeProcessSingletonStartup!)` fails with -`ERROR_ACCESS_DENIED`, and so does `CreateMutex` with a random UUID name -under both `Local\` and `Global\` — ruling out both a stale wrong-owner -object (`handle.exe` found nothing pre-existing) and a name-specific block -(endpoint security software targeting the well-known Chrome string was the -leading suspect until the random names failed identically too). The same -token's token identity is unremarkable — Medium integrity, session 1, not -`IsTokenRestricted` — and it creates named pipes, files and registry keys -under this exact token with no issue; only named synchronization objects -(mutex/event/semaphore) fail. The distinguishing factor: `LOGON32_LOGON_BATCH` -tokens carry the `NT AUTHORITY\BATCH` well-known SID rather than -`INTERACTIVE`, and it is a documented Windows security-hardening pattern -(CIS/STIG-style baselines) to scope `BaseNamedObjects` object-creation -rights to interactive/service identities specifically, excluding batch -logons — plausible on a baselined Windows Server test box. Fixed by -switching to `LOGON32_LOGON_SERVICE` (`SeServiceLogonRight`/ -`LogonAsService="yes"` instead of `SeBatchLogonRight`/`LogonAsBatchJob="yes"`) -— unverified as of this writing whether that alone resolves it or whether -`BaseNamedObjects` also needs an explicit ACL grant for the account, the -same way `ensure_desktop_access` already does for `WinSta0\Winlogon`. - -**Verified on a VM** (before any of the replacements above) against the manual -checklist in `e2e/README.md`: the tile appeared, the sign-in window opened on -the secure desktop, and both fresh logon and unlock completed. That VM run -predates both failures above and does not carry over to the current code — -the `WinSta0\Winlogon` ACL grant holding is still good evidence, since that -part is unchanged, but the token-acquisition and process-launch paths it -exercised no longer exist. Needs a fresh VM run. - -This file previously lived on `ee/wcp/rs-cef-fresh` and was deleted there in -a "cleanup" commit an hour after being written, along with three sibling -design docs; nothing indicates that was deliberate. Restored from -`git show 93549e00:ee/wcp/BROWSER_PRIVILEGE.md`. - -## Where things stand - -Neither the current implementation nor the C++ one it replaced has ever run -this browser sandboxed or unprivileged on a fresh logon. - -**Sandbox — off, always, in both.** `cef-host/src/main.rs` sets -`no_sandbox: 1` and passes a null `sandbox_info`. The C++ looked like it might -enable it — `ak_cred_provider/Provider.cpp` had both a `CefScopedSandboxInfo` -block and `settings.no_sandbox = true`, each guarded on `CEF_USE_SANDBOX` — but -that define only reaches a target through CEF's -`SET_EXECUTABLE_TARGET_PROPERTIES`/`SET_LIBRARY_TARGET_PROPERTIES` macros, the -one call site in `cefsimple/CMakeLists.txt` was commented out, and -`ak_cred_provider` never invoked either. So it compiled with the sandbox -disabled while `if(USE_SANDBOX)` still linked `cef_sandbox_lib` and applied -`SET_LPAC_ACLS` — a build that looks sandboxed and is not. - -**Identity — was SYSTEM on the path that matters; fixed on this branch.** -Before this branch, `credprovider::syscalls::acquire_interactive_token` tried -`WTSQueryUserToken` first and fell back to duplicating `winlogon.exe`'s -token: - -| Scenario | Token | Ran as | -| --- | --- | --- | -| `CPUS_LOGON` (fresh logon) | winlogon duplicate — nobody is signed in yet, so there was no user token to get | **SYSTEM** | -| `CPUS_UNLOCK_WORKSTATION` | `WTSQueryUserToken` | the locked-out user — but the spawn then failed, that token had no access to `WinSta0\Winlogon` | -| `CPUS_CREDUI` (debug) | none; the caller holds no `SE_TCB_NAME`, so it fell through to `CreateProcessW` | the interactive user | - -The C++ ran CEF in-process inside `ak_cred_provider.dll`, i.e. inside LogonUI, -so the browser process *was* LogonUI: SYSTEM, with helper processes inheriting -that token. It never chose an identity because it never spawned anything. - -So the old token machinery bought nothing on fresh logon (it worked hard to -arrive at the same SYSTEM the C++ got for free) and was broken on unlock — -not load-bearing for the sign-in flow, an unfinished attempt at this -document's goal. Replaced entirely: `credprovider::syscalls:: -service_account_token` now logs the dedicated account -(`SERVICE_ACCOUNT_NAME`, see Option B below) on with `LogonUserW` and its -stored password for both `CPUS_LOGON` and `CPUS_UNLOCK_WORKSTATION` — one -path, no per-scenario branching, no `WTSQueryUserToken`/winlogon-scanning -left in `syscalls.rs` at all. `CPUS_CREDUI` is unchanged. +Branch `ee/wcp/browser-privilege` builds Option B: a dedicated local service +account (`ak-wcp-browser`), created by the installer, that `ak_cef.exe` runs +as instead of SYSTEM. Option A (sandbox the CEF renderer/GPU processes +instead) needs real upstream-shaped work — see that section — so it's out of +scope here; the two are complementary, not exclusive. + +Verified on a VM against the manual checklist in `e2e/README.md` on an +earlier version of the token/spawn path; needs a fresh run against the +current code. + +### The design + +- **Token**: `LogonUserW(LOGON32_LOGON_SERVICE)` with a stored password, + then `CreateRestrictedToken(DISABLE_MAX_PRIVILEGE)` to strip every + privilege — the same pattern Google Credential Provider for Windows + (GCPW) uses for its own LogonUI-hosted helper identity + (`CreateLogonToken`, `chrome/credential_provider/gaiacp/gcp_utils.cc`). +- **Spawn**: `CreateProcessWithTokenW`, brokered through the Secondary + Logon service — the only API of the three tried that actually works from + inside LogonUI (see "Roads not taken"). +- **IPC**: named pipes, not anonymous-and-inherited — + `CreateProcessWithTokenW` has no handle-inheritance mechanism at all. A + random UUID name per launch; an SDDL DACL scopes access to SYSTEM plus + the service account's own SID. +- **Desktop**: `syscalls::ensure_desktop_access` grants the account's SID + `GENERIC_ALL` on `WinSta0` and its `Winlogon` desktop. +- **`BaseNamedObjects`**: `syscalls::ensure_base_named_objects_access` + grants the account's SID create-object rights on the session's + `BaseNamedObjects` directory via the native `NtOpenDirectoryObject` (no + Win32 wrapper exists for opening an arbitrary Object Manager directory) — + GCPW hits the identical requirement and answers it the identical way + (`AllowLogonSIDOnLocalBasedNamedObjects`, `os_process_manager.cc`). +- **Hardening**: `syscalls::deny_interactive_and_network_logon` denies the + account interactive/network/RDP logon rights — it must not be usable to + sign in at the screen it serves. +- **Cache/profile**: each launch gets its own unique subdirectory under + `wcp-cache` (`cef-host::browser_state_dir`), not a shared fixed path — + removed after a successful run, left behind after a failed one for + inspection. +- **CEF runtime style**: `RuntimeStyle::ALLOY`, not the Chrome-style + default — this window needs no Chrome UI (tabs, extensions, profile + manager). + +Previously, a fresh logon ran the browser as SYSTEM outright (no +interactive-user token existed yet to duplicate) and unlock used the +locked-out user's own token, which then failed to spawn — no access to the +secure desktop. Both scenarios now go through the same service-account token +unconditionally; `CPUS_CREDUI` (the debug-only scenario) is unchanged, and +still falls back to launching in the caller's own session. + +### Roads not taken + +Each of these looked like the obvious approach and turned out not to work, +confirmed against real installs rather than documentation alone: + +- **S4U (`LsaLogonUser`/`MSV1_0_S4U_LOGON`)** avoids a stored password + entirely, but needs `SE_TCB_NAME` — which LogonUI's token does not hold + *at all* (not merely disabled; `PsExec -s whoami /priv` shows SYSTEM + holding it fine, so LogonUI's own hosting process token is evidently not + that same generic SYSTEM token). This is by design: the + credential-provider model since Vista deliberately keeps LogonUI more + sandboxed than `winlogon.exe`/LSASS specifically so third-party code + loaded into it doesn't get TCB-level trust. GCPW has never used S4U for + the identical reason. +- **`CreateProcessAsUserW`** needs the caller to hold + `SE_ASSIGNPRIMARYTOKEN_NAME`/`SE_INCREASE_QUOTA_NAME` — also absent from + LogonUI's token, not disabled. `CreateProcessWithTokenW` sidesteps this: + brokered through the Secondary Logon service, it only needs the caller to + hold `SE_IMPERSONATE_NAME`, which is present. +- **`LOGON32_LOGON_BATCH`** (what GCPW itself uses for its own helper + identity) cannot create *any* named synchronization object + (mutex/event/semaphore) in the session's object namespace — confirmed via + `CreateMutex` failing with `ERROR_ACCESS_DENIED` even for random, + non-reserved names under both `Local\` and `Global\`, while the same + token creates named pipes, files and registry keys with no issue. Fatal + to Chromium's own `ProcessSingleton`, which runs unconditionally during + `CefInitialize`. GCPW never hits this itself because its sign-in UI + helper isn't a Chromium browser process at all — it's `rundll32.exe` + reloading their own DLL through an entrypoint (`ForkGaiaLogonStub`) — but + their code still explicitly grants `BaseNamedObjects` access to the logon + SID, which is what led to `ensure_base_named_objects_access` above. + `LOGON32_LOGON_SERVICE` is the fix: batch logons carry the + `NT AUTHORITY\BATCH` well-known SID rather than `INTERACTIVE`, and + hardened `BaseNamedObjects` ACLs commonly key on logon-type SID. +- **CEF's Chrome-vs-Alloy runtime style** looked like it might also explain + the `ProcessSingleton` failure (Chrome style pulls in that whole + subsystem), but doesn't: the choice only selects a *style* layered on top + of an always-Chrome *bootstrap* (confirmed via CEF's own architecture + docs and the "Delete Alloy bootstrap" change in M128) — + `ProcessSingleton` runs during `CefInitialize` regardless of + `runtime_style`. Kept Alloy anyway, since it's still the right choice for + a single-purpose window with no Chrome UI. ## Option A — enable the CEF sandbox -Do this first. It targets the actual risk — hostile input being parsed — rather -than the privilege of the process that hosts the parser, and it needs no new -account, no installer change and no ACL grant on the logon desktop. - -- Link `cef_sandbox.lib` and pass real `sandbox_info` to `initialize` instead of - a null pointer; drop `no_sandbox`. -- Verify renderer/GPU/utility processes actually come up sandboxed on the - secure desktop. This is the part to be sceptical about: Chromium's sandbox - creates its own alternate desktop for sandboxed children, and how that - interacts with `WinSta0\Winlogon` is unverified here. -- `SET_LPAC_ACLS` in the old C++ build hints the CEF sample expects LPAC ACLs on - the binary directory; check whether the MSI needs to apply the equivalent to - `bin/wcp/`. - -Leaves the browser process itself as SYSTEM. That is the C++'s posture, so it -is not a regression — but it is not the end state either. - -**Turns out to be more than "link a lib and pass a pointer".** The `cef` -crate actually in use here (151.4.0, `ee/wcp/cef-host/Cargo.toml`) has no -Windows sandbox support at the Rust level at all: `cef::sandbox::Sandbox` is -`#[cfg(target_os = "macos")]`-only, and `cef-dll-sys`'s vendored `wrapper.h` -only includes `cef_sandbox_mac.h`. `execute_process`/`initialize` accept a -`windows_sandbox_info: *mut c_void` on Windows, but nothing constructs one — -that requires CEF's C++-only `CefScopedSandboxInfo` -(`include/cef_sandbox_win.h`), which the crate's C-API bindings don't -expose. `cef-dll-sys`'s `build.rs` passes `USE_SANDBOX=ON` to CMake but only -builds the `libcef_dll_wrapper` target, never `cef_sandbox`, so nothing -actually links the sandbox lib today despite the `sandbox` feature being on -by default. Doing this properly means a small C++ shim exposing a C ABI -around `CefScopedSandboxInfo`, plus a patch to `cef-dll-sys`'s `build.rs` to -build and link `cef_sandbox.lib` — upstream-shaped work, independent of this -branch. Left for whoever picks Option A up. +Not started. Targets the actual risk — hostile input being parsed — rather +than the privilege of the process that hosts the parser, and needs no new +account, installer change, or desktop ACL grant. Complementary to Option B, +not a substitute for it. -## Option B — a dedicated local account from the installer - -Defence in depth on top of A: create a local account at install time and launch -`ak_cef.exe` as it. - -The reason to prefer this over the current code is that the account exists -before anyone signs in, so logon and unlock become one path and the whole -`WTSQueryUserToken` / winlogon-scan fallback chain in `syscalls.rs` can be -deleted rather than fixed. - -`WixToolset.Util.wixext` 6.0.0 is already referenced by -`vpkg/windows/authentik Agent Installer.wixproj` and `Package.wxs` already uses -the `util:` namespace, so `` needs no new dependency. - -### Getting a token — S4U looked free, wasn't - -`` wants a password and `LogonUser` would want it back later, which -means a secret at rest and a rotation story. `LsaLogonUser` with -`MSV1_0_S4U_LOGON` looked like it avoided that entirely: mint a token for a -local account with no credentials, given `SE_TCB_NAME`, which the doc assumed -LogonUI already holds. That assumption is wrong, and the branch's history -still has the wreckage: `LsaRegisterLogonProcess` failed with -`STATUS_PORT_CONNECTION_REFUSED` on a real install. Explicitly enabling -`SE_TCB_NAME` first (`AdjustTokenPrivileges`) made that failure go away — -which itself should have been the warning sign, since a token that legitimately -holds a privilege does not usually need it force-enabled — and once the code -started checking `AdjustTokenPrivileges`'s own success/failure properly -(`ERROR_NOT_ALL_ASSIGNED` is reported as success by the underlying Win32 call; -only `GetLastError` after the fact tells you it lied), the real answer came -back: `SE_TCB_NAME` is not held by LogonUI's token *at all*, on a real -install, full stop. Not disabled — absent. `PsExec -s whoami /priv` on the -same box shows SYSTEM holding it fine; the credential-provider-hosting -process's token is evidently not that same generic SYSTEM token. - -In hindsight this is exactly what the credential-provider model since Vista -is *for*: `LogonUI.exe` is a separate, deliberately more sandboxed process -from `winlogon.exe`/LSASS precisely so that third-party code loaded into it — -credential providers — does not get TCB-level trust. The built-in providers -never call LSA logon APIs directly; they hand LSA a serialized credential -blob via `GetSerialization` and let LSASS, which does have TCB, do the -privileged part (this codebase already does exactly that for the -interactive user's own logon — see `LOCAL_PASSWORD.md`, -`pack_kerb_interactive_unlock_logon`). Calling `LsaRegisterLogonProcess` -directly from inside a credential provider was never going to be supported. - -Chromium's own credential provider (Google Credential Provider for Windows, -GCPW) solves the identical problem — a Chromium-based sign-in UI hosted in -LogonUI, needing a token for a helper identity — and has never used S4U or -any LSA-direct call for it. `CreateLogonToken` in -`chrome/credential_provider/gaiacp/gcp_utils.cc` calls plain `LogonUserW` -with a real password (`LOGON32_LOGON_BATCH` for the non-interactive case), -then wraps the result in `CreateRestrictedToken(..., DISABLE_MAX_PRIVILEGE, -...)` before using it — stripping every privilege from the token rather than -relying on the account having few to begin with. `service_account_token` -follows the same pattern but with `LOGON32_LOGON_SERVICE` instead — see -"Assigning the token" below for why BATCH specifically does not work here. -GCPW never hits this: its own sign-in UI helper is not a browser process -(see "Assigning the token"), so it never calls anything like -`ProcessSingleton` that would expose the difference. `LogonUserW` needs no -special privilege for a non-admin account (confirmed independently: -`RealSyscalls::validate` already called it successfully for the interactive -user's password check, with no privilege-enabling, before any of this). - -This puts the service account's password handling on the same footing as -the interactive user's: a real secret has to exist and be kept somewhere. -`credprovider::syscalls::service_account_password` is the same state -machine as `credential.rs::account_password` (`LOCAL_PASSWORD.md`) — -established once, validated and reused on every subsequent call, changed -rather than reset once known — stored in the same `KeyringPasswordStore` -vault, keyed by this account's own SID instead of a signed-in user's. WiX's -`util:User` has no `GeneratePassword` attribute — it only takes a literal -`Password` (or one read from a `Property` via `PasswordAttribute`) — so the -installer creates the account with a fixed placeholder, and the first call -to `service_account_password` resets it to a real, stored value. `LogonAsService="yes"` -on `` grants the `SeServiceLogonRight` that `LOGON32_LOGON_SERVICE` -needs; the account is still denied interactive/network/RDP logon throughout, -and the *token `ak_cef.exe` actually runs with* has every privilege stripped -by `CreateRestrictedToken` regardless of what the logon itself produced. - -**The Kerberos/SPNEGO question this used to raise for S4U does not apply -here.** `LogonUserW` is a real password-based logon; the resulting token (net -of `CreateRestrictedToken`'s privilege stripping, which is unrelated to -network credentials) carries the account's actual credentials the same way -any normal interactive or batch logon does. Nothing about this design rules -out integrated auth for this account if it were ever needed — the question -in the original "Decisions" list is moot, not settled in either direction. - -### Assigning the token — `CreateProcessAsUserW` needed privileges LogonUI didn't have either - -Minting the token was only half the problem. `CreateProcessAsUserW` — the -call that actually launches `ak_cef.exe` with it — requires the *caller* to -hold `SE_ASSIGNPRIMARYTOKEN_NAME` and `SE_INCREASE_QUOTA_NAME`. A real -install produced `ERROR_PRIVILEGE_NOT_HELD` (0x80070522), and checking -`AdjustTokenPrivileges`'s actual result (not just its optimistic return -value, the same trap `SE_TCB_NAME` hit) confirmed the same story: both -privileges are absent from LogonUI's token, not disabled. - -`CreateProcessWithTokenW` sidesteps this. It is not a direct syscall — it -asks the Secondary Logon (`seclogon`) service to do the launch on its -behalf — so the caller only needs `SE_IMPERSONATE_NAME`, which `PsExec -s -whoami /priv` confirmed is present and enabled for SYSTEM on the same box. -`ipc.rs::spawn_with_token` enables it defensively before the call anyway, -on the now-established principle that "looks enabled by default" is not -something to trust without checking. - -The cost is that `CreateProcessWithTokenW` takes a plain `STARTUPINFOW`, not -`STARTUPINFOEXW`, and has no handle-inheritance mechanism at all — there is -no attribute list to hand it a set of handles to pass down, the way -`CreateProcessAsUserW` had. The result/cancel pipe pair that `ak_cef.exe` -uses to report its outcome back therefore could not stay anonymous-and- -inherited; `ipc.rs` now creates a **named** pipe pair instead -(`create_duplex_pipes`), with a random UUID in the name so nothing else can -guess and connect to either end, and an SDDL DACL -(`ConvertStringSecurityDescriptorToSecurityDescriptorW`) scoping access to -SYSTEM plus the service account's own SID — the only two identities that -should ever be able to open them. The child is handed the pipe *names* on -its command line instead of inherited handle values, and opens them itself -with `CreateFileW` -(`cef-host/src/handler.rs::connect_result_pipe`/`connect_cancel_pipe`); the -parent blocks on `ConnectNamedPipe` (`connect_duplex_pipes`, bounded to 15s) -before trusting the pipes are live on the other end. - -### The rest of the work - -- **Secure desktop ACL — done.** `credprovider::syscalls:: - ensure_desktop_access` grants the account's SID `GENERIC_ALL` on both - `WinSta0` and its `Winlogon` desktop, merged onto the existing DACL via - `GetSecurityInfo`/`SetEntriesInAclW`/`SetSecurityInfo` rather than - replacing it. Weigh it honestly: this grants a service account the right - to create windows on the desktop where credentials are typed. Still a - large net win — a compromised renderer no longer yields SYSTEM — but a - deliberate expansion of what can reach the logon desktop, not a free - improvement. Confirmed working on a VM. `GENERIC_ALL` is broader than this - account strictly needs; narrowing it to the specific window-station/desktop - rights CEF actually uses is still open — the VM run proves the grant is - sufficient, not that it is minimal. -- **Profile and cache.** `root_cache_path` is explicit already, but Chromium - also wants temp, fonts and crashpad paths. Without `LoadUserProfile` the - account gets the default profile. The MSI grants the account write access - to `wcp-cache` (previously ProgramData defaults only); temp/fonts/crashpad - are still not addressed. They were the expected source of first-run - failures, and the VM run did not hit them — but "did not fail on one VM" is - weaker than "handled", so leave this open. -- **Harden the account — mostly done.** `credprovider::syscalls:: - deny_interactive_and_network_logon` denies `SeDenyInteractiveLogonRight`, - `SeDenyNetworkLogonRight` and `SeDenyRemoteInteractiveLogonRight` via - `LsaAddAccountRights`, called on every load (idempotent). The installer - creates the account with `RemoveOnUninstall="yes"`/`UpdateIfExists="yes"` - and no group membership beyond the default. Minimal-privilege trimming - beyond that is not yet done. -- **Deployment friction.** GPO blocking local account creation, endpoint - monitoring flagging a new local account, no local accounts on a DC — still - open, not addressable from this codebase. +Turns out to be more than "link a lib and pass a pointer": the `cef` crate +in use here (151.4.0) has no Windows sandbox support at the Rust level — +`cef::sandbox::Sandbox` is macOS-only, and `cef-dll-sys`'s `build.rs` never +builds or links `cef_sandbox.lib` despite the `sandbox` feature being on by +default. Doing this properly needs a small C++ shim exposing a C ABI around +CEF's `CefScopedSandboxInfo`, plus a `cef-dll-sys` `build.rs` patch — +upstream-shaped work, independent of this branch. ## Decisions — resolved for this branch -1. ~~Is integrated auth (Kerberos/SPNEGO to an upstream IdP) ever in scope?~~ - **Moot.** This account no longer uses S4U, so the concern that raised this - question does not apply — see "Getting a token" above. -2. ~~Is granting a non-SYSTEM account access to `WinSta0\Winlogon` - acceptable?~~ **Yes**, scoped to this one account only. -3. Does the browser process need to be non-SYSTEM at all once renderers are - sandboxed, or is option A sufficient? Not settled — moot for this branch - since option A is out of scope here; revisit if/when option A is built. - -## Do not conflate this with the `add_child_view` crash - -At the time of writing `ak_cef.exe` intermittently dies with `0x80000003` -(`STATUS_BREAKPOINT`, a Chromium `CHECK`) inside `CefWindow::add_child_view` on -the secure desktop, while the identical call succeeds under `CPUS_CREDUI` in -CI. That was being chased separately; see `RUST_CEF_PLAN.md` in git history -(`git show be23c35c:ee/wcp/RUST_CEF_PLAN.md`) for the state of it at the time. -Changing the sandbox or the launch identity to chase that crash would be -changing the security posture for a debugging reason — if it needs doing -temporarily, do it on a throwaway branch, not here. +1. Integrated auth (Kerberos/SPNEGO)? Moot — this account uses a real + password-based logon, not S4U, so nothing about this design rules it out + if it's ever needed. +2. Granting a non-SYSTEM account access to `WinSta0\Winlogon`? Yes, scoped + to this one account only — a deliberate, documented expansion, not a + free improvement. +3. Does the browser need to be non-SYSTEM at all once renderers are + sandboxed (Option A)? Not settled; revisit if/when Option A is built. + +## Known gaps + +- **`GENERIC_ALL` on the desktop/`BaseNamedObjects` grants** is broader + than strictly needed. Narrowing to the specific rights CEF actually uses + is still open. +- **Profile paths beyond `root_cache_path`** (temp, fonts, crashpad) are + not yet addressed; not the source of any observed failure so far, but + "hasn't failed yet" isn't "handled." +- **Deployment friction** — GPO blocking local account creation, endpoint + monitoring flagging a new local account, no local accounts on a domain + controller — isn't addressable from this codebase. +- **The account's own password validation is a no-op**: + `RealSyscalls::validate` uses `LOGON32_LOGON_NETWORK`, but the account is + denied network logon by design (`deny_interactive_and_network_logon`), so + it always reports "inconclusive" and the stored password is used without + ever being re-verified. Harmless today (falls through to "use it + anyway"), but worth a real fix. diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index cbde3446..aa2c1f79 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -38,141 +38,6 @@ const CACHE_ROOT: &str = r"C:\ProgramData\Authentik Security Inc\wcp-cache"; /// sees a message Chromium raises below the Rust layer. const CHROMIUM_LOG_PATH: &str = r"C:\ProgramData\Authentik Security Inc\logs\ak_cef_chromium.log"; -/// `::CreateMutex(NULL, FALSE, name)`, logging the result through a channel -/// already confirmed to work — Chromium's own diagnostic for this exact call -/// (inside `ProcessSingleton::Create()`) is `DPLOG(FATAL)`, which compiles to -/// nothing in a release build, so it stays silent regardless of log -/// verbosity. -fn try_create_mutex(label: &str, name: &str) { - use windows::Win32::Foundation::CloseHandle; - use windows::Win32::System::Threading::CreateMutexW; - use windows::core::PCWSTR; - - let name_wide: Vec = name.encode_utf16().chain(std::iter::once(0)).collect(); - match unsafe { CreateMutexW(None, false, PCWSTR(name_wide.as_ptr())) } { - Ok(handle) => { - log::info!("diagnostic: CreateMutex({label}) succeeded"); - unsafe { - let _ = CloseHandle(handle); - } - } - Err(e) => log::error!("diagnostic: CreateMutex({label}) failed: {e}"), - } -} - -/// The exact call CEF's `ProcessSingleton::Create()` makes, confirmed -/// failing with `ERROR_ACCESS_DENIED` against a real install, on a fresh -/// create with nothing pre-existing (`handle.exe` found no stale object). -/// The three follow-up calls narrow down why: a random name under the same -/// `Local\` session namespace tells apart "this token cannot create named -/// mutexes in the session namespace at all" from "this specific, -/// publicly-documented name is blocked" (endpoint security software is the -/// leading suspect for the latter — it is exactly the kind of string a -/// malware/injection heuristic keys on); a `Global\` name checks whether the -/// restriction is specific to the per-session namespace; a name containing -/// "Chrome" but not the exact reserved string checks whether the block -/// matches on the literal known string or more loosely on the product name. -fn diagnose_process_singleton_mutex() { - try_create_mutex( - "ChromeProcessSingletonStartup!", - "Local\\ChromeProcessSingletonStartup!", - ); - try_create_mutex( - "random, Local", - &format!("Local\\ak-cef-diagnostic-{}", uuid::Uuid::new_v4()), - ); - try_create_mutex( - "random, Global", - &format!("Global\\ak-cef-diagnostic-{}", uuid::Uuid::new_v4()), - ); - try_create_mutex( - "Chrome-ish but not the reserved string", - &format!("Local\\ChromeDiagnosticProbe-{}", uuid::Uuid::new_v4()), - ); -} - -/// Logs this process's own token identity, mandatory integrity level, -/// session id, and whether Windows considers it a *restricted* token -/// (`IsTokenRestricted`, true only if `CreateRestrictedToken` was given SIDs -/// to disable or restrict — `service_account_token` gives it neither, only -/// `DISABLE_MAX_PRIVILEGE`, so this is expected to read false; confirming -/// that rules out the double access-check restricted tokens get as an -/// explanation for the `CreateMutex` failure above). -fn diagnose_token() { - use std::ffi::c_void; - use windows::Win32::Foundation::{CloseHandle, HANDLE}; - use windows::Win32::Security::{ - GetSidSubAuthority, GetSidSubAuthorityCount, GetTokenInformation, IsTokenRestricted, - TOKEN_MANDATORY_LABEL, TOKEN_QUERY, TokenIntegrityLevel, TokenSessionId, - }; - use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; - - let mut token = HANDLE::default(); - if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) }.is_err() { - log::error!("diagnostic: could not open this process's own token"); - return; - } - - let mut label_buf = [0u8; 64]; - let mut ret_len = 0u32; - let got_label = unsafe { - GetTokenInformation( - token, - TokenIntegrityLevel, - Some(label_buf.as_mut_ptr() as *mut c_void), - label_buf.len() as u32, - &mut ret_len, - ) - }; - if got_label.is_ok() { - let label = unsafe { &*(label_buf.as_ptr() as *const TOKEN_MANDATORY_LABEL) }; - let sid = label.Label.Sid; - let rid = unsafe { - let count = *GetSidSubAuthorityCount(sid); - *GetSidSubAuthority(sid, (count - 1) as u32) - }; - let level = match rid { - 0x0000 => "Untrusted", - 0x1000 => "Low", - 0x2000 => "Medium", - 0x2100 => "Medium Plus", - 0x3000 => "High", - 0x4000 => "System", - _ => "Unknown", - }; - log::info!("diagnostic: token integrity level = {level} (rid {rid:#06x})"); - } else { - log::error!("diagnostic: could not read token integrity level"); - } - - let mut session_id: u32 = u32::MAX; - let mut ret_len2 = 0u32; - let got_session = unsafe { - GetTokenInformation( - token, - TokenSessionId, - Some(std::ptr::from_mut(&mut session_id).cast()), - size_of::() as u32, - &mut ret_len2, - ) - }; - if got_session.is_ok() { - log::info!("diagnostic: token session id = {session_id}"); - } else { - log::error!("diagnostic: could not read token session id"); - } - - // `IsTokenRestricted` reports via Result rather than a bool: Ok means the - // token carries a restricted-SIDs list (the raw BOOL was TRUE), Err means - // it does not. - let restricted = unsafe { IsTokenRestricted(token) }.is_ok(); - log::info!("diagnostic: IsTokenRestricted = {restricted}"); - - unsafe { - let _ = CloseHandle(token); - } -} - fn arg_value(flag: &str) -> Option { let mut args = std::env::args(); while let Some(a) = args.next() { @@ -275,9 +140,6 @@ fn main() { }; let cancel_pipe = arg_value("--cancel-pipe"); - diagnose_token(); - diagnose_process_singleton_mutex(); - let cache_path = browser_state_dir(Path::new(CACHE_ROOT)); let settings = Settings { From c84996157b8901638a7a0c09ca3136c7ac449070 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 11:28:57 +0100 Subject: [PATCH 33/41] ee/wcp: proxy ak-sysd calls through credprovider instead of granting ak_cef.exe pipe access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ak_cef.exe now runs as the unprivileged service account and has no access to ak-sysd's named pipe (created with no explicit DACL, so only its creator's identity can reach it). Rather than widen that pipe's ACL for one caller, move both ak-sysd calls into credprovider, which already runs as SYSTEM: it fetches the sign-in URL and header token before spawning ak_cef.exe (passed on the command line), and validates the redirect's token itself once ak_cef.exe reports the callback URL over the result pipe. ak_cef.exe's own job shrinks to opening a URL, injecting a header, and reporting what came back — it no longer touches ak-sysd at all. --- Cargo.lock | 3 +- ee/wcp/BROWSER_PRIVILEGE.md | 13 +++ ee/wcp/cef-host/Cargo.toml | 2 - ee/wcp/cef-host/src/app.rs | 78 +++++++++--------- ee/wcp/cef-host/src/handler.rs | 32 +++----- ee/wcp/cef-host/src/main.rs | 11 ++- ee/wcp/cef-host/src/sysd.rs | 105 ------------------------ ee/wcp/credprovider/src/ipc.rs | 61 ++++++++++++-- ee/wcp/credprovider/src/sysd.rs | 61 +++++++++++++- ee/wcp/wire/Cargo.toml | 1 + ee/wcp/wire/src/lib.rs | 137 ++++++++++++++++++++------------ 11 files changed, 273 insertions(+), 231 deletions(-) delete mode 100644 ee/wcp/cef-host/src/sysd.rs diff --git a/Cargo.lock b/Cargo.lock index 7c9eb51a..3adaf555 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -204,10 +204,8 @@ dependencies = [ "ak-meta", "ak-platform", "cef", - "eyre", "log", "sentry", - "url", "uuid", "windows 0.62.2", ] @@ -233,6 +231,7 @@ name = "ak-ee-wcp-wire" version = "0.60.1" dependencies = [ "prost", + "url", ] [[package]] diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 2ca65f67..12d47ddf 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -48,6 +48,14 @@ current code. - **CEF runtime style**: `RuntimeStyle::ALLOY`, not the Chrome-style default — this window needs no Chrome UI (tabs, extensions, profile manager). +- **`ak-sysd`**: `ak_cef.exe` no longer talks to it at all — the service + account has no access to its pipe, and granting that would widen a pipe + every other platform/consumer shares for the sake of one caller. + `credprovider` fetches the sign-in URL and header token before spawning + (passed on the command line) and validates the redirect's token once + `ak_cef.exe` reports it over the result pipe (`ipc::auth_result_for`) — + `ak_cef.exe`'s own job shrinks to "open this URL, inject this header, + report what came back." Previously, a fresh logon ran the browser as SYSTEM outright (no interactive-user token existed yet to duplicate) and unlock used the @@ -136,6 +144,11 @@ upstream-shaped work, independent of this branch. - **Deployment friction** — GPO blocking local account creation, endpoint monitoring flagging a new local account, no local accounts on a domain controller — isn't addressable from this codebase. +- **The `ak-sysd` start call is now serialized before the spawn** rather than + overlapping `ak_cef.exe`'s own startup — simpler, and the one thing + `credprovider` cannot get wrong is which process the `ak-sysd` pipe trusts, + but it does mean the window appears after both round trips added together + rather than the slower of the two. - **The account's own password validation is a no-op**: `RealSyscalls::validate` uses `LOGON32_LOGON_NETWORK`, but the account is denied network logon by design (`deny_interactive_and_network_logon`), so diff --git a/ee/wcp/cef-host/Cargo.toml b/ee/wcp/cef-host/Cargo.toml index 9b1e908a..7912da54 100644 --- a/ee/wcp/cef-host/Cargo.toml +++ b/ee/wcp/cef-host/Cargo.toml @@ -18,7 +18,5 @@ ak-platform = { path = "../../../ak-platform" } log = { workspace = true } sentry = { workspace = true } windows = { workspace = true } -eyre = { workspace = true } -url = { workspace = true } uuid = { workspace = true } cef = { version = "151.4.0", features = ["build-util"] } diff --git a/ee/wcp/cef-host/src/app.rs b/ee/wcp/cef-host/src/app.rs index 2cdecb0c..428563e0 100644 --- a/ee/wcp/cef-host/src/app.rs +++ b/ee/wcp/cef-host/src/app.rs @@ -1,5 +1,6 @@ -//! CEF app/browser-process handler: on context init, fetches the sign-in -//! URL and opens the browser window. +//! CEF app/browser-process handler: on context init, opens the sign-in +//! window at the URL `credprovider` already resolved before spawning this +//! process. use cef::*; @@ -10,11 +11,18 @@ wrap_app! { pub struct HostApp { result_pipe: String, cancel_pipe: Option, + sign_in_url: String, + header_token: String, } impl App { fn browser_process_handler(&self) -> Option { - Some(HostBrowserProcessHandler::new(self.result_pipe.clone(), self.cancel_pipe.clone())) + Some(HostBrowserProcessHandler::new( + self.result_pipe.clone(), + self.cancel_pipe.clone(), + self.sign_in_url.clone(), + self.header_token.clone(), + )) } } } @@ -23,6 +31,8 @@ wrap_browser_process_handler! { struct HostBrowserProcessHandler { result_pipe: String, cancel_pipe: Option, + sign_in_url: String, + header_token: String, } impl BrowserProcessHandler { @@ -31,10 +41,14 @@ wrap_browser_process_handler! { // frames below the window creation. The C++ never built the browser // there: it only flagged the context as ready and created the window // later, once the loop was pumping. Post the work instead of doing it - // re-entrantly, which also keeps a blocking gRPC call out of - // `CefInitialize`. + // re-entrantly. fn on_context_initialized(&self) { - let mut task = OpenSignInWindow::new(self.result_pipe.clone(), self.cancel_pipe.clone()); + let mut task = OpenSignInWindow::new( + self.result_pipe.clone(), + self.cancel_pipe.clone(), + self.sign_in_url.clone(), + self.header_token.clone(), + ); post_task(ThreadId::UI, Some(&mut task)); } } @@ -44,11 +58,18 @@ wrap_task! { struct OpenSignInWindow { result_pipe: String, cancel_pipe: Option, + sign_in_url: String, + header_token: String, } impl Task { fn execute(&self) { - open_sign_in_window(self.result_pipe.clone(), self.cancel_pipe.clone()); + open_sign_in_window( + self.result_pipe.clone(), + self.cancel_pipe.clone(), + self.sign_in_url.clone(), + self.header_token.clone(), + ); } } } @@ -67,39 +88,12 @@ wrap_browser_view_delegate! { } } -fn open_sign_in_window(result_pipe: String, cancel_pipe: Option) { - // Most of the gap between the spawn and a window existing is spent here — - // a named-pipe round trip to `ak-sysd` and, behind it, a live call out to - // the authentik API. That gap is what the foreground grant issued at spawn - // has to survive, so it is worth knowing how long it actually was. - let started = std::time::Instant::now(); - let start = match crate::sysd::sys_auth_start_async() { - Ok(s) => s, - Err(e) => { - log::error!("sys_auth_start_async failed: {e}"); - match connect_result_pipe(&result_pipe) { - Ok(mut pipe) => { - let _ = ak_ee_wcp_wire::write_auth_result( - &mut pipe, - &ak_ee_wcp_wire::AuthResult::Failed { - reason: e.to_string(), - }, - ); - } - Err(conn_err) => { - log::error!("could not connect the result pipe either: {conn_err}"); - } - } - quit_message_loop(); - return; - } - }; - - log::info!( - "got the sign-in URL after {}ms", - started.elapsed().as_millis() - ); - +fn open_sign_in_window( + result_pipe: String, + cancel_pipe: Option, + sign_in_url: String, + header_token: String, +) { let result_pipe = match connect_result_pipe(&result_pipe) { Ok(pipe) => pipe, Err(e) => { @@ -117,7 +111,7 @@ fn open_sign_in_window(result_pipe: String, cancel_pipe: Option) { None } }); - let inner = SignInHandler::new(start.header_token, result_pipe, cancel_pipe); + let inner = SignInHandler::new(header_token, result_pipe, cancel_pipe); let mut client = SignInClient::new(inner); let browser_settings = BrowserSettings::default(); @@ -143,7 +137,7 @@ fn open_sign_in_window(result_pipe: String, cancel_pipe: Option) { log::info!("creating the top-level window"); let mut delegate = SignInWindowDelegate::new( std::cell::RefCell::new(browser_view), - start.url, + sign_in_url, std::rc::Rc::new(std::cell::Cell::new(false)), ); window_create_top_level(Some(&mut delegate)); diff --git a/ee/wcp/cef-host/src/handler.rs b/ee/wcp/cef-host/src/handler.rs index 94976cb7..907cfb58 100644 --- a/ee/wcp/cef-host/src/handler.rs +++ b/ee/wcp/cef-host/src/handler.rs @@ -7,7 +7,7 @@ use std::io::Read; use std::os::windows::io::FromRawHandle; use std::sync::{Arc, Mutex, Weak}; -use ak_ee_wcp_wire::AuthResult; +use ak_ee_wcp_wire::HostReport; use cef::*; use windows::Win32::Foundation::{ERROR_PIPE_BUSY, GENERIC_READ, GENERIC_WRITE}; use windows::Win32::Storage::FileSystem::{ @@ -62,7 +62,7 @@ impl SignInHandler { } if self.browser_list.is_empty() { log::info!("last browser closed; shutting the sign-in window down"); - signal_if_unsent(&self.result_pipe, AuthResult::Cancelled); + signal_if_unsent(&self.result_pipe, HostReport::Cancelled); quit_message_loop(); } } @@ -76,9 +76,9 @@ impl SignInHandler { } /// Sends the sign-in outcome once, then closes the window. Safe to call - /// more than once — only the first call's result is sent. - fn complete(&self, result: AuthResult) { - signal_if_unsent(&self.result_pipe, result); + /// more than once — only the first call's report is sent. + fn complete(&self, report: HostReport) { + signal_if_unsent(&self.result_pipe, report); let Some(this) = self.weak_self.upgrade() else { return; }; @@ -96,10 +96,10 @@ fn close_all_browsers(handler: &Arc>) { inner.close_all_browsers(); } -fn signal_if_unsent(result_pipe: &Mutex>, result: AuthResult) { +fn signal_if_unsent(result_pipe: &Mutex>, report: HostReport) { let mut guard = result_pipe.lock().unwrap_or_else(|e| e.into_inner()); if let Some(mut file) = guard.take() - && let Err(e) = ak_ee_wcp_wire::write_auth_result(&mut file, &result) + && let Err(e) = ak_ee_wcp_wire::write_host_report(&mut file, &report) { log::error!("failed to write result to pipe: {e}"); } @@ -131,7 +131,7 @@ fn watch_cancel_pipe(mut pipe: File, handler: Arc>) { Err(e) => log::error!("control pipe read failed ({e}); cancelling"), } let inner = handler.lock().unwrap_or_else(|e| e.into_inner()); - inner.complete(AuthResult::Cancelled); + inner.complete(HostReport::Cancelled); }); } @@ -219,18 +219,10 @@ wrap_resource_request_handler! { return ReturnValue::CONTINUE; } - let result = match crate::sysd::sys_auth_url(&url) { - Ok(Some(token)) => AuthResult::Completed { - username: token.username, - }, - Ok(None) => AuthResult::Failed { - reason: "token validation failed".to_string(), - }, - Err(e) => AuthResult::Failed { - reason: e.to_string(), - }, - }; - inner.complete(result); + // Validating the token needs `ak-sysd`, which this process has no + // access to (`BROWSER_PRIVILEGE.md`) — `credprovider` does it once + // this reaches the result pipe. + inner.complete(HostReport::Redirected { url }); ReturnValue::CANCEL } } diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index aa2c1f79..34ea0351 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -13,7 +13,6 @@ mod app; mod foreground; mod handler; mod icon; -mod sysd; mod window; use std::path::Path; @@ -139,6 +138,14 @@ fn main() { return; }; let cancel_pipe = arg_value("--cancel-pipe"); + let Some(sign_in_url) = arg_value("--sign-in-url") else { + log::error!("missing --sign-in-url argument"); + return; + }; + let Some(header_token) = arg_value("--header-token") else { + log::error!("missing --header-token argument"); + return; + }; let cache_path = browser_state_dir(Path::new(CACHE_ROOT)); @@ -150,7 +157,7 @@ fn main() { log_severity: LogSeverity::VERBOSE, ..Default::default() }; - let mut app = app::HostApp::new(result_pipe, cancel_pipe); + let mut app = app::HostApp::new(result_pipe, cancel_pipe, sign_in_url, header_token); let initialized = initialize( Some(cef_args.as_main_args()), Some(&settings), diff --git a/ee/wcp/cef-host/src/sysd.rs b/ee/wcp/cef-host/src/sysd.rs deleted file mode 100644 index df26a366..00000000 --- a/ee/wcp/cef-host/src/sysd.rs +++ /dev/null @@ -1,105 +0,0 @@ -//! The two `ak-sysd` calls the browser host makes: start an interactive -//! sign-in (yielding the URL to open and the header token to inject), and -//! validate the token the `goauthentik.io://` redirect returns. - -use eyre::Result; -use std::collections::HashMap; -use url::Url; - -use ak_ee_wcp_wire::TOKEN_QUERY_PARAM; -use ak_platform::generated::sys_auth::TokenAuthRequest; -use ak_platform::generated::sys_auth::system_auth_interactive_client::SystemAuthInteractiveClient; -use ak_platform::generated::sys_auth::system_auth_token_client::SystemAuthTokenClient; -use ak_platform::grpc::grpc_request; - -pub struct AuthStartAsync { - pub url: String, - pub header_token: String, -} - -pub struct TokenResponse { - pub username: String, -} - -pub fn sys_auth_start_async() -> Result { - let response = grpc_request(async |ch| { - Ok(SystemAuthInteractiveClient::new(ch) - .interactive_auth_async(()) - .await?) - })? - .into_inner(); - Ok(AuthStartAsync { - url: response.url, - header_token: response.header_token, - }) -} - -pub fn sys_auth_url(url: &str) -> Result> { - let raw_token = extract_token(url)?; - sys_auth_token_validate(&raw_token) -} - -fn extract_token(url: &str) -> Result { - let parsed = Url::parse(url)?; - let qm: HashMap<_, _> = parsed.query_pairs().into_owned().collect(); - qm.get(TOKEN_QUERY_PARAM) - .cloned() - .ok_or_else(|| eyre::eyre!("failed to get token from URL")) -} - -fn sys_auth_token_validate(raw_token: &str) -> Result> { - let response = grpc_request(async |ch| { - Ok(SystemAuthTokenClient::new(ch) - .token_auth(TokenAuthRequest { - username: String::new(), - token: raw_token.to_owned(), - }) - .await?) - })? - .into_inner(); - - if !response.successful { - return Ok(None); - } - Ok(Some(TokenResponse { - username: response - .token - .map(|t| t.preferred_username) - .unwrap_or_default(), - })) -} - -#[cfg(test)] -#[allow(clippy::unwrap_used)] -mod tests { - use super::*; - - #[test] - fn extracts_token_from_a_redirect_url() { - let url = format!( - "{}callback?{TOKEN_QUERY_PARAM}=abc123", - ak_ee_wcp_wire::REDIRECT_PREFIX - ); - assert_eq!(extract_token(&url).unwrap(), "abc123"); - } - - #[test] - fn extracts_token_alongside_other_query_params() { - let url = format!( - "{}callback?state=xyz&{TOKEN_QUERY_PARAM}=abc123&code=9", - ak_ee_wcp_wire::REDIRECT_PREFIX - ); - assert_eq!(extract_token(&url).unwrap(), "abc123"); - } - - #[test] - fn errors_when_the_token_param_is_absent() { - let url = format!("{}callback?state=xyz", ak_ee_wcp_wire::REDIRECT_PREFIX); - assert!(extract_token(&url).is_err()); - } - - #[test] - fn errors_on_an_unparseable_url() { - assert!(extract_token("not a url").is_err()); - } -} diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index e71aab9f..6de516c3 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -42,7 +42,8 @@ use windows::{ }; use crate::syscalls::{self, ForegroundControl}; -use ak_ee_wcp_wire::AuthResult; +use crate::sysd; +use ak_ee_wcp_wire::{AuthResult, HostReport}; /// Spawns `ak_cef.exe` and waits for its result. `should_continue` is polled /// while waiting, so LogonUI cancelling (the user backing out of the tile) @@ -277,6 +278,21 @@ fn run_cef_host( cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO, should_continue: &mut dyn FnMut() -> bool, ) -> AuthResult { + // Fetched here, not by `ak_cef.exe` itself: the service account it runs + // as has no access to `ak-sysd`'s pipe (`BROWSER_PRIVILEGE.md`). Doing + // this before the pipes/spawn also means a failure here costs nothing + // beyond the round trip itself, rather than a spawned window that can + // never load anything. + let start = match sysd::sys_auth_start_async() { + Ok(s) => s, + Err(e) => { + log::error!("sys_auth_start_async failed: {e}"); + return AuthResult::Failed { + reason: e.to_string(), + }; + } + }; + let connecting_sid = if may_launch_in_current_session(cpus) { None } else { @@ -303,7 +319,7 @@ fn run_cef_host( } }; - let spawn = spawn_cef_host(cef_exe, &pipes, cpus); + let spawn = spawn_cef_host(cef_exe, &pipes, cpus, &start.url, &start.header_token); let process = match spawn { Ok(p) => p, Err(e) => { @@ -435,6 +451,23 @@ enum PipeOutcome { Error(String), } +/// Turns the sign-in redirect's URL into a real outcome by validating its +/// token against `ak-sysd` — the one step `ak_cef.exe` cannot do itself +/// (`BROWSER_PRIVILEGE.md`). Runs on the result-pipe reader thread, not the +/// thread LogonUI called `Connect` on, so this blocking round trip does not +/// stall `should_continue` polling or the foreground nudge. +fn auth_result_for(url: &str) -> AuthResult { + match sysd::sys_auth_validate(url) { + Ok(Some(username)) => AuthResult::Completed { username }, + Ok(None) => AuthResult::Failed { + reason: "token validation failed".to_string(), + }, + Err(e) => AuthResult::Failed { + reason: e.to_string(), + }, + } +} + /// Polls in short slices so `should_continue` gets a turn. On cancellation it /// asks `ak_cef.exe` to close over the control pipe rather than killing it. /// @@ -451,8 +484,9 @@ fn wait_for_result( let mut result_file = unsafe { File::from_raw_handle(result_read.0) }; let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { - let outcome = match ak_ee_wcp_wire::read_auth_result(&mut result_file) { - Ok(Some(result)) => PipeOutcome::Result(result), + let outcome = match ak_ee_wcp_wire::read_host_report(&mut result_file) { + Ok(Some(HostReport::Redirected { url })) => PipeOutcome::Result(auth_result_for(&url)), + Ok(Some(HostReport::Cancelled)) => PipeOutcome::Result(AuthResult::Cancelled), Ok(None) => PipeOutcome::Eof, Err(e) => PipeOutcome::Error(e.to_string()), }; @@ -555,16 +589,29 @@ fn acquire_service_account_token() -> windows::core::Result { syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME, &password) } +/// Minimal Windows command-line quoting: wraps `s` in quotes and escapes any +/// embedded ones, so `CommandLineToArgvW` (what `std::env::args()` on the +/// far end is built on) sees it as a single argument. Neither a URL nor an +/// opaque token legitimately contains the backslash-before-quote sequence +/// the full algorithm exists to handle. +fn quote_arg(s: &str) -> String { + format!("\"{}\"", s.replace('"', "\\\"")) +} + fn spawn_cef_host( cef_exe: &Path, pipes: &DuplexPipes, cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO, + sign_in_url: &str, + header_token: &str, ) -> windows::core::Result { let cmdline = format!( - "\"{}\" --result-pipe {} --cancel-pipe {}", + "\"{}\" --result-pipe {} --cancel-pipe {} --sign-in-url {} --header-token {}", cef_exe.display(), pipes.result_name, - pipes.cancel_name + pipes.cancel_name, + quote_arg(sign_in_url), + quote_arg(header_token), ); let mut si = STARTUPINFOW { @@ -916,7 +963,7 @@ mod tests { std::env::var("COMSPEC").unwrap_or_else(|_| r"C:\Windows\System32\cmd.exe".to_string()), ); - let spawned = spawn_cef_host(&exe, &pipes, CPUS_CREDUI); + let spawned = spawn_cef_host(&exe, &pipes, CPUS_CREDUI, "https://example.com", "token"); unsafe { let _ = CloseHandle(pipes.result_server); diff --git a/ee/wcp/credprovider/src/sysd.rs b/ee/wcp/credprovider/src/sysd.rs index 2989992e..3c8c88e9 100644 --- a/ee/wcp/credprovider/src/sysd.rs +++ b/ee/wcp/credprovider/src/sysd.rs @@ -1,5 +1,9 @@ -//! The one `ak-sysd` call this DLL makes. Cached in HKLM so -//! `SetUsageScenario` doesn't need the daemon on every logon-screen paint. +//! The `ak-sysd` calls this DLL makes. `sys_caps` is cached in HKLM so +//! `SetUsageScenario` doesn't need the daemon on every logon-screen paint; +//! `sys_auth_start_async`/`sys_auth_validate` are the interactive sign-in +//! calls `ak_cef.exe` used to make itself, moved here because the service +//! account it now runs as has no access to `ak-sysd`'s pipe +//! (`BROWSER_PRIVILEGE.md`). use eyre::Result; use serde::{Deserialize, Serialize}; @@ -7,6 +11,9 @@ use winreg::enums::HKEY_LOCAL_MACHINE; use ak_platform::generated::ping::capabilities_response::Capability; use ak_platform::generated::ping::ping_client::PingClient; +use ak_platform::generated::sys_auth::TokenAuthRequest; +use ak_platform::generated::sys_auth::system_auth_interactive_client::SystemAuthInteractiveClient; +use ak_platform::generated::sys_auth::system_auth_token_client::SystemAuthTokenClient; use ak_platform::grpc::grpc_request; /// `ak_ee_wcp_e2e::harness` seeds this same key to turn on `debug`; keep the name and @@ -37,3 +44,53 @@ pub fn sys_caps() -> Result { key.encode(&caps)?; Ok(caps) } + +pub struct AuthStartAsync { + pub url: String, + pub header_token: String, +} + +/// Starts an interactive sign-in: `url` is what `ak_cef.exe` opens, and +/// `header_token` is what it injects on every request that page makes, so +/// the backend can tie them back to this one session. +pub fn sys_auth_start_async() -> Result { + let response = grpc_request(async |ch| { + Ok(SystemAuthInteractiveClient::new(ch) + .interactive_auth_async(()) + .await?) + })? + .into_inner(); + Ok(AuthStartAsync { + url: response.url, + header_token: response.header_token, + }) +} + +/// Validates the token embedded in the sign-in redirect's URL, returning +/// the username on success. `None` covers both an unextractable token and +/// one `ak-sysd` rejects — either way the sign-in did not complete. +pub fn sys_auth_validate(url: &str) -> Result> { + let Some(raw_token) = ak_ee_wcp_wire::extract_token(url) else { + return Ok(None); + }; + + let response = grpc_request(async |ch| { + Ok(SystemAuthTokenClient::new(ch) + .token_auth(TokenAuthRequest { + username: String::new(), + token: raw_token.clone(), + }) + .await?) + })? + .into_inner(); + + if !response.successful { + return Ok(None); + } + Ok(Some( + response + .token + .map(|t| t.preferred_username) + .unwrap_or_default(), + )) +} diff --git a/ee/wcp/wire/Cargo.toml b/ee/wcp/wire/Cargo.toml index 67e09445..6436eeba 100644 --- a/ee/wcp/wire/Cargo.toml +++ b/ee/wcp/wire/Cargo.toml @@ -9,3 +9,4 @@ workspace = true [dependencies] prost = "0.14" +url = { workspace = true } diff --git a/ee/wcp/wire/src/lib.rs b/ee/wcp/wire/src/lib.rs index 5564f36a..a73bdc89 100644 --- a/ee/wcp/wire/src/lib.rs +++ b/ee/wcp/wire/src/lib.rs @@ -4,9 +4,10 @@ use std::io::{self, Read, Write}; -/// Result of the browser sign-in flow, sent from `cef-host` to `credprovider` -/// over the result pipe. The public shape stays a plain enum; wire encoding -/// goes through `AuthResultProto` below. +/// Outcome `credprovider` hands back from the sign-in flow. Built entirely +/// on the `credprovider` side of the pipe (from a [`HostReport`] plus, for +/// `Redirected`, a validation call to `ak-sysd` that only `credprovider` can +/// reach) — never sent over the wire itself. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AuthResult { Completed { username: String }, @@ -14,43 +15,53 @@ pub enum AuthResult { Failed { reason: String }, } +/// Sent from `cef-host` to `credprovider` over the result pipe once the +/// sign-in flow reaches an end state `cef-host` cannot itself resolve: +/// validating the redirect's token needs `ak-sysd`, which only +/// `credprovider` has access to (`BROWSER_PRIVILEGE.md`). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum HostReport { + /// The sign-in redirect fired; here is the full callback URL to extract + /// and validate the token from. + Redirected { url: String }, + /// The window closed — or the provider asked it to — without ever + /// reaching the redirect. + Cancelled, +} + #[derive(Clone, PartialEq, prost::Message)] -struct AuthResultProto { - #[prost(oneof = "AuthOutcome", tags = "1, 2, 3")] - outcome: Option, +struct HostReportProto { + #[prost(oneof = "HostOutcome", tags = "1, 2")] + outcome: Option, } #[derive(Clone, PartialEq, prost::Oneof)] -enum AuthOutcome { +enum HostOutcome { #[prost(string, tag = "1")] - Completed(String), + Redirected(String), #[prost(bool, tag = "2")] Cancelled(bool), - #[prost(string, tag = "3")] - Failed(String), } -impl From<&AuthResult> for AuthResultProto { - fn from(r: &AuthResult) -> Self { +impl From<&HostReport> for HostReportProto { + fn from(r: &HostReport) -> Self { let outcome = match r { - AuthResult::Completed { username } => AuthOutcome::Completed(username.clone()), - AuthResult::Cancelled => AuthOutcome::Cancelled(true), - AuthResult::Failed { reason } => AuthOutcome::Failed(reason.clone()), + HostReport::Redirected { url } => HostOutcome::Redirected(url.clone()), + HostReport::Cancelled => HostOutcome::Cancelled(true), }; - AuthResultProto { + HostReportProto { outcome: Some(outcome), } } } -impl TryFrom for AuthResult { +impl TryFrom for HostReport { type Error = WireError; - fn try_from(p: AuthResultProto) -> Result { + fn try_from(p: HostReportProto) -> Result { match p.outcome { - Some(AuthOutcome::Completed(username)) => Ok(AuthResult::Completed { username }), - Some(AuthOutcome::Cancelled(_)) => Ok(AuthResult::Cancelled), - Some(AuthOutcome::Failed(reason)) => Ok(AuthResult::Failed { reason }), + Some(HostOutcome::Redirected(url)) => Ok(HostReport::Redirected { url }), + Some(HostOutcome::Cancelled(_)) => Ok(HostReport::Cancelled), None => Err(WireError::MissingOutcome), } } @@ -87,7 +98,7 @@ impl std::fmt::Display for WireError { WireError::Io(e) => write!(f, "pipe I/O error: {e}"), WireError::Decoding(e) => write!(f, "frame decoding error: {e}"), WireError::FrameTooLarge(n) => write!(f, "frame of {n} bytes exceeds limit"), - WireError::MissingOutcome => write!(f, "AuthResult frame had no outcome set"), + WireError::MissingOutcome => write!(f, "HostReport frame had no outcome set"), } } } @@ -143,20 +154,32 @@ pub fn read_frame(r: &mut R) -> Result(w: &mut W, result: &AuthResult) -> Result<(), WireError> { - write_frame(w, &AuthResultProto::from(result)) +/// Write a `HostReport` over the result pipe. +pub fn write_host_report(w: &mut W, report: &HostReport) -> Result<(), WireError> { + write_frame(w, &HostReportProto::from(report)) } -/// Read an `AuthResult` from the result pipe. See [`read_frame`] for EOF +/// Read a `HostReport` from the result pipe. See [`read_frame`] for EOF /// handling. -pub fn read_auth_result(r: &mut R) -> Result, WireError> { - match read_frame::(r)? { - Some(proto) => Ok(Some(AuthResult::try_from(proto)?)), +pub fn read_host_report(r: &mut R) -> Result, WireError> { + match read_frame::(r)? { + Some(proto) => Ok(Some(HostReport::try_from(proto)?)), None => Ok(None), } } +/// Pulls the interactive-auth token out of the sign-in redirect's query +/// string. `None` covers both an unparseable URL and a well-formed one +/// missing the parameter — `credprovider` treats either the same way, as a +/// failed validation. +pub fn extract_token(url: &str) -> Option { + let parsed = url::Url::parse(url).ok()?; + parsed + .query_pairs() + .find(|(k, _)| k == TOKEN_QUERY_PARAM) + .map(|(_, v)| v.into_owned()) +} + /// The four credential-provider tile fields, in display order. Field IDs are /// their index in this slice. pub const TILE_FIELDS: &[TileField] = &[ @@ -213,38 +236,31 @@ mod tests { use super::*; #[test] - fn round_trips_completed_through_a_stream() { - let msg = AuthResult::Completed { - username: "jdoe".to_string(), + fn round_trips_redirected_through_a_stream() { + let msg = HostReport::Redirected { + url: format!("{}callback?state=xyz", REDIRECT_PREFIX), }; let mut buf = Vec::new(); - write_auth_result(&mut buf, &msg).unwrap(); + write_host_report(&mut buf, &msg).unwrap(); let mut cursor = io::Cursor::new(buf); - let decoded = read_auth_result(&mut cursor).unwrap().unwrap(); + let decoded = read_host_report(&mut cursor).unwrap().unwrap(); assert_eq!(msg, decoded); } #[test] - fn round_trips_cancelled_and_failed() { - for msg in [ - AuthResult::Cancelled, - AuthResult::Failed { - reason: "token validation failed".to_string(), - }, - ] { - let mut buf = Vec::new(); - write_auth_result(&mut buf, &msg).unwrap(); - let mut cursor = io::Cursor::new(buf); - let decoded = read_auth_result(&mut cursor).unwrap().unwrap(); - assert_eq!(msg, decoded); - } + fn round_trips_cancelled() { + let mut buf = Vec::new(); + write_host_report(&mut buf, &HostReport::Cancelled).unwrap(); + let mut cursor = io::Cursor::new(buf); + let decoded = read_host_report(&mut cursor).unwrap().unwrap(); + assert_eq!(HostReport::Cancelled, decoded); } #[test] fn read_frame_reports_clean_eof_as_none() { let mut cursor = io::Cursor::new(Vec::::new()); - let decoded = read_auth_result(&mut cursor).unwrap(); + let decoded = read_host_report(&mut cursor).unwrap(); assert!(decoded.is_none()); } @@ -253,10 +269,33 @@ mod tests { let mut buf = Vec::new(); buf.extend_from_slice(&(MAX_FRAME_BYTES + 1).to_le_bytes()); let mut cursor = io::Cursor::new(buf); - let result = read_auth_result(&mut cursor); + let result = read_host_report(&mut cursor); assert!(matches!(result, Err(WireError::FrameTooLarge(_)))); } + #[test] + fn extracts_token_from_a_redirect_url() { + let url = format!("{REDIRECT_PREFIX}callback?{TOKEN_QUERY_PARAM}=abc123"); + assert_eq!(extract_token(&url), Some("abc123".to_string())); + } + + #[test] + fn extracts_token_alongside_other_query_params() { + let url = format!("{REDIRECT_PREFIX}callback?state=xyz&{TOKEN_QUERY_PARAM}=abc123&code=9"); + assert_eq!(extract_token(&url), Some("abc123".to_string())); + } + + #[test] + fn extract_token_is_none_when_the_param_is_absent() { + let url = format!("{REDIRECT_PREFIX}callback?state=xyz"); + assert_eq!(extract_token(&url), None); + } + + #[test] + fn extract_token_is_none_for_an_unparseable_url() { + assert_eq!(extract_token("not a url"), None); + } + #[test] fn cancel_signal_round_trips() { let mut buf = Vec::new(); From 785f61d8c9872b362fefa05399065504dc58e269 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 12:31:38 +0100 Subject: [PATCH 34/41] ee/wcp: log the pipe DACL grant and the connecting token's own identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The result-pipe connect (Access is denied, 0x80070005) is the first time a real service-account token has actually reached this code path — every prior attempt died further upstream. Log both sides of the grant so the next failure shows whether the SID credprovider granted matches the SID ak_cef.exe is actually running as, rather than needing a second correlated capture to find out. --- ee/wcp/cef-host/src/identity.rs | 88 +++++++++++++++++++++++++++++++++ ee/wcp/cef-host/src/main.rs | 6 ++- ee/wcp/credprovider/src/ipc.rs | 5 ++ 3 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 ee/wcp/cef-host/src/identity.rs diff --git a/ee/wcp/cef-host/src/identity.rs b/ee/wcp/cef-host/src/identity.rs new file mode 100644 index 00000000..f863f4dc --- /dev/null +++ b/ee/wcp/cef-host/src/identity.rs @@ -0,0 +1,88 @@ +//! One diagnostic: which account and SID this process's own token actually +//! carries. Logged unconditionally at startup rather than only on an ACL +//! failure — the pipe DACL `credprovider` grants is keyed on this exact SID +//! string, so having both sides in the log is what turns "access denied" into +//! either "the grant is wrong" or "the grant is right and something else is +//! going on" without another slow rebuild-and-retest round trip. + +use std::ffi::c_void; + +use windows::Win32::Foundation::{CloseHandle, HANDLE}; +use windows::Win32::Security::Authorization::ConvertSidToStringSidW; +use windows::Win32::Security::{ + GetTokenInformation, LookupAccountSidW, SID_NAME_USE, TOKEN_QUERY, TOKEN_USER, TokenUser, +}; +use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; +use windows::core::{PCWSTR, PWSTR}; + +/// `"DOMAIN\name (S-1-5-...)"`, or a placeholder describing which step +/// failed — every step here can fail independently, and which one did says +/// something different about what is actually running. +pub fn current_token_identity() -> String { + unsafe { + let mut token = HANDLE::default(); + if OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token).is_err() { + return "".to_string(); + } + + let mut buf = [0u8; 256]; + let mut ret_len = 0u32; + let got_user = GetTokenInformation( + token, + TokenUser, + Some(buf.as_mut_ptr() as *mut c_void), + buf.len() as u32, + &mut ret_len, + ); + + let identity = if got_user.is_ok() { + let sid = (*(buf.as_ptr() as *const TOKEN_USER)).User.Sid; + + let mut name = [0u16; 256]; + let mut name_len = name.len() as u32; + let mut domain = [0u16; 256]; + let mut domain_len = domain.len() as u32; + let mut use_ = SID_NAME_USE::default(); + let account = if LookupAccountSidW( + PCWSTR::null(), + sid, + Some(PWSTR(name.as_mut_ptr())), + &mut name_len, + Some(PWSTR(domain.as_mut_ptr())), + &mut domain_len, + &mut use_, + ) + .is_ok() + { + format!( + "{}\\{}", + String::from_utf16_lossy(&domain[..domain_len as usize]), + String::from_utf16_lossy(&name[..name_len as usize]) + ) + } else { + "".to_string() + }; + + let sid_string = { + let mut wide_sid = PWSTR(std::ptr::null_mut()); + if ConvertSidToStringSidW(sid, &mut wide_sid).is_ok() { + let len = (0..).take_while(|&i| *wide_sid.0.add(i) != 0).count(); + let s = String::from_utf16_lossy(std::slice::from_raw_parts(wide_sid.0, len)); + let _ = windows::Win32::Foundation::LocalFree(Some( + windows::Win32::Foundation::HLOCAL(wide_sid.0 as *mut c_void), + )); + s + } else { + "".to_string() + } + }; + + format!("{account} ({sid_string})") + } else { + "".to_string() + }; + + let _ = CloseHandle(token); + identity + } +} diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index 34ea0351..0567c3da 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -13,6 +13,7 @@ mod app; mod foreground; mod handler; mod icon; +mod identity; mod window; use std::path::Path; @@ -109,9 +110,10 @@ fn main() { // binary was built from, so a real-install log can be matched against // the source rather than assumed. log::info!( - "ak_cef.exe {} (build {})", + "ak_cef.exe {} (build {}), running as {}", ak_meta::full_version(), - ak_meta::build_hash() + ak_meta::build_hash(), + identity::current_token_identity() ); let _ = api_hash(sys::CEF_API_VERSION_LAST, 0); diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 6de516c3..bb201def 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -110,6 +110,11 @@ struct PipeSecurityDescriptor(PSECURITY_DESCRIPTOR); impl PipeSecurityDescriptor { fn new(sid: &str, access: &str) -> windows::core::Result { let sddl = format!("D:(A;;GA;;;SY)(A;;{access};;;{sid})"); + // This exact string is what the connecting process's SID has to match + // — logged unconditionally rather than only on a connect failure, so + // a failure on the far end can be diagnosed from this process's own + // log without a second, correlated capture. + log::info!("granting pipe access via {sddl}"); let sddl_wide: Vec = sddl.encode_utf16().chain(std::iter::once(0)).collect(); let mut sd = PSECURITY_DESCRIPTOR::default(); unsafe { From e8245bd73e7cc5edbc9b2ec361bf118ced0bd52c Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 14:21:49 +0100 Subject: [PATCH 35/41] ee/wcp: grant the pipe SIDs object-specific rights instead of generic ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SID granted access matched the connecting token's own SID exactly (both S-1-5-21-...-1031), yet CreateFileW(GENERIC_WRITE) still failed with ACCESS_DENIED — ruling out an identity mismatch. SDDL's GR/GW/GA letters land in the ACE as the literal, unmapped GENERIC_* bit, but CreateFileW's GENERIC_WRITE request gets mapped to FILE_GENERIC_WRITE before the access check runs; the two share no bits, so the check fails regardless of SID. Building the ACE from FILE_GENERIC_READ/FILE_GENERIC_WRITE directly sidesteps the mismatch instead of relying on a mapping step that never happens. --- ee/wcp/credprovider/src/ipc.rs | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index bb201def..913859e9 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -26,7 +26,8 @@ use windows::{ PSECURITY_DESCRIPTOR, SE_IMPERSONATE_NAME, SECURITY_ATTRIBUTES, }, Storage::FileSystem::{ - FILE_FLAGS_AND_ATTRIBUTES, PIPE_ACCESS_INBOUND, PIPE_ACCESS_OUTBOUND, + FILE_ALL_ACCESS, FILE_FLAGS_AND_ATTRIBUTES, FILE_GENERIC_READ, FILE_GENERIC_WRITE, + PIPE_ACCESS_INBOUND, PIPE_ACCESS_OUTBOUND, }, System::Environment::{CreateEnvironmentBlock, DestroyEnvironmentBlock}, System::Pipes::{ConnectNamedPipe, CreateNamedPipeW, NAMED_PIPE_MODE}, @@ -104,12 +105,25 @@ struct DuplexPipes { /// the only other identity that will ever try to open the pipe — the one /// direction it needs. A named pipe created with no explicit DACL is /// reachable only by its creator's own identity, which the service account -/// is not. `access` is an SDDL generic-rights token: `"GR"` or `"GW"`. +/// is not. +/// +/// `access` must be an already object-specific mask (`FILE_GENERIC_READ`/ +/// `FILE_GENERIC_WRITE`), not a bare `GENERIC_READ`/`GENERIC_WRITE` — SDDL's +/// `GR`/`GW`/`GA` letters land in the ACE as the literal, unmapped +/// `GENERIC_*` bit. `CreateFileW(GENERIC_WRITE)` gets that *request* mapped +/// to `FILE_GENERIC_WRITE` before the check, but the ACE is never put +/// through the same mapping, so a raw `GENERIC_WRITE` ACE and a +/// `GENERIC_WRITE` request share no bits and the check fails even though the +/// SID matches exactly. Passing the mapped mask up front sidesteps the +/// mismatch entirely. struct PipeSecurityDescriptor(PSECURITY_DESCRIPTOR); impl PipeSecurityDescriptor { - fn new(sid: &str, access: &str) -> windows::core::Result { - let sddl = format!("D:(A;;GA;;;SY)(A;;{access};;;{sid})"); + fn new(sid: &str, access: u32) -> windows::core::Result { + let sddl = format!( + "D:(A;;{:#x};;;SY)(A;;{access:#x};;;{sid})", + FILE_ALL_ACCESS.0 + ); // This exact string is what the connecting process's SID has to match // — logged unconditionally rather than only on a connect failure, so // a failure on the far end can be diagnosed from this process's own @@ -184,10 +198,10 @@ fn create_duplex_pipes(connecting_sid: Option<&str>) -> windows::core::Result Date: Thu, 20 Aug 2026 15:26:00 +0100 Subject: [PATCH 36/41] ee/wcp: log the pipe's DACL as the kernel actually stored it Both sides of the grant already match (same SID, logged separately) and the explicit-mask fix didn't change the outcome, so the next thing worth ruling out is whether the DACL handed to CreateNamedPipeW is what actually ended up on the object, rather than guessing further blind. --- ee/wcp/credprovider/src/ipc.rs | 58 ++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 913859e9..85fc2324 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -20,10 +20,14 @@ use windows::{ LocalFree, WAIT_OBJECT_0, }, Security::{ + ACL, Authorization::{ - ConvertStringSecurityDescriptorToSecurityDescriptorW, SDDL_REVISION_1, + ConvertSecurityDescriptorToStringSecurityDescriptorW, + ConvertStringSecurityDescriptorToSecurityDescriptorW, GetSecurityInfo, + SDDL_REVISION_1, SE_FILE_OBJECT, }, - PSECURITY_DESCRIPTOR, SE_IMPERSONATE_NAME, SECURITY_ATTRIBUTES, + DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, SE_IMPERSONATE_NAME, + SECURITY_ATTRIBUTES, }, Storage::FileSystem::{ FILE_ALL_ACCESS, FILE_FLAGS_AND_ATTRIBUTES, FILE_GENERIC_READ, FILE_GENERIC_WRITE, @@ -179,9 +183,59 @@ fn create_named_pipe( windows::core::HRESULT::from_win32(unsafe { GetLastError().0 }), )); } + if sd.is_some() { + log_effective_dacl(handle); + } Ok(handle) } +/// Reads back the DACL the kernel actually attached to `handle`, rather than +/// trusting that what was passed to `CreateNamedPipeW` is what stuck — the +/// two SIDs already match exactly (logged separately, on each side of the +/// pipe) and the connect still fails, so the next thing worth ruling out is +/// whether the grant even survived pipe creation at all. +fn log_effective_dacl(handle: HANDLE) { + unsafe { + let mut dacl: *mut ACL = std::ptr::null_mut(); + let mut sd = PSECURITY_DESCRIPTOR::default(); + if GetSecurityInfo( + handle, + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + None, + None, + Some(&mut dacl), + None, + Some(&mut sd), + ) + .is_err() + { + log::warn!("could not read back the pipe's own DACL"); + return; + } + + let mut string_sd = PWSTR::null(); + if ConvertSecurityDescriptorToStringSecurityDescriptorW( + sd, + SDDL_REVISION_1, + DACL_SECURITY_INFORMATION, + &mut string_sd, + None, + ) + .is_ok() + { + let len = (0..).take_while(|&i| *string_sd.0.add(i) != 0).count(); + let rendered = String::from_utf16_lossy(std::slice::from_raw_parts(string_sd.0, len)); + log::info!("pipe's actual DACL is {rendered}"); + let _ = LocalFree(Some(HLOCAL(string_sd.0 as *mut c_void))); + } else { + log::warn!("could not render the pipe's DACL back to a string"); + } + + let _ = LocalFree(Some(HLOCAL(sd.0))); + } +} + /// One named pipe pair, one instance each, scoped to `connecting_sid` — the /// service account for the real logon scenarios, or `None` under /// `CPUS_CREDUI`, whose child inherits the caller's own (not necessarily From 5e02d7f82fe14c705f1d49122f54ed339eea271b Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 16:16:25 +0100 Subject: [PATCH 37/41] ee/wcp: grant the service account access to the named-pipe namespace itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-pipe DACL is confirmed correct — read back and logged as exactly what was granted, same SID as the connecting token — yet the connect still fails. The remaining candidate is a device-level check on the Named Pipe File System's own namespace object (\.\pipe\), consulted before NPFS ever reaches an individual pipe's own DACL: the same two-tier shape as the BaseNamedObjects hardening already found and fixed on this box, just for a different Object Manager namespace. --- ee/wcp/credprovider/src/ipc.rs | 3 ++ ee/wcp/credprovider/src/syscalls.rs | 76 +++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 85fc2324..d7f73a7c 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -658,6 +658,9 @@ fn acquire_service_account_token() -> windows::core::Result { if let Err(e) = syscalls::ensure_base_named_objects_access(&sid) { log::warn!("could not grant the service account BaseNamedObjects access: {e}"); } + if let Err(e) = syscalls::ensure_named_pipe_namespace_access(&sid) { + log::warn!("could not grant the service account named-pipe namespace access: {e}"); + } syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME, &password) } diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index fe853754..c8ce582f 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -25,9 +25,10 @@ use windows::{ LsaLookupAuthenticationPackage, LsaOpenPolicy, POLICY_CREATE_ACCOUNT, }, Authorization::{ - ConvertSidToStringSidW, EXPLICIT_ACCESS_W, GetSecurityInfo, NO_MULTIPLE_TRUSTEE, - SE_OBJECT_TYPE, SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, - TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, + ConvertSidToStringSidW, EXPLICIT_ACCESS_W, GetNamedSecurityInfoW, GetSecurityInfo, + NO_MULTIPLE_TRUSTEE, SE_FILE_OBJECT, SE_OBJECT_TYPE, SE_WINDOW_OBJECT, SET_ACCESS, + SetEntriesInAclW, SetNamedSecurityInfoW, SetSecurityInfo, TRUSTEE_IS_SID, + TRUSTEE_IS_USER, TRUSTEE_W, }, CreateRestrictedToken, DACL_SECURITY_INFORMATION, DISABLE_MAX_PRIVILEGE, GetTokenInformation, LOGON32_LOGON_NETWORK, LOGON32_LOGON_SERVICE, @@ -36,7 +37,7 @@ use windows::{ SE_PRIVILEGE_ENABLED, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, TOKEN_PRIVILEGES, TOKEN_QUERY, TOKEN_USER, TokenUser, }, - Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, + Storage::FileSystem::{FILE_ALL_ACCESS, READ_CONTROL, WRITE_DAC}, System::StationsAndDesktops::{ DESKTOP_CONTROL_FLAGS, GetProcessWindowStation, OpenDesktopW, }, @@ -676,6 +677,73 @@ pub fn ensure_base_named_objects_access(sid: &[u8]) -> windows::core::Result<()> } } +/// Grants the service account's SID access to the Named Pipe File System's +/// own namespace object (`\\.\pipe\`) — separate from, and checked before, +/// any individual pipe's own DACL. Confirmed via a read-back +/// (`ipc::log_effective_dacl`) that the per-pipe grant on the result/cancel +/// pipes is exactly right and the SID matches the connecting token exactly, +/// yet the connect still fails — the remaining candidate is this +/// device-level check, the same two-tier shape as `BaseNamedObjects` +/// hardening (`ensure_base_named_objects_access`) just for a different +/// namespace. `FILE_ALL_ACCESS` rather than a narrower mask because it is +/// not yet known which specific right this device-level check wants; narrow +/// once that's confirmed (see `BROWSER_PRIVILEGE.md`'s "Known gaps"). +pub fn ensure_named_pipe_namespace_access(sid: &[u8]) -> windows::core::Result<()> { + let path = wide(r"\\.\pipe\"); + unsafe { + let mut old_dacl: *mut ACL = std::ptr::null_mut(); + let mut sd = PSECURITY_DESCRIPTOR::default(); + GetNamedSecurityInfoW( + PCWSTR(path.as_ptr()), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + None, + None, + Some(&mut old_dacl), + None, + &mut sd, + ) + .ok()?; + + let trustee = TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: NO_MULTIPLE_TRUSTEE, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_USER, + ptstrName: PWSTR(sid.as_ptr() as *mut u16), + }; + let entry = EXPLICIT_ACCESS_W { + grfAccessPermissions: FILE_ALL_ACCESS.0, + grfAccessMode: SET_ACCESS, + grfInheritance: NO_INHERITANCE, + Trustee: trustee, + }; + + let mut new_dacl: *mut ACL = std::ptr::null_mut(); + let entries_result = SetEntriesInAclW(Some(&[entry]), Some(old_dacl), &mut new_dacl); + let set_result = if entries_result.is_ok() { + SetNamedSecurityInfoW( + PCWSTR(path.as_ptr()), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + None, + None, + Some(new_dacl), + None, + ) + } else { + entries_result + }; + + let _ = LocalFree(Some(HLOCAL(sd.0))); + if !new_dacl.is_null() { + let _ = LocalFree(Some(HLOCAL(new_dacl as *mut std::ffi::c_void))); + } + + set_result.ok() + } +} + /// Denies the service account the logon types that would let it sign someone /// in — it must not be usable at the very screen it serves. `Service`, what /// `service_account_token` uses (`LOGON32_LOGON_SERVICE`), is deliberately From 0f2e690a89cc3bda3f5e67b2d51ef8a7bf5229e8 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Thu, 20 Aug 2026 23:45:43 +0100 Subject: [PATCH 38/41] ee/wcp: replace named-pipe DACLs with inherited handles, copying GCPW MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every layer of the named-pipe DACL checked out exactly right — SID match, correct rights, DACL confirmed via read-back — yet ak_cef.exe still got ACCESS_DENIED opening it by name, and widening the grant to the Named Pipe File System's own namespace object didn't help either. GCPW never opens a named pipe by path from its low-privilege side at all (CreatePipeForChildProcess, gcp_utils.cc): it opens both ends itself while still SYSTEM and lets the child inherit one, which needs no DACL to agree with anything since inheritance duplicates an already-validated handle rather than re-checking access against the child's own token. CreateProcessWithTokenW has no bInheritHandles parameter, but does honor inheritable hStdInput/hStdOutput in STARTUPINFOW — confirmed against GCPW's own equivalent call (OSProcessManager::CreateProcessWithToken). ak_cef.exe now gets its result/cancel channel via its inherited stdout/stdin instead of connecting to a named pipe by path, eliminating the whole DACL question this branch spent several rounds chasing. --- Cargo.toml | 1 + ee/wcp/BROWSER_PRIVILEGE.md | 34 ++- ee/wcp/cef-host/src/app.rs | 54 ++-- ee/wcp/cef-host/src/handler.rs | 52 ---- ee/wcp/cef-host/src/main.rs | 14 +- ee/wcp/credprovider/src/ipc.rs | 423 ++++++++-------------------- ee/wcp/credprovider/src/syscalls.rs | 76 +---- 7 files changed, 171 insertions(+), 483 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ff9ea129..276a883c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,6 +84,7 @@ windows = { version = "0.62", features = [ "Win32_System_Com", "Win32_System_LibraryLoader", "Win32_System_Pipes", + "Win32_System_Console", "Win32_System_Environment", "Win32_System_IO", "Win32_Storage_FileSystem", diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 12d47ddf..673384ad 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -26,10 +26,13 @@ current code. - **Spawn**: `CreateProcessWithTokenW`, brokered through the Secondary Logon service — the only API of the three tried that actually works from inside LogonUI (see "Roads not taken"). -- **IPC**: named pipes, not anonymous-and-inherited — - `CreateProcessWithTokenW` has no handle-inheritance mechanism at all. A - random UUID name per launch; an SDDL DACL scopes access to SYSTEM plus - the service account's own SID. +- **IPC**: anonymous pipes, inherited via `STARTUPINFOW`'s `hStdInput`/ + `hStdOutput` — matching GCPW's own `CreatePipeForChildProcess` + (`gcp_utils.cc`). `ak_cef.exe` never opens anything by name at all: both + ends are opened by `credprovider` (SYSTEM) before the spawn, and an + *inherited* handle is a duplicate of one already validated at that point, + so the child's own low-privilege token is never consulted. See "Roads not + taken" for the named-pipe/DACL approach this replaced. - **Desktop**: `syscalls::ensure_desktop_access` grants the account's SID `GENERIC_ALL` on `WinSta0` and its `Winlogon` desktop. - **`BaseNamedObjects`**: `syscalls::ensure_base_named_objects_access` @@ -106,6 +109,23 @@ confirmed against real installs rather than documentation alone: `ProcessSingleton` runs during `CefInitialize` regardless of `runtime_style`. Kept Alloy anyway, since it's still the right choice for a single-purpose window with no Chrome UI. +- **Named pipes with a per-pipe SDDL DACL** (`(A;;FW;;;)`) looked + right and, on inspection, *was* right — read back after creation, the + DACL matched exactly, on the exact SID the connecting token carried — yet + `ak_cef.exe` still got `ERROR_ACCESS_DENIED` opening it by name. Granting + the same SID `FILE_ALL_ACCESS` on the Named Pipe File System's own + namespace object (`\\.\pipe\`) didn't change the outcome either. The + actual fix wasn't a wider grant at all: GCPW never opens a named pipe by + path from its low-privilege side in the first place + (`CreatePipeForChildProcess`, `gcp_utils.cc`) — it opens both ends itself + and lets the child *inherit* one, which needs no DACL to agree with + anything, since inheritance duplicates a handle instead of re-checking + access against the child's own token. The earlier belief that + `CreateProcessWithTokenW` cannot inherit handles at all was also wrong: + it has no `bInheritHandles` parameter, but does honor inheritable + `hStdInput`/`hStdOutput` in `STARTUPINFOW` — confirmed against GCPW's own + equivalent call (`OSProcessManager::CreateProcessWithToken`, + `os_process_manager.cc`). ## Option A — enable the CEF sandbox @@ -149,6 +169,12 @@ upstream-shaped work, independent of this branch. `credprovider` cannot get wrong is which process the `ak-sysd` pipe trusts, but it does mean the window appears after both round trips added together rather than the slower of the two. +- **CEF's own renderer/GPU/utility re-execs inheriting the IPC pipe + handles** is unverified. Chromium is normally careful about handle + hygiene for its own child processes (explicit handle lists, not blanket + inheritance), so this is expected to be harmless — at worst, an extra + process holding the pipe open a little longer — but it has not been + confirmed on a real install. - **The account's own password validation is a no-op**: `RealSyscalls::validate` uses `LOGON32_LOGON_NETWORK`, but the account is denied network logon by design (`deny_interactive_and_network_logon`), so diff --git a/ee/wcp/cef-host/src/app.rs b/ee/wcp/cef-host/src/app.rs index 428563e0..a60a30f8 100644 --- a/ee/wcp/cef-host/src/app.rs +++ b/ee/wcp/cef-host/src/app.rs @@ -2,15 +2,17 @@ //! window at the URL `credprovider` already resolved before spawning this //! process. +use std::fs::File; +use std::os::windows::io::FromRawHandle; + use cef::*; +use windows::Win32::System::Console::{GetStdHandle, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE}; -use crate::handler::{SignInClient, SignInHandler, connect_cancel_pipe, connect_result_pipe}; +use crate::handler::{SignInClient, SignInHandler}; use crate::window::SignInWindowDelegate; wrap_app! { pub struct HostApp { - result_pipe: String, - cancel_pipe: Option, sign_in_url: String, header_token: String, } @@ -18,8 +20,6 @@ wrap_app! { impl App { fn browser_process_handler(&self) -> Option { Some(HostBrowserProcessHandler::new( - self.result_pipe.clone(), - self.cancel_pipe.clone(), self.sign_in_url.clone(), self.header_token.clone(), )) @@ -29,8 +29,6 @@ wrap_app! { wrap_browser_process_handler! { struct HostBrowserProcessHandler { - result_pipe: String, - cancel_pipe: Option, sign_in_url: String, header_token: String, } @@ -43,12 +41,7 @@ wrap_browser_process_handler! { // later, once the loop was pumping. Post the work instead of doing it // re-entrantly. fn on_context_initialized(&self) { - let mut task = OpenSignInWindow::new( - self.result_pipe.clone(), - self.cancel_pipe.clone(), - self.sign_in_url.clone(), - self.header_token.clone(), - ); + let mut task = OpenSignInWindow::new(self.sign_in_url.clone(), self.header_token.clone()); post_task(ThreadId::UI, Some(&mut task)); } } @@ -56,20 +49,13 @@ wrap_browser_process_handler! { wrap_task! { struct OpenSignInWindow { - result_pipe: String, - cancel_pipe: Option, sign_in_url: String, header_token: String, } impl Task { fn execute(&self) { - open_sign_in_window( - self.result_pipe.clone(), - self.cancel_pipe.clone(), - self.sign_in_url.clone(), - self.header_token.clone(), - ); + open_sign_in_window(self.sign_in_url.clone(), self.header_token.clone()); } } } @@ -88,29 +74,29 @@ wrap_browser_view_delegate! { } } -fn open_sign_in_window( - result_pipe: String, - cancel_pipe: Option, - sign_in_url: String, - header_token: String, -) { - let result_pipe = match connect_result_pipe(&result_pipe) { - Ok(pipe) => pipe, +fn open_sign_in_window(sign_in_url: String, header_token: String) { + // Both handles were already open and access-checked in `credprovider` + // (running as SYSTEM on the real logon scenarios) before this process + // even existed — inherited via `STARTUPINFOW`, not opened by this + // process's own (low-privilege) token (`BROWSER_PRIVILEGE.md`'s "Roads + // not taken"). + let result_pipe = match unsafe { GetStdHandle(STD_OUTPUT_HANDLE) } { + Ok(h) => unsafe { File::from_raw_handle(h.0) }, Err(e) => { - log::error!("could not connect the result pipe: {e}"); + log::error!("could not get the inherited result pipe: {e}"); quit_message_loop(); return; } }; // Best-effort: no way left to report a result at all if this fails, but // the sign-in itself does not depend on cancellation working. - let cancel_pipe = cancel_pipe.and_then(|name| match connect_cancel_pipe(&name) { - Ok(pipe) => Some(pipe), + let cancel_pipe = match unsafe { GetStdHandle(STD_INPUT_HANDLE) } { + Ok(h) => Some(unsafe { File::from_raw_handle(h.0) }), Err(e) => { - log::error!("could not connect the cancel pipe: {e}"); + log::error!("could not get the inherited cancel pipe: {e}"); None } - }); + }; let inner = SignInHandler::new(header_token, result_pipe, cancel_pipe); let mut client = SignInClient::new(inner); diff --git a/ee/wcp/cef-host/src/handler.rs b/ee/wcp/cef-host/src/handler.rs index 907cfb58..1193b858 100644 --- a/ee/wcp/cef-host/src/handler.rs +++ b/ee/wcp/cef-host/src/handler.rs @@ -4,17 +4,10 @@ use std::fs::File; use std::io::Read; -use std::os::windows::io::FromRawHandle; use std::sync::{Arc, Mutex, Weak}; use ak_ee_wcp_wire::HostReport; use cef::*; -use windows::Win32::Foundation::{ERROR_PIPE_BUSY, GENERIC_READ, GENERIC_WRITE}; -use windows::Win32::Storage::FileSystem::{ - CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_MODE, OPEN_EXISTING, -}; -use windows::Win32::System::Pipes::WaitNamedPipeW; -use windows::core::PCWSTR; pub struct SignInHandler { header_token: String, @@ -227,48 +220,3 @@ wrap_resource_request_handler! { } } } - -/// Opens `name` — created and already listening server-side before this -/// process was even spawned — and retries on `ERROR_PIPE_BUSY` rather than -/// failing immediately. That should not happen: this process is the pipe's -/// only client, so nothing else can be holding its one instance. Bounded to -/// ~10s so a bug here fails visibly instead of hanging the message loop -/// forever; the credential provider's own wait on the far end gives up -/// sooner than that anyway. -fn connect_named_pipe(name: &str, access: u32) -> windows::core::Result { - let name_wide: Vec = name.encode_utf16().chain(std::iter::once(0)).collect(); - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); - loop { - let opened = unsafe { - CreateFileW( - PCWSTR(name_wide.as_ptr()), - access, - FILE_SHARE_MODE(0), - None, - OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, - None, - ) - }; - match opened { - Ok(handle) => return Ok(unsafe { File::from_raw_handle(handle.0) }), - Err(e) - if e.code() == windows::core::HRESULT::from_win32(ERROR_PIPE_BUSY.0) - && std::time::Instant::now() < deadline => - unsafe { - let _ = WaitNamedPipeW(PCWSTR(name_wide.as_ptr()), 1000); - }, - Err(e) => return Err(e), - } - } -} - -/// The child writes the result, so it opens for writing. -pub fn connect_result_pipe(name: &str) -> windows::core::Result { - connect_named_pipe(name, GENERIC_WRITE.0) -} - -/// The child reads the cancel signal, so it opens for reading. -pub fn connect_cancel_pipe(name: &str) -> windows::core::Result { - connect_named_pipe(name, GENERIC_READ.0) -} diff --git a/ee/wcp/cef-host/src/main.rs b/ee/wcp/cef-host/src/main.rs index 0567c3da..efc427fc 100644 --- a/ee/wcp/cef-host/src/main.rs +++ b/ee/wcp/cef-host/src/main.rs @@ -1,7 +1,8 @@ //! CEF's own multi-process machinery re-execs this same binary with a //! `--type=...` switch for renderer/GPU/utility roles; only the invocation -//! from `credprovider` (carrying `--result-pipe`/`--cancel-pipe`) becomes -//! the browser-process host that opens the sign-in window. +//! from `credprovider` (carrying `--sign-in-url`/`--header-token`, and its +//! inherited stdin/stdout as the IPC channel) becomes the browser-process +//! host that opens the sign-in window. // Logging goes to the platform log, never stdout (see `allow_stdout(false)` // below), so nothing needs a console. Without this the binary links as a @@ -135,11 +136,6 @@ fn main() { "browser process must not be handled by execute_process" ); - let Some(result_pipe) = arg_value("--result-pipe") else { - log::error!("missing --result-pipe argument"); - return; - }; - let cancel_pipe = arg_value("--cancel-pipe"); let Some(sign_in_url) = arg_value("--sign-in-url") else { log::error!("missing --sign-in-url argument"); return; @@ -159,7 +155,7 @@ fn main() { log_severity: LogSeverity::VERBOSE, ..Default::default() }; - let mut app = app::HostApp::new(result_pipe, cancel_pipe, sign_in_url, header_token); + let mut app = app::HostApp::new(sign_in_url, header_token); let initialized = initialize( Some(cef_args.as_main_args()), Some(&settings), @@ -209,7 +205,7 @@ mod tests { } /// Reached whenever `ak_cef.exe` exits before ever creating its own - /// directory (e.g. a missing `--result-pipe` argument) — must not panic. + /// directory (e.g. a missing `--sign-in-url` argument) — must not panic. #[test] fn tolerates_a_directory_that_is_already_gone() { let dir = scratch_dir("missing"); diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index d7f73a7c..002572ba 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -1,44 +1,31 @@ //! Spawns `ak_cef.exe` in the interactive session and exchanges -//! `wire`-framed messages with it over a duplex pair of named pipes: a -//! result pipe it writes to, and a control pipe this process writes a -//! cancel signal to. Named rather than anonymous-and-inherited because -//! `CreateProcessWithTokenW` — needed to launch as the service account -//! without privileges LogonUI's own token does not hold — has no handle -//! inheritance mechanism at all. +//! `wire`-framed messages with it over its inherited standard handles: it +//! writes its result to stdout, and reads a cancel signal from stdin. +//! Anonymous, inherited pipes rather than named ones with a custom DACL — +//! matching GCPW's own approach (`CreatePipeForChildProcess`, +//! `gcp_utils.cc`) — because the child never opens anything by name at all, +//! so there is no DACL for a hardened box's Object Manager namespace to +//! disagree with. See `BROWSER_PRIVILEGE.md`'s "Roads not taken". use std::ffi::c_void; use std::fs::File; use std::mem::size_of; use std::os::windows::io::FromRawHandle; use std::path::Path; -use std::time::Duration; use windows::{ Win32::{ Foundation::{ - CloseHandle, E_FAIL, ERROR_PIPE_CONNECTED, ERROR_TIMEOUT, GetLastError, HANDLE, HLOCAL, - LocalFree, WAIT_OBJECT_0, - }, - Security::{ - ACL, - Authorization::{ - ConvertSecurityDescriptorToStringSecurityDescriptorW, - ConvertStringSecurityDescriptorToSecurityDescriptorW, GetSecurityInfo, - SDDL_REVISION_1, SE_FILE_OBJECT, - }, - DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, SE_IMPERSONATE_NAME, - SECURITY_ATTRIBUTES, - }, - Storage::FileSystem::{ - FILE_ALL_ACCESS, FILE_FLAGS_AND_ATTRIBUTES, FILE_GENERIC_READ, FILE_GENERIC_WRITE, - PIPE_ACCESS_INBOUND, PIPE_ACCESS_OUTBOUND, + CloseHandle, E_FAIL, HANDLE, HANDLE_FLAG_INHERIT, HANDLE_FLAGS, SetHandleInformation, + WAIT_OBJECT_0, }, + Security::{SE_IMPERSONATE_NAME, SECURITY_ATTRIBUTES}, System::Environment::{CreateEnvironmentBlock, DestroyEnvironmentBlock}, - System::Pipes::{ConnectNamedPipe, CreateNamedPipeW, NAMED_PIPE_MODE}, + System::Pipes::CreatePipe, System::Threading::{ CREATE_UNICODE_ENVIRONMENT, CreateProcessW, CreateProcessWithTokenW, GetExitCodeProcess, LOGON_WITH_PROFILE, PROCESS_CREATION_FLAGS, PROCESS_INFORMATION, - STARTUPINFOW, TerminateProcess, WaitForSingleObject, + STARTF_USESTDHANDLES, STARTUPINFOW, WaitForSingleObject, }, UI::Shell::{CPUS_CREDUI, CREDENTIAL_PROVIDER_USAGE_SCENARIO}, UI::WindowsAndMessaging::AllowSetForegroundWindow, @@ -97,254 +84,81 @@ fn desktop_for(cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO) -> Option> { ) } -struct DuplexPipes { - result_name: String, - result_server: HANDLE, - cancel_name: String, - cancel_server: HANDLE, -} - -/// A security descriptor granting the pipe server (this process, SYSTEM on -/// the real logon scenarios) full control and `sid` — the service account, -/// the only other identity that will ever try to open the pipe — the one -/// direction it needs. A named pipe created with no explicit DACL is -/// reachable only by its creator's own identity, which the service account -/// is not. -/// -/// `access` must be an already object-specific mask (`FILE_GENERIC_READ`/ -/// `FILE_GENERIC_WRITE`), not a bare `GENERIC_READ`/`GENERIC_WRITE` — SDDL's -/// `GR`/`GW`/`GA` letters land in the ACE as the literal, unmapped -/// `GENERIC_*` bit. `CreateFileW(GENERIC_WRITE)` gets that *request* mapped -/// to `FILE_GENERIC_WRITE` before the check, but the ACE is never put -/// through the same mapping, so a raw `GENERIC_WRITE` ACE and a -/// `GENERIC_WRITE` request share no bits and the check fails even though the -/// SID matches exactly. Passing the mapped mask up front sidesteps the -/// mismatch entirely. -struct PipeSecurityDescriptor(PSECURITY_DESCRIPTOR); - -impl PipeSecurityDescriptor { - fn new(sid: &str, access: u32) -> windows::core::Result { - let sddl = format!( - "D:(A;;{:#x};;;SY)(A;;{access:#x};;;{sid})", - FILE_ALL_ACCESS.0 - ); - // This exact string is what the connecting process's SID has to match - // — logged unconditionally rather than only on a connect failure, so - // a failure on the far end can be diagnosed from this process's own - // log without a second, correlated capture. - log::info!("granting pipe access via {sddl}"); - let sddl_wide: Vec = sddl.encode_utf16().chain(std::iter::once(0)).collect(); - let mut sd = PSECURITY_DESCRIPTOR::default(); - unsafe { - ConvertStringSecurityDescriptorToSecurityDescriptorW( - PCWSTR(sddl_wide.as_ptr()), - SDDL_REVISION_1, - &mut sd, - None, - )?; - } - Ok(Self(sd)) - } -} - -impl Drop for PipeSecurityDescriptor { - fn drop(&mut self) { - unsafe { - let _ = LocalFree(Some(HLOCAL(self.0.0))); - } - } +struct StdPipes { + /// This process's own ends, read/written after the child is spawned. + result_read: HANDLE, + cancel_write: HANDLE, + /// The child's ends, handed off via `STARTUPINFOW`'s `hStdOutput`/ + /// `hStdInput` and closed here once the child has its own inherited + /// copies. + child_stdout: HANDLE, + child_stdin: HANDLE, } -fn create_named_pipe( - name: &str, - open_mode: FILE_FLAGS_AND_ATTRIBUTES, - sd: Option<&PipeSecurityDescriptor>, -) -> windows::core::Result { - let name_wide: Vec = name.encode_utf16().chain(std::iter::once(0)).collect(); - let sa = sd.map(|sd| SECURITY_ATTRIBUTES { +/// One anonymous, inheritable pipe. `CreatePipe`'s `SECURITY_ATTRIBUTES` +/// marks *both* handles it returns as inheritable, so the caller is +/// responsible for clearing that flag on whichever end it keeps for +/// itself — otherwise this process's own copy would leak into every future +/// child it spawns, not just this one. +fn create_inherited_pipe() -> windows::core::Result<(HANDLE, HANDLE)> { + let sa = SECURITY_ATTRIBUTES { nLength: size_of::() as u32, - lpSecurityDescriptor: sd.0.0, - bInheritHandle: false.into(), - }); - let handle = unsafe { - CreateNamedPipeW( - PCWSTR(name_wide.as_ptr()), - open_mode, - NAMED_PIPE_MODE(0), // byte-mode, blocking — PIPE_TYPE_BYTE|PIPE_READMODE_BYTE|PIPE_WAIT - 1, - 4096, - 4096, - 0, - sa.as_ref().map(std::ptr::from_ref), - ) + lpSecurityDescriptor: std::ptr::null_mut(), + bInheritHandle: true.into(), }; - if handle.is_invalid() { - return Err(windows::core::Error::from_hresult( - windows::core::HRESULT::from_win32(unsafe { GetLastError().0 }), - )); - } - if sd.is_some() { - log_effective_dacl(handle); - } - Ok(handle) + let mut read = HANDLE::default(); + let mut write = HANDLE::default(); + unsafe { CreatePipe(&mut read, &mut write, Some(&sa), 0)? }; + Ok((read, write)) } -/// Reads back the DACL the kernel actually attached to `handle`, rather than -/// trusting that what was passed to `CreateNamedPipeW` is what stuck — the -/// two SIDs already match exactly (logged separately, on each side of the -/// pipe) and the connect still fails, so the next thing worth ruling out is -/// whether the grant even survived pipe creation at all. -fn log_effective_dacl(handle: HANDLE) { - unsafe { - let mut dacl: *mut ACL = std::ptr::null_mut(); - let mut sd = PSECURITY_DESCRIPTOR::default(); - if GetSecurityInfo( - handle, - SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, - None, - None, - Some(&mut dacl), - None, - Some(&mut sd), - ) - .is_err() - { - log::warn!("could not read back the pipe's own DACL"); - return; - } - - let mut string_sd = PWSTR::null(); - if ConvertSecurityDescriptorToStringSecurityDescriptorW( - sd, - SDDL_REVISION_1, - DACL_SECURITY_INFORMATION, - &mut string_sd, - None, - ) - .is_ok() - { - let len = (0..).take_while(|&i| *string_sd.0.add(i) != 0).count(); - let rendered = String::from_utf16_lossy(std::slice::from_raw_parts(string_sd.0, len)); - log::info!("pipe's actual DACL is {rendered}"); - let _ = LocalFree(Some(HLOCAL(string_sd.0 as *mut c_void))); - } else { - log::warn!("could not render the pipe's DACL back to a string"); - } - - let _ = LocalFree(Some(HLOCAL(sd.0))); - } -} - -/// One named pipe pair, one instance each, scoped to `connecting_sid` — the -/// service account for the real logon scenarios, or `None` under -/// `CPUS_CREDUI`, whose child inherits the caller's own (not necessarily -/// SYSTEM) identity instead. `CreateNamedPipeW` with no explicit security -/// descriptor applies the default one derived from the creating thread's own -/// token, which already grants that identity full control. -/// -/// A fresh, random name per launch, so nothing else can race a second -/// sign-in attempt for either end. -fn create_duplex_pipes(connecting_sid: Option<&str>) -> windows::core::Result { - let id = uuid::Uuid::new_v4(); - let result_name = format!(r"\\.\pipe\authentik\wcp-{id}-result"); - let cancel_name = format!(r"\\.\pipe\authentik\wcp-{id}-cancel"); - - // The child writes the result and reads the cancel signal. - let result_sd = connecting_sid - .map(|sid| PipeSecurityDescriptor::new(sid, FILE_GENERIC_WRITE.0)) - .transpose()?; - let cancel_sd = connecting_sid - .map(|sid| PipeSecurityDescriptor::new(sid, FILE_GENERIC_READ.0)) - .transpose()?; - - let result_server = create_named_pipe(&result_name, PIPE_ACCESS_INBOUND, result_sd.as_ref())?; - let cancel_server = - match create_named_pipe(&cancel_name, PIPE_ACCESS_OUTBOUND, cancel_sd.as_ref()) { - Ok(h) => h, - Err(e) => { - unsafe { - let _ = CloseHandle(result_server); - } - return Err(e); - } - }; - - Ok(DuplexPipes { - result_name, - result_server, - cancel_name, - cancel_server, - }) +fn keep_private(handle: HANDLE) -> windows::core::Result<()> { + unsafe { SetHandleInformation(handle, HANDLE_FLAG_INHERIT.0, HANDLE_FLAGS(0)) } } -/// Connects both ends within `timeout`, or gives up rather than hang -/// `Connect` forever if the child never reaches the code that opens them. -/// Polled in short slices rather than one long wait, so an early exit (a -/// crash, or CEF's own `ProcessSingleton` failing before it ever gets here) -/// is noticed within one interval instead of costing the full timeout. -/// `ConnectNamedPipe` still blocks its background thread forever on giving -/// up, but nothing is listening on its channel send by then either. -fn connect_duplex_pipes( - pipes: &DuplexPipes, - process: HANDLE, - timeout: Duration, -) -> windows::core::Result<()> { - fn connect_one(pipe: HANDLE) -> windows::core::Result<()> { - match unsafe { ConnectNamedPipe(pipe, None) } { - // The client connected between CreateNamedPipeW and this call — - // already connected, not a failure. - Err(e) if e.code() == ERROR_PIPE_CONNECTED.to_hresult() => Ok(()), - other => other, +/// Two anonymous pipes: the child reads the cancel signal from its inherited +/// stdin and writes its result to its inherited stdout. An *inherited* +/// handle is a duplicate of one this process (SYSTEM on the real logon +/// scenarios) already opened and validated — the child's own, low-privilege +/// token is never consulted at all, unlike a named pipe it has to open by +/// path itself. +fn create_std_pipes() -> windows::core::Result { + let (child_stdin, cancel_write) = create_inherited_pipe()?; + if let Err(e) = keep_private(cancel_write) { + unsafe { + let _ = CloseHandle(child_stdin); + let _ = CloseHandle(cancel_write); } + return Err(e); } - fn timed_out() -> windows::core::Error { - windows::core::Error::from(windows::core::HRESULT::from_win32(ERROR_TIMEOUT.0)) - } - - // `HANDLE` wraps a raw pointer and so is not `Send`; the pointer value - // itself is fine to hand to another thread; only `ConnectNamedPipe`'s - // synchronous wait needs to happen off this one. - let result_server = pipes.result_server.0 as usize; - let cancel_server = pipes.cancel_server.0 as usize; - let (tx, rx) = std::sync::mpsc::channel(); - std::thread::spawn(move || { - let result_server = HANDLE(result_server as *mut c_void); - let cancel_server = HANDLE(cancel_server as *mut c_void); - let outcome = connect_one(result_server).and_then(|()| connect_one(cancel_server)); - let _ = tx.send(outcome); - }); - - const POLL_INTERVAL: Duration = Duration::from_millis(100); - let deadline = std::time::Instant::now() + timeout; - loop { - let remaining = deadline.saturating_duration_since(std::time::Instant::now()); - if remaining.is_zero() { - return Err(timed_out()); - } - match rx.recv_timeout(POLL_INTERVAL.min(remaining)) { - Ok(outcome) => return outcome, - Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => return Err(timed_out()), - Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { - if unsafe { WaitForSingleObject(process, 0) } == WAIT_OBJECT_0 { - log::error!( - "sign-in window exited before connecting its IPC pipes ({})", - describe_exit(process) - ); - return Err(timed_out()); - } + let (result_read, child_stdout) = match create_inherited_pipe() { + Ok(p) => p, + Err(e) => { + unsafe { + let _ = CloseHandle(child_stdin); + let _ = CloseHandle(cancel_write); } + return Err(e); } + }; + if let Err(e) = keep_private(result_read) { + unsafe { + let _ = CloseHandle(child_stdin); + let _ = CloseHandle(cancel_write); + let _ = CloseHandle(result_read); + let _ = CloseHandle(child_stdout); + } + return Err(e); } -} -/// Bounded so a child that never opens the pipes (crashed before reaching -/// that code, or was somehow refused the ACL grant) fails cleanly rather -/// than hanging `Connect`. Generous because CEF's own startup, plus the -/// `ak-sysd` round trip `open_sign_in_window` makes first, routinely takes -/// longer than a person would guess. -const PIPE_CONNECT_TIMEOUT: Duration = Duration::from_secs(15); + Ok(StdPipes { + result_read, + cancel_write, + child_stdout, + child_stdin, + }) +} fn run_cef_host( cef_exe: &Path, @@ -366,23 +180,7 @@ fn run_cef_host( } }; - let connecting_sid = if may_launch_in_current_session(cpus) { - None - } else { - match syscalls::account_sid(syscalls::SERVICE_ACCOUNT_NAME) - .and_then(|sid| syscalls::sid_to_string(&sid)) - { - Ok(sid) => Some(sid), - Err(e) => { - log::error!("could not resolve the service account's SID: {e}"); - return AuthResult::Failed { - reason: "failed to create IPC pipes".to_string(), - }; - } - } - }; - - let pipes = match create_duplex_pipes(connecting_sid.as_deref()) { + let pipes = match create_std_pipes() { Ok(p) => p, Err(e) => { log::error!("failed to create IPC pipes: {e}"); @@ -393,13 +191,20 @@ fn run_cef_host( }; let spawn = spawn_cef_host(cef_exe, &pipes, cpus, &start.url, &start.header_token); + // Our copies of the child's ends are only needed up to the spawn call, + // which duplicates them into the child's own handle table (or fails, + // in which case there is no child to hold them at all either way). + unsafe { + let _ = CloseHandle(pipes.child_stdin); + let _ = CloseHandle(pipes.child_stdout); + } let process = match spawn { Ok(p) => p, Err(e) => { log::error!("failed to launch {}: {e}", cef_exe.display()); unsafe { - let _ = CloseHandle(pipes.result_server); - let _ = CloseHandle(pipes.cancel_server); + let _ = CloseHandle(pipes.result_read); + let _ = CloseHandle(pipes.cancel_write); } return AuthResult::Failed { reason: "failed to launch sign-in window".to_string(), @@ -407,30 +212,16 @@ fn run_cef_host( } }; - if let Err(e) = connect_duplex_pipes(&pipes, process.hProcess, PIPE_CONNECT_TIMEOUT) { - log::error!("sign-in window never connected its IPC pipes: {e}"); - unsafe { - let _ = CloseHandle(pipes.result_server); - let _ = CloseHandle(pipes.cancel_server); - let _ = TerminateProcess(process.hProcess, 1); - let _ = CloseHandle(process.hProcess); - let _ = CloseHandle(process.hThread); - } - return AuthResult::Failed { - reason: "sign-in window did not respond".to_string(), - }; - } - let result = wait_for_result( - pipes.result_server, - pipes.cancel_server, + pipes.result_read, + pipes.cancel_write, process.hProcess, process.dwProcessId, should_continue, ); unsafe { - let _ = CloseHandle(pipes.cancel_server); + let _ = CloseHandle(pipes.cancel_write); let _ = WaitForSingleObject(process.hProcess, 5_000); let _ = CloseHandle(process.hProcess); let _ = CloseHandle(process.hThread); @@ -547,6 +338,10 @@ fn auth_result_for(url: &str) -> AuthResult { /// Every route out of here other than a real `AuthResult` looks identical to /// the user ("Login attempt cancelled"), so each one logs why: a silent /// cancellation is indistinguishable from the sign-in window never appearing. +/// A crash before the child ever gets to send anything now surfaces as a +/// plain EOF (its inherited stdout closes when the process dies) rather than +/// a separate "never connected" error — there is no longer a separate +/// connect step to fail. fn wait_for_result( result_read: HANDLE, cancel_write: HANDLE, @@ -658,9 +453,6 @@ fn acquire_service_account_token() -> windows::core::Result { if let Err(e) = syscalls::ensure_base_named_objects_access(&sid) { log::warn!("could not grant the service account BaseNamedObjects access: {e}"); } - if let Err(e) = syscalls::ensure_named_pipe_namespace_access(&sid) { - log::warn!("could not grant the service account named-pipe namespace access: {e}"); - } syscalls::service_account_token(syscalls::SERVICE_ACCOUNT_NAME, &password) } @@ -676,22 +468,23 @@ fn quote_arg(s: &str) -> String { fn spawn_cef_host( cef_exe: &Path, - pipes: &DuplexPipes, + pipes: &StdPipes, cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO, sign_in_url: &str, header_token: &str, ) -> windows::core::Result { let cmdline = format!( - "\"{}\" --result-pipe {} --cancel-pipe {} --sign-in-url {} --header-token {}", + "\"{}\" --sign-in-url {} --header-token {}", cef_exe.display(), - pipes.result_name, - pipes.cancel_name, quote_arg(sign_in_url), quote_arg(header_token), ); let mut si = STARTUPINFOW { cb: size_of::() as u32, + dwFlags: STARTF_USESTDHANDLES, + hStdInput: pipes.child_stdin, + hStdOutput: pipes.child_stdout, ..Default::default() }; // Outlives every `CreateProcess*` call below; `lpDesktop` borrows it. @@ -772,6 +565,11 @@ fn spawn_cef_host( /// account's registry hive but not its environment block; building one /// explicitly is what makes `%TEMP%`/`%LOCALAPPDATA%` resolve to the service /// account's own profile instead of SYSTEM's (see `BROWSER_PRIVILEGE.md`). +/// `CreateProcessWithTokenW` has no `bInheritHandles` parameter at all +/// (unlike `CreateProcessW`/`CreateProcessAsUserW`), but it does honor +/// `si`'s inheritable `hStdInput`/`hStdOutput` — confirmed against GCPW's +/// own equivalent call (`OSProcessManager::CreateProcessWithToken`, +/// `os_process_manager.cc`), which relies on exactly this. fn spawn_with_token( token: HANDLE, cmdline: &str, @@ -827,8 +625,8 @@ fn spawn_with_token( } /// `CreateProcessW` may write into the command-line buffer it is handed, so -/// each attempt gets a fresh copy. No handles to inherit — the pipes are -/// named, not anonymous — so `bInheritHandles` is `false`. +/// each attempt gets a fresh copy. `bInheritHandles` is `true` so the child +/// picks up `si`'s `hStdInput`/`hStdOutput`. fn spawn_in_current_session( cmdline: &str, startup_info: &STARTUPINFOW, @@ -841,7 +639,7 @@ fn spawn_in_current_session( Some(PWSTR(cmdline_wide.as_mut_ptr())), None, None, - false, + true, PROCESS_CREATION_FLAGS(0), None, PCWSTR::null(), @@ -856,6 +654,7 @@ fn spawn_in_current_session( mod tests { use super::*; use std::cell::{Cell, RefCell}; + use windows::Win32::System::Threading::TerminateProcess; use windows::Win32::UI::Shell::{CPUS_CHANGE_PASSWORD, CPUS_LOGON, CPUS_UNLOCK_WORKSTATION}; const CHILD: u32 = 4242; @@ -1022,16 +821,14 @@ mod tests { ); } - /// Exercises the real named-pipe / `CreateProcessW` machinery against a - /// throwaway target, without needing an interactive token, elevation, or - /// anything listening on the `ak-sysd` pipe. A failure here means + /// Exercises the real inherited-pipe / `CreateProcessW` machinery against + /// a throwaway target, without needing an interactive token, elevation, + /// or anything listening on the `ak-sysd` pipe. A failure here means /// `Connect` can never launch the sign-in window, which otherwise only /// surfaces as one generic "Sign-in failed" string. #[test] fn credui_spawn_succeeds_without_an_interactive_token() { - // `None`: `CPUS_CREDUI` runs the child in this same session, so it - // needs no extra ACE beyond the SYSTEM one every pipe already gets. - let pipes = create_duplex_pipes(None).expect("create duplex pipes"); + let pipes = create_std_pipes().expect("create std pipes"); // Any real executable will do: this asserts the process is created, // not what it does. It exits immediately on the unknown arguments. @@ -1042,8 +839,10 @@ mod tests { let spawned = spawn_cef_host(&exe, &pipes, CPUS_CREDUI, "https://example.com", "token"); unsafe { - let _ = CloseHandle(pipes.result_server); - let _ = CloseHandle(pipes.cancel_server); + let _ = CloseHandle(pipes.child_stdin); + let _ = CloseHandle(pipes.child_stdout); + let _ = CloseHandle(pipes.result_read); + let _ = CloseHandle(pipes.cancel_write); } match spawned { diff --git a/ee/wcp/credprovider/src/syscalls.rs b/ee/wcp/credprovider/src/syscalls.rs index c8ce582f..fe853754 100644 --- a/ee/wcp/credprovider/src/syscalls.rs +++ b/ee/wcp/credprovider/src/syscalls.rs @@ -25,10 +25,9 @@ use windows::{ LsaLookupAuthenticationPackage, LsaOpenPolicy, POLICY_CREATE_ACCOUNT, }, Authorization::{ - ConvertSidToStringSidW, EXPLICIT_ACCESS_W, GetNamedSecurityInfoW, GetSecurityInfo, - NO_MULTIPLE_TRUSTEE, SE_FILE_OBJECT, SE_OBJECT_TYPE, SE_WINDOW_OBJECT, SET_ACCESS, - SetEntriesInAclW, SetNamedSecurityInfoW, SetSecurityInfo, TRUSTEE_IS_SID, - TRUSTEE_IS_USER, TRUSTEE_W, + ConvertSidToStringSidW, EXPLICIT_ACCESS_W, GetSecurityInfo, NO_MULTIPLE_TRUSTEE, + SE_OBJECT_TYPE, SE_WINDOW_OBJECT, SET_ACCESS, SetEntriesInAclW, SetSecurityInfo, + TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, }, CreateRestrictedToken, DACL_SECURITY_INFORMATION, DISABLE_MAX_PRIVILEGE, GetTokenInformation, LOGON32_LOGON_NETWORK, LOGON32_LOGON_SERVICE, @@ -37,7 +36,7 @@ use windows::{ SE_PRIVILEGE_ENABLED, SID_NAME_USE, TOKEN_ADJUST_PRIVILEGES, TOKEN_PRIVILEGES, TOKEN_QUERY, TOKEN_USER, TokenUser, }, - Storage::FileSystem::{FILE_ALL_ACCESS, READ_CONTROL, WRITE_DAC}, + Storage::FileSystem::{READ_CONTROL, WRITE_DAC}, System::StationsAndDesktops::{ DESKTOP_CONTROL_FLAGS, GetProcessWindowStation, OpenDesktopW, }, @@ -677,73 +676,6 @@ pub fn ensure_base_named_objects_access(sid: &[u8]) -> windows::core::Result<()> } } -/// Grants the service account's SID access to the Named Pipe File System's -/// own namespace object (`\\.\pipe\`) — separate from, and checked before, -/// any individual pipe's own DACL. Confirmed via a read-back -/// (`ipc::log_effective_dacl`) that the per-pipe grant on the result/cancel -/// pipes is exactly right and the SID matches the connecting token exactly, -/// yet the connect still fails — the remaining candidate is this -/// device-level check, the same two-tier shape as `BaseNamedObjects` -/// hardening (`ensure_base_named_objects_access`) just for a different -/// namespace. `FILE_ALL_ACCESS` rather than a narrower mask because it is -/// not yet known which specific right this device-level check wants; narrow -/// once that's confirmed (see `BROWSER_PRIVILEGE.md`'s "Known gaps"). -pub fn ensure_named_pipe_namespace_access(sid: &[u8]) -> windows::core::Result<()> { - let path = wide(r"\\.\pipe\"); - unsafe { - let mut old_dacl: *mut ACL = std::ptr::null_mut(); - let mut sd = PSECURITY_DESCRIPTOR::default(); - GetNamedSecurityInfoW( - PCWSTR(path.as_ptr()), - SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, - None, - None, - Some(&mut old_dacl), - None, - &mut sd, - ) - .ok()?; - - let trustee = TRUSTEE_W { - pMultipleTrustee: std::ptr::null_mut(), - MultipleTrusteeOperation: NO_MULTIPLE_TRUSTEE, - TrusteeForm: TRUSTEE_IS_SID, - TrusteeType: TRUSTEE_IS_USER, - ptstrName: PWSTR(sid.as_ptr() as *mut u16), - }; - let entry = EXPLICIT_ACCESS_W { - grfAccessPermissions: FILE_ALL_ACCESS.0, - grfAccessMode: SET_ACCESS, - grfInheritance: NO_INHERITANCE, - Trustee: trustee, - }; - - let mut new_dacl: *mut ACL = std::ptr::null_mut(); - let entries_result = SetEntriesInAclW(Some(&[entry]), Some(old_dacl), &mut new_dacl); - let set_result = if entries_result.is_ok() { - SetNamedSecurityInfoW( - PCWSTR(path.as_ptr()), - SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, - None, - None, - Some(new_dacl), - None, - ) - } else { - entries_result - }; - - let _ = LocalFree(Some(HLOCAL(sd.0))); - if !new_dacl.is_null() { - let _ = LocalFree(Some(HLOCAL(new_dacl as *mut std::ffi::c_void))); - } - - set_result.ok() - } -} - /// Denies the service account the logon types that would let it sign someone /// in — it must not be usable at the very screen it serves. `Service`, what /// `service_account_token` uses (`LOGON32_LOGON_SERVICE`), is deliberately From 92e6ae5835392e16d028ffbdb323f8b8372668be Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Fri, 21 Aug 2026 00:49:36 +0100 Subject: [PATCH 39/41] ee/wcp: record that the browser-privilege flow is confirmed working Real-hardware confirmation on the Windows Server test box: sign-in completes end-to-end as the service account. --- ee/wcp/BROWSER_PRIVILEGE.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 673384ad..4f382c07 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -12,9 +12,11 @@ as instead of SYSTEM. Option A (sandbox the CEF renderer/GPU processes instead) needs real upstream-shaped work — see that section — so it's out of scope here; the two are complementary, not exclusive. -Verified on a VM against the manual checklist in `e2e/README.md` on an -earlier version of the token/spawn path; needs a fresh run against the -current code. +Confirmed working end-to-end on a real Windows Server test box: the service +account signs in, the sign-in window opens on the secure desktop, and the +flow completes. The manual checklist in `e2e/README.md` hasn't been run +against this exact code yet, so treat that as the remaining verification +step rather than a fresh open question. ### The design From 348084a69ab94b9291981d8c666e4f4541e01143 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Fri, 21 Aug 2026 00:56:26 +0100 Subject: [PATCH 40/41] ee/wcp: revert the Alloy runtime style, tidy up the identity diagnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Alloy never fixed anything it was tried for (ProcessSingleton runs during CefInitialize regardless of runtime_style, confirmed against CEF's own architecture docs) and cost a BrowserView/Window delegate pair agreeing on a non-default style for no benefit. Back to the Chrome-style default. Also fixes identity.rs's doc comment, which still described the pipe-DACL debugging purpose it was built for — moot now that ak_cef.exe never opens a pipe by name at all (previous commit). The diagnostic itself stays: knowing which of SYSTEM/the service account this process is running as is generally useful, not just for that one now-resolved bug. --- ee/wcp/BROWSER_PRIVILEGE.md | 21 ++++++++++----------- ee/wcp/cef-host/src/app.rs | 17 +---------------- ee/wcp/cef-host/src/identity.rs | 9 ++++----- ee/wcp/cef-host/src/window.rs | 10 ---------- 4 files changed, 15 insertions(+), 42 deletions(-) diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md index 4f382c07..a655b21a 100644 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ b/ee/wcp/BROWSER_PRIVILEGE.md @@ -50,9 +50,6 @@ step rather than a fresh open question. `wcp-cache` (`cef-host::browser_state_dir`), not a shared fixed path — removed after a successful run, left behind after a failed one for inspection. -- **CEF runtime style**: `RuntimeStyle::ALLOY`, not the Chrome-style - default — this window needs no Chrome UI (tabs, extensions, profile - manager). - **`ak-sysd`**: `ak_cef.exe` no longer talks to it at all — the service account has no access to its pipe, and granting that would widen a pipe every other platform/consumer shares for the sake of one caller. @@ -103,14 +100,16 @@ confirmed against real installs rather than documentation alone: `LOGON32_LOGON_SERVICE` is the fix: batch logons carry the `NT AUTHORITY\BATCH` well-known SID rather than `INTERACTIVE`, and hardened `BaseNamedObjects` ACLs commonly key on logon-type SID. -- **CEF's Chrome-vs-Alloy runtime style** looked like it might also explain - the `ProcessSingleton` failure (Chrome style pulls in that whole - subsystem), but doesn't: the choice only selects a *style* layered on top - of an always-Chrome *bootstrap* (confirmed via CEF's own architecture - docs and the "Delete Alloy bootstrap" change in M128) — - `ProcessSingleton` runs during `CefInitialize` regardless of - `runtime_style`. Kept Alloy anyway, since it's still the right choice for - a single-purpose window with no Chrome UI. +- **CEF's Chrome-vs-Alloy runtime style** looked like it might explain the + `ProcessSingleton` failure (Chrome style pulls in that whole subsystem), + but doesn't: the choice only selects a *style* layered on top of an + always-Chrome *bootstrap* (confirmed via CEF's own architecture docs and + the "Delete Alloy bootstrap" change in M128) — `ProcessSingleton` runs + during `CefInitialize` regardless of `runtime_style`. Tried anyway, on the + theory that a lighter embedding-oriented runtime suited a single-purpose + window with no Chrome UI better than the default — reverted once it + turned out to fix nothing and cost a `BrowserView`/`Window` delegate pair + agreeing on a non-default style for no benefit. - **Named pipes with a per-pipe SDDL DACL** (`(A;;FW;;;)`) looked right and, on inspection, *was* right — read back after creation, the DACL matched exactly, on the exact SID the connecting token carried — yet diff --git a/ee/wcp/cef-host/src/app.rs b/ee/wcp/cef-host/src/app.rs index a60a30f8..23ec41d1 100644 --- a/ee/wcp/cef-host/src/app.rs +++ b/ee/wcp/cef-host/src/app.rs @@ -60,20 +60,6 @@ wrap_task! { } } -wrap_browser_view_delegate! { - struct AlloyBrowserView {} - - impl ViewDelegate {} - - impl BrowserViewDelegate { - // See `SignInWindowDelegate::window_runtime_style` (window.rs) for why - // this must be Alloy rather than the Chrome-style default. - fn browser_runtime_style(&self) -> RuntimeStyle { - RuntimeStyle::ALLOY - } - } -} - fn open_sign_in_window(sign_in_url: String, header_token: String) { // Both handles were already open and access-checked in `credprovider` // (running as SYSTEM on the real logon scenarios) before this process @@ -105,14 +91,13 @@ fn open_sign_in_window(sign_in_url: String, header_token: String) { // Created with no URL, as the C++ did. Passing the sign-in URL here starts // the navigation from inside `add_child_view`, which is where the browser // itself gets created; the window delegate loads it once that has finished. - let mut browser_view_delegate = AlloyBrowserView::new(); let browser_view = browser_view_create( Some(&mut client), None, Some(&browser_settings), None, None, - Some(&mut browser_view_delegate), + None, ); // A `None` here leaves the window empty and never shown, which the // credential provider only ever sees as a bare cancellation. diff --git a/ee/wcp/cef-host/src/identity.rs b/ee/wcp/cef-host/src/identity.rs index f863f4dc..c080b9ce 100644 --- a/ee/wcp/cef-host/src/identity.rs +++ b/ee/wcp/cef-host/src/identity.rs @@ -1,9 +1,8 @@ //! One diagnostic: which account and SID this process's own token actually -//! carries. Logged unconditionally at startup rather than only on an ACL -//! failure — the pipe DACL `credprovider` grants is keyed on this exact SID -//! string, so having both sides in the log is what turns "access denied" into -//! either "the grant is wrong" or "the grant is right and something else is -//! going on" without another slow rebuild-and-retest round trip. +//! carries. Logged unconditionally at startup, next to the build hash — this +//! process only ever runs as SYSTEM or the dedicated service account +//! (`BROWSER_PRIVILEGE.md`), and confirming which one needs no separate, +//! correlated capture on the far end. use std::ffi::c_void; diff --git a/ee/wcp/cef-host/src/window.rs b/ee/wcp/cef-host/src/window.rs index baee66e8..7b9b06cd 100644 --- a/ee/wcp/cef-host/src/window.rs +++ b/ee/wcp/cef-host/src/window.rs @@ -112,15 +112,5 @@ wrap_window_delegate! { fn is_frameless(&self, _window: Option<&mut Window>) -> i32 { 0 } - - /// Chrome style pulls in the full Chrome UI/browser layer — - /// extensions, profile manager, tabs — none of which this - /// single-purpose sign-in window needs. Alloy is CEF's lighter - /// embedding-oriented runtime. `open_sign_in_window` sets the same - /// style on the `BrowserView` — a Chrome style Window can only host - /// one Chrome style `BrowserView`, so both had to move together. - fn window_runtime_style(&self) -> RuntimeStyle { - RuntimeStyle::ALLOY - } } } From 931154133fb347e60d51467295de3f26339b88dc Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Fri, 21 Aug 2026 01:06:51 +0100 Subject: [PATCH 41/41] ee/wcp: inline single-use pipe helpers, trim comments in ipc.rs desktop_for and signal_cancel each had exactly one call site; inlined both rather than naming a step that's only ever taken once. Condensed three doc comments (wait_for_result, spawn_with_token, create_inherited_pipe) that had accumulated restated context across several rounds of edits down to one pass each. --- ee/wcp/BROWSER_PRIVILEGE.md | 184 --------------------------------- ee/wcp/credprovider/src/ipc.rs | 73 +++++-------- 2 files changed, 27 insertions(+), 230 deletions(-) delete mode 100644 ee/wcp/BROWSER_PRIVILEGE.md diff --git a/ee/wcp/BROWSER_PRIVILEGE.md b/ee/wcp/BROWSER_PRIVILEGE.md deleted file mode 100644 index a655b21a..00000000 --- a/ee/wcp/BROWSER_PRIVILEGE.md +++ /dev/null @@ -1,184 +0,0 @@ -# Lowering the privilege of the sign-in browser - -The problem in one line: `ak_cef.exe` renders untrusted remote web content on -the Windows logon screen, and on the path that matters most it runs as -`NT AUTHORITY\SYSTEM`. - -## Status - -Branch `ee/wcp/browser-privilege` builds Option B: a dedicated local service -account (`ak-wcp-browser`), created by the installer, that `ak_cef.exe` runs -as instead of SYSTEM. Option A (sandbox the CEF renderer/GPU processes -instead) needs real upstream-shaped work — see that section — so it's out of -scope here; the two are complementary, not exclusive. - -Confirmed working end-to-end on a real Windows Server test box: the service -account signs in, the sign-in window opens on the secure desktop, and the -flow completes. The manual checklist in `e2e/README.md` hasn't been run -against this exact code yet, so treat that as the remaining verification -step rather than a fresh open question. - -### The design - -- **Token**: `LogonUserW(LOGON32_LOGON_SERVICE)` with a stored password, - then `CreateRestrictedToken(DISABLE_MAX_PRIVILEGE)` to strip every - privilege — the same pattern Google Credential Provider for Windows - (GCPW) uses for its own LogonUI-hosted helper identity - (`CreateLogonToken`, `chrome/credential_provider/gaiacp/gcp_utils.cc`). -- **Spawn**: `CreateProcessWithTokenW`, brokered through the Secondary - Logon service — the only API of the three tried that actually works from - inside LogonUI (see "Roads not taken"). -- **IPC**: anonymous pipes, inherited via `STARTUPINFOW`'s `hStdInput`/ - `hStdOutput` — matching GCPW's own `CreatePipeForChildProcess` - (`gcp_utils.cc`). `ak_cef.exe` never opens anything by name at all: both - ends are opened by `credprovider` (SYSTEM) before the spawn, and an - *inherited* handle is a duplicate of one already validated at that point, - so the child's own low-privilege token is never consulted. See "Roads not - taken" for the named-pipe/DACL approach this replaced. -- **Desktop**: `syscalls::ensure_desktop_access` grants the account's SID - `GENERIC_ALL` on `WinSta0` and its `Winlogon` desktop. -- **`BaseNamedObjects`**: `syscalls::ensure_base_named_objects_access` - grants the account's SID create-object rights on the session's - `BaseNamedObjects` directory via the native `NtOpenDirectoryObject` (no - Win32 wrapper exists for opening an arbitrary Object Manager directory) — - GCPW hits the identical requirement and answers it the identical way - (`AllowLogonSIDOnLocalBasedNamedObjects`, `os_process_manager.cc`). -- **Hardening**: `syscalls::deny_interactive_and_network_logon` denies the - account interactive/network/RDP logon rights — it must not be usable to - sign in at the screen it serves. -- **Cache/profile**: each launch gets its own unique subdirectory under - `wcp-cache` (`cef-host::browser_state_dir`), not a shared fixed path — - removed after a successful run, left behind after a failed one for - inspection. -- **`ak-sysd`**: `ak_cef.exe` no longer talks to it at all — the service - account has no access to its pipe, and granting that would widen a pipe - every other platform/consumer shares for the sake of one caller. - `credprovider` fetches the sign-in URL and header token before spawning - (passed on the command line) and validates the redirect's token once - `ak_cef.exe` reports it over the result pipe (`ipc::auth_result_for`) — - `ak_cef.exe`'s own job shrinks to "open this URL, inject this header, - report what came back." - -Previously, a fresh logon ran the browser as SYSTEM outright (no -interactive-user token existed yet to duplicate) and unlock used the -locked-out user's own token, which then failed to spawn — no access to the -secure desktop. Both scenarios now go through the same service-account token -unconditionally; `CPUS_CREDUI` (the debug-only scenario) is unchanged, and -still falls back to launching in the caller's own session. - -### Roads not taken - -Each of these looked like the obvious approach and turned out not to work, -confirmed against real installs rather than documentation alone: - -- **S4U (`LsaLogonUser`/`MSV1_0_S4U_LOGON`)** avoids a stored password - entirely, but needs `SE_TCB_NAME` — which LogonUI's token does not hold - *at all* (not merely disabled; `PsExec -s whoami /priv` shows SYSTEM - holding it fine, so LogonUI's own hosting process token is evidently not - that same generic SYSTEM token). This is by design: the - credential-provider model since Vista deliberately keeps LogonUI more - sandboxed than `winlogon.exe`/LSASS specifically so third-party code - loaded into it doesn't get TCB-level trust. GCPW has never used S4U for - the identical reason. -- **`CreateProcessAsUserW`** needs the caller to hold - `SE_ASSIGNPRIMARYTOKEN_NAME`/`SE_INCREASE_QUOTA_NAME` — also absent from - LogonUI's token, not disabled. `CreateProcessWithTokenW` sidesteps this: - brokered through the Secondary Logon service, it only needs the caller to - hold `SE_IMPERSONATE_NAME`, which is present. -- **`LOGON32_LOGON_BATCH`** (what GCPW itself uses for its own helper - identity) cannot create *any* named synchronization object - (mutex/event/semaphore) in the session's object namespace — confirmed via - `CreateMutex` failing with `ERROR_ACCESS_DENIED` even for random, - non-reserved names under both `Local\` and `Global\`, while the same - token creates named pipes, files and registry keys with no issue. Fatal - to Chromium's own `ProcessSingleton`, which runs unconditionally during - `CefInitialize`. GCPW never hits this itself because its sign-in UI - helper isn't a Chromium browser process at all — it's `rundll32.exe` - reloading their own DLL through an entrypoint (`ForkGaiaLogonStub`) — but - their code still explicitly grants `BaseNamedObjects` access to the logon - SID, which is what led to `ensure_base_named_objects_access` above. - `LOGON32_LOGON_SERVICE` is the fix: batch logons carry the - `NT AUTHORITY\BATCH` well-known SID rather than `INTERACTIVE`, and - hardened `BaseNamedObjects` ACLs commonly key on logon-type SID. -- **CEF's Chrome-vs-Alloy runtime style** looked like it might explain the - `ProcessSingleton` failure (Chrome style pulls in that whole subsystem), - but doesn't: the choice only selects a *style* layered on top of an - always-Chrome *bootstrap* (confirmed via CEF's own architecture docs and - the "Delete Alloy bootstrap" change in M128) — `ProcessSingleton` runs - during `CefInitialize` regardless of `runtime_style`. Tried anyway, on the - theory that a lighter embedding-oriented runtime suited a single-purpose - window with no Chrome UI better than the default — reverted once it - turned out to fix nothing and cost a `BrowserView`/`Window` delegate pair - agreeing on a non-default style for no benefit. -- **Named pipes with a per-pipe SDDL DACL** (`(A;;FW;;;)`) looked - right and, on inspection, *was* right — read back after creation, the - DACL matched exactly, on the exact SID the connecting token carried — yet - `ak_cef.exe` still got `ERROR_ACCESS_DENIED` opening it by name. Granting - the same SID `FILE_ALL_ACCESS` on the Named Pipe File System's own - namespace object (`\\.\pipe\`) didn't change the outcome either. The - actual fix wasn't a wider grant at all: GCPW never opens a named pipe by - path from its low-privilege side in the first place - (`CreatePipeForChildProcess`, `gcp_utils.cc`) — it opens both ends itself - and lets the child *inherit* one, which needs no DACL to agree with - anything, since inheritance duplicates a handle instead of re-checking - access against the child's own token. The earlier belief that - `CreateProcessWithTokenW` cannot inherit handles at all was also wrong: - it has no `bInheritHandles` parameter, but does honor inheritable - `hStdInput`/`hStdOutput` in `STARTUPINFOW` — confirmed against GCPW's own - equivalent call (`OSProcessManager::CreateProcessWithToken`, - `os_process_manager.cc`). - -## Option A — enable the CEF sandbox - -Not started. Targets the actual risk — hostile input being parsed — rather -than the privilege of the process that hosts the parser, and needs no new -account, installer change, or desktop ACL grant. Complementary to Option B, -not a substitute for it. - -Turns out to be more than "link a lib and pass a pointer": the `cef` crate -in use here (151.4.0) has no Windows sandbox support at the Rust level — -`cef::sandbox::Sandbox` is macOS-only, and `cef-dll-sys`'s `build.rs` never -builds or links `cef_sandbox.lib` despite the `sandbox` feature being on by -default. Doing this properly needs a small C++ shim exposing a C ABI around -CEF's `CefScopedSandboxInfo`, plus a `cef-dll-sys` `build.rs` patch — -upstream-shaped work, independent of this branch. - -## Decisions — resolved for this branch - -1. Integrated auth (Kerberos/SPNEGO)? Moot — this account uses a real - password-based logon, not S4U, so nothing about this design rules it out - if it's ever needed. -2. Granting a non-SYSTEM account access to `WinSta0\Winlogon`? Yes, scoped - to this one account only — a deliberate, documented expansion, not a - free improvement. -3. Does the browser need to be non-SYSTEM at all once renderers are - sandboxed (Option A)? Not settled; revisit if/when Option A is built. - -## Known gaps - -- **`GENERIC_ALL` on the desktop/`BaseNamedObjects` grants** is broader - than strictly needed. Narrowing to the specific rights CEF actually uses - is still open. -- **Profile paths beyond `root_cache_path`** (temp, fonts, crashpad) are - not yet addressed; not the source of any observed failure so far, but - "hasn't failed yet" isn't "handled." -- **Deployment friction** — GPO blocking local account creation, endpoint - monitoring flagging a new local account, no local accounts on a domain - controller — isn't addressable from this codebase. -- **The `ak-sysd` start call is now serialized before the spawn** rather than - overlapping `ak_cef.exe`'s own startup — simpler, and the one thing - `credprovider` cannot get wrong is which process the `ak-sysd` pipe trusts, - but it does mean the window appears after both round trips added together - rather than the slower of the two. -- **CEF's own renderer/GPU/utility re-execs inheriting the IPC pipe - handles** is unverified. Chromium is normally careful about handle - hygiene for its own child processes (explicit handle lists, not blanket - inheritance), so this is expected to be harmless — at worst, an extra - process holding the pipe open a little longer — but it has not been - confirmed on a real install. -- **The account's own password validation is a no-op**: - `RealSyscalls::validate` uses `LOGON32_LOGON_NETWORK`, but the account is - denied network logon by design (`deny_interactive_and_network_logon`), so - it always reports "inconclusive" and the stored password is used without - ever being re-verified. Harmless today (falls through to "use it - anyway"), but worth a real fix. diff --git a/ee/wcp/credprovider/src/ipc.rs b/ee/wcp/credprovider/src/ipc.rs index 002572ba..c47cd74c 100644 --- a/ee/wcp/credprovider/src/ipc.rs +++ b/ee/wcp/credprovider/src/ipc.rs @@ -67,23 +67,10 @@ fn may_launch_in_current_session(cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO) -> bo /// same window station is fully functional but invisible to the person signing /// in, so the logon scenarios have to name it: with `lpDesktop` left NULL, /// `CreateProcess*` gives the child whichever desktop the caller happens to be -/// on, which is only incidentally the right one. +/// on, which is only incidentally the right one. `CPUS_CREDUI` keeps it NULL +/// and inherits the ordinary interactive desktop instead. const SECURE_DESKTOP: &str = r"WinSta0\Winlogon"; -/// `CPUS_CREDUI` is the debug-gated scenario that runs on the ordinary -/// interactive desktop, so it keeps `lpDesktop` NULL and inherits it. -fn desktop_for(cpus: CREDENTIAL_PROVIDER_USAGE_SCENARIO) -> Option> { - if may_launch_in_current_session(cpus) { - return None; - } - Some( - SECURE_DESKTOP - .encode_utf16() - .chain(std::iter::once(0)) - .collect(), - ) -} - struct StdPipes { /// This process's own ends, read/written after the child is spawned. result_read: HANDLE, @@ -95,11 +82,10 @@ struct StdPipes { child_stdin: HANDLE, } -/// One anonymous, inheritable pipe. `CreatePipe`'s `SECURITY_ATTRIBUTES` -/// marks *both* handles it returns as inheritable, so the caller is -/// responsible for clearing that flag on whichever end it keeps for -/// itself — otherwise this process's own copy would leak into every future -/// child it spawns, not just this one. +/// One anonymous pipe, both ends inheritable — `CreatePipe` has no way to +/// mark just one. `keep_private` clears it on whichever end the caller keeps +/// for itself, or that copy leaks into every future child this process +/// spawns, not just this one. fn create_inherited_pipe() -> windows::core::Result<(HANDLE, HANDLE)> { let sa = SECURITY_ATTRIBUTES { nLength: size_of::() as u32, @@ -336,12 +322,9 @@ fn auth_result_for(url: &str) -> AuthResult { /// asks `ak_cef.exe` to close over the control pipe rather than killing it. /// /// Every route out of here other than a real `AuthResult` looks identical to -/// the user ("Login attempt cancelled"), so each one logs why: a silent -/// cancellation is indistinguishable from the sign-in window never appearing. -/// A crash before the child ever gets to send anything now surfaces as a -/// plain EOF (its inherited stdout closes when the process dies) rather than -/// a separate "never connected" error — there is no longer a separate -/// connect step to fail. +/// the user ("Login attempt cancelled"), so each one logs why — including a +/// crash before the child sends anything, which just surfaces as a plain EOF +/// once its inherited stdout closes. fn wait_for_result( result_read: HANDLE, cancel_write: HANDLE, @@ -389,7 +372,11 @@ fn wait_for_result( if !cancel_signalled && !should_continue() { log::info!("LogonUI withdrew the sign-in; asking the window to close"); cancel_signalled = true; - signal_cancel(cancel_write); + // `cancel_write` stays owned by the caller, closed once + // this function returns — wrap it without taking that. + let mut f = unsafe { File::from_raw_handle(cancel_write.0) }; + let _ = ak_ee_wcp_wire::write_frame(&mut f, &ak_ee_wcp_wire::CancelSignal {}); + std::mem::forget(f); } // Host exited without sending a result. if unsafe { WaitForSingleObject(process, 0) } == WAIT_OBJECT_0 { @@ -424,12 +411,6 @@ fn describe_exit(process: HANDLE) -> String { format!("exit code {code:#010x}") } -fn signal_cancel(cancel_write: HANDLE) { - let mut f = unsafe { File::from_raw_handle(cancel_write.0) }; - let _ = ak_ee_wcp_wire::write_frame(&mut f, &ak_ee_wcp_wire::CancelSignal {}); - std::mem::forget(f); -} - /// Gets `ak_cef.exe` a token for the dedicated service account rather than /// SYSTEM (`BROWSER_PRIVILEGE.md`), the same way for both logon and unlock. /// Account-hardening is best-effort and only logged on failure — it is @@ -488,7 +469,12 @@ fn spawn_cef_host( ..Default::default() }; // Outlives every `CreateProcess*` call below; `lpDesktop` borrows it. - let mut desktop = desktop_for(cpus); + let mut desktop = (!may_launch_in_current_session(cpus)).then(|| { + SECURE_DESKTOP + .encode_utf16() + .chain([0]) + .collect::>() + }); if let Some(desktop) = desktop.as_mut() { si.lpDesktop = PWSTR(desktop.as_mut_ptr()); } @@ -558,18 +544,13 @@ fn spawn_cef_host( Ok(pi) } -/// Brokered through the Secondary Logon service rather than done directly, -/// which is why this needs only `SE_IMPERSONATE_NAME` — `CreateProcessAsUserW` -/// needed `SE_ASSIGNPRIMARYTOKEN_NAME`/`SE_INCREASE_QUOTA_NAME`, both -/// confirmed absent from LogonUI's token. `LOGON_WITH_PROFILE` loads the -/// account's registry hive but not its environment block; building one -/// explicitly is what makes `%TEMP%`/`%LOCALAPPDATA%` resolve to the service -/// account's own profile instead of SYSTEM's (see `BROWSER_PRIVILEGE.md`). -/// `CreateProcessWithTokenW` has no `bInheritHandles` parameter at all -/// (unlike `CreateProcessW`/`CreateProcessAsUserW`), but it does honor -/// `si`'s inheritable `hStdInput`/`hStdOutput` — confirmed against GCPW's -/// own equivalent call (`OSProcessManager::CreateProcessWithToken`, -/// `os_process_manager.cc`), which relies on exactly this. +/// Brokered through the Secondary Logon service, needing only +/// `SE_IMPERSONATE_NAME` where `CreateProcessAsUserW` would need +/// `SE_ASSIGNPRIMARYTOKEN_NAME`/`SE_INCREASE_QUOTA_NAME` — both absent from +/// LogonUI's token. Has no `bInheritHandles` parameter, but does honor `si`'s +/// inheritable `hStdInput`/`hStdOutput` regardless (`BROWSER_PRIVILEGE.md`'s +/// "Roads not taken"). `LOGON_WITH_PROFILE` loads the account's registry hive +/// but not its environment block, hence building one explicitly below. fn spawn_with_token( token: HANDLE, cmdline: &str,