summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorSteve Howard <showard@google.com>2010-07-22 11:33:50 -0700
committerSteve Howard <showard@google.com>2010-07-22 18:38:51 -0700
commitb06b739b078ce4b00600487cfec31659647bf31f (patch)
tree9cfe924b01710eb1c7a6ad7b553fdabb05106843
parent0d8d89105c00edbad95a268aaae65f2ff94ed5a1 (diff)
downloadandroid_packages_providers_DownloadProvider-b06b739b078ce4b00600487cfec31659647bf31f.tar.gz
android_packages_providers_DownloadProvider-b06b739b078ce4b00600487cfec31659647bf31f.tar.bz2
android_packages_providers_DownloadProvider-b06b739b078ce4b00600487cfec31659647bf31f.zip
Make DownloadProvider accessible for public API usage.
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
-rw-r--r--AndroidManifest.xml3
-rw-r--r--src/com/android/providers/downloads/DownloadProvider.java164
-rw-r--r--src/com/android/providers/downloads/Helpers.java8
-rw-r--r--tests/permission/src/com/android/providers/downloads/permission/tests/DownloadProviderPermissionsTest.java23
-rw-r--r--tests/public_api_access/Android.mk14
-rw-r--r--tests/public_api_access/AndroidManifest.xml37
-rw-r--r--tests/public_api_access/src/com/android/providers/downloads/public_api_access_tests/PublicApiAccessTest.java129
7 files changed, 311 insertions, 67 deletions
diff --git a/AndroidManifest.xml b/AndroidManifest.xml
index 374b985a..20fe6ca6 100644
--- a/AndroidManifest.xml
+++ b/AndroidManifest.xml
@@ -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">
diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java
index e543c443..2c0a2645 100644
--- a/src/com/android/providers/downloads/DownloadProvider.java
+++ b/src/com/android/providers/downloads/DownloadProvider.java
@@ -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
diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java
index dac4b55e..58ab578a 100644
--- a/src/com/android/providers/downloads/Helpers.java
+++ b/src/com/android/providers/downloads/Helpers.java
@@ -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);
diff --git a/tests/permission/src/com/android/providers/downloads/permission/tests/DownloadProviderPermissionsTest.java b/tests/permission/src/com/android/providers/downloads/permission/tests/DownloadProviderPermissionsTest.java
index ecdce93c..4c6717c3 100644
--- a/tests/permission/src/com/android/providers/downloads/permission/tests/DownloadProviderPermissionsTest.java
+++ b/tests/permission/src/com/android/providers/downloads/permission/tests/DownloadProviderPermissionsTest.java
@@ -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
index 00000000..6c6db1f4
--- /dev/null
+++ b/tests/public_api_access/Android.mk
@@ -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
index 00000000..01048460
--- /dev/null
+++ b/tests/public_api_access/AndroidManifest.xml
@@ -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
index 00000000..aca5791b
--- /dev/null
+++ b/tests/public_api_access/src/com/android/providers/downloads/public_api_access_tests/PublicApiAccessTest.java
@@ -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
+ }
+ }
+}