Java: add Android database taint and SQL injection sinks #4336
Conversation
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
Comment on lines
48
to
57
| // query(boolean distinct, String table, String[] columns, String selection, String[] selectionArgs, String groupBy, String having, String orderBy, String limit) | ||
| // query(boolean distinct, String table, String[] columns, String selection, String[] selectionArgs, String groupBy, String having, String orderBy, String limit, CancellationSignal cancellationSignal) | ||
| // query(String table, String[] columns, String selection, String[] selectionArgs, String groupBy, String having, String orderBy, String limit) | ||
| // query(String table, String[] columns, String selection, String[] selectionArgs, String groupBy, String having, String orderBy) | ||
| this.getName() = "query" and | ||
| (if this.getParameter(0).getType() instanceof TypeString then result = 2 else result = 3) | ||
| ( | ||
| if this.getParameter(0).getType() instanceof TypeString | ||
| then result = [2, 4, 5, 6, 7] | ||
| else result = [3, 5, 6, 7, 8] | ||
| ) |
Contributor
There was a problem hiding this comment.
Taking a quick look at the android SQLite surce code, it looks like all of these arguments besides selectionArgs are potential query injection sinks, rather than just the ones listed. Though maybe it's so rare for the tables/columns to be tainted that it isn't worth the effort.
Comment on lines
165
to
175
| private class QueryBuilderQueryMethod extends SQLiteRunner { | ||
| QueryBuilderQueryMethod() { | ||
| // query(SQLiteDatabase db, String[] projectionIn, String selection, String[] selectionArgs, String groupBy, String having, String sortOrder) | ||
| // query(SQLiteDatabase db, String[] projectionIn, String selection, String[] selectionArgs, String groupBy, String having, String sortOrder, String limit) | ||
| // query(SQLiteDatabase db, String[] projectionIn, String selection, String[] selectionArgs, String groupBy, String having, String sortOrder, String limit, CancellationSignal cancellationSignal) | ||
| this.getDeclaringType().getASourceSupertype*() instanceof TypeSQLiteQueryBuilder and | ||
| this.hasName("query") | ||
| } | ||
|
|
||
| override int sqlIndex() { result = [-1, 3, 5, 6, 7, 8] } | ||
| } |
Contributor
There was a problem hiding this comment.
These indicies look off by one - the selection arg is at index 2
aschackmull
previously approved these changes
Oct 6, 2020
aibaars
dismissed stale reviews from aschackmull and joefarebrother
via
8971092
aschackmull
approved these changes
Oct 6, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This pull request adds taint steps and SQL injection sinks for the following classes of the Android database library:
DatabaseUtils,SQLiteDatabase,SQLiteQueryBuilder, and part ofContentProvider/Resolver.The SQLiteQueryBuilder class mostly performs unverified string concatenation for any parts of the SQL query string. This can be mitigated by setting its strictness flags which harden the query builder against SQL injection attempts. This pull request does not model the strictness flags and can therefore lead to false positives. We can improve that in the future if the false positive rate is deemed too high.