Repository: google/go-flow-levee
Pull request #154 (2020-10-16 18:27:43)

Fix sanitization bug, add tests
This PR fixes a bug in how sanitization status is evaluated. It also introduces tests covering the previously buggy behavior.

This PR also adds tests documenting another, more complicated bug also involving sanitization. This bug are described in #155.

- [x] Tests pass
- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out.
- [x] Appropriate changes to README are included in PR

mlevesquedion: Patrick, Vinayak: I've added you as reviewers, but I'm not looking for an actual review, I would just like to get your thoughts on these test cases.
PurelyApplied: I do love tests.  LGTM.
mlevesquedion: Good! I would like to add these tests, but right now they would be failing.

I have created an issue (#155), because I am not sure how to handle this right now, and they don't feel very high priority. I guess the bottom line is that it seems that the current way we check for domination is incorrect. In the cases above, the sanitizing call *does* dominate the sink call on the possible paths from source to sink, which are the relevant paths. I'll look into a possible fix later today.

Patch:
diff --git a/internal/pkg/fieldtags/analyzer.go b/internal/pkg/fieldtags/analyzer.go
index 542d93d..9d50553 100644
--- a/internal/pkg/fieldtags/analyzer.go
+++ b/internal/pkg/fieldtags/analyzer.go
@@ -86,61 +86,6 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return ResultType(taggedFields), nil
 }
 
-func extractTags(field *ast.Field) (tags []fieldTag) {
-	if field.Tag == nil {
-		return
-	}
-
-	wholeTag := field.Tag.Value
-
-	// TODO: consider refactoring this logic into a regex matcher
-	i := 1 // skip initial quote
-	j := 1
-	for j < len(wholeTag) {
-		for j < len(wholeTag) && wholeTag[j] != ':' {
-			j++
-		}
-		key := wholeTag[i:j]
-		if key == "" {
-			return
-		}
-
-		i = j + 1 // skip colon
-		if i >= len(wholeTag) {
-			return
-		}
-		if wholeTag[i] == '\\' {
-			i++
-		}
-		i++ // skip quote
-
-		j = i
-		for j < len(wholeTag) && wholeTag[j] != '"' {
-			// skip escape character
-			if wholeTag[j] == '\\' {
-				j++
-			}
-			j++
-		}
-		value := wholeTag[i:j]
-		// remove trailing escaped quote if present
-		value = strings.TrimSuffix(value, `\"`)
-		if value == "" {
-			return
-		}
-
-		// value may be a list of comma separated values
-		for _, v := range strings.Split(value, ",") {
-			tags = append(tags, fieldTag{key: key, val: v})
-		}
-
-		i = j + 2 // skip closing quote and space
-		j = i
-	}
-
-	return tags
-}
-
 // IsSourceFieldAddr determines whether a ssa.FieldAddr is a source, that is whether it refers to a field previously identified as a source.
 func (r ResultType) IsSourceFieldAddr(fa *ssa.FieldAddr) bool {
 	// incantation plundered from the docstring for ssa.FieldAddr.Field
diff --git a/internal/pkg/sanitizer/sanitizer.go b/internal/pkg/sanitizer/sanitizer.go
index b703d45..fd05ab9 100644
--- a/internal/pkg/sanitizer/sanitizer.go
+++ b/internal/pkg/sanitizer/sanitizer.go
@@ -38,8 +38,8 @@ func (s Sanitizer) Dominates(target ssa.Instruction) bool {
 		return false
 	}
 
-	if !s.Call.Block().Dominates(target.Block()) {
-		return false
+	if s.Call.Block().Dominates(target.Block()) && s.Call.Block() != target.Block() {
+		return true
 	}
 
 	if s.Call.Block() == target.Block() {


Test patch:
diff --git a/internal/pkg/levee/levee_test.go b/internal/pkg/levee/levee_test.go
index 68b30fb..a491c4c 100644
--- a/internal/pkg/levee/levee_test.go
+++ b/internal/pkg/levee/levee_test.go
@@ -16,8 +16,10 @@ package internal
 
 import (
 	"flag"
+	"fmt"
 	"io/ioutil"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/google/go-flow-levee/internal/pkg/debug"
@@ -46,7 +48,28 @@ func findTestPatterns(t *testing.T, testsDir string) (patterns []string) {
 		t.Fatalf("Failed to read tests dir (%s): %v", testsDir, err)
 	}
 	for _, f := range files {
-		patterns = append(patterns, filepath.Join(testsDir, f.Name()))
+		path := filepath.Join(testsDir, f.Name())
+		// Make sure the directory contains a go file to avoid failure on empty directories.
+		if err := checkForGoFiles(path); err != nil {
+			t.Fatalf("Could not verify presence of Go files in test directory: %v", err)
+		}
+
+		patterns = append(patterns, path)
 	}
 	return
 }
+
+func checkForGoFiles(path string) error {
+	pkgFiles, err := ioutil.ReadDir(path)
+	if err != nil {
+		return fmt.Errorf("failed to examine test directory (%s): %v", path, err)
+	}
+
+	for _, file := range pkgFiles {
+		if strings.HasSuffix(file.Name(), ".go") {
+			return nil
+		}
+	}
+	return fmt.Errorf("found no Go files in test directory (%s)", path)
+
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go
index 5dc8787..b887894 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go
@@ -57,3 +57,83 @@ func TestIncorrectSanitizationByValue(s core.Source) {
 	core.Sanitize(s)
 	core.Sink(s) // TODO want "a source has reached a sink"
 }
+
+func TestOnlySanitizedIfLoopIsTaken() {
+	var e interface{} = core.Source{}
+	for false {
+		e = core.Sanitize(e)[0]
+	}
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestTaintedInLoopAndSanitizedAfterLoop() {
+	var e interface{}
+	for false {
+		e = core.Source{}
+	}
+	e = core.Sanitize(e)[0]
+	core.Sink(e)
+}
+
+func TestMaybeTaintedInLoopButSanitizedBeforeLoopExit() {
+	var e interface{}
+	for false {
+		if false {
+			e = core.Source{}
+		}
+		e = core.Sanitize(e)[0]
+	}
+	// TODO want no report here
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestTaintedInIfButSanitizedBeforeIfExit() {
+	var e interface{}
+	if false {
+		e = core.Source{}
+		e = core.Sanitize(e)[0]
+	}
+	// TODO want no report here
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestPointerTaintedInIfButSanitizedBeforeIfExit() {
+	var e interface{}
+	if false {
+		s := &core.Source{}
+		core.SanitizePtr(s)
+		e = s
+	}
+	// TODO want no report here
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestSanitizedBeforeSinkInLoop() {
+	var e interface{}
+	for false {
+		e = core.Source{}
+		e = core.Sanitize(e)[0]
+		core.Sink(e)
+	}
+}
+
+func TestSanitizedBeforeMaybeSinkingMaybeTaintedValue() {
+	var obj interface{}
+	if false {
+		obj = core.Source{}
+	} else {
+		obj = 10
+	}
+
+	obj = core.Sanitize(obj)[0]
+
+	if false {
+		core.Sink(obj)
+	}
+}
+
+func TestSanitizedAfterSink() {
+	s := core.Source{}
+	core.Sink(s) // want "a source has reached a sink"
+	core.SanitizePtr(&s)
+}
diff --git a/internal/pkg/sanitizer/sanitizer_test.go b/internal/pkg/sanitizer/sanitizer_test.go
index 491bc84..8c55031 100644
--- a/internal/pkg/sanitizer/sanitizer_test.go
+++ b/internal/pkg/sanitizer/sanitizer_test.go
@@ -15,7 +15,6 @@
 package sanitizer
 
 import (
-	"reflect"
 	"testing"
 
 	"golang.org/x/tools/go/analysis"
@@ -25,60 +24,43 @@ import (
 )
 
 var testAnalyzer = &analysis.Analyzer{
-	Name:       "domination",
-	Run:        run,
-	Doc:        "test harness for domination logic",
-	Requires:   []*analysis.Analyzer{buildssa.Analyzer},
-	ResultType: reflect.TypeOf([]*ssa.Call{}),
+	Name:     "domination",
+	Run:      run,
+	Doc:      "test harness for domination logic",
+	Requires: []*analysis.Analyzer{buildssa.Analyzer},
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	in := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
-	var calls []*ssa.Call
+	var sanitizers []Sanitizer
+	var sinks []*ssa.Call
 	for _, fn := range in.SrcFuncs {
 		for _, b := range fn.Blocks {
 			for _, i := range b.Instrs {
 				if c, ok := i.(*ssa.Call); ok {
-					calls = append(calls, c)
+					name := c.Call.StaticCallee().Name()
+					switch name {
+					case "scrub":
+						sanitizers = append(sanitizers, Sanitizer{c})
+					case "Print":
+						sinks = append(sinks, c)
+					}
 				}
 			}
 		}
 	}
 
-	return calls, nil
+	for _, sink := range sinks {
+		for _, san := range sanitizers {
+			if san.Dominates(sink) {
+				pass.Reportf(sink.Pos(), "dominated")
+			}
+		}
+	}
+	return nil, nil
 }
 
 func TestDomination(t *testing.T) {
 	dir := analysistest.TestData()
-	r := analysistest.Run(t, dir, testAnalyzer)
-	if len(r) != 1 {
-		t.Fatalf("Got %d results, wanted one", len(r))
-	}
-	calls, ok := r[0].Result.([]*ssa.Call)
-	if !ok {
-		t.Fatalf("Got result of type %T, wanted []*ssa.Call", calls)
-	}
-
-	/*
-		    Call Sequence:
-
-				call time.Now()
-				call (time.Time).Weekday(t0)
-				call scrub("P@ssword1":string) // Sanitizer - index 2
-				call log.Print(t7...) // Dominated sink
-				call log.Print(t13...) // Non dominated sink due to an if statement.
-
-	*/
-
-	sanitizer := &Sanitizer{calls[2]}
-	dominatedSink := calls[3]
-	nonDominatedSink := calls[4]
-
-	if !sanitizer.Dominates(dominatedSink) {
-		t.Fatalf("Sanitizer %v should dominate the leak %v due to an if statement", sanitizer.Call, dominatedSink)
-	}
-
-	if sanitizer.Dominates(nonDominatedSink) {
-		t.Fatalf("Sanitizer %v should not dominate the leak %v due to no if statement", sanitizer.Call, nonDominatedSink)
-	}
+	analysistest.Run(t, dir, testAnalyzer)
 }
diff --git a/internal/pkg/sanitizer/testdata/tests.go b/internal/pkg/sanitizer/testdata/tests.go
index ac9a826..0679b6f 100644
--- a/internal/pkg/sanitizer/testdata/tests.go
+++ b/internal/pkg/sanitizer/testdata/tests.go
@@ -20,11 +20,31 @@ import (
 	"time"
 )
 
-func notDominated() {
+func dominatedSameBlock() {
+	pwd := "password"
+	pwd = scrub(pwd)
+	log.Print(pwd) // want "dominated"
+}
+
+func notDominatedSameBlock() {
+	pwd := "password"
+	log.Print(pwd)
+	pwd = scrub(pwd)
+}
+
+func dominatedDifferentBlocks() {
+	pwd := "password"
+	pwd = scrub(pwd)
+	if time.Now().Weekday() == time.Monday {
+		log.Print(pwd) // want "dominated"
+	}
+}
+
+func notDominatedDifferentBlocks() {
 	pwd := "P@ssword1"
 	if time.Now().Weekday() == time.Monday {
 		pwd = scrub(pwd)
-		log.Print(pwd)
+		log.Print(pwd) // want "dominated"
 	}
 
 	log.Print(pwd)