From 1033cd79808da9bd5575fe747d113e7d3e3f3c5f Mon Sep 17 00:00:00 2001 From: Alex Wilkerson John Date: Thu, 2 Jul 2026 12:57:04 -0400 Subject: [PATCH] harden(security): close RCE gate gap + supply-chain/scan hardening SOC 3 / OWASP gap remediation from a full-repo security audit. - fix(security): gate config-supplied govulncheck_command through trust.GuardConfigCommand. It was exec'd ungated and default-on for Go targets, an RCE for untrusted PRs. Built-in default binary stays exempt. - ci: SHA-pin every third-party GitHub Action (incl. setup-homebrew@main); SLSA generator stays tag-pinned by design. - ci(release): move untrusted tag input into env:, gate auto-tag creation behind a create_missing_tag input set only by trusted callers, scope job permissions to least privilege, pass secrets explicitly. - build(docker): digest-pin base images and run as non-root USER. - docs: add SECURITY.md (disclosure policy) and .github/CODEOWNERS. - fix(config): make artifact-path containment symlink-aware (EvalSymlinks on base + deepest existing ancestor) to close a committed-symlink escape. - perf(scan): skip files over 32 MiB in the walk and read via a bounded reader to prevent memory exhaustion on untrusted repos. - feat(report): emit SARIF tool version + invocations for run attribution. Adds regression tests for each. Verified: build, vet, gofmt, go test (505), golangci-lint (0 issues), self-scan `make codeguard-ci` (0 fails). Co-Authored-By: Claude Fable 5 --- .claude/knowledge/architecture-boundaries.md | 4 +- .codeguard/codeguard.yaml | 2 +- .github/CODEOWNERS | 20 +++++ .github/workflows/cd.yml | 12 ++- .github/workflows/ci.yml | 22 ++--- .github/workflows/homebrew-validation.yml | 4 +- .github/workflows/release.yml | 72 +++++++++++++---- Dockerfile | 13 ++- Dockerfile.release | 10 ++- SECURITY.md | 62 ++++++++++++++ internal/codeguard/config/io.go | 42 ++++++++++ internal/codeguard/report/write.go | 33 +++++--- .../runner/govulncheck/govulncheck.go | 23 +++++- internal/codeguard/runner/support/corpus.go | 26 +++++- internal/codeguard/runner/support/utils.go | 17 ++++ .../security/config_containment_paths_test.go | 81 +++++++++++++++++++ tests/security/govulncheck_gate_test.go | 61 ++++++++++++++ tests/security/sarif_invocation_test.go | 78 ++++++++++++++++++ tests/security/scan_file_cap_test.go | 50 ++++++++++++ 19 files changed, 576 insertions(+), 56 deletions(-) create mode 100644 .github/CODEOWNERS create mode 100644 SECURITY.md create mode 100644 tests/security/config_containment_paths_test.go create mode 100644 tests/security/govulncheck_gate_test.go create mode 100644 tests/security/sarif_invocation_test.go create mode 100644 tests/security/scan_file_cap_test.go diff --git a/.claude/knowledge/architecture-boundaries.md b/.claude/knowledge/architecture-boundaries.md index 6868e4c..1812969 100644 --- a/.claude/knowledge/architecture-boundaries.md +++ b/.claude/knowledge/architecture-boundaries.md @@ -3,7 +3,7 @@ Key architectural decisions, service boundaries, data flow, integration points, and why things are the way they are. - **Repository config is an untrusted input.** codeguard runs in CI on PRs, so `codeguard.yaml`/rule packs are attacker-controllable. Command execution and non-allowlisted AI provider base URLs are gated behind `internal/codeguard/trust` (a process-global `Policy`), off by default. The trust anchor is env vars / CLI flags (`--allow-config-commands`, `--allow-config-ai-endpoints`), never config. See `docs/security.md`. -- **All config-derived command execution must go through `trust.GuardConfigCommand`** before `exec`. Existing chokepoints: `runner/support/commands.go:runCommandCheck`, `ai/runtime/provider.go` (command provider), `ai/nlrule/runtime.go`, `ai/semantic/runtime.go`. codeguard's *own* tool invocations (git, govulncheck, the built-in `node -e` TS runner) are NOT gated — only commands sourced from config. +- **All config-derived command execution must go through `trust.GuardConfigCommand`** before `exec`. Existing chokepoints: `runner/support/commands.go:runCommandCheck`, `ai/runtime/provider.go` (command provider), `ai/nlrule/runtime.go`, `ai/semantic/runtime.go`, and `runner/govulncheck/govulncheck.go:Run` (gates a config-supplied `security_rules.govulncheck_command` override; the built-in default binary `govulncheck` resolved from PATH stays ungated so default "auto" mode works). codeguard's *own* fixed tool invocations (git, the built-in `node -e` TS runner) are NOT gated — only commands (or command names) sourced from config. **NOTE:** the govulncheck override was historically an RCE hole — it exec'd the config value with no gate and defaulted on for Go targets; do not remove the guard. - **Outbound AI HTTP must use `internal/codeguard/ai/safehttp`**: `ValidateProviderURL` (allowlist of public hosts unless opted in) + `Client` (blocks loopback/private/link-local at dial time) + `MaxResponseBytes`. Triage (`ai/triage`) and the runtime providers (`ai/runtime`) both route through it. Triage merges env-then-config base URL; only the env-sourced value is treated as trusted (`BaseURLFromEnv`). - **OWASP categories are baked into the rule catalog at var-init time**, not via `init()`. The merged `catalog` var calls `withSecurityOWASP(mergeRuleCatalogs(...))` in `rules/catalog.go`; an `init()` would run *after* the `catalog` var initializer copied stale entries. The mapping lives in `rules/catalog_security_owasp.go`. All read paths (`Catalog`, `RuleCatalogForConfig`, SARIF, CLI) flow through `rules.Catalog()`. - Config-controlled artifact paths (`baseline.path`, `cache.path`, `ai.cache.path`) are resolved relative to the config dir and contained within it by `config.containConfigArtifactPaths` (in `config/io.go`, `LoadFile`). Paths escaping the config directory are rejected. @@ -11,7 +11,7 @@ Key architectural decisions, service boundaries, data flow, integration points, - **Secret/credential scanning (`checks/security/security_secrets.go`) runs as its own repository-wide pass in `securityTargetFindings`, unconditionally for BOTH the TS and non-TS branches** — precisely so it covers TS/JS targets that bypass `findingsForFile`. Tiers: `security.hardcoded-credential` (fail; known provider formats + `security_rules.secrets.custom_patterns`), `security.private-key` (fail), `security.hardcoded-secret` (warn; lower-confidence name-based heuristic — was fail before, deliberately downgraded). Patterns match the **raw** line (the token lives in a string literal that masking would blank). Placeholder/allowlist filtering is done in Go after the regex match because RE2 has no lookahead (mirrors `isInsecureDeserialization`). Config: `security_rules.secrets` (`enabled`/`allow_paths`/`allow_patterns`/`custom_patterns`), defaulted in `applySecurityDefaults`, validated in `validateSecretsRules`. - **PERF: the secret scanner is gated by a cheap literal pre-check (`builtinGatePasses`), not a combined regex.** Profiling showed `strings.Contains`/byte scans are ~85x faster than RE2 alternation, and that a leading `\b` or `(?i)` defeats RE2's literal-prefix fast path (those patterns ran ~1.8–7ms per 2000 lines; literal-prefix patterns like the Slack webhook ran ~50µs). The gate is `gateLiterals` (case-sensitive `strings.Contains`) + `gateFoldLiterals` (lowercase, via alloc-free `asciiContainsFold`); when a line contains no marker, the ~17 built-in regexes are skipped. Net: default scan went 3.2→99 MB/s, entropy mode 3.1→47 MB/s. **INVARIANT: the gate must be a superset of every built-in pattern — adding a credential pattern requires adding its required literal to the gate.** `TestBuiltinGateCoversPatterns` enforces this; custom config patterns have arbitrary markers so they bypass the gate and always run. - **CodeQL `go/regex/missing-regexp-anchor` (High) fires on unanchored regexes containing a host literal (e.g. `hooks\.slack\.com`), even for detection patterns.** The repo's accepted fix (see `quality_ai_style_drift.go:17` and the Slack webhook pattern in `security_secret_patterns.go`) is to bound the host with `(?:^|[^A-Za-z0-9_])(...)(?:$|[^A-Za-z0-9_])` — start/non-host-char on both sides — which adds the capture group as the matched value. CodeQL accepts this form; a bare `\b` or `(?:^|...)` left-only boundary may not. Apply this whenever adding a regex with an embedded domain literal. -- **HARDENING (codeguard scans untrusted PR content): the scanner bounds its work** — skips files > `maxScanFileBytes` (5 MiB), scans only the first `maxScanLineBytes` (64 KiB) of any line, and skips binary files (`looksBinary` = NUL byte in first 8 KiB). The git-history parser uses `bufio.Reader.ReadString` (grows for any line length), NOT `bufio.Scanner` — Scanner silently stops on an over-long token, which for a security scan would skip the rest of history undetected. +- **HARDENING (codeguard scans untrusted PR content): the scanner bounds its work** — skips files > `maxScanFileBytes` (5 MiB in the secrets/line scanner), scans only the first `maxScanLineBytes` (64 KiB) of any line, and skips binary files (`looksBinary` = NUL byte in first 8 KiB). Separately, the shared per-scan file corpus (`runner/support/utils.go:WalkFiles` + `corpus.go`) skips files above its own `maxScanFileBytes` (32 MiB) at walk time and reads via `readCappedFile` (LimitReader) — this is the cap that stops a single multi-GB file from OOMing the in-memory corpus; the two constants share a name but live in different packages. The git-history parser uses `bufio.Reader.ReadString` (grows for any line length), NOT `bufio.Scanner` — Scanner silently stops on an over-long token, which for a security scan would skip the rest of history undetected. - **The secret scanner exposes a pure API for reuse: `security.BuildScanner(cfg) Scanner` + `Scanner.ScanContent(content) []Match` + `Scanner.SkipPath(file)`.** `secretFindingsForFile` wraps `ScanContent` with `env.NewFinding`. Tiers in `scanLine` (one match per line, priority order): private-key → `credentialPatterns` (known formats + DB/Bearer/Azure, fail) → config `custom_patterns` → name-based (warn) → opt-in entropy (`security.high-entropy-string`, warn). Values are masked in messages via `maskSecret` (first4…last4). Entropy = `shannonEntropy` (bits/char) over whitespace-free quoted literals, gated by `secrets.entropy` (off by default; min_length 20, threshold 4.5). GitHub-token CRC32 checksum validation was deliberately NOT added — getting the algorithm slightly wrong yields false negatives (worse than a false positive for a security tool). - **Git-history secret scan lives in its own top-level package `internal/codeguard/history` (shells `git log -p -U0`), exposed via `pkg/codeguard.ScanGitHistory` and the `scan-history` CLI command.** It MUST be a separate package, not `runner/support`: `checks/security` imports `runner/support` (for `MatchPattern`), so `runner/support` importing `checks/security` would cycle. `history` imports `checks/security` (above it) and is consumed by `pkg` + CLI (both top-level). The parser tracks `+++ b/`, hunk headers (`@@ … +c`), and `+`/`-` line prefixes to attribute added lines to commits; dedupes by rule+path+masked-message keeping the newest commit (log is newest-first). - **GOTCHA: `support.ScanTargetFiles` caches per-file findings keyed by `(sectionID, target.Path, rel)`.** If two passes scan the same files with the same `sectionID`, the second pass gets a cache hit and returns the FIRST pass's findings. The secret pass therefore uses a distinct cache id `"security-secrets"` (not `"security"`) so it doesn't collide with the language pass. `sectionID` here only affects the cache key — findings still land in the real section via their RuleID/`FinalizeSection`. When adding any additional `ScanTargetFiles` call to an existing section, give it a unique cache id. diff --git a/.codeguard/codeguard.yaml b/.codeguard/codeguard.yaml index 41cbb6b..97f828e 100644 --- a/.codeguard/codeguard.yaml +++ b/.codeguard/codeguard.yaml @@ -56,7 +56,7 @@ checks: - RELEASE_PLEASE_TOKEN - path: .github/workflows/release.yml required_contains: - - goreleaser/goreleaser-action@v7 + - goreleaser/goreleaser-action - sync-homebrew-formula - Formula/codeguard.rb required_release_files: diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000..94f426a --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,20 @@ +# Code owners for codeguard. Owners are automatically requested for review on +# any pull request that touches matching paths, and — combined with branch +# protection's "Require review from Code Owners" — provide the change-management +# evidence (SOC 3 CC8.1) that changes are reviewed by an accountable party. +# +# Replace @alxxjohn with a @devr-tools/ handle once a maintainers team +# exists. Owners must have write access to the repo or GitHub ignores the entry. + +# Default owner for everything not matched below. +* @alxxjohn + +# Security-sensitive surfaces warrant explicit ownership. +/.github/ @alxxjohn +/.github/workflows/ @alxxjohn +/Dockerfile @alxxjohn +/Dockerfile.release @alxxjohn +/.goreleaser.yaml @alxxjohn +/SECURITY.md @alxxjohn +/internal/codeguard/trust/ @alxxjohn +/internal/codeguard/ai/safehttp/ @alxxjohn diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml index e991412..12ca299 100644 --- a/.github/workflows/cd.yml +++ b/.github/workflows/cd.yml @@ -34,7 +34,7 @@ jobs: tag: ${{ steps.version.outputs.tag }} steps: - name: Check out code - uses: actions/checkout@v7 + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: fetch-depth: 0 @@ -114,7 +114,9 @@ jobs: with: tag: ${{ needs.prepare-prerelease.outputs.tag }} prerelease: true - secrets: inherit + create_missing_tag: true + secrets: + RELEASE_PLEASE_TOKEN: ${{ secrets.RELEASE_PLEASE_TOKEN }} release-please: if: github.event_name == 'push' && (github.ref_name == 'main' || github.ref_name == 'master') @@ -142,7 +144,7 @@ jobs: - name: Run release-please id: release - uses: googleapis/release-please-action@v5 + uses: googleapis/release-please-action@45996ed1f6d02564a971a2fa1b5860e934307cf7 # v5 with: token: ${{ secrets.RELEASE_PLEASE_TOKEN }} config-file: .github/release-please-config.json @@ -160,4 +162,6 @@ jobs: with: tag: ${{ needs.release-please.outputs.tag_name }} prerelease: false - secrets: inherit + create_missing_tag: true + secrets: + RELEASE_PLEASE_TOKEN: ${{ secrets.RELEASE_PLEASE_TOKEN }} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f24e320..6825aee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,9 +24,9 @@ jobs: name: fmt runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: go.mod cache: true @@ -39,9 +39,9 @@ jobs: needs: fmt runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: go.mod cache: true @@ -50,7 +50,7 @@ jobs: run: make lint - name: Run golangci-lint - uses: golangci/golangci-lint-action@v9 + uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9 with: version: v2.12.2 @@ -59,9 +59,9 @@ jobs: needs: lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: go.mod cache: true @@ -82,9 +82,9 @@ jobs: - test runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: go.mod cache: true @@ -100,9 +100,9 @@ jobs: - codeguard runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - - uses: actions/setup-go@v6 + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: go.mod cache: true diff --git a/.github/workflows/homebrew-validation.yml b/.github/workflows/homebrew-validation.yml index d7c9de8..c0b82b9 100644 --- a/.github/workflows/homebrew-validation.yml +++ b/.github/workflows/homebrew-validation.yml @@ -22,10 +22,10 @@ jobs: steps: - name: Check out code - uses: actions/checkout@v7 + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - name: Set up Homebrew - uses: Homebrew/actions/setup-homebrew@main + uses: Homebrew/actions/setup-homebrew@64bb52ce84da6455bdbd6df88ac86151ba2ff16c # main - name: Clone Homebrew tap shell: bash diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index e6c57f5..e5c1768 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -11,6 +11,19 @@ on: description: Whether this release should stay in the prerelease channel. required: true type: boolean + create_missing_tag: + description: >- + Allow creating and pushing the tag if it does not yet exist. Set only + by the trusted automation callers (cd.yml) so freshly-computed + prerelease tags can be minted. Manual workflow_dispatch cannot set + this, so a dispatched run must reference an existing tag. + required: false + default: false + type: boolean + secrets: + RELEASE_PLEASE_TOKEN: + description: Token used to open the Homebrew tap formula PR. + required: false workflow_dispatch: inputs: tag: @@ -22,15 +35,17 @@ on: required: true type: boolean +# Least privilege by default; jobs elevate only what they need. permissions: - contents: write - packages: write - # Required for keyless cosign signing and SLSA provenance (OIDC token). - id-token: write + contents: read jobs: build-release: runs-on: ubuntu-latest + permissions: + contents: write # create the GitHub release and push a missing tag + packages: write # push the container image to GHCR + id-token: write # keyless cosign signing + SLSA provenance OIDC token outputs: prerelease: ${{ steps.release.outputs.prerelease }} tag: ${{ steps.release.outputs.tag }} @@ -82,11 +97,14 @@ jobs: - name: Validate inputs shell: bash + env: + TAG: ${{ steps.release.outputs.tag }} + PRERELEASE: ${{ steps.release.outputs.prerelease }} run: | set -euo pipefail - tag="${{ steps.release.outputs.tag }}" - prerelease="${{ steps.release.outputs.prerelease }}" + tag="$TAG" + prerelease="$PRERELEASE" tag_pattern='^([A-Za-z0-9._-]+-)?v[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?$' prerelease_pattern='^([A-Za-z0-9._-]+-)?v[0-9]+\.[0-9]+\.[0-9]+-[0-9A-Za-z.-]+$' @@ -107,16 +125,19 @@ jobs: fi - name: Check out code - uses: actions/checkout@v7 + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: fetch-depth: 0 - name: Ensure requested tag exists shell: bash + env: + TAG: ${{ steps.release.outputs.tag }} + ALLOW_TAG_CREATE: ${{ inputs.create_missing_tag }} run: | set -euo pipefail - tag="${{ steps.release.outputs.tag }}" + tag="$TAG" git fetch --tags --force @@ -124,41 +145,50 @@ jobs: : elif git ls-remote --exit-code --tags origin "refs/tags/$tag" >/dev/null 2>&1; then git fetch origin "refs/tags/$tag:refs/tags/$tag" - else + elif [ "$ALLOW_TAG_CREATE" = "true" ]; then + # Only reachable from the trusted automation callers (cd.yml), which + # set create_missing_tag=true so a freshly-computed prerelease tag can + # be minted. A manual workflow_dispatch cannot set this input, so it + # cannot forge an arbitrary release tag from an arbitrary SHA and + # bypass the release-please approval flow. git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" git tag -a "$tag" -m "Release $tag" "$GITHUB_SHA" git push origin "refs/tags/$tag" + else + echo "tag '$tag' does not exist and will not be created for this run." + echo "Push the tag first, or use the release-please flow, before dispatching a release." + exit 1 fi git checkout "$tag" - name: Set up Go - uses: actions/setup-go@v6 + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: go.mod - name: Set up QEMU - uses: docker/setup-qemu-action@v4 + uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v4 + uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4 - name: Install cosign - uses: sigstore/cosign-installer@v3 + uses: sigstore/cosign-installer@398d4b0eeef1380460a10c8013a76f728fb906ac # v3 - name: Install syft (SBOM generation) - uses: anchore/sbom-action/download-syft@v0 + uses: anchore/sbom-action/download-syft@e22c389904149dbc22b58101806040fa8d37a610 # v0 - name: Log in to GHCR - uses: docker/login-action@v4 + uses: docker/login-action@c99871dec2022cc055c062a10cc1a1310835ceb4 # v4 with: registry: ghcr.io username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} - name: Run GoReleaser - uses: goreleaser/goreleaser-action@v7 + uses: goreleaser/goreleaser-action@f06c13b6b1a9625abc9e6e439d9c05a8f2190e94 # v7 with: distribution: goreleaser version: "~> v2" @@ -177,7 +207,7 @@ jobs: echo "hashes=$hashes" >>"$GITHUB_OUTPUT" - name: Upload release bundle - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: dist-${{ steps.release.outputs.tag }} path: dist/* @@ -191,6 +221,10 @@ jobs: actions: read id-token: write contents: write + # Must be referenced by an immutable semantic-version tag, not a commit SHA: + # the SLSA generator verifies its own provenance against the tagged ref, and + # a SHA pin breaks that check. This is the one intentional exception to the + # SHA-pinning policy. uses: slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@v2.1.0 with: base64-subjects: ${{ needs.build-release.outputs.hashes }} @@ -200,6 +234,10 @@ jobs: needs: build-release if: needs.build-release.outputs.prerelease != 'true' runs-on: ubuntu-latest + # Writes only to the external tap repo via RELEASE_PLEASE_TOKEN; needs no + # write scope on GITHUB_TOKEN. + permissions: + contents: read env: TAP_REPOSITORY: devr-tools/homebrew-tap steps: diff --git a/Dockerfile b/Dockerfile index 9faa90f..ea0f93b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,7 @@ -FROM golang:1.26 AS build +# Base images are digest-pinned for build-input integrity (OWASP A08). The +# trailing tag comment records the human-readable version; Dependabot bumps the +# digest. +FROM golang:1.26@sha256:f96cc555eb8db430159a3aa6797cd5bae561945b7b0fe7d0e284c63a3b291609 AS build WORKDIR /src @@ -8,13 +11,17 @@ RUN go mod download COPY . . RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -trimpath -o /out/codeguard ./cmd/codeguard -FROM alpine:3.24 +FROM alpine:3.24@sha256:28bd5fe8b56d1bd048e5babf5b10710ebe0bae67db86916198a6eec434943f8b WORKDIR /workspace -RUN apk add --no-cache git +RUN apk add --no-cache git \ + && addgroup -S codeguard && adduser -S -G codeguard -H codeguard COPY --from=build /out/codeguard /usr/local/bin/codeguard +# Run as an unprivileged user; codeguard only needs to read the mounted repo. +USER codeguard + ENTRYPOINT ["codeguard"] CMD ["help"] diff --git a/Dockerfile.release b/Dockerfile.release index f88a01d..b453428 100644 --- a/Dockerfile.release +++ b/Dockerfile.release @@ -1,7 +1,13 @@ -FROM alpine:3.24 +# Digest-pinned for build-input integrity (OWASP A08); tag comment is +# human-readable and Dependabot bumps the digest. +FROM alpine:3.24@sha256:28bd5fe8b56d1bd048e5babf5b10710ebe0bae67db86916198a6eec434943f8b -RUN apk --no-cache add ca-certificates +RUN apk --no-cache add ca-certificates \ + && addgroup -S codeguard && adduser -S -G codeguard -H codeguard COPY codeguard /usr/bin/codeguard +# Run as an unprivileged user; codeguard only needs to read the mounted repo. +USER codeguard + ENTRYPOINT ["/usr/bin/codeguard"] diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..145d8ae --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,62 @@ +# Security Policy + +## Reporting a vulnerability + +We take the security of codeguard seriously. If you believe you have found a +security vulnerability, please report it privately — **do not open a public +issue, pull request, or discussion for security reports.** + +Preferred channel: use GitHub's private vulnerability reporting for this +repository. Go to the **Security** tab → **Report a vulnerability** +(). This opens a +private advisory visible only to you and the maintainers. + +If private reporting is unavailable to you, contact a maintainer directly rather +than disclosing publicly, and we will open a private advisory on your behalf. + +Please include, where possible: + +- a description of the vulnerability and its impact; +- the affected version (`codeguard version`) and platform; +- step-by-step reproduction, ideally with a minimal repository or config; and +- any proof-of-concept, logs, or suggested remediation. + +## Our commitment + +- **Acknowledgement** within **3 business days** of your report. +- **Triage and severity assessment** (CVSS-based) within **10 business days**, + including whether we accept the report and an initial remediation plan. +- **Progress updates** at least every **10 business days** until resolution. +- **Coordinated disclosure**: we aim to ship a fix and publish an advisory + within **90 days** of triage, and will credit reporters who wish to be named. + +Please give us a reasonable opportunity to remediate before any public +disclosure. + +## Supported versions + +codeguard is pre-1.0 and released from the latest tag. Security fixes are +applied to the most recent release only; please upgrade to the latest version +before reporting. + +| Version | Supported | +| --- | --- | +| Latest release | ✅ | +| Older releases | ❌ | + +## Scope + +In scope: the codeguard CLI, its GitHub Action, released container images, and +the release/supply-chain pipeline in this repository. + +The product's own trust model (running untrusted repository configuration in CI) +and its OWASP Top 10 coverage are documented in +[docs/security.md](docs/security.md). Reports that the tool executes +config-supplied commands, reaches non-allowlisted network endpoints, or writes +outside the configured directory **without the documented opt-in** are in scope +and valued. + +Out of scope: findings that require the operator to have explicitly enabled an +untrusted capability (`--allow-config-commands`, +`CODEGUARD_ALLOW_CONFIG_AI_ENDPOINTS`, etc.), and vulnerabilities in +third-party dependencies that have no impact on codeguard as shipped. diff --git a/internal/codeguard/config/io.go b/internal/codeguard/config/io.go index 9dfdd78..eb078a0 100644 --- a/internal/codeguard/config/io.go +++ b/internal/codeguard/config/io.go @@ -101,9 +101,51 @@ func containedPath(baseDir, p string) (string, error) { if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return "", fmt.Errorf("path %q escapes the config directory %q", p, absBase) } + + // The lexical check above is not enough: a symlink committed inside the + // config directory could still redirect a write outside it. Canonicalize + // both the base and the target's deepest existing ancestor (the target + // itself is an output path that may not exist yet) and re-check containment. + realBase, err := canonicalizeExistingPrefix(absBase) + if err != nil { + return "", err + } + realResolved, err := canonicalizeExistingPrefix(resolved) + if err != nil { + return "", err + } + realRel, err := filepath.Rel(realBase, realResolved) + if err != nil || realRel == ".." || strings.HasPrefix(realRel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("path %q resolves outside the config directory %q via a symlink", p, absBase) + } + return resolved, nil } +// canonicalizeExistingPrefix resolves symlinks in path, tolerating a path that +// does not exist yet by resolving the deepest existing ancestor and re-joining +// the remaining (necessarily non-symlink) components. +func canonicalizeExistingPrefix(path string) (string, error) { + current := filepath.Clean(path) + remainder := "" + for { + resolved, err := filepath.EvalSymlinks(current) + if err == nil { + return filepath.Join(resolved, remainder), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + parent := filepath.Dir(current) + if parent == current { + // Reached the filesystem root without an existing ancestor. + return filepath.Join(current, remainder), nil + } + remainder = filepath.Join(filepath.Base(current), remainder) + current = parent + } +} + func resolveRelativePaths(cfg *core.Config, baseDir string) { for i := range cfg.Targets { targetPath := strings.TrimSpace(cfg.Targets[i].Path) diff --git a/internal/codeguard/report/write.go b/internal/codeguard/report/write.go index f049827..0747a71 100644 --- a/internal/codeguard/report/write.go +++ b/internal/codeguard/report/write.go @@ -9,6 +9,7 @@ import ( "github.com/devr-tools/codeguard/internal/codeguard/core" rulespkg "github.com/devr-tools/codeguard/internal/codeguard/rules" + "github.com/devr-tools/codeguard/internal/version" ) func Write(w io.Writer, report core.Report, format string) error { @@ -92,18 +93,32 @@ func writeSARIF(w io.Writer, report core.Report) error { } } sort.Slice(sarifRules, func(i, j int) bool { return sarifRules[i].ID < sarifRules[j].ID }) - payload["runs"] = []any{ - map[string]any{ - "tool": map[string]any{ - "driver": map[string]any{ - "name": "codeguard", - "rules": sarifRules, - }, + + // invocation records that codeguard ran, so a consumer can attribute the + // SARIF file to a specific run (SOC 3 monitoring / audit trail). The analysis + // completing successfully is independent of whether findings were reported. + invocation := map[string]any{"executionSuccessful": true} + if report.GeneratedAt != "" { + invocation["endTimeUtc"] = report.GeneratedAt + } + run := map[string]any{ + "tool": map[string]any{ + "driver": map[string]any{ + "name": "codeguard", + "version": version.Number, + "semanticVersion": strings.TrimPrefix(version.Number, "v"), + "informationUri": "https://github.com/devr-tools/codeguard", + "rules": sarifRules, }, - "taxonomies": []any{owaspTaxonomy()}, - "results": results, }, + "taxonomies": []any{owaspTaxonomy()}, + "invocations": []any{invocation}, + "results": results, + } + if report.Profile != "" { + run["properties"] = map[string]any{"profile": report.Profile} } + payload["runs"] = []any{run} enc := json.NewEncoder(w) enc.SetIndent("", " ") return enc.Encode(payload) diff --git a/internal/codeguard/runner/govulncheck/govulncheck.go b/internal/codeguard/runner/govulncheck/govulncheck.go index 8b68646..ea053ba 100644 --- a/internal/codeguard/runner/govulncheck/govulncheck.go +++ b/internal/codeguard/runner/govulncheck/govulncheck.go @@ -9,8 +9,16 @@ import ( "github.com/devr-tools/codeguard/internal/codeguard/core" runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" + "github.com/devr-tools/codeguard/internal/codeguard/trust" ) +// defaultCommand is the built-in govulncheck binary name. It is resolved from +// PATH (never the working directory) and is a static analyzer that does not +// execute the code it scans, so it is safe to run unguarded against untrusted +// repositories. Any other command name comes from repository configuration and +// must pass the command-trust gate. +const defaultCommand = "govulncheck" + // limitedWriter writes to w until remaining bytes are exhausted, then silently // drops the rest while recording that truncation occurred. It lets a single // writer back both cmd.Stdout and cmd.Stderr under a shared byte budget. @@ -41,10 +49,19 @@ func (l *limitedWriter) Write(p []byte) (int, error) { const maxOutputBytes = 64 << 20 // 64 MiB func Run(ctx context.Context, dir string, cmdName string, sc runnersupport.Context) ([]core.Finding, error) { - if strings.TrimSpace(cmdName) == "" { - cmdName = "govulncheck" + cmdName = strings.TrimSpace(cmdName) + if cmdName == "" { + cmdName = defaultCommand + } + // A config-supplied override of the govulncheck binary is untrusted (the repo + // under scan may be an untrusted pull request) and must pass the command-trust + // gate. The built-in default is exempt so the default "auto" mode keeps working. + if cmdName != defaultCommand { + if err := trust.GuardConfigCommand("govulncheck_command", cmdName); err != nil { + return nil, err + } } - cmd := exec.CommandContext(ctx, cmdName, "./...") + cmd := exec.CommandContext(ctx, cmdName, "./...") //nolint:gosec // config override gated by trust.GuardConfigCommand above; default resolves from PATH cmd.Dir = dir var buf bytes.Buffer limited := &limitedWriter{w: &buf, remaining: maxOutputBytes} diff --git a/internal/codeguard/runner/support/corpus.go b/internal/codeguard/runner/support/corpus.go index 483ccfa..3624f4d 100644 --- a/internal/codeguard/runner/support/corpus.go +++ b/internal/codeguard/runner/support/corpus.go @@ -1,14 +1,36 @@ package support import ( + "fmt" "go/ast" "go/parser" "go/token" + "io" "os" "path/filepath" "sync" ) +// readCappedFile reads path but refuses to buffer more than maxScanFileBytes, +// bounding memory even if a file grew past the walk-time size filter (TOCTOU) or +// is read outside the walk. It reads one byte past the cap to distinguish an +// exactly-cap-sized file from an oversized one. +func readCappedFile(path string) ([]byte, error) { + f, err := os.Open(path) //nolint:gosec // path enumerated by WalkFiles under the scan root + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + data, err := io.ReadAll(io.LimitReader(f, maxScanFileBytes+1)) + if err != nil { + return nil, err + } + if len(data) > maxScanFileBytes { + return nil, fmt.Errorf("file %q exceeds the %d byte scan limit", path, maxScanFileBytes) + } + return data, nil +} + // fileCorpus memoizes, for the lifetime of a single scan, the expensive work // that would otherwise be repeated by every check section: the per-target // directory walk, the individual file reads, and Go AST parses. Every by-value @@ -83,7 +105,7 @@ func (c *fileCorpus) read(root string, rel string) ([]byte, error) { c.mu.Unlock() entry.once.Do(func() { - entry.data, entry.err = os.ReadFile(filepath.Join(root, rel)) //nolint:gosec // rel enumerated by WalkFiles under root + entry.data, entry.err = readCappedFile(filepath.Join(root, rel)) }) return entry.data, entry.err } @@ -129,7 +151,7 @@ func (sc Context) corpusRead(root string, rel string) ([]byte, error) { if sc.corpus != nil { return sc.corpus.read(root, rel) } - return os.ReadFile(filepath.Join(root, rel)) //nolint:gosec // rel enumerated by WalkFiles under root + return readCappedFile(filepath.Join(root, rel)) } // ParseGoFile returns a shared, read-only Go AST for the given source, parsed at diff --git a/internal/codeguard/runner/support/utils.go b/internal/codeguard/runner/support/utils.go index 92d15d9..50aba81 100644 --- a/internal/codeguard/runner/support/utils.go +++ b/internal/codeguard/runner/support/utils.go @@ -11,6 +11,15 @@ import ( "github.com/devr-tools/codeguard/internal/codeguard/core" ) +// maxScanFileBytes caps the size of an individual scanned repository file that +// codeguard will list and read into memory. Files above this are skipped: the +// repository under scan may be an untrusted pull request, and without a cap a +// single multi-gigabyte file (or many large ones) read fully into the in-memory +// file corpus could exhaust the CI runner (denial of service). Real source files +// are far smaller than this; oversized inputs are almost always generated blobs +// or vendored bundles that are not useful to scan. +const maxScanFileBytes = 32 << 20 // 32 MiB + // patternCache memoizes compiled glob patterns keyed by the raw glob string. // MatchPattern is the hottest compile site in the codebase (per file × per // pattern during the walk), so compiling once and reusing avoids recompiling @@ -57,6 +66,14 @@ func WalkFiles(root string, excludes []string, include func(string) bool) ([]str if d.IsDir() { return nil } + info, err := d.Info() + if err != nil { + return err + } + // Skip oversized files to bound scan memory (see maxScanFileBytes). + if info.Size() > maxScanFileBytes { + return nil + } if include(rel) { files = append(files, rel) } diff --git a/tests/security/config_containment_paths_test.go b/tests/security/config_containment_paths_test.go new file mode 100644 index 0000000..0139273 --- /dev/null +++ b/tests/security/config_containment_paths_test.go @@ -0,0 +1,81 @@ +package security_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + service "github.com/devr-tools/codeguard/pkg/codeguard" +) + +// writeConfig writes an example config into dir with the caller-supplied cache +// and ai-cache paths, leaving the baseline path empty so each artifact path can +// be exercised in isolation. +func writeConfig(t *testing.T, dir, cachePath, aiCachePath string) string { + t.Helper() + cfg := service.ExampleConfig() + cfg.Baseline.Path = "" + cfg.Cache.Path = cachePath + cfg.AI.Cache.Path = aiCachePath + path := filepath.Join(dir, "codeguard.yaml") + if err := service.WriteConfigFile(path, cfg); err != nil { + t.Fatalf("write config: %v", err) + } + return path +} + +func TestLoadConfigRejectsCachePathEscape(t *testing.T) { + dir := t.TempDir() + path := writeConfig(t, dir, "../escape-cache.json", "") + + if _, err := service.LoadConfigFile(path); err == nil { + t.Fatal("expected cache.path escaping the config directory to be rejected") + } +} + +func TestLoadConfigRejectsAICachePathEscape(t *testing.T) { + dir := t.TempDir() + path := writeConfig(t, dir, "", "../../escape-ai-cache.json") + + if _, err := service.LoadConfigFile(path); err == nil { + t.Fatal("expected ai.cache.path escaping the config directory to be rejected") + } +} + +func TestLoadConfigAllowsContainedCachePaths(t *testing.T) { + dir := t.TempDir() + path := writeConfig(t, dir, ".codeguard/cache.json", ".codeguard/ai-cache.json") + + cfg, err := service.LoadConfigFile(path) + if err != nil { + t.Fatalf("contained cache paths should load, got %v", err) + } + if want := filepath.Join(dir, ".codeguard/cache.json"); cfg.Cache.Path != want { + t.Fatalf("cache.path = %q, want %q", cfg.Cache.Path, want) + } + if want := filepath.Join(dir, ".codeguard/ai-cache.json"); cfg.AI.Cache.Path != want { + t.Fatalf("ai.cache.path = %q, want %q", cfg.AI.Cache.Path, want) + } +} + +// A symlink committed inside the config directory must not be usable to redirect +// an artifact write outside it. The lexical containment check passes here (the +// path is textually under the config dir), so this is the regression guard for +// the symlink-aware resolution. +func TestLoadConfigRejectsSymlinkEscape(t *testing.T) { + dir := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(dir, "link")); err != nil { + t.Skipf("symlinks unsupported on this platform: %v", err) + } + path := writeConfig(t, dir, "link/evil-cache.json", "") + + _, err := service.LoadConfigFile(path) + if err == nil { + t.Fatal("expected a cache.path routed through an in-config symlink to be rejected") + } + if !strings.Contains(err.Error(), "symlink") { + t.Fatalf("expected a symlink containment error, got %v", err) + } +} diff --git a/tests/security/govulncheck_gate_test.go b/tests/security/govulncheck_gate_test.go new file mode 100644 index 0000000..62dc3dc --- /dev/null +++ b/tests/security/govulncheck_gate_test.go @@ -0,0 +1,61 @@ +package security_test + +import ( + "context" + "errors" + "testing" + + govulncheckrunner "github.com/devr-tools/codeguard/internal/codeguard/runner/govulncheck" + runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" + "github.com/devr-tools/codeguard/internal/codeguard/trust" +) + +// A config-supplied override of govulncheck_command must not execute under the +// default (deny) trust policy: it is an arbitrary binary from a possibly +// untrusted pull request. This is the regression guard for the RCE that existed +// when the govulncheck runner exec'd the config command without the trust gate. +func TestGovulncheckOverrideRefusedByDefault(t *testing.T) { + trust.Set(trust.Policy{}) + t.Cleanup(trust.ResetFromEnv) + + _, err := govulncheckrunner.Run(context.Background(), t.TempDir(), "/bin/sh", runnersupport.Context{}) + if err == nil { + t.Fatal("expected config-supplied govulncheck command to be refused under the default policy") + } + var disabled trust.ErrConfigCommandsDisabled + if !errors.As(err, &disabled) { + t.Fatalf("expected ErrConfigCommandsDisabled, got %T: %v", err, err) + } + if disabled.Command != "/bin/sh" { + t.Fatalf("error did not capture the refused command, got %q", disabled.Command) + } +} + +// The built-in default binary (empty or "govulncheck") must remain exempt so the +// default "auto" mode keeps working without --allow-config-commands. It may fail +// for other reasons (binary absent), but never with the trust-gate error. +func TestGovulncheckDefaultCommandNotGated(t *testing.T) { + trust.Set(trust.Policy{}) + t.Cleanup(trust.ResetFromEnv) + + for _, cmd := range []string{"", "govulncheck"} { + _, err := govulncheckrunner.Run(context.Background(), t.TempDir(), cmd, runnersupport.Context{}) + var disabled trust.ErrConfigCommandsDisabled + if errors.As(err, &disabled) { + t.Fatalf("default command %q should not be gated by the trust policy", cmd) + } + } +} + +// With the operator opt-in, a config-supplied command passes the gate (it may +// still fail to execute, but not with the trust-gate error). +func TestGovulncheckOverrideAllowedWhenOptedIn(t *testing.T) { + trust.Set(trust.Policy{AllowConfigCommands: true}) + t.Cleanup(trust.ResetFromEnv) + + _, err := govulncheckrunner.Run(context.Background(), t.TempDir(), "/bin/sh", runnersupport.Context{}) + var disabled trust.ErrConfigCommandsDisabled + if errors.As(err, &disabled) { + t.Fatal("config command should be permitted once AllowConfigCommands is set") + } +} diff --git a/tests/security/sarif_invocation_test.go b/tests/security/sarif_invocation_test.go new file mode 100644 index 0000000..feabdc4 --- /dev/null +++ b/tests/security/sarif_invocation_test.go @@ -0,0 +1,78 @@ +package security_test + +import ( + "bytes" + "encoding/json" + "testing" + + "github.com/devr-tools/codeguard/internal/codeguard/core" + "github.com/devr-tools/codeguard/internal/codeguard/report" + "github.com/devr-tools/codeguard/internal/version" +) + +// SARIF output must carry the tool version and an invocation record so a CI +// consumer can attribute a results file to a specific codeguard run (SOC 3 +// monitoring / audit trail). +func TestSARIFCarriesVersionAndInvocation(t *testing.T) { + rep := core.Report{ + Profile: "default", + GeneratedAt: "2026-07-02T12:00:00Z", + Sections: []core.SectionResult{{ + Name: "Security", + Findings: []core.Finding{{ + RuleID: "security.taint.go", + Level: "fail", + Message: "tainted input reaches exec.Command", + Path: "main.go", + Line: 10, + }}, + }}, + } + + var buf bytes.Buffer + if err := report.Write(&buf, rep, "sarif"); err != nil { + t.Fatalf("write sarif: %v", err) + } + + var doc struct { + Runs []struct { + Tool struct { + Driver struct { + Name string `json:"name"` + Version string `json:"version"` + InformationURI string `json:"informationUri"` + } `json:"driver"` + } `json:"tool"` + Invocations []struct { + ExecutionSuccessful bool `json:"executionSuccessful"` + EndTimeUtc string `json:"endTimeUtc"` + } `json:"invocations"` + } `json:"runs"` + } + if err := json.Unmarshal(buf.Bytes(), &doc); err != nil { + t.Fatalf("parse sarif: %v\n%s", err, buf.String()) + } + if len(doc.Runs) != 1 { + t.Fatalf("expected 1 run, got %d", len(doc.Runs)) + } + d := doc.Runs[0].Tool.Driver + if d.Name != "codeguard" { + t.Errorf("driver.name = %q, want codeguard", d.Name) + } + if d.Version != version.Number { + t.Errorf("driver.version = %q, want %q", d.Version, version.Number) + } + if d.InformationURI == "" { + t.Error("driver.informationUri must be set") + } + inv := doc.Runs[0].Invocations + if len(inv) != 1 { + t.Fatalf("expected 1 invocation, got %d", len(inv)) + } + if !inv[0].ExecutionSuccessful { + t.Error("invocation.executionSuccessful must be true") + } + if inv[0].EndTimeUtc != rep.GeneratedAt { + t.Errorf("invocation.endTimeUtc = %q, want %q", inv[0].EndTimeUtc, rep.GeneratedAt) + } +} diff --git a/tests/security/scan_file_cap_test.go b/tests/security/scan_file_cap_test.go new file mode 100644 index 0000000..8559edb --- /dev/null +++ b/tests/security/scan_file_cap_test.go @@ -0,0 +1,50 @@ +package security_test + +import ( + "os" + "path/filepath" + "testing" + + runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" +) + +// An untrusted repository must not be able to exhaust scan memory with an +// oversized file: files above the scan size cap are skipped by the walk so no +// section ever reads them into memory. Normal-sized files are still listed. +func TestWalkFilesSkipsOversizedFiles(t *testing.T) { + root := t.TempDir() + + small := filepath.Join(root, "small.go") + if err := os.WriteFile(small, []byte("package main\n"), 0o600); err != nil { + t.Fatalf("write small file: %v", err) + } + + // A sparse file just over the 32 MiB cap; truncate does not allocate blocks. + big := filepath.Join(root, "big.go") + f, err := os.Create(big) + if err != nil { + t.Fatalf("create big file: %v", err) + } + if err = f.Truncate((32 << 20) + 1); err != nil { + t.Fatalf("truncate big file: %v", err) + } + if err = f.Close(); err != nil { + t.Fatalf("close big file: %v", err) + } + + files, err := runnersupport.WalkFiles(root, nil, func(string) bool { return true }) + if err != nil { + t.Fatalf("walk: %v", err) + } + + got := map[string]bool{} + for _, rel := range files { + got[rel] = true + } + if !got["small.go"] { + t.Errorf("expected small.go to be listed, got %v", files) + } + if got["big.go"] { + t.Errorf("oversized big.go must be skipped, got %v", files) + } +}