From b06b739b078ce4b00600487cfec31659647bf31f Mon Sep 17 00:00:00 2001 From: Steve Howard Date: Thu, 22 Jul 2010 11:33:50 -0700 Subject: 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 --- AndroidManifest.xml | 3 +- .../providers/downloads/DownloadProvider.java | 164 ++++++++++++++++----- src/com/android/providers/downloads/Helpers.java | 8 +- .../tests/DownloadProviderPermissionsTest.java | 23 +-- tests/public_api_access/Android.mk | 14 ++ tests/public_api_access/AndroidManifest.xml | 37 +++++ .../PublicApiAccessTest.java | 129 ++++++++++++++++ 7 files changed, 311 insertions(+), 67 deletions(-) create mode 100644 tests/public_api_access/Android.mk create mode 100644 tests/public_api_access/AndroidManifest.xml create mode 100644 tests/public_api_access/src/com/android/providers/downloads/public_api_access_tests/PublicApiAccessTest.java diff --git a/AndroidManifest.xml b/AndroidManifest.xml index 374b985a..20fe6ca6 100644 --- a/AndroidManifest.xml +++ b/AndroidManifest.xml @@ -34,8 +34,7 @@ + android:authorities="downloads" /> 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); } @@ -439,6 +442,98 @@ public final class DownloadProvider extends ContentProvider { return ret; } + /** + * 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 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 */ @@ -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 *

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 { @@ -64,28 +64,15 @@ public class DownloadProviderPermissionsTest extends AndroidTestCase { } } - /** - * Test that an untrusted app cannot read from the download provider - *

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 *

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 @@ + + + + + + + + + + + + + + + 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 + } + } +} -- cgit v1.2.3