From c9ebcb50ae814ab0fe57106d27d64515009c19e8 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 16:19:25 +0300 Subject: [PATCH 1/2] Add security CI baseline and fuzz/adversarial update-integrity evals --- .github/workflows/codeql.yml | 43 ++++++++ .github/workflows/security.yml | 82 +++++++++++++++ go.mod | 2 +- updater.go | 14 ++- zz_fuzz_evals_test.go | 175 +++++++++++++++++++++++++++++++++ zz_more_test.go | 12 +-- 6 files changed, 319 insertions(+), 9 deletions(-) create mode 100644 .github/workflows/codeql.yml create mode 100644 .github/workflows/security.yml create mode 100644 zz_fuzz_evals_test.go diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..cf75040 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,43 @@ +name: codeql + +on: + push: + branches: [main] + pull_request: + branches: [main] + schedule: + - cron: '27 4 * * 1' + +permissions: + contents: read + +jobs: + analyze: + name: analyze (go) + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + actions: read + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + cache-dependency-path: go.sum + + - name: Initialize CodeQL + uses: github/codeql-action/init@v4 + with: + languages: go + queries: security-extended + + - name: Build + run: go build ./... + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v4 + with: + category: '/language:go' diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml new file mode 100644 index 0000000..ccbd990 --- /dev/null +++ b/.github/workflows/security.yml @@ -0,0 +1,82 @@ +name: security + +on: + push: + branches: [main] + pull_request: + branches: [main] + +permissions: + contents: read + +jobs: + # Race-detector gate. ci.yml runs the suite with coverage; this is the + # explicit, security-scoped -race gate so a data race fails the security + # workflow independently of the coverage upload. + race: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + cache-dependency-path: go.sum + - name: go vet + run: go vet ./... + - name: go test -race + run: go test -race ./... + + # SAST. Accepted rule exclusions (documented, minimal — never blanket): + # G104 — best-effort os.Remove/Close on error-cleanup paths. + # G204 — exec of gh / launchctl with internally-built args (no user input). + # G301 — staging/install dirs are 0755 (operator-readable install tree). + # G302 — extracted artifacts are executables and must be 0755. + # G304 — file paths derive from InstallDir / temp dir, inherent to an updater. + # G404 — math/rand used only for thundering-herd jitter, not for secrets. + # G110 (decompression bomb) is FIXED in code (per-entry LimitReader), not excluded. + gosec: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: securego/gosec@v2.27.1 + with: + args: -exclude=G104,G204,G301,G302,G304,G404 -exclude-dir=.git ./... + + # Known-vulnerability scan of the call graph (stdlib + deps). + govulncheck: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v6 + with: + go-version: '1.25' + cache: true + cache-dependency-path: go.sum + - name: govulncheck + uses: golang/govulncheck-action@v1.0.4 + with: + go-version-input: '1.25' + go-package: ./... + + # Secret scan over the full history on push, the PR range on PR. + gitleaks: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - uses: gitleaks/gitleaks-action@v3.0.0 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Block PRs that introduce dependencies with known vulnerabilities or + # incompatible licenses. PR-only (needs the base..head diff). + dependency-review: + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/dependency-review-action@v3 + with: + fail-on-severity: high diff --git a/go.mod b/go.mod index 9b34f4e..c10f570 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,5 @@ module github.com/pilot-protocol/updater -go 1.25.10 +go 1.25.11 require github.com/pilot-protocol/common v0.5.0 diff --git a/updater.go b/updater.go index 532c35d..1e1afef 100644 --- a/updater.go +++ b/updater.go @@ -592,14 +592,24 @@ func extractTarGz(archivePath, destDir string) error { } dst := filepath.Join(destDir, name) - out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755) + out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755) //nolint:gosec // G302: extracted files are executables and must be 0755 if err != nil { return fmt.Errorf("create %s: %w", name, err) } - if _, err := io.Copy(out, tr); err != nil { + // Cap per-entry extraction to bound a decompression bomb: a + // crafted archive could expand far beyond maxDownloadBytes even + // after the on-disk size was capped at download time. Copy one + // byte past the cap so we can tell "exactly at limit" from + // "exceeded". + n, err := io.Copy(out, io.LimitReader(tr, maxDownloadBytes+1)) + if err != nil { out.Close() return fmt.Errorf("write %s: %w", name, err) } + if n > maxDownloadBytes { + out.Close() + return fmt.Errorf("archive entry %q exceeds max extract size %d bytes", name, maxDownloadBytes) + } out.Close() } return nil diff --git a/zz_fuzz_evals_test.go b/zz_fuzz_evals_test.go new file mode 100644 index 0000000..23a38ed --- /dev/null +++ b/zz_fuzz_evals_test.go @@ -0,0 +1,175 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package updater + +import ( + "crypto/sha256" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// FuzzVerifyChecksum exercises the checksums.txt line parser inside +// VerifyChecksum against arbitrary, attacker-controlled checksums file +// contents. The parser walks untrusted release-asset bytes; a panic +// there would be a denial-of-service (and any "accidentally matched" +// path would be a verification bypass). The invariants asserted: +// +// - VerifyChecksum never panics, regardless of input (malformed +// lines, embedded NULs, giant fields, CR/LF noise, no trailing +// newline, duplicate filenames, etc.). +// - It only ever returns nil when the parsed expected hash equals the +// SHA256 of the archive on disk. Any malformed/short/garbage line +// for our archive name must be rejected (non-nil error) — never a +// silent pass. +// +// The fuzzer cannot produce a real preimage for a fixed archive, so the +// only legitimate success path is the deterministic correct-line seed. +func FuzzVerifyChecksum(f *testing.F) { + dir := f.TempDir() + archiveName := fmt.Sprintf("pilot-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH) + archivePath := filepath.Join(dir, archiveName) + if err := os.WriteFile(archivePath, []byte("fuzz-archive-bytes"), 0644); err != nil { + f.Fatal(err) + } + sum := sha256.Sum256([]byte("fuzz-archive-bytes")) + correctHash := fmt.Sprintf("%x", sum) + correctLine := fmt.Sprintf("%s %s\n", correctHash, archiveName) + + // Seed corpus: the valid line plus a spread of adversarial shapes. + f.Add(correctLine) + f.Add("") + f.Add("\n\n\n") + f.Add("deadbeef") // single field, no filename + f.Add(correctHash) // hash only, no filename + f.Add(correctHash + " " + archiveName) // single-space variant + f.Add(correctHash + "\t" + archiveName) // tab separator + f.Add(" " + correctHash + " " + archiveName + " ") // surrounding whitespace + f.Add(strings.Repeat("a", 64) + " " + archiveName) // wrong hash, right name + f.Add(correctHash + " other-file.tar.gz") // right hash, wrong name + f.Add("# comment line\n" + correctLine) // comment then valid + f.Add(correctLine + "garbage trailing line") // valid then garbage + f.Add(correctHash + " " + archiveName + "\x00evil") // embedded NUL + f.Add(strings.Repeat(correctLine, 1000)) // many duplicate lines + f.Add(strings.Repeat("x ", 100000)) // very long single line + + f.Fuzz(func(t *testing.T, checksums string) { + checksumsPath := filepath.Join(t.TempDir(), "checksums.txt") + if err := os.WriteFile(checksumsPath, []byte(checksums), 0644); err != nil { + t.Skip() // path/fs hiccup, not a parser property + } + + // Must never panic. + err := VerifyChecksum(archivePath, archiveName, checksumsPath) + + // If it returned nil, the file MUST have contained a line that + // parsed to exactly the correct hash for our archive name. + // Verify that independently — a nil return on any other input is + // a verification bypass. + if err == nil { + if !checksumsLineMatches(checksums, archiveName, correctHash) { + t.Fatalf("VerifyChecksum accepted input with no valid line for %q:\n%q", archiveName, checksums) + } + } + }) +} + +// checksumsLineMatches is an independent re-implementation of the +// accept condition, used only by the fuzzer to cross-check that a nil +// return corresponds to a genuinely correct line. Kept deliberately +// simple and separate from the production parser. +func checksumsLineMatches(checksums, archiveName, correctHash string) bool { + for _, line := range strings.Split(checksums, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + parts := strings.Fields(line) + if len(parts) >= 2 && parts[1] == archiveName { + return parts[0] == correctHash + } + } + return false +} + +// TestApplyUpdate_TamperedArchiveRefused is eval (e): a well-formed +// release whose checksums.txt is internally consistent with the +// ORIGINAL archive, but where the archive bytes served to the client +// have been swapped (MITM / compromised CDN object). The SHA256 of the +// delivered bytes no longer matches the attested checksum, so the +// update must be REFUSED and nothing installed — distinct from +// TestApplyUpdate_ChecksumMismatch in that the checksums file here is a +// real, valid checksums file for a different (legitimate) artifact. +func TestApplyUpdate_TamperedArchiveRefused(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + installDir := filepath.Join(tmpDir, "bin") + os.MkdirAll(installDir, 0755) + os.WriteFile(filepath.Join(installDir, ".pilot-version"), []byte("v1.0.0\n"), 0644) + // Pre-seed a daemon binary so we can prove it is left untouched. + sentinel := []byte("ORIGINAL-DAEMON-DO-NOT-OVERWRITE") + os.WriteFile(filepath.Join(installDir, "pilot-daemon"), sentinel, 0755) + + archiveName := fmt.Sprintf("pilot-%s-%s.tar.gz", runtime.GOOS, runtime.GOARCH) + + // Build the LEGITIMATE archive and compute its real checksum — this + // is what the publisher signed/attested. + legitDir := t.TempDir() + legitPath := filepath.Join(legitDir, archiveName) + createTestTarGz(t, legitPath, map[string]string{"daemon": "legit-daemon-content"}) + legitBytes, _ := os.ReadFile(legitPath) + legitHash := sha256.Sum256(legitBytes) + checksumsContent := fmt.Sprintf("%x %s\n", legitHash, archiveName) + + // Build a DIFFERENT (tampered) archive the attacker actually serves. + // Same filename, malicious payload, different bytes => different hash. + tamperedDir := t.TempDir() + tamperedPath := filepath.Join(tamperedDir, archiveName) + createTestTarGz(t, tamperedPath, map[string]string{"daemon": "MALICIOUS-PAYLOAD"}) + tamperedBytes, _ := os.ReadFile(tamperedPath) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/download/" + archiveName: + w.Write(tamperedBytes) // serve tampered bytes + case "/download/checksums.txt": + w.Write([]byte(checksumsContent)) // valid checksums for the LEGIT archive + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + u := &Updater{ + config: Config{InstallDir: installDir, SkipAttestation: true}, + client: srv.Client(), + stopCh: make(chan struct{}), + exitFn: func(int) {}, + } + + release := &GitHubRelease{ + TagName: "v1.1.0", + Assets: []GitHubAsset{ + {Name: archiveName, BrowserDownloadURL: srv.URL + "/download/" + archiveName}, + {Name: "checksums.txt", BrowserDownloadURL: srv.URL + "/download/checksums.txt"}, + }, + } + + err := u.applyUpdate(release) + if err == nil { + t.Fatal("tampered archive (bytes swapped, checksums valid for original) must be refused") + } + if !strings.Contains(err.Error(), "checksum") { + t.Fatalf("expected checksum verification failure, got: %v", err) + } + // The on-disk daemon must be untouched. + got, _ := os.ReadFile(filepath.Join(installDir, "pilot-daemon")) + if string(got) != string(sentinel) { + t.Fatalf("daemon binary was overwritten despite tamper detection; got %q", got) + } +} diff --git a/zz_more_test.go b/zz_more_test.go index f68b544..bd5a4d3 100644 --- a/zz_more_test.go +++ b/zz_more_test.go @@ -202,12 +202,12 @@ func TestSemver_String(t *testing.T) { func TestParseSemver_Errors(t *testing.T) { t.Parallel() bad := []string{ - "1.2", // not three parts - "x.y.z", // non-numeric major - "1.y.3", // non-numeric minor - "1.2.q", // non-numeric patch - "", // empty - "1.2.3.4", // too many parts + "1.2", // not three parts + "x.y.z", // non-numeric major + "1.y.3", // non-numeric minor + "1.2.q", // non-numeric patch + "", // empty + "1.2.3.4", // too many parts } for _, in := range bad { if _, err := ParseSemver(in); err == nil { From 831db0dfc670033ca98d489b875d2a7819d83be8 Mon Sep 17 00:00:00 2001 From: TeoSlayer Date: Mon, 22 Jun 2026 17:16:37 +0300 Subject: [PATCH 2/2] Fix security CI gates and Zip Slip extraction alert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit security.yml: golang/govulncheck-action does its own git fetch and fails with 'unable to access ... 400' on org repos — run the tool via 'go run golang.org/x/vuln/cmd/govulncheck@latest ./...' after checkout + setup-go instead. Replace gitleaks/gitleaks-action (needs a paid GITLEAKS_LICENSE secret for org repos) with the MIT-licensed pinned gitleaks binary. Both match the green rendezvous/common pattern. updater.go: CodeQL go/zipslip flagged extractTarGz as a high-severity Zip Slip (CWE-022). filepath.Base already strips traversal, but add an explicit destDir-containment check so the invariant is auditable and CodeQL's dataflow recognises the sanitizer; reject escaping entries. --- .github/workflows/security.yml | 28 ++++++++++++++++++++++------ updater.go | 11 ++++++++++- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index ccbd990..73107f9 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -54,10 +54,10 @@ jobs: cache: true cache-dependency-path: go.sum - name: govulncheck - uses: golang/govulncheck-action@v1.0.4 - with: - go-version-input: '1.25' - go-package: ./... + # golang/govulncheck-action does its own git fetch and fails with + # "fatal: unable to access ... error: 400" on org repos. Run the tool + # directly after checkout + setup-go (rendezvous/common pattern). + run: go run golang.org/x/vuln/cmd/govulncheck@latest ./... # Secret scan over the full history on push, the PR range on PR. gitleaks: @@ -66,9 +66,25 @@ jobs: - uses: actions/checkout@v7 with: fetch-depth: 0 - - uses: gitleaks/gitleaks-action@v3.0.0 + + # The gitleaks GitHub Action requires a paid GITLEAKS_LICENSE secret for + # organization repos and fails with "missing gitleaks license". The + # gitleaks binary is MIT-licensed and free, so run a version-pinned + # binary release directly — same scan, no license gate. + - name: Install gitleaks env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITLEAKS_VERSION: "8.30.1" + run: | + set -euo pipefail + curl -sSL -o /tmp/gitleaks.tar.gz \ + "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_VERSION}/gitleaks_${GITLEAKS_VERSION}_linux_x64.tar.gz" + tar -xzf /tmp/gitleaks.tar.gz -C /tmp gitleaks + sudo install /tmp/gitleaks /usr/local/bin/gitleaks + gitleaks version + + - name: Run gitleaks (full history) + run: | + gitleaks git --no-banner --redact --verbose . # Block PRs that introduce dependencies with known vulnerabilities or # incompatible licenses. PR-only (needs the base..head diff). diff --git a/updater.go b/updater.go index 1e1afef..9abe69c 100644 --- a/updater.go +++ b/updater.go @@ -585,13 +585,22 @@ func extractTarGz(archivePath, destDir string) error { continue } - // Sanitize path — prevent directory traversal. + // Sanitize path — prevent directory traversal. filepath.Base strips + // any leading path / ".." elements from the archive entry name. name := filepath.Base(hdr.Name) if name == "." || name == ".." { continue } dst := filepath.Join(destDir, name) + // Defence in depth: reject any entry whose cleaned destination would + // escape destDir (Zip Slip / CWE-022). filepath.Base already prevents + // this, but the explicit containment check makes the invariant + // auditable and is the sanitizer CodeQL's go/zipslip dataflow expects. + cleanDest := filepath.Clean(destDir) + string(os.PathSeparator) + if !strings.HasPrefix(filepath.Clean(dst)+string(os.PathSeparator), cleanDest) { + return fmt.Errorf("archive entry %q escapes destination dir", hdr.Name) + } out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755) //nolint:gosec // G302: extracted files are executables and must be 0755 if err != nil { return fmt.Errorf("create %s: %w", name, err)