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..0f34d9a2b 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,247 @@ 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.** 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. + +### 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 +507,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..248f671d4 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,69 @@ 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() { + // `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)) + .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", firewall_applied) + .str( + "status", + if network_result.is_ok() && firewall_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,15 +1370,58 @@ 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); - if self.filesystem_mode == FilesystemMode::Bfs + let network = prepared.network_manager.stop_all(!preserve_policy, logger); + // 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_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()) + // 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); + 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() + } } impl SandboxBackend for AppContainerScriptRunner { @@ -1360,6 +1478,8 @@ impl SandboxBackend for AppContainerScriptRunner { prepared, self.filesystem_mode, request, + self.app_container_name.clone(), + logger, ))) } @@ -1394,6 +1514,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 +1553,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 +1563,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 +1579,120 @@ 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 + // 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 - { - self.prepared.bfs_manager.remove_configuration(&mut logger); + && !self.preserve_policy; + 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_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()) + .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. +/// 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), + ); + } + let bfs_ok = !bfs_requested || bfs_removed; + if firewall_removal_ok && bfs_ok { + (TeardownStatus::Success, None) + } else { + (TeardownStatus::Failure, None) } } @@ -1506,7 +1738,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 +1775,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 => 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")), }; @@ -1571,6 +1834,87 @@ 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_with_bfs; + use wxc_common::audit::{TeardownSkipReason, TeardownStatus}; + + for firewall_ok in [true, false] { + 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)); + } + } + } + + /// 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_with_bfs; + use wxc_common::audit::TeardownStatus; + + assert_eq!( + appcontainer_teardown_status_with_bfs(false, true, false, false), + (TeardownStatus::Success, None) + ); + assert_eq!( + 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; + 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..1970bfba2 100644 --- a/src/backends/appcontainer/common/src/network_manager.rs +++ b/src/backends/appcontainer/common/src/network_manager.rs @@ -87,6 +87,74 @@ 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. +/// +/// 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, +} + +/// 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." + /// + /// 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 @@ -98,39 +166,113 @@ 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 { created_rule_names: Vec::new(), wsa_initialized: false, proxy_coordinator: ProxyCoordinator::new(), + firewall_apply_ok: None, } } - 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, + }; } - (DefaultPolicy::Allow, use_firewall_rules) + 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) + } + + /// 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() + } + + /// 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( @@ -138,10 +280,21 @@ impl NetworkManager { 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 @@ -174,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( @@ -188,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( @@ -201,7 +360,7 @@ impl NetworkManager { )?; } - Ok(true) + Ok(()) } fn process_host_list( @@ -284,18 +443,49 @@ 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 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(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 remove + // anything, 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 { + 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 @@ -311,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; } } @@ -322,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> { @@ -472,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")); @@ -511,6 +744,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..403ca67cf 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; @@ -606,9 +616,37 @@ 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..eb89df14f 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; use wxc_common::models::{ ExecutionRequest, IsolationSessionConfig, IsolationSessionProvisionConfig, }; @@ -271,9 +272,18 @@ impl StatefulSandboxBackend for IsolationSessionRunner { let interactive = std::io::stdout().is_terminal(); let options = build_process_options(request, interactive); + // 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); + } 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..55dc268e2 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,41 @@ 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); + // 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 + // `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 +1066,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 +1105,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 +1123,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 +1154,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 +1238,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 +1271,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 +1298,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 +1318,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 +1342,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. @@ -1163,24 +1378,35 @@ 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()) + ); } } 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 +1426,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 +1660,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..991a6b39a --- /dev/null +++ b/src/core/wxc_common/src/audit.rs @@ -0,0 +1,728 @@ +// 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 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 +/// 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 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. +/// +/// Instead it allows through only the closed set of shapes MXC itself +/// produces, unconditionally redacting every caller-supplied `containerId`: +/// +/// * [`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 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; + } + 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 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 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 mxc_minted_identities_pass_sanitization_unchanged() { + for id in [ + "sandbox-a3f1c8e40029bd17", + "iso:wxc-abcd1234", + "wsb:deadbeef", + "CLI", + "", + ] { + 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 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 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", + "has space", + "has\"quote", + "has\nnewline", + &"x".repeat(MAX_IDENTITY_LEN + 1), + ] { + assert_eq!( + sanitize_identity(id), + REDACTED_IDENTITY, + "should have been redacted: {id:?}" + ); + } + } + + /// 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 + /// 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/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/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..4d4bf9937 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}\"}}")); @@ -158,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 @@ -307,9 +389,73 @@ 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 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 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..1bbdeb393 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") @@ -88,12 +97,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 +116,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 +152,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 +180,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 +214,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 +290,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 +308,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 +328,184 @@ 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"); } + /// 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!(out, "[{}] {}", secs, line.trim_end_matches('\r')); + if index + 1 < lines.len() || terminate { + out.push('\n'); + } + } + let _ = file.write_all(out.as_bytes()); + } + + /// 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(), + } + } + + /// 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) { 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 +566,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 +612,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 +625,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..e25979b47 --- /dev/null +++ b/src/core/wxc_common/src/policy_identity.rs @@ -0,0 +1,710 @@ +// 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. | +//! | `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 +//! 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. + // + // `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(), + 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)); + } + + /// `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()); + 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 mxc_minted_identities_pass_through_unredacted() { + for id in [ + "sandbox-a3f1c8e40029bd17", + "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 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 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] + 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..4c0315a72 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,53 @@ 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 +} + +/// 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 { + 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. @@ -450,11 +503,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 +521,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 +548,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()) } @@ -878,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:?}" + ); + } + } +}