neiam
/gitgud
Git Gud
public · Issues · Pulls · Labels · Forks · Compare · Actions success · Badges · Packages
⭐
1
Log in to mark this repository.
Qwen
9966e8a · gmorell · 2026-10-20 18:05
Files changed
added
security-review-ci.md
+67
−0
@@ -0,0 +1,67 @@
| 1 | +# CI Security Review: GitGud | |
| 2 | + | |
| 3 | +## Critical | |
| 4 | + | |
| 5 | +**1. Insecure default secrets in config** | |
| 6 | +`config/config.exs:141-149` — Two production-sensitive defaults shipped with the codebase: | |
| 7 | +- **Secrets master key**: `"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="` (all zeros). If `runtime.exs` doesn't override this, all repository/organization secrets stored in CI are encrypted with a known key. | |
| 8 | +- **Runner JWT secret**: `"dev-runner-jwt-secret-do-not-use-in-prod"` — forgeable runner tokens. An attacker with access to this could mint arbitrary runner tokens. | |
| 9 | + | |
| 10 | +**2. HS256-only JWT signing** | |
| 11 | +`lib/git_gud/workflows/runner_jwt.ex:174` — Runner JWTs use symmetric HS256. The same secret signs all tokens. If leaked from any single runner, the attacker can mint tokens for any job. No key rotation mechanism exists at the JWT layer (only app-level master key rotation for secrets). | |
| 12 | + | |
| 13 | +**3. Fork PR secrets exposure** | |
| 14 | +`lib/git_gud_web/controllers/runner_controller.ex:244-247` — Fork PRs correctly get zero secrets, which is good. However, `${{ github.token }}` in the `secrets` map is injected at `runner_controller.ex:213-215` as `GITHUB_TOKEN`, `GITEA_TOKEN`, `FORGEJO_TOKEN`. If a fork PR's workflow references `secrets.FORGEJO_TOKEN`, it would get the job token rather than the user secret (correct), but the naming could confuse users about which token is which. | |
| 15 | + | |
| 16 | +## High | |
| 17 | + | |
| 18 | +**4. Unpinned marketplace action versions** | |
| 19 | +`.forgejo/workflows/build.yml` uses `actions/checkout@v4`, `docker/setup-buildx-action@v3`, `docker/login-action@v3`, etc. Major-version pinning (`@v4`) allows minor/patch updates that could introduce supply chain compromises. SHA pinning (`@a7d94b...`) is the defense-in-depth standard. | |
| 20 | + | |
| 21 | +**5. Self-hosted runner with full repo access** | |
| 22 | +Both `build.yml:9` and `test.yml:5` use `runs-on: self-hosted` with no container isolation. A malicious workflow could access the runner's filesystem, other jobs' artifacts, or secrets. Self-hosted runners should run in isolated containers with restricted capabilities. | |
| 23 | + | |
| 24 | +**6. Build cache exposed in registry** | |
| 25 | +`.forgejo/workflows/build.yml:44-45` — Build cache is pushed to the OCI registry as `<image>:buildcache` with `mode=max`. This cache contains compiled source code and could be pulled by anyone with registry access. No access control is mentioned. | |
| 26 | + | |
| 27 | +**7. No SBOM or vulnerability scanning** | |
| 28 | +The entire build pipeline (`build.yml`) publishes Docker images and `.deb`/`.rpm` packages without any SBOM generation, image signing, or vulnerability scanning. Signed packages and signed container images are the supply chain baseline. | |
| 29 | + | |
| 30 | +**8. Android keystore shred reliability** | |
| 31 | +`.forgejo/workflows/build.yml:253-254` — `shred -u` on `always()` runs even on failure. On tmpfs/overlayFS (common in container runners), `shred` is a no-op and the `2>/dev/null || rm -f` fallback silently ignores shred failures. The keystore bytes may persist in the filesystem. | |
| 32 | + | |
| 33 | +## Medium | |
| 34 | + | |
| 35 | +**9. Expression injection surface** | |
| 36 | +`lib/git_gud/workflows/expr.ex` — The expression evaluator handles `${{ }}` interpolation. While it doesn't execute arbitrary code, the `fromJSON` function (`expr.ex:307-311`) decodes arbitrary JSON. If downstream code trusts `fromJSON` output without validation, malicious workflow payloads could craft edge-case inputs. | |
| 37 | + | |
| 38 | +**10. Runner registration token not expired** | |
| 39 | +`lib/git_gud_web/controllers/runner_controller.ex:66-93` — Registration uses a one-shot token. However, there's no evidence of token expiry enforcement in the controller. If a registration token is never consumed, it remains valid. | |
| 40 | + | |
| 41 | +**11. Large body read limits** | |
| 42 | +- `actions_artifact_controller.ex:76` — `read_body/2` with `length: 1_000_000_000` (1 GB) per artifact upload. This can cause memory exhaustion if multiple concurrent uploads hit the limit. | |
| 43 | +- `actions_cache_controller.ex:91` — `read_body/2` with `length: 200_000_000` (200 MB) per chunk. | |
| 44 | +- `runner_controller.ex:700` — `read_body/2` with `length: 50_000_000` (50 MB). | |
| 45 | + | |
| 46 | +**12. Missing audit logging for secret operations** | |
| 47 | +Secrets can be created/updated/deleted via the web UI but there's no explicit audit log for CI secret management operations. | |
| 48 | + | |
| 49 | +**13. `update_task` log-job fallback** | |
| 50 | +`runner_controller.ex:571-588` — When `task_id` doesn't match the runner's claimed job, `log_job/2` falls back to `claimed_job/runner` which attributes logs to whichever job the runner has running. If a runner has multiple concurrent jobs, log mixing could occur, obscuring what actually happened. | |
| 51 | + | |
| 52 | +**14. Pre-receive hook fail-open escape hatch** | |
| 53 | +The pre-receive hook (`priv/scripts/gitgud-prereceive`) has `GITGUD_PREHOOK_FAIL_OPEN=1` as an escape hatch. If accidentally set in production, branch protection is bypassed. | |
| 54 | + | |
| 55 | +**15. Content-type spoofing in artifact upload** | |
| 56 | +`actions_artifact_controller.ex:150` — Downloads return `artifact.content_type` from the database without validation. An attacker could set an arbitrary `content_type` during artifact creation, potentially enabling XSS if artifacts are served inline in a browser. | |
| 57 | + | |
| 58 | +## Low | |
| 59 | + | |
| 60 | +**16. Cache upload expiry is short** | |
| 61 | +`actions_cache_controller.ex:46` — Upload expiry is 600s (10 min). If a runner times out before finalizing, the upload is orphaned. The `CacheEviction` worker (`.gitlab-ci.yml:67` / `config/config.exs:113-122`) cleans abandoned uploads hourly, but in the meantime the storage is wasted. | |
| 62 | + | |
| 63 | +**17. No rate limiting on runner protocol** | |
| 64 | +The runner endpoints (`/api/actions/_apis/runner.v1.*`) have no rate limiting. A single runner could hammer `FetchTask` indefinitely, consuming DB resources. | |
| 65 | + | |
| 66 | +**18. Workflow file modification without branch protection** | |
| 67 | +`.forgejo/workflows/` can be modified via direct push to `master`. No branch protection or required reviews are enforced on workflow file changes. |
Parents: abee805