Repository: google/go-flow-levee
Pull request #264 (2021-01-19 22:36:00)

Allow interfaces to be source types
This PR reverts some of the changes from the recent #262 PR.

The gist of that PR was that when visiting a 0-arg method on an interface type, the code in `visitCall` could panic because it assumed the `Args` slice would be non-empty. Initially, I wrote that code thinking that interfaces couldn't be source types anyway, so the 0-arg case wouldn't arise. The PR fixed the panic, and made it impossible for interface types to be identified as source types.

After some offline discussion with @PurelyApplied, I am opening this PR to remove the restriction on interface types. Despite the check in `IsSourceType`, the restriction isn't documented anywhere, and using interface types isn't actually blocked in the config, so the current behavior could lead to confusing issues down the line.

To further justify the change, consider the following example of a way a user may wish to use interfaces to identify source types:

```go
type Secret interface {
        ASecret()
}
```

Right now, a user could configure such an interface to be a source:

```yaml
Sources:
  - Package: <the-package>
  - Type: Secret
```

But this would silently be blocked by the restriction in `IsSourceType` (removed by this PR).

- [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

(I also ran this change against the codebase where the panic fixed by #261/#262 was discovered.)

mlevesquedion: Good conversation indeed! I have opened #272 with what I think we should do, + a few more general thoughts. PTAL.

Patch:
diff --git a/internal/pkg/propagation/propagation.go b/internal/pkg/propagation/propagation.go
index 36908be..7181f6c 100644
--- a/internal/pkg/propagation/propagation.go
+++ b/internal/pkg/propagation/propagation.go
@@ -257,16 +257,18 @@ func (prop *Propagation) visitCall(call *ssa.Call, maxInstrReached map[*ssa.Basi
 	// Do traverse if the call is being reached through a tainted argument.
 	// 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 the receiver is not statically known (it has interface type) and the
+		// method has no arguments, Args will be empty.
 		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:] {
+		visitingFromArg := false
+		for i, a := range call.Call.Args {
+			// If the receiver's type is statically known,
+			// it will be the first element of the Args slice.
+			if !call.Call.IsInvoke() && i == 0 {
+				continue
+			}
 			if prop.tainted[a.(ssa.Node)] {
 				visitingFromArg = true
 			}
diff --git a/internal/pkg/sourcetype/sourcetype.go b/internal/pkg/sourcetype/sourcetype.go
index f8ba539..1cb9496 100644
--- a/internal/pkg/sourcetype/sourcetype.go
+++ b/internal/pkg/sourcetype/sourcetype.go
@@ -33,10 +33,6 @@ 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/levee/testdata/src/example.com/core/source.go b/internal/pkg/levee/testdata/src/example.com/core/source.go
index a72ddc7..048e450 100644
--- a/internal/pkg/levee/testdata/src/example.com/core/source.go
+++ b/internal/pkg/levee/testdata/src/example.com/core/source.go
@@ -62,3 +62,7 @@ func (i Innocuous) GetID() int {
 func (i Innocuous) GetData() string {
 	return i.Data
 }
+
+type SourceManipulator interface {
+	Propagate(string) string
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/method/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/method/tests.go
new file mode 100644
index 0000000..be7de64
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/method/tests.go
@@ -0,0 +1,29 @@
+// 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 method
+
+import (
+	"example.com/core"
+)
+
+func TestMethodCallOnStaticallyKnownReceiverPropagatesTaint(s core.Source) {
+	data := s.Propagate(s.Data)
+	core.Sink(data) // want "a source has reached a sink"
+}
+
+func TestMethodCallOnStaticallyUnknownReceiverPropagatesTaint(sm core.SourceManipulator, s core.Source) {
+	data := sm.Propagate(s.Data)
+	core.Sink(data) // want "a source has reached a sink"
+}
diff --git a/internal/pkg/levee/testdata/test-config.yaml b/internal/pkg/levee/testdata/test-config.yaml
index be2a2fd..ce8380d 100644
--- a/internal/pkg/levee/testdata/test-config.yaml
+++ b/internal/pkg/levee/testdata/test-config.yaml
@@ -16,6 +16,8 @@ Sources:
   - PackageRE: "^example.com/core$"
     TypeRE: "^Source$"
     FieldRE: "^Data"
+  - PackageRE: "^example.com/core$"
+    TypeRE: "^SourceManipulator$"
 Sinks:
   - PackageRE: "^example.com/core$"
     MethodRE: Sinkf?$
diff --git a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
index f9c5355..61dc95e 100644
--- a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
+++ b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
@@ -121,6 +121,5 @@ func TestTaggedSourceIdentification() {
 	_ = TaggedSource{} // want "source identified"
 }
 
-// No source should be identified here, because SourceInterface is an interface type.
-func TestNamedInterfaceIsNotSource(x SourceInterface) {
+func TestNamedInterface(x SourceInterface) { // want "source identified"
 }