Permissive Learning Mode 2/6 filesystem extraction#585
Permissive Learning Mode 2/6 filesystem extraction#585lilybarkley-msft wants to merge 9 commits into
Conversation
2d73df4 to
ebc5420
Compare
cdd12e8 to
673a3bc
Compare
87f340d to
80667ed
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
c39b649 to
3d9c67a
Compare
There was a problem hiding this comment.
Pull request overview
Adds the filesystem-extraction slice of Permissive Learning Mode (PLM): capturing ETW/WPR traces, decoding EventID=14 access-failures into file-path + access-mask findings, and merging those findings into filesystem.{readwritePaths, readonlyPaths}. It also wires PLM into wxc-exec --audit (Windows-only), including elevated plm.exe spawning and lifecycle coordination/cleanup.
Changes:
- Introduces the PLM crate (
plm.exe+ library) with WPR profile staging, start/stop/log flows, ETW XML walking, and access-failure decoding. - Implements filesystem path normalization + filtering and merges resulting access events into MXC config JSON.
- Adds
wxc-exec --auditintegration: injectpermissiveLearningMode, spawn elevatedplm.exe start/stop, and coordinate cleanup via shared primitives.
Reviewed changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/host/plm/src/wpr_path.rs | Safe absolute-path resolution for wpr.exe (avoids CWD/PATH/env spoof). |
| src/host/plm/src/stop.rs | plm stop flow: stop trace, parse ETL, merge FS findings into config in-memory. |
| src/host/plm/src/start.rs | plm start WPR trace start + cancel-and-retry logic, plus failure replay. |
| src/host/plm/src/profile_gen.rs | Embeds/stages plm.wprp next to the executable with atomic write. |
| src/host/plm/src/main.rs | Windows-only CLI entrypoint with singleton + ctrl-handler + subcommand dispatch. |
| src/host/plm/src/log.rs | Interactive plm log start/stop and blank-config merge preview. |
| src/host/plm/src/lib.rs | Exposes PLM modules; gates Windows-only modules. |
| src/host/plm/src/event_parser.rs | ETW event enumeration/rendering + parse accumulator + dispatch plumbing. |
| src/host/plm/src/coordination.rs | Shared singleton, ctrl-handler wait helper, and bypass signaling. |
| src/host/plm/src/config.rs | JSON config load + filesystem path normalization/filtering + merge logic. |
| src/host/plm/src/access_failure.rs | EventID=14 decode: normalization + filters + per-event accumulation. |
| src/host/plm/src/access_event.rs | LearningModeAccessEvent data model. |
| src/host/plm/readme.md | PLM documentation (how it works, CLI, layout, limitations). |
| src/host/plm/Cargo.toml | Adds new plm crate (deps + windows-gated deps + build script). |
| src/host/plm/build.rs | Embeds version info + admin manifest for release Windows builds. |
| src/core/wxc/src/plm_launch.rs | UAC-elevated spawning wrapper for plm.exe with captured stdio. |
| src/core/wxc/src/main.rs | Adds --audit/--audit-verbose, audit lifecycle wiring, and cleanup integration. |
| src/core/wxc/src/audit.rs | Implements --audit lifecycle state, cleanup guards, and plm invocation. |
| src/core/wxc/Cargo.toml | Adds plm + tempfile dependencies for audit integration. |
| src/core/wxc_common/src/models.rs | Adds ExecutionRequest.audit to carry --audit intent. |
| src/core/wxc_common/src/config_parser.rs | Adjusts permissiveLearningMode handling/logging in debug builds; initializes audit=false. |
| src/Cargo.toml | Adds host/plm workspace member; adds shared deps (chrono, roxmltree). |
| src/Cargo.lock | Locks new plm and roxmltree deps; adds tempfile dep to wxc. |
| src/backends/appcontainer/common/src/appcontainer_runner.rs | Enforces permissiveLearningMode in release only when request.audit is set. |
| sdk/tests/integration/test-helpers.ts | Allows plm.exe as an optional packaged binary. |
| sdk/tests/integration/package-lock.json | Updates integration test lockfile versions for local SDK link. |
| README.md | Documents wxc-exec --audit and links to PLM docs with release-build warning. |
| build.bat | Builds/stages plm.exe, adds --with-bfs, and forces SDK integration test relink. |
| .github/workflows/Build.Linux.Job.yml | CI gate to build/test plm cross-platform (stub + portable modules). |
| .azure-pipelines/templates/Rust.Build.Job.yml | ADO CI runs cargo test -p plm on Windows x64. |
Files not reviewed (1)
- sdk/tests/integration/package-lock.json: Generated file
pr1 was merged upstream without these follow-up fixes, so they move here (pr2) instead: - Deduplicate wpr path resolution: audit.rs now uses plm::wpr_path::wpr_command - Deduplicate wpr stop: log.rs now uses stop::stop_plm_trace_with - Fix run_plm_command docstring to describe capture+conditional replay - Remove orphaned plm.exe docstring from wxc main.rs import - Rewrite plm-dep comment in wxc Cargo.toml to reflect actual imports - Fix readme link: base-process-container/ -> process-container/ - Remove out-of-scope Stop options (bin_path, adjusted_config_path, verbose_logging) that pr1 was carrying prematurely; pr2's own filesystem-extraction commit reintroduces them where they belong. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Walks EventID=14 records from the captured .etl, decodes file paths through normalization + post-XPath filters, and merges them into ilesystem.readwritePaths / ilesystem.readonlyPaths on an in-memory copy of the input config. The Adjusted_*.json writer arrives in the next PR. New modules: - event_parser: EvtQuery/EvtRender walk + ParseAccumulator dispatch - access_failure: EventID=14 decoder + path normalization - access_event: LearningModeAccessEvent plain struct - config: WRITE/READ masks, filesystem init, update_from_access_events Wired stop.rs and log.rs to invoke the FS merge pipeline. Capability ACE-blob extraction, EventID=27 UI relaxation, the adjusted-config writer, and merge_capabilities arrive in subsequent PRs. 37 tests pass; cargo fmt + clippy clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the `std::fs::copy(config_path, &dest_config)` above the
parse.is_empty() early return so operators always have a
{trace.etl, <input>.json} pair in the log directory, even when
the parse yielded nothing mergeable.
Previously the config copy sat below the bail-out, so an empty
parse left a bare trace.etl in log_dir with no way for the
operator to correlate the trace with the input that produced it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Load the source config into memory BEFORE the copy-to-log_dir
side effect, and edit the pre-loaded Value rather than
re-reading dest_config after the copy.
This eliminates a Windows read-after-write hazard in the
previous flow (copy source -> dest_config, then load_config on
dest_config): an AV filter can occasionally serve a stale or
empty buffer for a file that std::fs::copy just wrote, producing
intermittent 'failed to parse JSON' errors that manifested as
test instability under load.
Reordering also gives the operator a stronger invariant: if
plm.exe reached the point where it would touch log_dir, the
input snapshot is guaranteed on disk before any edit is
attempted, so an interrupted run always leaves a coherent
{trace.etl, <input>.json} pair rather than a bare trace.etl
next to a half-loaded config.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3d9c67a to
a4308f6
Compare
- Fix config.rs module doc: capability-merge and Adjusted_*.json writer arrive in later PRs, not pr2 - Fix resolve_bin_path docstring to describe self-access filter consumption (was: 'exposed for tests only') - Fix readme: --config-path drives filesystem merge today (was described as fully deferred); log interactive mode is implemented - Restore stop --bin-path / --adjusted-config-path / --verbose-logging clap args now that pr1 (correctly) does not carry them Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- config.rs self-access filter comment: reword 'wxc-exec binary' to 'audited application binary'; bin_path is the --bin-path arg (or the plm.exe dir fallback), not wxc-exec's own path. - config.rs parent_for_write verbose branch: message was a stale PowerShell-parser artifact claiming 'Only files and directories are currently supported'. In Rust the branch only fires when the path has no final component; reword to reflect that. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1) event_parser::render_event_xml: change backing buffer from Vec<u8> to Vec<u16> so the *const u16 cast for from_raw_parts is always properly aligned (was UB even though it worked on x86). Treat EvtRender's BufferSize/BufferUsed as byte counts explicitly at the Win32 boundary. 2) stop::WprExeStopper::stop: pass the trace_file Path to wpr -stop directly via Command::arg(&Path) instead of round-tripping through to_string_lossy() (which would silently swap in U+FFFD for non-Unicode path bytes). 3) wpr_path::resolve_wpr_path: fix truncation check to use >= buf.len() per GetSystemDirectoryW's Win32 contract, and retry once with the required size rather than hardcoding a C:\\Windows\\System32\\wpr.exe fallback (which was wrong on any non-C: Windows install). Return Option<PathBuf> and cache it. 4) wpr_path: rename verify_wpr_signed -> verify_wpr_present. The function only checks is_file(); the old name suggested a signature verification we deliberately do not perform (see the module doc for the security rationale). Update the caller in main.rs. 5,6) config_parser::merge_appcontainer_config: replace debug-only eprintln! calls in the learningMode and permissiveLearningMode paths with logger.log(...) so the wxc_common library layer stops unconditionally writing to stderr. The CLI already installs a stderr-mirroring Logger; embedders that don't want the stderr side effect can now suppress it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1) src/host/plm/src/main.rs: convert stored blob back to CRLF to
match the file's line-ending convention in origin/main; earlier
edits inadvertently rewrote it as LF, making the PR diff show a
whole-file rewrite.
2) event_parser::render_event_xml: replace unsafe { buf.set_len(0) }
with buf.clear() — for Vec<u16> there is nothing to drop and the
safe API expresses intent more clearly.
4) event_parser::render_event_xml: fix growth calculation on the
ERROR_INSUFFICIENT_BUFFER retry. Vec::reserve(additional)
guarantees capacity >= len + additional, and len is 0 here
(buf was cleared), so passing (needed_u16 - buf.capacity())
under-reserved when needed_u16 was less than 2*capacity and
still under-reserved otherwise. Pass needed_u16 directly.
Reverted (was pr2 commit): --with-bfs build.bat additions; the
AppContainer Tier 2 BFS build flag will be re-introduced
separately per reviewer request.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a4308f6 to
08dbc86
Compare
Drop parent-directory widening on write events; always emit a
file-scope grant for the exact ETW-reported path. Widening from
'C:\a\b\c.txt' to 'C:\a\b' over-granted to unrelated siblings for
no policy-schema benefit (readwritePaths already accepts file
entries).
Keep is_drive_root as a defensive guard: a raw drive-root write
event ('C:\' or verbatim variants) is skipped rather than
promoted, since granting a bare volume was never the intent.
The pre-existing deny check on the raw event path already covers
denied-parent scenarios, so the post-widen deny recheck is
removed alongside parent_for_write.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| } | ||
|
|
||
| // Process Read Requests | ||
| if (ev.access_mask & READ_MASK) != 0 { |
There was a problem hiding this comment.
question: do we need the if is_drive_root clause here for the read as well?
| /// let `CreateProcess` fail cryptically later. | ||
| /// Returns `Err` if `GetSystemDirectoryW` failed outright, or if the | ||
| /// resolved path doesn't exist on disk — both indicate a | ||
| /// broken/stripped Windows install (WPT not present or `System32` |
There was a problem hiding this comment.
note: I think it's suppose to say WPR not present right?
| /// Free-function sibling of `ParseAccumulator::is_skippable` exposed | ||
| /// for unit tests that don't want to build a whole accumulator. Logic | ||
| /// must stay in lock-step with the cached form on `ParseAccumulator`. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
question: rather than have to keep in lock-step, is it too much work to exercise that code path in the tests?
| } | ||
| let new_cap_u16 = buf.capacity(); | ||
| let new_cap_bytes = new_cap_u16 * std::mem::size_of::<u16>(); | ||
| let second = unsafe { |
There was a problem hiding this comment.
issue: missing unsafe comments
| fn to_wide_z(s: &str) -> Vec<u16> { | ||
| s.encode_utf16().chain(std::iter::once(0)).collect() | ||
| } |
There was a problem hiding this comment.
issue: we can probably use the mxc string_utils crate for this.
|
|
||
| /// Decoded XML view of a single event's interesting fields. | ||
| pub(crate) struct ParsedEvent { | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
note: another marked as dead code. Needed?
| for child in ed.children().filter(|n| n.is_element()) { | ||
| let tag = child.tag_name().name(); | ||
| if tag == "Data" || tag == "ComplexData" { | ||
| event_data.push(child.text().unwrap_or("").to_string()); |
There was a problem hiding this comment.
question: should we unwraping into an empty string, is it possible that empty string is actually the text for the child? Might be useful to have "mxc-parse-error" instead.
| .chars() | ||
| .next() | ||
| .map(|c| c.is_ascii_alphabetic()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
note: we're unwrapping into a false here. Would is_drive_root being false due to an error affect code execution outcomes? I'm wondering if it's worth it erroring out here in general.
| /// Hot-path CWD / drive-letter filter for access events. Uses | ||
| /// precomputed lowercase forms of `current_directory` to avoid two | ||
| /// `String` allocs per event. | ||
| pub(crate) fn is_skippable(&self, file_path: &str) -> bool { |
There was a problem hiding this comment.
note: This function would probably be a bit easier to parse if we spelt out the variable names instead of abbreviating them.
| return; | ||
| }; | ||
| match ev.event_id { | ||
| 27 => { |
There was a problem hiding this comment.
issue: should be a static variable with a description in the file rather than a constant integer. This pattern will help if we add more id's.
📖 Description
PR 2 of 6 — stacked on PR1. Adds filesystem extraction to PLM.
EventID=14(access-failure) decoder:LearningModeAccessEventrows (file path + access mask)for_each_event_xml/EvtQuery/EvtRender) with bounded peak memoryParseAccumulator+ per-event dispatcher inevent_parserconfig.rsfoundations: load/parse,filesystem.{readwritePaths,readonlyPaths}merge,deny_file_setstop/logwired to the FS-extraction pipelineCapability extraction, config-generation summaries, and UI policy land in PR3–PR5.
requested_capabilitiesis exposed as an always-empty placeholder so call-sites stay stable across the split.🔗 References
user/lilybarkley/plm-pr1-audit-skeleton)Adjusted_<name>.jsonwrite + detection summaries)🔍 Validation
cargo build -p plm --target x86_64-pc-windows-msvc— cleancargo fmt --all -- --check— cleancargo clippy -p plm --target x86_64-pc-windows-msvc --all-targets -- -D warnings— cleancargo test -p plm --target x86_64-pc-windows-msvc— 37 passed (12 new over PR1; cover path-normalization, post-XPath filters, accumulator dispatch, ETW XML parsing).✅ Checklist
📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the official build pipeline that signs the binaries; itruns on merge to
mainand nightly, and Microsoft reviewers can trigger iton a PR with
/azp run. See docs/pull-requests.md.Microsoft Reviewers: Open in CodeFlow