FileContentProvider: prevent injection via ContentValues arguments

For this, verify all column names for ContentValues keys. Values are safe by default.

Co-authored-by: Tobias Kaminsky <tobias.kaminsky@nextcloud.com>
Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>

src/androidTest/java/com/owncloud/android/providers/FileContentProviderIT.kt

@@ -0,0 +1,101 @@
+/*
+ *
+ * Nextcloud Android client application
+ *
+ * @author Tobias Kaminsky
+ * Copyright (C) 2021 Tobias Kaminsky
+ * Copyright (C) 2021 Nextcloud GmbH
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+package com.owncloud.android.providers
+
+import java.lang.IllegalArgumentException
+import com.owncloud.android.db.ProviderMeta
+import android.content.ContentValues
+import com.owncloud.android.utils.MimeTypeUtil
+import org.junit.Test
+
+@Suppress("FunctionNaming")
+class FileContentProviderIT {
+
+    companion object {
+        private const val INVALID_COLUMN = "Invalid column"
+        private const val FILE_LENGTH = 120
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun verifyColumnName_Exception() {
+        val sut = FileContentProvider()
+        sut.verifyColumnName(INVALID_COLUMN)
+    }
+
+    @Test
+    fun verifyColumnName_OK() {
+        val sut = FileContentProvider()
+        sut.verifyColumnName(ProviderMeta.ProviderTableMeta.FILE_NAME)
+    }
+
+    @Test
+    fun verifyColumn_ContentValues_OK() {
+        val sut = FileContentProvider()
+        // with valid columns
+        val contentValues = ContentValues()
+        contentValues.put(ProviderMeta.ProviderTableMeta.FILE_CONTENT_LENGTH, FILE_LENGTH)
+        contentValues.put(ProviderMeta.ProviderTableMeta.FILE_CONTENT_TYPE, MimeTypeUtil.MIMETYPE_TEXT_MARKDOWN)
+        sut.verifyColumns(contentValues)
+
+        // empty
+        sut.verifyColumns(ContentValues())
+    }
+
+    @Test(expected = IllegalArgumentException::class)
+    fun verifyColumn_ContentValues_Exception() {
+        val sut = FileContentProvider()
+        // with valid columns
+        val contentValues = ContentValues()
+        contentValues.put(INVALID_COLUMN, FILE_LENGTH)
+        contentValues.put(ProviderMeta.ProviderTableMeta.FILE_CONTENT_TYPE, MimeTypeUtil.MIMETYPE_TEXT_MARKDOWN)
+        sut.verifyColumns(contentValues)
+
+        // empty
+        sut.verifyColumns(ContentValues())
+    }
+
+    // @Test
+    // fun verifyColumn_Uri() {
+    //     val sut = FileContentProvider()
+    //     sut.verifyColumnSql("path = ? AND file_owner=?")
+    // }
+    //
+    // @Test
+    // fun verifyColumn_StringArray() {
+    //     val sut = FileContentProvider()
+    //     val nullArray: Array<String>? = null
+    //     val emptyArray = arrayOf<String>()
+    //     val array = arrayOf(
+    //         ProviderMeta.ProviderTableMeta.FILE_PATH,
+    //         ProviderMeta.ProviderTableMeta.FILE_CONTENT_LENGTH
+    //     )
+    //     sut.verifyColumns(nullArray)
+    //     sut.verifyColumns(emptyArray)
+    //     sut.verifyColumns(array)
+    // }
+    // @Test
+    // fun parameterizedSelection() {
+    //     val sut = FileContentProvider()
+    //     TestCase.assertEquals("test =?", sut.buildParameterizedSelection("test"))
+    //     TestCase.assertEquals("column1 =?, column2 =?", sut.buildParameterizedSelection("column1, column2"))
+    // }
+}

src/main/java/com/owncloud/android/datamodel/ExternalLinksProvider.java

@@ -76,7 +76,9 @@ public class ExternalLinksProvider {
      * @return numbers of rows deleted
      */
     public int deleteAllExternalLinks() {
-        return mContentResolver.delete(ProviderMeta.ProviderTableMeta.CONTENT_URI_EXTERNAL_LINKS, " 1 = 1 ", null);
+        return mContentResolver.delete(ProviderMeta.ProviderTableMeta.CONTENT_URI_EXTERNAL_LINKS,
+                                       null,
+                                       null);
     }
 
     /**

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

@@ -554,6 +554,10 @@ public class FileDataStorageManager {
                     // ""+file.getFileId());
                     Uri file_uri = ContentUris.withAppendedId(ProviderTableMeta.CONTENT_URI_FILE, ocFile.getFileId());
                     String where = ProviderTableMeta.FILE_ACCOUNT_OWNER + AND + ProviderTableMeta.FILE_PATH + "=?";
+
+                    // TODO switch to string array with fixed keys in it
+                    // file_owner = ? and path =?
+                    // String[] { "file_owner",  "path"}
                     String[] whereArgs = new String[]{user.getAccountName(), ocFile.getRemotePath()};
                     int deleted = 0;
                     if (getContentProviderClient() != null) {
@@ -905,7 +909,7 @@ public class FileDataStorageManager {
                 ProviderTableMeta.FILE_PARENT + "=?",
                 new String[]{String.valueOf(parentId)},
                 null
-            );
+                                               );
         }
 
         if (cursor != null) {

src/main/java/com/owncloud/android/datamodel/SyncedFolderProvider.java

@@ -112,12 +112,12 @@ public class SyncedFolderProvider extends Observable {
      */
     public List<SyncedFolder> getSyncedFolders() {
         Cursor cursor = mContentResolver.query(
-                ProviderMeta.ProviderTableMeta.CONTENT_URI_SYNCED_FOLDERS,
-                null,
-                "1=1",
-                null,
-                null
-        );
+            ProviderMeta.ProviderTableMeta.CONTENT_URI_SYNCED_FOLDERS,
+            null,
+            null, // TODO check if this is correct, via test?
+            null,
+            null
+                                              );
 
         if (cursor != null) {
             List<SyncedFolder> list = new ArrayList<>(cursor.getCount());

src/main/java/com/owncloud/android/db/ProviderMeta.java

@@ -122,6 +122,7 @@ public class ProviderMeta {
             _ID,
             FILE_PARENT,
             FILE_NAME,
+            FILE_ENCRYPTED_NAME,
             FILE_CREATION,
             FILE_MODIFIED,
             FILE_MODIFIED_AT_LAST_SYNC_FOR_DATA,
@@ -129,9 +130,11 @@ public class ProviderMeta {
             FILE_CONTENT_TYPE,
             FILE_STORAGE_PATH,
             FILE_PATH,
+            FILE_PATH_DECRYPTED,
             FILE_ACCOUNT_OWNER,
             FILE_LAST_SYNC_DATE,
             FILE_LAST_SYNC_DATE_FOR_DATA,
+            FILE_KEEP_IN_SYNC,
             FILE_ETAG,
             FILE_ETAG_ON_SERVER,
             FILE_SHARED_VIA_LINK,
@@ -146,6 +149,9 @@ public class ProviderMeta {
             FILE_MOUNT_TYPE,
             FILE_HAS_PREVIEW,
             FILE_UNREAD_COMMENTS_COUNT,
+            FILE_OWNER_ID,
+            FILE_OWNER_DISPLAY_NAME,
+            FILE_NOTE,
             FILE_SHAREES,
             FILE_RICH_WORKSPACE));
 

src/main/java/com/owncloud/android/providers/FileContentProvider.java

@@ -64,8 +64,11 @@ import java.util.Locale;
 import javax.inject.Inject;
 
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 import dagger.android.AndroidInjection;
 
+
 /**
  * The ContentProvider for the ownCloud App.
  */
@@ -121,6 +124,10 @@ public class FileContentProvider extends ContentProvider {
     }
 
     private int delete(SQLiteDatabase db, Uri uri, String where, String... whereArgs) {
+        // TODO check every where: must be in allowed key set and then we concenate " =? " to each key value to have
+        //  a proper sanitized where clause
+//        verifyColumnsSQL(where);
+
         if (isCallerNotAllowed(uri)) {
             return -1;
         }
@@ -194,6 +201,7 @@ public class FileContentProvider extends ContentProvider {
         }
 
         if (uri.getPathSegments().size() > MINIMUM_PATH_SEGMENTS_SIZE) {
+            // TODO this *must* fail with new protection (as where is to be checked) 
             count += db.delete(ProviderTableMeta.FILE_TABLE_NAME,
                                ProviderTableMeta._ID + "=" + uri.getPathSegments().get(1)
                                    + (!TextUtils.isEmpty(where) ? " AND (" + where + ")" : ""), whereArgs);
@@ -215,6 +223,7 @@ public class FileContentProvider extends ContentProvider {
             if (remoteId == null) {
                 return 0;
             } else {
+                // TODO ask Lukas for a version of exploit and how to sanitize it
                 count = db.delete(ProviderTableMeta.FILE_TABLE_NAME,
                                   ProviderTableMeta._ID + "=" + uri.getPathSegments().get(1)
                                       + (!TextUtils.isEmpty(where) ? " AND (" + where + ")" : ""), whereArgs);
@@ -262,6 +271,16 @@ public class FileContentProvider extends ContentProvider {
     }
 
     private Uri insert(SQLiteDatabase db, Uri uri, ContentValues values) {
+        // verify only for those requests that are not internal (files table)
+        switch (mUriMatcher.match(uri)) {
+            case ROOT_DIRECTORY:
+            case SINGLE_FILE:
+            case DIRECTORY:
+                verifyColumns(values);
+                break;
+        }
+
+
         switch (mUriMatcher.match(uri)) {
             case ROOT_DIRECTORY:
             case SINGLE_FILE:
@@ -483,9 +502,27 @@ public class FileContentProvider extends ContentProvider {
         return result;
     }
 
-    private Cursor query(SQLiteDatabase db, Uri uri, String[] projectionArray, String selection, String[] selectionArgs,
+    private Cursor query(SQLiteDatabase db,
+                         Uri uri,
+                         String[] projectionArray,
+                         String selection,
+                         String[] selectionArgs,
                          String sortOrder) {
 
+        // verify only for those requests that are not internal
+        switch (mUriMatcher.match(uri)) {
+            case ROOT_DIRECTORY:
+            case SINGLE_FILE:
+            case DIRECTORY:
+                verifyColumnNames(projectionArray);
+                // TODO verify selection and sortorder
+//                verifyColumnsSQL(selection);
+//                verifyColumnsSQL(sortOrder);
+
+            default:
+                // do nothing
+        }
+
         SQLiteQueryBuilder sqlQuery = new SQLiteQueryBuilder();
 
         sqlQuery.setTables(ProviderTableMeta.FILE_TABLE_NAME);
@@ -622,6 +659,17 @@ public class FileContentProvider extends ContentProvider {
         if (isCallerNotAllowed(uri)) {
             return -1;
         }
+        // verify only for those requests that are not internal
+        switch (mUriMatcher.match(uri)) {
+            case ROOT_DIRECTORY:
+            case SINGLE_FILE:
+            case DIRECTORY:
+//                verifyColumnsSQL(selection);
+                verifyColumns(values);
+
+            default:
+                // do nothing
+        }
 
         int count;
         SQLiteDatabase db = mDbHelper.getWritableDatabase();
@@ -637,6 +685,10 @@ public class FileContentProvider extends ContentProvider {
     }
 
     private int update(SQLiteDatabase db, Uri uri, ContentValues values, String selection, String... selectionArgs) {
+        // TODO check every key of contentValues and match via our allowed list -> ProviderMeta:85
+        // TODO check all other insert and update methods
+        // for selection: same like delete > only pass string array of keys and we build a sql where clause here
+
         switch (mUriMatcher.match(uri)) {
             case DIRECTORY:
                 return 0;
@@ -1034,6 +1086,67 @@ public class FileContentProvider extends ContentProvider {
         }
     }
 
+    @VisibleForTesting
+    public void verifyColumns(@Nullable ContentValues contentValues) {
+        if (contentValues == null) {
+            return;
+        }
+
+        for (String name : contentValues.keySet()) {
+            verifyColumnName(name);
+        }
+    }
+
+    @VisibleForTesting
+    public void verifyColumnNames(@Nullable String[] columns) {
+        if (columns == null) {
+            return;
+        }
+
+        for (String name : columns) {
+            verifyColumnName(name);
+        }
+    }
+
+
+    /**
+     * matches against ProviderMeta, to verify that only allowed columns are used
+     */
+    @VisibleForTesting
+    public void verifyColumnsSQL(@Nullable String sql) {
+        if (sql == null) {
+            return;
+        }
+
+        for (String parameter : Uri.parse(sql).getQueryParameterNames()) {
+            String sanitized = parameter.replaceAll("AND", "").replaceAll("OR", "").replaceAll("\\s+", "");
+
+            verifyColumnName(sanitized);
+        }// check plain name
+    }
+
+    @VisibleForTesting
+    public void verifyColumnName(@NonNull String columnName) {
+        if (!ProviderTableMeta.FILE_ALL_COLUMNS.contains(columnName)) {
+            throw new IllegalArgumentException(String.format("Column name \"%s\" is not allowed", columnName));
+        }
+    }
+
+    public String buildParameterizedSelection(String selection) {
+        StringBuilder stringBuilder = new StringBuilder();
+        // add = ?
+        boolean moreThanOne = false;
+        for (String string : selection.split(",")) {
+            if (moreThanOne) {
+                stringBuilder.append(", ");
+            }
+            stringBuilder.append(string.trim());
+            stringBuilder.append(" =?");
+            moreThanOne = true;
+        }
+        return stringBuilder.toString();
+    }
+
     class DataBaseHelper extends SQLiteOpenHelper {
         DataBaseHelper(Context context) {
             super(context, ProviderMeta.DB_NAME, null, ProviderMeta.DB_VERSION);