Explicitly handle each node type #178
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,104 +75,185 @@ func New(in ssa.Node, config classifier) *Source { | |
| marked: make(map[ssa.Node]bool), | ||
| config: config, | ||
| } | ||
| s.dfs(in, map[*ssa.BasicBlock]int{}, nil, false) | ||
| return s | ||
| } | ||
|
|
||
| // dfs performs Depth-First-Search on the def-use graph of the input Source. | ||
| // While traversing the graph we also look for potential sanitizers of this Source. | ||
| // If the Source passes through a sanitizer, dfs does not continue through that Node. | ||
| func (s *Source) dfs(n ssa.Node, maxInstrReached map[*ssa.BasicBlock]int, lastBlockVisited *ssa.BasicBlock, isReferrer bool) { | ||
| if s.shouldNotVisit(n, maxInstrReached, lastBlockVisited, isReferrer) { | ||
| return | ||
| } | ||
| s.preOrder = append(s.preOrder, n) | ||
| s.marked[n] = true | ||
|
|
||
| mirCopy := map[*ssa.BasicBlock]int{} | ||
| for m, i := range maxInstrReached { | ||
| mirCopy[m] = i | ||
| } | ||
|
|
||
| if instr, ok := n.(ssa.Instruction); ok { | ||
| instrIndex, ok := indexInBlock(instr) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| if mirCopy[instr.Block()] < instrIndex { | ||
| mirCopy[instr.Block()] = instrIndex | ||
| } | ||
|
|
||
| lastBlockVisited = instr.Block() | ||
| } | ||
|
|
||
| s.visit(n, mirCopy, lastBlockVisited) | ||
| } | ||
|
|
||
| func (s *Source) shouldNotVisit(n ssa.Node, maxInstrReached map[*ssa.BasicBlock]int, lastBlockVisited *ssa.BasicBlock, isReferrer bool) bool { | ||
|
There was a problem hiding this comment. Style nit: Consider writing for the positive rather than the negative, i.e. There was a problem hiding this comment. In general I would agree, but in this case the function is only used in the negative case, so if it were named There was a problem hiding this comment. For me, it's about reading the function while you're in it. While I'm looking at this function, I don't necessarily know all of its use-cases. I also don't know that consumption won't change to invert this into a double negative. At least, that's where I'm coming from, philosophically. Then again, Naming is Hard:tm: and it's a bikeshed. It's fine as it is. |
||
| if s.marked[n] { | ||
| return true | ||
| } | ||
|
|
||
| // booleans can't meaningfully be tainted | ||
| if isBoolean(n) { | ||
|
There was a problem hiding this comment.
If we're excluding basic types, we should consider excluding all numeric and pointer types. Conversely, if memory serves, this came from the special handling surrounding There was a problem hiding this comment. This is a bit pathological, but if each
I think that could cause us to miss a lot of things. We don't know what a sink is going to do with a pointer argument, e.g. dereference it.
You are correct, but we also wanted to avoid things like There was a problem hiding this comment.
I was imprecise. I meant within the context of basic types - specifically, the
So when I said "pointers," I should have said "unsafe pointers." I feel like So of the above, I would consider taint as able to propagate to strings (typed and untyped), and optionally runes (typed and untypted) and bytes. You could make a similar pathological argument that someone could coerce a There was a problem hiding this comment. Also, this is another thing that falls into "could easily be its own PR." I was really just stirring conversation while it was front of mind, but we should shift this conversation to in Issue to not block this PR. There was a problem hiding this comment. Agreed. Issue here: #191 |
||
| return true | ||
| } | ||
|
|
||
| if instr, ok := n.(ssa.Instruction); ok { | ||
|
There was a problem hiding this comment. I'm doing a second
These should really be replaced by calls to dedicated functions. I'm happy to do it in this PR, but I could also do it in a future PR. There was a problem hiding this comment. I went ahead and extracted the first if (as well as other related instructions) into its own function. I think this reads much better. |
||
| instrIndex, ok := indexInBlock(instr) | ||
| if !ok { | ||
| return true | ||
| } | ||
|
|
||
| // If the referrer is in a different block from the one we last visited, | ||
| // and it can't be reached from the block we are visiting, then stop visiting. | ||
| if lastBlockVisited != nil && instr.Block() != lastBlockVisited && !s.canReach(lastBlockVisited, instr.Block()) { | ||
| return true | ||
| } | ||
|
|
||
| // If this call's index is lower than the highest seen so far in its block, | ||
| // then this call is "in the past". If this call is a referrer, | ||
| // then we would be propagating taint backwards in time, so stop traversing. | ||
| // (If the call is an operand, then it is being used as a value, so it does | ||
| // not matter when the call occurred.) | ||
| if _, ok := instr.(*ssa.Call); ok && instrIndex < maxInstrReached[instr.Block()] && isReferrer { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| func (s *Source) visit(n ssa.Node, maxInstrReached map[*ssa.BasicBlock]int, lastBlockVisited *ssa.BasicBlock) { | ||
| if s.node == n { | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| return | ||
| } | ||
|
|
||
| switch t := n.(type) { | ||
| case *ssa.Alloc: | ||
| // An Alloc represents the allocation of space for a variable. If a Node is an Alloc, | ||
| // and the thing being allocated is not an array, then either: | ||
| // a) it is a Source value, in which case it will get its own traversal when sourcesFromBlocks | ||
| // finds this Alloc | ||
| // b) it is not a Source value, in which case we should not visit it. | ||
| // However, if the Alloc is an array, then that means the source that we are visiting from | ||
| // is being placed into an array, slice or varags, so we do need to keep visiting. | ||
| if _, isArray := utils.Dereference(t.Type()).(*types.Array); isArray { | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| } | ||
|
|
||
| case *ssa.Call: | ||
| if callee := t.Call.StaticCallee(); callee != nil && s.config.IsSanitizer(utils.DecomposeFunction(callee)) { | ||
| s.sanitizers = append(s.sanitizers, &sanitizer.Sanitizer{Call: t}) | ||
| } | ||
|
|
||
| // This is to avoid attaching calls where the source is the receiver, ex: | ||
| // core.Sinkf("Source id: %v", wrapper.Source.GetID()) | ||
| if recv := t.Call.Signature().Recv(); recv != nil && s.config.IsSourceType(utils.DecomposeType(utils.Dereference(recv.Type()))) { | ||
| return | ||
| } | ||
|
Comment on lines
+174
to
+176
There was a problem hiding this comment. Why does this not also exclude, e.g., There was a problem hiding this comment. Good question. This is actually one of the more significant architectural flaws in the codebase right now, IMO. The reason is that As a side note, currently the propagation logic in There was a problem hiding this comment. Gotcha. And yeah, you know opinion on the pipeline of analyzers here. If these were unified in |
||
|
|
||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.visitOperands(n, maxInstrReached, lastBlockVisited) | ||
|
|
||
| case *ssa.FieldAddr: | ||
| deref := utils.Dereference(t.X.Type()) | ||
| typPath, typName := utils.DecomposeType(deref) | ||
| fieldName := utils.FieldName(t) | ||
| if !s.config.IsSourceField(typPath, typName, fieldName) { | ||
| return | ||
| } | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.visitOperands(n, maxInstrReached, lastBlockVisited) | ||
|
|
||
| // Everything but the actual integer Index should be visited. | ||
| case *ssa.Index: | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.dfs(t.X.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
|
|
||
| // The actual integer Index should not be visited. | ||
| // Everything but the actual integer Index should be visited. | ||
| case *ssa.IndexAddr: | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.dfs(t.X.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
|
|
||
| // Only the Map itself can be tainted by an Update. | ||
| // The Key can't be tainted. | ||
| // The Value can propagate taint to the Map, but not receive it. | ||
| // MapUpdate has no referrers, it is only an Instruction, not a Value. | ||
|
Comment on lines
+202
to
+205
There was a problem hiding this comment. Certainly out of scope for this PR, possibly out of scope for this analyzer, but do we need to consider where keys are tainted? E.g. func TestFoo() {
s := core.Source{Data: "password1234"}
m := map[core.Source]string{s: s.Data}
for src, str := range m {
core.Sink(src) // want "a source has reached a sink"
core.Sink(str) // want "a source has reached a sink"
}
}There was a problem hiding this comment. Hmm. With this case in mind, I think in general we may want to consider anything coming out of a tainted map to be tainted. The current tests don't do that, e.g. func TestRangeOverMapWithSourceAsKey() {
m := map[core.Source]string{core.Source{Data: "password1234"}: "don't sink me"}
for src, str := range m {
core.Sink(src) // want "a source has reached a sink"
core.Sink(str)
}
}In the above cases, the fix would be to propagate through the (For There was a problem hiding this comment. (I'm going to use sources in keys and strings in values in this comment. The same logic applies in reverse.) I think if we propagate through But the more I think about it, the more I think this might be out of scope. It feels like we're needing to decide categorically whether or not both keys and values are tainted if a map contains a source type value. Otherwise, we would have to make a distinction between the map "safely" holding its values versus having been tainted by receiving a tainted value as a key. There was a problem hiding this comment. SG. I've added it to #188. |
||
| case *ssa.MapUpdate: | ||
| s.dfs(t.Map.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
|
|
||
| // The only Operand that can be tainted by a Send is the Chan. | ||
| // The Value can propagate taint to the Chan, but not receive it. | ||
| // Send has no referrers, it is only an Instruction, not a Value. | ||
| case *ssa.Send: | ||
| s.dfs(t.Chan.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
|
|
||
| // These nodes' operands should not be visited, because they can only receive | ||
| // taint from their operands, not propagate taint to them. | ||
| case *ssa.BinOp, *ssa.ChangeInterface, *ssa.ChangeType, *ssa.Convert, *ssa.Extract, *ssa.MakeChan, *ssa.MakeMap, *ssa.MakeSlice, *ssa.Phi, *ssa.Range, *ssa.Slice, *ssa.UnOp: | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
|
|
||
| // These nodes don't have operands; they are Values, not Instructions. | ||
| case *ssa.Const, *ssa.FreeVar, *ssa.Global, *ssa.Lookup, *ssa.Parameter: | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
|
|
||
| // These nodes don't have referrers; they are Instructions, not Values. | ||
| case *ssa.Go, *ssa.Store: | ||
| s.visitOperands(n, maxInstrReached, lastBlockVisited) | ||
|
|
||
| // These nodes are both Instructions and Values, and currently have no special restrictions. | ||
| case *ssa.Field, *ssa.MakeInterface, *ssa.Select, *ssa.TypeAssert: | ||
|
There was a problem hiding this comment. Should There was a problem hiding this comment. I think almost certainly, yes. See #188. TLDR: since we don't have a good suite of tests for There was a problem hiding this comment. Related to an above comment, but I might expect the special handling of not propagating through the There was a problem hiding this comment. We could do that by checking if the Looking into this, it looks like the func TestSourcePointerAssertedFromParameterEfaceCommaOk(e interface{}) {
s, ok := e.(*core.Source)
core.Sink(s) // want "a source has reached a sink"
core.Sink(ok) <-- this is the line I added, no report should be produced here
} |
||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.visitOperands(n, maxInstrReached, lastBlockVisited) | ||
|
|
||
| // These nodes cannot propagate taint. | ||
| case *ssa.Builtin, *ssa.DebugRef, *ssa.Defer, *ssa.Function, *ssa.If, *ssa.Jump, *ssa.MakeClosure, *ssa.Next, *ssa.Panic, *ssa.Return, *ssa.RunDefers: | ||
|
|
||
| default: | ||
| fmt.Printf("unexpected node received: %T %v; please report this issue\n", n, n) | ||
| } | ||
| } | ||
|
|
||
| func (s *Source) visitReferrers(n ssa.Node, maxInstrReached map[*ssa.BasicBlock]int, lastBlockVisited *ssa.BasicBlock) { | ||
| if n.Referrers() == nil { | ||
| return | ||
| } | ||
| for _, r := range *n.Referrers() { | ||
| s.dfs(r.(ssa.Node), maxInstrReached, lastBlockVisited, true) | ||
| } | ||
| } | ||
|
|
||
| func (s *Source) visitOperands(n ssa.Node, maxInstrReached map[*ssa.BasicBlock]int, lastBlockVisited *ssa.BasicBlock) { | ||
| for _, o := range n.Operands(nil) { | ||
| if *o == nil { | ||
| continue | ||
| } | ||
| s.dfs((*o).(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
| } | ||
| } | ||
|
|
||
| func (s *Source) canReach(start *ssa.BasicBlock, dest *ssa.BasicBlock) bool { | ||
|
|
@@ -198,47 +279,6 @@ func (s *Source) canReach(start *ssa.BasicBlock, dest *ssa.BasicBlock) bool { | |
| return false | ||
| } | ||
|
|
||
| // compress removes the elements from the graph that are not required by the | ||
| // taint-propagation analysis. Concretely, only propagators, sanitizers and | ||
| // sinks should constitute the output. Since, we already know what the source | ||
|
|
@@ -374,7 +414,7 @@ func sourcesFromBlocks(fn *ssa.Function, conf classifier) []*Source { | |
| } | ||
|
|
||
| // source obtained through a field or an index operation | ||
| case *ssa.Field, *ssa.FieldAddr, *ssa.Index, *ssa.IndexAddr, *ssa.Lookup: | ||
|
|
||
| // source chan or map (arrays and slices have regular Allocs) | ||
| case *ssa.MakeMap, *ssa.MakeChan: | ||
|
|
||
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 function needed to be modified because the check for a call that is a referrer used to be done in
visitReferrers. SinceCallis the only special case, it feels cleaner to do it here. Other changes fell out of that change, because this function became unwieldy.