Python: more additional taint steps#4149
Conversation
I took the bits from ql/test/library-tests/taint/ that seemed easy to port. I left out namedtuple for now, but it is part of internal tracking ticket, so won't be forgotten.
This makes it easier to add a new test-case, and makes it easier to work with the existing files. It does have a downside on making it a bit more annoying looking at TestTaint.expected, and possible longer runtime, but I think it's still worth it.
deepcopy was already handled somehow, don't really know how :D
since the files was called `collection`, that conflicted with import system :|
Apprently it just works 😕 :magic:
We got into problems since using `string.py` would shadow the string module from the standard library. By some reason I adopted a pattern of `_` as suffix, but let us just use the standard pattern of `test_` prefix like a normal testing framework like pytest does.
|
should update now that #4161 has been merged |
yoff
left a comment
There was a problem hiding this comment.
It is currently a little inconsistent if there are comments in the test code when the result is an expected failure.
| ) | ||
|
|
||
| # For Python2, need to convert to unicode for StringIO to work | ||
| tainted_filelike = StringIO(unicode(json.dumps(ts))) |
There was a problem hiding this comment.
StringIO is not modeled currently.
| def contrived_2(): | ||
| # A contrived example. Don't know why anyone would ever actually do this. | ||
|
|
||
| # We currently only handle taint nested 2 levels. |
There was a problem hiding this comment.
This test passes, should it fail by the comment? (does the code represent 2 or 3 nested levels?)
(test results didn't change) Thanks @yoff 👍
I see I didn't keep them up to date as I implemented things
I'm pushing too fast it seems
|
@yoff thanks for thorough review. I see that I forgot to update a lot of the comments for the tests based on the new implementataion 😬 but I fixed all the cases you pointed out now 👍 |
yoff
left a comment
There was a problem hiding this comment.
All changes look good, thanks for also adding the extra test 👍
tausbn
left a comment
There was a problem hiding this comment.
Generally this looks good to me, but I have a few comments about how the code is structured.
| or | ||
| // constructor call | ||
| exists(CallNode call | call = nodeTo.asCfgNode() | | ||
| call.getFunction().(NameNode).getId() in ["list", "set", "frozenset", "dict", "defaultdict", |
There was a problem hiding this comment.
I wonder if this code would be cleaner if all the call.getFunction().(NameNode).getId() = ... and call.getFunction().(AttrNode).getObject(name) stuff was factored out into helper predicates (likely_builtin and likely_method, perhaps?). As it is right now, it gets sort of overwhelming to see the same construction over and over.
There was a problem hiding this comment.
Thea idea was that all these matches on call.getFunction().(NameNode).getId() are temporary until we have a proper replacement for Value::named(). It's not something I'm too concerned about fixing up. If it's something you feel very strongly about, I can give it a try 👍
There was a problem hiding this comment.
Nah, I'm in the lower half of the sliding scale, given the context on this one. If we start adding a bunch more cases like these (pre-replacement), I definitely think we should factor it out, if nothing else to make the replacement easier to implement.
| ) | ||
| or | ||
| // list.append, set.add | ||
| exists(CallNode call, string name | |
There was a problem hiding this comment.
Since all of these disjuncts introduce a CallNode I feel a strong urge to suggest that the exists be pulled out and combined across all disjuncts...
There was a problem hiding this comment.
Personally I don't find it too problematic, so how strong is your strong urge? 😊
The taint steps defined here are fairly rough, and are only using syntactical matching. The idea is that this should be good enough as a first step, and then we can focus on more interesting things such as using TypeTracking 😉
This implementation is very generous in handing out taint. As in #4124 we don't make any attempt to limit taint propagation based on types, so since we want
<tainted dict>.get(<key>)to propagate taint, we also propagate taint for<any tainted obj>.get(). I'm not too sure whether this turns out to be a problem or not.The one thing I need want to fix is the very syntactical approach to matching library calls. This is blocked on having a proper replacement for
Value::named("json.loads").getACall()(so we can also recognizefrom json import loads; loads(tainted_string)).