Harden app-store install path and cap-state reads#315
Merged
Conversation
Three install-path security fixes in pilotctl appstore: - Path traversal during staging: manifest binary.path was joined onto the bundle and staging dirs with no containment check, so a path like ../../etc/x escaped the staging tree. Add a resolveUnder guard (mirrors the supervisor's) at both join sites; reject any path that resolves outside the target dir. - Unbounded catalogue bundle download: the bundle tarball was copied with no size limit, so a hostile or compromised CDN could exhaust disk before the post-copy sha256 check ran. Cap the compressed download with io.LimitReader (maxBundleBytes) and fail closed when exceeded. - Cap-state reads no longer fail open: loadCapStateRecords silently skipped malformed lines and tampered HMAC records, under-reporting usage and hiding an erased cap. It now returns an error and the caps command fails closed on a malformed line, an HMAC mismatch, or a signed chain spliced with an unauthenticated record. Tests: ../../etc/x and absolute/empty paths rejected; oversized bundle download refused; malformed line and tampered/mixed cap-state records fail closed; legacy and nil-key reads still accepted.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three app-store security fixes in
cmd/pilotctl.1. Path traversal during staging
appstore.go: manifestbinary.pathwas joined onto the bundle and staging dirs with barefilepath.Join— a path like../../etc/xescaped the staging tree, letting a hostile manifest read an arbitrary host file as the "binary" or plant a copy outside staging. Added aresolveUndercontainment guard (mirrors the supervisor's) at both join sites; any path resolving outside the target dir is refused.2. Unbounded catalogue bundle download
appstore_catalogue.go: the bundle tarball was copied with no size cap, so a hostile/compromised CDN could stream an unbounded body and exhaust disk before the post-copy sha256 check ran. Added anio.LimitReadercap (maxBundleBytes, 128 MiB compressed) that fails closed when exceeded. The per-file untar cap already existed; this bounds the download itself.3. Cap-state reads fail open
loadCapStateRecordssilently skipped malformed lines and tampered HMAC records — under-reporting usage and making an erased cap look healthy. It now returns an error;appstore capsfails closed (integrity_error) on a malformed line, an HMAC mismatch, or a signed chain spliced with an unauthenticated record. Legacy (no-HMAC) and nil-key reads are still accepted for read-only reporting.Tests
TestResolveUnderRejectsTraversal—../../etc/x,../escape,a/../../escape, absolute, and empty paths rejected; nested relatives accepted.TestFetchAndUnpackBundle_RejectsOversizedDownload— oversized download refused, within-cap unpacks (maxBundleBytesis a var so the test avoids writing 128 MiB).TestLoadCapStateRecordsMalformedLineFailsClosed+ updatedTestLoadCapStateRecordsHMACChain— fail-closed on garbage/tamper/mixed, legacy + nil-key still accepted.Validation
GOWORK=off go vet ./...clean;go build ./cmd/daemon ./cmd/pilotctl;go test -short ./cmd/pilotctl/...andgo test -race -short ./cmd/pilotctl/green; gofmt clean on touched files.Cross-repo follow-up
Item 3 (HMAC) interoperates with pilot-protocol/wallet#22 (writer). The reader here already understood the
hmacfield; both derive the key via the same HKDF (info=pilot-cap-state-v1). No version bump required — this PR has no new dependency on an unpublished wallet/app-store release.