How to sanitize all variables passed to the selectionArgs array?

Question

Veracode Static Scan report points SQL Injection flaw in my Content Provider implementation.

Previously, I posted this question related to all my doubts regarding this flaw.

And after few discussions I came to a conclusion that there might be chances of it being a false positive in the report. Because according to what I researched and read, I was following the security guidelines mentioned in Android docs and other referenced sources to avoid SQL Injection.

There is suggestion everywhere to perform at least some input validation on the data passed to SQL queries.I want to cover this possibility which be the reason of flaw. Everyone is asking me to sanitize data before passing to query. How do I exactly sanitize variables passed to selectionArgs array passed to delete(), update() method of Content Provider?

Will DatabaseUtils.sqlEscapeString() be sufficient? Please suggest!

Here's the implementation where I need to sanitize the variable:

 public Loader<Cursor> onCreateLoader(int id, Bundle b) {
    switch (id) {
        case THOUGHT_LOADER:
            return new CursorLoader(getActivity(), NewsFeedTable.CONTENT_URI, NewsFeedTable.PROJECTION, NewsFeedTable._id + "=?", new String[]{tid}, null);
        case COMMENT_LOADER:
            return new CursorLoader(getActivity(), CommentTable.CONTENT_URI, CommentTable.PROJECTION, CommentTable.COLUMN_TID + "=?", new String[]{tid}, null);
        default:
            return null;
    }
}

Report points to the flaw :Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWEID 89) at this line

deleted = db.delete(BulletinTable.TABLE_NAME, selection, selectionArgs); in the below code:

 @Override
public int delete(Uri uri, String selection, String[] selectionArgs) {
    if (uri.equals(Contract.BASE_CONTENT_URI)) {
        deleteDatabase();
        return 1;
    }

    SQLiteDatabase db = openHelper.getWritableDatabase();

    int deleted = 0;
    switch (matcher.match(uri)) {
        case BULLETIN:
            deleted = db.delete(BulletinTable.TABLE_NAME, selection, selectionArgs);
            break;
        case CLASSROOMS:
            deleted = db.delete(ClassroomsTable.TABLE_NAME, selection, selectionArgs);
            break;
        default:
            throw new IllegalArgumentException("Unsupported URI: " + uri);
    }

    if (deleted > 0) {
        getContext().getContentResolver().notifyChange(uri, null);
    }

    return deleted;
}

@CL , I have added the code snippet where the reports points flaw in the question. It doesn't complain about any specific value but gives only the Line numbers in the code where this flaw has been detected along with details regarding the flaw and some recommendations. It's the delete() method of Content Provider implementation. I have removed image and added code in its place. — Priya Mall, Jan 05 '17 at 16:20
What have these two pieces of code to do with each other? Where is `delete` called? And why is this not a duplicate of your earlier question? — CL., Jan 05 '17 at 16:21
Yes, these are two different pieces of code. They don't have to do anything with each other. First code snippet: _Shows the implementation how am I passing variables in selectionArgs Array._ Second code snippet: _Points where the flaw is detected._ Mark as Duplicate: Maybe this question can be considered as duplicate.But I was trying to get answer of my another question about sanitizing data.So, I thought of creating a different question. — Priya Mall, Jan 05 '17 at 16:31

score 2 · Answer 1 · answered Jan 05 '17 at 17:26

Values in the selectionArgs array never need to be sanitized, because they cannot be interpreted as SQL commands (that's the whole point of having separate parameter values).

The purpose of sqlEscapeString() is to format a string so that it can be put into an SQL command (i.e., escape single quotes; all other characters have no meaning inside an SQL string). But when you know that there is a string, you should selectionArgs instead, so this function is not helpful.

You need to sanitize only strings that end up in the SQL command itself. In this case, this would be selection. If this value comes from the user, or from some other app, then you have no control over how much stuff your DELETE statement actually does (it could call SQL functions, or execute subqueries that access other parts of the database).

For practical purposes, it is not possible to sanitize strings that are intended to contain SQL commands, because then you would need a full SQL parser. If your content provider is available for external code, you should allow deleting specific items only through the URI, and disallow custom selections.

How to sanitize all variables passed to the selectionArgs array?

1 Answers1