Repository: google/go-flow-levee
Pull request #75 (2020-08-28 20:31:38)

Use unqualified receiver name when checking receiver type in config.IsSink(Function)
## Context

While running tests on kubernetes, I discovered that sources reaching sink method calls were not being reported.

## The bug(s)

There are at least 2 bugs, the first of which is addressed by the current PR. The second will be addressed in a future PR.
1. We use `recv.Type().String()` as the name of a receiver. That string actually includes the whole package path as well. So if the config says the receiver name is `^Source$`, we won't get a match because we'll compare the regex to e.g. `example.com/core.Source`.
2. We do not reach method calls while we are traversing from a source. This is due to the following snippet in `source.go:visitReferrers()`:
```
if v.Call.Signature().Recv() != nil {
    continue
}
```

## This PR

This PR fixes the 1st bug by checking the unqualified names of receivers (e.g. `Source`) instead of their fully qualified names (e.g. `example.com/core.Source`). 

## Why did we not catch this before?

Simply because these cases do not occur in our tests.

The 1st bug was not covered due to an unfortunate mistake on my part. In a past PR #69 that added cross-package tests for `config.IsSinkFunction`, I used `TypeRE` instead of `ReceiverRE` as a field in the object describing the `Sinker.Do` sink. Because of that, the regexp for that `callMatcher`'s `ReceiverRE` had a `nil` regexp, so it was matching everything. We were also missing a negative case, which could have allowed us to catch this.

In relation to that, I think it would be useful for us, and for our users, to do some JSON validation. We should at least check that the user isn't providing a bogus field.

The 2nd bug was not covered because we don't have any method sinks in the tests for levee. This will be resolved in the PR that fixes the bug.

## Future PR's

In addition to the PR fixing the 2nd bug, I intend to write an additional PR that will refactor the code involving `Function`s and `Call`s. This code contains a lot of duplication right now. I also intend to write tests for other parts of the config that are currently untested, or tested indirectly via levee and other analyzers.

- [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 58a4379..1a290fe 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"go/types"
 	"io/ioutil"
+	"strings"
 	"sync"
 
 	"github.com/google/go-flow-levee/internal/pkg/config/regexp"
@@ -61,7 +62,7 @@ func (c Config) IsSinkFunction(f *ssa.Function) bool {
 
 	var recvName string
 	if recv := f.Signature.Recv(); recv != nil {
-		recvName = recv.Type().String()
+		recvName = unqualifiedName(recv)
 	}
 
 	for _, p := range c.Sinks {
@@ -176,12 +177,21 @@ func (r callMatcher) Match(c *ssa.Call) bool {
 	recv := c.Call.Signature().Recv()
 	var recvName string
 	if recv != nil {
-		recvName = recv.Type().String()
+		recvName = unqualifiedName(recv)
 	}
 
 	return r.ReceiverRE.MatchString(recvName)
 }
 
+func unqualifiedName(v *types.Var) string {
+	packageQualifiedName := v.Type().String()
+	dotPos := strings.LastIndexByte(packageQualifiedName, '.')
+	if dotPos == -1 {
+		return packageQualifiedName
+	}
+	return packageQualifiedName[dotPos+1:]
+}
+
 var readFileOnce sync.Once
 var readConfigCached *Config
 var readConfigCachedErr error
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index d96ea46..f5dff48 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -17,6 +17,7 @@ package source
 
 import (
 	"fmt"
+	"go/token"
 	"go/types"
 	"strings"
 
@@ -251,6 +252,7 @@ func sourcesFromClosure(fn *ssa.Function, conf classifier) []*Source {
 	return sources
 }
 
+// sourcesFromBlocks finds Source values created by instructions within a function's body.
 func sourcesFromBlocks(fn *ssa.Function, conf classifier) []*Source {
 	var sources []*Source
 	for _, b := range fn.Blocks {
@@ -260,26 +262,42 @@ func sourcesFromBlocks(fn *ssa.Function, conf classifier) []*Source {
 		}
 
 		for _, instr := range b.Instrs {
+			// This type switch is used to catch instructions that could produce sources.
+			// All instructions that do not match one of the cases will hit the "default"
+			// and they will not be examined any further.
 			switch v := instr.(type) {
-			// Looking for sources of PII allocated within the body of a function.
-			case *ssa.Alloc:
-				if conf.IsSource(utils.Dereference(v.Type())) && !isProducedBySanitizer(v, conf) {
-					sources = append(sources, New(v, conf))
+			// drop anything that doesn't match one of the following cases
+			default:
+				continue
+
+			// source defined as a local variable or returned from a call
+			case *ssa.Alloc, *ssa.Call:
+				// Allocs and Calls are values
+				if isProducedBySanitizer(v.(ssa.Value), conf) {
+					continue
 				}
 
-				// Handling the case where PII may be in a receiver
-				// (ex. func(b *something) { log.Info(something.PII) }
-			case *ssa.FieldAddr:
-				if conf.IsSource(utils.Dereference(v.Type())) {
-					sources = append(sources, New(v, conf))
+			// source received from chan
+			case *ssa.UnOp:
+				// not a <-chan operation
+				if v.Op != token.ARROW {
+					continue
 				}
+
+			// source obtained through a field or an index operation
+			case *ssa.FieldAddr, *ssa.IndexAddr, *ssa.Lookup:
+			}
+
+			// all of the instructions that the switch lets through are values as per ssa/doc.go
+			if v := instr.(ssa.Value); conf.IsSource(utils.Dereference(v.Type())) {
+				sources = append(sources, New(v.(ssa.Node), conf))
 			}
 		}
 	}
 	return sources
 }
 
-func isProducedBySanitizer(v *ssa.Alloc, conf classifier) bool {
+func isProducedBySanitizer(v ssa.Value, conf classifier) bool {
 	for _, instr := range *v.Referrers() {
 		store, ok := instr.(*ssa.Store)
 		if !ok {


Test patch:
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
index 36d36d9..3da738b 100644
--- a/internal/pkg/config/config_test.go
+++ b/internal/pkg/config/config_test.go
@@ -21,6 +21,7 @@ import (
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/buildssa"
+	"golang.org/x/tools/go/ssa"
 )
 
 var testAnalyzer = &analysis.Analyzer{
@@ -42,6 +43,13 @@ func runTest(pass *analysis.Pass) (interface{}, error) {
 		if conf.IsSinkFunction(f) {
 			pass.Reportf(f.Pos(), "sink")
 		}
+		for _, b := range f.Blocks {
+			for _, i := range b.Instrs {
+				if c, ok := i.(*ssa.Call); ok && conf.IsSink(c) {
+					pass.Reportf(i.Pos(), "sink call")
+				}
+			}
+		}
 	}
 
 	return nil, nil
@@ -52,7 +60,8 @@ 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{"example.com/core", "example.com/notcore", "notexample.com/core"} {
-		analysistest.Run(t, testdata, testAnalyzer, p)
+	for _, p := range []string{"core", "notcore", "crosspkg"} {
+		analysistest.Run(t, testdata, testAnalyzer, filepath.Join(testdata, "src/example.com", p))
 	}
+	analysistest.Run(t, testdata, testAnalyzer, filepath.Join(testdata, "src/notexample.com/core"))
 }
diff --git a/internal/pkg/config/testdata/src/example.com/core/core.go b/internal/pkg/config/testdata/src/example.com/core/core.go
index 38cd223..35877ff 100644
--- a/internal/pkg/config/testdata/src/example.com/core/core.go
+++ b/internal/pkg/config/testdata/src/example.com/core/core.go
@@ -23,3 +23,17 @@ type Sinker struct{}
 func (s Sinker) Do() {} // want "sink"
 
 func (s Sinker) DoNot() {}
+
+type NotSinker struct{}
+
+func (ns NotSinker) Do() {}
+
+func Calls() {
+	Sink() // want "sink call"
+	NotSink()
+	s := Sinker{}
+	s.Do() // want "sink call"
+	s.DoNot()
+	ns := NotSinker{}
+	ns.Do()
+}
diff --git a/internal/pkg/config/testdata/src/example.com/crosspkg/crosspkg.go b/internal/pkg/config/testdata/src/example.com/crosspkg/crosspkg.go
new file mode 100644
index 0000000..f4b0733
--- /dev/null
+++ b/internal/pkg/config/testdata/src/example.com/crosspkg/crosspkg.go
@@ -0,0 +1,43 @@
+// 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 crosspkg
+
+import (
+	"example.com/core"
+	"example.com/notcore"
+	necore "notexample.com/core"
+)
+
+func CoreCalls() {
+	core.Sink() // want "sink call"
+	core.NotSink()
+	s := core.Sinker{}
+	s.Do() // want "sink call"
+	s.DoNot()
+}
+
+func NotCoreCalls() {
+	notcore.Sink()
+	notcore.NotSink()
+	s := notcore.Sinker{}
+	s.Do()
+	s.DoNot()
+}
+
+func NotExampleComCalls() {
+	necore.Sink()
+	s := necore.Sinker{}
+	s.Do()
+}
diff --git a/internal/pkg/config/testdata/src/example.com/notcore/notcore.go b/internal/pkg/config/testdata/src/example.com/notcore/notcore.go
index c469c1e..b32c8f4 100644
--- a/internal/pkg/config/testdata/src/example.com/notcore/notcore.go
+++ b/internal/pkg/config/testdata/src/example.com/notcore/notcore.go
@@ -23,3 +23,11 @@ type Sinker struct{}
 func (s Sinker) Do() {}
 
 func (s Sinker) DoNot() {}
+
+func Calls() {
+	Sink()
+	NotSink()
+	s := Sinker{}
+	s.Do()
+	s.DoNot()
+}
diff --git a/internal/pkg/config/testdata/src/notexample.com/core/core.go b/internal/pkg/config/testdata/src/notexample.com/core/core.go
index d56edbf..d9c8e2f 100644
--- a/internal/pkg/config/testdata/src/notexample.com/core/core.go
+++ b/internal/pkg/config/testdata/src/notexample.com/core/core.go
@@ -16,10 +16,6 @@ package core
 
 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
index de3b362..8161991 100644
--- a/internal/pkg/config/testdata/test-config.json
+++ b/internal/pkg/config/testdata/test-config.json
@@ -7,7 +7,7 @@
     {
       "PackageRE": "^example.com/core$",
       "MethodRE": "^Do$",
-      "TypeRE": "^Sinker$"
+      "ReceiverRE": "^Sinker$"
     }
   ]
 }
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/inlining/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/inlining/tests.go
new file mode 100644
index 0000000..d29179e
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/inlining/tests.go
@@ -0,0 +1,39 @@
+// 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 inlining
+
+import (
+	"example.com/core"
+)
+
+func NewSource() *core.Source {
+	return &core.Source{}
+}
+
+func TestInlinedCall() {
+	core.Sink(NewSource()) // want "a source has reached a sink"
+}
+
+func TestInlinedRecv(sources <-chan core.Source) {
+	core.Sink(<-sources) // want "a source has reached a sink"
+}
+
+func TestInlinedArrayIndex(sources [1]core.Source) {
+	core.Sink(sources[0]) // want "a source has reached a sink"
+}
+
+func TestInlinedMapKey(sources map[string]core.Source) {
+	core.Sink(sources["source"]) // want "a source has reached a sink"
+}