From 4b737a97851cc7a7f9b87cf5d123960d55e50006 Mon Sep 17 00:00:00 2001 From: Zvonimir Pavlinovic Date: Wed, 24 Jan 2024 22:12:36 +0000 Subject: [PATCH] internal/sarif: compute relative paths for findings And also make sure the paths are not added in binary mode. Updates golang/go#61347 Change-Id: If48fe57215cdecb01b8b687fbe042aae584f1d6d Reviewed-on: https://go-review.googlesource.com/c/vuln/+/558016 Reviewed-by: Maceo Thompson TryBot-Result: Gopher Robot LUCI-TryBot-Result: Go LUCI Run-TryBot: Zvonimir Pavlinovic --- .../source-call/source_call_sarif.ct | 110 ++++++++++++++---- .../source-module/source_module_sarif.ct | 20 +++- .../source-package/source_package_sarif.ct | 20 +++- internal/sarif/handler.go | 92 ++++++++++----- internal/sarif/sarif.go | 15 ++- 5 files changed, 193 insertions(+), 64 deletions(-) diff --git a/cmd/govulncheck/testdata/common/testfiles/source-call/source_call_sarif.ct b/cmd/govulncheck/testdata/common/testfiles/source-call/source_call_sarif.ct index 30598fc..548d608 100644 --- a/cmd/govulncheck/testdata/common/testfiles/source-call/source_call_sarif.ct +++ b/cmd/govulncheck/testdata/common/testfiles/source-call/source_call_sarif.ct @@ -113,7 +113,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -131,7 +134,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -148,7 +154,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/vuln", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "vuln.go", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 14, "startColumn": 20 @@ -163,7 +172,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 297, "startColumn": 12 @@ -178,7 +190,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 1881, "startColumn": 36 @@ -193,7 +208,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 220, "startColumn": 17 @@ -222,7 +240,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/vuln", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "vuln.go", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 14, "startColumn": 20 @@ -237,7 +258,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 297, "startColumn": 12 @@ -252,7 +276,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 1881, "startColumn": 36 @@ -267,7 +294,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 2587, "startColumn": 21 @@ -282,7 +312,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 2631, "startColumn": 21 @@ -297,7 +330,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 220, "startColumn": 17 @@ -321,7 +357,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -338,7 +377,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/vuln", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "vuln.go", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 13, "startColumn": 16 @@ -353,7 +395,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/x/text", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "language/parse.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 228, "startColumn": 6 @@ -382,7 +427,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/vuln", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "vuln.go", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 13, "startColumn": 16 @@ -397,7 +445,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/x/text", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "language/parse.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 228, "startColumn": 6 @@ -421,7 +472,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -438,7 +492,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/vuln", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "vuln.go", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 14, "startColumn": 20 @@ -453,7 +510,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 296, "startColumn": 17 @@ -482,7 +542,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "golang.org/vuln", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "vuln.go", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 14, "startColumn": 20 @@ -497,7 +560,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "module": "github.com/tidwall/gjson", "location": { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "gjson.go", + "uriBaseId": "%GOMODCACHE%" + }, "region": { "startLine": 296, "startColumn": 17 diff --git a/cmd/govulncheck/testdata/common/testfiles/source-module/source_module_sarif.ct b/cmd/govulncheck/testdata/common/testfiles/source-module/source_module_sarif.ct index e1fe76b..b3b5512 100644 --- a/cmd/govulncheck/testdata/common/testfiles/source-module/source_module_sarif.ct +++ b/cmd/govulncheck/testdata/common/testfiles/source-module/source_module_sarif.ct @@ -113,7 +113,10 @@ $ govulncheck -format sarif -scan module -C ${moddir}/vuln "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -131,7 +134,10 @@ $ govulncheck -format sarif -scan module -C ${moddir}/vuln "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -149,7 +155,10 @@ $ govulncheck -format sarif -scan module -C ${moddir}/vuln "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -167,7 +176,10 @@ $ govulncheck -format sarif -scan module -C ${moddir}/vuln "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } diff --git a/cmd/govulncheck/testdata/common/testfiles/source-package/source_package_sarif.ct b/cmd/govulncheck/testdata/common/testfiles/source-package/source_package_sarif.ct index d82a995..dd17a42 100644 --- a/cmd/govulncheck/testdata/common/testfiles/source-package/source_package_sarif.ct +++ b/cmd/govulncheck/testdata/common/testfiles/source-package/source_package_sarif.ct @@ -113,7 +113,10 @@ $ govulncheck -format sarif -scan package -C ${moddir}/vuln . "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -131,7 +134,10 @@ $ govulncheck -format sarif -scan package -C ${moddir}/vuln . "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -149,7 +155,10 @@ $ govulncheck -format sarif -scan package -C ${moddir}/vuln . "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } @@ -167,7 +176,10 @@ $ govulncheck -format sarif -scan package -C ${moddir}/vuln . "locations": [ { "physicalLocation": { - "artifactLocation": {}, + "artifactLocation": { + "uri": "go.mod", + "uriBaseId": "%SRCROOT%" + }, "region": { "startLine": 1 } diff --git a/internal/sarif/handler.go b/internal/sarif/handler.go index 7bf9326..f9aa2ee 100644 --- a/internal/sarif/handler.go +++ b/internal/sarif/handler.go @@ -10,6 +10,7 @@ import ( "io" "sort" + "golang.org/x/vuln/internal" "golang.org/x/vuln/internal/govulncheck" "golang.org/x/vuln/internal/osv" "golang.org/x/vuln/internal/traces" @@ -165,7 +166,10 @@ func results(h *handler) []Result { // Attach result to the go.mod file for source analysis. // But there is no such place for binaries. locs = []Location{{PhysicalLocation: PhysicalLocation{ - // TODO: add artifact location for go.mod + ArtifactLocation: ArtifactLocation{ + URI: "go.mod", + URIBaseID: SrcRootID, + }, Region: Region{StartLine: 1}, // for now, point to the first line }}} } @@ -174,8 +178,8 @@ func results(h *handler) []Result { RuleID: fs[0].OSV, Level: level(fs[0], h.cfg), Message: Description{Text: resultMessage(fs, h.cfg)}, - Stacks: stacks(fs), - CodeFlows: codeFlows(fs), + Stacks: stacks(h, fs), + CodeFlows: codeFlows(h, fs), Locations: locs, } results = append(results, res) @@ -250,14 +254,14 @@ func level(f *govulncheck.Finding, cfg *govulncheck.Config) string { } } -func stacks(fs []*govulncheck.Finding) []Stack { +func stacks(h *handler, fs []*govulncheck.Finding) []Stack { if fs[0].Trace[0].Function == "" { // not call level findings return nil } var stacks []Stack for _, f := range fs { - stacks = append(stacks, stack(f)) + stacks = append(stacks, stack(h, f)) } // Sort stacks for deterministic output. We sort by message // which is effectively sorting by full symbol name. The @@ -267,8 +271,9 @@ func stacks(fs []*govulncheck.Finding) []Stack { } // stack transforms call stack in f to a sarif stack. -func stack(f *govulncheck.Finding) Stack { +func stack(h *handler, f *govulncheck.Finding) Stack { trace := f.Trace + top := trace[len(trace)-1] // belongs to top level module var frames []Frame for i := len(trace) - 1; i >= 0; i-- { // vulnerable symbol is at the top frame @@ -277,19 +282,24 @@ func stack(f *govulncheck.Finding) Stack { if frame.Position != nil { pos = *frame.Position } - frames = append(frames, Frame{ - Module: frame.Module, - Location: Location{ - Message: Description{Text: symbol(frame)}, // show the (full) symbol name - PhysicalLocation: PhysicalLocation{ - // TODO: add artifact location - Region: Region{ - StartLine: pos.Line, - StartColumn: pos.Column, - }, + + sf := Frame{ + Module: frame.Module, + Location: Location{Message: Description{Text: symbol(frame)}}, // show the (full) symbol name + } + if h.cfg.ScanMode != govulncheck.ScanModeBinary { + sf.Location.PhysicalLocation = PhysicalLocation{ + ArtifactLocation: ArtifactLocation{ + URI: pos.Filename, + URIBaseID: uriID(top.Module, frame.Module), }, - }, - }) + Region: Region{ + StartLine: pos.Line, + StartColumn: pos.Column, + }, + } + } + frames = append(frames, sf) } return Stack{ @@ -298,7 +308,7 @@ func stack(f *govulncheck.Finding) Stack { } } -func codeFlows(fs []*govulncheck.Finding) []CodeFlow { +func codeFlows(h *handler, fs []*govulncheck.Finding) []CodeFlow { if fs[0].Trace[0].Function == "" { // not call level findings return nil } @@ -316,7 +326,7 @@ func codeFlows(fs []*govulncheck.Finding) []CodeFlow { var codeFlows []CodeFlow for fr, fs := range m { - tfs := threadFlows(fs) + tfs := threadFlows(h, fs) codeFlows = append(codeFlows, CodeFlow{ ThreadFlows: tfs, // TODO: should we instead show the message from govulncheck text output? @@ -330,10 +340,12 @@ func codeFlows(fs []*govulncheck.Finding) []CodeFlow { return codeFlows } -func threadFlows(fs []*govulncheck.Finding) []ThreadFlow { +func threadFlows(h *handler, fs []*govulncheck.Finding) []ThreadFlow { var tfs []ThreadFlow for _, f := range fs { trace := traces.Compact(f) + top := trace[len(trace)-1] // belongs to top level module + var tf []ThreadFlowLocation for i := len(trace) - 1; i >= 0; i-- { // vulnerable symbol is at the top frame // TODO: should we, similar to govulncheck text output, only @@ -343,20 +355,36 @@ func threadFlows(fs []*govulncheck.Finding) []ThreadFlow { if frame.Position != nil { pos = *frame.Position } - tf = append(tf, ThreadFlowLocation{ - Module: frame.Module, - Location: Location{ - Message: Description{Text: symbol(frame)}, // show the (full) symbol name - PhysicalLocation: PhysicalLocation{ - // TODO: add artifact location - Region: Region{ - StartLine: pos.Line, - StartColumn: pos.Column, - }, + + tfl := ThreadFlowLocation{ + Module: frame.Module, + Location: Location{Message: Description{Text: symbol(frame)}}, // show the (full) symbol name + } + if h.cfg.ScanMode != govulncheck.ScanModeBinary { + tfl.Location.PhysicalLocation = PhysicalLocation{ + ArtifactLocation: ArtifactLocation{ + URI: pos.Filename, + URIBaseID: uriID(top.Module, frame.Module), + }, + Region: Region{ + StartLine: pos.Line, + StartColumn: pos.Column, }, - }}) + } + } + tf = append(tf, tfl) } tfs = append(tfs, ThreadFlow{Locations: tf}) } return tfs } + +func uriID(top, module string) string { + if top == module { + return SrcRootID + } + if module == internal.GoStdModulePath { + return GoRootID + } + return GoModCacheID +} diff --git a/internal/sarif/sarif.go b/internal/sarif/sarif.go index fd00e2f..53d2ba3 100644 --- a/internal/sarif/sarif.go +++ b/internal/sarif/sarif.go @@ -5,8 +5,13 @@ // Package sarif defines Static Analysis Results Interchange Format // (SARIF) types supported by govulncheck. // -// See https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=sarif -// for more information on the SARIF format. +// The implementation covers the subset of the specification available +// at https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=sarif. +// +// If govulncheck is used in source mode, the locations will include a +// physical location implemented as a path relative to either the source +// module (%SRCROOT%), Go root (%GOROOT%), or Go module cache (%GOMODCACHE%) +// URI base id. package sarif import "golang.org/x/vuln/internal/govulncheck" @@ -151,6 +156,12 @@ type PhysicalLocation struct { Region Region `json:"region,omitempty"` } +const ( + SrcRootID = "%SRCROOT%" + GoRootID = "%GOROOT%" + GoModCacheID = "%GOMODCACHE%" +) + // ArtifactLocation is a path to an offending file. type ArtifactLocation struct { // URI is a path to the artifact. If URIBaseID is empty, then