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

Issue:
Allow suppression of false positives via code comments.
This is a continuation of #87.

At time of writing, report of diagnostics can be suppressed at a function-level granularity.  While functional, this has potential to suppress a wider breadth of reports than intended.

Suppression via code comment, e.g. `nolint:levee <Justification>` would allow fine-grained suppression by immediately preceding a sink call.  Optionally, we may consider suppression of source identification or taint propagation.

Pull request description:
Allow report suppression via comments
Fixes #198.

I went with `levee.DoNotReport` as a suppression string. I think the name is quite clear and the syntax is Go-like (it looks sort of like calling a public function from the `levee` package, without the parens), which may help developers remember it.

## Future work

I think it would be good to produce a warning when a suppression doesn't actually suppress anything. I see two main reasons to do this:
  * If the erroneous suppression isn't removed, and the code changes such that there is now actually a report being suppressed, the developers won't know about it.
  * From a maintenance/readability point of view, a suppression comment that doesn't suppress anything is misleading and unnecessarily clutters the code.

- [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/configuration/README.md b/configuration/README.md
index 6c55fc4..309704e 100644
--- a/configuration/README.md
+++ b/configuration/README.md
@@ -92,6 +92,38 @@ Since no receiver matcher was provided, it will match any method beginning with
 
 As just two examples, this may be used to avoid analyzing test code, or to suppress "false positive" reports.
 
+### Suppressing false positives
+
+The analyzer may produce reports on pieces of code that are actually safe. If you run across such a "false positive" report, you may suppress it by adding a comment above the line where the taint reached the `sink`:
+
+```go
+// levee.DoNotReport
+mysinks.SinkF("here's a value: %v", safeValue)
+```
+
+You may include a justification in the suppression comment, e.g.:
+
+```go
+// levee.DoNotReport: I have verified that "safeValue" cannot actually contain sensitive data (mlevesquedion, Feb 05 2021).
+```
+
+In fact, as long as the exact string `levee.DoNotReport` appears at the beginning of a line in a comment above the line where a report would be produced, the report will be suppressed. In most cases, you may also be able to suppress a report by placing the comment on the line itself, e.g.:
+
+```go
+mysinks.SinkF("here's a value: %v", safeValue) // levee.DoNotReport
+```
+
+Finally, note that you can't suppress a report for a specific argument, so the following will not work:
+
+```go
+mysinks.Sinkln(safeValue, // levee.DoNotReport
+  otherValue)
+```
+
+A few things to keep in mind when using suppression:
+* Before suppressing, you should validate that a tainted value really can't reach a sink (i.e., you are really suppressing a _false_ positive).
+* You should periodically reexamine your suppressions to make sure that they are still accurate. If you suppress a report, but later on the code changes such that the report on a given line would actually be a _true_ positive, the analyzer won't tell you about it.
+
 ### Example configuration
 
 The following configuration could be used to identify possible instances of credential logging in Kubernetes.
diff --git a/internal/pkg/levee/levee.go b/internal/pkg/levee/levee.go
index 006cffa..6ac3fa0 100644
--- a/internal/pkg/levee/levee.go
+++ b/internal/pkg/levee/levee.go
@@ -16,23 +16,31 @@ package internal
 
 import (
 	"fmt"
+	"go/ast"
+	"go/token"
 	"strings"
 
 	"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/suppression"
 	"github.com/google/go-flow-levee/internal/pkg/utils"
 	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/go/ssa"
 )
 
 var Analyzer = &analysis.Analyzer{
-	Name:     "levee",
-	Run:      run,
-	Flags:    config.FlagSet,
-	Doc:      "reports attempts to source data to sinks",
-	Requires: []*analysis.Analyzer{source.Analyzer, fieldtags.Analyzer},
+	Name:  "levee",
+	Run:   run,
+	Flags: config.FlagSet,
+	Doc:   "reports attempts to source data to sinks",
+	Requires: []*analysis.Analyzer{
+		fieldtags.Analyzer,
+		source.Analyzer,
+		suppression.Analyzer,
+	},
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
@@ -42,6 +50,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	}
 	funcSources := pass.ResultOf[source.Analyzer].(source.ResultType)
 	taggedFields := pass.ResultOf[fieldtags.Analyzer].(fieldtags.ResultType)
+	suppressedNodes := pass.ResultOf[suppression.Analyzer].(suppression.ResultType)
 
 	for fn, sources := range funcSources {
 		propagations := make(map[*source.Source]propagation.Propagation, len(sources))
@@ -54,13 +63,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(conf, pass, propagations, instr)
+						reportSourcesReachingSink(conf, pass, suppressedNodes, propagations, instr)
 					}
 				case *ssa.Panic:
 					if conf.AllowPanicOnTaintedValues {
 						continue
 					}
-					reportSourcesReachingSink(conf, pass, propagations, instr)
+					reportSourcesReachingSink(conf, pass, suppressedNodes, propagations, instr)
 				}
 			}
 		}
@@ -69,15 +78,55 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return nil, nil
 }
 
-func reportSourcesReachingSink(conf *config.Config, pass *analysis.Pass, propagations map[*source.Source]propagation.Propagation, sink ssa.Instruction) {
+func reportSourcesReachingSink(conf *config.Config, pass *analysis.Pass, suppressedNodes suppression.ResultType, propagations map[*source.Source]propagation.Propagation, sink ssa.Instruction) {
 	for src, prop := range propagations {
-		if prop.IsTainted(sink) {
+		if prop.IsTainted(sink) && !isSuppressed(sink.Pos(), suppressedNodes, pass) {
 			report(conf, pass, src, sink.(ssa.Node))
 			break
 		}
 	}
 }
 
+func isSuppressed(pos token.Pos, suppressedNodes suppression.ResultType, pass *analysis.Pass) bool {
+	for _, f := range pass.Files {
+		if pos < f.Pos() || f.End() < pos {
+			continue
+		}
+		// astutil.PathEnclosingInterval produces the list of nodes that enclose the provided
+		// position, from the leaf node that directly contains it up to the ast.File node
+		path, _ := astutil.PathEnclosingInterval(f, pos, pos)
+		if len(path) < 2 {
+			return false
+		}
+		// Given the position of a call, path[0] holds the ast.CallExpr and
+		// path[1] holds the ast.ExprStmt. A suppressing comment may be associated
+		// with the name of the function being called (Ident, SelectorExpr), with the
+		// call itself (CallExpr), or with the entire expression (ExprStmt).
+		if ce, ok := path[0].(*ast.CallExpr); ok {
+			switch t := ce.Fun.(type) {
+			case *ast.Ident:
+				/*
+					Sink( // levee.DoNotReport
+				*/
+				if suppressedNodes.IsSuppressed(t) {
+					return true
+				}
+			case *ast.SelectorExpr:
+				/*
+					core.Sink( // levee.DoNotReport
+				*/
+				if suppressedNodes.IsSuppressed(t.Sel) {
+					return true
+				}
+			}
+		} else {
+			fmt.Printf("unexpected node received: %v (type %T); please report this issue\n", path[0], path[0])
+		}
+		return suppressedNodes.IsSuppressed(path[0]) || suppressedNodes.IsSuppressed(path[1])
+	}
+	return false
+}
+
 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")
diff --git a/internal/pkg/propagation/propagation.go b/internal/pkg/propagation/propagation.go
index f2a94d2..7c23463 100644
--- a/internal/pkg/propagation/propagation.go
+++ b/internal/pkg/propagation/propagation.go
@@ -251,6 +251,7 @@ func (prop *Propagation) taintCall(call *ssa.Call, maxInstrReached map[*ssa.Basi
 
 	if callee := call.Call.StaticCallee(); callee != nil && prop.config.IsSanitizer(utils.DecomposeFunction(callee)) {
 		prop.sanitizers = append(prop.sanitizers, &sanitizer.Sanitizer{Call: call})
+		return
 	}
 
 	// Do not traverse through a method call if it is being reached via a Source receiver.
diff --git a/internal/pkg/suppression/analyzer.go b/internal/pkg/suppression/analyzer.go
new file mode 100644
index 0000000..c3289ae
--- /dev/null
+++ b/internal/pkg/suppression/analyzer.go
@@ -0,0 +1,72 @@
+// 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 suppression defines an analyzer that identifies calls
+// suppressed by a comment.
+package suppression
+
+import (
+	"go/ast"
+	"reflect"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+const suppressionString = "levee.DoNotReport"
+
+// ResultType is a set of nodes that are suppressed by a comment.
+type ResultType map[ast.Node]bool
+
+// IsSuppressed determines whether the given node is suppressed.
+func (rt ResultType) IsSuppressed(n ast.Node) bool {
+	_, ok := rt[n]
+	return ok
+}
+
+var Analyzer = &analysis.Analyzer{
+	Name:       "suppression",
+	Doc:        "This analyzer identifies ast nodes that are suppressed by comments.",
+	Run:        run,
+	ResultType: reflect.TypeOf(new(ResultType)).Elem(),
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	result := map[ast.Node]bool{}
+
+	for _, f := range pass.Files {
+		for node, commentGroups := range ast.NewCommentMap(pass.Fset, f, f.Comments) {
+			for _, cg := range commentGroups {
+				if isSuppressingCommentGroup(cg) {
+					result[node] = true
+					pass.Reportf(node.Pos(), "suppressed")
+				}
+			}
+		}
+	}
+
+	return ResultType(result), nil
+}
+
+// isSuppressingCommentGroup determines whether a comment group contains
+// the suppression string at the beginning of a line in the comment text.
+func isSuppressingCommentGroup(commentGroup *ast.CommentGroup) bool {
+	for _, line := range strings.Split(commentGroup.Text(), "\n") {
+		trimmed := strings.TrimSpace(strings.TrimPrefix(strings.TrimPrefix(line, "//"), "/*"))
+		if strings.HasPrefix(trimmed, suppressionString) {
+			return true
+		}
+	}
+	return false
+}


Test patch:
diff --git a/internal/pkg/levee/testdata/src/levee_analysistest/example/core/sink.go b/internal/pkg/levee/testdata/src/levee_analysistest/example/core/sink.go
index e220f81..b16444e 100644
--- a/internal/pkg/levee/testdata/src/levee_analysistest/example/core/sink.go
+++ b/internal/pkg/levee/testdata/src/levee_analysistest/example/core/sink.go
@@ -22,6 +22,10 @@ func (s Sinker) Sink(args ...interface{}) {}
 
 func Sink(args ...interface{}) {}
 
+func SinkAndReturn(args ...interface{}) []interface{} {
+	return args
+}
+
 func Sinkf(format string, args ...interface{}) {}
 
 func FSinkf(writer io.Writer, args ...interface{}) {}
diff --git a/internal/pkg/levee/testdata/src/levee_analysistest/example/suppression/tests.go b/internal/pkg/levee/testdata/src/levee_analysistest/example/suppression/tests.go
new file mode 100644
index 0000000..3a472ef
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/levee_analysistest/example/suppression/tests.go
@@ -0,0 +1,139 @@
+// 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 suppression contains tests for false positive suppression.
+package suppression
+
+import (
+	"fmt"
+	"levee_analysistest/example/core"
+)
+
+func TestNotSuppressed(s core.Source) {
+	core.Sink(s) // want "a source has reached a sink"
+}
+
+func TestOnelineLineComment(s core.Source) {
+	// levee.DoNotReport
+	core.Sink(s)
+}
+
+func TestOnelineGeneralComment(s core.Source) {
+	/* levee.DoNotReport */
+	core.Sink(s)
+}
+
+func TestInlineLineComment(s core.Source) {
+	core.Sink(s) // levee.DoNotReport
+}
+
+func TestInlineGeneralComment(s core.Source) {
+	core.Sink(s) /* levee.DoNotReport */
+}
+
+func TestMultilineLineComment(s core.Source) {
+	// Line 1
+	// levee.DoNotReport
+	// Line 3
+	core.Sink(s)
+}
+
+func TestMultilineGeneralComment(s core.Source) {
+	/*
+		Line 1
+		levee.DoNotReport
+		Line 3
+	*/
+	core.Sink(s)
+}
+
+func TestAdjacentReports(s core.Source) {
+	core.Sink(s) // levee.DoNotReport
+	core.Sink(s) // want "a source has reached a sink"
+}
+
+func TestReportsSeparatedByLineComment(s core.Source) {
+	core.Sink(s) // want "a source has reached a sink"
+	// levee.DoNotReport
+	core.Sink(s)
+}
+
+func TestReportsSeparatedByGeneralComment(s core.Source) {
+	core.Sink(s) /*
+		levee.DoNotReport
+	*/
+	core.Sink(s) // want "a source has reached a sink"
+}
+
+func TestLineCommentBeforeGeneralComment(s core.Source) {
+	// levee.DoNotReport
+	/*
+		The line comment above and this comment
+		are part of the same comment group.
+	*/
+	core.Sink(s)
+}
+
+func TestSuppressPanic(s core.Source) {
+	// levee.DoNotReport
+	panic(s)
+	panic( // levee.DoNotReport
+		s,
+	)
+}
+
+func TestSuppressMultilineCall(s core.Source) {
+	// levee.DoNotReport
+	core.Sink(
+		"arg 1",
+		s,
+		"arg 3",
+	)
+
+	core.Sink( // levee.DoNotReport
+		"arg 1",
+		s,
+		"arg 3")
+
+	core.Sink("arg 1",
+		s,
+	) // levee.DoNotReport
+
+	core.Sink(
+		"arg 1",
+		s) // levee.DoNotReport
+}
+
+func TestIncorrectSuppressionViaArgument(s core.Source) {
+	core.Sink( // want "a source has reached a sink"
+		"arg 1",
+		s, // levee.DoNotReport
+	)
+
+	core.Sink("arg1", // levee.DoNotReport // want "a source has reached a sink"
+		s,
+	)
+}
+
+func TestSuppressNestedCall(s core.Source) {
+	fmt.Println(
+		// levee.DoNotReport
+		core.SinkAndReturn(s),
+	)
+
+	// TODO(#284): we don't actually want a report here
+	fmt.Println(
+		core.SinkAndReturn(s), // levee.DoNotReport // want "a source has reached a sink"
+	)
+}
diff --git a/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/sanitization/tests.go b/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/sanitization/tests.go
index 33e043b..a52e2bc 100644
--- a/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/sanitization/tests.go
+++ b/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/sanitization/tests.go
@@ -83,8 +83,7 @@ func TestMaybeTaintedInLoopButSanitizedBeforeLoopExit() {
 		}
 		e = core.Sanitize(e)[0]
 	}
-	// TODO(#155) want no report here
-	core.Sink(e) // want "a source has reached a sink"
+	core.Sink(e)
 }
 
 func TestTaintedInIfButSanitizedBeforeIfExit() {
@@ -93,8 +92,7 @@ func TestTaintedInIfButSanitizedBeforeIfExit() {
 		e = core.Source{}
 		e = core.Sanitize(e)[0]
 	}
-	// TODO(#155) want no report here
-	core.Sink(e) // want "a source has reached a sink"
+	core.Sink(e)
 }
 
 func TestPointerTaintedInIfButSanitizedBeforeIfExit() {
diff --git a/internal/pkg/levee/testdata/test-config.yaml b/internal/pkg/levee/testdata/test-config.yaml
index 3b9267a..76ae394 100644
--- a/internal/pkg/levee/testdata/test-config.yaml
+++ b/internal/pkg/levee/testdata/test-config.yaml
@@ -21,6 +21,8 @@ Sources:
 Sinks:
   - Package: "levee_analysistest/example/core"
     MethodRE: Sinkf?$
+  - Package: "levee_analysistest/example/core"
+    Method: SinkAndReturn
 Sanitizers:
   - Package: "levee_analysistest/example/core"
     MethodRE: "^Sanitize"
diff --git a/internal/pkg/suppression/analyzer_test.go b/internal/pkg/suppression/analyzer_test.go
new file mode 100644
index 0000000..ed8d7d0
--- /dev/null
+++ b/internal/pkg/suppression/analyzer_test.go
@@ -0,0 +1,25 @@
+// 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 suppression
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzer(t *testing.T) {
+	analysistest.Run(t, analysistest.TestData(), Analyzer, "./...")
+}
diff --git a/internal/pkg/suppression/testdata/src/suppression_analysistest/comments/comments.go b/internal/pkg/suppression/testdata/src/suppression_analysistest/comments/comments.go
new file mode 100644
index 0000000..511683a
--- /dev/null
+++ b/internal/pkg/suppression/testdata/src/suppression_analysistest/comments/comments.go
@@ -0,0 +1,34 @@
+// 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 comments
+
+func TestSuppressionComments() {
+	// levee.DoNotReport
+	println() // want "suppressed"
+
+	println()
+
+	// This call has a non-suppressing comment
+	// associated with it
+	println()
+
+	/*
+		This comment is suppressing too.
+		levee.DoNotReport
+	*/
+	println() // want "suppressed"
+
+	println() // levee.DoNotReport // want "suppressed"
+}
diff --git a/internal/pkg/suppression/testdata/src/suppression_analysistest/go.mod b/internal/pkg/suppression/testdata/src/suppression_analysistest/go.mod
new file mode 100644
index 0000000..bf4c103
--- /dev/null
+++ b/internal/pkg/suppression/testdata/src/suppression_analysistest/go.mod
@@ -0,0 +1,3 @@
+module suppression_analysistest
+
+go 1.15