Repository: google/go-flow-levee
Pull request #53 (2020-08-18 15:51:15)

Add tests for propagation through arrays
This PR adds tests demonstrating the behavior of our analyzer concerning taint propagation through arrays.

At present, this behavior is subject to some limitations. We consider the whole array to be tainted as soon as it is touched by a `Source`, and we do not consider the array to be safe even if the `Source` value is overwritten. This is in relation with issue #47.

- [x] Tests pass
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/.travis.yml b/.travis.yml
index b38b8f4..9cfc008 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -10,9 +10,9 @@ script:
 - go test -v ./...
 - >
   echo "Checking goimports..." && (
-    if [[ -n $(goimports -l internal cmd) ]]; then
-      goimports -d internal cmd
-      echo 'Please run `goimports -w cmd internal`.'
+    if [[ -n $(goimports -l cmd internal pkg) ]]; then
+      goimports -d cmd internal pkg
+      echo 'Please run `goimports -w cmd internal pkg`.'
       false
     fi
   )
@@ -27,7 +27,7 @@ script:
   )
 - >
   echo "Checking for license headers..." && (
-    FILES=$(grep -rL 'https://www.apache.org/licenses/LICENSE-2.0' internal cmd | grep -v ".json")
+    FILES=$(grep -rL 'https://www.apache.org/licenses/LICENSE-2.0' cmd internal pkg | grep -v ".json")
     if [[ -n ${FILES} ]]; then
       echo "The following files are missing the Apache header:"
       echo ${FILES}
diff --git a/internal/pkg/fieldpropagator/analyzer.go b/internal/pkg/fieldpropagator/analyzer.go
index 0743841..c5f25d7 100644
--- a/internal/pkg/fieldpropagator/analyzer.go
+++ b/internal/pkg/fieldpropagator/analyzer.go
@@ -21,6 +21,7 @@ import (
 	"reflect"
 
 	"github.com/google/go-flow-levee/internal/pkg/config"
+	"github.com/google/go-flow-levee/internal/pkg/fieldtags"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/buildssa"
 	"golang.org/x/tools/go/ssa"
@@ -44,12 +45,13 @@ var Analyzer = &analysis.Analyzer{
 A field propagator is a function that returns a source field.`,
 	Flags:      config.FlagSet,
 	Run:        run,
-	Requires:   []*analysis.Analyzer{buildssa.Analyzer},
+	Requires:   []*analysis.Analyzer{buildssa.Analyzer, fieldtags.Analyzer},
 	ResultType: reflect.TypeOf(new(ResultType)).Elem(),
 	FactTypes:  []analysis.Fact{new(isFieldPropagator)},
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
+	taggedFields := pass.ResultOf[fieldtags.Analyzer].(fieldtags.ResultType)
 	ssaInput := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
 
 	conf, err := config.ReadConfig()
@@ -66,7 +68,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		methods := ssaProg.MethodSets.MethodSet(ssaType.Type())
 		for i := 0; i < methods.Len(); i++ {
 			meth := ssaProg.MethodValue(methods.At(i))
-			analyzeBlocks(pass, conf, meth)
+			analyzeBlocks(pass, conf, taggedFields, meth)
 		}
 	}
 
@@ -77,7 +79,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return FieldPropagators(isFieldPropagator), nil
 }
 
-func analyzeBlocks(pass *analysis.Pass, conf *config.Config, meth *ssa.Function) {
+func analyzeBlocks(pass *analysis.Pass, conf *config.Config, tf fieldtags.TaggedFields, meth *ssa.Function) {
 	// Function does not return anything
 	if res := meth.Signature.Results(); res == nil || (*res).Len() == 0 {
 		return
@@ -91,17 +93,17 @@ func analyzeBlocks(pass *analysis.Pass, conf *config.Config, meth *ssa.Function)
 		if !ok {
 			continue
 		}
-		analyzeResults(pass, conf, meth, ret.Results)
+		analyzeResults(pass, conf, tf, meth, ret.Results)
 	}
 }
 
-func analyzeResults(pass *analysis.Pass, conf *config.Config, meth *ssa.Function, results []ssa.Value) {
+func analyzeResults(pass *analysis.Pass, conf *config.Config, tf fieldtags.TaggedFields, meth *ssa.Function, results []ssa.Value) {
 	for _, r := range results {
 		fa, ok := fieldAddr(r)
 		if !ok {
 			continue
 		}
-		if conf.IsSourceFieldAddr(fa) {
+		if conf.IsSourceFieldAddr(fa) || tf.IsSource(fa) {
 			pass.ExportObjectFact(meth.Object(), &isFieldPropagator{})
 		}
 	}
diff --git a/internal/pkg/fieldtags/analyzer.go b/internal/pkg/fieldtags/analyzer.go
index 39b940c..22ff822 100644
--- a/internal/pkg/fieldtags/analyzer.go
+++ b/internal/pkg/fieldtags/analyzer.go
@@ -17,13 +17,18 @@
 package fieldtags
 
 import (
+	"fmt"
 	"go/ast"
+	"go/types"
 	"reflect"
 	"strings"
 
+	"github.com/google/go-flow-levee/internal/pkg/utils"
+
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/ssa"
 )
 
 type keyValue struct {
@@ -37,6 +42,17 @@ var patterns sourcePatterns = []keyValue{
 	{"levee", "source"},
 }
 
+// TaggedFields is named type representing a slice of TaggedFields.
+type TaggedFields []TaggedField
+
+// ResultType is a slice of TaggedFields.
+type ResultType = TaggedFields
+
+// A TaggedField contains the necessary information to identify a tagged struct field.
+type TaggedField struct {
+	pkg, typ, field string
+}
+
 var Analyzer = &analysis.Analyzer{
 	Name: "fieldtags",
 	Doc:  "This analyzer identifies Source fields based on their tags. Tags are expected to satisfy the `go vet -structtag` format.",
@@ -44,28 +60,39 @@ var Analyzer = &analysis.Analyzer{
 	Requires: []*analysis.Analyzer{
 		inspect.Analyzer,
 	},
-	ResultType: reflect.TypeOf(new([]*ast.Field)).Elem(),
+	ResultType: reflect.TypeOf(new(ResultType)).Elem(),
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspectResult := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	var results []*ast.Field
+	var taggedFields TaggedFields
 
 	nodeFilter := []ast.Node{
-		(*ast.Field)(nil),
+		(*ast.TypeSpec)(nil),
 	}
 
 	inspectResult.Preorder(nodeFilter, func(n ast.Node) {
-		field, ok := n.(*ast.Field)
+		t, ok := n.(*ast.TypeSpec)
 		if !ok {
 			return
 		}
-		if patterns.isSource(field) {
-			results = append(results, field)
-			pass.Reportf(field.Pos(), "tagged field")
+		s, ok := t.Type.(*ast.StructType)
+		if !ok || s.Fields == nil {
+			return
+		}
+		for _, f := range (*s).Fields.List {
+			if patterns.isSource(f) && len(f.Names) > 0 {
+				tf := TaggedField{
+					pkg:   pass.Pkg.Name(),
+					typ:   t.Name.Name,
+					field: f.Names[0].Name,
+				}
+				taggedFields = append(taggedFields, tf)
+				pass.Reportf(f.Pos(), "tagged field: %s", tf)
+			}
 		}
 	})
-	return results, nil
+	return taggedFields, nil
 }
 
 func (sp *sourcePatterns) isSource(field *ast.Field) bool {
@@ -119,3 +146,33 @@ func (sp *sourcePatterns) isSource(field *ast.Field) bool {
 
 	return false
 }
+
+// IsSource determines whether a FieldAddr is a source, that is whether it refers to a field previously identified as a source.
+func (t TaggedFields) IsSource(f *ssa.FieldAddr) bool {
+	n, ok := namedType(f)
+	if !ok {
+		return false
+	}
+	fName := utils.FieldName(f)
+	for _, tf := range t {
+		if tf.matches(n.String(), fName) {
+			return true
+		}
+	}
+	return false
+}
+
+func namedType(f *ssa.FieldAddr) (*types.Named, bool) {
+	structType := f.X.Type()
+	deref := utils.Dereference(structType)
+	named, ok := deref.(*types.Named)
+	return named, ok
+}
+
+func (t TaggedField) matches(typeName, fieldName string) bool {
+	return fmt.Sprintf("%s.%s", t.pkg, t.typ) == typeName && t.field == fieldName
+}
+
+func (t TaggedField) String() string {
+	return fmt.Sprintf("%s.%s.%s", t.pkg, t.typ, t.field)
+}
diff --git a/pkg/levee/levee.go b/pkg/levee/levee.go
index 77d0d4e..84690f3 100644
--- a/pkg/levee/levee.go
+++ b/pkg/levee/levee.go
@@ -1,3 +1,17 @@
+// Copyright 2019 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 levee exports the levee Analyzer.
 package levee
 


Test patch:
diff --git a/internal/pkg/fieldpropagator/testdata/src/source/test.go b/internal/pkg/fieldpropagator/testdata/src/source/test.go
index 931c55a..6d1dbf2 100644
--- a/internal/pkg/fieldpropagator/testdata/src/source/test.go
+++ b/internal/pkg/fieldpropagator/testdata/src/source/test.go
@@ -15,6 +15,7 @@
 package source
 
 type Source struct {
+	secret  string `levee:"source"`
 	data    string
 	dataPtr *string
 	id      int
@@ -24,6 +25,10 @@ func (s Source) ID() int {
 	return s.id
 }
 
+func (s Source) Secret() string { // want Secret:"field propagator identified"
+	return s.secret
+}
+
 func (s Source) Data() string { // want Data:"field propagator identified"
 	return s.data
 }
diff --git a/internal/pkg/fieldtags/testdata/src/tests/test.go b/internal/pkg/fieldtags/testdata/src/tests/test.go
index 7a44e12..3c90bd8 100644
--- a/internal/pkg/fieldtags/testdata/src/tests/test.go
+++ b/internal/pkg/fieldtags/testdata/src/tests/test.go
@@ -15,9 +15,9 @@
 package fieldtags
 
 type Person struct {
-	password           string      `levee:"source"`               // want "tagged field"
-	secret             string      `json:"secret" levee:"source"` // want "tagged field"
-	another            interface{} "levee:\"source\""             // want "tagged field"
+	password           string      `levee:"source"`               // want "tagged field: fieldtags.Person.password"
+	secret             string      `json:"secret" levee:"source"` // want "tagged field: fieldtags.Person.secret"
+	another            interface{} "levee:\"source\""             // want "tagged field: fieldtags.Person.another"
 	name               string      `some_key:"non_secret"`
 	someNotTaggedField int
 }
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go b/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go
new file mode 100644
index 0000000..9168132
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go
@@ -0,0 +1,42 @@
+// 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 TestArrayLiteralContainingSourceIsTainted(s core.Source) {
+	tainted := [1]core.Source{s}
+	core.Sink(tainted) // want "a source has reached a sink"
+}
+
+func TestArrayIsTaintedWhenSourceIsInserted(s core.Source) {
+	arr := [2]interface{}{nil, nil}
+	arr[0] = s
+	core.Sink(arr) // want "a source has reached a sink"
+}
+
+func TestValueObtainedFromTaintedArrayIsTainted(s core.Source) {
+	arr := [2]interface{}{nil, nil}
+	arr[0] = s
+	core.Sink(arr[1]) // want "a source has reached a sink"
+}
+
+func TestArrayRemainsTaintedWhenSourceIsOverwritten(s core.Source) {
+	arr := [2]interface{}{s, nil}
+	arr[0] = nil
+	core.Sink(arr) // want "a source has reached a sink"
+}