From ac7285dc1e13f30d59dad30fe2ad1116e5f676cb Mon Sep 17 00:00:00 2001 From: Jeff Davidson Date: Fri, 8 Aug 2014 15:12:47 -0700 Subject: [PATCH] Security-related cleanup for network scoring. -Perform additional checks for the SCORE_NETWORKS permission when broadcasting scoring requests to the active scorer and when accepting score updates. In theory, these checks are unnecessary as we manually check package manager when obtaining the list of valid scorers, but they cannot hurt to add. -Fix multi-user. Since the active scorer is a global setting, we ensure that scoring can only be done by apps available to the primary user / owner of the phone, and that the request scores broadcast is sent to that user's profile. When the scorer is changed, we send that to all user profiles as it's just informational, although it's unlikely that apps outside the primary user's profile would need to respond. Bug: 14117916 Bug: 16399238 Change-Id: Iaf06bda244eec730b590a30a3f4ffab4965bde96 --- core/java/android/net/NetworkScoreManager.java | 6 +++++- core/java/android/net/NetworkScorerAppManager.java | 12 ++++++++++-- .../java/com/android/server/NetworkScoreService.java | 3 ++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/core/java/android/net/NetworkScoreManager.java b/core/java/android/net/NetworkScoreManager.java index 921585321c381..3f68a4431d256 100644 --- a/core/java/android/net/NetworkScoreManager.java +++ b/core/java/android/net/NetworkScoreManager.java @@ -16,6 +16,7 @@ package android.net; +import android.Manifest; import android.annotation.SdkConstant; import android.annotation.SdkConstant.SdkConstantType; import android.annotation.SystemApi; @@ -25,6 +26,7 @@ import android.net.NetworkScorerAppManager.NetworkScorerAppData; import android.os.IBinder; import android.os.RemoteException; import android.os.ServiceManager; +import android.os.UserHandle; /** * Class that manages communication between network subsystems and a network scorer. @@ -238,7 +240,9 @@ public class NetworkScoreManager { intent.setPackage(activeScorer); intent.setFlags(Intent.FLAG_RECEIVER_REGISTERED_ONLY_BEFORE_BOOT); intent.putExtra(EXTRA_NETWORKS_TO_SCORE, networks); - mContext.sendBroadcast(intent); + // A scorer should never become active if its package doesn't hold SCORE_NETWORKS, but + // ensure the package still holds it to be extra safe. + mContext.sendBroadcastAsUser(intent, UserHandle.OWNER, Manifest.permission.SCORE_NETWORKS); return true; } diff --git a/core/java/android/net/NetworkScorerAppManager.java b/core/java/android/net/NetworkScorerAppManager.java index 87a68f739a462..c33f5ecf59c48 100644 --- a/core/java/android/net/NetworkScorerAppManager.java +++ b/core/java/android/net/NetworkScorerAppManager.java @@ -16,6 +16,7 @@ package android.net; +import android.Manifest; import android.Manifest.permission; import android.annotation.Nullable; import android.app.AppOpsManager; @@ -24,6 +25,7 @@ import android.content.Intent; import android.content.pm.ActivityInfo; import android.content.pm.PackageManager; import android.content.pm.ResolveInfo; +import android.os.UserHandle; import android.provider.Settings; import android.text.TextUtils; import android.util.Log; @@ -86,7 +88,9 @@ public final class NetworkScorerAppManager { List scorers = new ArrayList<>(); PackageManager pm = context.getPackageManager(); - List receivers = pm.queryBroadcastReceivers(SCORE_INTENT, 0 /* flags */); + // Only apps installed under the primary user of the device can be scorers. + List receivers = + pm.queryBroadcastReceivers(SCORE_INTENT, 0 /* flags */, UserHandle.USER_OWNER); for (ResolveInfo receiver : receivers) { // This field is a misnomer, see android.content.pm.ResolveInfo#activityInfo final ActivityInfo receiverInfo = receiver.activityInfo; @@ -186,10 +190,14 @@ public final class NetworkScorerAppManager { AppOpsManager appOpsMgr = (AppOpsManager) context.getSystemService(Context.APP_OPS_SERVICE); try { appOpsMgr.checkPackage(callingUid, defaultApp.mPackageName); - return true; } catch (SecurityException e) { return false; } + + // To be extra safe, ensure the caller holds the SCORE_NETWORKS permission. It always + // should, since it couldn't become the active scorer otherwise, but this can't hurt. + return context.checkCallingPermission(Manifest.permission.SCORE_NETWORKS) == + PackageManager.PERMISSION_GRANTED; } /** Returns the {@link NetworkScorerAppData} for the given app, or null if it's not a scorer. */ diff --git a/services/core/java/com/android/server/NetworkScoreService.java b/services/core/java/com/android/server/NetworkScoreService.java index 3e2f260ac8482..ab4d4dca06d4c 100644 --- a/services/core/java/com/android/server/NetworkScoreService.java +++ b/services/core/java/com/android/server/NetworkScoreService.java @@ -29,6 +29,7 @@ import android.net.NetworkScorerAppManager.NetworkScorerAppData; import android.net.ScoredNetwork; import android.os.Binder; import android.os.RemoteException; +import android.os.UserHandle; import android.text.TextUtils; import android.util.Log; @@ -164,7 +165,7 @@ public class NetworkScoreService extends INetworkScoreService.Stub { if (result) { Intent intent = new Intent(NetworkScoreManager.ACTION_SCORER_CHANGED); intent.putExtra(NetworkScoreManager.EXTRA_NEW_SCORER, packageName); - mContext.sendBroadcast(intent); + mContext.sendBroadcastAsUser(intent, UserHandle.ALL); } return result; } finally {