From d00c170d001daeff7d121991f4d994c1ad259b40 Mon Sep 17 00:00:00 2001 From: Zvonimir Pavlinovic Date: Tue, 19 Dec 2023 17:30:48 +0000 Subject: [PATCH] internal/sarif: add code flows A code flow is a compact representation of a trace used for the textual output of govulncheck. For that purpose, the logic for trace compaction is extracted into a separate internal package traces. We also add the message portion of Location object for code flows to reduce the number of CLs; the actual physical region part will come in following CLs. To make things consistent, we also add the Message part of the location for stacks. Updates golang/go#61347 Change-Id: I99065a7aab7aa794e7a08687cb4055bc21a610f8 Reviewed-on: https://go-review.googlesource.com/c/vuln/+/551375 TryBot-Result: Gopher Robot Run-TryBot: Zvonimir Pavlinovic Reviewed-by: Maceo Thompson LUCI-TryBot-Result: Go LUCI --- .../testfiles/binary-call/binary_sarif.ct | 114 +++++++++++- .../source-call/source_call_sarif.ct | 175 +++++++++++++++++- internal/sarif/handler.go | 66 ++++++- internal/sarif/sarif.go | 1 + internal/sarif/utils.go | 17 ++ internal/scan/template.go | 37 ++-- internal/scan/template_test.go | 63 +++++++ internal/traces/traces.go | 46 +++++ internal/traces/traces_test.go | 86 +++++++++ 9 files changed, 565 insertions(+), 40 deletions(-) create mode 100644 internal/scan/template_test.go create mode 100644 internal/traces/traces.go create mode 100644 internal/traces/traces_test.go diff --git a/cmd/govulncheck/testdata/common/testfiles/binary-call/binary_sarif.ct b/cmd/govulncheck/testdata/common/testfiles/binary-call/binary_sarif.ct index a6e400c..eaed232 100644 --- a/cmd/govulncheck/testdata/common/testfiles/binary-call/binary_sarif.ct +++ b/cmd/govulncheck/testdata/common/testfiles/binary-call/binary_sarif.ct @@ -116,6 +116,31 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "message": { "text": "Your code calls vulnerable functions in 1 package (github.com/tidwall/gjson)." }, + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Result.ForEach" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function github.com/tidwall/gjson.Result.ForEach" + } + } + ], "stacks": [ { "message": { @@ -129,7 +154,9 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Result.ForEach" + } } } ] @@ -142,6 +169,31 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "message": { "text": "Your code calls vulnerable functions in 1 package (golang.org/x/text/language)." }, + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "module": "golang.org/x/text", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "golang.org/x/text/language.Parse" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function golang.org/x/text/language.Parse" + } + } + ], "stacks": [ { "message": { @@ -155,7 +207,9 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "golang.org/x/text/language.Parse" + } } } ] @@ -168,6 +222,54 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "message": { "text": "Your code calls vulnerable functions in 1 package (github.com/tidwall/gjson)." }, + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Get" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function github.com/tidwall/gjson.Get" + } + }, + { + "threadFlows": [ + { + "locations": [ + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Result.Get" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function github.com/tidwall/gjson.Result.Get" + } + } + ], "stacks": [ { "message": { @@ -181,7 +283,9 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Get" + } } } ] @@ -198,7 +302,9 @@ $ govulncheck -format sarif -mode binary ${common_vuln_binary} "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Result.Get" + } } } ] 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 e8c062e..95115c1 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 @@ -117,6 +117,67 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "message": { "text": "Your code calls vulnerable functions in 1 package (github.com/tidwall/gjson)." }, + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "module": "golang.org/vuln", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "golang.org/vuln.main" + } + } + }, + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Result.Get" + } + } + }, + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Get" + } + } + }, + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Result.ForEach" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function github.com/tidwall/gjson.Result.ForEach" + } + } + ], "stacks": [ { "message": { @@ -130,7 +191,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "golang.org/vuln.main" + } } }, { @@ -140,7 +203,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Result.Get" + } } }, { @@ -150,7 +215,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Get" + } } }, { @@ -160,7 +227,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.execModifier" + } } }, { @@ -170,7 +239,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.modPretty" + } } }, { @@ -180,7 +251,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Result.ForEach" + } } } ] @@ -193,6 +266,43 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "message": { "text": "Your code calls vulnerable functions in 1 package (golang.org/x/text/language)." }, + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "module": "golang.org/vuln", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "golang.org/vuln.main" + } + } + }, + { + "module": "golang.org/x/text", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "golang.org/x/text/language.Parse" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function golang.org/x/text/language.Parse" + } + } + ], "stacks": [ { "message": { @@ -206,7 +316,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "golang.org/vuln.main" + } } }, { @@ -216,7 +328,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "golang.org/x/text/language.Parse" + } } } ] @@ -229,6 +343,43 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "message": { "text": "Your code calls vulnerable functions in 1 package (github.com/tidwall/gjson)." }, + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "module": "golang.org/vuln", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "golang.org/vuln.main" + } + } + }, + { + "module": "github.com/tidwall/gjson", + "location": { + "physicalLocation": { + "artifactLocation": {}, + "region": {} + }, + "message": { + "text": "github.com/tidwall/gjson.Result.Get" + } + } + } + ] + } + ], + "message": { + "text": "A summarized code flow for vulnerable function github.com/tidwall/gjson.Result.Get" + } + } + ], "stacks": [ { "message": { @@ -242,7 +393,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "golang.org/vuln.main" + } } }, { @@ -252,7 +405,9 @@ $ govulncheck -C ${moddir}/vuln -format sarif ./... "artifactLocation": {}, "region": {} }, - "message": {} + "message": { + "text": "github.com/tidwall/gjson.Result.Get" + } } } ] diff --git a/internal/sarif/handler.go b/internal/sarif/handler.go index d654689..ea9a2dd 100644 --- a/internal/sarif/handler.go +++ b/internal/sarif/handler.go @@ -12,6 +12,7 @@ import ( "golang.org/x/vuln/internal/govulncheck" "golang.org/x/vuln/internal/osv" + "golang.org/x/vuln/internal/traces" ) // handler for sarif output. @@ -163,8 +164,9 @@ func results(h *handler) []Result { RuleID: fs[0].OSV, Level: level(fs[0], h.cfg), Message: Description{Text: resultMessage(fs, h.cfg)}, - // TODO: add location and code flows - Stacks: stacks(fs), + // TODO: add location + Stacks: stacks(fs), + CodeFlows: codeFlows(fs), } results = append(results, res) } @@ -263,7 +265,10 @@ func stack(f *govulncheck.Finding) Stack { frame := trace[i] frames = append(frames, Frame{ Module: frame.Module, - // TODO: add location + Location: Location{ + Message: Description{Text: symbol(frame)}, // show the (full) symbol name + // TODO: add physical location + }, }) } @@ -275,6 +280,59 @@ func stack(f *govulncheck.Finding) Stack { vulnSym = vuln.Package + "." + vulnSym return Stack{ Frames: frames, - Message: Description{Text: fmt.Sprintf("A call stack for vulnerable function %s", vulnSym)}, + Message: Description{Text: fmt.Sprintf("A call stack for vulnerable function %s", symbol(trace[0]))}, + } +} + +func codeFlows(fs []*govulncheck.Finding) []CodeFlow { + if fs[0].Trace[0].Function == "" { // not call level findings + return nil + } + + // group call stacks per symbol. There should + // be one call stack currently per symbol, but + // this might change in the future. + m := make(map[govulncheck.Frame][]*govulncheck.Finding) + for _, f := range fs { + // fr.Position is currently the position + // of the definition of the vuln symbol + fr := *f.Trace[0] + m[fr] = append(m[fr], f) + } + + var codeFlows []CodeFlow + for fr, fs := range m { + tfs := threadFlows(fs) + codeFlows = append(codeFlows, CodeFlow{ + ThreadFlows: tfs, + // TODO: should we instead show the message from govulncheck text output? + Message: Description{Text: fmt.Sprintf("A summarized code flow for vulnerable function %s", symbol(&fr))}, + }) + } + // Sort flows for deterministic output. We sort by message + // which is effectively sorting by full symbol name. The + // performance should not be an issue here. + sort.SliceStable(codeFlows, func(i, j int) bool { return codeFlows[i].Message.Text < codeFlows[j].Message.Text }) + return codeFlows +} + +func threadFlows(fs []*govulncheck.Finding) []ThreadFlow { + var tfs []ThreadFlow + for _, f := range fs { + trace := traces.Compact(f) + 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 + // mention three elements of the compact trace? + frame := trace[i] + tf = append(tf, ThreadFlowLocation{ + Module: frame.Module, + Location: Location{ + Message: Description{Text: symbol(frame)}, // show the (full) symbol name + // TODO: add physical location + }}) + } + tfs = append(tfs, ThreadFlow{Locations: tf}) } + return tfs } diff --git a/internal/sarif/sarif.go b/internal/sarif/sarif.go index fd3d4e9..fd00e2f 100644 --- a/internal/sarif/sarif.go +++ b/internal/sarif/sarif.go @@ -110,6 +110,7 @@ type Result struct { type CodeFlow struct { // ThreadFlows is effectively a set of related information flows. ThreadFlows []ThreadFlow `json:"threadFlows,omitempty"` + Message Description `json:"message,omitempty"` } // ThreadFlow encodes an information flow as a sequence of locations. diff --git a/internal/sarif/utils.go b/internal/sarif/utils.go index cef2616..045cac7 100644 --- a/internal/sarif/utils.go +++ b/internal/sarif/utils.go @@ -6,6 +6,8 @@ package sarif import ( "strings" + + "golang.org/x/vuln/internal/govulncheck" ) func choose(s1, s2 string, cond bool) string { @@ -27,3 +29,18 @@ func list(elems []string) string { cList := strings.Join(elems[:l-1], ", ") return cList + choose("", ",", l == 2) + " and " + elems[l-1] } + +// symbol is simplified adaptation of internal/scan/symbol. +func symbol(fr *govulncheck.Frame) string { + if fr.Function == "" { + return "" + } + sym := strings.Split(fr.Function, "$")[0] + if fr.Receiver != "" { + sym = fr.Receiver + "." + sym + } + if fr.Package != "" { + sym = fr.Package + "." + sym + } + return sym +} diff --git a/internal/scan/template.go b/internal/scan/template.go index 419b2ed..083c2b9 100644 --- a/internal/scan/template.go +++ b/internal/scan/template.go @@ -16,6 +16,7 @@ import ( "golang.org/x/vuln/internal/govulncheck" "golang.org/x/vuln/internal/osv" + "golang.org/x/vuln/internal/traces" ) type findingSummary struct { @@ -193,44 +194,36 @@ func symbol(frame *govulncheck.Frame, short bool) string { // If the vulnerable symbol is in the users code, it will show the entry point // and the vulnerable symbol. func compactTrace(finding *govulncheck.Finding) string { - if len(finding.Trace) < 1 { + compact := traces.Compact(finding) + if len(compact) == 0 { return "" } - iTop := len(finding.Trace) - 1 - topModule := finding.Trace[iTop].Module - // search for the exit point of the top module - for i, frame := range finding.Trace { - if frame.Module == topModule { - iTop = i - break - } - } - - if iTop == 0 { - // all in one module, reset to the end - iTop = len(finding.Trace) - 1 - } + l := len(compact) + iTop := l - 1 buf := &strings.Builder{} - topPos := posToString(finding.Trace[iTop].Position) + topPos := posToString(compact[iTop].Position) if topPos != "" { buf.WriteString(topPos) buf.WriteString(": ") } - if iTop > 0 { - addSymbolName(buf, finding.Trace[iTop], true) + if l > 1 { + // print the root of the compact trace + addSymbolName(buf, compact[iTop], true) buf.WriteString(" calls ") } - if iTop > 1 { - addSymbolName(buf, finding.Trace[iTop-1], true) + if l > 2 { + // print next element of the trace, if any + addSymbolName(buf, compact[iTop-1], true) buf.WriteString(", which") - if iTop > 2 { + if l > 3 { + // don't print the third element, just acknowledge it buf.WriteString(" eventually") } buf.WriteString(" calls ") } - addSymbolName(buf, finding.Trace[0], true) + addSymbolName(buf, compact[0], true) // print the vulnerable symbol return buf.String() } diff --git a/internal/scan/template_test.go b/internal/scan/template_test.go new file mode 100644 index 0000000..9faa36e --- /dev/null +++ b/internal/scan/template_test.go @@ -0,0 +1,63 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package scan + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "golang.org/x/vuln/internal/govulncheck" +) + +func TestCompactTrace(t *testing.T) { + for _, tc := range []struct { + trace []*govulncheck.Frame + want string + }{ + { + // binary mode + []*govulncheck.Frame{{Function: "Foo"}}, + "Foo", + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "user", Function: "W"}, + {Module: "user", Function: "U"}, + }, + "W calls V", + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "interm", Function: "I"}, + {Module: "user", Function: "U"}, + {Module: "user", Function: "W"}, + }, + "U calls I, which calls V", + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "x", Function: "X"}, + {Module: "interm", Function: "K"}, + {Module: "interm", Function: "J"}, + {Module: "interm", Function: "I"}, + {Module: "user", Function: "U"}, + {Module: "user", Function: "W"}, + }, + "U calls I, which eventually calls V", + }, + } { + tc := tc + t.Run(tc.want, func(t *testing.T) { + f := &govulncheck.Finding{Trace: tc.trace} + got := compactTrace(f) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("(-want got+) %s", diff) + } + }) + } +} diff --git a/internal/traces/traces.go b/internal/traces/traces.go new file mode 100644 index 0000000..e413ae7 --- /dev/null +++ b/internal/traces/traces.go @@ -0,0 +1,46 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package traces + +import ( + "golang.org/x/vuln/internal/govulncheck" +) + +// Compact returns a summarization of finding.Trace. The first +// returned element is the vulnerable symbol and the last element +// is the exit point of the user module. There can also be two +// elements in between, if applicable, which are the two elements +// preceding the user module exit point. +func Compact(finding *govulncheck.Finding) []*govulncheck.Frame { + if len(finding.Trace) < 1 { + return nil + } + iTop := len(finding.Trace) - 1 + topModule := finding.Trace[iTop].Module + // search for the exit point of the top module + for i, frame := range finding.Trace { + if frame.Module == topModule { + iTop = i + break + } + } + + if iTop == 0 { + // all in one module, reset to the end + iTop = len(finding.Trace) - 1 + } + + compact := []*govulncheck.Frame{finding.Trace[0]} + if iTop > 1 { + if iTop > 2 { + compact = append(compact, finding.Trace[iTop-2]) + } + compact = append(compact, finding.Trace[iTop-1]) + } + if iTop > 0 { + compact = append(compact, finding.Trace[iTop]) + } + return compact +} diff --git a/internal/traces/traces_test.go b/internal/traces/traces_test.go new file mode 100644 index 0000000..876429a --- /dev/null +++ b/internal/traces/traces_test.go @@ -0,0 +1,86 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package traces + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "golang.org/x/vuln/internal/govulncheck" +) + +func TestCompact(t *testing.T) { + for _, tc := range []struct { + trace []*govulncheck.Frame + want []*govulncheck.Frame + }{ + { + []*govulncheck.Frame{}, + nil, + }, + { + // binary mode + []*govulncheck.Frame{{Function: "Foo"}}, + []*govulncheck.Frame{{Function: "Foo"}}, + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "user", Function: "U"}, + }, + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "user", Function: "U"}, + }, + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "user", Function: "W"}, + {Module: "user", Function: "U"}, + }, + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "user", Function: "W"}, + }, + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "interm", Function: "I"}, + {Module: "user", Function: "U"}, + {Module: "user", Function: "W"}, + }, + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "interm", Function: "I"}, + {Module: "user", Function: "U"}, + }, + }, + { + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "x", Function: "X"}, + {Module: "interm", Function: "K"}, + {Module: "interm", Function: "J"}, + {Module: "interm", Function: "I"}, + {Module: "user", Function: "U"}, + {Module: "user", Function: "W"}, + }, + []*govulncheck.Frame{ + {Module: "vuln", Function: "V"}, + {Module: "interm", Function: "J"}, + {Module: "interm", Function: "I"}, + {Module: "user", Function: "U"}, + }, + }, + } { + f := &govulncheck.Finding{Trace: tc.trace} + got := Compact(f) + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("(-want; got+): %s", diff) + } + } +}