From 8be3a92eb0b4105a9ed748be5a937ce79145f565 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 1 Aug 2016 10:24:24 -0600 Subject: Enforce calling identity before clearing. When opening a downloaded file, enforce that the caller can actually see the requested download before clearing their identity to read internal columns. Bug: 30537115 Change-Id: I01bbad7997e5e908bfb19f5d576860a24f59f295 --- src/com/android/providers/downloads/DownloadProvider.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'src/com/android/providers/downloads/DownloadProvider.java') diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 2d914c41..d2a9d847 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1192,6 +1192,19 @@ public final class DownloadProvider extends ContentProvider { logVerboseOpenFileInfo(uri, mode); } + // Perform normal query to enforce caller identity access before + // clearing it to reach internal-only columns + final Cursor probeCursor = query(uri, new String[] { + Downloads.Impl._DATA }, null, null, null); + try { + if ((probeCursor == null) || (probeCursor.getCount() == 0)) { + throw new FileNotFoundException( + "No file found for " + uri + " as UID " + Binder.getCallingUid()); + } + } finally { + IoUtils.closeQuietly(probeCursor); + } + final Cursor cursor = queryCleared(uri, new String[] { Downloads.Impl._DATA, Downloads.Impl.COLUMN_STATUS, Downloads.Impl.COLUMN_DESTINATION, Downloads.Impl.COLUMN_MEDIA_SCANNED }, null, -- cgit v1.2.3 From b440ceb00fd46c9233723066c680a538067fbf82 Mon Sep 17 00:00:00 2001 From: Adam Seaton Date: Fri, 26 Aug 2016 21:13:20 +0000 Subject: Revert "Enforce calling identity before clearing." This reverts commit 8be3a92eb0b4105a9ed748be5a937ce79145f565. Change-Id: I10401d57239b868f8e3514f81a0e20486838e29c --- src/com/android/providers/downloads/DownloadProvider.java | 13 ------------- 1 file changed, 13 deletions(-) (limited to 'src/com/android/providers/downloads/DownloadProvider.java') diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index d2a9d847..2d914c41 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1192,19 +1192,6 @@ public final class DownloadProvider extends ContentProvider { logVerboseOpenFileInfo(uri, mode); } - // Perform normal query to enforce caller identity access before - // clearing it to reach internal-only columns - final Cursor probeCursor = query(uri, new String[] { - Downloads.Impl._DATA }, null, null, null); - try { - if ((probeCursor == null) || (probeCursor.getCount() == 0)) { - throw new FileNotFoundException( - "No file found for " + uri + " as UID " + Binder.getCallingUid()); - } - } finally { - IoUtils.closeQuietly(probeCursor); - } - final Cursor cursor = queryCleared(uri, new String[] { Downloads.Impl._DATA, Downloads.Impl.COLUMN_STATUS, Downloads.Impl.COLUMN_DESTINATION, Downloads.Impl.COLUMN_MEDIA_SCANNED }, null, -- cgit v1.2.3 From c0496a0b0b26bec9c37e94753823507a6067d700 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 23 Aug 2016 15:09:08 -0600 Subject: DO NOT MERGE. Send "completed" broadcast if download cancelled. When a download is deleted, we may not have an active thread, so always send the broadcast from the provider. If an active thread encounters a deleted download, skip sending the broadcast twice. Change-Id: If8d5b99a1b7232bb64c6d11f22fdb4f5d6dbbfec Test: none Bug: 30883889 (cherry picked from commit efb1ac6b49692e62fde6830c3d20953c8632d2ba) --- .../providers/downloads/DownloadProvider.java | 27 ++++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-) (limited to 'src/com/android/providers/downloads/DownloadProvider.java') diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index d30018f7..194fd1e3 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -18,7 +18,6 @@ package com.android.providers.downloads; import static android.provider.BaseColumns._ID; import static android.provider.Downloads.Impl.COLUMN_DESTINATION; -import static android.provider.Downloads.Impl.COLUMN_MEDIAPROVIDER_URI; import static android.provider.Downloads.Impl.COLUMN_MEDIA_SCANNED; import static android.provider.Downloads.Impl.COLUMN_MIME_TYPE; import static android.provider.Downloads.Impl.DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD; @@ -29,6 +28,7 @@ import android.app.DownloadManager; import android.app.DownloadManager.Request; import android.app.job.JobScheduler; import android.content.ContentProvider; +import android.content.ContentResolver; import android.content.ContentUris; import android.content.ContentValues; import android.content.Context; @@ -1193,7 +1193,10 @@ public final class DownloadProvider extends ContentProvider { Helpers.validateSelection(where, sAppReadableColumnsSet); } - final JobScheduler scheduler = getContext().getSystemService(JobScheduler.class); + final Context context = getContext(); + final ContentResolver resolver = context.getContentResolver(); + final JobScheduler scheduler = context.getSystemService(JobScheduler.class); + final SQLiteDatabase db = mOpenHelper.getWritableDatabase(); int count; int match = sURIMatcher.match(uri); @@ -1205,16 +1208,17 @@ public final class DownloadProvider extends ContentProvider { final SqlSelection selection = getWhereClause(uri, where, whereArgs, match); deleteRequestHeaders(db, selection.getSelection(), selection.getParameters()); - try (Cursor cursor = db.query(DB_TABLE, new String[] { - _ID, _DATA, COLUMN_MEDIAPROVIDER_URI - }, selection.getSelection(), selection.getParameters(), null, null, null)) { + try (Cursor cursor = db.query(DB_TABLE, null, selection.getSelection(), + selection.getParameters(), null, null, null)) { + final DownloadInfo.Reader reader = new DownloadInfo.Reader(resolver, cursor); + final DownloadInfo info = new DownloadInfo(context); while (cursor.moveToNext()) { - final long id = cursor.getLong(0); - scheduler.cancel((int) id); + reader.updateFromDatabase(info); + scheduler.cancel((int) info.mId); - DownloadStorageProvider.onDownloadProviderDelete(getContext(), id); + DownloadStorageProvider.onDownloadProviderDelete(getContext(), info.mId); - final String path = cursor.getString(1); + final String path = info.mFileName; if (!TextUtils.isEmpty(path)) { try { final File file = new File(path).getCanonicalFile(); @@ -1227,7 +1231,7 @@ public final class DownloadProvider extends ContentProvider { } } - final String mediaUri = cursor.getString(2); + final String mediaUri = info.mMediaProviderUri; if (!TextUtils.isEmpty(mediaUri)) { final long token = Binder.clearCallingIdentity(); try { @@ -1237,6 +1241,9 @@ public final class DownloadProvider extends ContentProvider { Binder.restoreCallingIdentity(token); } } + + // Tell requester that download is finished + info.sendIntentIfRequested(); } } -- cgit v1.2.3 From 2be85cd23d2d56d1b0ea87f0f815ef80693b98c2 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 23 Aug 2016 14:01:11 -0600 Subject: DO NOT MERGE. Update notifications when deleting downloads. Otherwise we end up leaving stale notifications around after the underlying download was deleted. Change-Id: Ie262a9dd369034de6c06be28b0eedc4231ea2e75 Test: none Bug: 30697605 (cherry picked from commit 3b7e099588a2697305fd52c342f404a03ec9a9ab) --- src/com/android/providers/downloads/DownloadProvider.java | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src/com/android/providers/downloads/DownloadProvider.java') diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index d30018f7..95d90dc1 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1248,6 +1248,12 @@ public final class DownloadProvider extends ContentProvider { throw new UnsupportedOperationException("Cannot delete URI: " + uri); } notifyContentChanged(uri, match); + final long token = Binder.clearCallingIdentity(); + try { + Helpers.getDownloadNotifier(getContext()).update(); + } finally { + Binder.restoreCallingIdentity(token); + } return count; } -- cgit v1.2.3 From 7c1af8c62c8bdf6e8de5a00c1927daf9fd9c03d1 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 16 Sep 2016 12:12:17 -0600 Subject: Enforce calling identity before clearing. When opening a downloaded file, enforce that the caller can actually see the requested download before clearing their identity to read internal columns. However, this means that we can no longer return the "my_downloads" paths: if those Uris were shared beyond the app that requested the download, access would be denied. Instead, we need to switch to using "all_downloads" Uris so that permission grants can be issued to third-party viewer apps. Since an app requesting a download doesn't normally have permission to "all_downloads" paths, we issue narrow grants toward the owner of each download, both at device boot and when new downloads are started. Bug: 30537115, 30945409 Change-Id: If944aada020878a91c363963728d0da9f6fae3ea --- .../providers/downloads/DownloadProvider.java | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) (limited to 'src/com/android/providers/downloads/DownloadProvider.java') diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 2d914c41..87084130 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -458,6 +458,19 @@ public final class DownloadProvider extends ContentProvider { if (appInfo != null) { mDefContainerUid = appInfo.uid; } + + // Grant access permissions for all known downloads to the owning apps + final SQLiteDatabase db = mOpenHelper.getReadableDatabase(); + final Cursor cursor = db.query(DB_TABLE, new String[] { + Downloads.Impl._ID, Constants.UID }, null, null, null, null, null); + try { + while (cursor.moveToNext()) { + grantAllDownloadsPermission(cursor.getLong(0), cursor.getInt(1)); + } + } finally { + cursor.close(); + } + // start the DownloadService class. don't wait for the 1st download to be issued. // saves us by getting some initialization code in DownloadService out of the way. Context context = getContext(); @@ -675,6 +688,7 @@ public final class DownloadProvider extends ContentProvider { } insertRequestHeaders(db, rowID, values); + grantAllDownloadsPermission(rowID, Binder.getCallingUid()); notifyContentChanged(uri, match); // Always start service to handle notifications and/or scanning @@ -1166,6 +1180,7 @@ public final class DownloadProvider extends ContentProvider { try { while (cursor.moveToNext()) { final long id = cursor.getLong(0); + revokeAllDownloadsPermission(id); DownloadStorageProvider.onDownloadProviderDelete(getContext(), id); } } finally { @@ -1192,6 +1207,19 @@ public final class DownloadProvider extends ContentProvider { logVerboseOpenFileInfo(uri, mode); } + // Perform normal query to enforce caller identity access before + // clearing it to reach internal-only columns + final Cursor probeCursor = query(uri, new String[] { + Downloads.Impl._DATA }, null, null, null); + try { + if ((probeCursor == null) || (probeCursor.getCount() == 0)) { + throw new FileNotFoundException( + "No file found for " + uri + " as UID " + Binder.getCallingUid()); + } + } finally { + IoUtils.closeQuietly(probeCursor); + } + final Cursor cursor = queryCleared(uri, new String[] { Downloads.Impl._DATA, Downloads.Impl.COLUMN_STATUS, Downloads.Impl.COLUMN_DESTINATION, Downloads.Impl.COLUMN_MEDIA_SCANNED }, null, @@ -1373,4 +1401,20 @@ public final class DownloadProvider extends ContentProvider { to.put(key, defaultValue); } } + + private void grantAllDownloadsPermission(long id, int uid) { + final String[] packageNames = getContext().getPackageManager().getPackagesForUid(uid); + if (packageNames == null || packageNames.length == 0) return; + + // We only need to grant to the first package, since the + // platform internally tracks based on UIDs + final Uri uri = ContentUris.withAppendedId(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, id); + getContext().grantUriPermission(packageNames[0], uri, + Intent.FLAG_GRANT_READ_URI_PERMISSION | Intent.FLAG_GRANT_WRITE_URI_PERMISSION); + } + + private void revokeAllDownloadsPermission(long id) { + final Uri uri = ContentUris.withAppendedId(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, id); + getContext().revokeUriPermission(uri, ~0); + } } -- cgit v1.2.3 From f96f51e8d7386341ca209c795cc6621ab0608d45 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 14 Oct 2016 15:31:37 -0600 Subject: Only send DOWNLOAD_COMPLETE broadcast once. Apps might end up confused if we tell them a download was completed multiple times, so only send the broadcast exactly once when we transition it into a "completed" state, either during an update() or a delete() operation. Test: verified single broadcast with test app Bug: 31619480 Change-Id: I0b9139ea0e37f6d212b84314048692cd0c4f9cdf --- .../providers/downloads/DownloadProvider.java | 35 +++++++++++++++------- 1 file changed, 25 insertions(+), 10 deletions(-) (limited to 'src/com/android/providers/downloads/DownloadProvider.java') diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index b2b2c08d..72975582 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1077,10 +1077,14 @@ public final class DownloadProvider extends ContentProvider { Helpers.validateSelection(where, sAppReadableColumnsSet); - SQLiteDatabase db = mOpenHelper.getWritableDatabase(); + final Context context = getContext(); + final ContentResolver resolver = context.getContentResolver(); + + final SQLiteDatabase db = mOpenHelper.getWritableDatabase(); int count; boolean updateSchedule = false; + boolean isCompleting = false; ContentValues filteredValues; if (Binder.getCallingPid() != Process.myPid()) { @@ -1121,6 +1125,7 @@ public final class DownloadProvider extends ContentProvider { if (isRestart || isUserBypassingSizeLimit) { updateSchedule = true; } + isCompleting = status != null && Downloads.Impl.isStatusCompleted(status); } int match = sURIMatcher.match(uri); @@ -1137,14 +1142,20 @@ public final class DownloadProvider extends ContentProvider { final SqlSelection selection = getWhereClause(uri, where, whereArgs, match); count = db.update(DB_TABLE, filteredValues, selection.getSelection(), selection.getParameters()); - if (updateSchedule) { + if (updateSchedule || isCompleting) { final long token = Binder.clearCallingIdentity(); - try { - try (Cursor cursor = db.query(DB_TABLE, new String[] { _ID }, - selection.getSelection(), selection.getParameters(), - null, null, null)) { - while (cursor.moveToNext()) { - Helpers.scheduleJob(getContext(), cursor.getInt(0)); + try (Cursor cursor = db.query(DB_TABLE, null, selection.getSelection(), + selection.getParameters(), null, null, null)) { + final DownloadInfo.Reader reader = new DownloadInfo.Reader(resolver, + cursor); + final DownloadInfo info = new DownloadInfo(context); + while (cursor.moveToNext()) { + reader.updateFromDatabase(info); + if (updateSchedule) { + Helpers.scheduleJob(context, info); + } + if (isCompleting) { + info.sendIntentIfRequested(); } } } finally { @@ -1257,8 +1268,12 @@ public final class DownloadProvider extends ContentProvider { } } - // Tell requester that download is finished - info.sendIntentIfRequested(); + // If the download wasn't completed yet, we're + // effectively completing it now, and we need to send + // any requested broadcasts + if (!Downloads.Impl.isStatusCompleted(info.mStatus)) { + info.sendIntentIfRequested(); + } } } -- cgit v1.2.3