From eb5a82f1f3a3286506c2d527e7980db7ccae5398 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 16:53:57 +0000 Subject: [PATCH] Record security audit findings --- docs/roadmap/security.md | 194 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 docs/roadmap/security.md diff --git a/docs/roadmap/security.md b/docs/roadmap/security.md new file mode 100644 index 0000000..8ebde95 --- /dev/null +++ b/docs/roadmap/security.md @@ -0,0 +1,194 @@ +# Security Audit + +Reviewed revision: `b30c9f167dba16a9192d9bfde4aeb2b9c597ce0e`. +Audit date: 2026-10-25. + +## Scope And Trust Model + +This review examined input validation and budgets, import/discovery containment, +link resolution, collection authority, application callbacks, secret references +and bootstrap, historical sensitivity, diagnostics, effective rendering and +diffs, and the runtime/development dependency boundary. Knowledge-graph searches +and caller traces were supplemented by source inspection, existing tests, a +bounded standalone diagnostic, and an advisory-database spot-check. + +The [architecture](../policy/architecture.md) and +[configuration contract](../config.md) establish these boundaries: + +- ConfigKit is a synchronous startup library, not a network service or an + authentication/authorization framework. Applications select inputs, roots, + filesystem implementations, callbacks, and credential use. Applications own + remote acquisition and authentication of that data. +- The public facade delegates to a private engine and private document/YAML + packages. Input composition and inspection do not execute shell commands or + launch a background service. Ordinary loading does not change the process + environment; explicit secrets-directory bootstrap is the narrow exception. +- Host imports use canonical containment checks and owned rooted access under + the documented stable-input assumption. Borrowed native filesystems retain + their own behavior and are explicitly not a general sandbox guarantee. +- Secret references resolve after composition and application resolution. The + trusted returned configuration can expose credentials through `Value()`; + inspection retains names rather than credentials. Tags, a path predicate, + inheritance, and recorded dependencies govern redaction of other values. + Application-supplied messages and unrecorded secret transformations remain + application responsibilities. + +One new availability finding warrants remediation. No new credential disclosure, +command injection, application-authority bypass, or host containment escape was +established. This is a source-level audit with bounded checks, not a certification +that every input, dependency, platform, or deployment is secure. + +## 1. Finite Symlink Reuse Can Cause Exponential Import-Resolution Work + +Status: open. Priority: medium. Category: availability / input amplification. + +**Relevant code.** `resolveHostImportPath` and `resolveNativeImportPath` in +[filesystem_links.go](../../internal/engine/filesystem_links.go), lines 129–242, +expand link-target components with a frame stack and track only currently active +links. Completion removes a link from that set; a later use resolves its entire +target again. No completed-target result is reused. + +`importEntryTarget` invokes these resolvers for discovered links. Both +[directory selection](../../internal/engine/import_selection.go) and +[glob selection](../../internal/engine/glob.go) reach it. Directory selection +resolves the target before deciding whether recursive descent is enabled, so +`recursive: false` does not avoid this work. Native resolution is used when the +borrowed filesystem provides `fs.ReadLinkFS`; host resolution uses the analogous +rooted implementation. + +**Plausible scenario.** An application loads a configuration tree whose imported +assets can be supplied by a less-trusted user or configuration provider. That +provider controls link entries inside an allowed import directory. The +application's own main file need only select that directory with link following +enabled, which is the current default. No concurrent filesystem mutation, +outside-root access, executable callback, or malicious filesystem implementation +is required. This is conditional on influence over imported filesystem data; +it is not an anonymous remote vulnerability in ConfigKit by itself. + +Inside an ordinary `links` directory, create this finite graph: + +```text +s00 -> . +s01 -> s00/s00 +s02 -> s01/s01 +s03 -> s02/s02 +... +``` + +Each link resolves to the same contained directory. It has no active link cycle: +the first reference to a lower link finishes before the second begins. Resolving +level N nevertheless repeats the level N-1 expansion twice, giving a recurrence +of `T(N) = 2T(N-1) + O(1)` component work for this fixed-name fixture. Directory +enumeration contains only N+1 entries, and no imported configuration nodes or +new import edges account for those repeated expansions. Consequently, source, +entry, input-node, and active-import-depth budgets do not bound this work by the +size of the selected graph. This is repeated resolution of a compact graph, not +genuinely expanding configuration output or ordinary deterministic sorting. + +**Evidence.** A standalone public-API diagnostic created real temporary symlinks +and exposed them through `NewFilesystem` backed by `os.DirFS`. A thin +`fs.ReadLinkFS` wrapper counted metadata/link operations and imposed a diagnostic +ceiling of 12,000 `Lstat` calls. The main input selected `dir: links` with link +following enabled and default nonrecursive discovery. Successful loads produced +these counts: + +| Link entries | `Lstat` calls | `ReadLink` calls | +| --- | ---: | ---: | +| 3 | 32 | 11 | +| 5 | 128 | 57 | +| 7 | 512 | 247 | +| 9 | 2,048 | 1,013 | +| 11 | 8,192 | 4,083 | + +A 13-entry fixture reached the diagnostic ceiling and returned a safe load +failure when the wrapper stopped it. The same fixture with `follow: false` +completed with 17 `Lstat` calls and zero `ReadLink` calls. The operation counts +substantiate amplification in the native resolver; the host resolver's matching +frame-expansion logic was verified by source inspection and caller tracing, +not a separate host operation-count test. No timings, large attack graph, or +external-system exploitation were used. + +**Impact.** A small imported asset tree can monopolize startup/load execution for +an impractically long period. Applications that expose configuration validation +or repeated loading to less-trusted providers can also tie up their own workers. +The normal trusted-operator-only deployment reduces the adversarial exposure, +but accidental alias graphs can cause the same availability failure. + +**Potential remediation.** Reuse fully resolved link-target results within the +resolution operation, keyed by the appropriate rooted namespace and full link +path, while retaining active-path cycle detection and containment checks. +Equivalent dependency-based evaluation is another option. Preserve legitimate +finite reuse, relative/absolute host-link semantics, and the stable-input model; +do not replace the active set with a global "already visited means cycle" rule. +Review both resolvers and directory/glob entry points. The objective is to avoid +re-expanding completed dependencies, not to introduce a public cache or reject +valid graphs with an arbitrary link-count cap. Consumers can already opt out of +following discovered links where appropriate; changing that default is not part +of this recommendation. + +## Previously Reported Risks + +The [correctness audit](correctness.md) already records pointer-only recursive +schema nontermination, failed dynamic-descriptor cache state, and callback guards +left active during panic unwinding. The [resource audit](resources.md) owns +pre-budget typed-container allocation, incomplete retained-history accounting, +and an import-root cleanup gap after callback panic. These can affect availability +when their documented preconditions hold; they are not newly discovered remote +exploits or duplicate remediation tasks here. + +The [error-handling audit](error-handling.md) separately owns silently omitted +directory contributions after certain enumeration/close failures. The +[efficiency audit](efficiency.md) owns previously identified avoidable batch costs. +This review does not reclassify every expensive operation or application +programming error as a security vulnerability. + +## Dependency Spot-Check + +The ordinary package's non-standard-library dependency graph contains only +`go.yaml.in/yaml/v4 v4.0.0-rc.6`; ReleaseKit, Goldmark, and `x/mod` are maintainer +tool dependencies rather than ConfigKit runtime imports. Cached module integrity +verification passed. + +The current [Go vulnerability database module index](https://vuln.go.dev/index/modules.json) +contained no entries for the selected YAML v4, ReleaseKit, or Goldmark v2 module +paths. Its two `golang.org/x/mod` reports, +[GO-2026-6179](https://pkg.go.dev/vuln/GO-2026-6179) and +[GO-2026-6180](https://pkg.go.dev/vuln/GO-2026-6180), identify v0.40.0 as fixed, +which is the version selected here. The related toolchain fixes also precede the +Go 1.26.10 toolchain used for this audit. The database metadata reported a modified +time of `2026-10-21T00:27:29Z` when checked. + +This was a module-path/advisory spot-check, not a complete standard-library, +toolchain, transitive-reachability, or supply-chain attestation. `govulncheck`, +`gosec`, and `gitleaks` were not installed; no scanners were added to the project. +Absence of an advisory is not proof of absence of vulnerabilities, and +`go mod verify` does not audit dependency behavior or establish that upstream +content is safe. +The [database API documentation](https://go.dev/doc/security/vuln/database) +describes the advisory source used for this check. + +## Validation And Preserved Boundaries + +The following passed with Go 1.26.10 on Linux amd64 using cached dependencies and +disabled automatic dependency/toolchain acquisition: + +```sh +go test -race ./... -count=1 +go test ./internal/engine -run 'Secret|Sensitive|Sensitivity|Contain|Escap|Outside|Symlink|Nonregular|Budget|Limit|Diagnostic' -count=1 +go vet ./... +go mod verify +``` + +These checks include existing canary-based non-disclosure, historical and +bilateral diff sensitivity, safe diagnostic, parser-policy, containment, and +nonregular-file cases. The bounded link diagnostic ran outside the repository. +No production code or repository tests were added or changed; no native +macOS/Windows run, active external exploitation, or exhaustive fuzzing was done. + +This report does not propose authentication in the library, arbitrary environment +name restrictions, secret-file permission requirements, retries, memory erasure, +or protection against privileged concurrent filesystem mutation. Those would +need their own concrete requirement and threat analysis. It preserves borrowed +filesystem ownership, application-owned safe callback messages, public secret +reference names, explicit partial bootstrap semantics, and the current release +gates.