Remove the security assessment report
- Delete SEC_REPORT.md from the repository root
johannSocommitted
Showing 1 changed file with 0 additions and 208 deletions.
+0−208SEC_REPORT.mddeleted
| @@ -1,208 +0,0 @@ | ||
| 1 | - # Vectrum — Security Assessment | |
| 2 | - | |
| 3 | - **Target:** Vectrum — self-hosted Git forge (Next.js 16.3.7 + PocketBase + git over HTTP/SSH + CI runners) | |
| 4 | - **Date:** 2026-10-02 | |
| 5 | - **Scope:** Full application source review + live validation against the running instance (web `:3000`, SSH `:2222`) | |
| 6 | - **Assessor:** Authorized security review (account `johannso`, on the owner's own infrastructure) | |
| 7 | - | |
| 8 | - --- | |
| 9 | - | |
| 10 | - ## 1. Executive summary | |
| 11 | - | |
| 12 | - Vectrum is a compact, generally well-built Git forge. Authentication, the PocketBase | |
| 13 | - authorization model, token/secret handling, markdown sanitization, and server-action | |
| 14 | - access control are all sound (see §5). | |
| 15 | - | |
| 16 | - However, the assessment found **one critical, directly exploitable vulnerability**: a | |
| 17 | - **git argument-injection flaw that lets any authenticated user overwrite arbitrary files | |
| 18 | - on the server with attacker-controlled content**, as the server/container user. This leads | |
| 19 | - to remote code execution and full compromise of the instance. | |
| 20 | - | |
| 21 | - | # | Severity | Finding | Status | | |
| 22 | - |---|----------|---------|--------| | |
| 23 | - | 1 | 🔴 **Critical** | Git argument injection via branch names → arbitrary file overwrite → RCE | **Confirmed** (git-layer PoC proven; live code path verified) | | |
| 24 | - | 2 | 🟡 Low | Session-cookie `Secure` flag depends on a client-supplied header | Confirmed by review | | |
| 25 | - | 3 | 🟡 Low | Clone/runner URLs trust `Host` / `X-Forwarded-Host` | Confirmed by review | | |
| 26 | - | 4 | 🔵 Info | Runner tokens never expire / cannot be rotated | Confirmed by review | | |
| 27 | - | 5 | 🔵 Info | SSH server offers no post-quantum key exchange | Observed on live `:2222` | | |
| 28 | - | |
| 29 | - --- | |
| 30 | - | |
| 31 | - ## 2. Critical — Git argument injection → arbitrary file overwrite (RCE) | |
| 32 | - | |
| 33 | - ### 2.1 Summary | |
| 34 | - | |
| 35 | - Ref names (branches/tags) are passed to `git` as **positional arguments without an | |
| 36 | - `--end-of-options` / `--` barrier and without any leading-dash validation**. Git parses any | |
| 37 | - argument that begins with `-` as an **option**, not a revision. Because the push handlers | |
| 38 | - accept ref names that begin with `-`, an attacker can create a branch literally named | |
| 39 | - `--output=/absolute/path`, then cause a server-side `git log` to run it as an option that | |
| 40 | - **writes attacker-controlled commit text to any file path** the server process can write. | |
| 41 | - | |
| 42 | - ### 2.2 Affected code | |
| 43 | - | |
| 44 | - `lib/git.ts` — ref value is placed before `--` (or with no barrier at all): | |
| 45 | - | |
| 46 | - ```ts | |
| 47 | - // log() | |
| 48 | - ["log", `--format=${COMMIT_FORMAT}`, `-n${limit}`, `--skip=${skip}`, ref, "--", path] | |
| 49 | - // commitCount() | |
| 50 | - ["rev-list", "--count", ref] // no barrier | |
| 51 | - // listContributors() | |
| 52 | - ["shortlog", "-sne", ref, "--"] | |
| 53 | - // getCommit() | |
| 54 | - ["show", "-s", `--format=${COMMIT_FORMAT}`, sha, "--"] | |
| 55 | - ``` | |
| 56 | - | |
| 57 | - Three missing validations combine to make this reachable: | |
| 58 | - | |
| 59 | - 1. **No ref-name validation on push.** `lib/git-http.ts` (`handleServiceRpc`) and | |
| 60 | - `lib/ssh-server.ts` (`session.on("exec")`) stream the request straight into | |
| 61 | - `git receive-pack`, which accepts `refs/heads/--output=…`. | |
| 62 | - 2. **No leading-dash guard in ref resolution.** `resolveRefAndPath()` and `defaultBranch()` | |
| 63 | - in `lib/git.ts` match the attacker's branch name against the real ref list and return it | |
| 64 | - verbatim. | |
| 65 | - 3. **No barrier at the sink.** The commands above interpret the returned ref as an option. | |
| 66 | - | |
| 67 | - ### 2.3 Reachable sinks (web routes) | |
| 68 | - | |
| 69 | - - `app/[owner]/[repo]/commits/[...segments]/page.tsx` → `log(repo.dir, ref, …)` | |
| 70 | - — triggered by `GET /{owner}/{repo}/commits/--output=/path`. | |
| 71 | - - `app/[owner]/[repo]/page.tsx` (repo homepage) → `commitCount()` + `listContributors()` | |
| 72 | - on `repo.defaultBranch` — triggers **automatically** on page view if the malicious branch | |
| 73 | - is the repository's default/only branch. | |
| 74 | - | |
| 75 | - ### 2.4 Proof of concept (verified against git 2.55.0) | |
| 76 | - | |
| 77 | - Pushing a dash-prefixed ref is accepted: | |
| 78 | - | |
| 79 | - ```console | |
| 80 | - $ git push ../srv.git 'HEAD:refs/heads/--output=/tmp/vt/PWNED' | |
| 81 | - $ git --git-dir srv.git for-each-ref --format='%(refname:short)' refs/heads | |
| 82 | - --output=/tmp/vt/PWNED | |
| 83 | - ``` | |
| 84 | - | |
| 85 | - The server-side `git log` then writes **attacker-controlled commit content** to an | |
| 86 | - arbitrary path (here, a forged SSH key line taken from a commit subject the attacker chose): | |
| 87 | - | |
| 88 | - ```console | |
| 89 | - $ git --git-dir srv3.git log '--format=%s' -n36 --skip=0 '--output=/tmp/vt/authorized_keys' -- | |
| 90 | - $ cat /tmp/vt/authorized_keys | |
| 91 | - ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAATTACKERKEY attacker@evil | |
| 92 | - ``` | |
| 93 | - | |
| 94 | - ### 2.5 End-to-end attack (any authenticated user) | |
| 95 | - | |
| 96 | - 1. Create a repository (allowed for any signed-in user) and push a `main` branch whose | |
| 97 | - latest commit **subject** is the file contents you want to write. | |
| 98 | - 2. Push a second branch named, e.g., `--output=/home/node/.ssh/authorized_keys` | |
| 99 | - (or `/data/pocketbase-superuser.json`, a repo `hooks/post-receive`, app source, …). | |
| 100 | - Both the SSH and HTTP push paths accept it unchanged. | |
| 101 | - 3. Request `GET /{you}/{repo}/commits/--output=/home/node/.ssh/authorized_keys` | |
| 102 | - (or simply set that branch as the default and open the repo home). The server runs | |
| 103 | - `git log … --output=… --` and **overwrites that file** with your commit text. | |
| 104 | - | |
| 105 | - ### 2.6 Live validation | |
| 106 | - | |
| 107 | - - SSH to the running instance authenticates and reaches the push handler: | |
| 108 | - `ssh -T -p 2222 git@localhost` → `Hi johannso! You've successfully authenticated…` | |
| 109 | - - Web interface healthy (`GET :3000/api/health` → `{"ok":true}`), signups enabled. | |
| 110 | - - Source confirms **no** ref-name validation on the container's push path and **no** | |
| 111 | - leading-dash guard in ref resolution — identical to the proven PoC code path. | |
| 112 | - - Writing a file *inside the running container* was **not** performed: the host blocks it | |
| 113 | - (Docker socket access denied, no passwordless sudo, `/data` not host-visible). This is | |
| 114 | - host hygiene only; it does not mitigate the flaw in Vectrum. | |
| 115 | - | |
| 116 | - ### 2.7 Impact | |
| 117 | - | |
| 118 | - Arbitrary file creation/overwrite as the server (container) user, with substantially | |
| 119 | - attacker-controlled content. Realistic escalations: implant an SSH `authorized_keys`, | |
| 120 | - overwrite `data/pocketbase-superuser.json` to seize the PocketBase superuser, plant a git | |
| 121 | - hook or poison application source → **remote code execution / full instance compromise**. | |
| 122 | - Exploitable by any user who can sign up and push. | |
| 123 | - | |
| 124 | - ### 2.8 Remediation (defense in depth — apply all) | |
| 125 | - | |
| 126 | - 1. **Barrier at every sink.** In `lib/git.ts`, insert `--end-of-options` immediately before | |
| 127 | - any revision argument (or ensure refs are only ever passed after `--`). Cover `log`, | |
| 128 | - `rev-list`, `shortlog`, `show`, `cat-file`, `ls-tree`, and the transport commands. | |
| 129 | - Example: | |
| 130 | - ```ts | |
| 131 | - ["log", `--format=${COMMIT_FORMAT}`, `-n${limit}`, `--skip=${skip}`, | |
| 132 | - "--end-of-options", ref, "--", ...(path ? [path] : [])] | |
| 133 | - ``` | |
| 134 | - 2. **Reject dash-prefixed refs on read.** In `resolveRefAndPath`, `listBranches`, | |
| 135 | - `listTags`, and `defaultBranch`, drop/deny any ref whose name starts with `-`. | |
| 136 | - 3. **Validate refs on push.** Refuse ref updates whose names start with `-` or otherwise | |
| 137 | - fail `git check-ref-format`, in the receive-pack path (`lib/git-http.ts`, | |
| 138 | - `lib/ssh-server.ts`). | |
| 139 | - 4. Add a regression test using the PoC (push `--output=…`, assert no file is written and | |
| 140 | - the request 404s). | |
| 141 | - | |
| 142 | - --- | |
| 143 | - | |
| 144 | - ## 3. Low-severity findings | |
| 145 | - | |
| 146 | - ### 3.1 Session-cookie `Secure` flag depends on a client-supplied header | |
| 147 | - `lib/auth.ts` → `setSessionCookie()` sets `secure: h.get("x-forwarded-proto") === "https"`. | |
| 148 | - The 30-day, `sameSite: lax` session cookie is only marked `Secure` when a proxy header says | |
| 149 | - so. Correct behind the documented TLS proxy, but if the app is exposed directly, or the | |
| 150 | - proxy is misconfigured to forward the header, the session cookie can be sent in cleartext. | |
| 151 | - **Fix:** gate on an explicit server config (e.g. `SECURE_COOKIES=1` / a trusted-proxy flag) | |
| 152 | - rather than a request header. | |
| 153 | - | |
| 154 | - ### 3.2 Host-derived URLs trust `Host` / `X-Forwarded-Host` | |
| 155 | - `lib/repo.ts` (`cloneUrls`), `lib/request-origin.ts`, `lib/runner-api.ts` (`originOf`) build | |
| 156 | - clone/runner origins from `x-forwarded-host` / `host`. Impact is limited (spoofed clone URLs | |
| 157 | - shown to users, runner clone origin), but in multi-tenant/public deployments pin these to a | |
| 158 | - configured canonical host. | |
| 159 | - | |
| 160 | - --- | |
| 161 | - | |
| 162 | - ## 4. Informational | |
| 163 | - | |
| 164 | - ### 4.1 Runner tokens never expire | |
| 165 | - `lib/runners.ts` — runner tokens (`vrn_…`) are long-lived with no expiry or rotation, unlike | |
| 166 | - API tokens. A leaked runner token lets a machine claim scoped jobs and receive repo checkout | |
| 167 | - tokens indefinitely. Consider optional expiry/rotation. | |
| 168 | - | |
| 169 | - ### 4.2 SSH server lacks post-quantum key exchange | |
| 170 | - The live `:2222` server (ssh2 library) negotiates only classical KEX; OpenSSH clients warn | |
| 171 | - about "store now, decrypt later" exposure. Low risk for a git transport; track the ssh2 | |
| 172 | - library's PQ support for a future upgrade. | |
| 173 | - | |
| 174 | - --- | |
| 175 | - | |
| 176 | - ## 5. Reviewed and found solid | |
| 177 | - | |
| 178 | - - **PocketBase authorization:** all collection rules are superuser-only (`null`) except | |
| 179 | - `users.viewRule = "id = @request.auth.id"`; the Next.js server mediates every access. | |
| 180 | - - **Session handling:** sessions are PocketBase auth tokens in an `httpOnly` cookie, | |
| 181 | - re-validated by reading the user's own record with their token (`lib/auth.ts`). | |
| 182 | - - **Server-action authorization:** `repoForAction` / `viewerOrThrow` enforce | |
| 183 | - read/write/admin consistently; issue deletion is restricted to author-or-admin; label and | |
| 184 | - settings mutations require write/admin. No IDOR found (`app/actions/*`). | |
| 185 | - - **Secrets:** API, runner, and job tokens are 30-byte random values stored only as SHA-256 | |
| 186 | - hashes; passwords via PocketBase bcrypt; login avoids username-enumeration timing | |
| 187 | - (`lib/tokens.ts`, `lib/runners.ts`, `lib/runs.ts`, `lib/users.ts`). | |
| 188 | - - **Markdown XSS:** `rehype-raw` → `rehype-sanitize` ordering is correct (GitHub allowlist | |
| 189 | - applied last); `urlTransform` blocks `javascript:` and similar (`components/markdown.tsx`, | |
| 190 | - `components/markdown-preview.tsx`). Shiki highlight HTML is trusted input. | |
| 191 | - - **Path traversal:** `isValidName` / `isValidOwner` / `repoPath` constrain on-disk repo | |
| 192 | - paths; `cleanRepoPath` rejects `..`, `.git`, and control characters for web edits | |
| 193 | - (`lib/git.ts`, `lib/git-write.ts`). | |
| 194 | - - **Blob/image routes** set `X-Content-Type-Options: nosniff` and a sandboxing CSP, | |
| 195 | - preventing stored XSS via uploaded blobs/SVGs (`app/[owner]/[repo]/raw`, `/image`). | |
| 196 | - - **Repo images** are validated by magic bytes, not the browser-reported MIME type | |
| 197 | - (`lib/repos.ts` `sniffImageType`). | |
| 198 | - | |
| 199 | - --- | |
| 200 | - | |
| 201 | - ## 6. Priority | |
| 202 | - | |
| 203 | - 1. **Immediately:** Fix finding #1 (git argument injection) — small, localized change in | |
| 204 | - `lib/git.ts` plus ref validation on the push/resolve paths; add the regression test. | |
| 205 | - 2. Then: findings #2 and #3 (header-trust hardening). | |
| 206 | - 3. Backlog: #4 and #5. | |
| 207 | - | |
| 208 | - 🤖 Generated with [Claude Code](https://claude.com/claude-code) | |