Strip out parameters values in SQL queries instrumentation

[romtsn]

Description

Due to PII, we have to be cautious about parameter values in SQL queries instrumentation. For some queries, the parameter values are still captured, so we'd need to investigate in which cases and how to prevent that.

[bruno-garcia]

This is worth writing down on the develop docs. As guidance for other SDK engineers

[adinauer]

Check if we send param values in any of our integrations, if so do not send them.

[lbloder]

Here are my findings on the Instrumentation we do for SQLite queries:

SQLite

FrameworkSQLiteStatement (implements SupportSQLiteStatement)

We instrument androidx.sqlite.db.framework.FrameworkSQLiteStatement methods:

• executeInsert()
  • Used for Room Annotation @Insert
• executeUpdateDelete()
  • Used for Room Annotation @Update and @Delete

We call android.database.sqlite.SQLiteStatement toString() method to set the description of the Span.

android.database.sqlite.SQLiteStatement toString() calls getSql() of its SuperClass SQLiteProgram which returns the query string before any parameter replacement happened.

FrameworkSQLiteDatabase (implements SupportSQLiteDatabase)

We instrument the following methods:

• FrameworkQLiteDatabase.query(SupportSQLiteQuery supportQuery)
  • Used for Room Annotation @Query
• FrameworkQLiteDatabase.query(String query)
• FrameworkQLiteDatabase.query(String query, Object[] bindArgs)
• FrameworkQLiteDatabase.execSQL(String sql)
• FrameworkQLiteDatabase.execSQL(String sql, Object[] bindArgs)

In short, if Placeholders are used we always send the query string containing the placeholders and thus do not leak PII relevant data.

However, no one is stopping anyone from doing something like this:

• roomDatabase.openHelper.writableDatabase.execSQL("DELETE FROM User WHERE Name = '$name'")
• roomDatabase.openHelper.readableDatabase.query("SELECT * FROM User WHERE Name = '$name'")

And thus leaking PII relevant data.

We could potentially change the instrumentation code to not instrument FrameworkQLiteDatabase.query(String query) and FrameworkQLiteDatabase.execSQL(String sql)in order to mitigate this a little.

People could still do something like this, however:

roomDatabase.openHelper.readableDatabase.query(SimpleSQLiteQuery("SELECT * FROM User WHERE Name = '$name'"))

I would suggest to update the docs to tell our users to always use placeholders, which is good practice anyways.

WDYT in regards to changing the instrumentation to ignore the two methods mentioned above?

[adinauer]

We could link something like https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html in our docs.

Hmm things to also consider:

• A developer can basically get it wrong with any of the query or execSQL methods if they simply concatenate/interpolate variables into Strings. They might do this in combination with using bindArgs
• On the other hand someone could be doing everything correctly and simply run a query without args that would not be instrumented if we skipped certain methods.

So I'm in favor of "only" trying to educate developers via docs and leaving the instrumentation as is as I assume we'd have to turn it off completely if we wanted to be sure.