Explicitly handle each node type #178
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,14 +75,14 @@ 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.marked[n] { | ||
| return | ||
| } | ||
|
|
@@ -91,88 +91,156 @@ func (s *Source) dfs(n ssa.Node, maxInstrReached map[*ssa.BasicBlock]int, lastBl | |
| return | ||
| } | ||
|
|
||
| if instr, ok := n.(ssa.Instruction); ok { | ||
| instrIndex, ok := indexInBlock(instr) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
| } | ||
|
|
||
| mirCopy := map[*ssa.BasicBlock]int{} | ||
| for m, i := range maxInstrReached { | ||
| mirCopy[m] = i | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
|
||
| if mirCopy[instr.Block()] < instrIndex { | ||
| mirCopy[instr.Block()] = instrIndex | ||
| } | ||
|
|
||
| lastBlockVisited = instr.Block() | ||
| } | ||
|
|
||
| s.preOrder = append(s.preOrder, n) | ||
| s.marked[n] = true | ||
|
|
||
| s.visit(n, mirCopy, lastBlockVisited) | ||
| } | ||
|
|
||
| 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) | ||
|
|
||
| // 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. | ||
| 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. | ||
| 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.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) | ||
|
|
||
| case *ssa.Index: | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.dfs(t.X.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
|
|
||
| case *ssa.IndexAddr: | ||
| s.visitReferrers(n, maxInstrReached, lastBlockVisited) | ||
| s.dfs(t.X.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
|
|
||
| // These nodes are both Instructions and Values, and have no special restrictions. | ||
| case *ssa.Field, *ssa.FreeVar, *ssa.MakeInterface, *ssa.Select, *ssa.TypeAssert: | ||
| 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 +266,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 +401,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: | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.