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

Add failing MakeInterface test case #200

Conversation

mlevesquedion
Copy link
Contributor

This PR is a part of #188.

This PR adds a failing test showing incorrect propagation for MakeInterface instructions in some cases:

func TestTaintingInterfaceValueDoesNotTaintContainedValue(s *core.Source, str string) {
	var x interface{} = str
	colocate(s, x)
	// TODO: no report should be produced below
	core.Sink(str) // want "a source has reached a sink"
}

func colocate(s *core.Source, x interface{}) {}

The incorrect behavior here is similar to what is happening with Store instructions (see #199): we are propagating to the Value that is being made into an interface. In the above test case, that does not make sense: str cannot become tainted as a result of the interface{} value holding str being tainted.

A small tweak (changing str to be of *string type) to the above test case makes it less clear what the correct behavior would be:

func TestTaintingInterfaceValueDoesNotTaintContainedValue(s *core.Source, str *string) {
	var x interface{} = str
	colocate(s, x)
	core.Sink(str) // want "a source has reached a sink" ?
}

In this case, colocate could conceivably type assert the pointer out of the interface and change the string, so this code may not be safe. One mitigation against spurious reports like the one in this test case would therefore be to only propagate to the Value in a MakeInterface if it has a pointer-like type (like we are considering for calls in #185). We already have a case covering this behavior:

func TestTaintIsPropagatedToColocatedPointerArgumentsThroughEface(s core.Source, ip *core.Innocuous) {
	taintColocatedEface(s, ip)
	core.Sink(ip) // want "a source has reached a sink"
}

In the above case, the MakeInterface instruction is implicit. It is added by the ssa builder because taintColocatedEface takes interface{} type arguments.

  • Tests pass
  • (N/A) [ ] Running against a large codebase such as Kubernetes does not error out.
  • (N/A) [ ] Appropriate changes to README are included in PR

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.

A small tweak (changing str to be of *string type) to the above test case makes it less clear what the correct behavior would be [...]

I agree with your assessment - any pointer-like argument leaves the referenced value taintable, and so that should propagate through it.

@mlevesquedion mlevesquedion merged commit 5196b14 into google:master Dec 2, 2020
@mlevesquedion mlevesquedion deleted the add-failing-makeinterface-test-case branch December 2, 2020 22:54
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