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.

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

Propagate through go calls more selectively. #305

Merged

Conversation

mlevesquedion
Copy link
Contributor

This PR fixes some inconsistencies in how Call and Call-like instructions (Defer, Go) are handled.

The case that handles Defer says:

	// These nodes cannot propagate taint.
	case ..., *ssa.Defer, ...:

This is unfortunately inaccurate. For the most part, a Defer behaves like a Call (the main difference being when the call occurs.)

The case that handles Go says:

	// These nodes don't have referrers; they are Instructions, not Values.
	case *ssa.Go:
		prop.taintOperands(n, maxInstrReached, lastBlockVisited)

While it is true that a Go instruction can't have referrers, the way it propagates taint through its operands should be the same as for regular calls.

These inconsistencies can cause both false negatives and false positives. See the test cases for details.

I have moved the identification of sanitizers to the Call instruction's branch, because I don't think we want to consider defer sanitize(...) and go sanitize(...) to be valid sanitizations.

  • Running against a large codebase such as Kubernetes does not error out. (See DEVELOPING.md for instructions on how to do that.)
  • (N/A) [ ] Appropriate changes to README are included in PR

Michaël Lévesque-Dion added 2 commits April 15, 2021 09:34
Copy link
Collaborator

@PurelyApplied PurelyApplied left a comment

Choose a reason for hiding this comment

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

For the most part, a Defer behaves like a Call (the main difference being when the call occurs.)

That "when" is important, though. A deferred copy doesn't execute until after the sink. A goroutine copy may or may not propagate taint before a sink, irrespective of whether the argument was tainted when the goroutine was declared.

If we wanted to do this "right," we could properly post-process all defer and go calls after the propagation has spread through the containing blocks, in order to have a most-complete picture of the possible taint for when that instruction executes. Although it's a little moot at present; since we are now explicitly enumerating which calls can propagate, all of these scenarios are a little pathological. Why would someone defer a copy? I don't see a real-world use of go or defer that directly invokes something from the standard library (other than closing a channel or canceling a context, neither of which propagate).

If the scope of this PR is to just flesh out our stance on go and defer, we could certainly flesh out testing to include some anonymous functions.

internal/pkg/propagation/propagation.go Outdated Show resolved Hide resolved
internal/pkg/propagation/propagation.go Outdated Show resolved Hide resolved
Comment on lines 25 to 29
func TestGoBuiltin(dst, src []interface{}, s core.Source) {
src[0] = s
go copy(dst, src)
core.Sink(dst) // want "a source has reached a sink"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

go is indeterminate. Please add a test swapping the first to lines of this test, as that would also potentially taint the sink.

@mlevesquedion
Copy link
Contributor Author

mlevesquedion commented Apr 16, 2021

You make some great points here, I think I took on too much in this PR. What originally prompted me to make a change was that I encountered a false positive similar to this:

func TestGoUnknownFunction(i *core.Innocuous, s core.Source) {
	go baz(i, s)
	core.Sink(i) // a report is produced here, but to be coherent with how we handle regular calls, it shouldn't be
}

This is because currently, with Go instructions we propagate taint to all reference-like operands, which is incoherent with the way calls are handled since #292.

I agree that deferring/going a builtin or standard library function is likely to be very rare. I did a quick search in kubernetes and I didn't find much. There are a few uses of go io.Copy(..., ...), and a few uses of defer klog.Infof(...). That second one is a bit concerning because currently only Call instructions can be sinks.

I think for now, I'll just change the propagation for the Go instruction to avoid the false positives and create an issue outlining future work.

@mlevesquedion mlevesquedion changed the title Uniformize propagation for call instructions. Propagate through go calls more selectively. Apr 16, 2021
Michaël Lévesque-Dion added 2 commits April 16, 2021 08:11
Specifically:
* Do not change the behavior for Defer instructions
* For Go instructions, instead of propagating to all Operands,
  use taintStdlibCall to propagate similar to regular Calls.
Copy link
Collaborator

@PurelyApplied PurelyApplied left a comment

Choose a reason for hiding this comment

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

The thinner scope looks good to me.

@mlevesquedion mlevesquedion merged commit 99e7269 into google:master Apr 22, 2021
@mlevesquedion mlevesquedion deleted the uniformize-call-propagation branch April 22, 2021 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants