internal/vulncheck: remove stdlib confidence in call stacks

Call stacks going through standard library were designated as more
likely to be true positive, i.e., we have more confidence in them. This
does not seem to have a firm footing.

If the call stack consists of static calls, then the vulnerable symbol
is a std symbol, in which case comparison to other call stacks boils
down to length comparison (which we do) since standard library packages
import only standard library packages.

Otherwise if the call stacks have dynamic call sites, there is no reason
to believe that a callee of a dynamic site is more likely to be
correctly inferred compared to a dynamic site outside of the standard
library.

Updates golang/go#60708

Change-Id: Ib8cdd778ccf8902f5e42473fe145e452d28f3960
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/504715
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/vulncheck/witness.go

@@ -174,31 +174,13 @@ func isStdPackage(pkg string) bool {
 	return !strings.Contains(pkg, ".")
 }
 
-// confidence computes an approximate measure of whether the stack
-// is realizeable in practice. Currently, it equals the number of call
-// sites in stack that go through standard libraries. Such call stacks
-// have been experimentally shown to often result in false positives.
-func confidence(stack CallStack) int {
-	c := 0
-	for _, e := range stack {
-		if e.Function.Package != nil && isStdPackage(e.Function.Package.PkgPath) {
-			c += 1
-		}
-	}
-	return c
-}
-
 // stackLess compares two call stacks in terms of their estimated
-// value to the user. Shorter stacks generally come earlier in the ordering.
+// value to the user. Shorter stacks generally come earlier in the
+// ordering.
 //
-// Two stacks are lexicographically ordered by:
-// 1) their estimated level of confidence in being a real call stack,
-// 2) their length, and 3) the number of dynamic call sites in the stack.
+// Two stacks are lexicographically ordered by their length and the
+// number of dynamic call sites in the stack.
 func stackLess(s1, s2 CallStack) bool {
-	if c1, c2 := confidence(s1), confidence(s2); c1 != c2 {
-		return c1 < c2
-	}
-
 	if len(s1) != len(s2) {
 		return len(s1) < len(s2)
 	}

internal/vulncheck/witness_test.go

@@ -8,8 +8,6 @@ import (
 	"reflect"
 	"strings"
 	"testing"
-
-	"golang.org/x/tools/go/packages"
 )
 
 // stacksToString converts map *Vuln:stacks to Vuln.Symbol:["f1->...->fN", ...]
@@ -34,14 +32,14 @@ func TestCallStacks(t *testing.T) {
 	// Call graph structure for the test program
 	//    entry1      entry2
 	//      |           |
-	//    interm1(std)  |
+	//    interm1       |
 	//      |    \     /
 	//      |   interm2(interface)
 	//      |   /     |
 	//     vuln1    vuln2
 	e1 := &FuncNode{Name: "entry1"}
 	e2 := &FuncNode{Name: "entry2"}
-	i1 := &FuncNode{Name: "interm1", Package: &packages.Package{PkgPath: "net/http"}, CallSites: []*CallSite{{Parent: e1, Resolved: true}}}
+	i1 := &FuncNode{Name: "interm1", CallSites: []*CallSite{{Parent: e1, Resolved: true}}}
 	i2 := &FuncNode{Name: "interm2", CallSites: []*CallSite{{Parent: e2, Resolved: true}, {Parent: i1, Resolved: true}}}
 	v1 := &FuncNode{Name: "vuln1", CallSites: []*CallSite{{Parent: i1, Resolved: true}, {Parent: i2, Resolved: false}}}
 	v2 := &FuncNode{Name: "vuln2", CallSites: []*CallSite{{Parent: i2, Resolved: false}}}
@@ -53,7 +51,7 @@ func TestCallStacks(t *testing.T) {
 	}
 
 	want := map[string][]string{
-		"vuln1": {"entry2->interm2->vuln1", "entry1->interm1->vuln1"},
+		"vuln1": {"entry1->interm1->vuln1", "entry2->interm2->vuln1"},
 		"vuln2": {"entry2->interm2->vuln2", "entry1->interm1->interm2->vuln2"},
 	}