Repository: google/go-flow-levee
Pull request #262 (2021-01-13 21:35:28)

Fix incorrect source identification logic
Follow-up to #261.

## What's the bug?

The bug involves incorrect source identification logic in `sourcetype.IsSourceType`. In the following snippet taken from `propagation.visitCall`:

```go
if recv := call.Call.Signature().Recv(); recv != nil && sourcetype.IsSourceType(prop.config, prop.taggedFields, recv.Type()) {
    ...
    for _, a := range call.Call.Args[1:] {
```

I was expecting the slice operation on `Args` to be fine, because I thought `sourcetype.IsSourceType` would prevent the `if` from being entered when the receiver had an interface type. Indeed, interfaces are not supposed to be sources. However, the current code does not actually prevent interface types from being configured as sources:

```go
case *types.Named:
    return c.IsSourceType(utils.DecomposeType(tt)) || IsSourceType(c, tf, tt.Underlying())
```

If `tt` has `types.Interface` as `Underlying` type, and it matches one of the configured matchers, this will return `true`.

So in the snippet from `visitCall`, if an interface was configured as a source, and the method being called had no arguments, `call.Call.Args` would be empty, because `call.Call.Args` only contains the receiver if the receiver's type is statically known (which is not the case when the receiver has interface type).

## How did this panic come about?

The panic was unknowingly introduced in #252. The behavior that caused a panic wasn't exercised by our tests, nor by running the analyzer on kubernetes. Therefore, this error wouldn't have been caught by running a check on kubernetes in CI. (I am still planning to investigate GitHub Actions, because I think it would be good to have anyway). When I ran across this bug (while running on a different codebase, i.e. not kubernetes), my first thought was that I must have gone through the following sequence of actions:

1. Write #252 and open the PR. Run the check on kubernetes to confirm that the analyzer doesn't crash.
2. Review process occurs. I make a change that I (wrongly) think has no impact on the behavior and I forget to run the check again.

But I didn't make any such changes, so I didn't need to run the check again, and had I run it, it wouldn't have caught the bug.

## Next steps

We should consider making it "structurally" impossible for interface types to be configured as sources. There isn't much gain to being able to say that every field on a struct is sensitive by not providing a field matcher, and in practice, we don't use this facility in kubernetes, so we should consider making it mandatory to provide a field matcher. That way, it would be impossible to define a source matcher that matches an interface type, because interfaces have no fields.

- [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


mlevesquedion: Thank you for your kind words! I did make a special effort to be especially thorough on this one.

I agree that the commit message could be improved, I'll be sure to integrate your suggestion when I squash.

Patch:
diff --git a/internal/pkg/propagation/propagation.go b/internal/pkg/propagation/propagation.go
index 8894f61..36908be 100644
--- a/internal/pkg/propagation/propagation.go
+++ b/internal/pkg/propagation/propagation.go
@@ -258,7 +258,12 @@ func (prop *Propagation) visitCall(call *ssa.Call, maxInstrReached map[*ssa.Basi
 	// Source methods that return tainted values regardless of their arguments should be identified by the fieldpropagator analyzer.
 	if recv := call.Call.Signature().Recv(); recv != nil && sourcetype.IsSourceType(prop.config, prop.taggedFields, recv.Type()) {
 		visitingFromArg := false
+		// Interface types cannot be sources. If the receiver is not a source, the
+		// above condition will be false, so this code won't be executed.
+		// When the receiver's type is statically known (it isn't an interface type),
+		// it will be the first element of the Args slice.
 		if len(call.Call.Args) == 0 {
+			fmt.Printf("call.Call.Args was empty; please report this issue")
 			return
 		}
 		for _, a := range call.Call.Args[1:] {
diff --git a/internal/pkg/sourcetype/sourcetype.go b/internal/pkg/sourcetype/sourcetype.go
index 1e21025..f8ba539 100644
--- a/internal/pkg/sourcetype/sourcetype.go
+++ b/internal/pkg/sourcetype/sourcetype.go
@@ -26,13 +26,17 @@ import (
 
 // IsSourceType determines whether a Type is a Source Type.
 // A Source Type is either:
-// - A Named Type that is classified as a Source
-// - A composite type that contains a Source Type
+// - A Named Struct Type that is configured as a Source
 // - A Struct Type that contains a tagged field
+// - A composite type that contains a Source Type
 func IsSourceType(c *config.Config, tf fieldtags.ResultType, t types.Type) bool {
 	deref := utils.Dereference(t)
 	switch tt := deref.(type) {
 	case *types.Named:
+		if _, ok := utils.Dereference(tt.Underlying()).(*types.Interface); ok {
+			// interfaces cannot be sources
+			return false
+		}
 		return c.IsSourceType(utils.DecomposeType(tt)) || IsSourceType(c, tf, tt.Underlying())
 	case *types.Array:
 		return IsSourceType(c, tf, tt.Elem())


Test patch:
diff --git a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
index 7338698..f9c5355 100644
--- a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
+++ b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
@@ -120,3 +120,7 @@ type DeeplyNested map[string][]map[string]map[string][][]map[string]Source
 func TestTaggedSourceIdentification() {
 	_ = TaggedSource{} // want "source identified"
 }
+
+// No source should be identified here, because SourceInterface is an interface type.
+func TestNamedInterfaceIsNotSource(x SourceInterface) {
+}
diff --git a/internal/pkg/source/testdata/src/analyzertest/sourcetest/source.go b/internal/pkg/source/testdata/src/analyzertest/sourcetest/source.go
index 0ef7a4c..5582551 100644
--- a/internal/pkg/source/testdata/src/analyzertest/sourcetest/source.go
+++ b/internal/pkg/source/testdata/src/analyzertest/sourcetest/source.go
@@ -32,3 +32,5 @@ type TaggedSource struct {
 	Data string `levee:"source"`
 	ID   int
 }
+
+type SourceInterface interface{}
diff --git a/internal/pkg/source/testdata/src/analyzertest/test-config.yaml b/internal/pkg/source/testdata/src/analyzertest/test-config.yaml
index 043f7ae..56c29ac 100644
--- a/internal/pkg/source/testdata/src/analyzertest/test-config.yaml
+++ b/internal/pkg/source/testdata/src/analyzertest/test-config.yaml
@@ -16,3 +16,6 @@ Sources:
 - PackageRE: ""
   TypeRE: "^Source$"
   FieldRE: "^Data"
+# The below cannot actually be a Source, because it is an interface type
+- PackageRE: ""
+  Type: "SourceInterface"