From 234de02b594a6f98c7ba92760533d3c6a2592c01 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Wed, 25 Jul 2018 14:52:14 -0600 Subject: Extend SQLiteQueryBuilder for update and delete. Developers often accept selection clauses from untrusted code, and SQLiteQueryBuilder already supports a "strict" mode to help catch SQL injection attacks. This change extends the builder to support update() and delete() calls, so that we can help secure those selection clauses too. Bug: 111085900 Test: atest packages/providers/DownloadProvider/tests/ Test: atest cts/tests/app/src/android/app/cts/DownloadManagerTest.java Test: atest cts/tests/tests/database/src/android/database/sqlite/cts/SQLiteQueryBuilderTest.java Change-Id: Ibc8ce8b1801e7bfd946655fcf8b6865b6d71227c Merged-In: Ib4fc8400f184755ee7e971ab5f2095186341730c --- .../database/sqlite/SQLiteQueryBuilder.java | 271 ++++++++++++++++++--- 1 file changed, 234 insertions(+), 37 deletions(-) (limited to 'core/java') diff --git a/core/java/android/database/sqlite/SQLiteQueryBuilder.java b/core/java/android/database/sqlite/SQLiteQueryBuilder.java index dba3df46102c..6f907112ba6f 100644 --- a/core/java/android/database/sqlite/SQLiteQueryBuilder.java +++ b/core/java/android/database/sqlite/SQLiteQueryBuilder.java @@ -17,17 +17,26 @@ package android.database.sqlite; import android.annotation.UnsupportedAppUsage; +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.content.ContentValues; import android.database.Cursor; import android.database.DatabaseUtils; +import android.os.Build; import android.os.CancellationSignal; import android.os.OperationCanceledException; import android.provider.BaseColumns; import android.text.TextUtils; +import android.util.ArrayMap; import android.util.Log; +import com.android.internal.util.ArrayUtils; + +import java.util.Arrays; import java.util.Iterator; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.Set; import java.util.regex.Pattern; @@ -35,7 +44,8 @@ import java.util.regex.Pattern; * This is a convenience class that helps build SQL queries to be sent to * {@link SQLiteDatabase} objects. */ -public class SQLiteQueryBuilder { +public class SQLiteQueryBuilder +{ private static final String TAG = "SQLiteQueryBuilder"; private static final Pattern sLimitPattern = Pattern.compile("\\s*\\d+\\s*(,\\s*\\d+\\s*)?"); @@ -94,13 +104,10 @@ public class SQLiteQueryBuilder { * * @param inWhere the chunk of text to append to the WHERE clause. */ - public void appendWhere(@NonNull CharSequence inWhere) { + public void appendWhere(CharSequence inWhere) { if (mWhereClause == null) { mWhereClause = new StringBuilder(inWhere.length() + 16); } - if (mWhereClause.length() == 0) { - mWhereClause.append('('); - } mWhereClause.append(inWhere); } @@ -114,13 +121,10 @@ public class SQLiteQueryBuilder { * @param inWhere the chunk of text to append to the WHERE clause. it will be escaped * to avoid SQL injection attacks */ - public void appendWhereEscapeString(@NonNull String inWhere) { + public void appendWhereEscapeString(String inWhere) { if (mWhereClause == null) { mWhereClause = new StringBuilder(inWhere.length() + 16); } - if (mWhereClause.length() == 0) { - mWhereClause.append('('); - } DatabaseUtils.appendEscapedSQLString(mWhereClause, inWhere); } @@ -400,6 +404,11 @@ public class SQLiteQueryBuilder { return null; } + final String sql; + final String unwrappedSql = buildQuery( + projectionIn, selection, groupBy, having, + sortOrder, limit); + if (mStrict && selection != null && selection.length() > 0) { // Validate the user-supplied selection to detect syntactic anomalies // in the selection string that could indicate a SQL injection attempt. @@ -408,24 +417,161 @@ public class SQLiteQueryBuilder { // originally specified. An attacker cannot create an expression that // would escape the SQL expression while maintaining balanced parentheses // in both the wrapped and original forms. - String sqlForValidation = buildQuery(projectionIn, "(" + selection + ")", groupBy, + + // NOTE: The ordering of the below operations is important; we must + // execute the wrapped query to ensure the untrusted clause has been + // fully isolated. + + // Validate the unwrapped query + db.validateSql(unwrappedSql, cancellationSignal); // will throw if query is invalid + + // Execute wrapped query for extra protection + final String wrappedSql = buildQuery(projectionIn, wrap(selection), groupBy, having, sortOrder, limit); - db.validateSql(sqlForValidation, cancellationSignal); // will throw if query is invalid + sql = wrappedSql; + } else { + // Execute unwrapped query + sql = unwrappedSql; } - String sql = buildQuery( - projectionIn, selection, groupBy, having, - sortOrder, limit); - + final String[] sqlArgs = selectionArgs; if (Log.isLoggable(TAG, Log.DEBUG)) { - Log.d(TAG, "Performing query: " + sql); + if (Build.IS_DEBUGGABLE) { + Log.d(TAG, sql + " with args " + Arrays.toString(sqlArgs)); + } else { + Log.d(TAG, sql); + } } return db.rawQueryWithFactory( - mFactory, sql, selectionArgs, + mFactory, sql, sqlArgs, SQLiteDatabase.findEditTable(mTables), cancellationSignal); // will throw if query is invalid } + /** + * Perform an update by combining all current settings and the + * information passed into this method. + * + * @param db the database to update on + * @param selection A filter declaring which rows to return, + * formatted as an SQL WHERE clause (excluding the WHERE + * itself). Passing null will return all rows for the given URL. + * @param selectionArgs You may include ?s in selection, which + * will be replaced by the values from selectionArgs, in order + * that they appear in the selection. The values will be bound + * as Strings. + * @return the number of rows updated + * @hide + */ + public int update(@NonNull SQLiteDatabase db, @NonNull ContentValues values, + @Nullable String selection, @Nullable String[] selectionArgs) { + Objects.requireNonNull(mTables, "No tables defined"); + Objects.requireNonNull(db, "No database defined"); + Objects.requireNonNull(values, "No values defined"); + + final String sql; + final String unwrappedSql = buildUpdate(values, selection); + + if (mStrict) { + // Validate the user-supplied selection to detect syntactic anomalies + // in the selection string that could indicate a SQL injection attempt. + // The idea is to ensure that the selection clause is a valid SQL expression + // by compiling it twice: once wrapped in parentheses and once as + // originally specified. An attacker cannot create an expression that + // would escape the SQL expression while maintaining balanced parentheses + // in both the wrapped and original forms. + + // NOTE: The ordering of the below operations is important; we must + // execute the wrapped query to ensure the untrusted clause has been + // fully isolated. + + // Validate the unwrapped query + db.validateSql(unwrappedSql, null); // will throw if query is invalid + + // Execute wrapped query for extra protection + final String wrappedSql = buildUpdate(values, wrap(selection)); + sql = wrappedSql; + } else { + // Execute unwrapped query + sql = unwrappedSql; + } + + final ArrayMap rawValues = values.getValues(); + final String[] updateArgs = new String[rawValues.size()]; + for (int i = 0; i < updateArgs.length; i++) { + final Object arg = rawValues.valueAt(i); + updateArgs[i] = (arg != null) ? arg.toString() : null; + } + + final String[] sqlArgs = ArrayUtils.concat(String.class, updateArgs, selectionArgs); + if (Log.isLoggable(TAG, Log.DEBUG)) { + if (Build.IS_DEBUGGABLE) { + Log.d(TAG, sql + " with args " + Arrays.toString(sqlArgs)); + } else { + Log.d(TAG, sql); + } + } + return db.executeSql(sql, sqlArgs); + } + + /** + * Perform a delete by combining all current settings and the + * information passed into this method. + * + * @param db the database to delete on + * @param selection A filter declaring which rows to return, + * formatted as an SQL WHERE clause (excluding the WHERE + * itself). Passing null will return all rows for the given URL. + * @param selectionArgs You may include ?s in selection, which + * will be replaced by the values from selectionArgs, in order + * that they appear in the selection. The values will be bound + * as Strings. + * @return the number of rows deleted + * @hide + */ + public int delete(@NonNull SQLiteDatabase db, @Nullable String selection, + @Nullable String[] selectionArgs) { + Objects.requireNonNull(mTables, "No tables defined"); + Objects.requireNonNull(db, "No database defined"); + + final String sql; + final String unwrappedSql = buildDelete(selection); + + if (mStrict) { + // Validate the user-supplied selection to detect syntactic anomalies + // in the selection string that could indicate a SQL injection attempt. + // The idea is to ensure that the selection clause is a valid SQL expression + // by compiling it twice: once wrapped in parentheses and once as + // originally specified. An attacker cannot create an expression that + // would escape the SQL expression while maintaining balanced parentheses + // in both the wrapped and original forms. + + // NOTE: The ordering of the below operations is important; we must + // execute the wrapped query to ensure the untrusted clause has been + // fully isolated. + + // Validate the unwrapped query + db.validateSql(unwrappedSql, null); // will throw if query is invalid + + // Execute wrapped query for extra protection + final String wrappedSql = buildDelete(wrap(selection)); + sql = wrappedSql; + } else { + // Execute unwrapped query + sql = unwrappedSql; + } + + final String[] sqlArgs = selectionArgs; + if (Log.isLoggable(TAG, Log.DEBUG)) { + if (Build.IS_DEBUGGABLE) { + Log.d(TAG, sql + " with args " + Arrays.toString(sqlArgs)); + } else { + Log.d(TAG, sql); + } + } + return db.executeSql(sql, sqlArgs); + } + /** * Construct a SELECT statement suitable for use in a group of * SELECT statements that will be joined through UNION operators @@ -458,28 +604,10 @@ public class SQLiteQueryBuilder { String[] projectionIn, String selection, String groupBy, String having, String sortOrder, String limit) { String[] projection = computeProjection(projectionIn); - - StringBuilder where = new StringBuilder(); - boolean hasBaseWhereClause = mWhereClause != null && mWhereClause.length() > 0; - - if (hasBaseWhereClause) { - where.append(mWhereClause.toString()); - where.append(')'); - } - - // Tack on the user's selection, if present. - if (selection != null && selection.length() > 0) { - if (hasBaseWhereClause) { - where.append(" AND "); - } - - where.append('('); - where.append(selection); - where.append(')'); - } + String where = computeWhere(selection); return buildQueryString( - mDistinct, mTables, projection, where.toString(), + mDistinct, mTables, projection, where, groupBy, having, sortOrder, limit); } @@ -496,6 +624,42 @@ public class SQLiteQueryBuilder { return buildQuery(projectionIn, selection, groupBy, having, sortOrder, limit); } + /** {@hide} */ + public String buildUpdate(ContentValues values, String selection) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("Empty values"); + } + + StringBuilder sql = new StringBuilder(120); + sql.append("UPDATE "); + sql.append(mTables); + sql.append(" SET "); + + final ArrayMap rawValues = values.getValues(); + for (int i = 0; i < rawValues.size(); i++) { + if (i > 0) { + sql.append(','); + } + sql.append(rawValues.keyAt(i)); + sql.append("=?"); + } + + final String where = computeWhere(selection); + appendClause(sql, " WHERE ", where); + return sql.toString(); + } + + /** {@hide} */ + public String buildDelete(String selection) { + StringBuilder sql = new StringBuilder(120); + sql.append("DELETE FROM "); + sql.append(mTables); + + final String where = computeWhere(selection); + appendClause(sql, " WHERE ", where); + return sql.toString(); + } + /** * Construct a SELECT statement suitable for use in a group of * SELECT statements that will be joined through UNION operators @@ -670,4 +834,37 @@ public class SQLiteQueryBuilder { } return null; } + + private @Nullable String computeWhere(@Nullable String selection) { + final boolean hasInternal = !TextUtils.isEmpty(mWhereClause); + final boolean hasExternal = !TextUtils.isEmpty(selection); + + if (hasInternal || hasExternal) { + final StringBuilder where = new StringBuilder(); + if (hasInternal) { + where.append('(').append(mWhereClause).append(')'); + } + if (hasInternal && hasExternal) { + where.append(" AND "); + } + if (hasExternal) { + where.append('(').append(selection).append(')'); + } + return where.toString(); + } else { + return null; + } + } + + /** + * Wrap given argument in parenthesis, unless it's {@code null} or + * {@code ()}, in which case return it verbatim. + */ + private @Nullable String wrap(@Nullable String arg) { + if (TextUtils.isEmpty(arg)) { + return arg; + } else { + return "(" + arg + ")"; + } + } } -- cgit v1.2.3 From 0f73219cfadcbe9916b05603f238120f8b56f600 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 26 Jul 2018 09:39:18 -0600 Subject: Bind update() args as Object[] for performance. It's wasteful to convert them to String when SQLite already knows how to bind specific data types, including funky types like byte[]. Also promote to public API, since they're generally useful. Bug: 111085900 Test: atest packages/providers/DownloadProvider/tests/ Test: atest cts/tests/app/src/android/app/cts/DownloadManagerTest.java Test: atest cts/tests/tests/database/src/android/database/sqlite/cts/SQLiteQueryBuilderTest.java Merged-In: I5b418bca1204773fd2795156a2f47906ca1e1a6b Change-Id: I386cbc97cf3499823bc10251ff315ce381db24cc --- .../android/database/sqlite/SQLiteQueryBuilder.java | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) (limited to 'core/java') diff --git a/core/java/android/database/sqlite/SQLiteQueryBuilder.java b/core/java/android/database/sqlite/SQLiteQueryBuilder.java index 6f907112ba6f..4f24b958b954 100644 --- a/core/java/android/database/sqlite/SQLiteQueryBuilder.java +++ b/core/java/android/database/sqlite/SQLiteQueryBuilder.java @@ -30,7 +30,7 @@ import android.text.TextUtils; import android.util.ArrayMap; import android.util.Log; -import com.android.internal.util.ArrayUtils; +import libcore.util.EmptyArray; import java.util.Arrays; import java.util.Iterator; @@ -461,7 +461,6 @@ public class SQLiteQueryBuilder * that they appear in the selection. The values will be bound * as Strings. * @return the number of rows updated - * @hide */ public int update(@NonNull SQLiteDatabase db, @NonNull ContentValues values, @Nullable String selection, @Nullable String[] selectionArgs) { @@ -496,14 +495,19 @@ public class SQLiteQueryBuilder sql = unwrappedSql; } + if (selectionArgs == null) { + selectionArgs = EmptyArray.STRING; + } final ArrayMap rawValues = values.getValues(); - final String[] updateArgs = new String[rawValues.size()]; - for (int i = 0; i < updateArgs.length; i++) { - final Object arg = rawValues.valueAt(i); - updateArgs[i] = (arg != null) ? arg.toString() : null; + final int valuesLength = rawValues.size(); + final Object[] sqlArgs = new Object[valuesLength + selectionArgs.length]; + for (int i = 0; i < sqlArgs.length; i++) { + if (i < valuesLength) { + sqlArgs[i] = rawValues.valueAt(i); + } else { + sqlArgs[i] = selectionArgs[i - valuesLength]; + } } - - final String[] sqlArgs = ArrayUtils.concat(String.class, updateArgs, selectionArgs); if (Log.isLoggable(TAG, Log.DEBUG)) { if (Build.IS_DEBUGGABLE) { Log.d(TAG, sql + " with args " + Arrays.toString(sqlArgs)); @@ -527,7 +531,6 @@ public class SQLiteQueryBuilder * that they appear in the selection. The values will be bound * as Strings. * @return the number of rows deleted - * @hide */ public int delete(@NonNull SQLiteDatabase db, @Nullable String selection, @Nullable String[] selectionArgs) { -- cgit v1.2.3