Permissive Learning Mode 3/6 Config Generation

[lilybarkley-msft]

📖 Description

PR 3 of 6 — stacked on PR2. Adds config generation.

• Adjusted_<name>.json writer next to the captured trace (or override path)
• resolve_adjusted_config_path + save_adjusted_config
• Detection-summary writers (no UI fields yet — added in PR5):
  • write_added_paths_summary
  • write_detection_summary — per-path / per-mask grouping
  • write_requested_capabilities_summary
• Access-mask decoder + classifier (decode_access_mask / classify_mask)
• merge_capabilities stub that errors on non-empty input — PR4 fills in the real body. The stub keeps wiring intact without silently dropping findings if a stray caller appears mid-stack.

stop is wired to produce a full Adjusted_<name>.json + detection summary for filesystem deltas.

🔗 References

• Base: PR2 (user/lilybarkley/plm-pr2-fs-extraction)
• Next: PR4 — capability extraction (replaces merge_capabilities stub + adds DACL ACE blob decoding)

🔍 Validation

• cargo build -p plm --target x86_64-pc-windows-msvc — clean
• cargo fmt --all -- --check — clean
• cargo clippy -p plm --target x86_64-pc-windows-msvc --all-targets -- -D warnings — clean
• cargo test -p plm --target x86_64-pc-windows-msvc — 37 passed (test count steady; PR3 changes are write-paths exercised end-to-end in PR4+).

✅ Checklist

• Signed the Contributor License Agreement
• Linked to an issue
• Updated documentation (if applicable)
• Updated Copilot instructions (if build, architecture, or conventions changed)

📋 Issue Type

• Bug fix
• Feature
• Task

---

GitHub Actions runs the PR validation build automatically. The ADO pipeline
(MXC-PR-Build) is the official build pipeline that signs the binaries; it
runs on merge to main and nightly, and Microsoft reviewers can trigger it
on a PR with /azp run. See docs/pull-requests.md.

Microsoft Reviewers: Open in CodeFlow

[MGudgin]

save_adjusted_config writes the adjusted config non-atomically: serde_json::to_string_pretty followed by std::fs::write. std::fs::write opens the destination with truncate, so the existing file's contents are destroyed before the new bytes are fully written. If the process is killed, the disk fills, or the write otherwise fails partway through, the previously valid Adjusted_*.json is left truncated or partially written. This matters here because plm stop can be re-run against the same --adjusted-config-path, and because a downstream enforcing run consumes this file directly as its policy — a corrupt or truncated config could fail to parse or, worse, parse into a weaker policy than intended.

Please write atomically: serialize to a temporary file in the same directory as the destination, flush it, then rename it over the destination so readers only ever observe the complete old file or the complete new file. On Windows a plain rename onto an existing path fails, so use a replace that tolerates an existing target (e.g. std::fs::rename after removing the temp-vs-dest race, or a ReplaceFile-based helper). Keeping the temp file on the same volume ensures the rename is atomic rather than a copy.

Suggested shape:

pub fn save_adjusted_config(config: &Value, path: &Path) -> Result<()> {
let pretty = serde_json::to_string_pretty(config)?;
let dir = path.parent().unwrap_or_else(|| Path::new("."));
let mut tmp = tempfile::NamedTempFile::new_in(dir)
.with_context(|| format!("failed to create temp file in {}", dir.display()))?;
tmp.write_all(pretty.as_bytes())
.and_then(|_| tmp.as_file().sync_all())
.with_context(|| "failed to write temp adjusted config")?;
tmp.persist(path)
.map_err(|e| e.error)
.with_context(|| format!("failed to replace {}", path.display()))?;
Ok(())
}

(If you'd rather not add the tempfile dependency, a hand-rolled unique temp name in the same directory plus an atomic replace works too.)

[MGudgin]

esolve_adjusted_config_path takes the caller-supplied --adjusted-config-path verbatim (plumbed from stop.rs:204), creates any missing parent directories (line 522, and the create_dir_all error is swallowed), and returns it unchanged. save_adjusted_config then truncates and writes that path with std::fs::write (config.rs:537), which follows symlinks/junctions and does no owner/type/link check on the target.

Because plm.exe runs as admin, this write happens with admin privileges, which makes it a create-directories-and-overwrite-file primitive whose destination is an unvalidated, caller-named path.

Through the intended --audit flow this is not currently caller-controlled: wxc-exec never passes --adjusted-config-path, so plm defaults the output into its own random-suffix log dir. But --adjusted-config-path is a public plm stop flag (main.rs:255-256, "Override for the adjusted config output path"), so plm stop --adjusted-config-path writes attacker-controlled JSON to any admin-only location. A pre-planted symlink/junction in a writable output directory can also redirect the write elsewhere (there's a TOCTOU between the line 521 parent.exists() check and the line 537 write).

Worth calling out: plm_launch.rs already hardened the elevated child's capture files against exactly this class of problem — it creates the temp dir with a 16-byte random suffix, opens the log files with create_new(true), and explicitly refuses to pre-materialize them so a same-user medium-IL attacker can't pre-squat or symlink-redirect them. That same rigor just wasn't applied to the adjusted-config output path, so the two admin-level write surfaces are inconsistent.

Suggested changes:

• Prefer deriving the adjusted-config path inside the plm-created log dir (as the default already does) and drop or tightly constrain the arbitrary --adjusted-config-path override. If an override must stay, resolve it and reject anything that escapes an allow-listed root.
• Write via the same hardened pattern used for the capture files: create the destination directory with a known-safe owner, and open the target with create_new / no-follow semantics anchored to a verified directory handle, so a pre-planted symlink or junction can't redirect the write and an unexpected existing target is detected rather than silently clobbered. Pair this with the atomic temp-file-plus-rename fix from the reliability comment.
• Don't silently create parent directories for a caller-supplied path (line 522 also discards the create_dir_all error); a missing parent usually means the path is wrong.

Severity: High given plm runs as admin and the override path is public; lower only if you decide plm's output path will never be caller-influenced — in which case please remove/constrain the override so that property is enforced rather than assumed.

[MGudgin]

These two new lines are what makes a pre-existing, purely-lexical deny check start to matter, so I'm raising it here even though the matching logic itself lives in config.rs and isn't changed by this PR.

Up to this point, the promoted filesystem paths in config only existed in memory. These lines persist that merged config to disk as Adjusted_.json, and that file is then consumed as policy by the subsequent enforcing run. So any weakness in how paths were selected for promotion is now baked into a durable artifact rather than being a transient in-memory result.

The relevant weakness: the promotion decision in config::update_from_access_events compares config::normalize_path(event_path) against the denied set using a normalized string-prefix check. normalize_path is deliberately filesystem-free — it strips verbatim/device prefixes, lowercases, collapses separators, and rejects ADS and . / .. segments, but it never resolves symlinks, directory junctions/reparse points, or 8.3 short names. So an access that reaches a denied location through an alias won't match the deny entry and gets promoted into filesystem.readwritePaths / readonlyPaths. For example, with C:\Secrets denied, an event that arrives as an 8.3 name, or via a junction/symlink such as C:\work\link -> C:\Secrets (C:\work\link\token.dat), normalizes to something that doesn't start with c:\secrets and is therefore granted. The result written on line 205 then grants access the operator explicitly tried to deny.

I'm flagging it here because the rest of normalize_path is carefully fail-closed (it rejects . / .. precisely so a canonical-parent deny can't be dodged) — this alias case is the one canonicalization gap left open, and persisting the config is what turns it from theoretical into a shipped policy. Whether it's exploitable end-to-end depends on how the enforcing backend keys its own allow/deny (literal path vs final object identity), so you're better placed to judge severity.

Options, roughly in order of preference:

• Before persisting (i.e. before the merge that feeds these lines), resolve event paths and denied paths to a canonical object identity (canonicalize / final-path-by-handle, resolving reparse points and short names), and keep the existing fail-closed behavior when resolution fails so an unresolvable event is never promoted.
• If full canonicalization on the hot path isn't feasible (paths come from ETW and the object may no longer exist), at minimum skip promotion for any path that traverses a reparse point or contains an 8.3 component, and document that deny is enforced on canonical paths only.
• Either way, add regression tests for a junction, a symlink, and an 8.3 short-name variant of a denied directory, mirroring the existing trailing-dot / ADS deny-bypass tests.

If this gap is accepted for now, please at least document it explicitly next to the deny-matching code and in the PLM readme so it's a known, stated limitation rather than an implicit one — otherwise the Adjusted_*.json written on line 205 looks authoritative but can quietly encode a deny bypass.

[MGudgin]

Two related problems with resolve_adjusted_config_path — a design seam that blocks testing, and the untested edge cases hiding behind it. (The security/hardening angle on the caller-supplied override path is covered separately; this comment is just about correctness and testability of the resolver itself.)

Seam: the function looks pure — name, Path -> PathBuf signature, and the default branch are all path math — but the override branch mutates the filesystem via std::fs::create_dir_all(parent) on line 522. That side effect means you can't unit-test the resolution logic without it creating real directories, so in practice it goes untested (it has no unit tests today). The create_dir_all result is also discarded with let _ = , so a failed parent creation is silent and only surfaces later as a vaguer "failed to write" from save_adjusted_config. Fix: make this function pure — compute and return the PathBuf only — and move directory creation to stop.rs just before save_adjusted_config, with proper error propagation (with_context). A pure resolver is then trivially table-testable.

Edge cases the missing tests would catch, all in the default (non-override) branch:

• Empty file_name: if dest_config has no final component (a directory or a root like C:), file_name() is None and unwrap_or_default() yields "", so the result is a file literally named Adjusted_ .
• Root parent fallback: if dest_config is a root (C:), parent() is None and the unwrap_or_else(|| Path::new(".")) fallback resolves it to .\Adjusted_ in the current working directory rather than anywhere near the input — a silent, surprising relocation.
• Override with no parent: if the override path is itself a root, p.parent() is None, the create block is skipped, and save_adjusted_config later fails trying to write file contents to a directory.

In normal operation dest_config is a real file inside the plm log dir, so these don't fire today — but they're exactly the kind of thing a future caller (or the --adjusted-config-path override) trips, and the behavior is currently undefined-by-accident rather than by design.

Suggested change:

• Split responsibilities: resolve_adjusted_config_path becomes pure; stop.rs creates the parent dir (propagating errors) before save_adjusted_config.
• Validate the inputs instead of unwrap_or_default() / Path::new(".") fallbacks: require dest_config to have a file_name (error otherwise), and don't silently rebase a rootless/parentless path onto the CWD.
• Add table-driven unit tests for the override path, the derived Adjusted_ path, a dest_config with no file_name, and a root-only dest_config / override, asserting the returned PathBuf (now possible once the function is pure).

[MGudgin]

This save can silently destroy the input snapshot that lines 161-174 went out of their way to preserve. Line 172-174 copy the original config to dest_config = log_dir/ (e.g. log_dir/config.json), and the comment above it explicitly states that copy is the operator's only record of the pre-edit state and "losing it turns an Adjusted_*.json into an un-auditable delta." But line 204 resolves the adjusted-output path from that same dest_config, and line 205 writes to it with no check that the two paths differ.

In the default case they don't collide — resolve_adjusted_config_path derives Adjusted_, so log_dir/config.json vs log_dir/Adjusted_config.json. The problem is the override: opts.adjusted_config_path is passed straight through, so --adjusted-config-path /config.json (or any path that resolves to dest_config) makes line 205 overwrite the snapshot with the edited config. The snapshot and the adjusted output become the same file, and the pre-edit record the code promised to keep is gone — exactly the failure mode the comment warns about, just reachable via the override instead of by accident.

Suggested fix: enforce the documented invariant rather than assuming it. After resolving on line 204, reject (or redirect) an adjusted path that equals dest_config:

let adjusted = resolve_adjusted_config_path(&dest_config, opts.adjusted_config_path.as_deref());
if same_file(&adjusted, &dest_config) {
anyhow::bail!(
"adjusted config path {} would overwrite the input snapshot {}",
adjusted.display(),
dest_config.display()
);
}
save_adjusted_config(&config, &adjusted)?;

Compare canonically, not just as strings, so ./config.json, C:...\config.json, and 8.3/symlinked spellings of the same file are all caught (std::fs::canonicalize on both, or a same-file identity check; fall back to a normalized-path compare if either doesn't exist yet). A regression test that passes --adjusted-config-path equal to the copied input path and asserts the snapshot still contains the original bytes would lock this in.

[MGudgin]

per_path.entry(ev.file_path.clone()) on line 604 heap-allocates and copies the path string for every access event, even though the map only needs one entry per unique path. events is a borrowed slice (&[LearningModeAccessEvent]) and the map is a short-lived local that's dropped at the end of write_detection_summary, so the keys can just borrow from the events rather than own copies. As written, an .etl with, say, 200k events over a few hundred unique paths does 200k string allocations to build a few-hundred-entry map.

Fix is a one-liner — make the map key a &str tied to the events borrow:

let mut per_path: BTreeMap<&str, u32> = BTreeMap::new();
for ev in events {
*per_path.entry(ev.file_path.as_str()).or_insert(0) |= ev.access_mask;
}

The downstream println! loop over &per_path works unchanged (path is then &&str, still Displayable). This drops the per-event allocation to zero and the map to one small allocation.

Severity note: this is a one-shot CLI over a trace file, not a hot server path, so the practical impact is bounded and I wouldn't block merge on it — but the fix is trivial and free of downsides, so worth taking now. (The same borrow-instead-of-clone idea doesn't apply to the clones at lines 436/444/479/485, where the strings are pushed into an owned serde_json Value / the returned AddedPaths and genuinely need ownership — leave those as-is.)

[MGudgin]

A few small, related issues in the access-mask helpers. Grouping them because they share one root cause: the bit-to-meaning mapping is expressed three separate times.

1. Duplicated source of truth. WRITE_MASK (lines 24-31) and READ_MASK (lines 41-46) enumerate the recognized bits for classification, while the NAMED table here (line 545) re-lists the same bits for decoding. Nothing keeps them in sync — add a bit to policy handling but forget the NAMED table and it silently prints as OTHER(0x…) despite being classified and merged as known access, and vice versa. Consider driving both from one table, e.g. a single &[(u32, &str, Access)] where Access is Read/Write, and derive READ_MASK/WRITE_MASK and the decode names from it. Then a new bit is added in exactly one place.
2. Generic rights fall through both. GENERIC_READ (0x80000000), GENERIC_WRITE (0x40000000), GENERIC_EXECUTE (0x20000000), and GENERIC_ALL (0x10000000) are in neither mask nor the NAMED table. So decode_access_mask reports them as OTHER(0x…), classify_mask (line 585) buckets a generic-only event as "-", and — more than cosmetic — because they're absent from READ_MASK/WRITE_MASK, update_from_access_events won't promote a path whose event carried only a generic right. Worth confirming whether EventID=14 can surface generic bits at all (the kernel often maps generic to specific rights before logging); if it can, add the four generic bits to the shared table so they classify and decode correctly, if it can't, a one-line comment saying so would save the next reader the same investigation.
3. SYNCHRONIZE / READ_CONTROL classify as read. READ_MASK includes SYNCHRONIZE_MASK (0x100000) and READ_CONTROL_MASK (0x20000), so classify_mask labels a SYNCHRONIZE-only or READ_CONTROL-only event as "R" even though neither grants data read. That inflates the printed classification (and, via READ_MASK, could promote a path to readonly for an access that never actually read content). If these bits are only meant to be recognized-but-not-access-granting, they probably shouldn't live in READ_MASK; at minimum the summary shouldn't call them R.
4. No tests. decode_access_mask and classify_mask are pure u32 -> string functions with clear branch structure (NONE, single known bit, multiple known bits, unknown-only OTHER(0x…), known+unknown, and each of RW/W/R/-), but have no unit tests. A small table-driven test would lock the decoding/classification down and would immediately catch the divergence risk in point 1.

All Low severity — the output is primarily diagnostic — but points 2 and 3 do leak into the actual promotion masks, so they're worth a look rather than pure cosmetics.

[Copilot]

Pull request overview

Adds PLM adjusted-config generation from captured filesystem access events.

Changes:

• Writes atomic Adjusted_<name>.json outputs.
• Adds access-mask decoding and detection summaries.
• Removes arbitrary output-path overrides and documents behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/host/plm/src/stop.rs	Wires parsing, merging, and config persistence.
src/host/plm/src/main.rs	Updates stop CLI options.
src/host/plm/src/config.rs	Adds mask handling, summaries, and atomic writing.
src/host/plm/readme.md	Documents config generation and limitations.
src/host/plm/Cargo.toml	Adds tempfile.