From 23685b8604571ec623e539f4f9c66db65c9dde81 Mon Sep 17 00:00:00 2001 From: Nate Jiang Date: Thu, 3 Dec 2020 14:31:30 -0800 Subject: [Suggestion] Check foreground user for API call Also, squashes the follow up commit to create a single CL for backporting: ======= PasspointManager: Don't allow bg user to modify passpoint profiles Also, add safety net logging for this bug. ======= Bug: 174749461 Test: atest com.android.server.wifi Change-Id: Ifc79ffeb04a7be99a9c60d9414b72e88275c0514 Merged-In: Ifc79ffeb04a7be99a9c60d9414b72e88275c0514 (cherry picked from commit e799efba85cbe52044a067869af71d9c15b573bb) --- .../com/android/server/wifi/WifiConfigManager.java | 45 ++++++---------------- .../java/com/android/server/wifi/WifiInjector.java | 2 +- .../server/wifi/WifiNetworkSuggestionsManager.java | 29 ++++++++++++-- .../com/android/server/wifi/WifiServiceImpl.java | 13 ++++--- .../server/wifi/hotspot2/PasspointManager.java | 14 ++++++- .../server/wifi/util/WifiPermissionsUtil.java | 29 ++++++++++++++ 6 files changed, 88 insertions(+), 44 deletions(-) (limited to 'service') diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java index 83cabf756..8fc91cb09 100644 --- a/service/java/com/android/server/wifi/WifiConfigManager.java +++ b/service/java/com/android/server/wifi/WifiConfigManager.java @@ -912,32 +912,6 @@ public class WifiConfigManager { && mWifiPermissionsUtil.checkNetworkSettingsPermission(uid); } - /** - * Check if the given UID belongs to the current foreground user. This is - * used to prevent apps running in background users from modifying network - * configurations. - *

- * UIDs belonging to system internals (such as SystemUI) are always allowed, - * since they always run as {@link UserHandle#USER_SYSTEM}. - * - * @param uid uid of the app. - * @return true if the given UID belongs to the current foreground user, - * otherwise false. - */ - private boolean doesUidBelongToCurrentUser(int uid) { - if (uid == android.os.Process.SYSTEM_UID - // UIDs with the NETWORK_SETTINGS permission are always allowed since they are - // acting on behalf of the user. - || mWifiPermissionsUtil.checkNetworkSettingsPermission(uid)) { - return true; - } else { - UserHandle currentUser = UserHandle.of(mCurrentUserId); - UserHandle callingUser = UserHandle.getUserHandleForUid(uid); - return currentUser.equals(callingUser) - || mUserManager.isSameProfileGroup(currentUser, callingUser); - } - } - /** * Copy over public elements from an external WifiConfiguration object to the internal * configuration object if element has been set in the provided external WifiConfiguration. @@ -1333,7 +1307,7 @@ public class WifiConfigManager { */ public NetworkUpdateResult addOrUpdateNetwork(WifiConfiguration config, int uid, @Nullable String packageName) { - if (!doesUidBelongToCurrentUser(uid)) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { Log.e(TAG, "UID " + uid + " not visible to the current user"); return new NetworkUpdateResult(WifiConfiguration.INVALID_NETWORK_ID); } @@ -1439,7 +1413,7 @@ public class WifiConfigManager { * @return true if successful, false otherwise. */ public boolean removeNetwork(int networkId, int uid, String packageName) { - if (!doesUidBelongToCurrentUser(uid)) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { Log.e(TAG, "UID " + uid + " not visible to the current user"); return false; } @@ -1848,7 +1822,7 @@ public class WifiConfigManager { if (mVerboseLoggingEnabled) { Log.v(TAG, "Enabling network " + networkId + " (disableOthers " + disableOthers + ")"); } - if (!doesUidBelongToCurrentUser(uid)) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { Log.e(TAG, "UID " + uid + " not visible to the current user"); return false; } @@ -1886,7 +1860,7 @@ public class WifiConfigManager { if (mVerboseLoggingEnabled) { Log.v(TAG, "Disabling network " + networkId); } - if (!doesUidBelongToCurrentUser(uid)) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { Log.e(TAG, "UID " + uid + " not visible to the current user"); return false; } @@ -1954,7 +1928,7 @@ public class WifiConfigManager { if (mVerboseLoggingEnabled) { Log.v(TAG, "Update network last connect UID for " + networkId); } - if (!doesUidBelongToCurrentUser(uid)) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { Log.e(TAG, "UID " + uid + " not visible to the current user"); return false; } @@ -2926,7 +2900,8 @@ public class WifiConfigManager { Set removedNetworkIds = new HashSet<>(); // Remove any private networks of the old user before switching the userId. for (WifiConfiguration config : getConfiguredNetworks()) { - if ((!config.shared && doesUidBelongToCurrentUser(config.creatorUid)) + if ((!config.shared && !mWifiPermissionsUtil + .doesUidBelongToCurrentUser(config.creatorUid)) || config.ephemeral) { removedNetworkIds.add(config.networkId); localLog("clearInternalUserData: removed config." @@ -3153,7 +3128,8 @@ public class WifiConfigManager { // Migrate the legacy Passpoint configurations owned by the current user to // {@link PasspointManager}. - if (config.isLegacyPasspointConfig && doesUidBelongToCurrentUser(config.creatorUid)) { + if (config.isLegacyPasspointConfig && !mWifiPermissionsUtil + .doesUidBelongToCurrentUser(config.creatorUid)) { legacyPasspointNetId.add(config.networkId); // Migrate the legacy Passpoint configuration and add it to PasspointManager. if (!PasspointManager.addLegacyPasspointConfig(config)) { @@ -3173,7 +3149,8 @@ public class WifiConfigManager { // because all networks were previously stored in a central file. We cannot // write these private networks to the user specific store until the corresponding // user logs in. - if (config.shared || !doesUidBelongToCurrentUser(config.creatorUid)) { + if (config.shared || !mWifiPermissionsUtil + .doesUidBelongToCurrentUser(config.creatorUid)) { sharedConfigurations.add(config); } else { userConfigurations.add(config); diff --git a/service/java/com/android/server/wifi/WifiInjector.java b/service/java/com/android/server/wifi/WifiInjector.java index 993debba5..48f1b3f2e 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -305,7 +305,7 @@ public class WifiInjector { mPasspointManager = new PasspointManager(mContext, this, wifiHandler, mWifiNative, mWifiKeyStore, mClock, new PasspointObjectFactory(), mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager, - mMacAddressUtil); + mMacAddressUtil, mWifiPermissionsUtil); PasspointNetworkNominateHelper nominateHelper = new PasspointNetworkNominateHelper(mPasspointManager, mWifiConfigManager, mConnectivityLocalLog); diff --git a/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java b/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java index 464ced0ad..2632835e6 100644 --- a/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java +++ b/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java @@ -390,6 +390,7 @@ public class WifiNetworkSuggestionsManager { private boolean mIsLastUserApprovalUiDialog = false; private boolean mUserDataLoaded = false; + /** * Listener for app-ops changes for active suggestor apps. */ @@ -833,6 +834,10 @@ public class WifiNetworkSuggestionsManager { public @WifiManager.NetworkSuggestionsStatusCode int add( List networkSuggestions, int uid, String packageName, @Nullable String featureId) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL; + } if (!mUserDataLoaded) { Log.e(TAG, "Add Network suggestion before boot complete is not allowed."); return WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL; @@ -1108,6 +1113,10 @@ public class WifiNetworkSuggestionsManager { */ public @WifiManager.NetworkSuggestionsStatusCode int remove( List networkSuggestions, int uid, String packageName) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL; + } if (!mUserDataLoaded) { Log.e(TAG, "Remove Network suggestion before boot complete is not allowed."); return WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL; @@ -1166,8 +1175,12 @@ public class WifiNetworkSuggestionsManager { * Get all network suggestion for target App * @return List of WifiNetworkSuggestions */ - public @NonNull List get(@NonNull String packageName) { + public @NonNull List get(@NonNull String packageName, int uid) { List networkSuggestionList = new ArrayList<>(); + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return networkSuggestionList; + } if (!mUserDataLoaded) { Log.e(TAG, "Get Network suggestion before boot complete is not allowed."); return networkSuggestionList; @@ -1923,11 +1936,16 @@ public class WifiNetworkSuggestionsManager { * @param binder IBinder instance to allow cleanup if the app dies. * @param listener ISuggestionNetworkCallback instance to add. * @param listenerIdentifier identifier of the listener, should be hash code of listener. + * @param uid uid of the app. * @return true if succeed otherwise false. */ public boolean registerSuggestionConnectionStatusListener(@NonNull IBinder binder, @NonNull ISuggestionConnectionStatusListener listener, - int listenerIdentifier, String packageName) { + int listenerIdentifier, String packageName, int uid) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return false; + } ExternalCallbackTracker listenersTracker = mSuggestionStatusListenerPerApp.get(packageName); if (listenersTracker == null) { @@ -1942,9 +1960,14 @@ public class WifiNetworkSuggestionsManager { /** * Unregister a listener on network connection failure. * @param listenerIdentifier identifier of the listener, should be hash code of listener. + * @param uid uid of the app. */ public void unregisterSuggestionConnectionStatusListener(int listenerIdentifier, - String packageName) { + String packageName, int uid) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return; + } ExternalCallbackTracker listenersTracker = mSuggestionStatusListenerPerApp.get(packageName); if (listenersTracker == null || listenersTracker.remove(listenerIdentifier) == null) { diff --git a/service/java/com/android/server/wifi/WifiServiceImpl.java b/service/java/com/android/server/wifi/WifiServiceImpl.java index 5019f54e8..3b4f39796 100644 --- a/service/java/com/android/server/wifi/WifiServiceImpl.java +++ b/service/java/com/android/server/wifi/WifiServiceImpl.java @@ -3893,13 +3893,15 @@ public class WifiServiceImpl extends BaseWifiService { * @return a list of network suggestions suggested by this app */ public List getNetworkSuggestions(String callingPackageName) { - mAppOps.checkPackage(Binder.getCallingUid(), callingPackageName); + int callingUid = Binder.getCallingUid(); + mAppOps.checkPackage(callingUid, callingPackageName); enforceAccessPermission(); if (mVerboseLoggingEnabled) { mLog.info("getNetworkSuggestionList uid=%").c(Binder.getCallingUid()).flush(); } return mWifiThreadRunner.call(() -> - mWifiNetworkSuggestionsManager.get(callingPackageName), Collections.emptyList()); + mWifiNetworkSuggestionsManager.get(callingPackageName, callingUid), + Collections.emptyList()); } /** @@ -4227,7 +4229,7 @@ public class WifiServiceImpl extends BaseWifiService { mWifiThreadRunner.post(() -> mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(binder, listener, - listenerIdentifier, packageName)); + listenerIdentifier, packageName, uid)); } /** @@ -4237,14 +4239,15 @@ public class WifiServiceImpl extends BaseWifiService { public void unregisterSuggestionConnectionStatusListener( int listenerIdentifier, String packageName) { enforceAccessPermission(); + int uid = Binder.getCallingUid(); if (mVerboseLoggingEnabled) { mLog.info("unregisterSuggestionConnectionStatusListener uid=%") - .c(Binder.getCallingUid()).flush(); + .c(uid).flush(); } mWifiThreadRunner.post(() -> mWifiNetworkSuggestionsManager .unregisterSuggestionConnectionStatusListener(listenerIdentifier, - packageName)); + packageName, uid)); } @Override diff --git a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java index 9cb4254c0..d398759ab 100644 --- a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java +++ b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java @@ -57,6 +57,7 @@ import com.android.server.wifi.hotspot2.anqp.HSOsuProvidersElement; import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.proto.nano.WifiMetricsProto.UserActionEvent; import com.android.server.wifi.util.InformationElementUtil; +import com.android.server.wifi.util.WifiPermissionsUtil; import java.io.IOException; import java.io.PrintWriter; @@ -122,6 +123,7 @@ public class PasspointManager { private final AppOpsManager mAppOps; private final WifiCarrierInfoManager mWifiCarrierInfoManager; private final MacAddressUtil mMacAddressUtil; + private final WifiPermissionsUtil mWifiPermissionsUtil; /** * Map of package name of an app to the app ops changed listener for the app. @@ -309,7 +311,8 @@ public class PasspointManager { WifiConfigStore wifiConfigStore, WifiMetrics wifiMetrics, WifiCarrierInfoManager wifiCarrierInfoManager, - MacAddressUtil macAddressUtil) { + MacAddressUtil macAddressUtil, + WifiPermissionsUtil wifiPermissionsUtil) { mPasspointEventHandler = objectFactory.makePasspointEventHandler(wifiNative, new CallbackHandler(context)); mWifiInjector = wifiInjector; @@ -332,6 +335,7 @@ public class PasspointManager { mAppOps = (AppOpsManager) context.getSystemService(Context.APP_OPS_SERVICE); sPasspointManager = this; mMacAddressUtil = macAddressUtil; + mWifiPermissionsUtil = wifiPermissionsUtil; } /** @@ -412,6 +416,10 @@ public class PasspointManager { Log.e(TAG, "Set isTrusted to false on a non suggestion passpoint is not allowed"); return false; } + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return false; + } mWifiCarrierInfoManager.tryUpdateCarrierIdForPasspoint(config); // Create a provider and install the necessary certificates and keys. @@ -505,6 +513,10 @@ public class PasspointManager { + provider.getCreatorUid()); return false; } + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(callingUid)) { + Log.e(TAG, "UID " + callingUid + " not visible to the current user"); + return false; + } provider.uninstallCertsAndKeys(); String packageName = provider.getPackageName(); // Remove any configs corresponding to the profile in WifiConfigManager. diff --git a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java index 63197ea6d..ebe7ea481 100644 --- a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java +++ b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java @@ -30,6 +30,7 @@ import android.os.Build; import android.os.UserHandle; import android.os.UserManager; import android.provider.Settings; +import android.util.EventLog; import android.util.Log; import com.android.internal.annotations.GuardedBy; @@ -589,4 +590,32 @@ public class WifiPermissionsUtil { if (devicePolicyManager == null) return false; return devicePolicyManager.isProfileOwnerApp(packageName); } + + /** + * Check if the given UID belongs to the current foreground user. This is + * used to prevent apps running in background users from modifying network + * configurations. + *

+ * UIDs belonging to system internals (such as SystemUI) are always allowed, + * since they always run as {@link UserHandle#USER_SYSTEM}. + * + * @param uid uid of the app. + * @return true if the given UID belongs to the current foreground user, + * otherwise false. + */ + public boolean doesUidBelongToCurrentUser(int uid) { + if (uid == android.os.Process.SYSTEM_UID + // UIDs with the NETWORK_SETTINGS permission are always allowed since they are + // acting on behalf of the user. + || checkNetworkSettingsPermission(uid)) { + return true; + } + boolean isCurrentProfile = isCurrentProfile(uid); + if (!isCurrentProfile) { + // Fix for b/174749461 + EventLog.writeEvent(0x534e4554, "174749461", -1, + "Non foreground user trying to modify wifi configuration"); + } + return isCurrentProfile; + } } -- cgit v1.2.3