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

Reconsider traversal through Allocs #212

Open
mlevesquedion opened this issue Dec 4, 2020 · 0 comments
Open

Reconsider traversal through Allocs #212

mlevesquedion opened this issue Dec 4, 2020 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@mlevesquedion
Copy link
Contributor

We currently do not traverse through Allocs, except when they are slice-like:

source.go

	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)
		}

At the time that this was implemented, avoiding Allocs was allowing us to avoid many false positives. With changes to the traversal code over time, it seems that this check may need to be revisited, as it can cause false negatives:

func TestPointerToNamedStructIsTainted(s core.Source, i core.Innocuous) {
	// This test is failing because &x introduces an Alloc,
	// and we don't traverse through non-array Allocs
	colocateInnocPtr(s, &i)
	core.Sink(i) // TODO(212) want "a source has reached a sink"
}

At the time of writing, only a single false positive is obtained:

func TestSelectSourceAndInnoc(sources <-chan core.Source, innocs <-chan core.Innocuous) {
	select {
	case s := <-sources:
		core.Sink(s) // want "a source has reached a sink"
	case i := <-innocs:
		core.Sink(i) // an undesired report is produced here
	}
}

In this case the incorrect behavior is likely not traversal through Allocs, but a mishandling of the SSA instructions associated with select statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants