Skip to content

Enforce install-dir containment on uninstall; resolve manifest verification key - #35

Merged
TeoSlayer merged 2 commits into
mainfrom
sec/findings-skillinject
Jul 26, 2026
Merged

TeoSlayer merged 2 commits into
mainfrom
sec/findings-skillinject

Conversation

@TeoSlayer

Copy link
Copy Markdown
Contributor

Two audit findings in skillinject.

L27 (high) — uninstall omitted the containment guard the install path enforces

reconcilePluginFiles rejects any plugin.files[].name whose joined path resolves outside the plugin install directory. Uninstall did not: it joined the same manifest-supplied name and handed the result straight to removeOwnedFile, so a name like ../../../.ssh/authorized_keys deleted a file the install path had never written.

Both sides now share a pathWithin(dir, path) helper in manifest.go. Out-of-scope names are reported as a RemovalError and skipped. In-scope removals are unchanged.

L28 (high) — manifest verification was opt-in and nil-by-default

Config.ManifestPublicKey had to be supplied by the caller; nil silently skipped Ed25519 detached-signature verification, leaving TLS as the only check on the fetched manifest and repo files.

The key is now resolved in precedence order:

  1. Config.ManifestPublicKey
  2. PILOT_SKILLINJECT_PUBKEY (hex or base64)
  3. ~/.pilot/skillinject.pub
  4. compiled-in DefaultManifestPublicKeyHex

Config.RequireSignedManifest / PILOT_SKILLINJECT_REQUIRE_SIG turns a resolved key from optional into mandatory.

Both new controls default off: with no key configured anywhere, fetch behaviour is byte-identical to before, so nothing breaks for existing installs. Operators can now enable verification without a code change, and populating DefaultManifestPublicKeyHex (currently "") flips it on by default once a signing key is published.

Not addressed here: the "move to org repo" half of L28. DefaultManifestURL / DefaultRepoBaseURL still point at TeoSlayer/pilot-skills because that repo has not been transferred yet (its origin is still TeoSlayer/pilot-skills); repointing now would break every install. The existing TODO comments in manifest.go still track it.

Tests

  • zz_uninstall_traversal_test.go — an escaping name leaves a seeded file outside the install dir byte-identical and surfaces a RemovalError; an in-dir file is still deleted; the install side does not write outside either.
  • zz_manifest_key_test.gopathWithin table, key decoding (hex / base64 / raw base64 / bad input), full resolution precedence, require-flag parsing, and getOrVerify in all three modes (no key → unverified and no .sig fetched; require-without-key → error; resolved key → verifies and rejects a tampered body).

go build ./..., go vet ./..., go test ./... green. gofmt -l flags service.go and zz_skillinject_test.go, both pre-existing and untouched.

🤖 Generated with Claude Code

…cation key

Uninstall walked plugin file names straight from the manifest and passed
the joined path to removeOwnedFile, while the install path (reconcile-
PluginFiles) rejected any name that resolved outside the plugin install
directory. Both sides now share a pathWithin helper, so a name that
escapes installDir is reported as a RemovalError instead of being acted
on.

Manifest signature verification previously required the caller to supply
Config.ManifestPublicKey; a nil value silently skipped verification.
The key is now resolved from Config, then PILOT_SKILLINJECT_PUBKEY, then
~/.pilot/skillinject.pub, then the compiled-in DefaultManifestPublicKey-
Hex. Config.RequireSignedManifest (and PILOT_SKILLINJECT_REQUIRE_SIG)
makes a resolved key mandatory. Both default off, so behaviour with no
key configured is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@TeoSlayer
TeoSlayer merged commit f1816ef into main Jul 26, 2026
4 checks passed
@TeoSlayer
TeoSlayer deleted the sec/findings-skillinject branch July 26, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants