Repository: google/go-flow-levee
Pull request #52 (2020-08-18 15:43:17)

Use more appropriate shapes in .dot debug output
(This is just a small QOL change).

## Changes
* Use rectangles instead of diamonds for `ssa.Node`s that are *both* instructions and values.
* Use diamonds instead of squares for `ssa.Node`s that are merely instructions.

## Justification
As it turns out, most `ssa.Node`s are both instructions and values. Some of them have rather lengthy string representations. The "diamond" shape does not accommodate this nicely, producing unsightly nodes such as this :
![image](https://user-images.githubusercontent.com/28677454/90534397-af685780-e147-11ea-8e05-1af219543185.png)
Nodes that are merely `Instructions` usually have short string representations, so using "diamond" for them is not a problem.

What a rectangle looks like : 
![image](https://user-images.githubusercontent.com/28677454/90534919-5220d600-e148-11ea-974b-c1399b4b1cf0.png)

- [x] Tests pass
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/configuration/README.md b/configuration/README.md
index c8383e3..98fa684 100644
--- a/configuration/README.md
+++ b/configuration/README.md
@@ -28,7 +28,7 @@ Sources are identified via regexp according to package, type, and field names.
 }
 ```
 
-Sinks, sanitizers, and some propagators (below) are identified via regexp according to package, method, and (optional) receiver name.
+Sinks and sanitizers are identified via regexp according to package, method, and (optional) receiver name.
 
 ```json
 {
@@ -49,33 +49,7 @@ Sinks, sanitizers, and some propagators (below) are identified via regexp accord
 }
 ```
 
-We separate propagators into the following types:
-
-A *transforming propagator* produces a tainted value which may contain source data.
-For instance, a struct's `String()` method or a serializable's `Marshal()` method are transforming propagators.
-The value returned by either can contain source data without being identifiable as the specified source's type.
-Transforming propagators are identified via regexp according to package and function name.
-
-An *argument propagator* is a propagator which taints an input argument rather than a return value.
-This is done, e.g., by buffer writers such as `fmt.Fprintf`, though could conceivably apply to any function that takes a source and any other reference argument.
-At time of writing, argument propagators only applies to the first argument of a method, expecting a pattern similar to buffer writers.
-Configuration currently only allows for one argument propagator to be specified.
-Argument propagators are identified via regexp matching the fully-qualified type name.
-
-```json
-{
-  "TransformingPropagators": [
-    {
-      "PackageRE": "<package path regexp>",
-      "ReceiverRE": "<type name regexp>",
-      "MethodRE": "<method name regexp>"
-    }
-  ],
-  "PropagatorArgs": {
-    "ArgumentTypeRE": "^io\\.(?:Writer|ReadWriter|WriteCloser|ReadWriteCloser)$"
-  }
-}
-```
+Taint propagation is performed automatically and does not need to be explicitly configured.
 
 For matchers that accept a `ReceiverRE` regexp matcher, an unspecified string will match any (or no) receiver.
 To match only methods without any receiver (i.e., a top-level function), use the matcher `^$` to match an empty-string receiver name.
diff --git a/configuration/example-config.json b/configuration/example-config.json
index 5a2b883..4a86565 100644
--- a/configuration/example-config.json
+++ b/configuration/example-config.json
@@ -29,14 +29,4 @@
     }
   ],
   "Sanitizers": [],
-  "TransformingPropagators": [
-    {
-      "PackageName": ".*proto$",
-      "ReceiverRE": "",
-      "MethodRE": "^Clone$|^Marshal"
-    }
-  ],
-  "PropagatorArgs": {
-    "ArgumentTypeRE": "^io\\.(?:Writer|ReadWriter|WriteCloser|ReadWriteCloser)$"
-  }
 }
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 7be108b..6d3851a 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -37,40 +37,9 @@ func init() {
 
 // config contains matchers and analysis scope information
 type Config struct {
-	Sources                 []sourceMatcher
-	Sinks                   []callMatcher
-	Sanitizers              []callMatcher
-	TransformingPropagators []transformingPropagatorMatcher
-	PropagatorArgs          argumentPropagatorMatcher
-	Allowlist               []packageMatcher
-	AnalysisScope           []packageMatcher
-}
-
-// shouldSkip returns true for any function that is outside analysis scope,
-// that is allowlisted,
-// whose containing package imports "testing"
-// or whose containing package does not import any package containing a source or a sink.
-func (c Config) shouldSkip(pkg *types.Package) bool {
-	if isTestPkg(pkg) || !c.isInScope(pkg) || c.isAllowlisted(pkg) {
-		return true
-	}
-
-	// TODO Does this skip packages that own sources/sinks but don't import others?
-	for _, im := range pkg.Imports() {
-		for _, s := range c.Sinks {
-			if s.matchPackage(im) {
-				return false
-			}
-		}
-
-		for _, s := range c.Sources {
-			if s.PackageRE.MatchString(im.Path()) {
-				return false
-			}
-		}
-	}
-
-	return true
+	Sources    []sourceMatcher
+	Sinks      []callMatcher
+	Sanitizers []callMatcher
 }
 
 func (c Config) IsSink(call *ssa.Call) bool {
@@ -150,54 +119,6 @@ func (c Config) IsSourceFieldAddr(fa *ssa.FieldAddr) bool {
 	return false
 }
 
-func (c Config) IsPropagator(call *ssa.Call) bool {
-	return c.isTransformingPropagator(call)
-}
-
-// A call is a transforming propagator if its name matches a pattern in the config
-// and at least one of its arguments is a Source.
-func (c Config) isTransformingPropagator(call *ssa.Call) bool {
-	for _, p := range c.TransformingPropagators {
-		if !p.match(call) {
-			continue
-		}
-
-		for _, a := range call.Call.Args {
-			// TODO Handle ChangeInterface case.
-			switch t := a.(type) {
-			case *ssa.MakeInterface:
-				if c.IsSource(utils.Dereference(t.X.Type())) {
-					return true
-				}
-			case *ssa.Parameter:
-				if c.IsSource(utils.Dereference(t.Type())) {
-					return true
-				}
-			}
-		}
-	}
-
-	return false
-}
-
-func (c Config) isAllowlisted(pkg *types.Package) bool {
-	for _, w := range c.Allowlist {
-		if w.match(pkg) {
-			return true
-		}
-	}
-	return false
-}
-
-func (c Config) isInScope(pkg *types.Package) bool {
-	for _, s := range c.AnalysisScope {
-		if s.match(pkg) {
-			return true
-		}
-	}
-	return false
-}
-
 // A sourceMatcher defines what types are or contain sources.
 // Within a given type, specific field access can be specified as the actual source data
 // via the fieldRE.
@@ -216,33 +137,6 @@ func (s sourceMatcher) match(n *types.Named) bool {
 	return s.PackageRE.MatchString(n.Obj().Pkg().Path()) && s.TypeRE.MatchString(n.Obj().Name())
 }
 
-type transformingPropagatorMatcher struct {
-	PackageName string
-	MethodRE    regexp.Regexp
-}
-
-func (t transformingPropagatorMatcher) match(call *ssa.Call) bool {
-	if call.Call.StaticCallee() == nil ||
-		call.Call.StaticCallee().Pkg == nil ||
-		call.Call.StaticCallee().Pkg.Pkg.Path() != t.PackageName {
-		return false
-	}
-
-	return t.MethodRE.MatchString(call.Call.StaticCallee().Name())
-}
-
-type argumentPropagatorMatcher struct {
-	ArgumentTypeRE regexp.Regexp
-}
-
-type packageMatcher struct {
-	PackageNameRE regexp.Regexp
-}
-
-func (pm packageMatcher) match(pkg *types.Package) bool {
-	return pm.PackageNameRE.MatchString(pkg.Path())
-}
-
 type callMatcher struct {
 	PackageRE  regexp.Regexp
 	ReceiverRE regexp.Regexp
@@ -299,12 +193,3 @@ func ReadConfig() (*Config, error) {
 	_ = loadedFromCache
 	return readConfigCached, readConfigCachedErr
 }
-
-func isTestPkg(p *types.Package) bool {
-	for _, im := range p.Imports() {
-		if im.Name() == "testing" {
-			return true
-		}
-	}
-	return false
-}
diff --git a/internal/pkg/debug/render/dot.go b/internal/pkg/debug/render/dot.go
index 33b84f6..ba15a9e 100644
--- a/internal/pkg/debug/render/dot.go
+++ b/internal/pkg/debug/render/dot.go
@@ -102,9 +102,9 @@ func nodeShape(n ssa.Node) string {
 	_, isInstr := n.(ssa.Instruction)
 	switch {
 	case isValue && isInstr:
-		return "diamond"
+		return "rectangle"
 	case isInstr:
-		return "square"
+		return "diamond"
 	default:
 		return "ellipse"
 	}
diff --git a/internal/pkg/levee/levee.go b/internal/pkg/levee/levee.go
index d947013..6ee1c75 100644
--- a/internal/pkg/levee/levee.go
+++ b/internal/pkg/levee/levee.go
@@ -63,21 +63,6 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				case fieldPropagators.Contains(v):
 					sources = append(sources, source.New(v, conf))
 
-				case conf.IsPropagator(v):
-					// Handling the case where sources are propagated to io.Writer
-					// (ex. proto.MarshalText(&buf, c)
-					// In such cases, "buf" becomes a source, and not the return value of the propagator.
-					// TODO Do not hard-code logging sinks usecase
-					// TODO  Handle case of os.Stdout and os.Stderr.
-					// TODO  Do not hard-code the position of the argument, instead declaratively
-					//  specify the position of the propagated source.
-					// TODO  Consider turning propagators that take io.Writer into sinks.
-					if a := getArgumentPropagator(conf, v); a != nil {
-						sources = append(sources, source.New(a, conf))
-					} else {
-						sources = append(sources, source.New(v, conf))
-					}
-
 				case conf.IsSink(v):
 					c := makeCall(v)
 					for _, s := range sources {
@@ -107,18 +92,3 @@ func report(pass *analysis.Pass, source *source.Source, sink ssa.Node) {
 	fmt.Fprintf(&b, ", source: %v", pass.Fset.Position(source.Node().Pos()))
 	pass.Reportf(sink.Pos(), b.String())
 }
-
-func getArgumentPropagator(c *config.Config, call *ssa.Call) ssa.Node {
-	if call.Call.Signature().Params().Len() == 0 {
-		return nil
-	}
-
-	firstArg := call.Call.Signature().Params().At(0)
-	if c.PropagatorArgs.ArgumentTypeRE.MatchString(firstArg.Type().String()) {
-		if a, ok := call.Call.Args[0].(*ssa.MakeInterface); ok {
-			return a.X.(ssa.Node)
-		}
-	}
-
-	return nil
-}
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index eae79fa..b72be80 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -29,7 +29,6 @@ import (
 type classifier interface {
 	IsSource(types.Type) bool
 	IsSanitizer(*ssa.Call) bool
-	IsPropagator(*ssa.Call) bool
 	IsSourceFieldAddr(*ssa.FieldAddr) bool
 	IsSinkFunction(fn *ssa.Function) bool
 }


Test patch:
diff --git a/internal/pkg/levee/testdata/test-config.json b/internal/pkg/levee/testdata/test-config.json
index 68e7f0c..0664e03 100644
--- a/internal/pkg/levee/testdata/test-config.json
+++ b/internal/pkg/levee/testdata/test-config.json
@@ -17,10 +17,5 @@
       "PackageRE": "^example.com/core$",
       "MethodRE": "^Sanitize"
     }
-  ],
-  "TransformingPropagators": [
-  ],
-  "PropagatorArgs": {},
-  "Allowlist": [],
-  "AnalysisScope": []
+  ]
 }
diff --git a/internal/pkg/source/source_test.go b/internal/pkg/source/source_test.go
index 57d7acf..d6c815b 100644
--- a/internal/pkg/source/source_test.go
+++ b/internal/pkg/source/source_test.go
@@ -51,11 +51,6 @@ func (c *testConfig) IsSanitizer(call *ssa.Call) bool {
 	return match
 }
 
-func (c *testConfig) IsPropagator(call *ssa.Call) bool {
-	match, _ := regexp.MatchString(c.propagatorsPattern, call.String())
-	return match
-}
-
 func (c *testConfig) IsSourceFieldAddr(field *ssa.FieldAddr) bool {
 	match, _ := regexp.MatchString(c.fieldsPattern, utils.FieldName(field))
 	return match
diff --git a/internal/pkg/varargs/varargs_test.go b/internal/pkg/varargs/varargs_test.go
index 042eb31..47ce98d 100644
--- a/internal/pkg/varargs/varargs_test.go
+++ b/internal/pkg/varargs/varargs_test.go
@@ -41,10 +41,6 @@ func (c *testConfig) IsSanitizer(call *ssa.Call) bool {
 	return false
 }
 
-func (c *testConfig) IsPropagator(call *ssa.Call) bool {
-	return false
-}
-
 func (c *testConfig) IsSourceFieldAddr(field *ssa.FieldAddr) bool {
 	return false
 }