Fix sanitization bug, add tests

[mlevesquedion]

This PR fixes a bug in how sanitization status is evaluated. It also introduces tests covering the previously buggy behavior.

This PR also adds tests documenting another, more complicated bug also involving sanitization. This bug are described in #155.

• Tests pass
• Running against a large codebase such as Kubernetes does not error out.
• Appropriate changes to README are included in PR

[mlevesquedion]

Patrick, Vinayak: I've added you as reviewers, but I'm not looking for an actual review, I would just like to get your thoughts on these test cases.

[PurelyApplied]

I do love tests. LGTM.

[mlevesquedion]

Good! I would like to add these tests, but right now they would be failing.

I have created an issue (#155), because I am not sure how to handle this right now, and they don't feel very high priority. I guess the bottom line is that it seems that the current way we check for domination is incorrect. In the cases above, the sanitizing call does dominate the sink call on the possible paths from source to sink, which are the relevant paths.

Because of the pointers, currently I don't see a way to do this other than stepping through the instructions.

[PurelyApplied]

I think it would be alright to introduced the tests suppressed with // TODO want. It's good you've opened the Issue, and could include that in the TODO for documentation.

Because of the pointers, currently I don't see a way to do this other than stepping through the instructions.

I'm not sure how you mean this. Do you mean about handling the in situ sanitization?

[mlevesquedion]

I think it would be alright to introduced the tests suppressed with // TODO want. It's good you've opened the Issue, and could include that in the TODO for documentation.

We can't use // TODO want here sadly, because we actually do not want. I could comment out the call to core.Sink, though.

I'm not sure how you mean this. Do you mean about handling the in situ sanitization?

Yeah. In the general case I don't see a clean way to say "on this path, because of the sanitization by reference, this value is safe".

[PurelyApplied]

We can't use // TODO want here sadly, because we actually do not want. I could comment out the call to core.Sink, though.

I was perhaps being glib in the particulars. I would do something like this

func TestOnlySanitizedIfLoopIsTaken() {
	var e interface{} = core.Source{}
	for false {
		e = core.Sanitize(e)[0]
	}
	// TODO want to not emit here
	core.Sink(e) // want "a source has reached a sink"
}

I might be inclined to keep a consistent pattern for searchability on TODO want, even in this "want to not emit" pattern.

Yeah. In the general case I don't see a clean way to say "on this path, because of the sanitization by reference, this value is safe".

I see a few paths forward, but no obvious best.

• Opt into some interpreter approach, which has pros and cons we've discussed elsewhere.
• Return to the old pattern of tracking in-situ sanitization in the Source struct, and check for reachability when we find a source-sink pair. We stepped away from this pattern because reachability gets very complex when the source, sink, and sanitizer are in separate blocks.
• We could adopt a block-level sanitization requirement. This might be my personal preference. We assert that a developer should either sanitize a source (a) immediately upon acquisition of the source, or (b) immediately before it hits a sink. This would undoubtedly expose some false positives in gnarlier cases and imposes greater restrictions on users. But if we know a source leaves a block sanitized, we could lean on the ssa dominance methods rather than rolling our own.

[mlevesquedion]

I like your approach with the TODO want, I think maintaining our ability to grep for TODO want makes sense.

Concerning the paths you propose, I had 1 and 2 in mind as well but I didn't consider 3. Thank you for outlining these paths. I think it's very valuable that you brought up path 3.

I think with an interpretation approach, this would be easy to do. I agree that there are pros and cons, though.

I agree with your assessment of the 2nd path.

Path 3 seems quite pragmatic to me. I'm not sure if it would be too much of an inconvenience to developers. It would certainly be easier to implement, and I don't see any obvious ways that it would fail.

I think for now I'll just rewrite the tests using TODO want comments as you proposed. I don't think we're in a huge hurry to take care of these cases.

[mlevesquedion]

I've rewritten the failing tests with TODOs. These are captured in issue #155.

[PurelyApplied]

Path 3 seems quite pragmatic to me. I'm not sure if it would be too much of an inconvenience to developers. It would certainly be easier to implement, and I don't see any obvious ways that it would fail.

(Nit: maybe edit out the # so we don't get a spurious issue cross-reference.)

The only one that comes to mind is a convoluted switch case that joins in the middle, e.g.

func Foo() {
	var obj interface{}
	if flipCoin() {
		obj = Source{}
	} else {
		obj = 10
	}
	
	genericSanitizer(obj)
	
	if flipCoin() {
		sink(obj)
	} else {
		somethingElse(obj)
	}
}

I don't predict that being too common, but still within the realm of possibility. I could see it being useful for some generic delegation logic, especially if the if cases were be replaced by potentially-long switch t := obj.(type) cases, with maybe a sink as an error report in the default: case.

I'm still comfortable with reporting in that instance, though. And you could also probably layer that the genericSanitizer call does dominate the sink call using the ssa blocks, but the fact that it might not be a source in the first place could make tracking that somewhat convoluted. But this is wandering into implementation detail weeds.

I'll add this sort of test case to #149.

[mlevesquedion]

I was expecting that test case to pass under the current implementation, so I added it to check my understanding. It failed. I think the domination logic is wrong: if the sanitizer's block dominates the sink's block (as is the case in the example you gave), and they are in different blocks, then certainly the sanitizing instruction dominates the sink instruction. I've reworked the tests for the sanitizer package to reflect this.

Please review the fix.

[PurelyApplied]

I've removed myself from the review set for now. It's not clear if you're just iterating or if you wanted review now. Feel free to add me back when this PR is ready for review.

[mlevesquedion]

SG, sorry for the confusion. I'm done iterating, I do want a review.

[PurelyApplied]

LGTM. Since we've iterated a good bit here, please update the commit summary and description to be more meaningful than "fixed bug." Maybe Prevent report in block dominated by block containing sanitization.