From 33791bcb641951845fcda445b39dd58c870b9330 Mon Sep 17 00:00:00 2001 From: Zvonimir Pavlinovic Date: Wed, 27 Dec 2023 18:00:31 +0000 Subject: [PATCH] internal/sarif: add region part of the physical location Updates golang/go#61347 Change-Id: I725012e4b028b879a0d1720fc47632e76e699c04 Reviewed-on: https://go-review.googlesource.com/c/vuln/+/552955 Reviewed-by: Ian Cottrell TryBot-Result: Gopher Robot Run-TryBot: Zvonimir Pavlinovic LUCI-TryBot-Result: Go LUCI --- .../source-call/source_call_sarif.ct | 136 +++++++++++++++--- .../source-module/source_module_sarif.ct | 52 ++++++- .../source-package/source_package_sarif.ct | 52 ++++++- internal/sarif/handler.go | 44 +++++- internal/sarif/handler_test.go | 12 +- 5 files changed, 260 insertions(+), 36 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 95115c1..30598fc 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 @@ -109,7 +109,18 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "level": "note", "message": { "text": "Your code depends on 1 vulnerable module (golang.org/x/text), but doesn't appear to call any of the vulnerable symbols." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0054", @@ -117,6 +128,17 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "message": { "text": "Your code calls vulnerable functions in 1 package (github.com/tidwall/gjson)." }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ], "codeFlows": [ { "threadFlows": [ @@ -127,7 +149,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 14, + "startColumn": 20 + } }, "message": { "text": "golang.org/vuln.main" @@ -139,7 +164,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 297, + "startColumn": 12 + } }, "message": { "text": "github.com/tidwall/gjson.Result.Get" @@ -151,7 +179,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 1881, + "startColumn": 36 + } }, "message": { "text": "github.com/tidwall/gjson.Get" @@ -163,7 +194,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 220, + "startColumn": 17 + } }, "message": { "text": "github.com/tidwall/gjson.Result.ForEach" @@ -189,7 +223,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 14, + "startColumn": 20 + } }, "message": { "text": "golang.org/vuln.main" @@ -201,7 +238,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 297, + "startColumn": 12 + } }, "message": { "text": "github.com/tidwall/gjson.Result.Get" @@ -213,7 +253,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 1881, + "startColumn": 36 + } }, "message": { "text": "github.com/tidwall/gjson.Get" @@ -225,7 +268,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 2587, + "startColumn": 21 + } }, "message": { "text": "github.com/tidwall/gjson.execModifier" @@ -237,7 +283,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 2631, + "startColumn": 21 + } }, "message": { "text": "github.com/tidwall/gjson.modPretty" @@ -249,7 +298,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 220, + "startColumn": 17 + } }, "message": { "text": "github.com/tidwall/gjson.Result.ForEach" @@ -266,6 +318,17 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "message": { "text": "Your code calls vulnerable functions in 1 package (golang.org/x/text/language)." }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ], "codeFlows": [ { "threadFlows": [ @@ -276,7 +339,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 13, + "startColumn": 16 + } }, "message": { "text": "golang.org/vuln.main" @@ -288,7 +354,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 228, + "startColumn": 6 + } }, "message": { "text": "golang.org/x/text/language.Parse" @@ -314,7 +383,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 13, + "startColumn": 16 + } }, "message": { "text": "golang.org/vuln.main" @@ -326,7 +398,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 228, + "startColumn": 6 + } }, "message": { "text": "golang.org/x/text/language.Parse" @@ -343,6 +418,17 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "message": { "text": "Your code calls vulnerable functions in 1 package (github.com/tidwall/gjson)." }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ], "codeFlows": [ { "threadFlows": [ @@ -353,7 +439,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 14, + "startColumn": 20 + } }, "message": { "text": "golang.org/vuln.main" @@ -365,7 +454,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 296, + "startColumn": 17 + } }, "message": { "text": "github.com/tidwall/gjson.Result.Get" @@ -391,7 +483,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 14, + "startColumn": 20 + } }, "message": { "text": "golang.org/vuln.main" @@ -403,7 +498,10 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "location": { "physicalLocation": { "artifactLocation": {}, - "region": {} + "region": { + "startLine": 296, + "startColumn": 17 + } }, "message": { "text": "github.com/tidwall/gjson.Result.Get" 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 db80ec4..e1fe76b 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 @@ -109,28 +109,72 @@ $ govulncheck -format sarif -scan module -C ${moddir}/vuln "level": "error", "message": { "text": "Your code depends on 1 vulnerable module (golang.org/x/text). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0054", "level": "error", "message": { "text": "Your code depends on 1 vulnerable module (github.com/tidwall/gjson). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0113", "level": "error", "message": { "text": "Your code depends on 1 vulnerable module (golang.org/x/text). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0265", "level": "error", "message": { "text": "Your code depends on 1 vulnerable module (github.com/tidwall/gjson). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] } ] } 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 636fe46..d82a995 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 @@ -109,28 +109,72 @@ $ govulncheck -format sarif -scan package -C ${moddir}/vuln . "level": "warning", "message": { "text": "Your code depends on 1 vulnerable module (golang.org/x/text), but doesn't appear to import any of the vulnerable symbols." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0054", "level": "error", "message": { "text": "Your code imports 1 vulnerable package (github.com/tidwall/gjson). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0113", "level": "error", "message": { "text": "Your code imports 1 vulnerable package (golang.org/x/text/language). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] }, { "ruleId": "GO-2021-0265", "level": "error", "message": { "text": "Your code imports 1 vulnerable package (github.com/tidwall/gjson). Run the call-level analysis to understand whether your code actually calls the vulnerabilities." - } + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {}, + "region": { + "startLine": 1 + } + }, + "message": {} + } + ] } ] } diff --git a/internal/sarif/handler.go b/internal/sarif/handler.go index ea9a2dd..9043fc1 100644 --- a/internal/sarif/handler.go +++ b/internal/sarif/handler.go @@ -26,6 +26,8 @@ type handler struct { // an osv is indeed called, then all findings for // the osv will have call stack info. findings map[string][]*govulncheck.Finding + + binary bool } func NewHandler(w io.Writer) *handler { @@ -160,13 +162,23 @@ func rules(h *handler) []Rule { func results(h *handler) []Result { var results []Result for _, fs := range h.findings { + var locs []Location + if h.cfg.ScanMode != govulncheck.ScanModeBinary { + // 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 + Region: Region{StartLine: 1}, // for now, point to the first line + }}} + } + res := Result{ - RuleID: fs[0].OSV, - Level: level(fs[0], h.cfg), - Message: Description{Text: resultMessage(fs, h.cfg)}, - // TODO: add location + RuleID: fs[0].OSV, + Level: level(fs[0], h.cfg), + Message: Description{Text: resultMessage(fs, h.cfg)}, Stacks: stacks(fs), CodeFlows: codeFlows(fs), + Locations: locs, } results = append(results, res) } @@ -263,11 +275,21 @@ func stack(f *govulncheck.Finding) Stack { var frames []Frame for i := len(trace) - 1; i >= 0; i-- { // vulnerable symbol is at the top frame frame := trace[i] + pos := govulncheck.Position{} + 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 - // TODO: add physical location + PhysicalLocation: PhysicalLocation{ + // TODO: add artifact location + Region: Region{ + StartLine: pos.Line, + StartColumn: pos.Column, + }, + }, }, }) } @@ -325,11 +347,21 @@ func threadFlows(fs []*govulncheck.Finding) []ThreadFlow { // TODO: should we, similar to govulncheck text output, only // mention three elements of the compact trace? frame := trace[i] + pos := govulncheck.Position{} + 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 - // TODO: add physical location + PhysicalLocation: PhysicalLocation{ + // TODO: add artifact location + Region: Region{ + StartLine: pos.Line, + StartColumn: pos.Column, + }, + }, }}) } tfs = append(tfs, ThreadFlow{Locations: tf}) diff --git a/internal/sarif/handler_test.go b/internal/sarif/handler_test.go index 656bee1..1d160be 100644 --- a/internal/sarif/handler_test.go +++ b/internal/sarif/handler_test.go @@ -23,6 +23,12 @@ func scanLevel(f *govulncheck.Finding) string { return "module" } +func newTestHandler() *handler { + h := NewHandler(nil) + h.cfg = &govulncheck.Config{} + return h +} + func TestHandlerSymbol(t *testing.T) { fs := ` { @@ -96,7 +102,7 @@ func TestHandlerSymbol(t *testing.T) { } }` - h := NewHandler(nil) + h := newTestHandler() if err := govulncheck.HandleJSON(strings.NewReader(fs), h); err != nil { t.Fatal(err) } @@ -170,7 +176,7 @@ func TestHandlerPackage(t *testing.T) { } }` - h := NewHandler(nil) + h := newTestHandler() if err := govulncheck.HandleJSON(strings.NewReader(fs), h); err != nil { t.Fatal(err) } @@ -222,7 +228,7 @@ func TestHandlerModule(t *testing.T) { } }` - h := NewHandler(nil) + h := newTestHandler() if err := govulncheck.HandleJSON(strings.NewReader(fs), h); err != nil { t.Fatal(err) }