fix(security): strengthen file path safety checks in system administration handler - #1216
Merged
Conversation
Introduce a strings.HasPrefix-based path containment helper alongside its dedicated test suite. Not yet wired into any production call site; a subsequent commit applies it at the filesystem sink guards in system_permissions_handler.go.
Apply isWithinAllowlistBounds inline immediately before each of the four filesystem sink calls in repairPath and pathHasSymlink, using the exact value that reaches each sink. pathHasSymlink now takes the normalized allowlist and returns a new sentinel error when a walked path component falls outside it. No externally observable behavior change: every existing repairPath/pathHasSymlink test passes with its assertions unmodified.
…ation handler Add direct, function-local containment guards immediately before each filesystem sink in the permissions-repair path, plus new test coverage for the allowlist-bounds decision logic, the symlink-walk rejection path, and a symlink-escaping-allowlist regression case. Also corrects a pre-existing test that exercised the wrong rejection branch instead of the Lstat error path it was meant to cover. No externally observable behavior change: request/response shapes, status codes, and error codes are unchanged.
…ntation isWithinAllowlistBounds and its dedicated test were never called from production code -- the actual sink guards use firstAllowlistPrefix plus an inline strings.HasPrefix check in each sink's own function, added in the previous commit. Delete the dead helper and its test, add direct test coverage for firstAllowlistPrefix (the function actually in use), and update docs/plans/current_spec.md's as-built notes and acceptance criteria to describe the mechanism that was actually shipped. Also adds the previously-untracked issue doc for the separate, out-of-scope auth_handler.go finding referenced throughout this fix's scope notes.
Preserves the prior docs/plans/current_spec.md content under docs/plans/archive/ rather than leaving it as an untracked file, after current_spec.md was rewritten for the path-injection fix work.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
Codecov's patch status was configured with informational: true and documented across testing.instructions.md and copilot-instructions.md as "a suggestion" that "will not block PR approval." In practice it does block merges, so the docs were wrong, not the enforcement. Correct all three: codecov.yml now enforces patch coverage (target aligned with the project coverage target / CHARON_MIN_COVERAGE default of 87%, rather than the previously inconsistent 90%), and both instruction files now describe it as a mandatory Definition of Done gate.
Extract the outside-allowlist permissionsRepairResult literal (already duplicated at the pre-existing isWithinAllowlist guard and the new strings.HasPrefix guard) into a single outsideAllowlistResult helper. No behavior change -- both call sites return the exact same status, error code, and message as before. This also closes the patch-coverage gap on the new guard's structurally-unreachable rejection branch: it previously duplicated the full 6-line result literal (never executed in practice, since firstAllowlistPrefix is a proven superset of isWithinAllowlist's containment decision for any input that reaches it), inflating the uncovered footprint. Sharing the constructor shrinks that branch to a single already-covered-elsewhere call, without writing any test that doesn't reflect a real reachable state.
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
2 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.
Summary
go/path-injectionfindings insystem_permissions_handler.go's admin-only, root-required, single-container-mode-only permission-repair endpoint — all sharing the samereq.Pathstaint source, hittingos.Lstat(×2),os.Chown, andos.Chmod..., allowlist-checked, symlink-rejected) before every sink, but CodeQL's Go sanitizer only recognizes a direct, same-function, non-loopstrings.HasPrefix(taintedVar, ...)call as a barrier — not a call routed through a separate helper. This is a scanner-legibility hardening fix, not a fix for a previously-exploitable bug.firstAllowlistPrefix+ inlinestrings.HasPrefixguards at each sink, in the sink's own function, with zero change to existing validation semantics or the endpoint's external behavior.isWithinAllowlistBounds) once it was confirmed unused in production after the mechanism above proved to be the one CodeQL actually recognizes.go/cookie-secure-not-setsuppression-comment bug (auth_handler.go:191, not honored by CodeQL) as a separate tracked issue rather than bundling it here.docs/plans/current_spec.mdpreviously held the "What's New Changelog" plan) so it isn't lost when this fix's own plan replaced it.Test plan
go build ./...go test ./...(full suite)make lint-fast(staticcheck etc.) — 0 issuesscripts/go-test-coverage.sh— 89.2% (gate 87%)scripts/local-patch-report.sh— artifacts generatedlefthook run pre-commit— full suite passedlefthook run codeql(Go + JS) — 0 blocking findings; all 4go/path-injectionresults resolved (verified via SARIF/jq); only the pre-existing, trackedgo/cookie-secure-not-setwarning remains.trivyignore; 0 unsuppressedgovulncheck ./...— 0 vulnerabilities in Charon's own codeinternal/models/**or GORM query changes)auth_handler.goconfirmed untouched throughout