Python: Add example for how to write your own sanitizer

[RasmusWL]

I hope that we can gather a set of examples for how to achieve common things with the Python Library. Hopefully with little effort this can be added to our documentation at some point 😉

I think maybe this example went a little overboard, but the idea is to show how to achieve something common (in this case writing your own sanitizer), and which cases our analysis can and cannot handle. Let me know your thoughts 😊

---

This PR

I tried to improve the testing of taint even more, so now it will show if the actual result matches the expected. It only works for the binary choice of has taint vs. doesn't have taint, but so far I'm happy (again, makes it much easier to eyeball if everything is correct).

SanitizedEdges

EDIT: Looking back I have absolutely no clue what this is about 😕

I'm a bit confused by the fact that the SanitizedEdges contain

| MySanitizerHandlingNot | externally controlled string | test.py:40 | Pi(s_12) [false] |

is there some CFG-splitting going on here? How do you even see the resulting ESSA? showflow I guess? (will investigate a bit more tomorrow)

[BekaValentine]

The biggest thing I thing is needed here is a lot more written word. There's plenty of code but very little explanation. If this is to be an example for how to write a sanitizer, then a person who's never written one before will need to have a lot of framing context, lots of explanations, etc. for the overall picture of what's going on, and the reasons behind each part of the custom sanitizer.

[RasmusWL]

The biggest thing I thing is needed here is a lot more written word. There's plenty of code but very little explanation. If this is to be an example for how to write a sanitizer, then a person who's never written one before will need to have a lot of framing context, lots of explanations, etc. for the overall picture of what's going on, and the reasons behind each part of the custom sanitizer.

Agreed, that why I said Hopefully with little effort this can be added to our documentation at some point 😉

[tausbn]

I'm slightly worried about the performance of this. Apart from that, everything looks good.

[tausbn]

Argh. Github ate my comment, it seems. Welp. Let me redo all of that again.

[tausbn]

I have made (through a bunch of suggestions, which probably look completely incomprehensible) the changes I think would be necessary in order to remove the final_test argument, as I suggested in my previous review.

Note that even if you accept the suggestions, you'll probably need to autoformat the file again.

Too messy with all the separate suggestions. I think I've figured out a better way of doing it.

[tausbn]

Okay, take two, wherein I put all of the suggestions in the same comment. Much better.

[RasmusWL]

As far as I understand, after applying your suggestion, we will not change the number of tuples in the result of clears_taint_on_true. We will reduce the number of elements in the tuple (which I guess is good), and I like your suggestion better, so it's an improvement overall 👍

Anyway, to get back to the number of tuples:

For a deeply nested chain of not( not( ... not( not is_safe(s) ) ... ) ) that is used in a edge_refinement in my version we would get the result tuples:

• (CallNode for is_safe, UnaryExprNode for 'not' at depth 0, false, edge_refinement)
• (CallNode for is_safe, UnaryExprNode for 'not' at depth 1, true, edge_refinement)
• ...
• (CallNode for is_safe, UnaryExprNode for 'not' at depth n, true, edge_refinement)
• (CallNode for is_safe, CallNode for is_safe, false, edge_refinement)

In the new version we will get:

• (UnaryExprNode for 'not' at depth 0, false, edge_refinement)
• (UnaryExprNode for 'not' at depth 1, true, edge_refinement)
• ...
• (UnaryExprNode for 'not' at depth n, true, edge_refinement)
• (CallNode for is_safe, false, edge_refinement)

[tausbn]

For the case where we consider only a call to is_safe nested inside a bunch of nots, I agree that the number of tuples should be the same. More generally, I think the version with final_test could have more tuples. For instance, if we had not(not(...not(E)...)) where E is some expression involving many occurrences of is_safe, then I think you would end up with more tuples.

This is because the "magic" context test.getNode().(Expr).getASubExpression*() = final_test.getNode() captures more than just "nested sequence of nots" — getASubExpression* is too general.

[RasmusWL]

For instance, if we had not(not(...not(E)...)) where E is some expression involving many occurrences of is_safe, then I think you would end up with more tuples.

Right. So in the general case, including final_test is not a good approach. And since this example should (hopefully) be used as a recipe, we should handle the general case properly.

However, in our specific case, since we always use
nested_test = test.(UnaryExprNode).getOperand() in the recursive-case, and
test = Value::named("test.is_safe").getACall() in the base-case, we would never match on not(not(...not(E)...)) where E is some expression involving many occurrences of is_safe.

[tausbn]

However, in our specific case, since we always use
nested_test = test.(UnaryExprNode).getOperand() in the recursive-case, and
test = Value::named("test.is_safe").getACall() in the base-case, we would never match on not(not(...not(E)...)) where E is some expression involving many occurrences of is_safe.

I think this is wrong. The base case doesn't know anything about the recursive case, because the base case precedes the recursive case when we're evaluating bottom-up. The right way to approach this is to look at each disjunct in isolation. So, for just the first disjunct, the body of the predicate is as follows:

edge_refinement.getTest().getNode().(Expr).getASubExpression*() = test.getNode() and
test.getNode().(Expr).getASubExpression*() = final_test.getNode() and
final_test = test and
final_test = Value::named("test.is_safe").getACall() and
edge_refinement.getInput().getAUse() = final_test.getAnArg() and
sense = true

So what does this tell us? Let's rewrite it a bit to remove some redundancies:

test.getNode().(Expr).getASubExpression*() = final_test.getNode()

becomes (since final_test = test):

final_test.getNode().(Expr).getASubExpression*() = final_test.getNode()

But this we would expect to hold for any final_test that happens to be an Expr (and that's all of them as far as we're concerned), so we can just elide this.
So we have (after a bit of reordering)

edge_refinement.getTest().getNode().(Expr).getASubExpression*() = final_test.getNode() and
edge_refinement.getInput().getAUse() = final_test.getAnArg() and
final_test = Value::named("test.is_safe").getACall() and
final_test = test and
sense = true

We can also (basically) ignore the last two lines, as these only serve to fix some of the parts of the tuples we're producing, and cannot result in further tuples themselves. So the final form is

edge_refinement.getTest().getNode().(Expr).getASubExpression*() = final_test.getNode() and
edge_refinement.getInput().getAUse() = final_test.getAnArg() and
final_test = Value::named("test.is_safe").getACall() and

Because this is (essentially) one of the disjuncts of the original predicate, whatever tuples the above produces will be part of the final predicate. And there's nothing that limits final_test in any way other than requiring it to be inside the Expr corresponding to the edge_refinement.

(In fact, this now makes me think that your version had a bug in it — it would accept, say, not(not(is_safe(x) or True)) as sanitizing x, even though it doesn't matter what is_safe(x) returns in this case. Though this might also have been caught by pruning.)

[RasmusWL]

Thanks for the detailed explanation. Glad to slightly improve my intuitive understanding of QL evaluation a bit more.

I need to confess that you had me at

I think this is wrong. The base case doesn't know anything about the recursive case, because the base case precedes the recursive case when we're evaluating bottom-up.

but the detailed explanation was very thoughtful ❤️

My version had a bug? 🐛

(In fact, this now makes me think that your version had a bug in it — it would accept, say, not(not(is_safe(x) or True)) as sanitizing x, even though it doesn't matter what is_safe(x) returns in this case. Though this might also have been caught by pruning.)

I don't think so. The original clears_taint_on_true would hold when using
not(not(is_safe(x) or True)) for test, giving us is_safe(x) or True as final_test. However, sanitizingEdge required that final_test must be a CallNode, so sanitizingEdge wouldn't hold.

In the fixed version, clears_taint_on_true would not hold when using not(not(is_safe(x) or True)) for test, since final_test is required to be a CallNode (and it is is_safe(x) or True).