Make DownloadProvider accessible for public API usage.
Steve Howard [Thu, 22 Jul 2010 18:33:50 +0000 (11:33 -0700)]
This change removes the requirement that apps have the
ACCESS_DOWNLOAD_MANAGER permission in order to access
DownloadProvider.  This enables the public API to work.  Instead,
DownloadProvider enforces the new permissions model for the public
API:
* insert() requires INTERNET permission
* insert() checks that input fits within the restricted input allowed
  for the public API
* insert() also strictly checks the file URI provided with
  DESTINATION_FILE_URI (and still requires WRITE_EXTERNAL_STORAGE
  permission if that is supplied)

Note that if an app has the ACCESS_DOWNLOAD_MANAGER permission, legacy
behavior is retained.

I've added a test to cover this new access, and updated the existing
permissions tests.

I also fixed a bug in WHERE clause construction in update() and
delete(), and refactored the code to eliminate duplication.

Change-Id: I53a08df137b35c2788c36350276c9dff24858af1

AndroidManifest.xml
src/com/android/providers/downloads/DownloadProvider.java
src/com/android/providers/downloads/Helpers.java
tests/permission/src/com/android/providers/downloads/permission/tests/DownloadProviderPermissionsTest.java
tests/public_api_access/Android.mk [new file with mode: 0644]
tests/public_api_access/AndroidManifest.xml [new file with mode: 0644]
tests/public_api_access/src/com/android/providers/downloads/public_api_access_tests/PublicApiAccessTest.java [new file with mode: 0644]

index 374b985..20fe6ca 100644 (file)
@@ -34,8 +34,7 @@
     <application android:process="android.process.media"
                  android:label="@string/app_label">
         <provider android:name=".DownloadProvider"
-                android:authorities="downloads"
-                android:permission="android.permission.ACCESS_DOWNLOAD_MANAGER" />
+                android:authorities="downloads" />
         <service android:name=".DownloadService"
                 android:permission="android.permission.ACCESS_DOWNLOAD_MANAGER" />
         <receiver android:name=".DownloadReceiver" android:exported="false">
index e543c44..2c0a264 100644 (file)
@@ -34,6 +34,7 @@ import android.database.sqlite.SQLiteOpenHelper;
 import android.database.sqlite.SQLiteQueryBuilder;
 import android.net.Uri;
 import android.os.Binder;
+import android.os.Environment;
 import android.os.ParcelFileDescriptor;
 import android.os.Process;
 import android.provider.Downloads;
@@ -319,6 +320,7 @@ public final class DownloadProvider extends ContentProvider {
      */
     @Override
     public Uri insert(final Uri uri, final ContentValues values) {
+        checkInsertPermissions(values);
         SQLiteDatabase db = mOpenHelper.getWritableDatabase();
 
         if (sURIMatcher.match(uri) != DOWNLOADS) {
@@ -349,6 +351,7 @@ public final class DownloadProvider extends ContentProvider {
                         android.Manifest.permission.WRITE_EXTERNAL_STORAGE,
                         Binder.getCallingPid(), Binder.getCallingUid(),
                         "need WRITE_EXTERNAL_STORAGE permission to use DESTINATION_FILE_URI");
+                checkFileUriDestination(values);
             }
             filteredValues.put(Downloads.Impl.COLUMN_DESTINATION, dest);
         }
@@ -440,6 +443,98 @@ public final class DownloadProvider extends ContentProvider {
     }
 
     /**
+     * Check that the file URI provided for DESTINATION_FILE_URI is valid.
+     */
+    private void checkFileUriDestination(ContentValues values) {
+        String fileUri = values.getAsString(Downloads.Impl.COLUMN_FILE_NAME_HINT);
+        if (fileUri == null) {
+            throw new IllegalArgumentException(
+                    "DESTINATION_FILE_URI must include a file URI under COLUMN_FILE_NAME_HINT");
+        }
+        Uri uri = Uri.parse(fileUri);
+        if (!uri.getScheme().equals("file")) {
+            throw new IllegalArgumentException("Not a file URI: " + uri);
+        }
+        File path = new File(uri.getSchemeSpecificPart());
+        String externalPath = Environment.getExternalStorageDirectory().getAbsolutePath();
+        if (!path.getPath().startsWith(externalPath)) {
+            throw new SecurityException("Destination must be on external storage: " + uri);
+        }
+    }
+
+    /**
+     * Apps with the ACCESS_DOWNLOAD_MANAGER permission can access this provider freely, subject to
+     * constraints in the rest of the code. Apps without that may still access this provider through
+     * the public API, but additional restrictions are imposed. We check those restrictions here.
+     *
+     * @param values ContentValues provided to insert()
+     * @throws SecurityException if the caller has insufficient permissions
+     */
+    private void checkInsertPermissions(ContentValues values) {
+        if (getContext().checkCallingOrSelfPermission(Downloads.Impl.PERMISSION_ACCESS)
+                == PackageManager.PERMISSION_GRANTED) {
+            return;
+        }
+
+        getContext().enforceCallingOrSelfPermission(android.Manifest.permission.INTERNET,
+                "INTERNET permission is required to use the download manager");
+
+        // ensure the request fits within the bounds of a public API request
+        // first copy so we can remove values
+        values = new ContentValues(values);
+
+        // check columns whose values are restricted
+        enforceAllowedValues(values, Downloads.Impl.COLUMN_IS_PUBLIC_API, Boolean.TRUE);
+        enforceAllowedValues(values, Downloads.Impl.COLUMN_DESTINATION,
+                Downloads.Impl.DESTINATION_CACHE_PARTITION_PURGEABLE,
+                Downloads.Impl.DESTINATION_FILE_URI);
+        enforceAllowedValues(values, Downloads.Impl.COLUMN_VISIBILITY,
+                null, Downloads.Impl.VISIBILITY_VISIBLE);
+
+        // remove the rest of the columns that are allowed (with any value)
+        values.remove(Downloads.Impl.COLUMN_URI);
+        values.remove(Downloads.Impl.COLUMN_TITLE);
+        values.remove(Downloads.Impl.COLUMN_DESCRIPTION);
+        values.remove(Downloads.Impl.COLUMN_MIME_TYPE);
+        values.remove(Downloads.Impl.COLUMN_FILE_NAME_HINT); // checked later in insert()
+        values.remove(Downloads.Impl.COLUMN_NOTIFICATION_PACKAGE); // checked later in insert()
+        values.remove(Downloads.Impl.COLUMN_ALLOWED_NETWORK_TYPES);
+        values.remove(Downloads.Impl.COLUMN_ALLOW_ROAMING);
+
+        // any extra columns are extraneous and disallowed
+        if (values.size() > 0) {
+            StringBuilder error = new StringBuilder("Invalid columns in request: ");
+            boolean first = true;
+            for (Map.Entry<String, Object> entry : values.valueSet()) {
+                if (!first) {
+                    error.append(", ");
+                }
+                error.append(entry.getKey());
+            }
+            throw new SecurityException(error.toString());
+        }
+    }
+
+    /**
+     * Remove column from values, and throw a SecurityException if the value isn't within the
+     * specified allowedValues.
+     */
+    private void enforceAllowedValues(ContentValues values, String column,
+            Object... allowedValues) {
+        Object value = values.get(column);
+        values.remove(column);
+        for (Object allowedValue : allowedValues) {
+            if (value == null && allowedValue == null) {
+                return;
+            }
+            if (value != null && value.equals(allowedValue)) {
+                return;
+            }
+        }
+        throw new SecurityException("Invalid value for " + column + ": " + value);
+    }
+
+    /**
      * Starts a database query
      */
     @Override
@@ -606,7 +701,7 @@ public final class DownloadProvider extends ContentProvider {
      */
     private void deleteRequestHeaders(SQLiteDatabase db, String where, String[] whereArgs) {
         String[] projection = new String[] {Downloads.Impl._ID};
-        Cursor cursor = db.query(DB_TABLE, projection , where, whereArgs, null, null, null, null);
+        Cursor cursor = db.query(DB_TABLE, projection, where, whereArgs, null, null, null, null);
         try {
             for (cursor.moveToFirst(); !cursor.isAfterLast(); cursor.moveToNext()) {
                 long id = cursor.getLong(0);
@@ -682,26 +777,9 @@ public final class DownloadProvider extends ContentProvider {
         switch (match) {
             case DOWNLOADS:
             case DOWNLOADS_ID: {
-                String myWhere;
-                if (where != null) {
-                    if (match == DOWNLOADS) {
-                        myWhere = "( " + where + " )";
-                    } else {
-                        myWhere = "( " + where + " ) AND ";
-                    }
-                } else {
-                    myWhere = "";
-                }
-                if (match == DOWNLOADS_ID) {
-                    String segment = getDownloadIdFromUri(uri);
-                    rowId = Long.parseLong(segment);
-                    myWhere += " ( " + Downloads.Impl._ID + " = " + rowId + " ) ";
-                }
-                if (shouldRestrictVisibility()) {
-                    myWhere += " AND " + getRestrictedUidClause();
-                }
+                String fullWhere = getWhereClause(uri, where);
                 if (filteredValues.size() > 0) {
-                    count = db.update(DB_TABLE, filteredValues, myWhere, whereArgs);
+                    count = db.update(DB_TABLE, filteredValues, fullWhere, whereArgs);
                 } else {
                     count = 0;
                 }
@@ -722,6 +800,22 @@ public final class DownloadProvider extends ContentProvider {
         return count;
     }
 
+    private String getWhereClause(final Uri uri, final String where) {
+        StringBuilder myWhere = new StringBuilder();
+        if (where != null) {
+            myWhere.append("( " + where + " )");
+        }
+        if (sURIMatcher.match(uri) == DOWNLOADS_ID) {
+            String segment = getDownloadIdFromUri(uri);
+            long rowId = Long.parseLong(segment);
+            appendClause(myWhere, " ( " + Downloads.Impl._ID + " = " + rowId + " ) ");
+        }
+        if (shouldRestrictVisibility()) {
+            appendClause(myWhere, getRestrictedUidClause());
+        }
+        return myWhere.toString();
+    }
+
     /**
      * Deletes a row in the database
      */
@@ -737,26 +831,9 @@ public final class DownloadProvider extends ContentProvider {
         switch (match) {
             case DOWNLOADS:
             case DOWNLOADS_ID: {
-                String myWhere;
-                if (where != null) {
-                    if (match == DOWNLOADS) {
-                        myWhere = "( " + where + " )";
-                    } else {
-                        myWhere = "( " + where + " ) AND ";
-                    }
-                } else {
-                    myWhere = "";
-                }
-                if (match == DOWNLOADS_ID) {
-                    String segment = getDownloadIdFromUri(uri);
-                    long rowId = Long.parseLong(segment);
-                    myWhere += " ( " + Downloads.Impl._ID + " = " + rowId + " ) ";
-                }
-                if (shouldRestrictVisibility()) {
-                    myWhere += " AND " + getRestrictedUidClause();
-                }
-                deleteRequestHeaders(db, where, whereArgs);
-                count = db.delete(DB_TABLE, myWhere, whereArgs);
+                String fullWhere = getWhereClause(uri, where);
+                deleteRequestHeaders(db, fullWhere, whereArgs);
+                count = db.delete(DB_TABLE, fullWhere, whereArgs);
                 break;
             }
             default: {
@@ -769,6 +846,13 @@ public final class DownloadProvider extends ContentProvider {
         getContext().getContentResolver().notifyChange(uri, null);
         return count;
     }
+    
+    private void appendClause(StringBuilder whereClause, String newClause) {
+        if (whereClause.length() != 0) {
+            whereClause.append(" AND ");
+        }
+        whereClause.append(newClause);
+    }
 
     /**
      * Remotely opens a file
index dac4b55..58ab578 100644 (file)
@@ -117,13 +117,7 @@ public class Helpers {
     }
 
     private static String getPathForFileUri(String hint) throws GenerateSaveFileError {
-        Uri uri = Uri.parse(hint);
-        if (!uri.getScheme().equals("file")) {
-            Log.d(Constants.TAG, "Not a file URI: " + hint);
-            throw new GenerateSaveFileError(Downloads.Impl.STATUS_FILE_ERROR);
-        }
-
-        String path = uri.getSchemeSpecificPart();
+        String path = Uri.parse(hint).getSchemeSpecificPart();
         if (new File(path).exists()) {
             Log.d(Constants.TAG, "File already exists: " + path);
             throw new GenerateSaveFileError(Downloads.Impl.STATUS_FILE_ERROR);
index ecdce93..4c6717c 100644 (file)
@@ -46,7 +46,7 @@ public class DownloadProviderPermissionsTest extends AndroidTestCase {
     /**
      * Test that an app cannot access the /cache filesystem
      * <p>Tests Permission:
-     *   {@link com.android.providers.downloads.Manifest.permission#ACCESS_CACHE_FILESYSTEM}
+     *   {@link android.Manifest.permission#ACCESS_CACHE_FILESYSTEM}
      */
     @MediumTest
     public void testAccessCacheFilesystem() throws IOException {
@@ -65,27 +65,14 @@ public class DownloadProviderPermissionsTest extends AndroidTestCase {
     }
 
     /**
-     * Test that an untrusted app cannot read from the download provider
-     * <p>Tests Permission:
-     *   {@link com.android.providers.downloads.Manifest.permission#ACCESS_DOWNLOAD_MANAGER}
-     */
-    @MediumTest
-    public void testReadDownloadProvider() throws IOException {
-        try {
-            mContentResolver.query(Downloads.Impl.CONTENT_URI, null, null, null, null);
-            fail("read from provider did not throw SecurityException as expected.");
-        } catch (SecurityException e) {
-            // expected
-        }
-    }
-
-    /**
      * Test that an untrusted app cannot write to the download provider
      * <p>Tests Permission:
      *   {@link com.android.providers.downloads.Manifest.permission#ACCESS_DOWNLOAD_MANAGER}
+     *   and
+     *   {@link android.Manifest.permission#INTERNET}
      */
     @MediumTest
-    public void testWriteDownloadProvider() throws IOException {
+    public void testWriteDownloadProvider() {
         try {
             ContentValues values = new ContentValues();
             values.put(Downloads.Impl.COLUMN_URI, "foo");
@@ -102,7 +89,7 @@ public class DownloadProviderPermissionsTest extends AndroidTestCase {
      *   {@link com.android.providers.downloads.Manifest.permission#ACCESS_DOWNLOAD_MANAGER}
      */
     @MediumTest
-    public void testStartDownloadService() throws IOException {
+    public void testStartDownloadService() {
         try {
             Intent downloadServiceIntent = new Intent();
             downloadServiceIntent.setClassName("com.android.providers.downloads",
diff --git a/tests/public_api_access/Android.mk b/tests/public_api_access/Android.mk
new file mode 100644 (file)
index 0000000..6c6db1f
--- /dev/null
@@ -0,0 +1,14 @@
+LOCAL_PATH:= $(call my-dir)
+include $(CLEAR_VARS)
+
+# We only want this apk build for tests.
+LOCAL_MODULE_TAGS := tests
+
+# Include all test java files.
+LOCAL_SRC_FILES := $(call all-java-files-under, src)
+
+LOCAL_JAVA_LIBRARIES := android.test.runner
+LOCAL_PACKAGE_NAME := DownloadPublicApiAccessTests
+
+include $(BUILD_PACKAGE)
+
diff --git a/tests/public_api_access/AndroidManifest.xml b/tests/public_api_access/AndroidManifest.xml
new file mode 100644 (file)
index 0000000..0104846
--- /dev/null
@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!--
+ * Copyright (C) 2009 Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ -->
+
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+          package="com.android.providers.downloads.public_api_access_tests">
+
+    <application>
+        <uses-library android:name="android.test.runner" />
+    </application>
+
+    <uses-permission android:name="android.permission.INTERNET"/>
+
+    <!--
+    The test declared in this instrumentation can be run via this command
+    "adb shell am instrument -w com.android.providers.downloads.permission.tests/android.test.InstrumentationTestRunner"
+    We intentionally target our own package to ensure this runs in a separate process under a
+    separate UID.
+    -->
+    <instrumentation android:name="android.test.InstrumentationTestRunner"
+                     android:targetPackage="com.android.providers.downloads.public_api_access_tests"
+                     android:label="Tests for public API access channels to DownloadProvider"/>
+
+</manifest>
diff --git a/tests/public_api_access/src/com/android/providers/downloads/public_api_access_tests/PublicApiAccessTest.java b/tests/public_api_access/src/com/android/providers/downloads/public_api_access_tests/PublicApiAccessTest.java
new file mode 100644 (file)
index 0000000..aca5791
--- /dev/null
@@ -0,0 +1,129 @@
+/*
+ * Copyright (C) 2009 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.providers.downloads.public_api_access_tests;
+
+import android.content.ContentResolver;
+import android.content.ContentValues;
+import android.provider.Downloads;
+import android.test.AndroidTestCase;
+import android.test.suitebuilder.annotation.MediumTest;
+
+/**
+ * DownloadProvider allows apps without permission ACCESS_DOWNLOAD_MANAGER to access it -- this is
+ * how the public API works.  But such access is subject to strict constraints on what can be
+ * inserted.  This test suite checks those constraints.
+ */
+@MediumTest
+public class PublicApiAccessTest extends AndroidTestCase {
+    private static final String[] DISALLOWED_COLUMNS = new String[] {
+                    Downloads.Impl.COLUMN_COOKIE_DATA,
+                    Downloads.Impl.COLUMN_REFERER,
+                    Downloads.Impl.COLUMN_USER_AGENT,
+                    Downloads.Impl.COLUMN_NO_INTEGRITY,
+                    Downloads.Impl.COLUMN_NOTIFICATION_CLASS,
+                    Downloads.Impl.COLUMN_NOTIFICATION_EXTRAS,
+                    Downloads.Impl.COLUMN_OTHER_UID,
+                    Downloads.Impl.COLUMN_APP_DATA,
+                    Downloads.Impl.COLUMN_CONTROL,
+                    Downloads.Impl.COLUMN_STATUS,
+            };
+
+    private ContentResolver mContentResolver;
+
+    @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+        mContentResolver = getContext().getContentResolver();
+    }
+
+    @Override
+    protected void tearDown() throws Exception {
+        if (mContentResolver != null) {
+            mContentResolver.delete(Downloads.CONTENT_URI, null, null); 
+        }
+        super.tearDown();
+    }
+
+    public void testMinimalValidWrite() {
+        mContentResolver.insert(Downloads.Impl.CONTENT_URI, buildValidValues());
+    }
+    
+    public void testMaximalValidWrite() {
+        ContentValues values = buildValidValues();
+        values.put(Downloads.Impl.COLUMN_TITLE, "foo");
+        values.put(Downloads.Impl.COLUMN_DESCRIPTION, "foo");
+        values.put(Downloads.Impl.COLUMN_MIME_TYPE, "foo");
+        values.put(Downloads.Impl.COLUMN_NOTIFICATION_PACKAGE, "foo");
+        values.put(Downloads.Impl.COLUMN_ALLOWED_NETWORK_TYPES, 0);
+        values.put(Downloads.Impl.COLUMN_ALLOW_ROAMING, true);
+        mContentResolver.insert(Downloads.Impl.CONTENT_URI, values);
+    }
+
+    private ContentValues buildValidValues() {
+        ContentValues values = new ContentValues();
+        values.put(Downloads.Impl.COLUMN_URI, "foo");
+        values.put(Downloads.Impl.COLUMN_DESTINATION, 
+                Downloads.Impl.DESTINATION_CACHE_PARTITION_PURGEABLE);
+        values.put(Downloads.Impl.COLUMN_IS_PUBLIC_API, true);
+        return values;
+    }
+    
+    public void testNoPublicApi() {
+        ContentValues values = buildValidValues();
+        values.remove(Downloads.Impl.COLUMN_IS_PUBLIC_API);
+        testInvalidValues(values);
+    }
+    
+    public void testInvalidDestination() {
+        ContentValues values = buildValidValues();
+        values.put(Downloads.Impl.COLUMN_DESTINATION, Downloads.Impl.DESTINATION_EXTERNAL);
+        testInvalidValues(values);
+        values.put(Downloads.Impl.COLUMN_DESTINATION, Downloads.Impl.DESTINATION_CACHE_PARTITION);
+        testInvalidValues(values);
+    }
+    
+    public void testInvalidVisibility() {
+        ContentValues values = buildValidValues();
+        values.put(Downloads.Impl.COLUMN_VISIBILITY, 
+                Downloads.Impl.VISIBILITY_VISIBLE_NOTIFY_COMPLETED);
+        testInvalidValues(values);
+    }
+    
+    public void testDisallowedColumns() {
+        for (String column : DISALLOWED_COLUMNS) {
+            ContentValues values = buildValidValues();
+            values.put(column, 1);
+            testInvalidValues(values);
+        }
+    }
+    
+    public void testFileUriWithoutExternalPermission() {
+        ContentValues values = buildValidValues();
+        values.put(Downloads.Impl.COLUMN_DESTINATION, Downloads.Impl.DESTINATION_FILE_URI);
+        values.put(Downloads.Impl.COLUMN_FILE_NAME_HINT, "file:///sdcard/foo");
+        testInvalidValues(values);
+    }
+
+    private void testInvalidValues(ContentValues values) {
+        try {
+            mContentResolver.insert(Downloads.Impl.CONTENT_URI, values);
+            fail("Didn't get SecurityException as expected");
+        } catch (SecurityException exc) {
+            // expected
+        }
+    }
+}