Repository: google/go-flow-levee
Pull request #65 (2020-08-26 15:01:25)

Add tests for propagation through channels
See the test cases for details.

## Discussion
One place where channels differ from other collections is that logging a channel typically does not reveal its contents. Because of that, it may not make much sense to produce a report when a channel is sinked. (It would be possible for a user to write a custom logger that empties a channel and writes its content to some form of log, but that would be rather odd).

In fact, in general a user is unlikely to log a channel. They might log a structure that contains a channel, though.

For our analysis, it is useful to consider that a channel can be tainted, because that makes it easy to recognize that a value obtained from such a channel is also tainted.

I think the way it works right now is acceptable. We could add some code to avoid producing a report when the tainted value being sinked is a channel, but I don't think it's worth it. (Plus if our users do write weird custom loggers they will expect reports if they are used incorrectly).

- [x] Tests pass
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 6d3851a..76ac020 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -53,12 +53,25 @@ func (c Config) IsSink(call *ssa.Call) bool {
 }
 
 func (c Config) IsSinkFunction(f *ssa.Function) bool {
+	// according to the documentation for ssa.Function, this can happen if
+	// f is a "shared func", e.g. "wrappers and error.Error"
+	if f.Pkg == nil {
+		return false
+	}
+
+	var recvName string
+	if recv := f.Signature.Recv(); recv != nil {
+		recvName = recv.Type().String()
+	}
+
 	for _, p := range c.Sinks {
-		if p.MethodRE.MatchString(f.Name()) {
+		if !p.PackageRE.MatchString(f.Pkg.Pkg.Name()) || !p.MethodRE.MatchString(f.Name()) {
+			continue
+		}
+		if p.ReceiverRE.MatchString(recvName) {
 			return true
 		}
 	}
-
 	return false
 }
 
diff --git a/internal/pkg/sanitizer/sanitizer.go b/internal/pkg/sanitizer/sanitizer.go
index 50822be..b703d45 100644
--- a/internal/pkg/sanitizer/sanitizer.go
+++ b/internal/pkg/sanitizer/sanitizer.go
@@ -57,11 +57,5 @@ func (s Sanitizer) Dominates(target ssa.Instruction) bool {
 		return sanitizationIdx < targetIdx
 	}
 
-	for _, d := range s.Call.Block().Dominees() {
-		if target.Block() == d {
-			return true
-		}
-	}
-
 	return false
 }


Test patch:
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
new file mode 100644
index 0000000..0c951cf
--- /dev/null
+++ b/internal/pkg/config/config_test.go
@@ -0,0 +1,56 @@
+// 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 config
+
+import (
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/buildssa"
+)
+
+var testAnalyzer = &analysis.Analyzer{
+	Name:     "config",
+	Run:      runTest,
+	Doc:      "test harness for the logic related to config",
+	Requires: []*analysis.Analyzer{buildssa.Analyzer},
+}
+
+func runTest(pass *analysis.Pass) (interface{}, error) {
+	in := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
+
+	conf, err := ReadConfig()
+	if err != nil {
+		return nil, err
+	}
+
+	for _, f := range in.SrcFuncs {
+		if conf.IsSinkFunction(f) {
+			pass.Reportf(f.Pos(), "sink")
+		}
+	}
+
+	return nil, nil
+}
+
+func TestConfig(t *testing.T) {
+	testdata := analysistest.TestData()
+	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.json")); err != nil {
+		t.Fatal(err)
+	}
+	analysistest.Run(t, testdata, testAnalyzer, "core", "notcore")
+}
diff --git a/internal/pkg/config/testdata/src/core/core.go b/internal/pkg/config/testdata/src/core/core.go
new file mode 100644
index 0000000..38cd223
--- /dev/null
+++ b/internal/pkg/config/testdata/src/core/core.go
@@ -0,0 +1,25 @@
+// 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 core
+
+func Sink() {} // want "sink"
+
+func NotSink() {}
+
+type Sinker struct{}
+
+func (s Sinker) Do() {} // want "sink"
+
+func (s Sinker) DoNot() {}
diff --git a/internal/pkg/config/testdata/src/notcore/notcore.go b/internal/pkg/config/testdata/src/notcore/notcore.go
new file mode 100644
index 0000000..c469c1e
--- /dev/null
+++ b/internal/pkg/config/testdata/src/notcore/notcore.go
@@ -0,0 +1,25 @@
+// 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 notcore
+
+func Sink() {}
+
+func NotSink() {}
+
+type Sinker struct{}
+
+func (s Sinker) Do() {}
+
+func (s Sinker) DoNot() {}
diff --git a/internal/pkg/config/testdata/test-config.json b/internal/pkg/config/testdata/test-config.json
new file mode 100644
index 0000000..4d03234
--- /dev/null
+++ b/internal/pkg/config/testdata/test-config.json
@@ -0,0 +1,13 @@
+{
+  "Sinks": [
+    {
+      "PackageRE": "^core$",
+      "MethodRE": "^Sink$"
+    },
+    {
+      "PackageRE": "^core$",
+      "MethodRE": "^Do$",
+      "TypeRE": "^Sinker$"
+    }
+  ]
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/collections/chans.go b/internal/pkg/levee/testdata/src/example.com/tests/collections/chans.go
new file mode 100644
index 0000000..752cafa
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/collections/chans.go
@@ -0,0 +1,41 @@
+// 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 collections
+
+import (
+	"example.com/core"
+)
+
+func TestSourceReceivedFromChannelIsTainted(sources <-chan core.Source) {
+	s := <-sources
+	core.Sink(s) // want "a source has reached a sink"
+}
+
+func TestChannelIsTaintedWhenSourceIsPlacedOnIt(sources chan<- core.Source) {
+	sources <- core.Source{}
+	core.Sink(sources) // want "a source has reached a sink"
+}
+
+func TestValueObtainedFromTaintedChannelIsTainted(c chan interface{}) {
+	c <- core.Source{}
+	s := <-c
+	core.Sink(s) // want "a source has reached a sink"
+}
+
+func TestChannelIsNoLongerTaintedWhenNilledOut(sources chan core.Source) {
+	sources <- core.Source{}
+	sources = nil
+	core.Sink(sources)
+}