Predictable IDs in TemporaryStorageProvider

[mgeier63]

Currently the IDs in the TemporaryStorageProvider are completely predictable, i.e. increment by 1, starting from 1.
Thus a malicious app is able to read/access temporary files by just trying IDs starting from 1

It'd probably be better to have a random UUID as the key to access files in the temporary storage provider.

Sorry, no PR due to my offline setup, but here's a patch file:

Index: OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/TemporaryStorageProvider.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/TemporaryStorageProvider.java   (revision 63ca81d8717bad8df9a9c159126612eb62867647)
+++ OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/TemporaryStorageProvider.java   (revision )
@@ -35,6 +35,7 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.util.UUID;

 public class TemporaryStorageProvider extends ContentProvider {

@@ -66,7 +67,7 @@
         @Override
         public void onCreate(SQLiteDatabase db) {
             db.execSQL("CREATE TABLE IF NOT EXISTS " + TABLE_FILES + " (" +
-                    COLUMN_ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
+                    COLUMN_ID + " TEXT PRIMARY KEY, " +
                     COLUMN_NAME + " TEXT, " +
                     COLUMN_TIME + " INTEGER" +
                     ");");
@@ -82,13 +83,13 @@

     private File getFile(Uri uri) throws FileNotFoundException {
         try {
-            return getFile(Integer.parseInt(uri.getLastPathSegment()));
+            return getFile(uri.getLastPathSegment());
         } catch (NumberFormatException e) {
             throw new FileNotFoundException();
         }
     }

-    private File getFile(int id) {
+    private File getFile(String id) {
         return new File(getContext().getCacheDir(), "temp/" + id);
     }

@@ -133,13 +134,15 @@
         if (!values.containsKey(COLUMN_TIME)) {
             values.put(COLUMN_TIME, System.currentTimeMillis());
         }
+        String uuid = UUID.randomUUID().toString();
+        values.put(COLUMN_ID, uuid);
         int insert = (int) db.getWritableDatabase().insert(TABLE_FILES, null, values);
         try {
-            getFile(insert).createNewFile();
+            getFile(uuid).createNewFile();
         } catch (IOException e) {
             return null;
         }
-        return Uri.withAppendedPath(BASE_URI, Long.toString(insert));
+        return Uri.withAppendedPath(BASE_URI, uuid);
     }

     @Override
@@ -152,7 +155,7 @@
                 selectionArgs, null, null, null);
         if (files != null) {
             while (files.moveToNext()) {
-                getFile(files.getInt(0)).delete();
+                getFile(files.getString(0)).delete();
             }
             files.close();
             return db.getWritableDatabase().delete(TABLE_FILES, selection, selectionArgs);

[dschuermann]

You are right. We should definitly switch to using https://developer.android.com/reference/android/support/v4/content/FileProvider.html
I will apply your patch.
Many many thanks for looking into this and reporting the problem.

[dschuermann]

Your patch has been applied in 4e42549 as a temporary solution until we do #1241