App uses SQLite database and executes raw SQL query

[bkimondiu]

App uses SQLite Database and execute raw SQL query. Untrusted user input in raw SQL queries can cause SQL Injection. Also sensitive information should be encrypted and written to the database.

Standards

CWE: CWE-89: Improper Neutralization of Special
Elements used in an SQL Command ('SQL Injection') 
OWASP Top 10: M7: Client Code Quality

Files com/onaio/steps/helper/DatabaseHelper.java

[owais-vd]

I highly recommend, to migrating the raw SQLite database into the Android Room persistence library. Room provides an abstraction layer over SQLite to allow fluent database access while harnessing the full power of SQLite. Here is some reason.

SQLite	Room
There is no compile-time verification of raw SQLite queries	But in Room, there is SQL validation at compile time
As the schema changes, you need to update the affected SQL queries manually	Room solves this problem and provide smooth migration
Need to use lots of boilerplate code to convert between SQL queries and Java data objects	Room maps our database objects to Java Object without boilerplate code
SQLite does not build to work with LiveData or RxJava	Room is built to work with LiveData and RxJava for data observation

• Why Room https://developer.android.com/training/data-storage/sqlite
• Migration guide to Room https://developer.android.com/training/data-storage/room/sqlite-room-migration

cc: @bkimondiu @ukanga @ekigamba

[ukanga]

What's the LOE on this switch? Is there a migration plan that should be there in the event we have initial data collection with the current SQLite DB?

[ukanga]

Could we simple handle this issue by using the bindArgs approach ?

public void execSQL (String sql, Object[] bindArgs)

I think if we switch from the string concatenation we are currently doing to this approach will make SQLite queries not susceptible to SQL injection.

From:

    private void purgeTables(SQLiteDatabase sqLiteDatabase) {
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS "+Household.TABLE_NAME);
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS "+Member.TABLE_NAME);
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS "+ ReElectReason.TABLE_NAME);
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS "+ Participant.TABLE_NAME);
    }

To:

    private void purgeTables(SQLiteDatabase sqLiteDatabase) {
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS  %s", [Household.TABLE_NAME]);
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS  %s", [Member.TABLE_NAME]);
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS  %s", [ReElectReason.TABLE_NAME]);
        sqLiteDatabase.execSQL("DROP TABLE IF EXISTS  %s", [Participant.TABLE_NAME]);
    }

[owais-vd]

We don't have any security implications on the current SQLite implementation in code. We are using rawQuery and exceSQL methods for static and internal managed queries and both methods are not affected by user input i.e no user interaction is required to manage the query.

User input is required only when we insert and update the data but both queries are managed by SQLite API provided methods called db.insert() and db.update().

Evidence for Non-Interaction in rawQuery and execSQL

execSQL

https://github.com/onaio/steps-app/blob/e425d03119ac48df7dd00fdbb743760e8ba8d110/src/main/java/com/onaio/steps/helper/DatabaseHelper.java#L63-L75

https://github.com/onaio/steps-app/blob/e425d03119ac48df7dd00fdbb743760e8ba8d110/src/main/java/com/onaio/steps/helper/DatabaseHelper.java#L110-L114

rawQuery

have many references of rawQuery but pasting only final one.

https://github.com/onaio/steps-app/blob/e425d03119ac48df7dd00fdbb743760e8ba8d110/src/main/java/com/onaio/steps/helper/DatabaseHelper.java#L100-L103

Evidence for User-Interaction in insert and update

insert

https://github.com/onaio/steps-app/blob/e425d03119ac48df7dd00fdbb743760e8ba8d110/src/main/java/com/onaio/steps/helper/DatabaseHelper.java#L86-L91

update

https://github.com/onaio/steps-app/blob/e425d03119ac48df7dd00fdbb743760e8ba8d110/src/main/java/com/onaio/steps/helper/DatabaseHelper.java#L93-L98

[ekigamba]

Thanks @owais-vd for the detailed justification

[ukanga]

We are trusting that someone is not going to hijack the variable, Household.TABLE_NAME. in for example the code below:

sqLiteDatabase.execSQL("DROP TABLE IF EXISTS "+Household.TABLE_NAME); 

This can be addressed by changing the above to what we have below. Is this going to fail? The tool is interpreting the concatenation with the + as whatever variable we are passing might involve a user's input. Having the code as below silences that security warning.

sqLiteDatabase.execSQL("DROP TABLE IF EXISTS ? ", [Household.TABLE_NAME]);

[ukanga]

I have switched to a ? instead of %s which might have been something I picked up from Python.

I think not making the change means we are choosing to ignore the warning.

[ekigamba]

Cool, I am going to approving this PR in that case https://github.com/onaio/steps-app/pull/208/files