Log boundary relaxations with secure-default warnings - #726
Log boundary relaxations with secure-default warnings#726Gudge (MGudgin) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds centralized logging for sandbox policy settings that relax security boundaries.
Changes:
- Detects network, UI, ProcessContainer, and Seatbelt relaxations.
- Sanitizes free-form log values.
- Adds five unit tests for warning behavior.
Show a summary per file
| File | Description |
|---|---|
src/core/wxc_common/src/config_parser.rs |
Adds boundary-relaxation diagnostics and tests. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 4
- Review effort level: Balanced
| // ProcessContainer capabilities (effective regardless of ui.disable). | ||
| if !policy.capabilities.is_empty() { | ||
| logger.log_line(&format!( | ||
| "{P} processContainer.capabilities ({} cap(s))", | ||
| policy.capabilities.len() | ||
| )); | ||
| } |
| }; | ||
| } | ||
|
|
||
| log_boundary_relaxations(&policy, seatbelt.as_ref(), logger); |
| fn log_boundary_relaxations( | ||
| policy: &ContainerPolicy, | ||
| seatbelt: Option<&SeatbeltConfig>, | ||
| logger: &mut Logger, |
| // UI. Clipboard, injection, windows, and the BaseProcess desktop knobs are | ||
| // inert while UI is disabled; only warn when ui.disable=false opens them. | ||
| // `ime` and capabilities stay effective regardless and are reported below. |
| policy.allowed_hosts.len() | ||
| )); | ||
| } | ||
| // blockedHosts is intentionally not reported: it only subtracts |
There was a problem hiding this comment.
On Hyperlight, a non-empty blocked_hosts returns BlockList ("rest allowed") before the default_network_policy == Block -> None (networking-disabled) check; on NanVix, any host list enables networking "regardless of defaultPolicy." So, under defaultPolicy=block, blockedHosts flips networking OFF->ON- a real relaxation this feature should log?
| "{P} processContainer.ui.desktopSystemControl=true" | ||
| )); | ||
| } | ||
| if ui.system_settings != "none" { |
There was a problem hiding this comment.
Enforcement (ui_policy.rs) only relaxes for all / parameters / display; any unrecognized value falls through to default-deny. Warning on != "none" emits a false boundary relaxed line for inert/garbage values. Gate on matches! (ui.system_settings.as_str(), "all" | "parameters" | "display")
| '\t' => out.push_str("\\t"), | ||
| '"' => out.push_str("\\\""), | ||
| '\\' => out.push_str("\\\\"), | ||
| c if c.is_control() => out.push_str(&format!("\\u{{{:04x}}}", c as u32)), |
There was a problem hiding this comment.
char::is_control() only covers category Cc; U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR) are not control chars and pass through unescaped?
| } | ||
|
|
||
| // ProcessContainer capabilities (effective regardless of ui.disable). | ||
| if !policy.capabilities.is_empty() { |
There was a problem hiding this comment.
processContainer.learningMode=true injects learningModeLogging into policy.capabilities, which is observability-only- the note at :833 states enforcement is unchanged. This block then falsely reports it as a capabilities relaxation?
| // redundant on Windows (where UI-disabled forces clipboard blocks) but never | ||
| // misses a real relaxation. | ||
| if policy.ui.clipboard != ClipboardPolicy::None { | ||
| logger.log_line(&format!("{P} ui.clipboard={:?}", policy.ui.clipboard)); |
There was a problem hiding this comment.
{:?} writes internal variant names (All / Read), not the stable lowercase wire form, so the persisted audit line and the test (ui.clipboard=All) break if the enum is renamed?
There was a problem hiding this comment.
Review details
Suppressed comments (5)
src/core/wxc_common/src/config_parser.rs:820
- Bubblewrap is also missing from this check when
network.enforcementModeis the defaultcapabilities:bwrap_command.rs:137-149drops--unshare-netwhenever any host list exists, whilebwrap_runner.rs:480-492installs iptables only forfirewall/both. Therefore a deny-by-default Bubblewrap config containing onlyblockedHostsopens shared networking but emits noblockedHostssecurity warning. Include this mode and add it to the regression test.
let list_implies_networking = matches!(
containment,
ContainmentBackend::MicroVm | ContainmentBackend::Hyperlight
);
src/core/wxc_common/src/config_parser.rs:798
allowLocalNetworkis only consumed by the Seatbelt profile builder; all other backends ignore this domain field. (The Windows SDK separately translates its policy option intoprivateNetworkClientServer.) A direct LXC/WSLC/ProcessContainer config therefore gets a “boundary relaxed” line without changing enforcement. Restrict this field-level warning to Seatbelt; Windows SDK requests will still be covered by the effective capability warning.
if policy.allow_local_network {
logger.warning_line(&format!("{P} network.allowLocalNetwork=true"));
}
src/core/wxc_common/src/config_parser.rs:857
- These UI warnings are emitted for every containment backend, but only ProcessContainer and Seatbelt read
policy.ui(the only backend usages are in the AppContainer/BaseContainer and Seatbelt crates). For example, a valid LXC config withui.disable=falsenow reports “windows allowed” although LXC ignores the setting and no boundary changes. Gate the UI diagnostics on an active backend that actually applies them so the audit stream does not report nonexistent relaxations.
if !policy.ui.disable {
logger.warning_line(&format!("{P} ui.disable=false (windows allowed)"));
if policy.ui.injection {
logger.warning_line(&format!("{P} ui.injection=true"));
}
src/core/wxc_common/src/config_parser.rs:825
- The PR description says
blockedHostsis deliberately never reported because it is purely subtractive, but this branch correctly reports it for backends where a list enables networking. Please update the description (and its stated test count/list) so reviewers and future audit documentation match the implemented behavior.
if list_implies_networking
&& !policy.blocked_hosts.is_empty()
&& policy.default_network_policy == NetworkPolicy::Block
{
logger.warning_line(&format!(
src/core/wxc_common/src/config_parser.rs:911
- Learning mode is not the only synthesized capability. Both Windows SDK builders derive
internetClientandprivateNetworkClientServerfrom the network policy (sdk/node/src/sandbox.ts:142-146andmxc_engine/src/policy.rs:747-755). Consequently an SDKallowOutboundrequest emits both the network warning and aprocessContainer.capabilitieswarning that is incorrectly attributed to a caller-requested capability. Exclude already-accounted derived network capabilities or preserve capability origin through parsing.
let requested_capabilities = policy
.capabilities
.iter()
.filter(|c| {
!c.eq_ignore_ascii_case(LEARNING_MODE_LOGGING_CAPABILITY)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (6)
src/core/wxc_common/src/config_parser.rs:799
allowedHostsonly opens connectivity underdefaultPolicy=block. Underallowit is either redundant (LXC/Bubblewrap/WSLc/Seatbelt) or restrictive (Hyperlight/NanVix), so labeling it as another boundary relaxation is a false audit entry. Match theblockedHostslogic by considering the effective default.
if !policy.allowed_hosts.is_empty() {
src/core/wxc_common/src/config_parser.rs:791
- On Hyperlight/NanVix an allowlist overrides
defaultPolicy, sodefaultPolicy=allowplusallowedHostsproduces an allowlist—not “network open by default.” This currently emits a false relaxation line in addition to the allowlist line. Suppress the default-policy warning when those backends have an allowlist, or describe the effective policy instead.
if policy.default_network_policy == NetworkPolicy::Allow {
src/core/wxc_common/src/config_parser.rs:847
- Clipboard warnings are emitted for every backend, but only ProcessContainer and Seatbelt consume
policy.ui; Bubblewrap, LXC, WSLc, and the VM backends ignore it. For those successful runs this records a boundary relaxation that never occurred, contrary to the active-backend audit contract.
if policy.ui.clipboard != ClipboardPolicy::None {
src/core/wxc_common/src/config_parser.rs:853
- This generic UI branch also runs for backends that never inspect
policy.ui, so e.g.containment=microvmwithui.disable=falsereports “windows allowed” although execution is unchanged. Gate the branch to ProcessContainer/Seatbelt so the audit describes the selected sandbox.
if !policy.ui.disable {
src/core/wxc_common/src/config_parser.rs:855
- For Seatbelt,
injection=trueonly removes an explicit HID deny; deny-default still blocks HID unlessguiAccessemits the broad(allow iokit-open). Thusui.disable=false,injection=true,guiAccess=falseis inert but is logged as a relaxation. Keep the unconditional behavior for ProcessContainer, but require effectiveguiAccessfor Seatbelt.
if policy.ui.injection {
src/core/wxc_common/src/config_parser.rs:783
- The PR promises a warning for every setting that opens a boundary, but this helper cannot inspect
experimental.wslc.portMappings. A nonempty mapping is passed toWslcSetContainerSettingsPortMappingsand forwards a Windows host port (src/backends/wslc/common/src/wsl_container_runner.rs:1093-1125), yet it produces no SECURITY line. Pass the active WSLc config into this audit and warn for nonempty mappings.
fn log_boundary_relaxations(
policy: &ContainerPolicy,
seatbelt: Option<&SeatbeltConfig>,
containment: &ContainmentBackend,
logger: &mut Logger,
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (4)
src/core/wxc_common/src/config_parser.rs:813
allowedHostsis only a relaxation relative to deny-by-default. WithdefaultPolicy=allow, LXC still ends its chain inACCEPT(network_iptables.rs:226-232), while Hyperlight turns the request into anAllowList(hyperlight/common/src/lib.rs:327-341), so this line produces a false security warning for a redundant or stricter setting. Only report this field when the effective baseline isblock(in addition to backend support gating).
if !policy.allowed_hosts.is_empty() {
logger.warning_line(&format!(
"{P} network.allowedHosts ({} host(s))",
policy.allowed_hosts.len()
));
}
src/core/wxc_common/src/config_parser.rs:963
- When
profileOverrideis present,build_profile_with_proxyreturns the override immediately (profile_builder.rs:43-49) and never applies generated-profile network, UI, keychain, or extra-Mach settings. This function has already warned for those ignored settings and continues to warn for keychain/extra lookups below, so the audit stream does not describe the profile that runs. Detect the override before profile-derived checks and suppress those diagnostics, while retaining theprofileOverridewarning and the independentlaunchMethodwarning.
if sb.profile_override.is_some() {
logger.warning_line(&format!(
"{P} seatbelt.profileOverride (generated profile bypassed)"
));
}
src/core/wxc_common/src/config_parser.rs:870
- On ProcessContainer,
ui.disable=trueforces both clipboard restrictions regardless ofui.clipboard(ui_policy.rs:53-65), so this condition emits a “boundary relaxed” warning when no Windows boundary changed. Keep the unconditional clipboard warning only for Seatbelt; on ProcessContainer require UI to be enabled.
if honors_ui && policy.ui.clipboard != ClipboardPolicy::None {
logger.warning_line(&format!(
"{P} ui.clipboard={}",
policy.ui.clipboard.wire_name()
));
src/core/wxc_common/src/config_parser.rs:796
- This also fires for backends that reject the setting rather than relaxing anything. Windows Sandbox rejects
defaultPolicy=allow(windows_sandbox/lifecycle/src/policy.rs:52-58), and IsolationSession rejects every non-blockpolicy (isolation_session/common/src/policy.rs:70-80); because this runs during parsing, the log claims “boundary relaxed” before those backends fail validation. The same issue affects the unconditional host-list and proxy warnings below. Gate each network warning on the active backend's supported/effective policy, or emit these diagnostics only after backend validation succeeds.
This issue also appears on line 808 of the same file.
if policy.default_network_policy == NetworkPolicy::Allow {
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
45177a7 to
31d6420
Compare
31d6420 to
3f543f5
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/core/wxc_common/src/config_parser.rs:796
- This reports a relaxation for backends that reject this policy. Windows Sandbox rejects
defaultPolicy=allowinwindows_sandbox/lifecycle/src/policy.rs:52-58, and IsolationSession rejects every non-Blockpolicy inisolation_session/common/src/policy.rs:70-79, so these requests open no boundary but still emit a forged-lookingSECURITYaudit entry before backend validation fails. Gate the warning to backends that can actually honorAllow.
if policy.default_network_policy == NetworkPolicy::Allow {
src/core/wxc_common/src/config_parser.rs:812
- This unconditional audit claim also fires when
allowedHostsdoes not relax anything. WithdefaultPolicy=allow, LXC appends a terminal ACCEPT after the host rules, WSLC explicitly ignoresallowedHostsin Allow mode, and Hyperlight/Seatbelt can only narrow or leave an already-open policy unchanged. Conversely, Bubblewrap's default capabilities mode makes an allowlist open the entire unfiltered host namespace, but this generic message does not disclose that severity. Gate this on the effective backend/default-policy combination and report the fail-open cases explicitly so eachboundary relaxedline describes the sandbox that will actually run.
if !policy.allowed_hosts.is_empty() {
logger.warning_line(&format!(
"{P} network.allowedHosts ({} host(s))",
policy.allowed_hosts.len()
));
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This PR adds deterministic security warnings for sandbox policy settings that relax secure defaults and exposes them through CLI and Rust SDK warning channels. Details * Evaluate effective network, UI, ProcessContainer, and Seatbelt relaxations for the selected backend. * Suppress inert and parser-injected settings, sanitize caller-controlled values, and use stable wire names in audit output. * Preserve parser warnings on SandboxRequest and merge them with backend and spawn-time warnings without duplicates. Tests * `cargo fmt --all -- --check` passed. * `cargo check --workspace --all-targets` passed. * `cargo clippy --workspace --all-targets -- -D warnings` passed. * `cargo test --workspace` passed: 2,003 tests across 80 suites; 24 host-dependent tests ignored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c98a35b9-0503-4e95-9f0c-b2a0d7d13174
ea3e437
3f543f5 to
ea3e437
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (5)
src/core/wxc_common/src/config_parser.rs:796
- This emits a “boundary relaxed” audit event for Windows Sandbox and IsolationSession even though both backend validators reject
defaultPolicy=allow; no relaxed sandbox can run. Gate this warning off for those backends so the audit stream describes effective policy rather than a rejected request.
if policy.default_network_policy == NetworkPolicy::Allow {
src/core/wxc_common/src/config_parser.rs:850
- Proxy support is not cross-backend: LXC never consumes
network_proxy, and Hyperlight, MicroVM, Windows Sandbox, and IsolationSession reject it. This unconditional line therefore claims an effective relaxation for ignored or rejected policy. Restrict the warning to backends that actually implement proxy handling (and account for ProcessContainer contract/version constraints).
if policy.network_proxy.is_enabled() {
src/core/wxc_common/src/config_parser.rs:808
- With
defaultPolicy=allow,allowedHostsdoes not open a boundary: Hyperlight/NanVix turn it into a restrictive allowlist, while backends such as LXC already allow the unlisted destinations via their default action. The unconditional warning therefore labels a tightening or no-op as a relaxation; only emit it under the deny-by-default policy.
if !policy.allowed_hosts.is_empty() {
src/core/mxc_engine/src/policy.rs:702
policy_warningsis cached before the request's public policy mutators run.set_seatbelt_extra_mach_lookupsandset_seatbelt_keychain_accessmodifyinnerafterward, so enabling either on a secure request produces no SDK warning, while disabling a previously enabled setting can leave a stale warning. Recompute warnings from the final request at spawn time, or keep this cache synchronized in every boundary-affecting setter.
/// Security warnings emitted while the policy was parsed and validated.
/// Spawn-time warnings are merged with these before the public SDK sees the
/// resulting sandbox handle.
pub(crate) policy_warnings: Vec<String>,
src/core/wxc_common/src/config_parser.rs:866
- This reports an inert setting on ProcessContainer when
ui.disableis true:resolve_ui_restrictionsunconditionally blocks both clipboard directions in that state. Seatbelt does honor clipboard independently, so keep the unconditional behavior only there and require UI to be enabled on ProcessContainer.
if honors_ui && policy.ui.clipboard != ClipboardPolicy::None {
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This PR adds deterministic security warnings for sandbox policy settings that relax secure defaults, making weakened boundaries visible through CLI diagnostics and Rust SDK warning results. It changes logging only; policy defaults and enforcement remain unchanged.
Details
Logger::warning_lineso successful CLI runs cannot silently discard them.SandboxRequestand merge them with backend and spawn-time warnings forSandbox::warnings()andOutput::warnings().Tests
cargo fmt --all -- --check,cargo check --workspace --all-targets, andcargo clippy --workspace --all-targets -- -D warningspassed.cargo test --workspacepassed: 2,003 tests across 80 suites; 24 host-dependent tests ignored.wxc-execverification confirmed relaxed policies emit the expectedSECURITY: boundary relaxed:lines without--debug, while secure-default and learning-mode-only policies stay silent.Related pull requests
Stack, merge bottom-up:
seatbelt.profileOverridein shipped builds