-
Notifications
You must be signed in to change notification settings - Fork 2k
Swift: add String taint steps
#11185
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
Merged
karimhamdanali
merged 8 commits into
github:main
from
karimhamdanali:swift-string-taint-steps
Nov 30, 2022
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
58a8739
add taint steps for fields of String
karimhamdanali 7541b01
add test case for utf8CString
karimhamdanali 9b3c4e8
add test case for unicodeScalars
karimhamdanali f0d9dab
updated expected output for LocalTaint and Tain
karimhamdanali c0085cb
fix expected output for Taint.ql
karimhamdanali 9d17fae
fix expected output for TaintInline
karimhamdanali 9048d5d
fix expected output for LocalTaint
karimhamdanali f6bc884
update the expected output for CWE-079
karimhamdanali File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| | data.swift:85:12:85:12 | dataTainted | Fixed missing result:tainted=81 | | ||
| | data.swift:86:12:86:12 | dataTainted2 | Fixed missing result:tainted=81 | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.firstandString.last. These things can't contain a lot of data (especiallyString.isEmptywhich is aBool) 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 throughstrlen(equivalent toString.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?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:which is usually a reasonable thing to do, even if
inputStringis tainted. There were other similar patterns, and in the end we blocked taint throughstrlen. This particular example ought to be uncommon in Swift, because you don't need to explicitly allocate like this when you're using a properStringclass. I suppose we could take a wait-and-see approach on whether flow through.countis actually a problem (or a positive) on Swift?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may also be worth asking other language teams about this. Experience from C/C++ might not apply very well to this judgment call, but experience from Java/C#/JavaScript/Python and related languages might be a better indication of which direction which should go for Swift.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Java, we usually (we aren't 100% consistent) don't consider things like
lengthto be propagating taint, because there are few opportunities in a memory-safe language where a tainted integer could be a problem. Although IMHO, I think we should have those models as well, because some queries actually look for tainted integers. Other queries not interested in that (like code injection) can always add a type-based sanitizer that filters out uninteresting types.In Swift, where memory management can be a thing, I think modeling these kind of methods makes even more sense. Maybe
String.countshould be a specific sanitizer for overflow-like vulnerabilities instead of not modeling it at all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we opt for leaving
.countet al as taint propagating steps, maybe we could have a library predicate likeisConversionToSmallType(DataFlow::Node), so that queries that don't want this behavior can sanitize it out without reinventing the wheel.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we keep all those fields/members as potential sources of taint until proven otherwise. As folks have mentioned here, even integers can be sources of taint in some cases. I'm wondering though if there's a way to implement @d10c suggestion without the query writer doing too much work here.