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

Handle slice instructions #236

Merged

Conversation

mlevesquedion
Copy link
Contributor

@mlevesquedion mlevesquedion commented Dec 18, 2020

This PR is a part of #188 and constitutes a deep investigation into the ssa.Slice instruction.

Currently, the way we handle Slice instructions is that we visit every referrer and operand.

This can create false positives:

func TestSliceBoundariesAreNotTainted(lo, hi, max int) {
	sources := [1]core.Source{{Data: "secret"}}
	slice := sources[lo:hi:max]
	core.Sink(lo)      // a false positive report is produced here
	core.Sink(hi)      // a false positive report is produced here
	core.Sink(max)  // a false positive report is produced here
	_ = slice
}

One could argue that in the above case, the incorrect behavior is actually that we are tainting integers. I encourage you to discuss this issue here: #191. In any case, within the context of a Slice instruction, it is incorrect to visit these Operands.

I originally thought that the only thing that should legitimately be visited is the Referrers. However, consider this case:

func TestSlicedArrayIsTainted() {
	innocs := [1]interface{}{nil}
	slice := innocs[:]
	slice[0] = core.Source{Data: "secret"}
	core.Sink(innocs) // want "a source has reached a sink"
	_ = slice
}

In this case, the backing array of slice is written to when doing slice[0] = core.Source{Data: "secret"}, so to properly model the taint propagation, we need to visit the Slice's X field (one of its Operands).

  • 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

Comment on lines 204 to 205
// This allows taint to propagate backwards into the sliced value
// when the resulting slice is tainted
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: the underlying collection is not necessarily a Slice. Consider a more generic phrasing here.

@mlevesquedion mlevesquedion merged commit a473ace into google:master Dec 22, 2020
@mlevesquedion mlevesquedion deleted the only-propagate-to-slice-referrers branch December 22, 2020 21:44
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