-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
Allow interfaces to be source types #264
Conversation
// If the receiver's type is statically known, | ||
// it will be the first element of the Args slice. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
// 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good conversation. Please open an Issue for some of the interface conversation, either for further examination or documenting how we're handling those sources identified by interface-type.
Good conversation indeed! I have opened #272 with what I think we should do, + a few more general thoughts. PTAL. |
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 theArgs
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:
Right now, a user could configure such an interface to be a source:
But this would silently be blocked by the restriction in
IsSourceType
(removed by this PR).(I also ran this change against the codebase where the panic fixed by #261/#262 was discovered.)