Skip to content

Commit

Permalink
Allow report suppression via comments (#283)
Browse files Browse the repository at this point in the history
Reports can be suppressed using a code comment containing the string "levee.DoNotReport" at the beginning of a line in the comment. The comment has to be associated with the call, not with one of the arguments.
  • Loading branch information
mlevesquedion authored Feb 17, 2021
1 parent a278ac7 commit a885620
Show file tree
Hide file tree
Showing 9 changed files with 369 additions and 9 deletions.
32 changes: 32 additions & 0 deletions configuration/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
67 changes: 58 additions & 9 deletions internal/pkg/levee/levee.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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))
Expand All @@ -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)
}
}
}
Expand All @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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{}) {}
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
)
}
2 changes: 2 additions & 0 deletions internal/pkg/levee/testdata/test-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
72 changes: 72 additions & 0 deletions internal/pkg/suppression/analyzer.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading

0 comments on commit a885620

Please sign in to comment.