Repository: google/go-flow-levee
Pull request #127 (2020-09-23 16:00:55)

Allow exclusion of functions from analysis by path
This PR addresses #87 and #88 to some extent by providing a simple mechanism by which users may exclude functions from analysis.

## Two discussion items

1. The exclusion mechanism proposed in this PR is rather opaque: users have no way to know _what_, if anything, was excluded from analysis. It could be a good idea to report this somehow, e.g. `exclusion pattern <pattern> caused the following functions to be excluded from analysis: [...]`.

2. The mechanism also conflates "we want to exclude this entire package _because it is just test code_" and "we want to exclude this specific function _because it contains a false positive_". We may want an explicit distinction here, because in the "false positive suppression" case, we may in fact want to analyze the function anyway, and produce a report if no potentials leaks are found. Placing myself in the shoes of a diligent user, I would want to use the false positive suppression mechanism as little as possible, to avoid cases of "we suppressed a false positive and later someone added vulnerable code but we didn't catch it because of the suppression". In fact, to minimize the risk of something like that happening, we might even want the user to provide _specific_ expectations, e.g. in function `foo`, I am expecting an incorrect report that says that the `Sink` named `leakSecrets` was called on a safe value of type `error` (or a value tainted by `Source` `bar`, etc.). This could be refined using #89. We could even ask for a line number, but that could be annoying to users because unrelated changes could break the expectation. Instead of line numbers, if there are multiple expectations, we could enforce the order in the specification to match the order (by line number) in the function.

- [x] Tests pass
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/configuration/README.md b/configuration/README.md
index 98fa684..68f05f8 100644
--- a/configuration/README.md
+++ b/configuration/README.md
@@ -54,6 +54,23 @@ Taint propagation is performed automatically and does not need to be explicitly
 For matchers that accept a `ReceiverRE` regexp matcher, an unspecified string will match any (or no) receiver.
 To match only methods without any receiver (i.e., a top-level function), use the matcher `^$` to match an empty-string receiver name.
 
+### Restricting analysis scope
+
+Functions can be explicitly excluded from analysis using regexps:
+```json
+{
+  "Exclude": [
+    {
+      "PathRE": "^myproject/mypackage\.myfunction$"
+    }
+  ]
+}
+```
+
+The above will match the function `myfunction` from the `myproject/mypackage` package. It will also match a method named `myfunction` in the same package.
+
+As just two examples, this may be used to avoid analyzing test code, or to suppress "false positive" reports.
+
 ### Example configuration
 
 The following configuration could be used to identify possible instances of credential logging in Kubernetes.
diff --git a/configuration/example-config.json b/configuration/example-config.json
index 26c228e..4dbe357 100644
--- a/configuration/example-config.json
+++ b/configuration/example-config.json
@@ -28,5 +28,10 @@
       "MethodRE": "Info|Warning|Error|Fatal|Exit"
     }
   ],
-  "Sanitizers": []
+  "Sanitizers": [],
+  "Exclude": [
+    {
+      "PathRE": "^k8s.io/kubernetes/test"
+    }
+  ]
 }
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index a0dba33..561be51 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -41,6 +41,23 @@ type Config struct {
 	Sources    []sourceMatcher
 	Sinks      []funcMatcher
 	Sanitizers []funcMatcher
+	Exclude    []pathMatcher
+}
+
+type pathMatcher struct {
+	PathRE regexp.Regexp
+}
+
+// IsExcluded determines if a function's fully qualified name (package path + name)
+// matches one of the exclusion patterns in the Config.
+func (c Config) IsExcluded(fn *ssa.Function) bool {
+	path := fmt.Sprintf("%s.%s", fn.Pkg.Pkg.Path(), fn.Name())
+	for _, pm := range c.Exclude {
+		if pm.PathRE.MatchString(path) {
+			return true
+		}
+	}
+	return false
 }
 
 func (c Config) IsSinkCall(call *ssa.Call) bool {
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index 86b824b..ab8e82e 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -33,6 +33,7 @@ type classifier interface {
 	IsSanitizer(*ssa.Call) bool
 	IsSourceFieldAddr(*ssa.FieldAddr) bool
 	IsSinkFunction(fn *ssa.Function) bool
+	IsExcluded(fn *ssa.Function) bool
 }
 
 // Source represents a Source in an SSA call tree.
@@ -293,8 +294,8 @@ func identify(conf classifier, ssaInput *buildssa.SSA) map[*ssa.Function][]*Sour
 	sourceMap := make(map[*ssa.Function][]*Source)
 
 	for _, fn := range ssaInput.SrcFuncs {
-		// no need to analyze the body of sinks
-		if conf.IsSinkFunction(fn) {
+		// no need to analyze the body of sinks, nor of excluded functions
+		if conf.IsSinkFunction(fn) || conf.IsExcluded(fn) {
 			continue
 		}
 


Test patch:
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
index bc51c5b..a2d1cce 100644
--- a/internal/pkg/config/config_test.go
+++ b/internal/pkg/config/config_test.go
@@ -43,6 +43,9 @@ func runTest(pass *analysis.Pass) (interface{}, error) {
 		if conf.IsSinkFunction(f) {
 			pass.Reportf(f.Pos(), "sink")
 		}
+		if conf.IsExcluded(f) {
+			pass.Reportf(f.Pos(), "excluded")
+		}
 		for _, b := range f.Blocks {
 			for _, i := range b.Instrs {
 				if c, ok := i.(*ssa.Call); ok && conf.IsSinkCall(c) {
@@ -60,8 +63,10 @@ func TestConfig(t *testing.T) {
 	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.json")); err != nil {
 		t.Fatal(err)
 	}
-	for _, p := range []string{"core", "notcore", "crosspkg"} {
+	for _, p := range []string{"core", "crosspkg", "exclusion", "notcore"} {
 		analysistest.Run(t, testdata, testAnalyzer, filepath.Join(testdata, "src/example.com", p))
 	}
-	analysistest.Run(t, testdata, testAnalyzer, filepath.Join(testdata, "src/notexample.com/core"))
+	for _, p := range []string{"core", "exclusion"} {
+		analysistest.Run(t, testdata, testAnalyzer, filepath.Join(testdata, "src/notexample.com", p))
+	}
 }
diff --git a/internal/pkg/config/testdata/src/example.com/exclusion/test.go b/internal/pkg/config/testdata/src/example.com/exclusion/test.go
new file mode 100644
index 0000000..c5bec0d
--- /dev/null
+++ b/internal/pkg/config/testdata/src/example.com/exclusion/test.go
@@ -0,0 +1,19 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package test
+
+func Foo() {} // want "excluded"
+
+func Bar() {}
diff --git a/internal/pkg/config/testdata/src/notexample.com/exclusion/test.go b/internal/pkg/config/testdata/src/notexample.com/exclusion/test.go
new file mode 100644
index 0000000..e025634
--- /dev/null
+++ b/internal/pkg/config/testdata/src/notexample.com/exclusion/test.go
@@ -0,0 +1,19 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package exclusion
+
+func Foo() {} // want "excluded"
+
+func Bar() {} // want "excluded"
diff --git a/internal/pkg/config/testdata/test-config.json b/internal/pkg/config/testdata/test-config.json
index 8161991..418bddc 100644
--- a/internal/pkg/config/testdata/test-config.json
+++ b/internal/pkg/config/testdata/test-config.json
@@ -9,5 +9,13 @@
       "MethodRE": "^Do$",
       "ReceiverRE": "^Sinker$"
     }
+  ],
+    "Exclude": [
+    {
+      "PathRE": "^example.com/exclusion.Foo$"
+    },
+    {
+      "PathRE": "^notexample.com/exclusion"
+    }
   ]
 }
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/excludedpackage/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/excludedpackage/tests.go
new file mode 100644
index 0000000..be9920d
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/excludedpackage/tests.go
@@ -0,0 +1,27 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package excludedpackage
+
+import (
+	"example.com/core"
+)
+
+func Oops(s core.Source) {
+	core.Sink(s) // we do not expect a report here, because this package is excluded from analysis
+}
+
+func OopsIDidItAgain(s core.Source) {
+	core.Sink(s) // we do not expect a report here, because this package is excluded from analysis
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/includedpackage/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/includedpackage/tests.go
new file mode 100644
index 0000000..7991440
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/includedpackage/tests.go
@@ -0,0 +1,27 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package includedpackage
+
+import (
+	"example.com/core"
+)
+
+func Oops(s core.Source) {
+	core.Sink(s) // we do not expect a report here, because this specific function is excluded from analysis
+}
+
+func OopsIDidItAgain(s core.Source) {
+	core.Sink(s) // want "a source has reached a sink"
+}
diff --git a/internal/pkg/levee/testdata/test-config.json b/internal/pkg/levee/testdata/test-config.json
index 0664e03..115a7ca 100644
--- a/internal/pkg/levee/testdata/test-config.json
+++ b/internal/pkg/levee/testdata/test-config.json
@@ -17,5 +17,13 @@
       "PackageRE": "^example.com/core$",
       "MethodRE": "^Sanitize"
     }
+  ],
+  "Exclude": [
+    {
+      "PathRE": "^example.com/tests/excludedpackage"
+    },
+    {
+      "PathRE": "^example.com/tests/includedpackage.Oops$"
+    }
   ]
 }
diff --git a/internal/pkg/source/source_test.go b/internal/pkg/source/source_test.go
index 38e250d..0459e70 100644
--- a/internal/pkg/source/source_test.go
+++ b/internal/pkg/source/source_test.go
@@ -60,6 +60,10 @@ func (c *testConfig) IsSinkFunction(f *ssa.Function) bool {
 	return match
 }
 
+func (c *testConfig) IsExcluded(f *ssa.Function) bool {
+	return false
+}
+
 var testAnalyzer = &analysis.Analyzer{
 	Name:     "source",
 	Run:      runTest,