[Draft] Trace taint when reporting. #281
Conversation
* Partially addresses google#155
|
While still a Draft, my key questions are these:
|
There was a problem hiding this comment.
The refactor to taintCall ...
Having this refactor in this PR is fine with me.
What is a good first step for showing the taint trace?
I think showing the positions is a good start. I don't especially like the repetition, however:
analysistest.go:333: levee_analysistest/example/tests/callorder/colocation.go:35:12: unexpected diagnostic: a source has reached a sink
source: /home/runner/work/go-flow-levee/go-flow-levee/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/callorder/colocation.go:31:2
taints: /home/runner/work/go-flow-levee/go-flow-levee/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/callorder/colocation.go:33:17
taints: /home/runner/work/go-flow-levee/go-flow-levee/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/callorder/colocation.go:32:19
taints: /home/runner/work/go-flow-levee/go-flow-levee/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/callorder/colocation.go:34:16
taints: /home/runner/work/go-flow-levee/go-flow-levee/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/callorder/colocation.go:35:13
sinks at: /home/runner/work/go-flow-levee/go-flow-levee/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/callorder/colocation.go:35:12
Since taint traces are currently confined to a single function, I think we should show the filepath a single time, and we may want to print the function's name. Source locations can then be indicated via just the line and column. (This will break your workflow, though.)
There's obviously a lot of clean-up left to do with all the TODOs and ignored return values ...
Oops. I read this before starting my review and promptly forgot about it while reviewing. Please ignore my comments if they are impertinent.
| return recvOk && tbOk && recvVal == tbVal | ||
| } | ||
|
|
||
| // getCallComponents returns the ssa.Value(s) for the call's receiver and arguments. |
There was a problem hiding this comment.
Nit: decomposeCall maybe? Feels similar to the utils functions.
| // The returned slice is tainted if either the slice argument or the values | ||
| // are tainted, so we need to visit the referrers. | ||
| prop.taintCallArg(c.Call.Args[0], taintedBy, maxInstrReached, lastBlockVisited) | ||
| // The returned slice is taintedBy if either the slice argument or the values |
There was a problem hiding this comment.
I think this should still say tainted (see also next line).
| prop.taint(s.Chan.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
| // If the sent value (Send) is taintedBy, propagate taint to the channel | ||
| case s.Dir == types.SendOnly && taintedSend: | ||
| // TODO Does the channel get tainted by the select? Does that make sense? |
There was a problem hiding this comment.
I think it would be preferable to use the value being sent, i.e. s.Send.
| return nil, call.Call.Args | ||
| } | ||
|
|
||
| func taintedByInArgs(taintedBy ssa.Node, args []ssa.Value) bool { |
There was a problem hiding this comment.
This appears to be dead.
| @@ -340,7 +391,8 @@ func (prop *Propagation) taintSelect(sel *ssa.Select, maxInstrReached map[*ssa.B | |||
| if !ok || e.Index != extractIndex[s] { | |||
| continue | |||
| } | |||
| prop.taint(e, maxInstrReached, lastBlockVisited, false) | |||
| // TODO Does the extract get tainted by the select? Does that make sense? | |||
There was a problem hiding this comment.
Here I would use the channel itself, s.Chan.
| var taintedSend, taintedChan bool | ||
| if s.Send != nil { | ||
| _, taintedSend = prop.taintedBy[s.Send.(ssa.Node)] | ||
|
|
There was a problem hiding this comment.
Extraneous blank line here.
| // If the sent value (Send) is tainted, propagate taint to the channel | ||
| case s.Dir == types.SendOnly && prop.tainted[s.Send.(ssa.Node)]: | ||
| prop.taint(s.Chan.(ssa.Node), maxInstrReached, lastBlockVisited, false) | ||
| // If the sent value (Send) is taintedBy, propagate taint to the channel |
There was a problem hiding this comment.
taintedBy should be justed tainted.
|
|
||
| func (prop *Propagation) TaintPathTo(n ssa.Node) ([]ssa.Node, bool) { | ||
| var path []ssa.Node | ||
| var hadJump bool |
There was a problem hiding this comment.
When would there be a "jump" in the path?
| // Here, we only display the position of nodes with a Pos(). | ||
| // Additionally, we filter out positions that have the same file and linenumber as the previous. | ||
| prevPos := pass.Fset.Position(source.Node.Pos()) | ||
| for i := len(path) - 2; i > 0; i-- { |
There was a problem hiding this comment.
Nit: consider not including the root in the path if we're just going to skip it here anyway.
| // Additionally, we filter out positions that have the same file and linenumber as the previous. | ||
| prevPos := pass.Fset.Position(source.Node.Pos()) | ||
| for i := len(path) - 2; i > 0; i-- { | ||
| if path[i].Pos() == 0 { |
There was a problem hiding this comment.
Nit: prefer token.NoPos instead of 0.
Themes of this PR:
tainted map[Node]booltotaintedBy map[Node]Node.taintCallso that removing taint fromCalldoes not break taint trace.Callcan break the taint chain entirely. Taint now propagates directly between arguments rather than requiring propagation through theCall.