Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow interfaces to be source types #264

Merged
merged 7 commits into from
Jan 29, 2021
Merged

Allow interfaces to be source types #264

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
26c1839
Rework comments involving Args
Jan 19, 2021
3fa7daf
Remove print statement in case of empty Args
Jan 19, 2021
642f144
Allow interfaces to be sources
Jan 19, 2021
74e705a
Add missing tests
Jan 21, 2021
bbfa778
Fix bug: skip only receiver arg when statically known
Jan 21, 2021
e7e00aa
Use !IsInvoke() instead of StaticCallee() != nil
Jan 27, 2021
e3565d7
Merge master, fix conflict
Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions internal/pkg/propagation/propagation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,13 @@ 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 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

type SourceProducer interface {  // configure as source
  Produce string
}

func TestProduction(sp SourceProducer) {
  core.Sink(sp.Produce())  // TODO want ".*"
}

If we're wanting to permit identification of sources via interface, we need to not skip len(call.Call.Args) == 0 entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I think this needs to be handled by the fieldpropagator analyzer. At least, that's how it works for structs. This analyzer avoids source methods, and then we start new traversals from fieldpropagators. I am planning on implementing this in a future PR. WDYT?

Copy link
Contributor Author

@mlevesquedion mlevesquedion Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is what I meant in my comment in the other thread, sorry if I didn't express myself clearly.)

Also, I note that the fieldpropagator's functionality should perhaps be extended to include non-struct types, as currently it will only identify method propagators that return a field. (In a future PR.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... this still feels like a hole to me.

If we want fieldpropogator to identify certain methods as returning sources, I'm fine with that. If we want the logic here to not propagate through a value already identified as a source, I'm okay with that. But I don't like something that makes the assumption that it will be caught elsewhere. I think we've butted against this before, where we struggle with propagating taint through non-source fields of source-types.

If we're here for optimization, I think it's premature optimization. If we're skipping here because sp.Produce() should be identified as a source, we should be checking for source directly rather than implicitly.

Maybe this is all something that requires a reexamination of fieldpropagator though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't like something that makes the assumption that it will be caught elsewhere.

I agree with your sentiment. However, I think that in the current state, for consistency, we should delegate to the fieldpropagator analyzer. I see too ways that methods can propagate taint: 1. the method is called with a tainted argument (the propagation code handles this), 2. the method always returns a tainted value, regardless of its arguments (the fieldpropagator analyzer handles this). If the method always returns a tainted value, then it is a source, and will be identified as such.

If we're here for optimization ...

Optimization was definitely not my intent, and I don't think either option has a significant impact on performance.

Maybe this is all something that requires a reexamination of fieldpropagator though.

I agree.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see too [sic] ways that methods can propagate [...]

I see a third: if the value is tainted, every method should propagate taint, not just those identified by fieldpropagator. This is the case that makes me uncomfortable here.

type FooFace interface{
  Foo() interface{}
}

type Foo struct {
  Valfoo string
}

func (f Foo) Foo() interface{} {
  return f.Valfoo
}

func MakeFoo(s Source) FooFace {
  return Foo{s}
}

func ContaminateInterface(s Source) {
  foo := MakeFoo(s)  // foo should be tainted by s
  val := foo.Foo()  // that taint should propagate to val, but I believe would fail to do so here.
  core.sink(val)
}

If I recall, this is an issue with structs right now, too, is it not? If s.innocentData becomes contaminated, we don't necessarily propagate that through s.GetInnocentData()? That's what I was trying to get at when I said

I think we've butted against this before, where we struggle with propagating taint through non-source fields of source-types.

I think that in the current state, for consistency, we should delegate to the fieldpropagator analyzer

A proper delegation would work for me, but we're not calling IsFieldPropagator(call) here. (We can't without introducing a circular import, but that's wandering into weeds out of scope.) We're not delegating, we're assuming that it will have been caught elsewhere.

I guess part of the issue here is that we don't have a good definition of what it means for an interface value to be a source. In the context of a struct, we have the idea that the field Source.Data can be a source, so Source is a source type holding it. If SourceInterface is a source, is every method a propagator? Is that what you have in mind for how fieldpropagator should handle interfaces? Or if we are specifying interface-types as a source, does that mean we need to specify which specific methods are the source-y methods?

</ramble>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example you give, if FooFace is a source, then I would want the fieldpropagator analyzer to flag all of its methods as source-producing (currently, this is not the case; also, field* no longer feels like an appropriate name). I think this is the best we can do for now, for interfaces (and likely for other named types, e.g. for a named string I think it is reasonable to assume that the methods will propagate taint).

If FooFace is not a source, then currently there won't be an issue because the propagation code only skips methods on sources. I've tested it locally and it works.

We're not delegating, we're assuming that it will have been caught elsewhere.

Maybe my choice of words was poor. In any case, this is the way that things are working right now. We don't propagate when the receiver is a source, unless one of the arguments is tainted. If the receiver is a source, and the fieldpropagator analyzer determined that it propagates taint, then we start a new traversal.

If SourceInterface is a source, is every method a propagator?

Yes, that feels to me like the most pragmatic stance.

In practice, we haven't seen any interface sources yet, but we're also not preventing users from doing that, and I see how it might make sense for some users. I'm just trying to support this in a way that is consistent with what we have already, so that if we want to handle this differently later, perhaps when we have more information, it is easy to spot the relevant pieces.

fmt.Printf("call.Call.Args was empty; please report this issue")
return
}
// If the receiver's type is statically known,
// it will be the first element of the Args slice.
Copy link
Collaborator

@PurelyApplied PurelyApplied Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this imply for, e.g.,

func Foo(si SourceInterface, s Source) {
  t := si.TaintingMethod(s)
  Sink(t)
}

From the comments here, I'd think that the dynamically dispatched TaintingMethod wouldn't get si as an arg? Which means taint [edit: won't] propagate correctly in the above case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you meant, incorrectly, as that is indeed the case. Good catch, we should only skip the first argument when the receiver is statically known. I've added tests verifying this and fixed the bug.

Copy link
Contributor Author

@mlevesquedion mlevesquedion Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I note that the fieldpropagator's functionality should perhaps be extended to include non-struct types, as currently it will only identify method propagators that return a field. (In a future PR.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did drop my negative. Way to read what I mean, not what I say.

for _, a := range call.Call.Args[1:] {
if prop.tainted[a.(ssa.Node)] {
visitingFromArg = true
Expand Down
3 changes: 1 addition & 2 deletions internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
4 changes: 0 additions & 4 deletions internal/pkg/sourcetype/sourcetype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down