Swift: add String taint steps
#11185
Conversation
if a String is tainted, then all its fields (including those declared in extensions) should be tainted as well
| this.getField().getEnclosingDecl().(ExtensionDecl).getExtendedTypeDecl().getFullName() = | ||
| "String" | ||
| } | ||
| } |
There was a problem hiding this comment.
This appears to be a sensible approach, but I will note a few rough edges in the case of String: String.count, String.isEmpty, String.first and String.last. These things can't contain a lot of data (especially String.isEmpty which is a Bool) so it would be very difficult for an attacker to utilize a theoretical taint path through any of them. In CPP in particular we've found taint flow through strlen (equivalent to String.count) usually produces many more false positive results than useful ones.
I'm not sure what the right solution should be. We could try to exclude the ones we don't want from this TaintInheritingContent, or switch to an explicit modelling approach. We could potentially block all taint through 'small' types everywhere, solving the problem for good (but would this limit some uses of taint?). Or we could stick to our principles that these are taint flows and expect queries to sanitize on an ad-hoc basis if they tend to find false positive results?
There was a problem hiding this comment.
On the one hand, taint flow through small types is irrelevant for most "code injection" type queries. On the other hand, there was Heartbleed.
There was a problem hiding this comment.
Good point, there have been plenty of real bugs related to tainted integers (malicious image width/height bugs spring to mind as a second example) - and as you suggest our recent focus on code injection queries may be biasing my viewpoint.
I remain somewhat nervous of taint flow through strlen / .count. In CPP we've seen a lot of code like this:
char *buffer = (char *)malloc(sizeof(char) * strlen(inputString));
which is usually a reasonable thing to do, even if inputString is tainted. There were other similar patterns, and in the end we blocked taint through strlen. This particular example ought to be uncommon in Swift, because you don't need to explicitly allocate like this when you're using a proper String class. I suppose we could take a wait-and-see approach on whether flow through .count is actually a problem (or a positive) on Swift?
If a
Stringis tainted, then all its fields should be tainted including computed properties such asutf8. This support follows a similar solution to what we do with URLs usingTaintInheritingContent.