From 2804c487e431a376ed69883c996f3f516741019f Mon Sep 17 00:00:00 2001 From: RamonArjona4 Date: Tue, 4 Aug 2026 16:38:55 -0700 Subject: [PATCH 1/4] feat: add OpenShell-compatible diagnostics Add local structured diagnostics and audit records for sandbox policy, identity, lifecycle, enforcement, network, timeout, and rejection events. Keep diagnostics local and out of SDK output, with redaction and hardened diagnostic IPC. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7effbd2a-1c76-4c8b-bf1b-a33701a0dc1c --- docs/diagnostics.md | 44 +- docs/telemetry/telemetry.md | 310 +++++++-- src/Cargo.lock | 1 + src/Cargo.toml | 2 + .../common/src/appcontainer_runner.rs | 282 +++++++- .../common/src/base_container_runner.rs | 215 +++++- .../appcontainer/common/src/dispatcher.rs | 222 +++++- .../common/src/fallback_detector.rs | 216 +++++- .../appcontainer/common/src/job_object.rs | 33 +- .../common/src/network_manager.rs | 225 +++++- .../isolation_session/common/src/manager.rs | 66 +- .../isolation_session/common/src/one_shot.rs | 2 +- .../common/src/state_aware.rs | 8 +- src/core/mxc_engine/src/dispatch.rs | 4 + src/core/mxc_engine/src/lib.rs | 2 +- src/core/mxc_engine/src/run.rs | 56 ++ src/core/mxc_engine/src/state_aware.rs | 2 +- src/core/wxc/src/main.rs | 434 +++++++++++- src/core/wxc_common/Cargo.toml | 3 +- src/core/wxc_common/src/audit.rs | 637 +++++++++++++++++ src/core/wxc_common/src/config_parser.rs | 20 +- src/core/wxc_common/src/diagnostic.rs | 75 +- src/core/wxc_common/src/lib.rs | 2 + src/core/wxc_common/src/logger.rs | 321 ++++++++- src/core/wxc_common/src/models.rs | 23 + src/core/wxc_common/src/policy_identity.rs | 641 ++++++++++++++++++ src/tools/mxc_diagnostic_console/src/main.rs | 84 ++- 27 files changed, 3676 insertions(+), 254 deletions(-) create mode 100644 src/core/wxc_common/src/audit.rs create mode 100644 src/core/wxc_common/src/policy_identity.rs diff --git a/docs/diagnostics.md b/docs/diagnostics.md index 514ac3e21..e571ff5c1 100644 --- a/docs/diagnostics.md +++ b/docs/diagnostics.md @@ -14,11 +14,14 @@ All layers stream into a single `mxc-diagnostic-console.exe` window in real time ## Quick Start ```powershell -# Terminal 1: start the diagnostic console (run as admin for ETW) +# Terminal 1: choose one token and start the diagnostic console +$env:MXC_DIAG_PIPE_TOKEN = [guid]::NewGuid().ToString("N") +$env:MXC_DIAG_PIPE_TOKEN mxc-diagnostic-console.exe -# Terminal 2: enable diagnostics and run +# Terminal 2: use the same token, enable diagnostics, and run $env:MXC_DIAG_CONSOLE = "1" +$env:MXC_DIAG_PIPE_TOKEN = "" wxc-exec.exe --experimental my-config.json ``` @@ -26,7 +29,9 @@ wxc-exec.exe --experimental my-config.json | Method | Setting | Description | |--------|---------|-------------| +| CLI flag | `--log-file ` | Write diagnostics and structured audit records to a file without starting the named-pipe console | | Env var | `MXC_DIAG_CONSOLE=1` | Enable diagnostic pipe output and auto-inject `learningModeLogging` capability | +| Env var | `MXC_DIAG_PIPE_TOKEN=` | Select the per-session diagnostic pipe; use the same high-entropy token for the console and `wxc-exec` | ## What Gets Logged @@ -34,6 +39,27 @@ wxc-exec.exe --experimental my-config.json - Sandbox spec details (size, UI flags, capabilities, filesystem/network policy) - Process lifecycle (command line, identity, child PID, exit code, elapsed time) - Section markers for key execution stages +- **Structured audit records** — one JSON object per line, prefixed `{"event":"mxc.` + +### Structured audit records + +Alongside the human-readable prose above, both sinks carry machine-readable +audit records: process exit / timeout / kill outcome, enforcement-tier +degradation, policy hash, network policy applied, sandbox teardown, config +rejection, and the sandbox identity join key. + +They are written **only** to the sinks described here — a `--log-file` path or +the `MXC_DIAG_CONSOLE` pipe — never to stdout, so they cannot pollute an SDK +caller's captured output. With neither sink configured, nothing is emitted and +no record is even built. + +These are **local diagnostics, not ETW telemetry**: no provider, no consent gate, +nothing uploaded. See +[`docs/telemetry/telemetry.md` § Local audit log records](telemetry/telemetry.md#local-audit-log-records) +for the JSON-lines format, the full record inventory with fields, the content +rules (bounded vocabularies, config field *paths* but never values, no raw user +identifiers), +and a parsing recipe. ## Diagnostic Console @@ -94,4 +120,16 @@ messages work without elevation. ## Scope -Diagnostic logging currently covers the **BaseContainer runner only**. +The **prose** diagnostic logging described above currently covers the +**BaseContainer runner only**. + +The **structured audit records** have a different (and also partial) scope: they +are emitted from the Windows ProcessContainer runners (BaseContainer and both +AppContainer tiers) and from `wxc-exec`'s config-rejection and state-aware +dispatch paths. `mxc.PolicyHash` is cross-platform (it is emitted by the shared +engine), but the lifecycle records are **not**: `lxc-exec` (LXC, Bubblewrap) and +`mxc-exec-mac` (Seatbelt) emit no `mxc.ProcessExited`, `mxc.SandboxTornDown`, +`mxc.ConfigRejected`, or `mxc.SandboxIdentity`. That is a known gap, not a +statement that those backends are uninteresting — the originating requirement was +Windows-only. Do not read a missing record on Linux or macOS as "the event did not +happen". diff --git a/docs/telemetry/telemetry.md b/docs/telemetry/telemetry.md index 6de0c1117..fa878b689 100644 --- a/docs/telemetry/telemetry.md +++ b/docs/telemetry/telemetry.md @@ -4,6 +4,12 @@ MXC uses the Rust [`tracelogging`](https://crates.io/crates/tracelogging) crate (published by Microsoft) for TraceLogging ETW telemetry. No C++ shim, WIL, or FFI is required. +> **Not to be confused with the local audit log.** MXC also writes structured +> **local diagnostic audit records** that are *not* ETW, *not* uploaded, and +> *not* consent-gated. They are a separate mechanism with a separate sink; see +> [Local audit log records](#local-audit-log-records) below. Nothing in that +> section touches the `Microsoft.MXC` provider described here. + ## Overview ``` @@ -47,6 +53,8 @@ Rust constants and `write_event!` struct fields. The remaining gap (activity tracking) is not needed for current events. If needed later, it can be added incrementally. + + ## Common Event Fields (Part C) Every MXC telemetry event includes a `COMMON_MXC_PARAMS` struct grouping @@ -231,6 +239,243 @@ free-form text. | Linux | No-op — all telemetry functions return immediately | | macOS | No-op — all telemetry functions return immediately | +--- + +## Local audit log records + +Separate from the ETW telemetry above, MXC emits **structured local diagnostic +audit records**: one JSON object per line, written only to the auxiliary +diagnostic sinks. They exist so facts MXC already knows — a process exit code, a +tier fallback, a teardown result, a rejected config — are *machine-readable* +instead of being discarded or rendered only as prose. + +**These are not ETW events.** No provider, no keyword, no privacy tag, no +correlation vector, and no consent gate. Nothing leaves the host. + +### Enabling them + +A record is written when — and only when — a diagnostic sink is attached. There +is no additional flag: + +```powershell +# File sink: every audit record lands here alongside the normal diagnostics. +wxc-exec.exe --log-file .\mxc-audit.log .\config.json + +# Named-pipe sink (Windows): use the same high-entropy token in both processes. +$env:MXC_DIAG_PIPE_TOKEN = [guid]::NewGuid().ToString('N') +$env:MXC_DIAG_PIPE_TOKEN +# Terminal 1: start the console with the shared token. +mxc-diagnostic-console.exe + +# Terminal 2: enable the pipe sink for wxc-exec with the same token. +$env:MXC_DIAG_PIPE_TOKEN = '' +$env:MXC_DIAG_CONSOLE = '1' +wxc-exec.exe .\config.json +``` + +With neither configured, the emit call is a cheap no-op. + +Records deliberately do **not** reach the primary console/buffer sink — that +channel is the SDK caller's captured stdout / debug buffer, and writing to it +would change the observable output of every existing consumer. The diagnostic +logger routes them only to the explicitly configured auxiliary sinks. + +### Format + +``` +{"event":"mxc.ProcessExited","backend":"processcontainer","identity":"sandbox-a3f1c8e40029bd17","tier":"base-container","pid":1234,"exit_code":0} +``` + +As it appears in a `--log-file` (the existing `[] ` stamp is +prepended by the file sink): + +``` +[1785549205] {"event":"mxc.ProcessExited","backend":"processcontainer",…} +``` + +The following invariants are enforced by MXC: + +1. **`event` is always the first key**, so a consumer can classify a line with a + prefix match (`{"event":"mxc.`) before parsing it. +2. **Field order is the call site's declaration order** — deterministic per + record. +3. **Values are strings, integers, or booleans only.** No nested objects, no + arrays; a set-valued field is a comma-joined bounded string plus an explicit + `_count` companion. +4. **String values are `serde_json`-escaped**, so an embedded quote, backslash, + or newline can never break the one-record-per-line invariant. +5. **Event names come from the closed `AuditEventName` enum** — a typo is a + compile error, not a silently unmatched record. + +Parsing is therefore just `ConvertFrom-Json` / `jq`: + +```powershell +Get-Content .\mxc-audit.log | + ForEach-Object { if ($_ -match '^\[\d+\]\s(\{.*\})$') { $Matches[1] } } | + ConvertFrom-Json | + Where-Object { $_.event -like 'mxc.*' } | + Format-Table event, backend, identity, tier +``` + +### Content rules + +The same bounded-vocabulary discipline as the ETW events applies, and for the +same reason: a diagnostic log file is routinely attached to a bug report or +collected by a fleet log agent. + +* **No free-form text.** Reasons, statuses, and methods are closed enums with an + `as_str()`; error detail is reduced to a numeric code. +* **No config values, no filesystem paths, no command lines.** Config field + *paths* (`process.commandLine`) are permitted — they are bounded and already + public in the schema. Field *values* are not. +* **No raw user identities.** Identity-bearing sandbox records use a constant + redaction marker instead of a user identifier. A truncated SHA-256 is not used: + a low-entropy identity could be recovered by dictionary attack. The cost is + that these sandboxes have no MXC-side join key in the local log. +* **No caller-supplied identifiers verbatim.** Sandbox identities derived from + configuration are retained only when they are bounded, opaque tokens (ASCII + alphanumeric plus `-`, `_`, `.`, ≤64 chars); anything else is replaced with + `redacted`. +* **Counts, not names, for network rules.** Rule names can contain host and + process identifiers, so only counts are recorded. + +### Record inventory + +Fields are record-specific. Process-boundary records include `backend`, +`identity`, `tier` (for `process_container`), and `pid`. Early records emitted +before a sandbox exists carry only the fields shown in the table below; in +particular, `mxc.PolicyHash` has `backend`, `policy_hash`, and +`config_schema_version`, `mxc.EnforcementDegraded` has `backend` and `tier`, +and `mxc.ConfigRejected` has `backend` plus its rejection fields. + +| Record | When | Fields beyond the common ones | +|---|---|---| +| `mxc.PolicyHash` | Every launch, after the effective request is resolved | `policy_hash`, `config_schema_version` | +| `mxc.SandboxIdentity` | After a successful state-aware phase | `phase` | +| `mxc.EnforcementDegraded` | ProcessContainer dispatch resolved below the preferred tier | `needs_dacl_augmentation`, `effective_enforcement_level`, `degradation_reasons`, `degradation_reason_count` | +| `mxc.NetworkPolicyApplied` | After network policy setup, on success **and** failure | `backend`, `identity`, `tier` (no `pid` yet), plus `enforcement_mode`, `default_policy`, `proxy_port`, `firewall_rules_created`, `firewall_applied`, `status` | +| `mxc.ProcessExited` | Sandboxed process exited on its own | `exit_code` | +| `mxc.ProcessTimedOut` | `scriptTimeout` breached | `timeout_ms` | +| `mxc.ProcessKillFailed` | A kill/terminate call failed (**failure only**) | `kill_method`, `error_code` | +| `mxc.SandboxTornDown` | Per-run resources released, once per handle | `status`, `firewall_rules_removed`, `firewall_removal_ok`, `bfs_removed`, `proxy_stopped`, `preserve_policy`, `container_released`, `skip_reason` | +| `mxc.ConfigRejected` | A request was refused before it could run | `reason`, `offending_field`, `phase` | + +Notes on the ones that are easy to misread: + +* **`mxc.EnforcementDegraded` is absent on a clean run.** It fires only when + selected enforcement is below the preferred level, additional host setup was + needed, or a bounded reason was recorded. The effective level is a closed MXC + vocabulary describing the enforcement mechanism selected by MXC; it is not an + assertion about an independent OS telemetry field. The streaming path emits + the record *before* the spawn, so it exists even when the spawn then fails. +* **`mxc.ProcessKillFailed` is not automatically a defect.** Termination can + race with normal process exit. The record is captured but never propagated — + the kill path stays best-effort and non-fatal. +* **`mxc.SandboxTornDown` reports unavailable cleanup honestly.** If a cleanup + operation is not implemented for a backend, the record reports that fact + rather than claiming a cleanup that did not happen. +* **`mxc.PolicyHash` covers the *policy*, not the command.** See below. + +### The policy hash + +MXC produces `sha256:<64 hex>` over an explicit **allow-list** projection of the +effective request, canonicalised (object keys sorted at every depth, array order +preserved). It is computed after every policy-affecting mutation, so it +describes what actually ran, not what was requested. + +An allow-list is deliberate: a field added to the model later is excluded until +someone opts it in, which fails safe rather than accidentally hashing a secret. +To stop that from rotting into a silent coverage gap, the projection +**exhaustively destructures** `ExecutionRequest` and `ExperimentalConfig` — adding +a field to either is a compile error until it is classified. + +Excluded, and why: + +| Excluded | Reason | +|---|---| +| `script_code` | The command line is *what runs*, not the policy it runs under; it routinely embeds credentials. | +| `env` | Environment variables are the classic secret carrier. | +| `experimental.telemetry`, `experimental.test` | No enforcement effect. | +| `experimental.*.user` | Carries identity credentials. Stripped recursively; the rest of the isolation-session section *is* hashed. | +| proxy `original_url` | Can embed `user:password@`. The host and port *are* hashed. | +| `dry_run`, `testing_features_enabled` | Invocation modes, not policy. | + +The rest of `experimental` **is** hashed — `windows_sandbox`, `wslc`, and +`isolation_session` carry those backends' entire enforcement policy, so omitting +them would make two materially different policies hash identically. + +`config_schema_version` is named for the *schema*: it does not change when the +policy changes and must never be read as a policy version. + +**Residual disclosure property (accepted).** The hash is deterministic and +unkeyed, so it is a confirmation oracle for the fields it covers: a reader who +already knows every hashed field but one can brute-force the remaining one. In +practice that means testing a guess at a single `readwritePaths` entry while +already knowing the container id, working directory, timeout, capability list, +and every other path exactly. This is accepted because the alternative (a keyed +digest) needs a machine-local secret whose storage and rotation are out of scope +for a local log, and because the genuinely sensitive inputs — command line, +environment, tokens, proxy userinfo — are excluded from the hash entirely, so no +oracle exists for them at any difficulty. **Do not add a low-entropy secret to +the projection without switching to a keyed construction first.** + +### Platform scope + +The records are **not** uniformly available across platforms, and a missing +record must not be read as "the event did not happen": + +| Record | Windows ProcessContainer | Windows state-aware | Linux (LXC / Bubblewrap) | macOS (Seatbelt) | +|---|---|---|---|---| +| `mxc.PolicyHash` | ✅ | ✅ | ✅ | ✅ | +| `mxc.SandboxIdentity` | — | ✅ | — | — | +| `mxc.ConfigRejected` | ✅ (`wxc-exec`) | ✅ | — | — | +| `mxc.EnforcementDegraded` | ✅ | — | n/a (no tier model) | n/a | +| `mxc.NetworkPolicyApplied` | ✅ (T2/T3) | — | — | — | +| `mxc.ProcessExited` / `TimedOut` / `KillFailed` | ✅ (including isolation-session one-shot) | — | — | — | +| `mxc.SandboxTornDown` | ✅ | — | — | — | + +The shared audit types compile identically on all three platforms; the gap is +that the *emission sites* were added only to the +Windows runners and the `wxc-exec` binary, because the originating requirement +was Windows-only. Extending them to `lxc-exec` and `mxc-exec-mac` is tracked +follow-up work, not a design decision that Linux and macOS do not need an audit +trail. + +The isolation-session `ProcessTimedOut` and `ProcessKillFailed` records are +emitted by the one-shot runner, where `wxc-exec` supplies the local diagnostic +logger to the backend. The state-aware backend trait does not currently carry a +logger into its `exec` method, so state-aware isolation-session exec remains +unrecorded by these MXC-local process-boundary events. + + + +| Requirement | Existing OS coverage | MXC local coverage | Join/correlation notes | +|---|---|---|---| +| M-ETW-1 process outcome | Existing OS process-lifecycle records cover normal exit. The OS does not provide a verified timeout or kill-failure record for this requirement. | `mxc.ProcessTimedOut` and `mxc.ProcessKillFailed` cover the one-shot MXC boundary; state-aware exec is not currently logger-backed. | Join the OS lifecycle identity to the MXC sandbox identity where available; use the process ID for process records. | +| M-ETW-2 enforcement degradation | Not applicable to this backend: `isolation_session` has no MXC process-container tier/fallback model. | `mxc.EnforcementDegraded` covers process-container tier selection and includes `effective_enforcement_level`. | No isolation-session tier join is expected. | +| M-ETW-3 policy hash | No policy hash field is emitted by the isolation-session OS provider. | `mxc.PolicyHash` records the effective MXC policy locally, excluding secrets and command content. | Correlate by the invocation/lifecycle context; the hash is an MXC record, not an OS field. | +| M-ETW-4 network policy | Not applicable today: MXC rejects isolation-session network and proxy policy before OS provisioning. | `mxc.NetworkPolicyApplied` covers supported process-container network setup only. | This row changes only if the separate M1 network-proxy requirement is implemented. | +| M-ETW-5 teardown | Existing OS lifecycle and security records cover OS cleanup. | `mxc.SandboxTornDown` covers supported process-container cleanup; isolation-session phase outcomes remain in the existing lifecycle records. | Join by the lifecycle identity where available; OS cleanup may outlive the MXC process boundary. | +| M-ETW-6 provider selection and correlation | `mxc.ConfigRejected` records MXC-owned rejection reason, field, and phase locally. | MXC validation commonly occurs before the OS call, so no OS rejection event should be expected for those records. | + +This table documents coverage and correlation; it does not convert the local +audit records into OS telemetry. OS event names and capture procedures vary by +OS build and are outside this local audit format. + +### Stability contract + +The `event` name is the anchor. If a record's field set has to change +incompatibly, mint a new record name rather than redefining an existing one. +New record names and new fields are additive; a consumer that filters on +`{"event":"mxc.` sees only what it recognises. + +The shared record format is cross-platform, so Windows, Linux, and macOS +compile and test the same serialization code. +See [Platform scope](#platform-scope) for which *emission sites* exist where — +that is where the real asymmetry lives. + +--- + ## Private GUID Substitution (Internal Builds) MXC supports an optional Microsoft telemetry group GUID for internal builds. @@ -258,68 +503,3 @@ name using the standard ETW name-hash algorithm (the same algorithm used by `{7f10def4-a258-5fea-510e-2c3bb976687f}`. Keeping the name and GUID in lockstep this way prevents drift and avoids hard-coding a literal that could collide with another team's GUID. - -### CI pipeline steps - -Internal Microsoft builds set `MXC_TELEMETRY_PROVIDER_GROUP_GUID` to the real -Microsoft telemetry group GUID before `cargo build` on Windows, so events route -through the telemetry pipeline. Community forks that lack access to the private -GUID do not set this variable — the provider is registered without a group GUID -(plain ETW only). - -> **Follow-up:** The provider group GUID is now provided by a secret variable -> on the official Windows build pipeline, so official builds can route events -> through the telemetry pipeline. The build has always honored the variable -> (see *Local developer testing* below); public builds and community forks, -> which do not have access to the variable, continue to register the provider -> without a group GUID (plain ETW only). - -### Local developer testing - -```powershell -# Test with a dummy group GUID (not the real one) -$env:MXC_TELEMETRY_PROVIDER_GROUP_GUID = '00000000-1111-2222-3333-444444444444' -cargo build -p mxc_telemetry - -# Test without (public build) -Remove-Item Env:\MXC_TELEMETRY_PROVIDER_GROUP_GUID -cargo build -p mxc_telemetry -``` - -### What's public vs. private - -| Item | Public? | Why | -|------|---------|-----| -| Provider name `"Microsoft.MXC"` | ✅ | Standard ETW naming | -| Provider GUID `{7f10def4-a258-5fea-510e-2c3bb976687f}` | ✅ | Derived from the name; identifies the provider, harmless | -| `build.rs` env var mechanism | ✅ | Mechanism is public | -| `MXC_TELEMETRY_PROVIDER_GROUP_GUID` env var name | ✅ | Key is public; value is private | -| Actual Microsoft telemetry group GUID | ❌ | Private — set in CI only | - -## SDK License Override (EULA for npm Package) - -The public GitHub repo ships `sdk/node/LICENSE.md` as a plain MIT license. For -internal npm publishes, a separate EULA containing a **Section 2 — DATA** -clause (covering telemetry disclosure, opt-out, and GDPR) will be updated at -pack/publish time. - -### How it works - -``` -1. CI pipeline (or local script) sets MXC_LICENSE_OVERRIDE env var - pointing to the markdown file of the EULA including additional telemetry language. - Note that the new EULA will include language outlining what data can be collected but - will otherwise remain MIT licensed. - -2. A license-override script (added in a follow-up build-integration PR) runs: - ├── MXC_LICENSE_OVERRIDE is set: - │ ├── Back up sdk/node/LICENSE.md → sdk/node/LICENSE.md.public - │ └── Copy new EULA over sdk/node/LICENSE.md - └── MXC_LICENSE_OVERRIDE is NOT set: - └── Restore sdk/node/LICENSE.md from .public backup (if exists) - -3. npm pack / npm publish picks up the new EULA as the LICENSE.md - in the published package (sdk/node/package.json "files" includes LICENSE.md). - -4. After publish, the revert path restores the original EULA document. -``` diff --git a/src/Cargo.lock b/src/Cargo.lock index 15e7a1b9b..33df7f054 100644 --- a/src/Cargo.lock +++ b/src/Cargo.lock @@ -3148,6 +3148,7 @@ dependencies = [ "serde", "serde_json", "serde_path_to_error", + "sha2 0.10.9", "tempfile", "thiserror", "unicode-general-category", diff --git a/src/Cargo.toml b/src/Cargo.toml index 4a9f7e2ce..4901e0985 100644 --- a/src/Cargo.toml +++ b/src/Cargo.toml @@ -98,6 +98,8 @@ windows-core = "0.62" serde = { version = "1", features = ["derive"] } serde_json = "1" serde_path_to_error = "0.1" +# SHA-256 for the canonical policy hash (`wxc_common::policy_identity`). +sha2 = "0.10" unicode-general-category = "1" thiserror = "2" anyhow = "1" diff --git a/src/backends/appcontainer/common/src/appcontainer_runner.rs b/src/backends/appcontainer/common/src/appcontainer_runner.rs index 1539c830e..c6d2635d0 100644 --- a/src/backends/appcontainer/common/src/appcontainer_runner.rs +++ b/src/backends/appcontainer/common/src/appcontainer_runner.rs @@ -32,10 +32,15 @@ use windows_core::{PCWSTR, PWSTR}; use crate::job_object::UiJobObject; use crate::process_mitigation; +use wxc_common::audit::{ + sanitize_identity, AuditEvent, AuditEventName, KillMethod, OperationStatus, TeardownSkipReason, + TeardownStatus, +}; use wxc_common::error::WxcError; use wxc_common::logger::Logger; use wxc_common::models::{ - ExecutionRequest, FailurePhase, NetworkEnforcementMode, NetworkPolicy, ScriptResponse, + ContainmentBackend, ExecutionRequest, FailurePhase, NetworkEnforcementMode, NetworkPolicy, + ScriptResponse, }; use wxc_common::process_util::{ create_std_pipes, InterruptiblePipeReader, OwnedHandle, PipeReadCanceller, PipeWriter, @@ -395,6 +400,20 @@ pub enum FilesystemMode { Dacl, } +impl FilesystemMode { + /// The isolation tier this filesystem mode corresponds to. + /// + /// The dispatcher picks the mode *from* a tier and the audit records need + /// the tier back; keeping the mapping in one place stops the two directions + /// from drifting. + pub fn isolation_tier(self) -> crate::fallback_detector::IsolationTier { + match self { + FilesystemMode::Bfs => crate::fallback_detector::IsolationTier::AppContainerBfs, + FilesystemMode::Dacl => crate::fallback_detector::IsolationTier::AppContainerDacl, + } + } +} + /// Config capability string that enables **learning mode**: the OS logs every /// *failed* access check for the AppContainer, but the access is still /// **denied** (deny-and-record). Containment is unchanged. @@ -1272,13 +1291,55 @@ impl AppContainerScriptRunner { } let mut network_manager = NetworkManager::new(); - match network_manager.start( + let network_result = network_manager.start( &principal_id, &self.app_container_name, &request.policy, self.app_container_sid, logger, - ) { + ); + + // Record what network policy was actually installed, on both the + // success and failure arms — "the policy I tried to install and failed" + // is as auditable a fact as a successful one. + if logger.has_diagnostic_sink() { + let plan = NetworkManager::describe_policy(&request.policy); + let record = AuditEvent::new(AuditEventName::NetworkPolicyApplied) + .str("backend", ContainmentBackend::ProcessContainer.wire_name()) + .str("identity", sanitize_identity(&self.app_container_name)) + .str("tier", self.tier_str()) + .str( + "enforcement_mode", + request.policy.network_enforcement_mode.as_str(), + ) + .str( + "default_policy", + request.policy.default_network_policy.as_str(), + ) + .u64( + "proxy_port", + network_manager + .proxy_address() + .map(|a| a.port as u64) + .unwrap_or(0), + ) + .u64( + "firewall_rules_created", + network_manager.rule_count() as u64, + ) + .bool("firewall_applied", plan.rules_will_be_installed) + .str( + "status", + if network_result.is_ok() { + OperationStatus::Success.as_str() + } else { + OperationStatus::Failure.as_str() + }, + ); + logger.log_audit_event(&record); + } + + match network_result { Ok(()) => { self.proxy_address = network_manager.proxy_address().cloned(); } @@ -1295,14 +1356,41 @@ impl AppContainerScriptRunner { /// Tear down the per-run firewall and filesystem policy. Idempotent at the /// manager level; called once after the child exits. + /// + /// This path can run after network policy was installed but before the + /// child process was created, so cleanup failures must remain observable. fn teardown(&self, prepared: &mut Prepared, preserve_policy: bool, logger: &mut Logger) { - prepared.network_manager.stop_all(!preserve_policy, logger); + let network = prepared.network_manager.stop_all(!preserve_policy, logger); if self.filesystem_mode == FilesystemMode::Bfs && prepared.bfs_manager.configured() && !preserve_policy { prepared.bfs_manager.remove_configuration(logger); } + if logger.has_diagnostic_sink() { + let (status, skip_reason) = + appcontainer_teardown_status(preserve_policy, network.firewall_removal_ok); + let mut record = AuditEvent::new(AuditEventName::SandboxTornDown) + .str("backend", ContainmentBackend::ProcessContainer.wire_name()) + .str("identity", &self.app_container_name) + .str("tier", self.tier_str()) + .str("status", status.as_str()) + .u64("firewall_rules_removed", network.rules_removed as u64) + .bool("firewall_removal_ok", network.firewall_removal_ok) + .bool("proxy_stopped", network.proxy_stopped) + .bool("preserve_policy", preserve_policy) + .bool("container_released", false); + if let Some(reason) = skip_reason { + record = record.str("skip_reason", reason.as_str()); + } + logger.log_audit_event(&record); + } + } + + /// The isolation tier this runner implements, as + /// [`crate::fallback_detector::IsolationTier::as_str`]. + fn tier_str(&self) -> &'static str { + self.filesystem_mode.isolation_tier().as_str() } } @@ -1360,6 +1448,8 @@ impl SandboxBackend for AppContainerScriptRunner { prepared, self.filesystem_mode, request, + self.app_container_name.clone(), + logger, ))) } @@ -1394,6 +1484,19 @@ struct AppContainerSandboxProcess { preserve_policy: bool, timeout_ms: u32, teardown_done: bool, + kill_requested: bool, + /// Sandbox identity join key — the AppContainer profile name MXC created + /// this sandbox under. Carried so the audit records emitted from `wait()`, + /// `kill()`, and `run_teardown()` can be joined to the OS-side records + /// without recomputing it. + identity: String, + /// The isolation tier this handle is running under, as + /// [`crate::fallback_detector::IsolationTier::as_str`]. + tier: &'static str, + /// Detached clone of the caller's diagnostic sinks, so audit records emitted + /// from `wait()` / `Drop` (neither of which receives a `Logger`) are + /// actually observable. See [`Logger::clone_diagnostic_sink`]. + audit_logger: Logger, } // SAFETY: the fields are Windows HANDLEs / handle-owning managers and owned @@ -1420,6 +1523,8 @@ impl AppContainerSandboxProcess { prepared: Prepared, filesystem_mode: FilesystemMode, request: &ExecutionRequest, + identity: String, + logger: &Logger, ) -> Self { let process = SendOwnedHandle::take(&mut child.process); let thread = SendOwnedHandle::take(&mut child.thread); @@ -1428,6 +1533,7 @@ impl AppContainerSandboxProcess { let stderr = child.stderr_read.take().map(InterruptiblePipeReader::new); let stdout_canceller = stdout.as_ref().map(InterruptiblePipeReader::canceller); let stderr_canceller = stderr.as_ref().map(InterruptiblePipeReader::canceller); + let tier = filesystem_mode.isolation_tier().as_str(); Self { process, _thread: thread, @@ -1443,24 +1549,93 @@ impl AppContainerSandboxProcess { preserve_policy: request.lifecycle.preserve_policy, timeout_ms: child.timeout_ms, teardown_done: false, + kill_requested: false, + // The AppContainer profile name is the caller's `containerId`, i.e. + // a config value — sanitize before it can reach a record. + identity: sanitize_identity(&identity).to_string(), + tier, + audit_logger: logger.clone_diagnostic_sink(), } } + /// Start an audit record pre-populated with this handle's attribution + /// (backend, identity, tier, pid) so no call site can forget one of them. + fn audit(&self, name: AuditEventName) -> AuditEvent { + AuditEvent::new(name) + .str("backend", ContainmentBackend::ProcessContainer.wire_name()) + .str("identity", &self.identity) + .str("tier", self.tier) + .u64("pid", self.pid as u64) + } + + /// Whether an audit record built here would reach a sink. Checked before + /// building one so a run with no diagnostic sink pays nothing. + fn audit_enabled(&self) -> bool { + self.audit_logger.has_diagnostic_sink() + } + fn run_teardown(&mut self) { if self.teardown_done { return; } self.teardown_done = true; let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); - self.prepared + let network = self + .prepared .network_manager .stop_all(!self.preserve_policy, &mut logger); - if self.filesystem_mode == FilesystemMode::Bfs + let bfs_removed = self.filesystem_mode == FilesystemMode::Bfs && self.prepared.bfs_manager.configured() - && !self.preserve_policy - { + && !self.preserve_policy; + if bfs_removed { self.prepared.bfs_manager.remove_configuration(&mut logger); } + + if !self.audit_enabled() { + return; + } + let (status, skip_reason) = + appcontainer_teardown_status(self.preserve_policy, network.firewall_removal_ok); + let mut record = self + .audit(AuditEventName::SandboxTornDown) + .str("status", status.as_str()) + .u64("firewall_rules_removed", network.rules_removed as u64) + .bool("firewall_removal_ok", network.firewall_removal_ok) + .bool("bfs_removed", bfs_removed) + .bool("proxy_stopped", network.proxy_stopped) + .bool("preserve_policy", self.preserve_policy) + // The AppContainer tiers delete no profile here (the runner's own + // `Drop` owns profile deletion), so this handle releases no + // container. + .bool("container_released", false); + if let Some(reason) = skip_reason { + record = record.str("skip_reason", reason.as_str()); + } + self.audit_logger.log_audit_event(&record); + } +} + +/// Decide the audit `status` / `skip_reason` for an AppContainer teardown. +/// +/// Pure so the mapping is unit-testable without launching a sandbox: the +/// emission site in `run_teardown` only supplies the two facts it already has. +/// +/// `preserve_policy` is a deliberate request to leave enforcement in place, so +/// it is `skipped` rather than a success or a failure — the record must not +/// claim a release that was never attempted. +fn appcontainer_teardown_status( + preserve_policy: bool, + firewall_removal_ok: bool, +) -> (TeardownStatus, Option) { + if preserve_policy { + ( + TeardownStatus::Skipped, + Some(TeardownSkipReason::PreservePolicy), + ) + } else if firewall_removal_ok { + (TeardownStatus::Success, None) + } else { + (TeardownStatus::Failure, None) } } @@ -1506,7 +1681,23 @@ impl SandboxProcess for AppContainerSandboxProcess { fn kill(&mut self) -> std::io::Result<()> { // Terminate the whole job: the child and every descendant assigned to // it die together (tree-kill). - self.job.terminate(u32::MAX); + // + // A failure here is recorded but never propagated: this is a + // best-effort cleanup path whose most common failure is "the job's + // processes already exited", and every caller (`wait()`, `Drop`, and + // the `SandboxProcess` consumer) relies on it staying infallible. + if let Err(error) = self.job.terminate(u32::MAX) { + if !self.audit_enabled() { + return Ok(()); + } + let record = self + .audit(AuditEventName::ProcessKillFailed) + .str("kill_method", KillMethod::TerminateJobObject.as_str()) + .i64("error_code", error.code().0 as i64); + self.audit_logger.log_audit_event(&record); + } else { + self.kill_requested = true; + } Ok(()) } @@ -1527,13 +1718,28 @@ impl SandboxProcess for AppContainerSandboxProcess { if unsafe { GetExitCodeProcess(self.process.get(), &mut code) }.is_err() { Err(std::io::Error::other("GetExitCodeProcess failed")) } else { - Ok(code as i32) + let exit_code = code as i32; + if self.audit_enabled() && !self.kill_requested { + let record = self + .audit(AuditEventName::ProcessExited) + .i64("exit_code", exit_code as i64); + self.audit_logger.log_audit_event(&record); + } + Ok(exit_code) + } + } + WAIT_TIMEOUT => { + if self.audit_enabled() { + let record = self + .audit(AuditEventName::ProcessTimedOut) + .u64("timeout_ms", self.timeout_ms as u64); + self.audit_logger.log_audit_event(&record); } + Err(std::io::Error::new( + std::io::ErrorKind::TimedOut, + format!("script timed out after {}ms", self.timeout_ms), + )) } - WAIT_TIMEOUT => Err(std::io::Error::new( - std::io::ErrorKind::TimedOut, - format!("script timed out after {}ms", self.timeout_ms), - )), _ => Err(std::io::Error::other("WaitForSingleObject failed")), }; @@ -1571,6 +1777,54 @@ impl Drop for AppContainerSandboxProcess { #[cfg(test)] mod tests { + /// `preserve_policy` is an explicit request to leave enforcement in place, + /// so it must be reported as `skipped` — never as a green `success` that + /// implies a release the code never attempted, and never as a `failure`. + #[test] + fn teardown_status_reports_preserve_policy_as_skipped() { + use super::appcontainer_teardown_status; + use wxc_common::audit::{TeardownSkipReason, TeardownStatus}; + + for firewall_ok in [true, false] { + let (status, reason) = appcontainer_teardown_status(true, firewall_ok); + assert_eq!(status, TeardownStatus::Skipped); + assert_eq!(reason, Some(TeardownSkipReason::PreservePolicy)); + } + } + + /// A partially-failed firewall removal must be distinguishable from a clean + /// one — that distinction is the whole reason `stop_all` stopped discarding + /// its `Result`. + #[test] + fn teardown_status_distinguishes_failed_firewall_removal() { + use super::appcontainer_teardown_status; + use wxc_common::audit::TeardownStatus; + + assert_eq!( + appcontainer_teardown_status(false, true), + (TeardownStatus::Success, None) + ); + assert_eq!( + appcontainer_teardown_status(false, false), + (TeardownStatus::Failure, None) + ); + } + + #[test] + fn filesystem_mode_maps_to_exactly_one_tier() { + use super::FilesystemMode; + use crate::fallback_detector::IsolationTier; + + assert_eq!( + FilesystemMode::Bfs.isolation_tier(), + IsolationTier::AppContainerBfs + ); + assert_eq!( + FilesystemMode::Dacl.isolation_tier(), + IsolationTier::AppContainerDacl + ); + } + #[test] fn attr_count_neither() { assert_eq!(super::compute_attr_count(false, false, false), 1); diff --git a/src/backends/appcontainer/common/src/base_container_runner.rs b/src/backends/appcontainer/common/src/base_container_runner.rs index 58ee4cf4f..369ff8408 100644 --- a/src/backends/appcontainer/common/src/base_container_runner.rs +++ b/src/backends/appcontainer/common/src/base_container_runner.rs @@ -41,6 +41,7 @@ use windows::Win32::System::Threading::{ }; use windows_core::{PCWSTR, PWSTR}; +use crate::fallback_detector::IsolationTier; use crate::job_object::UiJobObject; use crate::launch_diagnostics::{ diagnose_create_process_failure, diagnose_environment_not_supported, diagnose_process_exit, @@ -52,13 +53,16 @@ use sandbox_spec::base_container_layout::{ finish_sandbox_spec_buffer, proxy_info, proxy_infoArgs, IntegrityLevel, NetworkPolicy as FbsNetworkPolicy, NetworkPolicyArgs, SandboxSpec, SandboxSpecArgs, }; +use wxc_common::audit::{ + sanitize_identity, AuditEvent, AuditEventName, KillMethod, TeardownSkipReason, TeardownStatus, +}; use wxc_common::log_symbols::{ EMOJI_ALLOWED, EMOJI_BLOCKED, EMOJI_NEUTRAL, EMOJI_SECTION, EMOJI_WARNING, }; use wxc_common::logger::Logger; use wxc_common::models::{ - CaptureDenialsOutput, ExecutionRequest, FailurePhase, NetworkEnforcementMode, NetworkPolicy, - ProxyAddress, SandboxOutputMetadata, ScriptResponse, + CaptureDenialsOutput, ContainmentBackend, ExecutionRequest, FailurePhase, + NetworkEnforcementMode, NetworkPolicy, ProxyAddress, SandboxOutputMetadata, ScriptResponse, }; use wxc_common::process_util::{ create_std_pipes, InterruptiblePipeReader, OwnedHandle, PipeReadCanceller, PipeWriter, @@ -1557,6 +1561,7 @@ impl BaseContainerRunner { identity, sid_string, proxy_coordinator: std::mem::take(&mut self.proxy_coordinator), + preserve_policy: request.lifecycle.preserve_policy, capture_session, capture_etl_path, capture_output_path, @@ -1586,6 +1591,9 @@ struct BaseChild { identity: String, sid_string: String, proxy_coordinator: ProxyCoordinator, + /// `lifecycle.preservePolicy`, carried so the teardown record can report it + /// rather than inferring it from `destroy_on_exit` (a different field). + preserve_policy: bool, /// Live learning-mode capture session (`Some` only when `captureDenials` /// is configured and the OS API is available). Sealed in `run_teardown` /// after the child exits. @@ -1646,7 +1654,9 @@ impl SandboxBackend for BaseContainerRunner { // the binary's own std handles / console (a TTY when the binary has one). let capture = stdio == StdioMode::Pipes; let child = self.spawn_base(request, logger, capture)?; - Ok(Box::new(BaseContainerSandboxProcess::from_child(child))) + Ok(Box::new(BaseContainerSandboxProcess::from_child( + child, logger, + ))) } fn diagnose_exit(&self, request: &ExecutionRequest, exit_code: i32) -> Option { @@ -1677,6 +1687,10 @@ struct BaseContainerSandboxProcess { stderr_canceller: Option, timeout_ms: u32, destroy_on_exit: bool, + /// `lifecycle.preservePolicy` — a request to leave enforcement in place + /// after the run. Distinct from `destroy_on_exit`; carried so the teardown + /// record reports it truthfully instead of inferring it. + preserve_policy: bool, proxy_enabled: bool, identity: String, sid_string: String, @@ -1684,6 +1698,7 @@ struct BaseContainerSandboxProcess { /// Cached teardown outcome so repeated terminal waits cannot hide a /// capture failure after the session has been consumed. teardown_result: Option>, + kill_requested: bool, /// Live learning-mode capture session, moved from the `BaseChild`. Sealed /// in `run_teardown` once the child has exited and been reaped. capture_session: Option>, @@ -1691,6 +1706,10 @@ struct BaseContainerSandboxProcess { capture_etl_path: Option, /// Resolved JSON denials deliverable path. capture_output_path: Option, + /// Detached clone of the caller's diagnostic sinks, so audit records emitted + /// from `wait()` / `Drop` (neither of which receives a `Logger`) are + /// actually observable. See [`Logger::clone_diagnostic_sink`]. + audit_logger: Logger, /// Exit code of the child, recorded by `wait` before teardown so the /// denials summary can carry it. `None` on the `Drop`/early-exit path. last_exit_code: Option, @@ -1705,7 +1724,7 @@ struct BaseContainerSandboxProcess { unsafe impl Send for BaseContainerSandboxProcess {} impl BaseContainerSandboxProcess { - fn from_child(mut child: BaseChild) -> Self { + fn from_child(mut child: BaseChild, logger: &Logger) -> Self { let process = SendOwnedHandle::take(&mut child.process); let thread = SendOwnedHandle::take(&mut child.thread); let stdin = child.stdin_write.take().map(PipeWriter::new); @@ -1725,19 +1744,41 @@ impl BaseContainerSandboxProcess { stderr_canceller, timeout_ms: child.timeout_ms, destroy_on_exit: child.destroy_on_exit, + preserve_policy: child.preserve_policy, proxy_enabled: child.proxy_enabled, - identity: std::mem::take(&mut child.identity), + // On this tier MXC mints the identity itself (`sandbox-<16 hex>`) + // when `destroy_on_exit` is set, but otherwise it is the caller's + // `containerId` — a config value. Sanitize either way. + identity: sanitize_identity(&std::mem::take(&mut child.identity)).to_string(), sid_string: std::mem::take(&mut child.sid_string), proxy_coordinator: std::mem::take(&mut child.proxy_coordinator), teardown_result: None, + kill_requested: false, capture_session: child.capture_session.take(), capture_etl_path: child.capture_etl_path.take(), capture_output_path: child.capture_output_path.take(), + audit_logger: logger.clone_diagnostic_sink(), last_exit_code: None, output_metadata: None, } } + /// Start an audit record pre-populated with this handle's attribution + /// (backend, identity, tier, pid) so no call site can forget one of them. + fn audit(&self, name: AuditEventName) -> AuditEvent { + AuditEvent::new(name) + .str("backend", ContainmentBackend::ProcessContainer.wire_name()) + .str("identity", &self.identity) + .str("tier", IsolationTier::BaseContainer.as_str()) + .u64("pid", self.pid as u64) + } + + /// Whether an audit record built here would reach a sink. Checked before + /// building one so a run with no diagnostic sink pays nothing. + fn audit_enabled(&self) -> bool { + self.audit_logger.has_diagnostic_sink() + } + fn run_teardown(&mut self) -> std::io::Result<()> { if let Some(result) = &self.teardown_result { return result.clone().map_err(std::io::Error::other); @@ -1811,21 +1852,74 @@ impl BaseContainerSandboxProcess { } sandbox_tracking::unregister_ctrl_c_cleanup(); } + let proxy_stopped = self.proxy_coordinator.is_active(); self.proxy_coordinator.stop(&mut logger); let result = capture_result .map(|_| ()) .map_err(|error| error.to_string()); + self.log_teardown(&result, proxy_stopped); self.teardown_result = Some(result.clone()); result.map_err(std::io::Error::other) } + /// Record `mxc.SandboxTornDown` for this handle. + /// + /// **This deliberately reports the Tier 1 cleanup stub honestly.** + /// `run_sandbox_cleanup` is a documented no-op — it ignores `identity`, + /// `sid_string`, and `proxy_enabled` because child-process tracking is not + /// implemented — and the OS does not release per-sandbox state either. So + /// the record reports `container_released = false` and a `skipped` status, + /// turning a silent product gap into an auditable one. Claiming a green + /// teardown here would be worse than emitting nothing. + fn log_teardown(&mut self, capture_result: &Result<(), String>, proxy_stopped: bool) { + if !self.audit_enabled() { + return; + } + let (status, skip_reason) = base_container_teardown_status( + capture_result.is_err(), + self.destroy_on_exit, + self.preserve_policy, + ); + let mut record = self + .audit(AuditEventName::SandboxTornDown) + .str("status", status.as_str()) + // Tier 1 installs no MXC firewall rules at all: the enforcement is + // the OS sandbox plus the cooperative proxy. Explicit zeros make + // that an auditable positive fact rather than an absence. + .u64("firewall_rules_removed", 0) + .bool("firewall_removal_ok", true) + .bool("bfs_removed", false) + .bool("proxy_stopped", proxy_stopped) + .bool("preserve_policy", self.preserve_policy) + .bool("container_released", false); + if let Some(reason) = skip_reason { + record = record.str("skip_reason", reason.as_str()); + } + self.audit_logger.log_audit_event(&record); + } + fn kill_process_tree(&mut self) -> std::io::Result<()> { - if let Some(job) = &self.job { - job.terminate(u32::MAX); + // Best-effort, as on the AppContainer path: a failure is recorded for + // audit and never propagated, because the most common cause is that the + // target has already exited and every caller depends on this staying + // infallible. + let outcome = if let Some(job) = &self.job { + job.terminate(u32::MAX) + .map_err(|e| (KillMethod::TerminateJobObject, e)) } else { - unsafe { - let _ = TerminateProcess(self.process.get(), u32::MAX); + // SAFETY: `self.process` is a valid, owned process handle. + unsafe { TerminateProcess(self.process.get(), u32::MAX) } + .map_err(|e| (KillMethod::TerminateProcess, e)) + }; + if let Err((method, error)) = outcome { + if !self.audit_enabled() { + return Ok(()); } + let record = self + .audit(AuditEventName::ProcessKillFailed) + .str("kill_method", method.as_str()) + .i64("error_code", error.code().0 as i64); + self.audit_logger.log_audit_event(&record); } Ok(()) } @@ -1980,6 +2074,42 @@ fn insert_run_id_into_stem(path: &Path, run_id: &str) -> PathBuf { } } +/// Decide the audit `status` / `skip_reason` for a BaseContainer teardown. +/// +/// Pure so the mapping is unit-testable without launching a sandbox. The order +/// of the arms is the contract: +/// +/// 1. a failed denial-capture finalisation is a real failure and outranks +/// everything else; +/// 2. `preserve_policy` is an explicit request to leave enforcement in place, so +/// it is `skipped` — matching the AppContainer tiers, which report the same +/// thing for the same request; +/// 3. `destroy_on_exit` asks for per-sandbox cleanup that `run_sandbox_cleanup` +/// does not actually implement, so it is `skipped` with the honest reason +/// rather than a green `success`; +/// 4. otherwise nothing was left behind that the caller did not ask for. +fn base_container_teardown_status( + capture_failed: bool, + destroy_on_exit: bool, + preserve_policy: bool, +) -> (TeardownStatus, Option) { + if capture_failed { + (TeardownStatus::Failure, None) + } else if preserve_policy { + ( + TeardownStatus::Skipped, + Some(TeardownSkipReason::PreservePolicy), + ) + } else if destroy_on_exit { + ( + TeardownStatus::Skipped, + Some(TeardownSkipReason::CleanupNotImplemented), + ) + } else { + (TeardownStatus::Success, None) + } +} + impl SandboxProcess for BaseContainerSandboxProcess { fn output_metadata(&self) -> Option<&SandboxOutputMetadata> { self.output_metadata.as_ref() @@ -2027,6 +2157,7 @@ impl SandboxProcess for BaseContainerSandboxProcess { } fn kill(&mut self) -> std::io::Result<()> { + self.kill_requested = true; // Tree-kill via the job object when the child was successfully assigned // to one; otherwise fall back to terminating the root process. self.kill_process_tree() @@ -2049,13 +2180,28 @@ impl SandboxProcess for BaseContainerSandboxProcess { if unsafe { GetExitCodeProcess(self.process.get(), &mut code) }.is_err() { Err(std::io::Error::other("GetExitCodeProcess failed")) } else { - Ok(code as i32) + let exit_code = code as i32; + if self.audit_enabled() && !self.kill_requested { + let record = self + .audit(AuditEventName::ProcessExited) + .i64("exit_code", exit_code as i64); + self.audit_logger.log_audit_event(&record); + } + Ok(exit_code) } } - WAIT_TIMEOUT => Err(std::io::Error::new( - std::io::ErrorKind::TimedOut, - format!("script timed out after {}ms", self.timeout_ms), - )), + WAIT_TIMEOUT => { + if self.audit_enabled() { + let record = self + .audit(AuditEventName::ProcessTimedOut) + .u64("timeout_ms", self.timeout_ms as u64); + self.audit_logger.log_audit_event(&record); + } + Err(std::io::Error::new( + std::io::ErrorKind::TimedOut, + format!("script timed out after {}ms", self.timeout_ms), + )) + } _ => Err(std::io::Error::other("WaitForSingleObject failed")), }; @@ -2174,6 +2320,47 @@ mod tests { use wxc_common::models::{ClipboardPolicy, ProxyConfig, UiPolicy}; use wxc_common::ui_policy::EffectiveUiRestrictions; + /// Tier 1's `run_sandbox_cleanup` is a documented no-op, so a run that asked + /// for `destroy_on_exit` must be reported as `skipped` with the honest + /// reason — not as a green `success` that claims a cleanup which did not + /// happen. + #[test] + fn teardown_status_reports_the_tier1_cleanup_stub_honestly() { + let (status, reason) = base_container_teardown_status(false, true, false); + assert_eq!(status, TeardownStatus::Skipped); + assert_eq!(reason, Some(TeardownSkipReason::CleanupNotImplemented)); + } + + /// `preserve_policy` must produce the same `skipped` / `preserve_policy` + /// answer as the AppContainer tiers — the two runners previously disagreed, + /// with BaseContainer reporting a bare `success` for the same request. + #[test] + fn teardown_status_matches_appcontainer_for_preserve_policy() { + let (status, reason) = base_container_teardown_status(false, true, true); + assert_eq!(status, TeardownStatus::Skipped); + assert_eq!(reason, Some(TeardownSkipReason::PreservePolicy)); + } + + /// A failed denial-capture finalisation is a real failure and outranks every + /// skip reason. + #[test] + fn teardown_status_reports_capture_failure_as_failure() { + for (destroy, preserve) in [(true, true), (true, false), (false, true), (false, false)] { + assert_eq!( + base_container_teardown_status(true, destroy, preserve), + (TeardownStatus::Failure, None) + ); + } + } + + #[test] + fn teardown_status_is_success_when_nothing_was_requested() { + assert_eq!( + base_container_teardown_status(false, false, false), + (TeardownStatus::Success, None) + ); + } + struct FakeCaptureSession { finish_error: Option<(&'static str, u32)>, finish_calls: Arc, diff --git a/src/backends/appcontainer/common/src/dispatcher.rs b/src/backends/appcontainer/common/src/dispatcher.rs index 24b9fda76..7fb782793 100644 --- a/src/backends/appcontainer/common/src/dispatcher.rs +++ b/src/backends/appcontainer/common/src/dispatcher.rs @@ -69,14 +69,89 @@ use std::path::PathBuf; use crate::appcontainer_runner::{derive_sid_string, AppContainerScriptRunner, FilesystemMode}; use crate::base_container_runner::BaseContainerRunner; -use crate::fallback_detector::{self, FallbackError, IsolationTier}; +use crate::fallback_detector::{self, DegradationReason, FallbackError, IsolationTier}; +use wxc_common::audit::{AuditEvent, AuditEventName, EffectiveEnforcementLevel}; use wxc_common::error::WxcError; use wxc_common::filesystem_dacl::{DaclError, DaclManager, RO_MASK, RW_MASK}; use wxc_common::logger::Logger; -use wxc_common::models::{ExecutionRequest, ScriptResponse}; +use wxc_common::models::{ContainmentBackend, ExecutionRequest, ScriptResponse}; use wxc_common::sandbox_process::{Runner, SandboxBackend, SandboxProcess, StdioMode}; use wxc_common::script_runner::ScriptRunner; +/// Bounded, machine-readable summary of how far the selected tier fell short of +/// the preferred one. Carried alongside the free-form `warnings` so audit +/// records never have to parse prose. +/// +/// This exists because the dispatcher outlives the [`TierDecision`] that +/// produced it (the decision's `bfscfg_path` is moved into the runner). The +/// *semantics* — what counts as degraded, and how the reason codes render — live +/// exactly once, in [`fallback_detector::is_degraded`] and +/// [`fallback_detector::reason_codes`], which this type delegates to. +#[derive(Debug, Clone, Default)] +pub struct Degradation { + /// Whether the chosen tier needs host-DACL augmentation to enforce the + /// policy. Previously computed by the detector and then dropped on the + /// floor by the dispatcher. + pub needs_dacl_augmentation: bool, + /// Bounded reason codes, produced at the detector branches themselves. + pub reasons: Vec, +} + +fn effective_enforcement_level( + tier: IsolationTier, + needs_dacl_augmentation: bool, +) -> EffectiveEnforcementLevel { + match (tier, needs_dacl_augmentation) { + (IsolationTier::BaseContainer, false) => EffectiveEnforcementLevel::BaseContainer, + (IsolationTier::BaseContainer, true) => { + EffectiveEnforcementLevel::BaseContainerDaclAugmented + } + (IsolationTier::AppContainerBfs, false) => EffectiveEnforcementLevel::AppContainerBfs, + (IsolationTier::AppContainerBfs, true) => { + EffectiveEnforcementLevel::AppContainerBfsDaclAugmented + } + (IsolationTier::AppContainerDacl, _) => EffectiveEnforcementLevel::AppContainerDacl, + } +} + +/// Emit `mxc.EnforcementDegraded` when the preferred tier was not selected. +/// +/// A clean Tier 1 run produces no record, so the stream stays signal-bearing. +/// Called from both dispatch surfaces (run-to-completion and streaming), +/// including the streaming spawn-failure arm — a tier was already chosen there, +/// so the degradation is real even though the spawn did not succeed. +pub(crate) fn log_enforcement_degraded( + logger: &mut Logger, + tier: IsolationTier, + degradation: &Degradation, +) { + if !fallback_detector::is_degraded( + tier, + degradation.needs_dacl_augmentation, + °radation.reasons, + ) { + return; + } + if !logger.has_diagnostic_sink() { + return; + } + let effective_level = effective_enforcement_level(tier, degradation.needs_dacl_augmentation); + let record = AuditEvent::new(AuditEventName::EnforcementDegraded) + .str("backend", ContainmentBackend::ProcessContainer.wire_name()) + .str("tier", tier.as_str()) + .str("effective_enforcement_level", effective_level.as_str()) + .bool( + "needs_dacl_augmentation", + degradation.needs_dacl_augmentation, + ) + .str( + "degradation_reasons", + &fallback_detector::reason_codes(°radation.reasons), + ) + .u64("degradation_reason_count", degradation.reasons.len() as u64); + logger.log_audit_event(&record); +} + /// Result of a successful dispatch decision: a phased handle holding a /// runner and (optionally) a `DaclManager`, with **private fields** so /// callers cannot reorder their drops. @@ -102,6 +177,10 @@ pub struct Dispatched { pub tier: IsolationTier, /// Operator-visible warnings collected during tier selection. pub warnings: Vec, + /// Bounded, machine-readable degradation facts for the + /// `mxc.EnforcementDegraded` audit record. Distinct from `warnings`, which + /// is free-form prose that can embed filesystem paths. + pub degradation: Degradation, } impl Dispatched { @@ -126,6 +205,17 @@ impl Dispatched { pub(crate) fn has_dacl_guard(&self) -> bool { self.dacl_manager.is_some() } + + /// Record `mxc.EnforcementDegraded` when the preferred tier was not + /// selected. A no-op for a clean Tier 1 dispatch. + /// + /// This lives on `Dispatched` rather than inside + /// [`dispatch_with_fallback`] because that function takes no `Logger`; the + /// caller (which has one) invokes it right where it already logs the + /// selected tier and the tier-selection warnings. + pub fn log_enforcement_degraded(&self, logger: &mut Logger) { + log_enforcement_degraded(logger, self.tier, &self.degradation); + } } /// Errors that can abort dispatch before the runner executes. @@ -318,6 +408,23 @@ impl SandboxBackend for SelectedBackend { } } +/// Everything [`select_backend_with_fallback`] resolves for a request: the +/// concrete backend, the (already-applied) DACL guard, the selected tier, the +/// operator-visible warnings, and the bounded degradation facts. +/// +/// A struct rather than a tuple so the two dispatch surfaces cannot bind the +/// fields in the wrong order, and so adding a field later is not a +/// call-site-wide edit. +struct BackendPlan { + backend: SelectedBackend, + /// When present, this manager has **already applied its ACEs** and MUST + /// outlive the run — its `Drop` restores the host ACEs. + dacl_manager: Option, + tier: IsolationTier, + warnings: Vec, + degradation: Degradation, +} + /// Run tier selection and construct the backend + (optional) DACL guard for /// `request`. This is the single source of truth for the tier → (backend, DACL) /// mapping, shared by the run-to-completion ([`dispatch_with_fallback`]) and @@ -327,17 +434,7 @@ impl SandboxBackend for SelectedBackend { /// its ACEs** and MUST outlive the run (its `Drop` restores the host ACEs). The /// selected [`IsolationTier`] and any tier-selection warnings are returned for /// telemetry. -fn select_backend_with_fallback( - request: &ExecutionRequest, -) -> Result< - ( - SelectedBackend, - Option, - IsolationTier, - Vec, - ), - DispatchError, -> { +fn select_backend_with_fallback(request: &ExecutionRequest) -> Result { let decision = fallback_detector::detect(&request.policy, /*prefer_bc=*/ true)?; if request.policy.capture_denials.is_some() && decision.tier != IsolationTier::BaseContainer { return Err(DispatchError::CaptureDenialsUnsupported { @@ -425,7 +522,16 @@ fn select_backend_with_fallback( } }; - Ok((backend, dacl_manager, decision.tier, decision.warnings)) + Ok(BackendPlan { + backend, + dacl_manager, + tier: decision.tier, + warnings: decision.warnings, + degradation: Degradation { + needs_dacl_augmentation: decision.needs_dacl_augmentation, + reasons: decision.reasons, + }, + }) } /// Build a runner with appropriate DACL augmentation for the @@ -437,13 +543,14 @@ fn select_backend_with_fallback( /// applied its ACEs. Use [`Dispatched::into_runner_and_guard`] to /// extract both; the manager MUST stay alive through the run. pub fn dispatch_with_fallback(request: &ExecutionRequest) -> Result { - let (backend, dacl_manager, tier, warnings) = select_backend_with_fallback(request)?; - let runner: Box = Box::new(Runner::new(backend)); + let plan = select_backend_with_fallback(request)?; + let runner: Box = Box::new(Runner::new(plan.backend)); Ok(Dispatched { runner, - dacl_manager, - tier, - warnings, + dacl_manager: plan.dacl_manager, + tier: plan.tier, + warnings: plan.warnings, + degradation: plan.degradation, }) } @@ -459,7 +566,6 @@ pub struct DispatchedProcess { /// Operator-visible warnings collected during tier selection. pub warnings: Vec, } - /// Error from the streaming [`spawn_with_fallback`] path. Kept distinct from a /// flat error so the caller can preserve fallback semantics: tier-selection / /// DACL failures map to `backend_unavailable` (as the run-to-completion path @@ -508,8 +614,17 @@ pub fn spawn_with_fallback( logger: &mut Logger, stdio: StdioMode, ) -> Result { - let (mut backend, dacl_manager, tier, warnings) = - select_backend_with_fallback(request).map_err(SpawnDispatchError::Dispatch)?; + let BackendPlan { + mut backend, + dacl_manager, + tier, + warnings, + degradation, + } = select_backend_with_fallback(request).map_err(SpawnDispatchError::Dispatch)?; + + // A tier has been chosen: record any degradation now, so the record exists + // whether or not the spawn below succeeds. + log_enforcement_degraded(logger, tier, °radation); // Spawn with the DACL ACEs (if any) already applied. On a spawn failure the // `dacl_manager` local drops here, restoring any ACEs that were stamped; we @@ -625,6 +740,49 @@ mod tests { fn empty_policy() -> ContainerPolicy { ContainerPolicy::default() } + + #[test] + fn effective_enforcement_level_covers_all_tier_combinations() { + let cases = [ + ( + IsolationTier::BaseContainer, + false, + EffectiveEnforcementLevel::BaseContainer, + ), + ( + IsolationTier::BaseContainer, + true, + EffectiveEnforcementLevel::BaseContainerDaclAugmented, + ), + ( + IsolationTier::AppContainerBfs, + false, + EffectiveEnforcementLevel::AppContainerBfs, + ), + ( + IsolationTier::AppContainerBfs, + true, + EffectiveEnforcementLevel::AppContainerBfsDaclAugmented, + ), + ( + IsolationTier::AppContainerDacl, + false, + EffectiveEnforcementLevel::AppContainerDacl, + ), + ( + IsolationTier::AppContainerDacl, + true, + EffectiveEnforcementLevel::AppContainerDacl, + ), + ]; + + for (tier, needs_dacl_augmentation, expected) in cases { + assert_eq!( + effective_enforcement_level(tier, needs_dacl_augmentation), + expected + ); + } + } fn policy_with_denied_temp() -> (ContainerPolicy, tempfile::TempDir) { let dir = tempfile::tempdir().expect("temp dir"); let mut p = ContainerPolicy::default(); @@ -879,8 +1037,8 @@ mod tests { fn select_backend_t1_builds_base_container_no_dacl() { let _g = ForceTierGuard::set("base-container"); let req = test_request(empty_policy()); - let (backend, dacl, tier, _w) = - select_backend_with_fallback(&req).expect("T1 selection should succeed"); + let plan = select_backend_with_fallback(&req).expect("T1 selection should succeed"); + let (backend, dacl, tier) = (plan.backend, plan.dacl_manager, plan.tier); assert!(matches!(tier, IsolationTier::BaseContainer)); assert!( matches!(backend, SelectedBackend::BaseContainer(_)), @@ -896,8 +1054,8 @@ mod tests { fn select_backend_t2_no_deny_builds_appcontainer_no_dacl() { let _g = ForceTierGuard::set("appcontainer-bfs"); let req = test_request(empty_policy()); - let (backend, dacl, tier, _w) = - select_backend_with_fallback(&req).expect("T2 selection should succeed"); + let plan = select_backend_with_fallback(&req).expect("T2 selection should succeed"); + let (backend, dacl, tier) = (plan.backend, plan.dacl_manager, plan.tier); assert!(matches!(tier, IsolationTier::AppContainerBfs)); assert!(matches!(backend, SelectedBackend::AppContainer(_))); assert!( @@ -911,8 +1069,8 @@ mod tests { let _g = ForceTierGuard::set("appcontainer-bfs"); let (policy, _tmp) = policy_with_denied_temp(); let req = test_request(policy); - let (backend, dacl, tier, _w) = - select_backend_with_fallback(&req).expect("T2+deny selection should succeed"); + let plan = select_backend_with_fallback(&req).expect("T2+deny selection should succeed"); + let (backend, dacl, tier) = (plan.backend, plan.dacl_manager, plan.tier); assert!(matches!(tier, IsolationTier::AppContainerBfs)); assert!(matches!(backend, SelectedBackend::AppContainer(_))); assert!( @@ -926,8 +1084,8 @@ mod tests { let _g = ForceTierGuard::set("appcontainer-dacl"); let (policy, _tmp) = policy_with_rw_temp(); let req = test_request(policy); - let (backend, dacl, tier, _w) = - select_backend_with_fallback(&req).expect("T3 selection should succeed"); + let plan = select_backend_with_fallback(&req).expect("T3 selection should succeed"); + let (backend, dacl, tier) = (plan.backend, plan.dacl_manager, plan.tier); assert!(matches!(tier, IsolationTier::AppContainerDacl)); assert!(matches!(backend, SelectedBackend::AppContainer(_))); assert!( @@ -946,8 +1104,8 @@ mod tests { // with the "bfscfg.exe is not available" error. let _g = BcUsableGuard::set(false); let req = test_request(empty_policy()); - let (backend, dacl, tier, _w) = - select_backend_with_fallback(&req).expect("selection should succeed"); + let plan = select_backend_with_fallback(&req).expect("selection should succeed"); + let (backend, dacl, tier) = (plan.backend, plan.dacl_manager, plan.tier); assert!(matches!(tier, IsolationTier::AppContainerDacl)); assert!(matches!(backend, SelectedBackend::AppContainer(_))); assert!(dacl.is_some()); diff --git a/src/backends/appcontainer/common/src/fallback_detector.rs b/src/backends/appcontainer/common/src/fallback_detector.rs index ffc6eb24b..cbf9228f1 100644 --- a/src/backends/appcontainer/common/src/fallback_detector.rs +++ b/src/backends/appcontainer/common/src/fallback_detector.rs @@ -43,6 +43,62 @@ impl IsolationTier { } } +/// Bounded reason a higher isolation tier was rejected. +/// +/// Each variant is produced at exactly the branch that causes it — the enum is +/// **never** derived by pattern-matching [`TierDecision::warnings`], whose +/// human-readable strings can embed filesystem paths and are not a stable +/// vocabulary. +/// +/// Used by the `mxc.EnforcementDegraded` audit record +/// ([`wxc_common::audit::AuditEventName::EnforcementDegraded`]). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DegradationReason { + /// BaseContainer was preferred and usable, but the OS does not advertise + /// `SANDBOX_CAP_FS_DENY`, so a `deniedPaths` policy cannot be enforced + /// natively at Tier 1. + BaseContainerDenyUnsupported, + /// BaseContainer was not selected — either it was not preferred, or the + /// backend is not usable on this host. + BaseContainerUnavailable, + /// AppContainer + BFS is not compiled into this binary (the `tier2_bfs` + /// Cargo feature is off), so Tier 2 was skipped entirely. + Tier2FeatureDisabled, + /// `bfscfg.exe` could not be resolved, so BFS could not enforce the + /// filesystem policy. + BfscfgUnavailable, + /// The selected tier has to mutate host DACLs to enforce the policy. + DaclAugmentationRequired, + /// The system-drive metadata ACEs `wxc-host-prep prepare-system-drive` + /// stamps are not in effect on this machine. + HostPrepSystemDriveMissing, + /// The `\Device\Null` security descriptor `wxc-host-prep + /// prepare-null-device` applies is not in effect (the kernel resets it at + /// every boot). + HostPrepNullDeviceMissing, +} + +impl DegradationReason { + /// Stable snake_case code for the audit record. + pub fn as_str(self) -> &'static str { + match self { + Self::BaseContainerDenyUnsupported => "base_container_deny_unsupported", + Self::BaseContainerUnavailable => "base_container_unavailable", + Self::Tier2FeatureDisabled => "tier2_feature_disabled", + Self::BfscfgUnavailable => "bfscfg_unavailable", + Self::DaclAugmentationRequired => "dacl_augmentation_required", + Self::HostPrepSystemDriveMissing => "host_prep_system_drive_missing", + Self::HostPrepNullDeviceMissing => "host_prep_null_device_missing", + } + } +} + +impl std::fmt::Display for DegradationReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + /// Outcome of [`detect`]: the chosen tier plus any operator-visible warnings /// gathered while walking the decision algorithm. #[derive(Debug, Clone)] @@ -65,7 +121,55 @@ pub struct TierDecision { pub bfscfg_path: Option, /// Human-readable degradation messages explaining why a higher tier was /// rejected. Empty when the preferred tier was selected. + /// + /// **Not for structured consumption.** These strings can embed filesystem + /// paths and are not a stable vocabulary; use [`TierDecision::reasons`] for + /// anything machine-readable. pub warnings: Vec, + /// Bounded, machine-readable counterpart to [`TierDecision::warnings`], + /// populated at the same branches. Empty when the preferred tier was + /// selected. + pub reasons: Vec, +} + +impl TierDecision { + /// Whether enforcement was degraded relative to the preferred tier — the + /// condition under which `mxc.EnforcementDegraded` fires. A clean Tier 1 + /// selection returns `false` and produces no record. + pub fn is_degraded(&self) -> bool { + is_degraded(self.tier, self.needs_dacl_augmentation, &self.reasons) + } + + /// The bounded reason codes joined into the single comma-separated string + /// the audit record carries (arrays are not part of the record format). + pub fn reason_codes(&self) -> String { + reason_codes(&self.reasons) + } +} + +/// Whether the selected tier represents degraded enforcement. +/// +/// A free function rather than only a `TierDecision` method because the +/// dispatcher keeps the degradation facts after the `TierDecision` itself has +/// been consumed (its `bfscfg_path` is moved into the runner). Having exactly +/// one definition is what stops the two layers from drifting apart on what +/// "degraded" means. +pub fn is_degraded( + tier: IsolationTier, + needs_dacl_augmentation: bool, + reasons: &[DegradationReason], +) -> bool { + tier != IsolationTier::BaseContainer || needs_dacl_augmentation || !reasons.is_empty() +} + +/// Join bounded reason codes into the comma-separated string the audit record +/// carries. Single definition, shared with the dispatcher — see [`is_degraded`]. +pub fn reason_codes(reasons: &[DegradationReason]) -> String { + reasons + .iter() + .map(|r| r.as_str()) + .collect::>() + .join(",") } /// Errors that abort tier selection. @@ -161,6 +265,7 @@ pub fn detect( } let mut warnings: Vec = Vec::new(); + let mut reasons: Vec = Vec::new(); // Tier 1 — BaseContainer if prefer_base_container && is_base_container_usable() { @@ -173,6 +278,7 @@ pub fn detect( needs_dacl_augmentation: false, bfscfg_path: None, warnings, + reasons, }); } warnings.push( @@ -181,6 +287,13 @@ pub fn detect( for deniedPaths enforcement" .to_string(), ); + reasons.push(DegradationReason::BaseContainerDenyUnsupported); + } else { + // Tier 1 was not selected at all: either the caller did not prefer it, + // or the backend is not usable on this host. Recorded as a bounded + // reason here rather than inferred later, so the audit record can + // distinguish "T1 rejected the policy" from "T1 was never available". + reasons.push(DegradationReason::BaseContainerUnavailable); } // Tier 2 — AppContainer + BFS // @@ -210,25 +323,30 @@ pub fn detect( if denied { ensure_dacl_augmentation_allowed(policy)?; verify_write_dac_all(&policy.denied_paths)?; + reasons.push(DegradationReason::DaclAugmentationRequired); } return Ok(TierDecision { tier: IsolationTier::AppContainerBfs, needs_dacl_augmentation: denied, bfscfg_path, warnings, + reasons, }); } warnings.push("bfscfg.exe not present; falling back to AppContainer + DACL".to_string()); + reasons.push(DegradationReason::BfscfgUnavailable); } else { warnings.push( "BaseContainer tier not selected, and AppContainer + BFS is not \ compiled into this binary; falling back to AppContainer + DACL" .to_string(), ); + reasons.push(DegradationReason::Tier2FeatureDisabled); } // Tier 3 — AppContainer + DACL ensure_dacl_augmentation_allowed(policy)?; + reasons.push(DegradationReason::DaclAugmentationRequired); // For RW / RO paths we only need `WRITE_DAC` if we'd actually have // to add an ACE. When the path's existing DACL already grants the // needed mask to the well-known AppContainer SIDs (typically @@ -250,24 +368,25 @@ pub fn detect( // boot (the `\Device\Null` descriptor). Surface actionable // `wxc-host-prep` recommendations, but only for the preparations // that are not already in effect on this machine. - push_host_prep_warnings(&mut warnings); + push_host_prep_warnings(&mut warnings, &mut reasons); Ok(TierDecision { tier: IsolationTier::AppContainerDacl, needs_dacl_augmentation: true, bfscfg_path: None, warnings, + reasons, }) } -/// Append `wxc-host-prep` recommendations to `warnings` for any -/// host-side preparation the AppContainer + DACL tier relies on that is -/// not currently in effect on this machine. +/// Append `wxc-host-prep` recommendations to `warnings` (and their bounded +/// counterparts to `reasons`) for any host-side preparation the AppContainer + +/// DACL tier relies on that is not currently in effect on this machine. /// /// Each check is read-only and best-effort: if the machine state cannot /// be determined we err on the side of surfacing the recommendation /// rather than silently swallowing it. -fn push_host_prep_warnings(warnings: &mut Vec) { +fn push_host_prep_warnings(warnings: &mut Vec, reasons: &mut Vec) { if !system_drive_prepared() { warnings.push( "AppContainer + DACL tier selected: AppContainer processes may be unable to read \ @@ -276,6 +395,7 @@ fn push_host_prep_warnings(warnings: &mut Vec) { minimal metadata ACEs." .to_string(), ); + reasons.push(DegradationReason::HostPrepSystemDriveMissing); } if !null_device_prepared() { warnings.push( @@ -285,6 +405,7 @@ fn push_host_prep_warnings(warnings: &mut Vec) { the documented security descriptor." .to_string(), ); + reasons.push(DegradationReason::HostPrepNullDeviceMissing); } } @@ -447,6 +568,7 @@ fn forced_decision( needs_dacl_augmentation: needs_dacl, bfscfg_path: None, warnings: Vec::new(), + reasons: Vec::new(), }) } @@ -692,6 +814,68 @@ mod tests { assert!(matches!(d.tier, IsolationTier::BaseContainer)); assert!(!d.needs_dacl_augmentation); assert!(d.warnings.is_empty()); + assert!(d.reasons.is_empty()); + assert!( + !d.is_degraded(), + "a clean Tier 1 selection must not report degradation" + ); + } + + #[test] + fn degradation_reason_codes_are_bounded_snake_case_and_unique() { + let all = [ + DegradationReason::BaseContainerDenyUnsupported, + DegradationReason::BaseContainerUnavailable, + DegradationReason::Tier2FeatureDisabled, + DegradationReason::BfscfgUnavailable, + DegradationReason::DaclAugmentationRequired, + DegradationReason::HostPrepSystemDriveMissing, + DegradationReason::HostPrepNullDeviceMissing, + ]; + let mut seen = std::collections::HashSet::new(); + for r in all { + let code = r.as_str(); + assert!( + code.chars() + .all(|c| c.is_ascii_lowercase() || c == '_' || c.is_ascii_digit()), + "not snake_case: {code}" + ); + assert!(seen.insert(code), "duplicate reason code: {code}"); + } + } + + #[test] + fn reason_codes_join_without_whitespace_so_the_field_stays_parseable() { + let decision = TierDecision { + tier: IsolationTier::AppContainerDacl, + needs_dacl_augmentation: true, + bfscfg_path: None, + warnings: Vec::new(), + reasons: vec![ + DegradationReason::BaseContainerUnavailable, + DegradationReason::Tier2FeatureDisabled, + ], + }; + assert_eq!( + decision.reason_codes(), + "base_container_unavailable,tier2_feature_disabled" + ); + assert!(decision.is_degraded()); + } + + #[test] + fn a_non_base_container_tier_is_always_degraded_even_without_reasons() { + // A forced decision carries no reasons, but landing below Tier 1 is + // itself the degradation the record exists to report. + let decision = TierDecision { + tier: IsolationTier::AppContainerBfs, + needs_dacl_augmentation: false, + bfscfg_path: None, + warnings: Vec::new(), + reasons: Vec::new(), + }; + assert!(decision.is_degraded()); + assert_eq!(decision.reason_codes(), ""); } #[test] fn empty_policy_no_filesystem_t2_path() { @@ -1022,15 +1206,23 @@ mod tests { /// The host-prep state is machine-dependent, so we assert only the /// contract: at most the two known recommendations, and each names - /// the corresponding `wxc-host-prep` verb. + /// the corresponding `wxc-host-prep` verb. The bounded `reasons` must stay + /// 1:1 with the prose warnings so the audit record can never disagree with + /// what the operator was told. #[test] fn push_host_prep_warnings_are_actionable_and_bounded() { let mut warnings = Vec::new(); - push_host_prep_warnings(&mut warnings); + let mut reasons = Vec::new(); + push_host_prep_warnings(&mut warnings, &mut reasons); assert!( warnings.len() <= 2, "expected at most two host-prep warnings, got {warnings:?}" ); + assert_eq!( + warnings.len(), + reasons.len(), + "each host-prep warning needs exactly one bounded reason code" + ); for w in &warnings { assert!( w.contains("wxc-host-prep prepare-system-drive") @@ -1038,6 +1230,16 @@ mod tests { "host-prep warning should name a wxc-host-prep verb, got: {w}" ); } + for r in &reasons { + assert!( + matches!( + r, + DegradationReason::HostPrepSystemDriveMissing + | DegradationReason::HostPrepNullDeviceMissing + ), + "unexpected host-prep reason: {r}" + ); + } } /// Read-only `\Device\Null` probe must not panic; the result is diff --git a/src/backends/appcontainer/common/src/job_object.rs b/src/backends/appcontainer/common/src/job_object.rs index 2080d7ade..5272b7e7f 100644 --- a/src/backends/appcontainer/common/src/job_object.rs +++ b/src/backends/appcontainer/common/src/job_object.rs @@ -277,13 +277,17 @@ impl UiJobObject { /// Terminate every process currently assigned to this job (the sandboxed /// child and all of its descendants) with the given exit code. Used to - /// tree-kill a running sandbox. Best-effort: errors are ignored since the - /// processes may already have exited. - pub fn terminate(&self, exit_code: u32) { + /// tree-kill a running sandbox. + /// + /// Best-effort: the returned error is **diagnostic only** and callers must + /// keep treating a failure as non-fatal, because the processes may simply + /// have exited already (`TerminateJobObject` reports `ERROR_ACCESS_DENIED` + /// for an already-terminated job). The result is returned rather than + /// discarded so a caller can record *that* the kill failed instead of + /// silently reporting a clean teardown. + pub fn terminate(&self, exit_code: u32) -> windows::core::Result<()> { // SAFETY: `self.handle` is a valid job handle owned by this struct. - unsafe { - let _ = TerminateJobObject(self.handle, exit_code); - } + unsafe { TerminateJobObject(self.handle, exit_code) } } } @@ -318,6 +322,23 @@ mod tests { drop(job); } + /// `terminate` must surface its `Result` so a caller can *record* a failed + /// kill, while remaining safe to call on an empty job (the common case: + /// every process already exited). + /// + /// The `let _ =` at the call sites is deliberate — the audit records + /// consume the error, and control flow must not change — so this test locks + /// in that the value is available at all. + #[test] + fn terminate_returns_a_result_and_succeeds_on_an_empty_job() { + let job = UiJobObject::new().expect("create"); + let result: windows::core::Result<()> = job.terminate(u32::MAX); + assert!( + result.is_ok(), + "terminating an empty job should succeed: {result:?}" + ); + } + #[test] fn encoder_known_bit_positions() { // Sanity-check that the encoder produces the documented winnt.h diff --git a/src/backends/appcontainer/common/src/network_manager.rs b/src/backends/appcontainer/common/src/network_manager.rs index 9abb2c342..901dd7d82 100644 --- a/src/backends/appcontainer/common/src/network_manager.rs +++ b/src/backends/appcontainer/common/src/network_manager.rs @@ -89,6 +89,38 @@ pub struct NetworkManager { proxy_coordinator: ProxyCoordinator, } +/// What [`NetworkManager::stop_all`] actually released. +/// +/// Previously the firewall-removal `Result` was discarded at the call site, so +/// a partially-failed cleanup was indistinguishable from a clean one. These are +/// diagnostic values only — teardown remains best-effort and non-fatal. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct NetworkTeardown { + /// How many firewall rules this manager had created and attempted to + /// remove. Zero when no rules were installed or cleanup was skipped. + pub rules_removed: usize, + /// Whether every rule removal succeeded. `true` when there was nothing to + /// remove. + pub firewall_removal_ok: bool, + /// Whether an active proxy coordinator was stopped. + pub proxy_stopped: bool, +} + +impl Default for NetworkTeardown { + /// "Nothing to remove, and that is fine." + /// + /// Written by hand because the derived default would set + /// `firewall_removal_ok: false`, which reads as a *failed* removal — the + /// opposite of what an empty teardown means. + fn default() -> Self { + Self { + rules_removed: 0, + firewall_removal_ok: true, + proxy_stopped: false, + } + } +} + /// Invariant context for creating firewall rules within a single /// `apply_firewall_rules` call: the firewall interface (valid only for the /// current COM apartment / thread) and the AppContainer principal the rules are @@ -98,6 +130,23 @@ struct RuleContext<'a> { principal_id: &'a str, } +/// The outcome of evaluating a request's network policy: the effective default +/// policy, whether the caller asked for firewall-rule enforcement, and whether +/// any rules will actually be installed. +/// +/// The last two are **not** the same, and conflating them is a real bug: with +/// `enforcementMode: firewall`, no host lists, and `defaultPolicy: allow`, the +/// caller asked for firewall enforcement but there is nothing to install. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct NetworkPolicyPlan { + /// Effective default policy for the rules that get installed. + pub default_policy: DefaultPolicy, + /// Whether `enforcementMode` selects firewall-rule enforcement at all. + pub firewall_mode_selected: bool, + /// Whether this policy actually produces firewall rules to install. + pub rules_will_be_installed: bool, +} + impl NetworkManager { pub fn new() -> Self { Self { @@ -107,30 +156,62 @@ impl NetworkManager { } } - pub fn initialize_policy( - policy: &ContainerPolicy, - logger: &mut Logger, - ) -> (DefaultPolicy, bool) { - let use_firewall_rules = matches!( + /// Decide the effective default policy and whether firewall rules will be + /// installed for `policy` — the pure core of [`Self::initialize_policy`], + /// factored out so callers that only need the *decision* (e.g. audit + /// records) do not have to log an "Applying network firewall rules" line as + /// a side effect of asking. + pub fn describe_policy(policy: &ContainerPolicy) -> NetworkPolicyPlan { + let firewall_mode_selected = matches!( policy.network_enforcement_mode, NetworkEnforcementMode::Firewall | NetworkEnforcementMode::Both ); - if use_firewall_rules + if firewall_mode_selected && (!policy.allowed_hosts.is_empty() || !policy.blocked_hosts.is_empty() || policy.default_network_policy == NetworkPolicy::Block) { - logger.log_line("Applying network firewall rules..."); let default_policy = if policy.default_network_policy == NetworkPolicy::Block { DefaultPolicy::Block } else { DefaultPolicy::Allow }; - return (default_policy, true); + return NetworkPolicyPlan { + default_policy, + firewall_mode_selected, + rules_will_be_installed: true, + }; + } + + NetworkPolicyPlan { + default_policy: DefaultPolicy::Allow, + firewall_mode_selected, + rules_will_be_installed: false, + } + } + + pub fn initialize_policy( + policy: &ContainerPolicy, + logger: &mut Logger, + ) -> (DefaultPolicy, bool) { + let plan = Self::describe_policy(policy); + // The operator-visible line fires only when rules will actually be + // created — not merely because `enforcementMode` asked for firewall + // enforcement. + if plan.rules_will_be_installed { + logger.log_line("Applying network firewall rules..."); } + (plan.default_policy, plan.firewall_mode_selected) + } - (DefaultPolicy::Allow, use_firewall_rules) + /// Number of firewall rules currently created by this manager. + /// + /// The *count* is deliberately what the audit record carries: rule names + /// embed the AppContainer principal id and a timestamp, which is + /// high-cardinality and semi-identifying. + pub fn rule_count(&self) -> usize { + self.created_rule_names.len() } pub fn apply_firewall_rules( @@ -284,13 +365,36 @@ impl NetworkManager { } /// Stop all network resources: firewall rules, proxy policy, test proxy. - pub fn stop_all(&mut self, cleanup_policy: bool, logger: &mut Logger) { + /// + /// Returns what was actually released, so a caller can record an honest + /// teardown record instead of assuming success. Failures remain non-fatal — + /// this is a best-effort cleanup path and the return value is diagnostic. + pub fn stop_all(&mut self, cleanup_policy: bool, logger: &mut Logger) -> NetworkTeardown { + let rules_at_entry = self.created_rule_names.len(); + let mut outcome = NetworkTeardown { + rules_removed: 0, + firewall_removal_ok: true, + proxy_stopped: false, + }; + if self.rules_applied() && cleanup_policy { - let _ = self.remove_firewall_rules(logger); + match self.remove_firewall_rules(logger) { + Ok(all_success) => { + outcome.rules_removed = rules_at_entry; + outcome.firewall_removal_ok = all_success; + } + Err(_) => { + // `remove_firewall_rules` failed before it could clear the + // list, so nothing was removed. + outcome.firewall_removal_ok = false; + } + } } if self.proxy_coordinator.is_active() { self.proxy_coordinator.stop(logger); + outcome.proxy_stopped = true; } + outcome } pub fn remove_firewall_rules(&mut self, logger: &mut Logger) -> Result { @@ -511,6 +615,105 @@ mod tests { assert_eq!(default_policy, DefaultPolicy::Block); } + /// `describe_policy` is the pure core of `initialize_policy`; the audit + /// record calls it instead so asking the question does not also emit an + /// "Applying network firewall rules..." line as a side effect. + #[test] + fn describe_policy_matches_initialize_policy_but_is_side_effect_free() { + let cases = [ + ContainerPolicy { + network_enforcement_mode: NetworkEnforcementMode::Firewall, + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }, + ContainerPolicy { + network_enforcement_mode: NetworkEnforcementMode::Capabilities, + default_network_policy: NetworkPolicy::Block, + ..Default::default() + }, + ContainerPolicy { + network_enforcement_mode: NetworkEnforcementMode::Both, + default_network_policy: NetworkPolicy::Allow, + ..Default::default() + }, + ContainerPolicy { + network_enforcement_mode: NetworkEnforcementMode::Firewall, + default_network_policy: NetworkPolicy::Allow, + ..Default::default() + }, + ]; + for policy in cases { + let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); + let (default_policy, use_fw) = NetworkManager::initialize_policy(&policy, &mut logger); + let plan = NetworkManager::describe_policy(&policy); + assert_eq!(default_policy, plan.default_policy, "policy: {policy:?}"); + assert_eq!(use_fw, plan.firewall_mode_selected, "policy: {policy:?}"); + // The operator-visible line must track "rules will actually be + // installed", not "firewall mode was selected". + assert_eq!( + logger + .get_buffer() + .contains("Applying network firewall rules"), + plan.rules_will_be_installed, + "log line must track rule installation; policy: {policy:?}" + ); + } + } + + /// Regression guard for the split between "the caller asked for firewall + /// enforcement" and "rules will actually be installed". With + /// `enforcementMode: firewall`, no host lists, and `defaultPolicy: allow` + /// there is nothing to install, so `initialize_policy` must stay silent — + /// exactly as it did before `describe_policy` was extracted. + #[test] + fn firewall_mode_without_rules_installs_nothing_and_logs_nothing() { + let policy = ContainerPolicy { + network_enforcement_mode: NetworkEnforcementMode::Firewall, + default_network_policy: NetworkPolicy::Allow, + ..Default::default() + }; + let plan = NetworkManager::describe_policy(&policy); + assert!(plan.firewall_mode_selected); + assert!(!plan.rules_will_be_installed); + + let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); + let (_, use_fw) = NetworkManager::initialize_policy(&policy, &mut logger); + assert!(use_fw, "the caller did select firewall enforcement"); + assert!( + !logger + .get_buffer() + .contains("Applying network firewall rules"), + "must not claim rules are being applied; buffer: {}", + logger.get_buffer() + ); + } + + /// `stop_all` on a manager that installed nothing must report a clean, + /// empty teardown — not a failure. The derived `Default` for + /// `NetworkTeardown` would get this backwards, which is why it is + /// hand-written. + #[test] + fn stop_all_with_nothing_installed_reports_a_clean_teardown() { + let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); + let mut manager = NetworkManager::new(); + let outcome = manager.stop_all(true, &mut logger); + assert_eq!(outcome, NetworkTeardown::default()); + assert_eq!(outcome.rules_removed, 0); + assert!(outcome.firewall_removal_ok); + assert!(!outcome.proxy_stopped); + } + + /// Skipping cleanup (`preservePolicy`) must not be reported as a failed + /// removal. + #[test] + fn stop_all_without_cleanup_reports_no_removal_failure() { + let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); + let mut manager = NetworkManager::new(); + let outcome = manager.stop_all(false, &mut logger); + assert!(outcome.firewall_removal_ok); + assert_eq!(outcome.rules_removed, 0); + } + #[test] fn test_initialize_policy_capabilities_mode() { let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); diff --git a/src/backends/isolation_session/common/src/manager.rs b/src/backends/isolation_session/common/src/manager.rs index 586aacd27..93042614b 100644 --- a/src/backends/isolation_session/common/src/manager.rs +++ b/src/backends/isolation_session/common/src/manager.rs @@ -6,6 +6,8 @@ //! plus the `share_folders` non-lifecycle op. `create_process` also drives //! the ConPTY relay setup + shutdown ladder against the local console. +use wxc_common::audit::{AuditEvent, AuditEventName, KillMethod}; +use wxc_common::logger::Logger; use wxc_common::models::IsolationSessionConfigurationId; use wxc_common::process_util::OwnedHandle; @@ -285,6 +287,7 @@ impl IsolationSessionManager { pub(super) fn create_process( &self, options: &ProcessOptions, + logger: Option<&mut Logger>, ) -> Result { let proc_options = build_iso_process_options(options)?; @@ -506,7 +509,9 @@ impl IsolationSessionManager { .WaitForExit(options.timeout_ms) .map_err(|e| lifecycle_err(format!("WaitForExit failed: {}", e)))?; - let exit_code = wait_with_graceful_shutdown(&process)?; + let identity = self.provision_id.to_string_lossy(); + let exit_code = + wait_with_graceful_shutdown(&process, options.timeout_ms, &identity, logger)?; // Signal the stdin relay to exit. Effective for waitable (console) // handles; for pipe handles the bounded wait below expires and we @@ -589,7 +594,7 @@ impl IsolationSessionManager { /// running after `WaitForExit(timeout_ms)` returns. Tier 1: close stdin — /// many REPLs exit on EOF alone. Tier 2: `SendCtrlClose` — ConPTY-only; /// `E_NOTIMPL` outside ConPTY, benign. Tier 3: force-terminate, wait -/// infinitely (`WaitForExit(0)` = INFINITE) for the kill to land. +/// for up to five seconds for the kill to land. /// /// The first `ExitCode()` query is `?`-propagated: a failure there means /// the kernel handle is broken, and the cleanup methods on the same @@ -597,7 +602,12 @@ impl IsolationSessionManager { /// than to fire blind. Per-tier subsequent queries fall back to /// `STILL_ACTIVE` so a transient read failure does not short-circuit the /// escalation. -fn wait_with_graceful_shutdown(process: &IsoSessionProcess) -> Result { +fn wait_with_graceful_shutdown( + process: &IsoSessionProcess, + timeout_ms: u32, + identity: &str, + mut logger: Option<&mut Logger>, +) -> Result { // `STILL_ACTIVE` (0x103) is exposed by the `windows` crate as // `STATUS_PENDING: NTSTATUS` — same numeric value, different name. use windows::Win32::Foundation::STATUS_PENDING; @@ -609,6 +619,18 @@ fn wait_with_graceful_shutdown(process: &IsoSessionProcess) -> Result Result code, Err(e) => { let _ = manager.stop_session(); diff --git a/src/backends/isolation_session/common/src/state_aware.rs b/src/backends/isolation_session/common/src/state_aware.rs index fa5e50b73..dced8c923 100644 --- a/src/backends/isolation_session/common/src/state_aware.rs +++ b/src/backends/isolation_session/common/src/state_aware.rs @@ -11,6 +11,7 @@ use std::io::IsTerminal; use serde::Serialize; use wxc_common::id::mint_random_token; +use wxc_common::logger::{Logger, Mode}; use wxc_common::models::{ ExecutionRequest, IsolationSessionConfig, IsolationSessionProvisionConfig, }; @@ -271,9 +272,14 @@ impl StatefulSandboxBackend for IsolationSessionRunner { let interactive = std::io::stdout().is_terminal(); let options = build_process_options(request, interactive); + let mut logger = Logger::new(Mode::Buffer); + let diagnostic_config = wxc_common::diagnostic::DiagnosticConfig::from_environment(); + if diagnostic_config.console_enabled { + logger.enable_diagnostics(&diagnostic_config); + } let exit_code = manager - .create_process(&options) + .create_process(&options, Some(&mut logger)) .map_err(map_lifecycle_error)?; // The output relay completed inside `create_process`. The dispatcher diff --git a/src/core/mxc_engine/src/dispatch.rs b/src/core/mxc_engine/src/dispatch.rs index 76f7ae5e1..a47ad5380 100644 --- a/src/core/mxc_engine/src/dispatch.rs +++ b/src/core/mxc_engine/src/dispatch.rs @@ -64,6 +64,10 @@ pub fn spawn_runner( "dry_run is not supported for streaming spawns", )); } + // Anchor the run to its policy identity before any backend is engaged, so + // the streaming surface produces the same `mxc.PolicyHash` record as the + // run-to-completion one. + crate::run::log_policy_hash(request, logger); match &request.containment { ContainmentBackend::Seatbelt => spawn_seatbelt(request, logger), ContainmentBackend::Bubblewrap => spawn_bubblewrap(request, logger), diff --git a/src/core/mxc_engine/src/lib.rs b/src/core/mxc_engine/src/lib.rs index 9c34039ac..12f9159e1 100644 --- a/src/core/mxc_engine/src/lib.rs +++ b/src/core/mxc_engine/src/lib.rs @@ -41,7 +41,7 @@ pub use policy::{ FilesystemPolicyResult, SandboxPolicy, SandboxRequest, }; #[cfg(any(target_os = "windows", target_os = "linux", target_os = "macos"))] -pub use run::{resolve_runner, run, ResolvedRunner}; +pub use run::{log_policy_hash, resolve_runner, run, ResolvedRunner}; pub use state_aware::{exec_state_aware_json, run_state_aware, run_state_aware_json}; use wxc_common::logger::{Logger, Mode}; diff --git a/src/core/mxc_engine/src/run.rs b/src/core/mxc_engine/src/run.rs index 5cf6b6abd..f0f57cb7e 100644 --- a/src/core/mxc_engine/src/run.rs +++ b/src/core/mxc_engine/src/run.rs @@ -85,9 +85,42 @@ pub fn resolve_runner( request: &ExecutionRequest, logger: &mut Logger, ) -> Result { + log_policy_hash(request, logger); resolve_runner_inner(request, logger).map_err(Error::from) } +/// Record `mxc.PolicyHash`: the canonical identity of the effective policy this +/// run is about to be launched under, plus the config *schema* version. +/// +/// Emitted from [`resolve_runner`] — the single funnel every run-to-completion +/// launch passes through, and the first point at which every policy-affecting +/// mutation (CLI command override, `--audit` permissive-learning-mode +/// injection, capability injection) has already been applied. A hash taken +/// earlier would not describe what actually ran. +/// +/// `config_schema_version` is deliberately named for the *schema*: it does not +/// change when the policy changes, so it must never be read as a policy +/// version. +pub fn log_policy_hash(request: &ExecutionRequest, logger: &mut Logger) { + use wxc_common::audit::{AuditEvent, AuditEventName}; + + // Computing the hash serialises the whole effective request, canonicalises + // it, and runs SHA-256 over the result. That is far too much work to do on + // every launch only for `log_audit_event` to discard it, so check for a sink + // before building anything. + if !logger.has_diagnostic_sink() { + return; + } + let record = AuditEvent::new(AuditEventName::PolicyHash) + .str("backend", request.containment.wire_name()) + .str( + "policy_hash", + &wxc_common::policy_identity::policy_hash(request), + ) + .str("config_schema_version", &request.schema_version); + logger.log_audit_event(&record); +} + /// Resolve `request`'s backend and run it to completion. /// /// Convenience over [`resolve_runner`] for callers without external guard / @@ -130,6 +163,7 @@ fn resolve_runner_inner( "selected isolation tier: {}", dispatched.tier.as_str() ); + dispatched.log_enforcement_degraded(logger); let (runner, dacl_manager) = dispatched.into_runner_and_guard(); Ok(ResolvedRunner { runner, @@ -418,3 +452,25 @@ mod tests { assert!(warning.contains("fresh VM")); } } + +#[cfg(test)] +mod audit_tests { + use super::log_policy_hash; + use std::fs; + use wxc_common::logger::{Logger, Mode}; + use wxc_common::models::ExecutionRequest; + + #[test] + fn policy_hash_reaches_the_diagnostic_sink() { + let path = std::env::temp_dir().join(format!("mxc-policy-hash-{}.log", std::process::id())); + let mut logger = Logger::new(Mode::Buffer); + logger.enable_file_sink(&path).expect("file sink"); + + log_policy_hash(&ExecutionRequest::default(), &mut logger); + + let contents = fs::read_to_string(&path).expect("read audit log"); + assert!(contents.contains(r#""event":"mxc.PolicyHash""#)); + assert!(contents.contains(r#""policy_hash":"sha256:"#)); + let _ = fs::remove_file(path); + } +} diff --git a/src/core/mxc_engine/src/state_aware.rs b/src/core/mxc_engine/src/state_aware.rs index 1c7f33fd6..01fc274f0 100644 --- a/src/core/mxc_engine/src/state_aware.rs +++ b/src/core/mxc_engine/src/state_aware.rs @@ -110,7 +110,7 @@ fn parse_error_to_mxc(e: wxc_common::config_parser::ParseError) -> MxcError { use wxc_common::config_parser::ParseError; match e { ParseError::StateAware(err) => err, - ParseError::Decode(err) | ParseError::OneShot(err) => { + ParseError::Decode(err) | ParseError::OneShot(err) | ParseError::OneShotMalformed(err) => { MxcError::malformed_request(err.to_string()) } } diff --git a/src/core/wxc/src/main.rs b/src/core/wxc/src/main.rs index ccac609b5..59bb30726 100644 --- a/src/core/wxc/src/main.rs +++ b/src/core/wxc/src/main.rs @@ -15,14 +15,16 @@ use std::time::Instant; use appcontainer_common::appcontainer_runner::delete_app_container_profile; use clap::Parser; +use wxc_common::audit::{AuditEvent, AuditEventName, RejectionReason}; use wxc_common::cmdline::{cmdline_from_argv_for_context, CommandLineContext, CommandLineError}; use wxc_common::config_parser::{ load_mxc_request_with_options, load_request, LoadOptions, ParseError, }; +#[cfg(target_os = "windows")] use wxc_common::diagnostic::DiagnosticConfig; use wxc_common::logger::{Logger, Mode}; use wxc_common::models::{ContainmentBackend, ExecutionRequest, ScriptResponse}; -use wxc_common::mxc_error::{MxcError, ResponseEnvelope}; +use wxc_common::mxc_error::{MxcError, MxcErrorCode, ResponseEnvelope}; use wxc_common::script_runner::{handle_dry_run_exit, ScriptRunner}; use wxc_common::state_aware_dispatch::{resolve_backend, DispatchOutcome}; use wxc_common::state_aware_request::{MxcRequest, ParsedStateAwareRequest, Phase}; @@ -338,6 +340,94 @@ fn log_state_aware_dispatch_error(logger: &mut Logger, error: &MxcError) { logger.log_diagnostic_line(&error.to_string()); } +/// Record `mxc.ConfigRejected` for a request that was refused before it could +/// run. +/// +/// **Bounded content only.** The `reason` is a closed [`RejectionReason`] +/// variant and `offending_field` is a *config field path* (e.g. +/// `process.commandLine`) — never a field value, never the rich error text. +/// The human-readable diagnostic still reaches the operator on stderr and, for +/// state-aware phases, in the JSON error envelope; this record exists so the +/// same rejection is machine-readable without parsing prose. +/// +/// Unlike the ETW path this replaces, there is no initialisation-ordering +/// problem: `Logger` is constructed from CLI flags before any config is read, +/// so *every* rejection site — including "the input was not JSON at all" — can +/// reach it. +fn log_config_rejected( + logger: &mut Logger, + reason: RejectionReason, + backend: &str, + offending_field: &str, + phase: &str, +) { + let record = AuditEvent::new(AuditEventName::ConfigRejected) + .str("backend", backend) + .str("reason", reason.as_str()) + .str_opt("offending_field", offending_field) + .str_opt("phase", phase); + logger.log_audit_event(&record); +} + +/// Backend name for a rejection that happened before (or without) backend +/// resolution. +const UNKNOWN_BACKEND: &str = "unknown"; + +/// Resolve the wire backend name for a parsed state-aware request, falling back +/// to [`UNKNOWN_BACKEND`] when the request is too malformed to name one. +fn backend_name_for_state_aware(parsed: &ParsedStateAwareRequest) -> String { + resolve_backend(parsed) + .map(|b| b.wire_name().to_string()) + .unwrap_or_else(|_| UNKNOWN_BACKEND.to_string()) +} + +/// Map a state-aware [`MxcError`] to its bounded [`RejectionReason`]. +/// +/// Driven by the error's own `code`, which is already an exhaustive closed set — +/// no message-text matching. +fn rejection_reason_for(error: &MxcError) -> RejectionReason { + match error.code { + MxcErrorCode::MalformedRequest => RejectionReason::SchemaViolation, + MxcErrorCode::MalformedId => RejectionReason::IdentityShapeInvalid, + MxcErrorCode::PolicyValidation => RejectionReason::UnsupportedFieldForBackend, + MxcErrorCode::UnsupportedContainment => RejectionReason::UnsupportedContainment, + MxcErrorCode::UnsupportedPhase => RejectionReason::UnsupportedPhase, + MxcErrorCode::BackendUnavailable + | MxcErrorCode::StaleId + | MxcErrorCode::NotProvisioned + | MxcErrorCode::NotStarted + | MxcErrorCode::AlreadyStarted + | MxcErrorCode::AlreadyStopped + | MxcErrorCode::BackendError => RejectionReason::RunnerUnavailable, + } +} + +/// Resolve the sandbox id to report on `mxc.SandboxIdentity` for a completed +/// state-aware dispatch. +/// +/// `provision` mints the id, so it is read out of the result envelope; every +/// later phase carries the id inbound. Returns `None` when the phase failed — +/// a failed dispatch produced no sandbox to identify, and emitting an identity +/// record for one would be a lie. +fn sandbox_id_for_identity_record( + outcome: &Result, + incoming_sandbox_id: Option<&str>, +) -> Option { + let Ok(outcome) = outcome else { + return None; + }; + if let DispatchOutcome::Envelope(value) = outcome { + if let Some(minted) = value + .get("result") + .and_then(|r| r.get("sandboxId")) + .and_then(|v| v.as_str()) + { + return Some(minted.to_string()); + } + } + incoming_sandbox_id.map(str::to_string) +} + /// Drives the state-aware dispatch flow. On envelope success, writes the /// JSON to stdout and exits 0. On exec success, exits with the script's /// exit code (output already streamed). On failure, writes a JSON error @@ -414,9 +504,32 @@ fn run_state_aware_main( } let started = Instant::now(); + // Captured before `run_state_aware` consumes `parsed`. On provision the id + // does not exist yet and is read back out of the result envelope below. + let incoming_sandbox_id = parsed.sandbox_id.clone(); + // State-aware dispatch bypasses the one-shot runner funnel, so anchor the + // effective lifecycle policy here before the request is consumed. + mxc_engine::log_policy_hash(&parsed.request, logger); let mut outcome = mxc_engine::run_state_aware(parsed, dry_run); let elapsed = started.elapsed(); + // Record the sandbox identity join key. For `isolation_session` the + // `sandboxId` tail is the OS-side `provisionId`, which is what joins an MXC + // record to the `Microsoft.Windows.IsolationSession` OS records. Emitted on + // success only: a failed phase produced no sandbox to identify. + if let Some(sandbox_id) = + sandbox_id_for_identity_record(&outcome, incoming_sandbox_id.as_deref()) + { + let record = AuditEvent::new(AuditEventName::SandboxIdentity) + .str("backend", backend) + .str( + "identity", + &wxc_common::policy_identity::redact_identity(&sandbox_id), + ) + .str_opt("phase", phase); + logger.log_audit_event(&record); + } + // For provision, return the freshly-seeded correlation vector to the client // by injecting it into the result envelope so it can be relayed into later // phases. Gated on telemetry so provision output is unchanged when telemetry @@ -944,6 +1057,15 @@ fn main() { } } + // Initialize the diagnostic console before parsing so early rejection + // records have an active sink. + #[cfg(target_os = "windows")] + let diag_config = DiagnosticConfig::from_environment(); + #[cfg(target_os = "windows")] + if diag_config.console_enabled { + logger.enable_diagnostics(&diag_config); + } + // Delete mode if cli.delete { let name = match cli.containername { @@ -974,6 +1096,13 @@ fn main() { match command_override_context_for_state_aware(&parsed, has_command_override) { Ok(context) => context, Err(e) => { + log_config_rejected( + &mut logger, + rejection_reason_for(&e), + &backend_name_for_state_aware(&parsed), + "", + parsed.phase.as_str(), + ); print_error_envelope(&e); eprint!("{}", logger.get_buffer()); process::exit(1); @@ -985,6 +1114,13 @@ fn main() { { Ok(command_override) => command_override.flatten(), Err(e) => { + log_config_rejected( + &mut logger, + RejectionReason::InvalidCommandOverride, + &backend_name_for_state_aware(&parsed), + "process.commandLine", + parsed.phase.as_str(), + ); print_error_envelope(&MxcError::malformed_request(format!( "invalid CLI command override: {e}" ))); @@ -1009,11 +1145,50 @@ fn main() { parsed.request.dry_run = cli.dry_run; run_state_aware_main(parsed, cli.dry_run, cli.experimental, &mut logger) } - Err(ParseError::OneShot(_)) | Err(ParseError::Decode(_)) => { + Err(ParseError::Decode(_)) => { + // The payload could not even be decoded into JSON, so no backend or + // field path is known — the record still exists so a rejected run + // is never invisible. + log_config_rejected( + &mut logger, + RejectionReason::MalformedJson, + UNKNOWN_BACKEND, + "", + "", + ); + eprint!("Request error\n{}", logger.get_buffer()); + process::exit(1); + } + Err(ParseError::OneShotMalformed(_)) => { + log_config_rejected( + &mut logger, + RejectionReason::MalformedJson, + UNKNOWN_BACKEND, + "", + "", + ); + eprint!("Request error\n{}", logger.get_buffer()); + process::exit(1); + } + Err(ParseError::OneShot(_)) => { + log_config_rejected( + &mut logger, + RejectionReason::SchemaViolation, + UNKNOWN_BACKEND, + "", + "", + ); eprint!("Request error\n{}", logger.get_buffer()); process::exit(1); } Err(ParseError::StateAware(e)) => { + log_config_rejected( + &mut logger, + rejection_reason_for(&e), + UNKNOWN_BACKEND, + "", + "", + ); print_error_envelope(&e); eprint!("{}", logger.get_buffer()); process::exit(1); @@ -1054,6 +1229,13 @@ fn main() { ) { Ok(command_override) => command_override, Err(e) => { + log_config_rejected( + &mut logger, + RejectionReason::InvalidCommandOverride, + request.containment.wire_name(), + "process.commandLine", + "", + ); eprintln!("Request error\ninvalid CLI command override: {e}"); eprint!("{}", logger.get_buffer()); telemetry::emit_early_exit( @@ -1080,6 +1262,13 @@ fn main() { #[cfg(target_os = "windows")] if cli.audit { if let Err(message) = validate_audit_request(&request) { + log_config_rejected( + &mut logger, + RejectionReason::UnsupportedFieldForBackend, + request.containment.wire_name(), + "containment", + "", + ); eprintln!("Error: {message}"); telemetry::emit_early_exit( telemetry_active, @@ -1100,6 +1289,13 @@ fn main() { // Final validation: a command line must come from somewhere. If neither // the policy nor the CLI supplied one we cannot proceed. if request.script_code.is_empty() { + log_config_rejected( + &mut logger, + RejectionReason::MissingCommand, + request.containment.wire_name(), + "process.commandLine", + "", + ); eprintln!( "Error: no command to run. Provide `process.commandLine` in the policy or pass the command as arguments after the config path." ); @@ -1113,7 +1309,17 @@ fn main() { } // Inject learningModeLogging capability when diagnostic console is enabled. - let learning_mode_injected = if DiagnosticConfig::force_learning_mode() + let learning_mode_requested = { + #[cfg(target_os = "windows")] + { + DiagnosticConfig::force_learning_mode() + } + #[cfg(not(target_os = "windows"))] + { + false + } + }; + let learning_mode_injected = if learning_mode_requested && !request.policy.capabilities.iter().any(|c| { c.eq_ignore_ascii_case("learningModeLogging") || c.eq_ignore_ascii_case("permissiveLearningMode") @@ -1127,31 +1333,31 @@ fn main() { false }; - // Initialize diagnostic logging (registry/env-controlled). - let diag_config = DiagnosticConfig::from_environment(); - if diag_config.console_enabled { - logger.enable_diagnostics(&diag_config); - - // Log the preamble - let exe_path = std::env::current_exe() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_else(|_| "unknown".to_string()); - let parent_info = wxc_common::diagnostic::get_parent_process_info(); - let _ = writeln!( - logger, - "wxc-exec v{} (PID {})", - env!("CARGO_PKG_VERSION"), - std::process::id() - ); - let _ = writeln!(logger, "\tpath: {}", exe_path); - let _ = writeln!(logger, "\tparent: {}", parent_info); - - // Log if we're injecting Learning Mode - if learning_mode_injected { + // Emit the diagnostic preamble after the request is available. + #[cfg(target_os = "windows")] + { + if diag_config.console_enabled { + // Log the preamble + let exe_path = std::env::current_exe() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_else(|_| "unknown".to_string()); + let parent_info = wxc_common::diagnostic::get_parent_process_info(); let _ = writeln!( + logger, + "wxc-exec v{} (PID {})", + env!("CARGO_PKG_VERSION"), + std::process::id() + ); + let _ = writeln!(logger, "\tpath: {}", exe_path); + let _ = writeln!(logger, "\tparent: {}", parent_info); + + // Log if we're injecting Learning Mode + if learning_mode_injected { + let _ = writeln!( logger, "WARNING: injected 'learningModeLogging' capability via ForceLearningMode registry key" ); + } } // Log the raw input JSON config before any transformation. @@ -1171,16 +1377,19 @@ fn main() { let _ = writeln!(logger, "SECTION: Request simplified"); log_request(&request, &mut logger); - // Emit the full (redacted) request policy for diagnostics. - let _ = writeln!( - logger, - "SECTION: Full `ExecutionRequest` configuration (redacted)" - ); - let _ = writeln!( - logger, - "{}", - wxc_common::diagnostic::redacted_request_json(&request) - ); + #[cfg(target_os = "windows")] + { + // Emit the full (redacted) request policy for diagnostics. + let _ = writeln!( + logger, + "SECTION: Full `ExecutionRequest` configuration (redacted)" + ); + let _ = writeln!( + logger, + "{}", + wxc_common::diagnostic::redacted_request_json(&request) + ); + } // Run script in the selected containment backend. Backend selection and // runner construction — including the ProcessContainer BaseContainer / @@ -1200,6 +1409,13 @@ fn main() { resolved.runner } Err(e) => { + log_config_rejected( + &mut logger, + RejectionReason::RunnerUnavailable, + request.containment.wire_name(), + "containment", + "", + ); eprintln!("error: {}", e.message); eprint!("{}", logger.get_buffer()); telemetry::emit_early_exit( @@ -1427,6 +1643,156 @@ mod tests { Logger::new(Mode::Buffer) } + /// Every rejection reason must come from the error's own closed `code`, not + /// from matching its message text — the message is prose that can embed + /// paths and is not a stable vocabulary. + #[test] + fn rejection_reason_is_driven_by_the_error_code() { + let cases = [ + ( + MxcError::malformed_request("x"), + RejectionReason::SchemaViolation, + ), + ( + MxcError::malformed_id("x"), + RejectionReason::IdentityShapeInvalid, + ), + ( + MxcError::policy_validation("x"), + RejectionReason::UnsupportedFieldForBackend, + ), + ( + MxcError::unsupported_containment("x"), + RejectionReason::UnsupportedContainment, + ), + ( + MxcError::unsupported_phase("x"), + RejectionReason::UnsupportedPhase, + ), + ( + MxcError::backend_unavailable("x"), + RejectionReason::RunnerUnavailable, + ), + (MxcError::stale_id("x"), RejectionReason::RunnerUnavailable), + ( + MxcError::not_provisioned("x"), + RejectionReason::RunnerUnavailable, + ), + ( + MxcError::not_started("x"), + RejectionReason::RunnerUnavailable, + ), + ( + MxcError::already_started("x"), + RejectionReason::RunnerUnavailable, + ), + ( + MxcError::already_stopped("x"), + RejectionReason::RunnerUnavailable, + ), + ( + MxcError::backend_error("x"), + RejectionReason::RunnerUnavailable, + ), + ]; + for (error, expected) in cases { + assert_eq!( + rejection_reason_for(&error), + expected, + "code {:?} mapped to the wrong reason", + error.code + ); + } + } + + /// The record must carry the bounded reason and the field *path* — never + /// the offending value, and never the rich error text. + #[test] + fn config_rejected_record_carries_no_free_form_text() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("audit.log"); + let mut logger = test_logger(); + logger.enable_file_sink(&path).expect("file sink"); + + log_config_rejected( + &mut logger, + RejectionReason::MissingCommand, + "processcontainer", + "process.commandLine", + "", + ); + drop(logger); + + let contents = std::fs::read_to_string(&path).expect("read log"); + assert!( + contents.contains(r#""reason":"missing_command""#), + "got: {contents}" + ); + assert!( + contents.contains(r#""offending_field":"process.commandLine""#), + "got: {contents}" + ); + // A one-shot run has no lifecycle phase, so the field is omitted rather + // than emitted as a meaningless empty string. + assert!(!contents.contains("\"phase\""), "got: {contents}"); + } + + #[test] + fn identity_record_reads_the_minted_id_out_of_a_provision_envelope() { + let outcome = Ok(DispatchOutcome::Envelope( + serde_json::json!({"result": {"sandboxId": "iso:wxc-abcd1234"}}), + )); + assert_eq!( + sandbox_id_for_identity_record(&outcome, None).as_deref(), + Some("iso:wxc-abcd1234") + ); + } + + #[test] + fn identity_record_falls_back_to_the_inbound_id_for_later_phases() { + // Later phases return an envelope with no `sandboxId` (the client + // already has it), so the inbound id is the one to report. + let outcome = Ok(DispatchOutcome::Envelope(serde_json::json!({"result": {}}))); + assert_eq!( + sandbox_id_for_identity_record(&outcome, Some("iso:wxc-abcd1234")).as_deref(), + Some("iso:wxc-abcd1234") + ); + + // Exec completes without an envelope at all. + let exec = Ok(DispatchOutcome::ExecCompleted { exit_code: 0 }); + assert_eq!( + sandbox_id_for_identity_record(&exec, Some("iso:wxc-abcd1234")).as_deref(), + Some("iso:wxc-abcd1234") + ); + } + + #[test] + fn no_identity_record_for_a_failed_phase() { + // A failed dispatch produced no sandbox to identify; claiming one would + // be a lie. + let outcome = Err(MxcError::backend_unavailable("nope")); + assert!(sandbox_id_for_identity_record(&outcome, Some("iso:wxc-abcd1234")).is_none()); + } + + #[test] + fn entra_provision_ids_are_never_logged_verbatim() { + // `state_aware.rs::provision` sets `provision_id = user.upn` for Entra + // sandboxes, so the sandboxId tail is a real user identifier. It must not + // reach a log file in any recoverable form. + let outcome = Ok(DispatchOutcome::Envelope( + serde_json::json!({"result": {"sandboxId": "iso:alice@contoso.com"}}), + )); + let id = sandbox_id_for_identity_record(&outcome, None).expect("id"); + let rendered = wxc_common::policy_identity::redact_identity(&id); + assert!(!rendered.contains("alice"), "got: {rendered}"); + assert!(!rendered.contains('@'), "got: {rendered}"); + assert_eq!( + rendered, + wxc_common::policy_identity::ENTRA_UPN_MARKER, + "got: {rendered}" + ); + } + #[test] fn audit_mode_replaces_deny_and_record_capability() { let mut capabilities = vec![ diff --git a/src/core/wxc_common/Cargo.toml b/src/core/wxc_common/Cargo.toml index 339674f13..e62cfe73a 100644 --- a/src/core/wxc_common/Cargo.toml +++ b/src/core/wxc_common/Cargo.toml @@ -12,8 +12,9 @@ schema-gen = ["dep:schemars"] [dependencies] serde = { workspace = true } -serde_json = { workspace = true, features = ["raw_value"] } +serde_json = { workspace = true, features = ["raw_value"] } serde_path_to_error = { workspace = true } +sha2 = { workspace = true } unicode-general-category = { workspace = true } thiserror = { workspace = true } base64 = { workspace = true } diff --git a/src/core/wxc_common/src/audit.rs b/src/core/wxc_common/src/audit.rs new file mode 100644 index 000000000..f8f174239 --- /dev/null +++ b/src/core/wxc_common/src/audit.rs @@ -0,0 +1,637 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Structured **local** diagnostic audit records. +//! +//! These are *not* ETW events. Each record is serialised to a single JSON line +//! and written through [`Logger::log_audit_event`](crate::logger::Logger::log_audit_event), +//! which forwards it to the auxiliary diagnostic sinks only — the `--log-file` +//! sink and the `MXC_DIAG_CONSOLE` named pipe. It deliberately never reaches the +//! primary console/buffer sink, which is the SDK caller's captured output. +//! +//! # Format +//! +//! One record = one line = one JSON object: +//! +//! ```text +//! {"event":"mxc.ProcessExited","backend":"processcontainer","identity":"sandbox-a3f1c8e40029bd17","pid":1234,"exit_code":0} +//! ``` +//! +//! Invariants, all enforced by this module: +//! +//! 1. `"event"` is always the first key, so a consumer can classify a line with a +//! prefix match before parsing it. +//! 2. Field order is the builder's call order — deterministic per call site. +//! 3. Values are strings, integers, or booleans only. No nested objects, no +//! arrays: a set-valued field is emitted as a comma-joined bounded string plus +//! an explicit `_count` companion. +//! 4. String values are escaped by `serde_json`, so an embedded quote, backslash, +//! or newline can never break the one-record-per-line invariant. +//! 5. Event names come from the closed [`AuditEventName`] enum — a typo is a +//! compile error, not a silently unmatched record. +//! +//! # Content rules +//! +//! No free-form text. Reasons, statuses, and methods are bounded enums; error +//! detail is reduced to a numeric code. **No config values, no filesystem paths, +//! no command lines, and no raw UPNs** — a diagnostic log file is routinely +//! attached to a bug report. Config field *paths* are permitted (they are bounded +//! and already public in the schema); field *values* are not. +//! +//! Caller-supplied identifiers (e.g. `containerId`, which becomes the +//! AppContainer profile name and therefore the sandbox identity) are *config +//! values*. Pass them through [`sanitize_identity`] before they reach a record. + +/// Closed set of audit record names. The `mxc.` prefix namespaces the record/// against unrelated lines sharing the same sink. +/// +/// The name is the stability anchor for a record's field set: if a record has to +/// change incompatibly, mint a new variant rather than redefining an existing +/// one. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AuditEventName { + /// A `process_container` sandboxed process exited on its own. + ProcessExited, + /// A sandboxed process exceeded `scriptTimeout` and was force-terminated. + ProcessTimedOut, + /// A kill/terminate call failed. Emitted only on failure, so a healthy run + /// produces none. + ProcessKillFailed, + /// The preferred isolation tier was not selected. + EnforcementDegraded, + /// The canonical hash of the effective policy at launch. + PolicyHash, + /// Network policy was installed (or failed to install) for a sandbox. + NetworkPolicyApplied, + /// Per-run sandbox resources were released. + SandboxTornDown, + /// A request was rejected before (or during) validation. + ConfigRejected, + /// The sandbox identity join key, emitted once per lifecycle. + SandboxIdentity, +} + +impl AuditEventName { + /// Wire name written into the `event` field. + pub fn as_str(self) -> &'static str { + match self { + Self::ProcessExited => "mxc.ProcessExited", + Self::ProcessTimedOut => "mxc.ProcessTimedOut", + Self::ProcessKillFailed => "mxc.ProcessKillFailed", + Self::EnforcementDegraded => "mxc.EnforcementDegraded", + Self::PolicyHash => "mxc.PolicyHash", + Self::NetworkPolicyApplied => "mxc.NetworkPolicyApplied", + Self::SandboxTornDown => "mxc.SandboxTornDown", + Self::ConfigRejected => "mxc.ConfigRejected", + Self::SandboxIdentity => "mxc.SandboxIdentity", + } + } +} + +impl std::fmt::Display for AuditEventName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +/// Which OS primitive was used for a kill attempt. Determined by the call site, +/// never inferred from an error. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum KillMethod { + /// `TerminateJobObject` — tree-kill via the job the child was assigned to. + TerminateJobObject, + /// `TerminateProcess` — root-only fallback when no job is available. + TerminateProcess, +} + +impl KillMethod { + pub fn as_str(self) -> &'static str { + match self { + Self::TerminateJobObject => "terminate_job_object", + Self::TerminateProcess => "terminate_process", + } + } +} + +/// Outcome of a teardown pass. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TeardownStatus { + /// Everything that was supposed to be released was released. + Success, + /// At least one release step reported a failure. + Failure, + /// Release was deliberately not attempted (e.g. `preserve_policy`, or a + /// cleanup path that is not implemented yet). + Skipped, +} + +impl TeardownStatus { + pub fn as_str(self) -> &'static str { + match self { + Self::Success => "success", + Self::Failure => "failure", + Self::Skipped => "skipped", + } + } +} + +/// Why a teardown reported [`TeardownStatus::Skipped`]. Bounded so the record +/// never carries prose. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TeardownSkipReason { + /// `lifecycle.preservePolicy` asked for the policy to outlive the run. + PreservePolicy, + /// The BaseContainer per-sandbox cleanup path is a documented no-op stub + /// (child-process tracking is not implemented). Reporting this honestly is + /// preferable to a green record that claims a cleanup that did not happen. + CleanupNotImplemented, +} + +impl TeardownSkipReason { + pub fn as_str(self) -> &'static str { + match self { + Self::PreservePolicy => "preserve_policy", + Self::CleanupNotImplemented => "cleanup_not_implemented", + } + } +} + +/// Generic success/failure status for a single operation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum OperationStatus { + Success, + Failure, +} + +impl OperationStatus { + pub fn as_str(self) -> &'static str { + match self { + Self::Success => "success", + Self::Failure => "failure", + } + } +} + +/// Closed vocabulary for the effective process-container enforcement level. +/// +/// This is derived from the selected isolation tier and whether host-DACL +/// augmentation was required. It describes the enforcement mechanism MXC +/// selected; it does not claim any additional OS telemetry state. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum EffectiveEnforcementLevel { + BaseContainer, + BaseContainerDaclAugmented, + AppContainerBfs, + AppContainerBfsDaclAugmented, + AppContainerDacl, +} + +impl EffectiveEnforcementLevel { + pub fn as_str(self) -> &'static str { + match self { + Self::BaseContainer => "base-container", + Self::BaseContainerDaclAugmented => "base-container-dacl-augmented", + Self::AppContainerBfs => "appcontainer-bfs", + Self::AppContainerBfsDaclAugmented => "appcontainer-bfs-dacl-augmented", + Self::AppContainerDacl => "appcontainer-dacl", + } + } +} + +/// Closed set of reasons a request was rejected. Each variant corresponds to a +/// distinct existing rejection path — none is invented, and none is derived by +/// pattern-matching an error message. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RejectionReason { + /// The input was not valid JSON (or not valid base64-wrapped JSON). + MalformedJson, + /// The input parsed as JSON but violated the config schema. + SchemaViolation, + /// Neither the policy nor the CLI supplied a command line. + MissingCommand, + /// A field is valid in the schema but unsupported by the resolved backend. + UnsupportedFieldForBackend, + /// The CLI command override could not be applied. + InvalidCommandOverride, + /// A sandbox id or other identifier had the wrong shape. + IdentityShapeInvalid, + /// The requested containment backend is not supported here. + UnsupportedContainment, + /// The requested state-aware phase is not supported by the backend. + UnsupportedPhase, + /// A runner could not be resolved for the request on this host. + RunnerUnavailable, +} + +impl RejectionReason { + pub fn as_str(self) -> &'static str { + match self { + Self::MalformedJson => "malformed_json", + Self::SchemaViolation => "schema_violation", + Self::MissingCommand => "missing_command", + Self::UnsupportedFieldForBackend => "unsupported_field_for_backend", + Self::InvalidCommandOverride => "invalid_command_override", + Self::IdentityShapeInvalid => "identity_shape_invalid", + Self::UnsupportedContainment => "unsupported_containment", + Self::UnsupportedPhase => "unsupported_phase", + Self::RunnerUnavailable => "runner_unavailable", + } + } +} + +/// A single audit record under construction. +/// +/// Build with [`AuditEvent::new`] and the typed `str`/`u64`/`i64`/`bool` +/// setters, then hand it to +/// [`Logger::log_audit_event`](crate::logger::Logger::log_audit_event). +/// +/// ``` +/// use wxc_common::audit::{AuditEvent, AuditEventName}; +/// +/// let line = AuditEvent::new(AuditEventName::ProcessExited) +/// .str("backend", "processcontainer") +/// .u64("pid", 1234) +/// .i64("exit_code", 0) +/// .to_json_line(); +/// assert!(line.starts_with(r#"{"event":"mxc.ProcessExited","backend":"processcontainer""#)); +/// ``` +#[derive(Debug, Clone)] +pub struct AuditEvent { + name: AuditEventName, + /// Pre-rendered `"key":value` fragments in declaration order. Rendering + /// eagerly keeps the struct free of an enum-per-value and makes the escaping + /// rule impossible to bypass. + fields: Vec, +} + +impl AuditEvent { + /// Start a record with the given closed event name. + pub fn new(name: AuditEventName) -> Self { + Self { + name, + fields: Vec::new(), + } + } + + /// Append a string field. The value is JSON-escaped, so it can never break + /// the one-record-per-line invariant. + /// + /// Callers are responsible for the *content* rule: bounded vocabularies, + /// identifiers, and config field paths only — never a config value, a + /// filesystem path, a command line, or a raw UPN. + pub fn str(mut self, key: &str, value: &str) -> Self { + self.push_field(key, &escape_json_string(value)); + self + } + + /// Append a string field only when `value` is non-empty. Keeps records + /// narrow when a field is genuinely inapplicable (e.g. `phase` for a + /// one-shot run) instead of emitting a meaningless empty string. + pub fn str_opt(self, key: &str, value: &str) -> Self { + if value.is_empty() { + self + } else { + self.str(key, value) + } + } + + /// Append an unsigned integer field. + pub fn u64(mut self, key: &str, value: u64) -> Self { + self.push_field(key, &value.to_string()); + self + } + + /// Append a signed integer field. + pub fn i64(mut self, key: &str, value: i64) -> Self { + self.push_field(key, &value.to_string()); + self + } + + /// Append a boolean field. + pub fn bool(mut self, key: &str, value: bool) -> Self { + self.push_field(key, if value { "true" } else { "false" }); + self + } + + /// Render the record as one JSON object on a single line (no trailing + /// newline — the sink adds it). + pub fn to_json_line(&self) -> String { + // 32 bytes covers `{"event":"…"}` for every current name; the fields add + // the rest. One allocation for the common case. + let mut out = + String::with_capacity(32 + self.fields.iter().map(|f| f.len() + 1).sum::()); + out.push_str("{\"event\":\""); + // Every `AuditEventName` is a compile-time `mxc.` literal + // with nothing to escape, asserted by `event_names_need_no_escaping`. + out.push_str(self.name.as_str()); + out.push('"'); + for field in &self.fields { + out.push(','); + out.push_str(field); + } + out.push('}'); + out + } + + fn push_field(&mut self, key: &str, rendered_value: &str) { + debug_assert!( + is_bare_json_key(key), + "audit field keys must be lower_snake_case ASCII so they need no escaping: {key:?}" + ); + let mut field = String::with_capacity(key.len() + rendered_value.len() + 4); + field.push('"'); + field.push_str(key); + field.push_str("\":"); + field.push_str(rendered_value); + self.fields.push(field); + } +} + +/// Whether `key` is a bare `lower_snake_case` ASCII identifier, i.e. contains +/// nothing JSON would need to escape. +/// +/// Every key in this codebase is a call-site literal, so this holds by +/// construction; the check exists so a future non-literal key fails loudly in a +/// debug build instead of silently producing malformed JSON. +fn is_bare_json_key(key: &str) -> bool { + !key.is_empty() + && key + .bytes() + .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'_') +} + +/// Maximum length of a sanitized identity. Long enough for the two shapes MXC +/// mints itself (`sandbox-<16 hex>` = 24 chars, `wxc-`), short enough +/// that a caller cannot smuggle a payload through the field. +const MAX_IDENTITY_LEN: usize = 64; + +/// Placeholder written when a caller-supplied identity is not an opaque token. +pub const REDACTED_IDENTITY: &str = "redacted"; + +/// Render a sandbox identity so it is safe to write to a diagnostic log file. +/// +/// **Sandbox identities are not always MXC-minted.** On the Windows +/// ProcessContainer path with `destroy_on_exit = false`, the identity is the +/// AppContainer profile name, which is the caller-supplied `containerId` +/// straight out of the config. That makes it a *config value*, and config values +/// must never reach a record (see the module-level content rules) — a caller can +/// otherwise put a UPN, a path, a ticket number, or an arbitrary string into the +/// audit stream. +/// +/// This function therefore allows through only identities that are recognisably +/// opaque tokens: +/// +/// * bounded length ([`MAX_IDENTITY_LEN`]); +/// * ASCII alphanumeric plus `-`, `_`, and `.` only — with the two MXC-minted +/// `iso:` and `wsb:` shapes also accepted; no `@` (UPN), no +/// `\` or `/` (path), no whitespace, no control characters. +/// +/// Anything else becomes [`REDACTED_IDENTITY`]. That loses the join key for +/// callers who chose a non-opaque `containerId`, which is the correct trade: a +/// record with no join key is recoverable, a leaked identifier is not. +pub fn sanitize_identity(identity: &str) -> &str { + if identity.is_empty() { + return identity; + } + let opaque = identity.len() <= MAX_IDENTITY_LEN + && identity + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.'); + let mxc_opaque = identity + .split_once(':') + .map(|(prefix, token)| { + matches!(prefix, "iso" | "wsb") + && !token.is_empty() + && token + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.') + }) + .unwrap_or(false); + if opaque || mxc_opaque { + identity + } else { + REDACTED_IDENTITY + } +} + +/// Render `value` as a quoted, escaped JSON string. +/// +/// `serde_json::to_string` on a `&str` is infallible (a `&str` is always valid +/// UTF-8 and the only failure mode of the `String` serializer is an I/O error, +/// which cannot occur for an in-memory buffer). +fn escape_json_string(value: &str) -> String { + serde_json::to_string(value).expect("serializing a &str to JSON is infallible") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn event_key_is_always_first() { + let line = AuditEvent::new(AuditEventName::SandboxIdentity) + .str("backend", "processcontainer") + .to_json_line(); + assert!( + line.starts_with(r#"{"event":"mxc.SandboxIdentity","#), + "got: {line}" + ); + } + + #[test] + fn fields_keep_declaration_order() { + let line = AuditEvent::new(AuditEventName::ProcessExited) + .str("backend", "processcontainer") + .str("identity", "sandbox-0123456789abcdef") + .u64("pid", 4242) + .i64("exit_code", -1) + .to_json_line(); + assert_eq!( + line, + r#"{"event":"mxc.ProcessExited","backend":"processcontainer","identity":"sandbox-0123456789abcdef","pid":4242,"exit_code":-1}"# + ); + } + + #[test] + fn string_values_are_escaped_so_one_record_stays_one_line() { + let line = AuditEvent::new(AuditEventName::ConfigRejected) + .str("offending_field", "weird\"key\nwith\\breaks") + .to_json_line(); + assert!(!line.contains('\n'), "record spans lines: {line}"); + assert!( + line.contains(r#""weird\"key\nwith\\breaks""#), + "got: {line}" + ); + } + + #[test] + fn every_record_parses_as_json() { + let line = AuditEvent::new(AuditEventName::SandboxTornDown) + .str("backend", "processcontainer") + .str("status", TeardownStatus::Failure.as_str()) + .bool("firewall_removal_ok", false) + .u64("firewall_rules_removed", 3) + .to_json_line(); + let parsed: serde_json::Value = serde_json::from_str(&line).expect("valid JSON"); + assert_eq!(parsed["event"], "mxc.SandboxTornDown"); + assert_eq!(parsed["status"], "failure"); + assert_eq!(parsed["firewall_removal_ok"], false); + assert_eq!(parsed["firewall_rules_removed"], 3); + } + + #[test] + fn str_opt_omits_empty_values() { + let line = AuditEvent::new(AuditEventName::ConfigRejected) + .str_opt("phase", "") + .str_opt("backend", "lxc") + .to_json_line(); + assert!(!line.contains("phase"), "got: {line}"); + assert!(line.contains(r#""backend":"lxc""#), "got: {line}"); + } + + #[test] + fn bounded_vocabularies_are_snake_case_and_distinct() { + let names = [ + KillMethod::TerminateJobObject.as_str(), + KillMethod::TerminateProcess.as_str(), + TeardownStatus::Success.as_str(), + TeardownStatus::Failure.as_str(), + TeardownStatus::Skipped.as_str(), + TeardownSkipReason::PreservePolicy.as_str(), + TeardownSkipReason::CleanupNotImplemented.as_str(), + RejectionReason::MalformedJson.as_str(), + RejectionReason::SchemaViolation.as_str(), + RejectionReason::MissingCommand.as_str(), + RejectionReason::UnsupportedFieldForBackend.as_str(), + RejectionReason::InvalidCommandOverride.as_str(), + RejectionReason::IdentityShapeInvalid.as_str(), + RejectionReason::UnsupportedContainment.as_str(), + RejectionReason::UnsupportedPhase.as_str(), + RejectionReason::RunnerUnavailable.as_str(), + ]; + for name in names { + assert!( + name.chars() + .all(|c| c.is_ascii_lowercase() || c == '_' || c.is_ascii_digit()), + "not snake_case: {name}" + ); + } + } + + #[test] + fn effective_enforcement_levels_are_stable() { + assert_eq!( + EffectiveEnforcementLevel::BaseContainer.as_str(), + "base-container" + ); + assert_eq!( + EffectiveEnforcementLevel::BaseContainerDaclAugmented.as_str(), + "base-container-dacl-augmented" + ); + assert_eq!( + EffectiveEnforcementLevel::AppContainerBfs.as_str(), + "appcontainer-bfs" + ); + assert_eq!( + EffectiveEnforcementLevel::AppContainerBfsDaclAugmented.as_str(), + "appcontainer-bfs-dacl-augmented" + ); + assert_eq!( + EffectiveEnforcementLevel::AppContainerDacl.as_str(), + "appcontainer-dacl" + ); + } + + #[test] + fn event_names_are_unique_and_prefixed() { + let names = [ + AuditEventName::ProcessExited, + AuditEventName::ProcessTimedOut, + AuditEventName::ProcessKillFailed, + AuditEventName::EnforcementDegraded, + AuditEventName::PolicyHash, + AuditEventName::NetworkPolicyApplied, + AuditEventName::SandboxTornDown, + AuditEventName::ConfigRejected, + AuditEventName::SandboxIdentity, + ]; + let mut seen = std::collections::HashSet::new(); + for name in names { + assert!(name.as_str().starts_with("mxc."), "got: {name}"); + assert!(seen.insert(name.as_str()), "duplicate: {name}"); + } + } + + /// `to_json_line` writes the event name without escaping it, which is only + /// sound because every name is an ASCII `mxc.` literal. + #[test] + fn event_names_need_no_escaping() { + let names = [ + AuditEventName::ProcessExited, + AuditEventName::ProcessTimedOut, + AuditEventName::ProcessKillFailed, + AuditEventName::EnforcementDegraded, + AuditEventName::PolicyHash, + AuditEventName::NetworkPolicyApplied, + AuditEventName::SandboxTornDown, + AuditEventName::ConfigRejected, + AuditEventName::SandboxIdentity, + ]; + for name in names { + let raw = name.as_str(); + assert_eq!( + serde_json::to_string(raw).expect("infallible"), + format!("\"{raw}\""), + "event name would need escaping: {raw}" + ); + } + } + + /// Field keys are written without escaping too, so they must all be bare + /// snake_case. Exercised through the debug assertion in `push_field`. + #[test] + fn field_keys_must_be_bare_snake_case() { + assert!(is_bare_json_key("exit_code")); + assert!(is_bare_json_key("pid")); + assert!(is_bare_json_key("degradation_reason_count")); + assert!(!is_bare_json_key("")); + assert!(!is_bare_json_key("Exit-Code")); + assert!(!is_bare_json_key("quote\"key")); + } + + #[test] + fn opaque_identities_pass_sanitization_unchanged() { + for id in [ + "sandbox-a3f1c8e40029bd17", + "wxc-abcd1234", + "iso:wxc-abcd1234", + "wsb:deadbeef", + "CLI", + "my.container_id-7", + "", + ] { + assert_eq!(sanitize_identity(id), id); + } + } + + /// A caller-supplied `containerId` becomes the AppContainer profile name and + /// therefore the sandbox identity. It is a config value, so anything that is + /// not recognisably an opaque token must not reach a record. + #[test] + fn non_opaque_caller_identities_are_redacted() { + for id in [ + "alice@contoso.com", + "C:\\Users\\alice\\secret", + "/home/alice/secret", + "has space", + "has\"quote", + "has\nnewline", + &"x".repeat(MAX_IDENTITY_LEN + 1), + ] { + assert_eq!( + sanitize_identity(id), + REDACTED_IDENTITY, + "should have been redacted: {id:?}" + ); + } + } +} diff --git a/src/core/wxc_common/src/config_parser.rs b/src/core/wxc_common/src/config_parser.rs index a51dba6df..22094bd9b 100644 --- a/src/core/wxc_common/src/config_parser.rs +++ b/src/core/wxc_common/src/config_parser.rs @@ -30,6 +30,8 @@ pub enum ParseError { Decode(WxcError), /// Discriminated as one-shot; conversion to `ExecutionRequest` failed. OneShot(WxcError), + /// Discriminated as one-shot, but the JSON payload was malformed. + OneShotMalformed(WxcError), /// Discriminated as state-aware; conversion to `ParsedStateAwareRequest` /// failed. Carries an `MxcError` so the driver can emit a typed envelope. StateAware(MxcError), @@ -44,14 +46,16 @@ enum ErrorOutput { impl ParseError { fn output(&self) -> ErrorOutput { match self { - Self::Decode(_) | Self::OneShot(_) => ErrorOutput::Primary, + Self::Decode(_) | Self::OneShot(_) | Self::OneShotMalformed(_) => ErrorOutput::Primary, Self::StateAware(_) => ErrorOutput::DiagnosticOnly, } } fn message(&self) -> String { match self { - Self::Decode(error) | Self::OneShot(error) => error.to_string(), + Self::Decode(error) | Self::OneShot(error) | Self::OneShotMalformed(error) => { + error.to_string() + } Self::StateAware(error) => error.to_string(), } } @@ -229,8 +233,16 @@ fn parse_mxc_request_json( .map(MxcRequest::StateAware) .map_err(|e| ParseError::StateAware(MxcError::malformed_request(e.to_string()))) } else { - let cfg: wire::MxcConfig = config_deserialize::from_str(json_str) - .map_err(|error| ParseError::OneShot(WxcError::ConfigParse(error.to_string())))?; + let cfg: wire::MxcConfig = config_deserialize::from_str(json_str).map_err(|error| { + let error = WxcError::ConfigParse(error.to_string()); + if error.to_string().contains("expected value") + || error.to_string().contains("EOF while parsing") + { + ParseError::OneShotMalformed(error) + } else { + ParseError::OneShot(error) + } + })?; convert_wire_config(cfg, logger, true, allow_missing_command) .map(MxcRequest::OneShot) .map_err(ParseError::OneShot) diff --git a/src/core/wxc_common/src/diagnostic.rs b/src/core/wxc_common/src/diagnostic.rs index 93f7ed016..bf229971d 100644 --- a/src/core/wxc_common/src/diagnostic.rs +++ b/src/core/wxc_common/src/diagnostic.rs @@ -15,20 +15,45 @@ use windows::Win32::Security::{GetTokenInformation, TokenUser, TOKEN_QUERY, TOKE use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; const ENV_CONSOLE: &str = "MXC_DIAG_CONSOLE"; +const ENV_PIPE_TOKEN: &str = "MXC_DIAG_PIPE_TOKEN"; const PIPE_NAME_PREFIX: &str = r"\\.\pipe\mxc-diagnostics"; -/// Build the diagnostic pipe name for the current user: `\\.\pipe\mxc-diagnostics-{SID}`. -/// -/// Falls back to the bare prefix if the SID cannot be determined. +/// Build the per-user, per-session diagnostic pipe name. pub fn diagnostic_pipe_name() -> String { - match get_current_user_sid() { - Some(sid) => format!("{PIPE_NAME_PREFIX}-{sid}"), - None => PIPE_NAME_PREFIX.to_string(), + let suffix = diagnostic_pipe_token() + .map(|token| format!("-{token}")) + .unwrap_or_default(); + match current_user_sid() { + Some(sid) => format!("{PIPE_NAME_PREFIX}-{sid}{suffix}"), + None => format!("{PIPE_NAME_PREFIX}{suffix}"), + } +} + +/// Return the caller-provided per-session pipe token when it has sufficient +/// entropy for a pipe name. +pub fn diagnostic_pipe_token() -> Option { + let token = env::var(ENV_PIPE_TOKEN).ok()?; + if !is_valid_pipe_token(&token) { + return None; } + Some(token) +} + +fn is_valid_pipe_token(token: &str) -> bool { + token.len() >= 32 + && token + .bytes() + .all(|byte| byte.is_ascii_hexdigit() || byte == b'-') + && token + .bytes() + .filter(|byte| *byte != b'-') + .collect::>() + .len() + >= 4 } /// Retrieve the SID string for the current process token's user. -fn get_current_user_sid() -> Option { +pub fn current_user_sid() -> Option { use crate::string_util::sid_to_string; use windows::Win32::Foundation::HANDLE; @@ -99,7 +124,7 @@ impl DiagnosticConfig { /// `learningModeLogging` capability is automatically injected into the /// container policy so that access-check ETW events are captured. pub fn force_learning_mode() -> bool { - env_bool(ENV_CONSOLE).unwrap_or(false) + env_bool(ENV_CONSOLE).unwrap_or(false) && diagnostic_pipe_token().is_some() } } @@ -134,6 +159,14 @@ pub fn redacted_request_json(request: &ExecutionRequest) -> String { .push_str(&format!("... ({total_len} chars total)")); } + // Never persist Entra credentials or account identifiers in diagnostics. + if let Some(isolation_session) = redacted.experimental.isolation_session.as_mut() { + if let Some(user) = isolation_session.user.as_mut() { + user.upn = "".to_string(); + user.wam_token = "".to_string(); + } + } + // Serialize the redacted request. let json = serde_json::to_string_pretty(&redacted) .unwrap_or_else(|e| format!("{{\"error\": \"failed to serialize request: {e}\"}}")); @@ -307,9 +340,35 @@ mod tests { assert!(json.contains("network_proxy: disabled")); } + #[test] + fn redacted_request_hides_isolation_session_credentials() { + let mut request = ExecutionRequest::default(); + request.experimental.isolation_session = Some(crate::models::IsolationSessionConfig { + user: Some(crate::models::IsolationSessionUser { + upn: "user@example.com".to_string(), + wam_token: "super-secret-bearer-token".to_string(), + }), + ..Default::default() + }); + + let json = redacted_request_json(&request); + assert!(!json.contains("user@example.com")); + assert!(!json.contains("super-secret-bearer-token")); + assert_eq!(json.matches("").count(), 2); + } + #[test] fn env_bool_parses_correctly() { // env_bool on non-existent var returns None assert!(env_bool("MXC_TEST_NONEXISTENT_VAR_12345").is_none()); } + + #[test] + fn pipe_tokens_require_length_and_safe_characters() { + assert!(is_valid_pipe_token("0123456789abcdef0123456789abcdef")); + assert!(is_valid_pipe_token("0123456789abcdef0123456789ab-cdef")); + assert!(!is_valid_pipe_token("0123456789abcdef")); + assert!(!is_valid_pipe_token("0123456789abcdef0123456789abcde!")); + assert!(!is_valid_pipe_token("--------------------------------")); + } } diff --git a/src/core/wxc_common/src/lib.rs b/src/core/wxc_common/src/lib.rs index ffe87af44..db36ee2a4 100644 --- a/src/core/wxc_common/src/lib.rs +++ b/src/core/wxc_common/src/lib.rs @@ -3,6 +3,7 @@ // Platform-agnostic modules (shared by wxc-exec, lxc-exec, mxc-exec-mac // and every backend crate). +pub mod audit; pub mod cmdline; mod config_deserialize; pub mod config_parser; @@ -20,6 +21,7 @@ pub mod logger; pub mod microvm_staging; pub mod models; pub mod mxc_error; +pub mod policy_identity; pub mod proxy_env; pub mod sandbox_process; pub mod script_runner; diff --git a/src/core/wxc_common/src/logger.rs b/src/core/wxc_common/src/logger.rs index 25b99b6b0..ae8fa50ec 100644 --- a/src/core/wxc_common/src/logger.rs +++ b/src/core/wxc_common/src/logger.rs @@ -88,12 +88,15 @@ impl Logger { /// level or above to prevent a rogue process from intercepting diagnostic data. #[cfg(target_os = "windows")] fn connect_diagnostic_pipe(&mut self) { + if crate::diagnostic::diagnostic_pipe_token().is_none() { + eprintln!( + "[MXC Diagnostics] Refusing an unauthenticated diagnostic pipe; \ + set MXC_DIAG_PIPE_TOKEN to a high-entropy token." + ); + return; + } use std::os::windows::fs::OpenOptionsExt; use std::os::windows::io::AsRawHandle; - use windows::Win32::Foundation::{CloseHandle, HANDLE}; - use windows::Win32::Security::{ - GetTokenInformation, TokenIntegrityLevel, TOKEN_MANDATORY_LABEL, TOKEN_QUERY, - }; use windows::Win32::Storage::FileSystem::FILE_FLAG_WRITE_THROUGH; use windows::Win32::System::Pipes::GetNamedPipeServerProcessId; use windows::Win32::System::Threading::{ @@ -104,6 +107,11 @@ impl Logger { match std::fs::OpenOptions::new() .write(true) + .access_mode( + windows::Win32::Storage::FileSystem::FILE_GENERIC_WRITE.0 + | 0x0002_0000 + | windows::Win32::Storage::FileSystem::FILE_READ_ATTRIBUTES.0, + ) .custom_flags(FILE_FLAG_WRITE_THROUGH.0) .open(&pipe_path) { @@ -135,10 +143,23 @@ impl Logger { } } - /// Verify the pipe server process is running at High integrity level or above. + /// Verify the pipe belongs to the current user and its server runs at + /// High integrity level or above. fn verify_server_integrity(pipe_file: &std::fs::File) -> Result<(), String> { - // 1. Get the server PID from the pipe handle. + use windows::core::PWSTR; + use windows::Win32::Foundation::{CloseHandle, HANDLE}; + use windows::Win32::Security::{ + GetTokenInformation, TokenIntegrityLevel, TokenUser, TOKEN_MANDATORY_LABEL, + TOKEN_QUERY, TOKEN_USER, + }; + use windows::Win32::System::SystemServices::SECURITY_MANDATORY_HIGH_RID; + use windows::Win32::System::Threading::{ + QueryFullProcessImageNameW, PROCESS_NAME_FORMAT, + }; + let pipe_handle = HANDLE(pipe_file.as_raw_handle()); + + // Get the server PID from the pipe handle. let mut server_pid: u32 = 0; // SAFETY: pipe_handle is valid (from an open File); server_pid is a valid out pointer. unsafe { GetNamedPipeServerProcessId(pipe_handle, &mut server_pid) } @@ -150,6 +171,32 @@ impl Logger { unsafe { OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, server_pid) } .map_err(|e| format!("OpenProcess({server_pid}) failed: {e}"))?; + // Validate the executable identity as well as the PID. If the + // original server exits and its PID is reused, an unrelated + // process cannot satisfy this check. + let mut image = [0u16; 1024]; + let mut image_len = image.len() as u32; + unsafe { + QueryFullProcessImageNameW( + process, + PROCESS_NAME_FORMAT(0), + PWSTR(image.as_mut_ptr()), + &mut image_len, + ) + } + .map_err(|e| { + let _ = unsafe { CloseHandle(process) }; + format!("QueryFullProcessImageNameW failed: {e}") + })?; + let image_name = String::from_utf16_lossy(&image[..image_len as usize]); + let executable = image_name.rsplit('\\').next().unwrap_or(&image_name); + if !executable.eq_ignore_ascii_case("mxc-diagnostic-console.exe") { + let _ = unsafe { CloseHandle(process) }; + return Err(format!( + "unexpected diagnostic server executable: {executable}" + )); + } + // 3. Open the process token. let mut token = HANDLE::default(); // SAFETY: `process` is a valid handle from OpenProcess above; token is a valid out ptr. @@ -158,6 +205,41 @@ impl Logger { format!("OpenProcessToken failed: {e}") })?; + // Validate the server identity from its token rather than the + // pipe object's default owner, which may differ for elevated + // processes. + let expected_sid = match crate::diagnostic::current_user_sid() { + Some(sid) => sid, + None => { + let _ = unsafe { CloseHandle(token) }; + let _ = unsafe { CloseHandle(process) }; + return Err("current user SID could not be determined".to_string()); + } + }; + let mut user_buf = vec![0u8; 256]; + let mut user_returned: u32 = 0; + unsafe { + GetTokenInformation( + token, + TokenUser, + Some(user_buf.as_mut_ptr().cast()), + user_buf.len() as u32, + &mut user_returned, + ) + } + .map_err(|e| { + let _ = unsafe { CloseHandle(token) }; + let _ = unsafe { CloseHandle(process) }; + format!("GetTokenInformation(TokenUser) failed: {e}") + })?; + let server_user = unsafe { &*(user_buf.as_ptr() as *const TOKEN_USER) }; + let server_sid = unsafe { crate::string_util::sid_to_string(server_user.User.Sid.0) }; + if server_sid.as_deref() != Some(expected_sid.as_str()) { + let _ = unsafe { CloseHandle(token) }; + let _ = unsafe { CloseHandle(process) }; + return Err("server token user does not match current user".to_string()); + } + // 4. Query TokenIntegrityLevel. let mut buf = vec![0u8; 256]; let mut returned: u32 = 0; @@ -199,7 +281,6 @@ impl Logger { let _ = unsafe { CloseHandle(token) }; let _ = unsafe { CloseHandle(process) }; - use windows::Win32::System::SystemServices::SECURITY_MANDATORY_HIGH_RID; let high_rid = SECURITY_MANDATORY_HIGH_RID as u32; if integrity_rid >= high_rid { Ok(()) @@ -218,11 +299,7 @@ impl Logger { Mode::Buffer => self.buffer.push_str(msg), } if let Some(ref mut f) = self.file { - let secs = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .unwrap_or_default() - .as_secs(); - let _ = write!(f, "[{}] {}", secs, msg); + Self::write_timestamped_file(f, msg, false); } self.diag_accumulate(msg); } @@ -242,28 +319,115 @@ impl Logger { /// without duplicating it in the primary console/buffer output. pub fn log_diagnostic_line(&mut self, msg: &str) { if let Some(ref mut f) = self.file { - let secs = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .unwrap_or_default() - .as_secs(); - let _ = writeln!(f, "[{}] {}", secs, msg); + Self::write_timestamped_file(f, msg, true); } // log_line is a complete line -- flush any prior fragments, then this line. self.diag_accumulate(msg); self.diag_accumulate("\n"); } + fn write_timestamped_file(file: &mut File, msg: &str, terminate: bool) { + let secs = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + let lines: Vec<&str> = msg.split('\n').collect(); + for (index, line) in lines.iter().enumerate() { + let is_trailing_empty_line = index + 1 == lines.len() && line.is_empty(); + if is_trailing_empty_line && msg.ends_with('\n') { + break; + } + let _ = write!(file, "[{}] {}", secs, line.trim_end_matches('\r')); + if index + 1 < lines.len() || terminate { + let _ = file.write_all(b"\n"); + } + } + } + + /// Emit a structured [`AuditEvent`](crate::audit::AuditEvent) as a single + /// JSON line on the auxiliary diagnostic sinks only. + /// + /// Audit records are *internal diagnostics*, not user-facing output, so this + /// writes to the diagnostic sinks directly and deliberately never + /// touches the primary console/buffer sink — that channel is the SDK + /// caller's captured stdout / debug buffer, and adding to it would change + /// the observable output of every existing consumer. + /// + /// These are **local log lines, not ETW events**: there is no consent gate, + /// no administrative policy ceiling, and no config kill-switch. The record is + /// written when (and only when) a diagnostic sink is attached — a `--log-file` + /// path or the `MXC_DIAG_CONSOLE` named pipe — and is otherwise a cheap + /// no-op. + pub fn log_audit_event(&mut self, event: &crate::audit::AuditEvent) { + // Skip the render entirely when nothing would consume it. + if !self.has_diagnostic_sink() { + return; + } + let line = event.to_json_line(); + if let Some(ref mut f) = self.file { + Self::write_timestamped_file(f, &line, true); + } + self.diag_flush_audit(&line); + } + + /// Whether any auxiliary diagnostic sink is attached, i.e. whether + /// [`Logger::log_diagnostic_line`] would reach a consumer. + /// + /// Public so a caller can skip *building* an expensive record — the policy + /// hash serialises and digests the whole effective request — rather than + /// building it and having [`Logger::log_audit_event`] discard it. + pub fn has_diagnostic_sink(&self) -> bool { + #[cfg(target_os = "windows")] + { + self.file.is_some() || self.diag_pipe.is_some() + } + #[cfg(not(target_os = "windows"))] + { + self.file.is_some() + } + } + + /// Produce a detached logger that shares this logger's **diagnostic sinks + /// only** (the `--log-file` handle and, on Windows, the diagnostic-console + /// pipe), with an empty primary buffer. + /// + /// This exists for owners that must emit diagnostics from a context with no + /// caller-supplied logger — most importantly `Drop`, whose signature takes + /// no arguments, and the teardown paths reachable from it. Such call sites + /// currently build a throwaway `Logger::new(Mode::Buffer)` whose output is + /// discarded; holding a clone of the real sinks is what makes their records + /// observable. + /// + /// The handles are duplicated with [`std::fs::File::try_clone`], so the + /// clone writes to the same file / pipe as the original: + /// + /// * the log file is opened in append mode, so duplicated handles always + /// write at the end regardless of the shared file pointer; + /// * the diagnostic pipe is a `PIPE_TYPE_MESSAGE` pipe, so each + /// `write_all` is a discrete message that cannot interleave with another + /// handle's message. + /// + /// If a handle cannot be duplicated the corresponding sink is simply absent + /// from the clone — a failed duplication must never take down a run. + pub fn clone_diagnostic_sink(&self) -> Logger { + Logger { + mode: Mode::Buffer, + buffer: String::new(), + warnings: Vec::new(), + file: self.file.as_ref().and_then(|f| f.try_clone().ok()), + #[cfg(target_os = "windows")] + diag_pipe: self.diag_pipe.as_ref().and_then(|p| p.try_clone().ok()), + diag_line_buf: String::new(), + } + } + /// Emit a security warning through an always-visible channel and retain it /// for in-process callers. pub fn warning_line(&mut self, msg: &str) { eprintln!("{msg}"); self.warnings.push(msg.to_string()); if let Some(ref mut f) = self.file { - let secs = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .unwrap_or_default() - .as_secs(); - let _ = writeln!(f, "[{}] {}", secs, msg); + Self::write_timestamped_file(f, msg, true); } self.diag_accumulate(msg); self.diag_accumulate("\n"); @@ -324,6 +488,26 @@ impl Logger { let _ = line; } + /// Send a structured audit record with an envelope that cannot be + /// produced by the plain diagnostic-line path. + fn diag_flush_audit(&mut self, record: &str) { + #[cfg(target_os = "windows")] + if self.diag_pipe.is_some() { + if !self.diag_line_buf.is_empty() { + let pending = std::mem::take(&mut self.diag_line_buf); + self.diag_flush_line(&pending); + } + let envelope = format!("{{\"kind\":\"audit\",\"record\":{record}}}"); + if let Some(ref mut pipe) = self.diag_pipe { + if pipe.write_all(envelope.as_bytes()).is_err() || pipe.flush().is_err() { + self.diag_pipe = None; + } + } + } + #[cfg(not(target_os = "windows"))] + let _ = record; + } + /// Flush and close diagnostic sinks. pub fn close_diagnostics(&mut self) { // Flush any remaining buffered text as a final line. @@ -350,6 +534,7 @@ impl fmt::Write for Logger { #[cfg(test)] mod tests { use super::*; + use crate::audit::{AuditEvent, AuditEventName}; #[test] fn security_warnings_are_retained_outside_the_debug_buffer() { @@ -362,4 +547,96 @@ mod tests { assert_eq!(logger.take_warnings(), ["security warning"]); assert!(logger.warnings().is_empty()); } + + #[test] + fn audit_events_reach_the_file_sink_but_not_the_buffer() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("audit.log"); + let mut logger = Logger::new(Mode::Buffer); + logger.enable_file_sink(&path).expect("file sink"); + + logger.log_audit_event( + &AuditEvent::new(AuditEventName::ProcessExited) + .str("backend", "processcontainer") + .i64("exit_code", 3), + ); + // Drop the logger so the file handle is flushed/closed before reading. + drop(logger); + + let contents = std::fs::read_to_string(&path).expect("read log"); + let records: Vec = contents + .lines() + .map(|line| { + let json = line + .split_once("] ") + .expect("audit record must have a timestamp prefix") + .1; + serde_json::from_str(json).expect("audit record must be JSON") + }) + .collect(); + assert_eq!(records.len(), 1); + assert_eq!(records[0]["event"], "mxc.ProcessExited"); + assert_eq!(records[0]["exit_code"], 3); + // One record, one line. + assert_eq!(contents.lines().count(), 1, "got: {contents}"); + } + + #[test] + fn multiline_diagnostic_lines_are_individually_prefixed() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("diagnostic.log"); + let mut logger = Logger::new(Mode::Buffer); + logger.enable_file_sink(&path).expect("file sink"); + + logger.log_diagnostic_line("first\n{\"event\":\"spoof\"}"); + drop(logger); + + let contents = std::fs::read_to_string(&path).expect("read log"); + let lines: Vec<&str> = contents.lines().collect(); + assert_eq!(lines.len(), 2); + assert!(lines.iter().all(|line| line.starts_with('['))); + } + + #[test] + fn audit_events_are_a_no_op_without_a_diagnostic_sink() { + let mut logger = Logger::new(Mode::Buffer); + + logger.log_audit_event(&AuditEvent::new(AuditEventName::SandboxIdentity)); + + assert!(logger.get_buffer().is_empty()); + assert!(logger.warnings().is_empty()); + } + + #[test] + fn cloned_diagnostic_sink_shares_the_file_but_not_the_buffer() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("audit.log"); + let mut logger = Logger::new(Mode::Buffer); + logger.enable_file_sink(&path).expect("file sink"); + logger.log_line("primary line"); + + let mut detached = logger.clone_diagnostic_sink(); + detached.log_audit_event( + &AuditEvent::new(AuditEventName::SandboxTornDown).str("status", "success"), + ); + drop(detached); + drop(logger); + + let contents = std::fs::read_to_string(&path).expect("read log"); + assert!(contents.contains("primary line"), "got: {contents}"); + assert!( + contents.contains(r#"{"event":"mxc.SandboxTornDown","status":"success"}"#), + "got: {contents}" + ); + } + + #[test] + fn cloned_diagnostic_sink_without_sinks_is_inert() { + let logger = Logger::new(Mode::Buffer); + let mut detached = logger.clone_diagnostic_sink(); + + detached.log_audit_event(&AuditEvent::new(AuditEventName::ProcessExited)); + + assert!(detached.get_buffer().is_empty()); + } } diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index 1bc33be4f..f86301294 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -350,6 +350,17 @@ pub enum NetworkPolicy { Block, } +impl NetworkPolicy { + /// Canonical wire string, matching the JSON schema enum. Bounded + /// vocabulary for structured logs. + pub fn as_str(&self) -> &'static str { + match self { + Self::Allow => "allow", + Self::Block => "block", + } + } +} + impl From for NetworkPolicy { fn from(p: crate::wire::NetworkPolicy) -> Self { match p { @@ -368,6 +379,18 @@ pub enum NetworkEnforcementMode { Both, } +impl NetworkEnforcementMode { + /// Canonical wire string, matching the JSON schema enum. Bounded + /// vocabulary for structured logs. + pub fn as_str(&self) -> &'static str { + match self { + Self::Capabilities => "capabilities", + Self::Firewall => "firewall", + Self::Both => "both", + } + } +} + impl From for NetworkEnforcementMode { fn from(m: crate::wire::NetworkEnforcement) -> Self { match m { diff --git a/src/core/wxc_common/src/policy_identity.rs b/src/core/wxc_common/src/policy_identity.rs new file mode 100644 index 000000000..7da045f34 --- /dev/null +++ b/src/core/wxc_common/src/policy_identity.rs @@ -0,0 +1,641 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Canonical policy identity — a stable hash of the *effective* enforcement +//! policy for a run. +//! +//! [`policy_hash`] answers "which policy is this sandbox running under?" with a +//! value that is: +//! +//! * **stable** across formatting, key ordering, and base64-vs-file input, so +//! two runs of the same policy produce the same hash; +//! * **sensitive** to every enforcement-relevant field, so changing one +//! `readwritePaths` entry changes it; +//! * **insensitive** to things that do not change enforcement (telemetry +//! settings, dry-run, testing flags); +//! * **free of credential material**, so it cannot be used as a confirmation +//! oracle against a secret embedded in a config. +//! +//! The value is emitted on the `mxc.PolicyHash` audit record. +//! +//! # What is hashed +//! +//! An explicit **allow-list** projection of [`ExecutionRequest`], not the whole +//! struct. An allow-list is deliberate: a field added to the model later is +//! excluded until someone opts it in, which fails safe (a missing field +//! weakens sensitivity) rather than unsafe (an accidentally-hashed secret is a +//! disclosure risk that cannot be undone once hashes are in logs). +//! +//! To stop that safety property from silently rotting into a coverage gap, +//! [`policy_projection`] **exhaustively destructures** `ExecutionRequest` and +//! `ExperimentalConfig`. Adding a field to either is a compile error until it is +//! classified as hashed or explicitly excluded with a reason. +//! +//! # What is excluded, and why +//! +//! | Excluded | Reason | +//! |---|---| +//! | `script_code` | The command line is *what runs*, not the policy under which it runs; it also routinely embeds credentials (`curl -H "Authorization: …"`). | +//! | `env` | Environment variables are the classic secret carrier. | +//! | `experimental.telemetry` | Does not affect enforcement. | +//! | `experimental.isolation_session[.start].user` | Carries a WAM bearer token and a UPN. | +//! | `network_proxy.original_url` | A proxy URL can embed `user:password@`. The host and port *are* hashed. | +//! | `dry_run`, `testing_features_enabled` | Invocation modes, not policy. | +//! +//! `ContainerPolicy::network_proxy` is `#[serde(skip)]`, so the proxy's +//! credential-bearing URL cannot reach the hash through the blanket policy +//! serialization even by accident; the enforcement-relevant parts (enabled, +//! host, port) are added back explicitly. +//! +//! # Residual disclosure property (accepted, documented) +//! +//! The hash is deterministic and unkeyed, so it is a **confirmation oracle for +//! the fields it covers**: a reader who already knows every hashed field but one +//! can brute-force the remaining one. In practice that means someone holding the +//! log can test a guess at, say, a single `readwritePaths` entry — but only if +//! they already know the container id, working directory, timeout, capability +//! list, network policy, and every other path exactly. This is deliberately +//! accepted: +//! +//! * the alternative (a keyed digest) needs a machine-local secret whose +//! storage, rotation, and failure modes are out of scope for a local +//! diagnostic log; +//! * the fields covered are the operator's own policy, already visible to anyone +//! who can read the config the log sits next to; +//! * the genuinely sensitive inputs — command line, environment, tokens, proxy +//! userinfo — are excluded from the hash entirely, so no oracle exists for +//! them at any difficulty. +//! +//! Do not add a low-entropy secret to the projection without switching to a +//! keyed construction first. + +use serde_json::{Map, Value}; +use sha2::{Digest, Sha256}; + +use crate::models::{ExecutionRequest, ExperimentalConfig}; + +/// Algorithm tag prefixed to the hex digest, so the algorithm can change +/// without breaking a consumer that only does equality comparison. +const ALGORITHM_TAG: &str = "sha256"; + +/// Compute the canonical policy hash for `request`, formatted as +/// `"sha256:<64 lowercase hex chars>"`. +/// +/// Call this **after** every mutation that changes enforcement (CLI command +/// override, `--audit`'s permissive-learning-mode injection, capability +/// injection), so the hash describes what actually ran rather than what was +/// requested. +pub fn policy_hash(request: &ExecutionRequest) -> String { + let canonical = canonical_json(&policy_projection(request)); + let digest = Sha256::digest(canonical.as_bytes()); + let mut out = String::with_capacity(ALGORITHM_TAG.len() + 1 + digest.len() * 2); + out.push_str(ALGORITHM_TAG); + out.push(':'); + for byte in digest { + use std::fmt::Write as _; + let _ = write!(out, "{byte:02x}"); + } + out +} + +/// Build the allow-listed projection of the request that the hash covers. +/// +/// `ExecutionRequest` is **exhaustively destructured** (no `..`), so adding a +/// field to the model is a compile error here until it is either hashed or +/// bound to a named `_excluded_*` local with a reason. That is the tripwire +/// that keeps the allow-list from silently falling behind the model. +fn policy_projection(request: &ExecutionRequest) -> Value { + let ExecutionRequest { + schema_version, + container_id, + working_directory, + script_timeout, + containment, + lifecycle, + policy, + lxc_config, + seatbelt, + experimental_enabled, + experimental, + // --- deliberately excluded; see the module docs --- + // The command line is what runs, not the policy it runs under, and it + // routinely embeds credentials. + script_code: _excluded_command_line, + // Environment variables are the classic secret carrier. + env: _excluded_environment, + // Invocation modes, not policy. + dry_run: _excluded_dry_run, + testing_features_enabled: _excluded_testing_features, + } = request; + + let mut root = Map::new(); + + root.insert( + "schemaVersion".into(), + Value::String(schema_version.clone()), + ); + root.insert( + "containment".into(), + Value::String(containment.wire_name().to_string()), + ); + root.insert("containerId".into(), Value::String(container_id.clone())); + root.insert( + "workingDirectory".into(), + Value::String(working_directory.clone()), + ); + root.insert( + "scriptTimeout".into(), + Value::Number((*script_timeout).into()), + ); + root.insert( + "experimentalEnabled".into(), + Value::Bool(*experimental_enabled), + ); + root.insert( + "lifecycle".into(), + serde_json::to_value(lifecycle).unwrap_or(Value::Null), + ); + // `ContainerPolicy` serialization already omits `network_proxy` + // (`#[serde(skip)]`), so no credential-bearing proxy URL can reach the hash + // through this line. + root.insert( + "policy".into(), + serde_json::to_value(policy).unwrap_or(Value::Null), + ); + root.insert("proxy".into(), proxy_projection(request)); + root.insert( + "lxc".into(), + serde_json::to_value(lxc_config).unwrap_or(Value::Null), + ); + root.insert( + "seatbelt".into(), + serde_json::to_value(seatbelt).unwrap_or(Value::Null), + ); + root.insert("experimental".into(), experimental_projection(experimental)); + + Value::Object(root) +} + +/// The enforcement-relevant, non-credential parts of the experimental block. +/// +/// These matter: for `windows_sandbox`, `wslc`, and `isolation_session` the +/// experimental section carries the sandbox's **entire** filesystem / network / +/// resource policy. Omitting it wholesale (the first cut of this module did) +/// would have made two materially different policies hash identically on those +/// backends. +/// +/// `ExperimentalConfig` is exhaustively destructured for the same tripwire +/// reason as [`policy_projection`]. +fn experimental_projection(experimental: &ExperimentalConfig) -> Value { + let ExperimentalConfig { + windows_sandbox, + wslc, + isolation_session, + // A placeholder feature with no enforcement effect. + test: _excluded_test_feature, + // Telemetry settings do not affect enforcement. + telemetry: _excluded_telemetry, + } = experimental; + + let mut out = Map::new(); + out.insert( + "windows_sandbox".into(), + serde_json::to_value(windows_sandbox).unwrap_or(Value::Null), + ); + out.insert( + "wslc".into(), + serde_json::to_value(wslc).unwrap_or(Value::Null), + ); + // `IsolationSessionConfig::user` carries a WAM bearer token and a UPN, so + // the section is serialized and then the credential key is stripped rather + // than being passed through. + let mut iso = serde_json::to_value(isolation_session).unwrap_or(Value::Null); + strip_keys(&mut iso, &["user"]); + out.insert("isolation_session".into(), iso); + + Value::Object(out) +} + +/// Recursively remove every object entry whose key is in `keys`. +/// +/// Used to excise credential-bearing sub-objects from an otherwise +/// blanket-serialized section, so the section's enforcement-relevant fields can +/// still be hashed. +fn strip_keys(value: &mut Value, keys: &[&str]) { + match value { + Value::Object(map) => { + for key in keys { + map.remove(*key); + } + for child in map.values_mut() { + strip_keys(child, keys); + } + } + Value::Array(items) => { + for item in items { + strip_keys(item, keys); + } + } + _ => {} + } +} + +/// The enforcement-relevant, non-credential parts of the proxy configuration: +/// whether a proxy is in force, its host, and its port. The original URL is +/// deliberately dropped because it can carry `user:password@` userinfo. +fn proxy_projection(request: &ExecutionRequest) -> Value { + let proxy = &request.policy.network_proxy; + let mut out = Map::new(); + out.insert("enabled".into(), Value::Bool(proxy.is_enabled())); + out.insert( + "builtinTestServer".into(), + Value::Bool(proxy.builtin_test_server), + ); + match &proxy.address { + Some(addr) => { + out.insert("address".into(), Value::String(addr.address.clone())); + out.insert("port".into(), Value::Number(addr.port.into())); + } + None => { + out.insert("address".into(), Value::Null); + out.insert("port".into(), Value::Null); + } + } + Value::Object(out) +} + +/// Render `value` as canonical JSON: object keys sorted lexicographically at +/// every depth, array order preserved (array order is semantically meaningful +/// for path lists), and no insignificant whitespace. +/// +/// Only the *container* kinds are walked by hand, and only to pin key ordering: +/// `serde_json::Map` is a `BTreeMap` unless the `preserve_order` feature is on, +/// so its iteration order is usually already sorted — but a feature flag flipped +/// by an unrelated crate in the dependency graph must not silently change every +/// hash MXC has ever emitted. Scalars are handed straight to `serde_json`, so +/// string escaping and number formatting are not reimplemented here. +fn canonical_json(value: &Value) -> String { + let mut out = String::new(); + write_canonical(value, &mut out); + out +} + +fn write_canonical(value: &Value, out: &mut String) { + match value { + Value::Object(map) => { + let mut keys: Vec<&String> = map.keys().collect(); + keys.sort_unstable(); + out.push('{'); + for (i, key) in keys.iter().enumerate() { + if i > 0 { + out.push(','); + } + out.push_str(&scalar_json(&Value::String((*key).clone()))); + out.push(':'); + // A key present in `keys` is by construction present in `map`. + if let Some(child) = map.get(*key) { + write_canonical(child, out); + } + } + out.push('}'); + } + Value::Array(items) => { + out.push('['); + for (i, item) in items.iter().enumerate() { + if i > 0 { + out.push(','); + } + write_canonical(item, out); + } + out.push(']'); + } + // Null / Bool / Number / String have no ordering concern, so there is + // nothing to hand-roll: `serde_json` already renders them canonically. + scalar => out.push_str(&scalar_json(scalar)), + } +} + +/// Render a non-container `Value`. Serializing a scalar `Value` to a `String` +/// is infallible (the only failure mode of the `String` serializer is an I/O +/// error, which cannot occur for an in-memory buffer). +fn scalar_json(value: &Value) -> String { + serde_json::to_string(value).expect("serializing a scalar Value to JSON is infallible") +} + +/// Render a sandbox identity so it is safe to write to a diagnostic log file. +/// +/// Two distinct hazards are handled: +/// +/// 1. **UPN-shaped identities.** For `isolation_session` Entra sandboxes the +/// `provisionId` **is the user's UPN** (`state_aware.rs::provision` sets +/// `provision_id = user.upn`). A UPN must never be written to a file that is +/// routinely attached to a bug report. +/// 2. **Caller-supplied identities.** On the ProcessContainer path the identity +/// is the AppContainer profile name, i.e. the config's `containerId`. That is +/// a config value and is handled by [`crate::audit::sanitize_identity`]. +/// +/// For (1) this function emits the bounded marker `"entra-upn"` and **no +/// account-derived value at all**. +/// +/// > A truncated SHA-256 of a UPN was considered and rejected. A UPN is +/// > low-entropy and enumerable within a tenant, so an unsalted digest is +/// > trivially reversed by dictionary attack — it is pseudonymisation, not +/// > redaction, and would have made the log's privacy posture look stronger than +/// > it is. A keyed HMAC would work but needs a machine-local secret whose own +/// > storage, rotation, and failure modes are out of scope here. The cost of the +/// > marker is that Entra sandboxes have **no MXC-side join key** in the local +/// > log; the OS-side `Microsoft.Windows.IsolationSession` records still carry +/// > the real `provisionId` for anyone who legitimately needs to correlate. +pub fn redact_identity(identity: &str) -> String { + if is_upn_shaped(identity) { + return ENTRA_UPN_MARKER.to_string(); + } + crate::audit::sanitize_identity(identity).to_string() +} + +/// Marker written in place of a UPN-derived identity. Bounded and constant, so +/// it discloses only the *kind* of identity, never the account. +pub const ENTRA_UPN_MARKER: &str = "entra-upn"; + +/// Whether `identity` looks like a UPN (or a `:` sandbox id). +/// +/// An `@` is the discriminator: none of the identity shapes MXC mints itself +/// (`sandbox-`, `wxc-`, `wsb:`, `iso:wxc-`) contains one, +/// and every UPN does. +fn is_upn_shaped(identity: &str) -> bool { + identity.contains('@') +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::{ContainmentBackend, ProxyAddress}; + + fn request() -> ExecutionRequest { + let mut r = ExecutionRequest { + schema_version: "0.7.0-alpha".to_string(), + container_id: "test".to_string(), + script_code: "echo hello".to_string(), + working_directory: "C:\\work".to_string(), + script_timeout: 30, + containment: ContainmentBackend::ProcessContainer, + ..Default::default() + }; + r.policy.readwrite_paths.push("C:\\tmp".to_string()); + r.policy.readonly_paths.push("C:\\ro".to_string()); + r + } + + #[test] + fn hash_is_prefixed_and_hex() { + let h = policy_hash(&request()); + let Some(hex) = h.strip_prefix("sha256:") else { + panic!("missing algorithm tag: {h}"); + }; + assert_eq!(hex.len(), 64, "got: {h}"); + assert!( + hex.chars() + .all(|c| c.is_ascii_digit() || ('a'..='f').contains(&c)), + "not lowercase hex: {h}" + ); + } + + #[test] + fn identical_policies_hash_identically() { + assert_eq!(policy_hash(&request()), policy_hash(&request())); + } + + #[test] + fn changing_a_readwrite_path_changes_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.policy.readwrite_paths[0] = "C:\\other".to_string(); + assert_ne!(baseline, policy_hash(&changed)); + } + + #[test] + fn adding_a_denied_path_changes_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.policy.denied_paths.push("C:\\secret".to_string()); + assert_ne!(baseline, policy_hash(&changed)); + } + + #[test] + fn changing_the_network_policy_changes_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.policy.default_network_policy = crate::models::NetworkPolicy::Allow; + assert_ne!(baseline, policy_hash(&changed)); + } + + #[test] + fn changing_the_containment_backend_changes_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.containment = ContainmentBackend::Lxc; + assert_ne!(baseline, policy_hash(&changed)); + } + + #[test] + fn changing_the_proxy_port_changes_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.policy.network_proxy.address = + Some(ProxyAddress::new("localhost".to_string(), 8080)); + let with_8080 = policy_hash(&changed); + assert_ne!(baseline, with_8080); + + changed.policy.network_proxy.address = + Some(ProxyAddress::new("localhost".to_string(), 9090)); + assert_ne!(with_8080, policy_hash(&changed)); + } + + #[test] + fn telemetry_settings_do_not_change_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.experimental.telemetry = Some(crate::models::TelemetryConfig { + enabled: Some(true), + }); + assert_eq!( + baseline, + policy_hash(&changed), + "telemetry does not affect enforcement and must not perturb the policy identity" + ); + } + + #[test] + fn credential_bearing_fields_do_not_change_the_hash() { + let baseline = policy_hash(&request()); + + // A proxy URL that embeds userinfo must not become a confirmation + // oracle: only host + port are hashed. + let mut with_userinfo = request(); + with_userinfo.policy.network_proxy.address = Some(ProxyAddress::from_url( + "http://user:hunter2@localhost:8080", + "localhost".to_string(), + 8080, + )); + let mut without_userinfo = request(); + without_userinfo.policy.network_proxy.address = + Some(ProxyAddress::new("localhost".to_string(), 8080)); + assert_eq!( + policy_hash(&with_userinfo), + policy_hash(&without_userinfo), + "the proxy URL's userinfo must not reach the hash" + ); + + // Environment variables and the command line routinely carry secrets. + let mut with_secrets = request(); + with_secrets.env.push("API_KEY=hunter2".to_string()); + with_secrets.script_code = "curl -H 'Authorization: Bearer hunter2'".to_string(); + assert_eq!( + baseline, + policy_hash(&with_secrets), + "env and command line are excluded from the policy identity" + ); + } + + #[test] + fn invocation_modes_do_not_change_the_hash() { + let baseline = policy_hash(&request()); + let mut changed = request(); + changed.dry_run = true; + changed.testing_features_enabled = true; + assert_eq!(baseline, policy_hash(&changed)); + } + + #[test] + fn canonical_json_sorts_keys_at_every_depth() { + let value: Value = + serde_json::from_str(r#"{"b":1,"a":{"z":[3,1,2],"y":true}}"#).expect("valid JSON"); + assert_eq!( + canonical_json(&value), + r#"{"a":{"y":true,"z":[3,1,2]},"b":1}"# + ); + } + + #[test] + fn canonical_json_preserves_array_order() { + // Path lists are order-bearing in the policy, so reordering them is a + // real change and must produce a different canonical form. + let a: Value = serde_json::from_str(r#"["x","y"]"#).expect("valid JSON"); + let b: Value = serde_json::from_str(r#"["y","x"]"#).expect("valid JSON"); + assert_ne!(canonical_json(&a), canonical_json(&b)); + } + + #[test] + fn canonical_json_escapes_strings() { + let value = Value::String("quote\" and \\ backslash".to_string()); + let rendered = canonical_json(&value); + assert_eq!(rendered, r#""quote\" and \\ backslash""#); + // Round-trips, so the canonical form is still parseable JSON. + let reparsed: Value = serde_json::from_str(&rendered).expect("valid JSON"); + assert_eq!(reparsed, value); + } + + #[test] + fn opaque_identities_pass_through_unredacted() { + for id in [ + "sandbox-a3f1c8e40029bd17", + "wxc-abcd1234", + "iso:wxc-abcd1234", + "wsb:deadbeef", + "CLI", + "", + ] { + assert_eq!(redact_identity(id), id); + } + } + + #[test] + fn upn_shaped_identities_never_reach_the_log() { + for upn in [ + "alice@contoso.com", + "iso:alice@contoso.com", + "BOB@Contoso.OnMicrosoft.com", + ] { + let redacted = redact_identity(upn); + assert_eq!(redacted, ENTRA_UPN_MARKER, "got: {redacted}"); + } + } + + /// The marker must be constant, so it cannot be reversed by dictionary + /// attack the way a truncated unsalted digest of a low-entropy UPN could. + #[test] + fn the_upn_marker_carries_no_account_derived_entropy() { + assert_eq!( + redact_identity("alice@contoso.com"), + redact_identity("bob@fabrikam.com"), + "distinct accounts must render identically; any per-account value \ + would be a reversible pseudonym" + ); + } + + /// A caller-supplied `containerId` becomes the sandbox identity on the + /// ProcessContainer path, so non-opaque values must be redacted rather than + /// echoed into a record. + #[test] + fn non_opaque_identities_are_redacted() { + assert_eq!( + redact_identity("C:\\Users\\alice\\ticket-1234"), + crate::audit::REDACTED_IDENTITY + ); + } + + #[test] + fn experimental_backend_policy_changes_the_hash() { + // The experimental block carries the ENTIRE enforcement policy for + // windows_sandbox / wslc / isolation_session. Omitting it would make two + // materially different policies hash identically on those backends. + let mut baseline = request(); + baseline.containment = ContainmentBackend::Wslc; + let before = policy_hash(&baseline); + + let mut changed = baseline.clone(); + changed.experimental.wslc = Some(crate::models::WslcConfig { + image: "python:3.12".to_string(), + gpu: true, + ..Default::default() + }); + assert_ne!(before, policy_hash(&changed)); + + let mut more = changed.clone(); + if let Some(cfg) = more.experimental.wslc.as_mut() { + cfg.memory_mb = Some(4096); + } + assert_ne!(policy_hash(&changed), policy_hash(&more)); + } + + #[test] + fn isolation_session_credentials_do_not_change_the_hash() { + let mut baseline = request(); + baseline.containment = ContainmentBackend::IsolationSession; + baseline.experimental.isolation_session = + Some(crate::models::IsolationSessionConfig::default()); + let before = policy_hash(&baseline); + + let mut with_user = baseline.clone(); + if let Some(cfg) = with_user.experimental.isolation_session.as_mut() { + cfg.user = Some(crate::models::IsolationSessionUser { + upn: "alice@contoso.com".to_string(), + wam_token: "super-secret-bearer-token".to_string(), + }); + } + assert_eq!( + before, + policy_hash(&with_user), + "the WAM token and UPN must be stripped before hashing" + ); + } + + #[test] + fn strip_keys_removes_nested_credential_objects() { + let mut value: serde_json::Value = + serde_json::from_str(r#"{"a":{"user":{"wamToken":"x"},"keep":1},"b":[{"user":2}]}"#) + .expect("valid JSON"); + strip_keys(&mut value, &["user"]); + assert_eq!(canonical_json(&value), r#"{"a":{"keep":1},"b":[{}]}"#); + } +} diff --git a/src/tools/mxc_diagnostic_console/src/main.rs b/src/tools/mxc_diagnostic_console/src/main.rs index a2fb6d38e..5cf100d2d 100644 --- a/src/tools/mxc_diagnostic_console/src/main.rs +++ b/src/tools/mxc_diagnostic_console/src/main.rs @@ -9,7 +9,8 @@ //! Usage: //! mxc-diagnostic-console.exe //! -//! Then run `wxc-exec.exe` with `MXC_DIAG_CONSOLE=1` (or registry key). +//! Set the same high-entropy `MXC_DIAG_PIPE_TOKEN` for the console and +//! `wxc-exec.exe`, then enable `MXC_DIAG_CONSOLE=1` (or the registry key). mod etw; @@ -22,10 +23,12 @@ use std::time::{Duration, SystemTime}; use clap::Parser; -use windows::Win32::Foundation::{CloseHandle, HANDLE, INVALID_HANDLE_VALUE}; +use windows::Win32::Foundation::{CloseHandle, LocalFree, HANDLE, HLOCAL, INVALID_HANDLE_VALUE}; +use windows::Win32::Security::Authorization::{ + ConvertStringSecurityDescriptorToSecurityDescriptorW, SDDL_REVISION_1, +}; use windows::Win32::Security::{ - InitializeSecurityDescriptor, SetSecurityDescriptorDacl, PSECURITY_DESCRIPTOR, - SECURITY_ATTRIBUTES, SECURITY_DESCRIPTOR, TOKEN_ELEVATION, TOKEN_QUERY, + PSECURITY_DESCRIPTOR, SECURITY_ATTRIBUTES, TOKEN_ELEVATION, TOKEN_QUERY, }; use windows::Win32::Storage::FileSystem::FILE_FLAG_FIRST_PIPE_INSTANCE; use windows::Win32::System::Pipes::{ @@ -182,6 +185,13 @@ fn main() { }; // Compute the per-user pipe name (includes current user's SID). + if wxc_common::diagnostic::diagnostic_pipe_token().is_none() { + eprintln!( + "[error] Set MXC_DIAG_PIPE_TOKEN to a high-entropy token in the \ + environment shared with wxc-exec before starting the diagnostic console." + ); + std::process::exit(1); + } let pipe_name = wxc_common::diagnostic::diagnostic_pipe_name(); // Enable ANSI escape codes on Windows console. @@ -361,19 +371,23 @@ fn create_pipe_instance(pipe_name: &str, first: bool) -> Result let name_wide: Vec = pipe_name.encode_utf16().chain(std::iter::once(0)).collect(); use windows::Win32::Storage::FileSystem::FILE_FLAGS_AND_ATTRIBUTES; - // Create a security descriptor with a NULL DACL (allows all access). - // This is required so that medium/low integrity clients (e.g. sandboxed processes) - // can connect to the pipe when the server is running elevated. - let mut sd = SECURITY_DESCRIPTOR::default(); - let psd = PSECURITY_DESCRIPTOR(std::ptr::addr_of_mut!(sd).cast()); - // SAFETY: `psd` points to a valid stack-allocated SECURITY_DESCRIPTOR; - // revision 1 is the only valid value. SetSecurityDescriptorDacl with None - // sets a NULL DACL (allow all). + let sid = wxc_common::diagnostic::current_user_sid() + .ok_or_else(|| "could not determine current user SID".to_string())?; + let sddl = format!("D:(A;;GA;;;{sid})S:(ML;;NW;;;LW)"); + let sddl_wide: Vec = sddl.encode_utf16().chain(std::iter::once(0)).collect(); + let mut psd = PSECURITY_DESCRIPTOR::default(); + // SAFETY: the SDDL buffer is null terminated and remains alive for the call. unsafe { - InitializeSecurityDescriptor(psd, 1) - .map_err(|e| format!("InitializeSecurityDescriptor: {e}"))?; - SetSecurityDescriptorDacl(psd, true, None, false) - .map_err(|e| format!("SetSecurityDescriptorDacl: {e}"))?; + ConvertStringSecurityDescriptorToSecurityDescriptorW( + PCWSTR(sddl_wide.as_ptr()), + SDDL_REVISION_1, + &mut psd, + None, + ) + .map_err(|e| format!("ConvertStringSecurityDescriptorToSecurityDescriptorW: {e}"))?; + } + if psd.0.is_null() { + return Err("security descriptor conversion returned NULL".to_string()); } let sa = SECURITY_ATTRIBUTES { @@ -403,14 +417,29 @@ fn create_pipe_instance(pipe_name: &str, first: bool) -> Result ) }; - if handle == INVALID_HANDLE_VALUE { - return Err(format!( + let result = if handle == INVALID_HANDLE_VALUE { + Err(format!( "CreateNamedPipeW failed: {}", std::io::Error::last_os_error() - )); + )) + } else { + Ok(handle) + }; + unsafe { + let _ = LocalFree(Some(HLOCAL(psd.0))); } - Ok(handle) + result +} + +fn sanitize_display_text(text: &str) -> String { + text.chars() + .flat_map(|ch| match ch { + '\t' | '\n' => Some(ch).into_iter().collect::>(), + '\u{20}'..='\u{7e}' => vec![ch], + _ => format!("\\x{:02X}", ch as u32 & 0xff).chars().collect(), + }) + .collect() } /// Get the client process ID from a connected pipe handle. @@ -450,11 +479,14 @@ fn client_reader(pipe: HANDLE, pid: u32, tx: mpsc::Sender) { continue; } if let Some(msg) = parse_log_message(segment) { - let _ = tx.send(DisplayEvent::Message { pid, text: msg }); + let _ = tx.send(DisplayEvent::Message { + pid, + text: sanitize_display_text(&msg), + }); } else { let _ = tx.send(DisplayEvent::Message { pid, - text: segment.to_string(), + text: sanitize_display_text(segment), }); } } @@ -465,7 +497,7 @@ fn client_reader(pipe: HANDLE, pid: u32, tx: mpsc::Sender) { let partial = String::from_utf8_lossy(&buf).to_string(); let _ = tx.send(DisplayEvent::Message { pid, - text: format!("{partial}... (truncated)"), + text: sanitize_display_text(&format!("{partial}... (truncated)")), }); continue; } @@ -492,9 +524,13 @@ fn client_reader(pipe: HANDLE, pid: u32, tx: mpsc::Sender) { /// Parse a JSON log message envelope and extract the text. /// -/// Expected format: `{"msg": "the log text"}` +/// Expected formats are `{"msg": "the log text"}` and +/// `{"kind":"audit","record":{...}}`. fn parse_log_message(json: &str) -> Option { let v: serde_json::Value = serde_json::from_str(json).ok()?; + if v.get("kind").and_then(|k| k.as_str()) == Some("audit") { + return v.get("record").map(ToString::to_string); + } v.get("msg").and_then(|m| m.as_str()).map(|s| s.to_string()) } From 49a90e1bdf8f24f4a8013a934bdd96627fc14587 Mon Sep 17 00:00:00 2001 From: RamonArjona4 Date: Tue, 4 Aug 2026 18:04:25 -0700 Subject: [PATCH 2/4] fix: address Copilot diagnostics review Harden audit identity handling, report actual network and teardown outcomes, preserve diagnostic sinks across state-aware execution, and correct lifecycle, hashing, identity, and Unicode behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7effbd2a-1c76-4c8b-bf1b-a33701a0dc1c --- .../common/src/appcontainer_runner.rs | 144 +++++++++++++--- .../common/src/network_manager.rs | 159 ++++++++++++++++-- .../isolation_session/common/src/manager.rs | 33 +++- .../common/src/state_aware.rs | 8 +- src/core/wxc/src/main.rs | 9 + src/core/wxc_common/src/audit.rs | 50 +++++- src/core/wxc_common/src/logger.rs | 65 +++++++ src/core/wxc_common/src/policy_identity.rs | 77 ++++++++- src/tools/mxc_diagnostic_console/src/main.rs | 89 +++++++++- 9 files changed, 574 insertions(+), 60 deletions(-) diff --git a/src/backends/appcontainer/common/src/appcontainer_runner.rs b/src/backends/appcontainer/common/src/appcontainer_runner.rs index c6d2635d0..248f671d4 100644 --- a/src/backends/appcontainer/common/src/appcontainer_runner.rs +++ b/src/backends/appcontainer/common/src/appcontainer_runner.rs @@ -1303,7 +1303,21 @@ impl AppContainerScriptRunner { // success and failure arms — "the policy I tried to install and failed" // is as auditable a fact as a successful one. if logger.has_diagnostic_sink() { + // `firewall_applied` reflects the actual apply outcome (rules + // installed), NOT the policy plan. Deriving the audit field from + // the plan would report `firewall_applied=true` on a + // partially-failed install with zero rules on-device — an operator + // reading the record would see a green enforcement claim on a run + // whose enforcement never landed. + let firewall_applied = network_manager.firewall_applied(); + // Aggregate status: a proxy-only success with no firewall step + // remains a success; a firewall-plan run whose install failed is a + // failure regardless of `network_result` (the caller may have kept + // the run going after a proxy-only failure). Both terms must be + // green for the record to be green. let plan = NetworkManager::describe_policy(&request.policy); + let firewall_ok = !plan.rules_will_be_installed + || matches!(network_manager.firewall_apply_ok(), Some(true)); let record = AuditEvent::new(AuditEventName::NetworkPolicyApplied) .str("backend", ContainmentBackend::ProcessContainer.wire_name()) .str("identity", sanitize_identity(&self.app_container_name)) @@ -1327,10 +1341,10 @@ impl AppContainerScriptRunner { "firewall_rules_created", network_manager.rule_count() as u64, ) - .bool("firewall_applied", plan.rules_will_be_installed) + .bool("firewall_applied", firewall_applied) .str( "status", - if network_result.is_ok() { + if network_result.is_ok() && firewall_ok { OperationStatus::Success.as_str() } else { OperationStatus::Failure.as_str() @@ -1361,22 +1375,38 @@ impl AppContainerScriptRunner { /// child process was created, so cleanup failures must remain observable. fn teardown(&self, prepared: &mut Prepared, preserve_policy: bool, logger: &mut Logger) { let network = prepared.network_manager.stop_all(!preserve_policy, logger); - if self.filesystem_mode == FilesystemMode::Bfs + // BFS removal is only attempted when we asked for it; the manager + // reports back whether the removal actually landed. Capture the + // per-call result so a requested-but-failed removal can downgrade the + // aggregate status to `failure`, instead of being silently discarded + // and reported as a clean teardown. + let bfs_requested = self.filesystem_mode == FilesystemMode::Bfs && prepared.bfs_manager.configured() - && !preserve_policy - { - prepared.bfs_manager.remove_configuration(logger); - } + && !preserve_policy; + let bfs_removed = if bfs_requested { + prepared.bfs_manager.remove_configuration(logger) + } else { + false + }; if logger.has_diagnostic_sink() { - let (status, skip_reason) = - appcontainer_teardown_status(preserve_policy, network.firewall_removal_ok); + let (status, skip_reason) = appcontainer_teardown_status_with_bfs( + preserve_policy, + network.firewall_removal_ok, + bfs_requested, + bfs_removed, + ); let mut record = AuditEvent::new(AuditEventName::SandboxTornDown) .str("backend", ContainmentBackend::ProcessContainer.wire_name()) - .str("identity", &self.app_container_name) + // The AppContainer profile name is the caller's `containerId`, + // i.e. a config value — sanitize identically to the successful + // teardown path so caller-supplied non-opaque ids can't reach a + // record from the early-failure teardown either. + .str("identity", sanitize_identity(&self.app_container_name)) .str("tier", self.tier_str()) .str("status", status.as_str()) .u64("firewall_rules_removed", network.rules_removed as u64) .bool("firewall_removal_ok", network.firewall_removal_ok) + .bool("bfs_removed", bfs_removed) .bool("proxy_stopped", network.proxy_stopped) .bool("preserve_policy", preserve_policy) .bool("container_released", false); @@ -1584,18 +1614,28 @@ impl AppContainerSandboxProcess { .prepared .network_manager .stop_all(!self.preserve_policy, &mut logger); - let bfs_removed = self.filesystem_mode == FilesystemMode::Bfs + // Whether BFS removal was actually requested this teardown, vs whether + // it landed. The two used to be conflated: `bfs_removed` was inferred + // from `filesystem_mode` alone, so a failed `remove_configuration` + // still recorded `bfs_removed=true` and did not affect `status`. + let bfs_requested = self.filesystem_mode == FilesystemMode::Bfs && self.prepared.bfs_manager.configured() && !self.preserve_policy; - if bfs_removed { - self.prepared.bfs_manager.remove_configuration(&mut logger); - } + let bfs_removed = if bfs_requested { + self.prepared.bfs_manager.remove_configuration(&mut logger) + } else { + false + }; if !self.audit_enabled() { return; } - let (status, skip_reason) = - appcontainer_teardown_status(self.preserve_policy, network.firewall_removal_ok); + let (status, skip_reason) = appcontainer_teardown_status_with_bfs( + self.preserve_policy, + network.firewall_removal_ok, + bfs_requested, + bfs_removed, + ); let mut record = self .audit(AuditEventName::SandboxTornDown) .str("status", status.as_str()) @@ -1623,16 +1663,33 @@ impl AppContainerSandboxProcess { /// `preserve_policy` is a deliberate request to leave enforcement in place, so /// it is `skipped` rather than a success or a failure — the record must not /// claim a release that was never attempted. -fn appcontainer_teardown_status( +/// Extended teardown-status mapping that additionally accounts for the BFS +/// removal outcome. A requested-but-failed BFS removal downgrades the aggregate +/// status to `failure`, so the audit stream cannot show a green +/// `SandboxTornDown` on a run that left the BFS configuration behind. +/// +/// `bfs_requested` and `bfs_removed` are independent facts: `bfs_removed` is +/// meaningful only when `bfs_requested` is true. A caller that did not request +/// BFS removal (either not the BFS tier, or `preserve_policy`) passes +/// `bfs_requested = false` and the BFS outcome does not affect the status. +/// +/// `preserve_policy` is a deliberate request to leave enforcement in place, so +/// it is `skipped` rather than a success or a failure — the record must not +/// claim a release that was never attempted. +fn appcontainer_teardown_status_with_bfs( preserve_policy: bool, firewall_removal_ok: bool, + bfs_requested: bool, + bfs_removed: bool, ) -> (TeardownStatus, Option) { if preserve_policy { - ( + return ( TeardownStatus::Skipped, Some(TeardownSkipReason::PreservePolicy), - ) - } else if firewall_removal_ok { + ); + } + let bfs_ok = !bfs_requested || bfs_removed; + if firewall_removal_ok && bfs_ok { (TeardownStatus::Success, None) } else { (TeardownStatus::Failure, None) @@ -1782,13 +1839,16 @@ mod tests { /// implies a release the code never attempted, and never as a `failure`. #[test] fn teardown_status_reports_preserve_policy_as_skipped() { - use super::appcontainer_teardown_status; + use super::appcontainer_teardown_status_with_bfs; use wxc_common::audit::{TeardownSkipReason, TeardownStatus}; for firewall_ok in [true, false] { - let (status, reason) = appcontainer_teardown_status(true, firewall_ok); - assert_eq!(status, TeardownStatus::Skipped); - assert_eq!(reason, Some(TeardownSkipReason::PreservePolicy)); + for (bfs_req, bfs_ok) in [(false, false), (true, true), (true, false)] { + let (status, reason) = + appcontainer_teardown_status_with_bfs(true, firewall_ok, bfs_req, bfs_ok); + assert_eq!(status, TeardownStatus::Skipped); + assert_eq!(reason, Some(TeardownSkipReason::PreservePolicy)); + } } } @@ -1797,19 +1857,49 @@ mod tests { /// its `Result`. #[test] fn teardown_status_distinguishes_failed_firewall_removal() { - use super::appcontainer_teardown_status; + use super::appcontainer_teardown_status_with_bfs; use wxc_common::audit::TeardownStatus; assert_eq!( - appcontainer_teardown_status(false, true), + appcontainer_teardown_status_with_bfs(false, true, false, false), (TeardownStatus::Success, None) ); assert_eq!( - appcontainer_teardown_status(false, false), + appcontainer_teardown_status_with_bfs(false, false, false, false), (TeardownStatus::Failure, None) ); } + /// A failed BFS removal must downgrade the aggregate status to `failure`, + /// so a reader cannot look at a green `SandboxTornDown` on a run whose BFS + /// configuration was still installed at teardown. Firewall-clean case: + /// only the BFS outcome makes the difference. + #[test] + fn teardown_status_reports_failed_bfs_removal_as_failure() { + use super::appcontainer_teardown_status_with_bfs; + use wxc_common::audit::TeardownStatus; + + // Requested + landed → success. + assert_eq!( + appcontainer_teardown_status_with_bfs(false, true, true, true), + (TeardownStatus::Success, None) + ); + // Requested + failed → failure, regardless of the firewall outcome. + assert_eq!( + appcontainer_teardown_status_with_bfs(false, true, true, false), + (TeardownStatus::Failure, None) + ); + assert_eq!( + appcontainer_teardown_status_with_bfs(false, false, true, false), + (TeardownStatus::Failure, None) + ); + // Not requested → does not affect status. + assert_eq!( + appcontainer_teardown_status_with_bfs(false, true, false, false), + (TeardownStatus::Success, None) + ); + } + #[test] fn filesystem_mode_maps_to_exactly_one_tier() { use super::FilesystemMode; diff --git a/src/backends/appcontainer/common/src/network_manager.rs b/src/backends/appcontainer/common/src/network_manager.rs index 901dd7d82..1970bfba2 100644 --- a/src/backends/appcontainer/common/src/network_manager.rs +++ b/src/backends/appcontainer/common/src/network_manager.rs @@ -87,6 +87,14 @@ pub struct NetworkManager { created_rule_names: Vec, wsa_initialized: bool, proxy_coordinator: ProxyCoordinator, + /// The result of the most recent `apply_firewall_rules` call. `None` when + /// apply has never been attempted (e.g. `start` returned early on a proxy + /// failure). `Some(true)` when apply succeeded — either all requested rules + /// were installed, or the policy required no rules at all. `Some(false)` + /// when apply failed. Consulted by the caller when building the + /// `NetworkPolicyApplied` audit record so `firewall_applied` reflects the + /// actual apply outcome rather than the policy *plan*. + firewall_apply_ok: Option, } /// What [`NetworkManager::stop_all`] actually released. @@ -106,6 +114,34 @@ pub struct NetworkTeardown { pub proxy_stopped: bool, } +/// What [`NetworkManager::remove_firewall_rules`] actually released. +/// +/// Windows Firewall `Rules.Remove` is per-rule: some can land while others +/// fail. Reporting the entry count as `rules_removed` when only a subset +/// actually came out would hide a partial-cleanup failure — the two are kept +/// distinct here so `stop_all` can carry the truthful count into the audit +/// record and derive `firewall_removal_ok` from the aggregate. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct FirewallRemoval { + /// Number of rules whose `Rules.Remove` call returned success. + pub removed: usize, + /// Whether every removal succeeded. `false` when at least one rule + /// removal failed. + pub all_success: bool, +} + +impl Default for FirewallRemoval { + /// "Nothing to remove, and that is fine." Written by hand for the same + /// reason as [`NetworkTeardown::default`]: the derived default would set + /// `all_success: false`, which reads as a failed removal. + fn default() -> Self { + Self { + removed: 0, + all_success: true, + } + } +} + impl Default for NetworkTeardown { /// "Nothing to remove, and that is fine." /// @@ -153,6 +189,7 @@ impl NetworkManager { created_rule_names: Vec::new(), wsa_initialized: false, proxy_coordinator: ProxyCoordinator::new(), + firewall_apply_ok: None, } } @@ -214,15 +251,50 @@ impl NetworkManager { self.created_rule_names.len() } + /// Actual apply outcome of the most recent `apply_firewall_rules` call. + /// + /// * `None` — apply was never attempted (e.g. `start` returned early on a + /// proxy failure). + /// * `Some(true)` — apply succeeded, meaning either all requested rules + /// were installed or the policy required none. + /// * `Some(false)` — apply failed. + /// + /// Used by the caller to derive the `NetworkPolicyApplied` audit record's + /// `firewall_applied` field and aggregate `status` from the *observed* + /// outcome, not from the policy plan. + pub fn firewall_apply_ok(&self) -> Option { + self.firewall_apply_ok + } + + /// Whether firewall rules were actually installed by the most recent apply. + /// `false` when apply was never attempted, failed, or the policy required + /// no rules. This is the truth value the `firewall_applied` audit field + /// should carry — the policy-plan value can be true while the actual + /// installed rule count is zero. + pub fn firewall_applied(&self) -> bool { + matches!(self.firewall_apply_ok, Some(true)) && !self.created_rule_names.is_empty() + } + pub fn apply_firewall_rules( &mut self, principal_id: &str, policy: &ContainerPolicy, logger: &mut Logger, - ) -> Result { + ) -> Result<(), WxcError> { + let outcome = self.apply_firewall_rules_inner(principal_id, policy, logger); + self.firewall_apply_ok = Some(outcome.is_ok()); + outcome + } + + fn apply_firewall_rules_inner( + &mut self, + principal_id: &str, + policy: &ContainerPolicy, + logger: &mut Logger, + ) -> Result<(), WxcError> { let (default_policy, use_firewall_rules) = Self::initialize_policy(policy, logger); if !use_firewall_rules { - return Ok(true); + return Ok(()); } // Open a COM apartment and create the firewall interface for the @@ -255,7 +327,10 @@ impl NetworkManager { if default_policy == DefaultPolicy::Block { let block_all_name = format!("{}_BlockAll", rule_prefix); if !self.create_rule(&ctx, &block_all_name, NET_FW_ACTION_BLOCK, "", logger)? { - return Ok(false); + return Err(WxcError::Firewall(format!( + "failed to install primary firewall rule '{}'", + block_all_name + ))); } self.created_rule_names.push(block_all_name); self.process_host_list( @@ -269,7 +344,10 @@ impl NetworkManager { } else { let allow_all_name = format!("{}_AllowAll", rule_prefix); if !self.create_rule(&ctx, &allow_all_name, NET_FW_ACTION_ALLOW, "*", logger)? { - return Ok(false); + return Err(WxcError::Firewall(format!( + "failed to install primary firewall rule '{}'", + allow_all_name + ))); } self.created_rule_names.push(allow_all_name); self.process_host_list( @@ -282,7 +360,7 @@ impl NetworkManager { )?; } - Ok(true) + Ok(()) } fn process_host_list( @@ -370,7 +448,6 @@ impl NetworkManager { /// teardown record instead of assuming success. Failures remain non-fatal — /// this is a best-effort cleanup path and the return value is diagnostic. pub fn stop_all(&mut self, cleanup_policy: bool, logger: &mut Logger) -> NetworkTeardown { - let rules_at_entry = self.created_rule_names.len(); let mut outcome = NetworkTeardown { rules_removed: 0, firewall_removal_ok: true, @@ -379,13 +456,16 @@ impl NetworkManager { if self.rules_applied() && cleanup_policy { match self.remove_firewall_rules(logger) { - Ok(all_success) => { - outcome.rules_removed = rules_at_entry; - outcome.firewall_removal_ok = all_success; + Ok(removal) => { + // `rules_removed` counts *successful* removals only; a + // partial success no longer inflates the count to the + // full list length. `firewall_removal_ok` is the aggregate. + outcome.rules_removed = removal.removed; + outcome.firewall_removal_ok = removal.all_success; } Err(_) => { - // `remove_firewall_rules` failed before it could clear the - // list, so nothing was removed. + // `remove_firewall_rules` failed before it could remove + // anything, so nothing was removed. outcome.firewall_removal_ok = false; } } @@ -397,9 +477,15 @@ impl NetworkManager { outcome } - pub fn remove_firewall_rules(&mut self, logger: &mut Logger) -> Result { + pub fn remove_firewall_rules( + &mut self, + logger: &mut Logger, + ) -> Result { if self.created_rule_names.is_empty() { - return Ok(true); + return Ok(FirewallRemoval { + removed: 0, + all_success: true, + }); } // Re-acquire a fresh firewall interface in its own apartment on the @@ -415,10 +501,17 @@ impl NetworkManager { let rules = unsafe { fw_policy.Rules() } .map_err(|e| WxcError::Firewall(format!("Failed to get firewall rules: {}", e)))?; + // Count *successful* removals, not the entry count. Windows Firewall + // `Rules.Remove` returns per-rule success/failure; conflating the two + // makes a partially-failed cleanup indistinguishable from a clean one + // in the audit record. + let mut removed = 0usize; let mut all_success = true; for rule_name in &self.created_rule_names { let bstr_name = BSTR::from(rule_name.as_str()); - if unsafe { rules.Remove(&bstr_name) }.is_err() { + if unsafe { rules.Remove(&bstr_name) }.is_ok() { + removed += 1; + } else { all_success = false; } } @@ -426,7 +519,10 @@ impl NetworkManager { if !all_success { logger.log_line("Warning: some firewall rules could not be removed"); } - Ok(all_success) + Ok(FirewallRemoval { + removed, + all_success, + }) } fn ensure_wsa_initialized(&mut self, _logger: &mut Logger) -> Result<(), WxcError> { @@ -576,6 +672,39 @@ pub fn validate_ip_or_cidr(address: &str) -> bool { mod tests { use super::*; + /// `firewall_applied` in the audit record used to come from the policy + /// *plan* ("rules will be installed"). With apply now propagating its + /// actual outcome, a manager that never even attempted apply reports + /// `firewall_applied() == false` — the truth value the audit field should + /// carry. + #[test] + fn firewall_applied_defaults_to_false_when_apply_not_attempted() { + let mgr = NetworkManager::new(); + assert_eq!(mgr.firewall_apply_ok(), None); + assert!(!mgr.firewall_applied()); + } + + /// `stop_all` on a manager that installed nothing reports zero removals + /// as a clean teardown, and — regression for finding #7 — the + /// `FirewallRemoval` empty-list branch matches. + #[test] + fn remove_firewall_rules_with_nothing_installed_is_clean() { + let mut logger = Logger::new(wxc_common::logger::Mode::Buffer); + let mut manager = NetworkManager::new(); + let removal = manager.remove_firewall_rules(&mut logger).unwrap(); + assert_eq!(removal.removed, 0); + assert!(removal.all_success); + } + + /// `FirewallRemoval::default` must read as "nothing to remove, and that + /// is fine". The derived default sets `all_success: false`, which reads + /// as a *failed* removal — the opposite of what an empty removal means. + #[test] + fn firewall_removal_default_is_clean() { + assert_eq!(FirewallRemoval::default().removed, 0); + assert!(FirewallRemoval::default().all_success); + } + #[test] fn test_validate_ip_or_cidr_valid_ipv4() { assert!(validate_ip_or_cidr("192.168.1.1")); diff --git a/src/backends/isolation_session/common/src/manager.rs b/src/backends/isolation_session/common/src/manager.rs index 93042614b..403ca67cf 100644 --- a/src/backends/isolation_session/common/src/manager.rs +++ b/src/backends/isolation_session/common/src/manager.rs @@ -616,6 +616,22 @@ fn wait_with_graceful_shutdown( .ExitCode() .map_err(|e| lifecycle_err(format!("get ExitCode failed: {}", e)))?; if exit_code != STILL_ACTIVE { + // Normal pre-timeout completion. Emit `mxc.ProcessExited` so a + // clean run has a terminal audit record. Deliberately NOT emitted + // on the graceful-shutdown tiers below — those already emitted + // `ProcessTimedOut` and possibly `ProcessKillFailed`, and the run + // is not a normal exit. + if let Some(logger) = logger.as_mut() { + let record = AuditEvent::new(AuditEventName::ProcessExited) + .str("backend", "isolation_session") + .str( + "identity", + &wxc_common::policy_identity::redact_identity(identity), + ) + .u64("pid", process.ProcessId().unwrap_or_default() as u64) + .i64("exit_code", exit_code as i64); + logger.log_audit_event(&record); + } return Ok(exit_code); } @@ -680,8 +696,23 @@ fn wait_with_graceful_shutdown( } return Ok(exit_code); } + // Successful `Terminate()` — wait up to five seconds for the kill to + // land, then verify. If the process is still `STILL_ACTIVE` after the + // wait, or `ExitCode()` fails, surface a lifecycle error rather than + // returning `-1` as if the process had cleanly exited with that code: + // both outcomes are indistinguishable from a real exit code in the + // caller's contract, and both mean the terminate did not observably + // land. Matches the retry branch above. let _ = process.WaitForExit(5000); - Ok(process.ExitCode().unwrap_or(-1)) + let final_exit = process + .ExitCode() + .map_err(|e| lifecycle_err(format!("get ExitCode after terminate failed: {}", e)))?; + if final_exit == STILL_ACTIVE { + return Err(lifecycle_err( + "isolation-session process remained active after successful terminate", + )); + } + Ok(final_exit) } #[cfg(test)] diff --git a/src/backends/isolation_session/common/src/state_aware.rs b/src/backends/isolation_session/common/src/state_aware.rs index dced8c923..eb89df14f 100644 --- a/src/backends/isolation_session/common/src/state_aware.rs +++ b/src/backends/isolation_session/common/src/state_aware.rs @@ -11,7 +11,7 @@ use std::io::IsTerminal; use serde::Serialize; use wxc_common::id::mint_random_token; -use wxc_common::logger::{Logger, Mode}; +use wxc_common::logger::Logger; use wxc_common::models::{ ExecutionRequest, IsolationSessionConfig, IsolationSessionProvisionConfig, }; @@ -272,7 +272,11 @@ impl StatefulSandboxBackend for IsolationSessionRunner { let interactive = std::io::stdout().is_terminal(); let options = build_process_options(request, interactive); - let mut logger = Logger::new(Mode::Buffer); + // Inherit any diagnostic sinks (--log-file, diagnostic console pipe) + // the driver installed on this thread. When the driver has not + // installed anything this behaves identically to `Logger::new(Buffer)`, + // so the environment-driven pipe fallback below is still exercised. + let mut logger = Logger::inherit_thread_diagnostic_sink(); let diagnostic_config = wxc_common::diagnostic::DiagnosticConfig::from_environment(); if diagnostic_config.console_enabled { logger.enable_diagnostics(&diagnostic_config); diff --git a/src/core/wxc/src/main.rs b/src/core/wxc/src/main.rs index 59bb30726..cf937c1bc 100644 --- a/src/core/wxc/src/main.rs +++ b/src/core/wxc/src/main.rs @@ -510,7 +510,16 @@ fn run_state_aware_main( // State-aware dispatch bypasses the one-shot runner funnel, so anchor the // effective lifecycle policy here before the request is consumed. mxc_engine::log_policy_hash(&parsed.request, logger); + // Publish the driver's diagnostic sinks (--log-file, and the diagnostic + // console pipe on Windows) on this thread so a backend whose + // `StatefulSandboxBackend::exec` signature has no `Logger` parameter + // can inherit them via `Logger::inherit_thread_diagnostic_sink` instead + // of building a throwaway `Logger::new(Mode::Buffer)` and silently + // dropping every record. Cleared before this function returns so we + // never leak duplicated handles across independent invocations. + logger.install_thread_diagnostic_sink(); let mut outcome = mxc_engine::run_state_aware(parsed, dry_run); + Logger::clear_thread_diagnostic_sink(); let elapsed = started.elapsed(); // Record the sandbox identity join key. For `isolation_session` the diff --git a/src/core/wxc_common/src/audit.rs b/src/core/wxc_common/src/audit.rs index f8f174239..d54310ab8 100644 --- a/src/core/wxc_common/src/audit.rs +++ b/src/core/wxc_common/src/audit.rs @@ -392,10 +392,20 @@ pub fn sanitize_identity(identity: &str) -> &str { if identity.is_empty() { return identity; } - let opaque = identity.len() <= MAX_IDENTITY_LEN - && identity - .bytes() - .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.'); + // Length cap applies to BOTH accepted shapes. Without this bound on the + // `iso:`/`wsb:` shape a caller who chose an overlong `containerId` could + // still smuggle a payload through the record by tagging it with a + // recognised prefix. Byte length matches the character length here because + // both shapes accept only ASCII (`is_ascii_alphanumeric` plus `-`/`_`/`.` + // / `:`), so a rejection on `.len()` and a rejection on `.chars().count()` + // are equivalent — the byte bound is used to stay consistent with the + // existing contract. + if identity.len() > MAX_IDENTITY_LEN { + return REDACTED_IDENTITY; + } + let opaque = identity + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.'); let mxc_opaque = identity .split_once(':') .map(|(prefix, token)| { @@ -634,4 +644,36 @@ mod tests { ); } } + + /// The length cap applies to both accepted identity shapes, including the + /// prefixed `iso:` / `wsb:` shape. Without the bound on the prefixed shape + /// a caller could still smuggle a payload of arbitrary length through the + /// record by tagging it with a recognised prefix. + #[test] + fn overlong_prefixed_identities_are_redacted() { + for prefix in ["iso", "wsb"] { + let token = "a".repeat(MAX_IDENTITY_LEN); + let overlong = format!("{}:{}", prefix, token); + assert!(overlong.len() > MAX_IDENTITY_LEN); + assert_eq!( + sanitize_identity(&overlong), + REDACTED_IDENTITY, + "prefixed identity beyond MAX_IDENTITY_LEN should have been redacted: {overlong:?}" + ); + } + } + + /// The length cap is inclusive — a prefixed identity that just fits the + /// cap still passes through unredacted, so the tighter bound does not + /// accidentally reject well-formed short ids. + #[test] + fn prefixed_identities_within_len_pass_through() { + for prefix in ["iso", "wsb"] { + let colon_and_prefix = prefix.len() + 1; + let token = "a".repeat(MAX_IDENTITY_LEN - colon_and_prefix); + let id = format!("{}:{}", prefix, token); + assert_eq!(id.len(), MAX_IDENTITY_LEN); + assert_eq!(sanitize_identity(&id), id); + } + } } diff --git a/src/core/wxc_common/src/logger.rs b/src/core/wxc_common/src/logger.rs index ae8fa50ec..b2727faac 100644 --- a/src/core/wxc_common/src/logger.rs +++ b/src/core/wxc_common/src/logger.rs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +use std::cell::RefCell; use std::fmt; use std::fs::{File, OpenOptions}; use std::io::Write as IoWrite; @@ -40,6 +41,14 @@ pub struct Logger { diag_line_buf: String, } +thread_local! { + /// Per-thread parking slot for a `Logger` clone whose diagnostic sinks + /// (`--log-file` + Windows diagnostic pipe) should be inherited by any + /// downstream call that constructs its own logger. See + /// [`Logger::install_thread_diagnostic_sink`] for the rationale. + static THREAD_DIAG_SINK: RefCell> = const { RefCell::new(None) }; +} + impl fmt::Debug for Logger { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Logger") @@ -421,6 +430,62 @@ impl Logger { } } + /// Publish this logger's diagnostic sinks (`--log-file` handle and, on + /// Windows, the diagnostic-console pipe) on the **current thread** so a + /// downstream Rust API call whose signature has no `Logger` parameter can + /// still route its records to the driver's sinks instead of a discarded + /// throwaway buffer. + /// + /// The concrete need this exists for: `wxc-exec` opens a `--log-file` on + /// its main `Logger`, then calls `mxc_engine::run_state_aware`, which + /// consumes the parsed request and internally dispatches to a backend's + /// `StatefulSandboxBackend::exec` — a trait method that has no `Logger` + /// argument. Without this hook, the backend would build a fresh + /// `Logger::new(Mode::Buffer)`, wire it to the diagnostic-console pipe + /// from the environment, and drop every record into a buffer the caller + /// never reads — the `--log-file` sink is silently missing. + /// + /// The hook stores a `clone_diagnostic_sink` of this logger — the file + /// handle is `try_clone`-duplicated (append mode, so a shared file pointer + /// is safe) and, on Windows, the diagnostic pipe is duplicated the same + /// way (`PIPE_TYPE_MESSAGE`, so each `write_all` remains a discrete + /// message that cannot interleave). Publication is per-thread, so + /// concurrent runs on different threads never share sinks by accident. + /// + /// The installed sink is reclaimed with + /// [`Logger::clear_thread_diagnostic_sink`]; leaving one installed at + /// process exit is harmless (the sinks close with the process). + pub fn install_thread_diagnostic_sink(&self) { + let clone = self.clone_diagnostic_sink(); + THREAD_DIAG_SINK.with(|slot| { + *slot.borrow_mut() = Some(clone); + }); + } + + /// Clear any diagnostic sink installed on the current thread by + /// [`Logger::install_thread_diagnostic_sink`]. + pub fn clear_thread_diagnostic_sink() { + THREAD_DIAG_SINK.with(|slot| { + slot.borrow_mut().take(); + }); + } + + /// Return a fresh `Buffer`-mode logger that inherits the current thread's + /// installed diagnostic sinks. If no sink is installed the returned + /// logger has no diagnostic sinks — the same result as + /// `Logger::new(Mode::Buffer)`. + /// + /// This is what a Rust API path with no `Logger` parameter should use + /// **instead of** `Logger::new(Mode::Buffer)`: identical in the + /// no-sink case, but preserves the driver's `--log-file` (and pipe) + /// when the driver installed one. + pub fn inherit_thread_diagnostic_sink() -> Logger { + THREAD_DIAG_SINK.with(|slot| match slot.borrow().as_ref() { + Some(sink) => sink.clone_diagnostic_sink(), + None => Logger::new(Mode::Buffer), + }) + } + /// Emit a security warning through an always-visible channel and retain it /// for in-process callers. pub fn warning_line(&mut self, msg: &str) { diff --git a/src/core/wxc_common/src/policy_identity.rs b/src/core/wxc_common/src/policy_identity.rs index 7da045f34..fe54984cc 100644 --- a/src/core/wxc_common/src/policy_identity.rs +++ b/src/core/wxc_common/src/policy_identity.rs @@ -40,6 +40,7 @@ //! | `experimental.telemetry` | Does not affect enforcement. | //! | `experimental.isolation_session[.start].user` | Carries a WAM bearer token and a UPN. | //! | `network_proxy.original_url` | A proxy URL can embed `user:password@`. The host and port *are* hashed. | +//! | `capture_denials.output_path` | Only decides where the diagnostic JSON deliverable is written; not enforcement. `capture_denials.mode` remains hashed. | //! | `dry_run`, `testing_features_enabled` | Invocation modes, not policy. | //! //! `ContainerPolicy::network_proxy` is `#[serde(skip)]`, so the proxy's @@ -158,10 +159,21 @@ fn policy_projection(request: &ExecutionRequest) -> Value { // `ContainerPolicy` serialization already omits `network_proxy` // (`#[serde(skip)]`), so no credential-bearing proxy URL can reach the hash // through this line. - root.insert( - "policy".into(), - serde_json::to_value(policy).unwrap_or(Value::Null), - ); + // + // `captureDenials.outputPath` is stripped: it only controls where the + // diagnostic JSON deliverable is written and has no effect on enforcement. + // Hashing it would perturb the policy identity across otherwise-identical + // runs whose only difference is the output-file location — an operator + // moving the diagnostic file has not changed the policy the sandbox ran + // under. `captureDenials.mode` DOES stay hashed: it decides whether each + // recorded access is blocked or allowed, which is an enforcement change. + let mut policy_value = serde_json::to_value(policy).unwrap_or(Value::Null); + if let Value::Object(policy_map) = &mut policy_value { + if let Some(Value::Object(cd)) = policy_map.get_mut("capture_denials") { + cd.remove("output_path"); + } + } + root.insert("policy".into(), policy_value); root.insert("proxy".into(), proxy_projection(request)); root.insert( "lxc".into(), @@ -451,6 +463,63 @@ mod tests { assert_ne!(with_8080, policy_hash(&changed)); } + /// `captureDenials.outputPath` decides where the diagnostic JSON file is + /// written and has no effect on enforcement. It must not perturb the + /// policy identity: moving the output file is a diagnostic-plumbing + /// change, not a policy change. + #[test] + fn capture_denials_output_path_does_not_change_the_hash() { + use crate::models::{CaptureDenialsConfig, CaptureDenialsMode}; + + let mut baseline = request(); + baseline.policy.capture_denials = Some(CaptureDenialsConfig { + mode: CaptureDenialsMode::Block, + output_path: None, + }); + let base = policy_hash(&baseline); + + for path in [ + Some("C:\\logs\\denials.json".to_string()), + Some("D:\\other\\denials.json".to_string()), + None, + ] { + let mut changed = baseline.clone(); + if let Some(cd) = changed.policy.capture_denials.as_mut() { + cd.output_path = path; + } + assert_eq!( + base, + policy_hash(&changed), + "output_path controls diagnostic plumbing only and must not enter the hash" + ); + } + } + + /// `captureDenials.mode` decides whether each recorded access is blocked + /// or allowed, which is an enforcement decision. Changing it MUST change + /// the hash — this is the other half of the finding-3 contract. + #[test] + fn capture_denials_mode_changes_the_hash() { + use crate::models::{CaptureDenialsConfig, CaptureDenialsMode}; + + let mut baseline = request(); + baseline.policy.capture_denials = Some(CaptureDenialsConfig { + mode: CaptureDenialsMode::Block, + output_path: Some("C:\\logs\\denials.json".to_string()), + }); + let block_hash = policy_hash(&baseline); + + let mut allow = baseline.clone(); + if let Some(cd) = allow.policy.capture_denials.as_mut() { + cd.mode = CaptureDenialsMode::Allow; + } + let allow_hash = policy_hash(&allow); + assert_ne!( + block_hash, allow_hash, + "capture_denials.mode is an enforcement decision and MUST enter the hash" + ); + } + #[test] fn telemetry_settings_do_not_change_the_hash() { let baseline = policy_hash(&request()); diff --git a/src/tools/mxc_diagnostic_console/src/main.rs b/src/tools/mxc_diagnostic_console/src/main.rs index 5cf100d2d..4c0315a72 100644 --- a/src/tools/mxc_diagnostic_console/src/main.rs +++ b/src/tools/mxc_diagnostic_console/src/main.rs @@ -432,14 +432,38 @@ fn create_pipe_instance(pipe_name: &str, first: bool) -> Result result } +/// Render a message so it is safe to write to the diagnostic console TTY. +/// +/// Two orthogonal concerns are handled: +/// +/// 1. **Control-character hardening.** Anything below `0x20` other than `\t` +/// or `\n` is escaped, so a rogue client cannot inject terminal escape +/// sequences (cursor moves, colour changes, title updates) into the shared +/// console. +/// 2. **Lossless rendering of high Unicode.** Non-ASCII characters go through +/// [`char::escape_default`], which emits `\u{NNNN}` for anything outside +/// the printable ASCII range. The previous implementation masked the +/// Unicode scalar with `& 0xff` and rendered `\xNN`, which collided for +/// every pair of characters whose scalars agreed in the low byte (e.g. +/// `\u{0100}` and `\u{0200}` both rendered as `\x00`). fn sanitize_display_text(text: &str) -> String { - text.chars() - .flat_map(|ch| match ch { - '\t' | '\n' => Some(ch).into_iter().collect::>(), - '\u{20}'..='\u{7e}' => vec![ch], - _ => format!("\\x{:02X}", ch as u32 & 0xff).chars().collect(), - }) - .collect() + let mut out = String::with_capacity(text.len()); + for ch in text.chars() { + match ch { + // `\t` and `\n` are the only sub-`0x20` characters allowed through + // literally: they're the ordinary logical structure of a log line + // and rendering them as escapes would obscure it. + '\t' | '\n' => out.push(ch), + '\u{20}'..='\u{7e}' => out.push(ch), + // Everything else — control characters, DEL, and every non-ASCII + // codepoint — is escaped losslessly. `escape_default` emits `\uNNNN` + // form for high Unicode, `\xNN` for `< 0x80` non-printables, and + // `\\` / `\'` / `\"` for the corresponding literals; each escape + // is uniquely reversible so two distinct scalars can never collide. + _ => out.extend(ch.escape_default()), + } + } + out } /// Get the client process ID from a connected pipe handle. @@ -914,3 +938,54 @@ fn register_ctrl_handler() { let _ = SetConsoleCtrlHandler(Some(handler), true); } } + +#[cfg(test)] +mod tests { + use super::sanitize_display_text; + + /// The previous implementation masked the Unicode scalar with `& 0xff`, + /// so any pair of characters whose scalars agreed in the low byte + /// rendered identically (e.g. `\u{0100}` and `\u{0200}` both produced + /// `\x00`). `char::escape_default` guarantees a unique escape for every + /// distinct scalar. Regression for finding #10. + #[test] + fn high_unicode_scalars_do_not_collide() { + let a = sanitize_display_text("\u{0100}"); + let b = sanitize_display_text("\u{0200}"); + assert_ne!( + a, b, + "distinct scalars must render distinctly: {a:?} vs {b:?}" + ); + } + + /// Printable ASCII passes through unchanged, and `\t` / `\n` remain + /// literal (the two allow-listed control characters); every other control + /// character — including the terminal escape byte — MUST be escaped so a + /// rogue client cannot inject cursor / colour / title sequences into the + /// shared console. + #[test] + fn ascii_and_control_hardening_is_preserved() { + assert_eq!(sanitize_display_text("hello"), "hello"); + assert_eq!(sanitize_display_text("a\tb\nc"), "a\tb\nc"); + let esc_input = "\x1b[31mX"; + let esc_output = sanitize_display_text(esc_input); + assert!( + !esc_output.contains('\x1b'), + "raw ESC must not appear in output: {esc_output:?}" + ); + } + + /// Non-ASCII characters render losslessly. `char::escape_default` emits + /// `\u{NNNN}` for anything above `0x7F`, so the escaped form uniquely + /// decodes back to the original scalar — no `\xNN` low-byte collisions. + #[test] + fn non_ascii_is_rendered_losslessly() { + for ch in ['\u{00e9}', '\u{0100}', '\u{1F600}'] { + let rendered = sanitize_display_text(&ch.to_string()); + assert!( + rendered.starts_with("\\u{"), + "expected \\u{{NNNN}} form for {ch:?}, got {rendered:?}" + ); + } + } +} From 91cdeab8550f87de519332ec11ba165c7e69fad0 Mon Sep 17 00:00:00 2001 From: Ramon Arjona Date: Tue, 4 Aug 2026 22:35:10 -0700 Subject: [PATCH 3/4] fix: address remaining Copilot review comments on identity redaction and audit log writes - sanitize_identity now allows through only the closed set of shapes MXC itself mints (literal "CLI", sandbox-<16 hex>, iso:/wsb: state-aware ids), redacting every other caller-supplied containerId unconditionally. Character/ length checks alone could not prove a value was opaque vs. caller-chosen (e.g. alice, ticket-1234), so the permissive fallback branch is removed. - write_timestamped_file now assembles each timestamped line (including its terminator) into a single buffer and issues one write_all call, preventing concurrent writers from interleaving and corrupting the one-JSON-object- per-line audit log format. - Updated audit.rs and policy_identity.rs tests for the stricter redaction contract, and updated docs/telemetry/telemetry.md's content-rules bullet to describe the closed-set behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/telemetry/telemetry.md | 12 ++- src/core/wxc_common/src/audit.rs | 109 +++++++++++++++------ src/core/wxc_common/src/logger.rs | 17 +++- src/core/wxc_common/src/policy_identity.rs | 18 ++-- 4 files changed, 111 insertions(+), 45 deletions(-) diff --git a/docs/telemetry/telemetry.md b/docs/telemetry/telemetry.md index fa878b689..0f34d9a2b 100644 --- a/docs/telemetry/telemetry.md +++ b/docs/telemetry/telemetry.md @@ -332,10 +332,14 @@ collected by a fleet log agent. redaction marker instead of a user identifier. A truncated SHA-256 is not used: a low-entropy identity could be recovered by dictionary attack. The cost is that these sandboxes have no MXC-side join key in the local log. -* **No caller-supplied identifiers verbatim.** Sandbox identities derived from - configuration are retained only when they are bounded, opaque tokens (ASCII - alphanumeric plus `-`, `_`, `.`, ≤64 chars); anything else is replaced with - `redacted`. +* **No caller-supplied identifiers verbatim.** A sandbox identity derived from + configuration (the AppContainer profile name is the caller's `containerId`) + is retained only when it matches one of the closed set of shapes MXC itself + mints — the literal default `CLI`, `sandbox-<16 hex>`, or the state-aware + `iso:` / `wsb:` ids (≤64 chars, opaque token characters only). + Any other `containerId` the caller chose is replaced with `redacted`, + regardless of how opaque it looks — character/length checks alone cannot + prove a value wasn't caller-chosen. * **Counts, not names, for network rules.** Rule names can contain host and process identifiers, so only counts are recorded. diff --git a/src/core/wxc_common/src/audit.rs b/src/core/wxc_common/src/audit.rs index d54310ab8..991a6b39a 100644 --- a/src/core/wxc_common/src/audit.rs +++ b/src/core/wxc_common/src/audit.rs @@ -359,14 +359,24 @@ fn is_bare_json_key(key: &str) -> bool { .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'_') } -/// Maximum length of a sanitized identity. Long enough for the two shapes MXC -/// mints itself (`sandbox-<16 hex>` = 24 chars, `wxc-`), short enough -/// that a caller cannot smuggle a payload through the field. +/// Maximum length of a sanitized identity. Long enough for the MXC-minted +/// shapes (`sandbox-<16 hex>` = 24 chars, `iso:`/`wsb:` state-aware ids), +/// short enough that a caller cannot smuggle a payload through the field. const MAX_IDENTITY_LEN: usize = 64; /// Placeholder written when a caller-supplied identity is not an opaque token. pub const REDACTED_IDENTITY: &str = "redacted"; +/// Literal identity written by `AppContainerRunner`/`BaseContainerRunner` when +/// the caller left `containerId` empty. Not caller-controlled, so it is always +/// safe to log. +const DEFAULT_CLI_IDENTITY: &str = "CLI"; + +/// Length of the hex portion of a `sandbox-` identity minted by +/// `sandbox_tracking::generate_sandbox_identity` (8 bytes of CSPRNG +/// randomness rendered as lowercase hex). +const SANDBOX_ID_HEX_LEN: usize = 16; + /// Render a sandbox identity so it is safe to write to a diagnostic log file. /// /// **Sandbox identities are not always MXC-minted.** On the Windows @@ -374,38 +384,51 @@ pub const REDACTED_IDENTITY: &str = "redacted"; /// AppContainer profile name, which is the caller-supplied `containerId` /// straight out of the config. That makes it a *config value*, and config values /// must never reach a record (see the module-level content rules) — a caller can -/// otherwise put a UPN, a path, a ticket number, or an arbitrary string into the -/// audit stream. +/// otherwise put a UPN, a path, a ticket number, or any other arbitrary string +/// into the audit stream. Character and length checks alone cannot distinguish +/// a caller-chosen opaque-looking string (e.g. `alice`, `ticket-1234`) from a +/// value MXC actually minted, so this function does not attempt to recognise +/// "opaque-looking" input at all. /// -/// This function therefore allows through only identities that are recognisably -/// opaque tokens: +/// Instead it allows through only the closed set of shapes MXC itself +/// produces, unconditionally redacting every caller-supplied `containerId`: /// -/// * bounded length ([`MAX_IDENTITY_LEN`]); -/// * ASCII alphanumeric plus `-`, `_`, and `.` only — with the two MXC-minted -/// `iso:` and `wsb:` shapes also accepted; no `@` (UPN), no -/// `\` or `/` (path), no whitespace, no control characters. +/// * [`DEFAULT_CLI_IDENTITY`] — the literal default used when `containerId` +/// is empty; +/// * `sandbox-<16 lowercase hex>` — minted by +/// `sandbox_tracking::generate_sandbox_identity` for the BaseContainer +/// backend; +/// * `iso:` / `wsb:` — state-aware sandbox ids minted by the +/// IsolationSession / Windows Sandbox backends, bounded by +/// [`MAX_IDENTITY_LEN`] and restricted to opaque token characters. /// /// Anything else becomes [`REDACTED_IDENTITY`]. That loses the join key for -/// callers who chose a non-opaque `containerId`, which is the correct trade: a +/// callers who chose their own `containerId`, which is the correct trade: a /// record with no join key is recoverable, a leaked identifier is not. pub fn sanitize_identity(identity: &str) -> &str { if identity.is_empty() { return identity; } - // Length cap applies to BOTH accepted shapes. Without this bound on the - // `iso:`/`wsb:` shape a caller who chose an overlong `containerId` could - // still smuggle a payload through the record by tagging it with a - // recognised prefix. Byte length matches the character length here because - // both shapes accept only ASCII (`is_ascii_alphanumeric` plus `-`/`_`/`.` - // / `:`), so a rejection on `.len()` and a rejection on `.chars().count()` - // are equivalent — the byte bound is used to stay consistent with the - // existing contract. + if identity == DEFAULT_CLI_IDENTITY { + return identity; + } + if let Some(hex) = identity.strip_prefix("sandbox-") { + if hex.len() == SANDBOX_ID_HEX_LEN + && hex + .bytes() + .all(|b| b.is_ascii_digit() || (b'a'..=b'f').contains(&b)) + { + return identity; + } + return REDACTED_IDENTITY; + } + // Length cap applies before shape-checking the `iso:`/`wsb:` prefix. Without + // this bound a caller who chose an overlong `containerId` could still + // smuggle a payload through the record by tagging it with a recognised + // prefix. if identity.len() > MAX_IDENTITY_LEN { return REDACTED_IDENTITY; } - let opaque = identity - .bytes() - .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.'); let mxc_opaque = identity .split_once(':') .map(|(prefix, token)| { @@ -416,7 +439,7 @@ pub fn sanitize_identity(identity: &str) -> &str { .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.') }) .unwrap_or(false); - if opaque || mxc_opaque { + if mxc_opaque { identity } else { REDACTED_IDENTITY @@ -609,14 +632,12 @@ mod tests { } #[test] - fn opaque_identities_pass_sanitization_unchanged() { + fn mxc_minted_identities_pass_sanitization_unchanged() { for id in [ "sandbox-a3f1c8e40029bd17", - "wxc-abcd1234", "iso:wxc-abcd1234", "wsb:deadbeef", "CLI", - "my.container_id-7", "", ] { assert_eq!(sanitize_identity(id), id); @@ -624,11 +645,19 @@ mod tests { } /// A caller-supplied `containerId` becomes the AppContainer profile name and - /// therefore the sandbox identity. It is a config value, so anything that is - /// not recognisably an opaque token must not reach a record. + /// therefore the sandbox identity. It is a config value, so it must always be + /// redacted regardless of shape — character/length checks cannot prove a + /// value is opaque rather than caller-chosen (e.g. `alice`, `ticket-1234`, + /// or a standalone `wxc-`-looking token that did not come through the + /// `iso:`/`wsb:` minting path). #[test] - fn non_opaque_caller_identities_are_redacted() { + fn caller_supplied_identities_are_always_redacted() { for id in [ + "alice", + "secret", + "ticket-1234", + "my.container_id-7", + "wxc-abcd1234", "alice@contoso.com", "C:\\Users\\alice\\secret", "/home/alice/secret", @@ -645,6 +674,26 @@ mod tests { } } + /// A `sandbox-` prefix alone does not grant a pass: the hex portion must be + /// exactly [`SANDBOX_ID_HEX_LEN`] lowercase hex characters, matching + /// `sandbox_tracking::generate_sandbox_identity`'s output shape. + #[test] + fn malformed_sandbox_prefixed_identities_are_redacted() { + for id in [ + "sandbox-", + "sandbox-abc", + "sandbox-A3F1C8E40029BD17", + "sandbox-a3f1c8e40029bd17extra", + "sandbox-not-hex-at-all!!", + ] { + assert_eq!( + sanitize_identity(id), + REDACTED_IDENTITY, + "should have been redacted: {id:?}" + ); + } + } + /// The length cap applies to both accepted identity shapes, including the /// prefixed `iso:` / `wsb:` shape. Without the bound on the prefixed shape /// a caller could still smuggle a payload of arbitrary length through the diff --git a/src/core/wxc_common/src/logger.rs b/src/core/wxc_common/src/logger.rs index b2727faac..1bbdeb393 100644 --- a/src/core/wxc_common/src/logger.rs +++ b/src/core/wxc_common/src/logger.rs @@ -335,22 +335,35 @@ impl Logger { self.diag_accumulate("\n"); } + /// Renders `msg` (possibly multi-line) into timestamped lines and issues a + /// **single** `write_all` for the whole assembled buffer. + /// + /// Two clones of the same `Logger` (or two processes) can append to the + /// same file concurrently. `File::write_all` on most platforms does not + /// guarantee a single syscall's data lands atomically relative to a + /// concurrent writer once split across multiple `write`/`write_all` calls, + /// so building the complete line(s) first and writing them in one call + /// keeps the on-disk format's one-record-per-line invariant even under + /// concurrent appenders. fn write_timestamped_file(file: &mut File, msg: &str, terminate: bool) { + use std::fmt::Write as _; let secs = SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap_or_default() .as_secs(); let lines: Vec<&str> = msg.split('\n').collect(); + let mut out = String::with_capacity(msg.len() + lines.len() * 16); for (index, line) in lines.iter().enumerate() { let is_trailing_empty_line = index + 1 == lines.len() && line.is_empty(); if is_trailing_empty_line && msg.ends_with('\n') { break; } - let _ = write!(file, "[{}] {}", secs, line.trim_end_matches('\r')); + let _ = write!(out, "[{}] {}", secs, line.trim_end_matches('\r')); if index + 1 < lines.len() || terminate { - let _ = file.write_all(b"\n"); + out.push('\n'); } } + let _ = file.write_all(out.as_bytes()); } /// Emit a structured [`AuditEvent`](crate::audit::AuditEvent) as a single diff --git a/src/core/wxc_common/src/policy_identity.rs b/src/core/wxc_common/src/policy_identity.rs index fe54984cc..e25979b47 100644 --- a/src/core/wxc_common/src/policy_identity.rs +++ b/src/core/wxc_common/src/policy_identity.rs @@ -605,10 +605,9 @@ mod tests { } #[test] - fn opaque_identities_pass_through_unredacted() { + fn mxc_minted_identities_pass_through_unredacted() { for id in [ "sandbox-a3f1c8e40029bd17", - "wxc-abcd1234", "iso:wxc-abcd1234", "wsb:deadbeef", "CLI", @@ -643,14 +642,15 @@ mod tests { } /// A caller-supplied `containerId` becomes the sandbox identity on the - /// ProcessContainer path, so non-opaque values must be redacted rather than - /// echoed into a record. + /// ProcessContainer path, so it must be redacted rather than echoed into a + /// record — even when it happens to look like an opaque token, since + /// character/length checks alone cannot prove it wasn't chosen by the + /// caller (e.g. `alice`, `ticket-1234`). #[test] - fn non_opaque_identities_are_redacted() { - assert_eq!( - redact_identity("C:\\Users\\alice\\ticket-1234"), - crate::audit::REDACTED_IDENTITY - ); + fn caller_supplied_identities_are_redacted() { + for id in ["C:\\Users\\alice\\ticket-1234", "alice", "ticket-1234"] { + assert_eq!(redact_identity(id), crate::audit::REDACTED_IDENTITY); + } } #[test] From f6faf28db42389586f7c88d928eb9b84aa154b8b Mon Sep 17 00:00:00 2001 From: Ramon Arjona Date: Wed, 5 Aug 2026 12:00:13 -0700 Subject: [PATCH 4/4] fix: redact secret-bearing fields in raw JSON config diagnostic dump The 'SECTION: JSON Config' diagnostic block wrote the caller-supplied config verbatim before the runner validated/rejected it. A one-shot IsolationSession request's experimental.isolationSession.user bundle (upn, wamToken) is only rejected by the runner after this point, so credentials could reach diagnostic sinks in the clear even though the parsed 'Full ExecutionRequest configuration (redacted)' section below it already redacted them. Add wxc_common::diagnostic::redact_raw_config_json, which parses the raw text and recursively blanks any JSON object key matching the same secret-bearing markers already used for config-parse-error redaction (config_deserialize::is_secret_path_field, now shared pub(crate)). Malformed JSON that cannot be parsed is replaced with a placeholder rather than emitted raw, since we cannot prove it is credential-free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/core/wxc/src/main.rs | 12 ++- src/core/wxc_common/src/config_deserialize.rs | 17 +++- src/core/wxc_common/src/diagnostic.rs | 87 +++++++++++++++++++ 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/src/core/wxc/src/main.rs b/src/core/wxc/src/main.rs index cf937c1bc..55dc268e2 100644 --- a/src/core/wxc/src/main.rs +++ b/src/core/wxc/src/main.rs @@ -1378,8 +1378,16 @@ fn main() { fs::read_to_string(&config_data).ok() }; if let Some(json) = raw_json { - let _ = writeln!(logger, "SECTION: JSON Config"); - let _ = writeln!(logger, "{}", json.trim()); + // Redact secret-bearing fields (e.g. `experimental.isolationSession.user.{upn,wamToken}`) + // before writing: a one-shot IsolationSession request's credential + // bundle is only rejected by the runner after this point, so the + // raw config as received from the caller may still contain it. + let _ = writeln!(logger, "SECTION: JSON Config (redacted)"); + let _ = writeln!( + logger, + "{}", + wxc_common::diagnostic::redact_raw_config_json(json.trim()) + ); } } diff --git a/src/core/wxc_common/src/config_deserialize.rs b/src/core/wxc_common/src/config_deserialize.rs index 56e0ae97e..7e3ee5716 100644 --- a/src/core/wxc_common/src/config_deserialize.rs +++ b/src/core/wxc_common/src/config_deserialize.rs @@ -34,6 +34,18 @@ const SECRET_PATH_MARKERS: &[&str] = &[ /// never leaks one. const SECRET_PATH_SEGMENTS: &[&str] = &["user"]; +/// Whether a single lower-cased JSON object key is secret-bearing, per +/// [`SECRET_PATH_SEGMENTS`] (whole-field match) and [`SECRET_PATH_MARKERS`] +/// (substring match). Shared by error-path redaction (this module) and raw +/// config redaction (`diagnostic::redact_raw_config_json`) so both use one +/// definition of "secret-bearing". +pub(crate) fn is_secret_path_field(field: &str) -> bool { + SECRET_PATH_SEGMENTS.contains(&field) + || SECRET_PATH_MARKERS + .iter() + .any(|marker| field.contains(marker)) +} + /// A JSON deserialization failure with the path at which typed policy parsing /// failed. Syntax errors have no meaningful policy path. #[derive(Debug)] @@ -90,10 +102,7 @@ impl ConfigDeserializeError { // Match on the field name only, dropping any array-index suffix // so `field[0]` matches on `field`. let field = segment.split('[').next().unwrap_or(segment); - SECRET_PATH_SEGMENTS.contains(&field) - || SECRET_PATH_MARKERS - .iter() - .any(|marker| field.contains(marker)) + is_secret_path_field(field) }) }) } diff --git a/src/core/wxc_common/src/diagnostic.rs b/src/core/wxc_common/src/diagnostic.rs index bf229971d..4d4bf9937 100644 --- a/src/core/wxc_common/src/diagnostic.rs +++ b/src/core/wxc_common/src/diagnostic.rs @@ -191,6 +191,55 @@ pub fn redacted_request_json(request: &ExecutionRequest) -> String { format!("{json}{proxy_info}") } +/// Parse caller-supplied raw config JSON and redact secret-bearing fields +/// (same closed set of markers as [`crate::config_deserialize`]'s error-path +/// redaction, e.g. `token`, `secret`, and the whole `user` credential bundle +/// used by `experimental.isolationSession.user.{upn,wamToken}`) before it is +/// safe to write to a diagnostic sink. +/// +/// This must run *before* any policy validation: an `IsolationSession` +/// one-shot request's credential bundle is only rejected by the runner after +/// the request has already been parsed and logged, so the raw text emitted +/// here cannot rely on downstream validation having stripped it first. +/// +/// If the text fails to parse as JSON, a placeholder is returned instead of +/// the raw text, since malformed input cannot be proven free of embedded +/// credentials. +pub fn redact_raw_config_json(raw: &str) -> String { + match serde_json::from_str::(raw) { + Ok(mut value) => { + redact_secret_fields(&mut value); + serde_json::to_string_pretty(&value) + .unwrap_or_else(|_| "".to_string()) + } + Err(_) => "".to_string(), + } +} + +/// Recursively blank JSON object values whose key is secret-bearing (see +/// [`crate::config_deserialize::is_secret_path_field`]). Over-redaction fails +/// safe: e.g. blanking the whole `user` object (rather than only `upn`/ +/// `wamToken` within it) never leaks a credential. +fn redact_secret_fields(value: &mut serde_json::Value) { + match value { + serde_json::Value::Object(map) => { + for (key, entry) in map.iter_mut() { + if crate::config_deserialize::is_secret_path_field(&key.to_ascii_lowercase()) { + *entry = serde_json::Value::String("".to_string()); + } else { + redact_secret_fields(entry); + } + } + } + serde_json::Value::Array(items) => { + for item in items.iter_mut() { + redact_secret_fields(item); + } + } + _ => {} + } +} + /// Get the parent process name and PID (e.g. `"node.exe:67890"`). /// /// Returns `"unknown"` if the parent PID cannot be determined, or @@ -357,6 +406,44 @@ mod tests { assert_eq!(json.matches("").count(), 2); } + #[test] + fn redact_raw_config_json_hides_isolation_session_user_bundle() { + let raw = r#"{ + "process": {"commandLine": "echo hi"}, + "containment": "isolation_session", + "experimental": { + "isolationSession": { + "user": {"upn": "alice@contoso.com", "wamToken": "super-secret-bearer-token"} + } + } + }"#; + let redacted = redact_raw_config_json(raw); + assert!(!redacted.contains("alice@contoso.com")); + assert!(!redacted.contains("super-secret-bearer-token")); + assert!(redacted.contains("")); + // Non-secret fields survive untouched. + assert!(redacted.contains("echo hi")); + assert!(redacted.contains("isolation_session")); + } + + #[test] + fn redact_raw_config_json_hides_bare_token_and_secret_fields() { + let raw = r#"{"apiKey": "abc123", "clientSecret": "xyz", "commandLine": "run"}"#; + let redacted = redact_raw_config_json(raw); + assert!(!redacted.contains("abc123")); + assert!(!redacted.contains("xyz")); + assert!(redacted.contains("run")); + } + + #[test] + fn redact_raw_config_json_falls_back_on_malformed_json() { + let redacted = redact_raw_config_json("{ not valid json"); + assert_eq!( + redacted, + "" + ); + } + #[test] fn env_bool_parses_correctly() { // env_bool on non-existent var returns None