Repository: google/go-flow-levee (go)

Issue:
Handle propagation via source methods
Consider the following (slightly contrived) test case:

```go
func TestPropagationViaSourceMethod(s core.Source) {
	str := s.Propagate(s.Data)
	core.Sink(str) // TODO want "a source has reached a sink"
}
```

Currently, we do *not* produce a report in the above case. This is because of the following snippet in `propagation.go`:

```go
// This is to avoid attaching calls where the source is the receiver, ex:
// core.Sinkf("Source id: %v", wrapper.Source.GetID())
if recv := t.Call.Signature().Recv(); recv != nil && sourcetype.IsSourceType(prop.config, prop.taggedFields, recv.Type()) {
	return
}
```

As far as I know, this check is to avoid incorrectly propagating through safe getters such as `Source.GetID()`. If a getter is not safe, it should be identified by the `fieldpropagators` analyzer, which will cause the `Call` to be identified as a source in `source.go`.

However, as seen in the test case at the beginning of this issue, this does not take into accounts methods on source types that can propagate taint received via one of their arguments. This could cause false negatives, and is inconsistent with how we handle calls in general : if a tainted argument goes in, we assume that whatever comes out is tainted.

Pull request:
Allow traversal through source methods
Fixes #249.

Allowing traversal through source methods is trickier than I expected. Consider this example:

```go
func TestPropagationViaSourceMethod(s core.Source) {
	tainted := s.Propagate(s.Data)
	core.Sink(tainted) // want "a source has reached a sink"
}
```

![image](https://user-images.githubusercontent.com/28677454/103672047-b999bf80-4f49-11eb-8991-54ac21ac369b.png)

Under the current propagation logic, the call to `Propagate` will be reached first via the `Alloc --> UnOp --> Call` chain. Thus when the call is first visited, the propagation does not yet know that taint can reach the call via an argument.

The solution proposed by the current PR is to unmark the node if it is visited from the Source receiver. This allows a future traversal to taint the call via an argument.

- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out. (See [DEVELOPING.md](https://github.com/google/go-flow-levee/blob/master/DEVELOPING.md) for instructions on how to do that.)
- (N/A) [ ] Appropriate changes to README are included in PR


Patch:
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 791c3cf..1f92ffd 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -41,6 +41,7 @@ func init() {
 
 // Config contains matchers and analysis scope information.
 type Config struct {
+	ReportMessage             string
 	Sources                   []sourceMatcher
 	Sinks                     []funcMatcher
 	Sanitizers                []funcMatcher
@@ -285,27 +286,62 @@ func (fm funcMatcher) MatchFunction(path, receiver, name string) bool {
 	return fm.Package.MatchString(path) && fm.Receiver.MatchString(receiver) && fm.Method.MatchString(name)
 }
 
-var readFileOnce sync.Once
-var readConfigCached *Config
-var readConfigCachedErr error
-
-// ReadConfig reads, parses, and validates config file.
+// ReadConfig fetches configuration from the config cache.
+// The cache reads, parses, and validates config file if necessary.
 func ReadConfig() (*Config, error) {
-	readFileOnce.Do(func() {
+	return cache.read(configFile)
+}
+
+// configCacheElement reduces disk access across multiple ReadConfig calls.
+type configCacheElement struct {
+	once       sync.Once
+	conf       *Config
+	err        error
+	sourceFile string
+}
+
+func (r *configCacheElement) readOnce() (*Config, error) {
+	r.once.Do(func() {
 		c := new(Config)
-		bytes, err := ioutil.ReadFile(configFile)
+		bytes, err := ioutil.ReadFile(r.sourceFile)
 		if err != nil {
-			readConfigCachedErr = fmt.Errorf("error reading analysis config: %v", err)
+			r.err = fmt.Errorf("error reading analysis config: %v", err)
 			return
 		}
 
 		if err := yaml.UnmarshalStrict(bytes, c); err != nil {
-			readConfigCachedErr = err
+			r.err = err
 			return
 		}
-		readConfigCached = c
+		r.conf = c
 	})
-	return readConfigCached, readConfigCachedErr
+
+	return r.conf, r.err
+}
+
+// configCache safely stores a configCacheElement per source file for concurrent access.
+type configCache struct {
+	mu    sync.Mutex
+	cache map[string]*configCacheElement
+}
+
+// Instantiates configCacheElement if file has not yet been loaded
+func (w *configCache) read(file string) (*Config, error) {
+	return w.getCacheForFile(file).readOnce()
+}
+
+func (w *configCache) getCacheForFile(file string) *configCacheElement {
+	w.mu.Lock()
+	defer w.mu.Unlock()
+
+	if _, ok := w.cache[file]; !ok {
+		w.cache[file] = &configCacheElement{sourceFile: file}
+	}
+	return w.cache[file]
+}
+
+var cache = configCache{
+	cache: make(map[string]*configCacheElement),
 }
 
 func validateFieldNames(bytes *[]byte, matcherType string, validFields []string) error {
diff --git a/internal/pkg/levee/levee.go b/internal/pkg/levee/levee.go
index 57fe85c..006cffa 100644
--- a/internal/pkg/levee/levee.go
+++ b/internal/pkg/levee/levee.go
@@ -21,11 +21,10 @@ import (
 	"github.com/google/go-flow-levee/internal/pkg/config"
 	"github.com/google/go-flow-levee/internal/pkg/fieldtags"
 	"github.com/google/go-flow-levee/internal/pkg/propagation"
+	"github.com/google/go-flow-levee/internal/pkg/source"
 	"github.com/google/go-flow-levee/internal/pkg/utils"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/ssa"
-
-	"github.com/google/go-flow-levee/internal/pkg/source"
 )
 
 var Analyzer = &analysis.Analyzer{
@@ -55,13 +54,13 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				switch v := instr.(type) {
 				case *ssa.Call:
 					if callee := v.Call.StaticCallee(); callee != nil && conf.IsSink(utils.DecomposeFunction(callee)) {
-						reportSourcesReachingSink(pass, propagations, instr)
+						reportSourcesReachingSink(conf, pass, propagations, instr)
 					}
 				case *ssa.Panic:
 					if conf.AllowPanicOnTaintedValues {
 						continue
 					}
-					reportSourcesReachingSink(pass, propagations, instr)
+					reportSourcesReachingSink(conf, pass, propagations, instr)
 				}
 			}
 		}
@@ -70,18 +69,21 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return nil, nil
 }
 
-func reportSourcesReachingSink(pass *analysis.Pass, propagations map[*source.Source]propagation.Propagation, sink ssa.Instruction) {
-	for source, prop := range propagations {
+func reportSourcesReachingSink(conf *config.Config, pass *analysis.Pass, propagations map[*source.Source]propagation.Propagation, sink ssa.Instruction) {
+	for src, prop := range propagations {
 		if prop.IsTainted(sink) {
-			report(pass, source, sink.(ssa.Node))
+			report(conf, pass, src, sink.(ssa.Node))
 			break
 		}
 	}
 }
 
-func report(pass *analysis.Pass, source *source.Source, sink ssa.Node) {
+func report(conf *config.Config, pass *analysis.Pass, source *source.Source, sink ssa.Node) {
 	var b strings.Builder
 	b.WriteString("a source has reached a sink")
-	fmt.Fprintf(&b, ", source: %v", pass.Fset.Position(source.Pos()))
+	fmt.Fprintf(&b, "\n source: %v", pass.Fset.Position(source.Pos()))
+	if conf.ReportMessage != "" {
+		fmt.Fprintf(&b, "\n %v", conf.ReportMessage)
+	}
 	pass.Reportf(sink.Pos(), b.String())
 }
diff --git a/internal/pkg/propagation/propagation.go b/internal/pkg/propagation/propagation.go
index 7d4329c..1bff2d0 100644
--- a/internal/pkg/propagation/propagation.go
+++ b/internal/pkg/propagation/propagation.go
@@ -253,10 +253,21 @@ func (prop *Propagation) visitCall(call *ssa.Call, maxInstrReached map[*ssa.Basi
 		prop.sanitizers = append(prop.sanitizers, &sanitizer.Sanitizer{Call: call})
 	}
 
-	// This is to avoid attaching calls where the source is the receiver, ex:
-	// core.Sinkf("Source id: %v", wrapper.Source.GetID())
+	// Do not traverse through a method call if it is being reached via a Source receiver.
+	// Do traverse if the call is being reached through a tainted argument.
+	// Source methods that return tainted values regardless of their arguments should be identified by the fieldpropagator analyzer.
 	if recv := call.Call.Signature().Recv(); recv != nil && sourcetype.IsSourceType(prop.config, prop.taggedFields, recv.Type()) {
-		return
+		visitingFromArg := false
+		for _, a := range call.Call.Args[1:] {
+			if prop.tainted[a.(ssa.Node)] {
+				visitingFromArg = true
+			}
+		}
+		if !visitingFromArg {
+			// unmark the node so that it may be visited again when taint propagates to an argument
+			prop.tainted[call] = false
+			return
+		}
 	}
 
 	prop.taintReferrers(call, maxInstrReached, lastBlockVisited)


Test patch:
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
index cd7dee9..0d25027 100644
--- a/internal/pkg/config/config_test.go
+++ b/internal/pkg/config/config_test.go
@@ -61,6 +61,31 @@ func runTest(pass *analysis.Pass) (interface{}, error) {
 	return nil, nil
 }
 
+func TestConfigCache(t *testing.T) {
+	testdata := analysistest.TestData()
+	if err := FlagSet.Set("config", filepath.Join(testdata, "empty-config.yaml")); err != nil {
+		t.Fatal(err)
+	}
+
+	emptyConfig, err := ReadConfig()
+	if err != nil {
+		t.Errorf("failed to read config: %v", err)
+	}
+
+	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.yaml")); err != nil {
+		t.Fatal(err)
+	}
+
+	testConfig, err := ReadConfig()
+	if err != nil {
+		t.Errorf("failed to read config: %v", err)
+	}
+
+	if testConfig == emptyConfig {
+		t.Error("Returned the same config for separate config source files")
+	}
+}
+
 func TestConfig(t *testing.T) {
 	testdata := analysistest.TestData()
 	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.yaml")); err != nil {
diff --git a/internal/pkg/config/testdata/empty-config.yaml b/internal/pkg/config/testdata/empty-config.yaml
new file mode 100644
index 0000000..2b2fba0
--- /dev/null
+++ b/internal/pkg/config/testdata/empty-config.yaml
@@ -0,0 +1,16 @@
+# 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.
+---
+
+# This config left intentionally empty.
diff --git a/internal/pkg/levee/levee_test.go b/internal/pkg/levee/levee_test.go
index 7828ae5..3825d56 100644
--- a/internal/pkg/levee/levee_test.go
+++ b/internal/pkg/levee/levee_test.go
@@ -32,7 +32,7 @@ func TestLevee(t *testing.T) {
 	if *debugging {
 		Analyzer.Requires = append(Analyzer.Requires, debug.Analyzer)
 	}
-	analysistest.Run(t, dataDir, Analyzer, "./...")
+	analysistest.Run(t, dataDir, Analyzer, "./src/example.com/...")
 }
 
 func TestLeveeDoesNotCreateReportsForPanicIfPanicingOnTaintedValuesIsAllowed(t *testing.T) {
@@ -45,3 +45,19 @@ func TestLeveeDoesNotCreateReportsForPanicIfPanicingOnTaintedValuesIsAllowed(t *
 	}
 	analysistest.Run(t, dataDir, Analyzer, "./src/nopanic.com/...")
 }
+
+func TestFormattingWithCustomReportMessage(t *testing.T) {
+	dataDir := analysistest.TestData()
+	if err := Analyzer.Flags.Set("config", dataDir+"/with-custom-message.yaml"); err != nil {
+		t.Error(err)
+	}
+	analysistest.Run(t, dataDir, Analyzer, "./src/custom.message.com/withcustom")
+}
+
+func TestFormattingWithoutCustomReportMessage(t *testing.T) {
+	dataDir := analysistest.TestData()
+	if err := Analyzer.Flags.Set("config", dataDir+"/no-custom-message.yaml"); err != nil {
+		t.Error(err)
+	}
+	analysistest.Run(t, dataDir, Analyzer, "./src/custom.message.com/nocustom")
+}
diff --git a/internal/pkg/levee/testdata/allowpanicontaintedvalues-config.yaml b/internal/pkg/levee/testdata/allowpanicontaintedvalues-config.yaml
index 7d16e60..d1250c4 100644
--- a/internal/pkg/levee/testdata/allowpanicontaintedvalues-config.yaml
+++ b/internal/pkg/levee/testdata/allowpanicontaintedvalues-config.yaml
@@ -17,3 +17,6 @@ Sources:
 - Package: "nopanic.com/core"
   Type: "Source"
   Field: "Data"
+Sinks:
+- Package: "nopanic.com/core"
+  Method: "Sink"
diff --git a/internal/pkg/levee/testdata/no-custom-message.yaml b/internal/pkg/levee/testdata/no-custom-message.yaml
new file mode 100644
index 0000000..4398017
--- /dev/null
+++ b/internal/pkg/levee/testdata/no-custom-message.yaml
@@ -0,0 +1,19 @@
+# Copyright 2021 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.
+---
+Sources:
+- Type: Source
+
+Sinks:
+- Method: Sink
diff --git a/internal/pkg/levee/testdata/src/custom.message.com/nocustom/nocustom.go b/internal/pkg/levee/testdata/src/custom.message.com/nocustom/nocustom.go
new file mode 100644
index 0000000..2432b01
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/custom.message.com/nocustom/nocustom.go
@@ -0,0 +1,27 @@
+// Copyright 2021 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 nocustom
+
+type Source struct {
+	Data string
+	ID   int
+}
+
+func Sink(interface{}) {}
+
+func TestReportMessage() {
+	s := Source{Data: "password", ID: 1337}
+	Sink(s) // want "^a source has reached a sink\n source: .*custom.go:25:2$"
+}
diff --git a/internal/pkg/levee/testdata/src/custom.message.com/withcustom/withcustom.go b/internal/pkg/levee/testdata/src/custom.message.com/withcustom/withcustom.go
new file mode 100644
index 0000000..bea3968
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/custom.message.com/withcustom/withcustom.go
@@ -0,0 +1,27 @@
+// Copyright 2021 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 nocustom
+
+type Source struct {
+	Data string
+	ID   int
+}
+
+func Sink(interface{}) {}
+
+func TestReportMessage() {
+	s := Source{Data: "password", ID: 1337}
+	Sink(s) // want "^a source has reached a sink\n source: .*custom.go:25:2\n This custom message is included with report.$"
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/core/source.go b/internal/pkg/levee/testdata/src/example.com/core/source.go
index ba19f85..a72ddc7 100644
--- a/internal/pkg/levee/testdata/src/example.com/core/source.go
+++ b/internal/pkg/levee/testdata/src/example.com/core/source.go
@@ -40,6 +40,10 @@ func (s Source) CopyPointer() (*Source, error) {
 	return &s, nil
 }
 
+func (s Source) Propagate(str string) string {
+	return str
+}
+
 type TaggedSource struct {
 	Data string `levee:"source"`
 	ID   int
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/position/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/position/tests.go
index cfe9dc9..3447225 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/position/tests.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/position/tests.go
@@ -20,7 +20,7 @@ import (
 
 func TestSourcePointerExtract() {
 	s, _ := NewSource()
-	core.Sink(s) // want "a source has reached a sink, source: .*tests.go:22:19"
+	core.Sink(s) // want "a source has reached a sink\n source: .*tests.go:22:19"
 }
 
 // In order for the SSA to contain a FieldAddr, the EmbedsSource instance's fields have to be addressable.
@@ -33,13 +33,13 @@ func TestSourcePointerExtract() {
 func TestEmbeddedSourceFieldAddr() {
 	es := EmbedsSource{}
 	d := es.Data
-	core.Sink(d) // want "a source has reached a sink, source: .*tests.go:34:2"
+	core.Sink(d) // want "a source has reached a sink\n source: .*tests.go:34:2"
 }
 
 // In order for the SSA to contain a Field, the EmbedsSource instance's fields must not be addressable.
 // One way to do this is to create a literal and to access the field directly, as part of the same expression.
 func TestEmbeddedSourceField() {
-	core.Sink(EmbedsSource{}.Data) // want "a source has reached a sink, source: .*tests.go:42:24"
+	core.Sink(EmbedsSource{}.Data) // want "a source has reached a sink\n source: .*tests.go:42:24"
 }
 
 type EmbedsSource struct {
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/propagation/functions.go b/internal/pkg/levee/testdata/src/example.com/tests/propagation/functions.go
index 435464c..beef51d 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/propagation/functions.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/propagation/functions.go
@@ -38,3 +38,8 @@ func TestToStringPropagator(s core.Source) {
 	v := ToString(s)
 	core.Sink(v) // want "a source has reached a sink"
 }
+
+func TestPropagationViaSourceMethod(s core.Source) {
+	tainted := s.Propagate(s.Data)
+	core.Sink(tainted) // want "a source has reached a sink"
+}
diff --git a/internal/pkg/levee/testdata/src/nopanic.com/core/core.go b/internal/pkg/levee/testdata/src/nopanic.com/core/core.go
index 74e4c23..c668d09 100644
--- a/internal/pkg/levee/testdata/src/nopanic.com/core/core.go
+++ b/internal/pkg/levee/testdata/src/nopanic.com/core/core.go
@@ -19,6 +19,13 @@ type Source struct {
 	ID   int
 }
 
+func Sink(...interface{}) {}
+
+func TestTaintedValuesAreNotAllowedToReachSinks() {
+	s := Source{Data: "password", ID: 1337}
+	Sink(s) // want "a source has reached a sink"
+}
+
 func TestPanickingOnTaintedValueIsAllowed() {
 	s := Source{Data: "password", ID: 1337}
 	panic(s)
diff --git a/internal/pkg/levee/testdata/with-custom-message.yaml b/internal/pkg/levee/testdata/with-custom-message.yaml
new file mode 100644
index 0000000..c41e3fa
--- /dev/null
+++ b/internal/pkg/levee/testdata/with-custom-message.yaml
@@ -0,0 +1,21 @@
+# Copyright 2021 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.
+---
+ReportMessage: This custom message is included with report.
+
+Sources:
+- Type: Source
+
+Sinks:
+- Method: Sink