diff --git a/docs/prs/2026-07-08-PR#000-security-audit-hardening.md b/docs/prs/2026-07-08-PR#000-security-audit-hardening.md new file mode 100644 index 0000000..73aff8f --- /dev/null +++ b/docs/prs/2026-07-08-PR#000-security-audit-hardening.md @@ -0,0 +1,161 @@ +# Security Audit and Hardening + +> **WARNING:** This document describes only the change applied in this PR. +> It may already be outdated once the next PR is merged. +> Historic PR-documentation is not maintained along with new PRs — treat it as a snapshot, not as current documentation. + +## Related Links + +- ADR 0004 — server-rendered UI, systemd behind the host's reverse proxy (the deployment posture this audit assumes). +- ADR 0005 — opt-in managed nginx/TLS container for hosts without a reverse proxy (e.g. Hostsharing, a multi-tenant host — relevant to the secret-file findings). + +## The Audit Summary + +A security audit of the Kotlin/Spring source was performed on 2026-07-08. +It found no critical injection or memory-safety class bug — the classic sinks are correctly closed (see [Verified Safe](#verified-safe-checked-not-vulnerable)). +The real weaknesses are deployment-posture and secret-handling issues. + +Two threat actors frame the findings: + +- Anyone who can push a branch or open a pull request — they influence branch names and the built commit. +- Anyone who can reach the HTTP port — the UI and API are unauthenticated by default and bind `0.0.0.0`. + +The whole security model leans on an external reverse proxy providing access control. +Nothing in the app enforces that, and the one in-app control (the control token) is undermined by how it is distributed. +The highest-impact confidentiality issue is a world-readable Gitea-token file on exactly the multi-tenant host GitTally is meant to run on. + +This PR tracks the remediation as a checklist. +It may be split into smaller PRs per severity; the filename and scenario IDs use the `PR#000` placeholder until a pull request is opened. + +## Non-Goals + +- The applicaiton has deliberately no authentication/authorization layer (no Spring Security, no user accounts) — the reverse proxy stays the primary access gate. +- No change to the "build config comes from the primary checkout, never the build worktree" trust model — this PR documents and pins it, it does not rework it. +- No re-audit of the confirmed-safe areas; they are recorded here so future changes do not silently regress them. + +## The Identified Issues + +The remediation, grouped by severity. +Each item is a TODO with the background that justifies it. + +### High + +#### TODO 1 — Restrict the Gitea-token config file to the owner at creation + +- [ ] In [`InitCommand.kt:86-106`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt), create `.git/gittally/.gittally.yml` and its parent `.git/gittally/` with `0600`/`0700`, atomically at creation (as [`GitAskPass`](../../src/main/kotlin/de/hoennig/gittally/git/GitAskPass.kt) does), not via plain `writeText` at the umask default. + +**Background.** +`init` creates the file it labels "secrets" — where the operator pastes the Gitea API token — with `writeText` and no permission restriction, so it inherits the umask (typically `0644`, world-readable). +The shell path (`tools/setup-gittally-instance:245-253`) already does `chmod 600` on its equivalent file, and `ControlTokenService`/`GitAskPass` both restrict theirs — the Kotlin `init` path is the lone exception. +On a shared host (Hostsharing is a multi-tenant deployment target, per ADR 0005) any local user can then read the token, which grants git push and commit-status access to the repo. + +#### TODO 2 — Stop handing the control token to every unauthenticated reader + +- [ ] Reconsider the token distribution in [`UiController.kt:186`](../../src/main/kotlin/de/hoennig/gittally/server/UiController.kt) and [`fragments.html:9`](../../src/main/resources/templates/fragments.html): do not embed the live token in public HTML, or gate the pages behind the same check as the mutations. +- [ ] At minimum, document loudly that under the current design any read access equals full write access. + +**Background.** +Every server-rendered page embeds the live control token in `` so [`gittally.js`](../../src/main/resources/static/gittally.js) can read it, but no GET is authenticated. +So `curl -s http://host:18080/ | grep gittally-control-token` yields the token, which unlocks restart/cancel/delete. +Read access therefore equals write access, and the token provides no real second trust tier. +Blast radius is limited to build-lifecycle operations (a DoS/integrity concern, not secret disclosure or RCE), which is why this is a design flaw rather than Critical — but the token gives a false sense of protection. + +### Medium + +#### TODO 3 — Accept the control token via header only + +- [ ] Remove the `token` query-parameter variant from the three mutating endpoints in [`BuildsApiController.kt:90,115,129`](../../src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt); keep only the `X-GitTally-Token` header (which the bundled UI already uses). + +**Background.** +Tokens in URLs are routinely written to access logs, reverse-proxy logs, browser history, and the `Referer` header on outbound navigation. +The token never expires (static file secret), so any historical log capture yields a still-valid credential. +The query-param path exists only for legacy convenience. + +#### TODO 4 — Redact the Gitea token in `config:print` + +- [ ] Mask `git.token` (and any future secret) by default in [`ConfigPrintCommand.kt:20-31`](../../src/main/kotlin/de/hoennig/gittally/commands/ConfigPrintCommand.kt); gate the plaintext value behind an explicit `--show-secrets` flag. +- [ ] Update `tools/setup-gittally-instance:272`, which currently steers the operator to run `config:print --full` to view the token. + +**Background.** +Both the `--full` and default branches print the token verbatim to stdout, landing it in terminal scrollback, `script(1)` captures, screen-shares, or CI logs. +There is no redaction and no masked default. + +#### TODO 5 — Reduce information disclosure on the unauthenticated read API + +- [ ] Decide whether build-log streaming ([`BuildsApiController`](../../src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt) `/api/builds/current/{key}/log`), [`SystemApiController`](../../src/main/kotlin/de/hoennig/gittally/server/SystemApiController.kt), and [`WatcherApiController`](../../src/main/kotlin/de/hoennig/gittally/server/WatcherApiController.kt) should stay fully public, or be gated / scrubbed. + +**Background.** +Every read endpoint is public. +Raw build output may contain secrets echoed by build scripts; `/api/system` exposes host metrics; `/api/watcher` exposes last fetch/poll error strings, which can leak git remote URLs or error detail. + +### Low / defense-in-depth + +#### TODO 6 — Default `bindAddress` to `127.0.0.1` + +- [ ] Change the default in [`GitTallyConfig.kt:21`](../../src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt) and the `init` template ([`InitCommand.kt:126`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt)) from `0.0.0.0` to `127.0.0.1`; require operators to opt into all-interfaces. + +**Background.** +The current default binds all interfaces, which — combined with the public read surface and token-in-HTML — exposes the whole UI and the control token to the network whenever the reverse proxy is forgotten. +The deployment doc already recommends `127.0.0.1`; the default and template should match it. + +#### TODO 7 — Compare fixed-length hashes in the token check + +- [ ] In [`ControlTokenService.kt:35-37`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt), compare `SHA-256(submitted)` against `SHA-256(secret)` with `MessageDigest.isEqual`, so the comparison is always over equal-length buffers. + +**Background.** +`MessageDigest.isEqual` returns early on a length mismatch, leaking the token length via timing. +Largely theoretical given the 192-bit CSPRNG token, but a cheap deviation from constant-time best practice to close. + +#### TODO 8 — Add `--` before positional refnames in git calls + +- [ ] Insert `--` before the branch argument in `checkout`, `fetchBranch`, and `resetHardToOrigin` in [`GitService.kt:145,40,155`](../../src/main/kotlin/de/hoennig/gittally/git/GitService.kt) (e.g. `git switch -- `). + +**Background.** +Git accepts refnames beginning with `-` (verified: `git check-ref-format 'refs/heads/-foo'` exits 0), so a pushed branch name could in principle be read as a git option. +These three methods currently have no production callers and the actively-used paths embed the branch in a `refs/...` prefix, so this is dormant — but the guard should be in place before any of them is wired to server-triggered input. + +#### TODO 9 — Pin the "config from primary checkout" invariant + +- [ ] Add a code comment / assertion at the config-read sites ([`BuildExecutor.kt:181`](../../src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt), [`DockerBuildRunner.kt`](../../src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt)) and in `AGENTS.md`, stating build config must be read from the primary checkout, never the build worktree. + +**Background.** +`buildCommand` is executed via `bash -c "$3"` inside the build container ([`DockerBuildRunner.kt:285`](../../src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt)). +Today that is safe because config comes from `configLoader.load(workingDir)` (the operator's checkout), not the attacker's committed `.gittally.yml`. +The moment a future feature reads per-branch config from the branch itself, this becomes direct RCE — so the invariant should be explicit, not implicit. + +#### TODO 10 — Create secret files restricted atomically + +- [ ] Set the mode at creation for the control-token file ([`ControlTokenService.kt:29-30`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt)) and the setup-script YAML (`tools/setup-gittally-instance:253`), instead of `chmod 0600` after the write. + +**Background.** +Both currently write the file at the umask default and tighten it afterward, leaving a brief window where the secret exists world-readable. +A small TOCTOU gap; `GitAskPass`'s atomic-at-creation approach is the pattern to copy. + +## Open Questions + +- TODO 2: full fix (gating the pages) versus documentation-only — the pages are the UI, so gating them needs a decision on how operators authenticate; the current implemented behavior is fully public. +- TODO 5: whether public read access is acceptable by design (it matches legacy) or should change; current behavior leaves all reads public. +- TODO 6: changing the default `bindAddress` is a behavior change for existing installs that rely on `0.0.0.0`; needs a migration note. + +## Additional Changes + +- Update `docs/deployment.md` to state explicitly that the reverse proxy is a hard requirement, not a recommendation, and that read access currently implies write access via the embedded token (until TODO 2 lands). + +## Follow-up PRs + +- If TODO 2 grows into real UI authentication, that is a separate PR with its own ADR (it revises the "no auth layer" stance of ADR 0004). + +## Attachments + +### Verified Safe (checked, not vulnerable) + +Recorded so future changes do not silently regress these. + +- **Path traversal in artifact serving** — [`ArtifactFileController`](../../src/main/kotlin/de/hoennig/gittally/server/ArtifactFileController.kt) normalizes then enforces `startsWith(artifactDir)` and `isRegularFile(..., NOFOLLOW_LINKS)` (rejects symlink escape); [`FileArtifactStore`](../../src/main/kotlin/de/hoennig/gittally/artifacts/FileArtifactStore.kt) whitelists the key to `[A-Za-z0-9._-]+` and requires `dir.parent == branchesDir`. +- **Command injection** — every git/docker/certbot/nginx call uses `ProcessBuilder(List)` ([`GitCommandRunner.kt`](../../src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt)); no `Runtime.exec(String)`, no shell-string interpolation; the only `sh -c`/`bash -c` uses pass data as positional args. +- **Branch-name → filesystem** — always routed through [`ArtifactKeys.sanitize`](../../src/main/kotlin/de/hoennig/gittally/build/ArtifactKeys.kt) (`/` → `_`) plus a SHA suffix, so worktree/container/volume names cannot traverse. +- **XSS** — no `th:utext` in any template; `gittally.js` builds all DOM via `createElement` + `textContent`/`dataset`, no `innerHTML`. +- **SSRF** — the Gitea status proxy validates the commit against `[0-9a-fA-F]{7,40}` ([`StatusApiController.kt`](../../src/main/kotlin/de/hoennig/gittally/server/StatusApiController.kt)); the Gitea base URL/owner/repo/token come from config, not the request. +- **Deserialization** — Jackson JSON/YAML without polymorphic/default typing (no gadget-chain RCE); unreadable state files degrade to empty. +- **Credential transport** — Gitea token sent as an `Authorization` header, never in a URL ([`GiteaClient.kt`](../../src/main/kotlin/de/hoennig/gittally/gitea/GiteaClient.kt)); git auth via env-based `GIT_ASKPASS` with a `0700` script containing no secrets, deleted in `finally`; TLS verification never disabled. +- **nginx/certbot** — `serverName`/`upstreamHost` validated against `[A-Za-z0-9][A-Za-z0-9.-]*` before substitution ([`NginxProxyManager.kt`](../../src/main/kotlin/de/hoennig/gittally/server/NginxProxyManager.kt)); ports range-checked.