microsoft / microsoft/hve-core

docs(skills): add a STRIDE security model for hve-core-installer and verify its cross-platform containment

Open
#2,693 1 comment 0 reactions 1 assignee Claimed by @WilliamBerryiii View on GitHub
documentation needs-triage priority-2 security skills testing
Dominant language
Python
Stars
1.5k
Forks
301
Avg merge
3d 3h
Merged PRs (30d)
92

Description

## Summary

The `hve-core-installer` skill has no `SECURITY.md`. It ships 16 scripts that write files into a user's repository, and it is the only skill of comparable blast radius without a threat model. 11 other skills have one.

This issue authors that model **and** closes the cross-platform verification gaps it would otherwise have to record as unknowns. Doing both in one pass means the skill is opened once and the model ships with verified platform claims rather than placeholders.

Work items:

1. Verify Windows and Linux containment behaviour (below) — do this first, so the model states facts.
2. Author `SECURITY.md` for the skill.
3. Register it in the repo-wide security model and documentation index.
4. Correct the stale powerpoint row while in that table.

## Why this matters concretely

During PR #2599, a reviewer reported a symlink-traversal bug in this skill's `component-copy` scripts: the destination containment check passed while writes landed outside the target root. Triaging it was slower than it should have been because there was no documented threat model to measure the finding against.

The comparison is instructive. The `powerpoint` skill had an analogous weakness, but its `SECURITY.md` already recorded `G-EOP-1` stating the control was not a sandbox. That single documented gap made triage fast and turned an alarming report into a known accepted risk. The installer had no such anchor, so the same class of finding had no owner and no baseline.

## Scope

Skill root: `.github/skills/installer/hve-core-installer/`

16 scripts as 8 mirrored PowerShell and Bash pairs: `component-copy`, `collision-detection`, `detect-environment`, `eject`, `file-status-check`, `upgrade-detection`, `validate-extension`, `validate-installation`.

## Required structure

Follow `.github/instructions/skill-security-model.instructions.md` exactly — it is enforced. **Read it first.** Use `.github/skills/experimental/powerpoint/SECURITY.md` as a shape reference.

Two conformance details that are easy to get wrong:

- Gap IDs are `G-{TOKEN}-{N}`, scoped per file, where `{TOKEN}` is STRIDE-aligned: `SPF`, `TAM`, `REP`, `INF`, `DOS`, `EOP`, plus `SUP` (supply chain) and `TLS` (transport). Do **not** invent skill-letter or topic prefixes such as `A-` or `SSRF`.
- The `Severity` column takes a bare `{Category}-{Level}` token, for example `TAM-Med`. Qualifiers belong in the Gap or Status prose, not the Severity cell.

## Content that must be covered

- The write path into a user's repository, and the containment control now implemented (`Assert-WithinTargetRoot` / `assert_within_target_root`: normalize, compare separator-terminated, walk ancestors rejecting reparse points).
- The residual TOCTOU window. Containment is re-verified immediately before the write, which **narrows but does not eliminate** the check-to-write race. Do not describe it as atomic.
- The eject path and the sha256 `managed` / `ejected` manifest status in `.hve-tracking.json` — what integrity property it does and does not provide.
- Cross-platform divergence: the PowerShell and Bash implementations must stay behaviourally equivalent, and Windows reparse points include junctions, not only symlinks.
- Platform coverage. State which platforms the containment claims were verified on. macOS remains unverified — see the residual-gap note in the verification section.

## Also required: register the model repo-wide

Authoring the file is not sufficient. `docs/security/security-model.md` states that a new skill model must be registered **in that table and in the security documentation index**. Two edits outside the skill directory:

1. Add an `hve-core-installer` row to the Skill Security Models table in `docs/security/security-model.md`. Columns are Skill, Runtime surface, Primary residual gaps, Security model. The link is an absolute `https://github.com/microsoft/hve-core/blob/main/...` URL, not a relative path — match the existing rows.
2. Add the matching entry to `docs/security/README.md#skill-security-models`.

### While in that table: the powerpoint row is stale

PR #2599 changed powerpoint's `content-extra.py` execution from unconditional-with-a-denylist to opt-in, and rewrote its `SECURITY.md` accordingly. **The registry row was never updated** and still reads:

> Runtime surface: `Sandboxed content-extra.py execution; ...`
> Primary residual gaps: `Denylist confinement is not OS-level; external-parser CVE exposure`

That contradicts the skill's own `SECURITY.md`, which no longer contains the word "denylist" at all and states that opt-in authorization, not the lint, is the control. The instructions require updating this table when a skill's runtime surface materially changes; that step was missed.

Correcting it is a two-cell edit in the same table this issue already touches, so it is folded in here rather than filed separately. Split it out if you would rather keep the scopes clean.

## Warning from the last round

The `powerpoint` `SECURITY.md` previously claimed its denylist "constrain[ed] an author script's ability to exfiltrate host data" while `pathlib` and `open` were never blocked. The document asserted a control that did not exist. Describe what the code does, not what it was intended to do. The stale registry row above is the same failure one layer up.

## Cross-platform verification — do this first

Two containment claims were tested only on Linux/WSL. Verifying them before writing the model means the model states facts instead of unknowns.

PR #2599 fixed a destination-containment bug in `component-copy.ps1` and `component-copy.sh`. Two deliberate cross-platform choices were made:

- PowerShell uses `FileAttributes.ReparsePoint`, chosen because it covers Windows **junctions as well as symlinks**, and reports POSIX symlinks under PowerShell 7.
- Bash uses an ancestor `-L` walk, deliberately avoiding `realpath -m` because `-m` is a GNU coreutils extension not dependable on macOS.

### Windows — native host required, not WSL

**W1.** Does the check refuse a directory **junction** at the destination, not just a symlink?

Run `.github/skills/installer/hve-core-installer/tests/component-copy.Tests.ps1`. The test `Refuses a component whose destination parent is a directory junction` currently **skips** on Linux. Confirm it runs and passes on Windows.

⚠️ Known gotcha, already cost time once: `New-Item -ItemType Junction` on Linux **emits a success record while creating nothing** — a following `Get-Item` throws `ItemNotFoundException`. The skip guard therefore verifies the item exists *and* carries `ReparsePoint` rather than relying on `catch`. Confirm that guard does not wrongly skip on Windows.

**W2.** Does Git for Windows materialize cloned symlinks as real links, or as plain text files? Default `core.symlinks=false` suggests the latter. **If symlinks do not materialize, the original attack's primary delivery vector does not reproduce on a default Windows install**, which would lower the assessed severity. Determine this empirically and record it as a severity qualifier in the model.

### Linux and WSL — confirmatory only

WSL is Linux, and the Linux path is what the original fix was verified on. Re-running there confirms no regression but adds no new platform coverage. Run `npm run test:ps -- -TestPath .github/skills/installer/hve-core-installer/tests/component-copy.Tests.ps1` and confirm the suite still passes with the junction case skipping cleanly.

### macOS — residual gap, do not claim coverage

Neither a Windows box nor WSL exercises the BSD-versus-GNU coreutils difference that motivated the Bash design. macOS therefore stays unverified and must be recorded as a `G-TAM-*` gap in the model.

Assessed risk is low, and the reason is worth stating in the model: the Bash implementation deliberately avoids `realpath -m` and uses a POSIX-portable ancestor `-L` walk, so there is no known GNU-specific dependency to break. That is a design argument, not evidence. Do not upgrade it to a verified claim without running it.

### Optional, and worth considering

Every CI runner in this repository is `ubuntu-latest`. The junction test therefore **skips in CI permanently** and can only ever pass on a developer's Windows machine. If Windows containment behaviour is worth protecting against regression, a `windows-latest` job for this Pester suite is the only thing that will catch a future break. Out of scope here; raise it separately if you agree.

## Acceptance Criteria

- [ ] W1 and W2 each have a verdict — confirmed, refuted, or still unverified — with the exact commands and observed output.
- [ ] `SECURITY.md` exists at the skill root and conforms to `skill-security-model.instructions.md`.
- [ ] Every control claim is verified against the actual scripts, not inferred from documentation.
- [ ] Verified platforms are named explicitly; macOS is recorded as a `G-TAM-*` gap rather than asserted.
- [ ] If W2 shows Windows does not materialize cloned symlinks, the model records that as a severity qualifier.
- [ ] The skill is registered in `docs/security/security-model.md` and `docs/security/README.md#skill-security-models`.
- [ ] The stale powerpoint row is corrected, or split into its own issue with a link recorded here.
- [ ] Any genuine defect found during verification is either fixed or filed separately.
- [ ] `npm run validate:skills`, `npm run lint:md`, and `npm run lint:tables` pass. Run `npm run format:tables` after adding tables.

## Constraint

Reproduce; do not reason from documentation. Every claim in this work has been established by execution and that standard should hold. Where a platform is unavailable, record the gap rather than substituting inference.

Verified against `main` at `278eb128`.

## Related

- Related: #2663 (installer script hardening) — complementary scope; that issue lists duplicating target-root containment checks as an explicit non-goal and requires the existing containment, symlink, and parity tests to keep passing.
- Depends on: #2664 (open pull request for #2663) — run the verification steps after it merges, so results describe the merged installer rather than a superseded one.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.