diff options
author | Nate Jiang <qiangjiang@google.com> | 2020-12-03 14:31:30 -0800 |
---|---|---|
committer | Roshan Pius <rpius@google.com> | 2020-12-24 03:54:55 +0000 |
commit | 95673e85133c78773870fb5161bb300b7f2ee51e (patch) | |
tree | 4f1cbb5f3051a155f2f4464eaf032608322d9458 /service | |
parent | 7b701dfe989079784ff1076ce67cb2fb7665b798 (diff) |
[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)
(cherry picked from commit 23685b8604571ec623e539f4f9c66db65c9dde81)
Diffstat (limited to 'service')
7 files changed, 71 insertions, 41 deletions
diff --git a/service/java/com/android/server/wifi/ClientModeImpl.java b/service/java/com/android/server/wifi/ClientModeImpl.java index d98d022b7..12c45e90f 100644 --- a/service/java/com/android/server/wifi/ClientModeImpl.java +++ b/service/java/com/android/server/wifi/ClientModeImpl.java @@ -3678,7 +3678,7 @@ public class ClientModeImpl extends StateMachine { break; case CMD_REMOVE_PASSPOINT_CONFIG: int removeResult = mPasspointManager.removeProvider( - (String) message.obj) ? SUCCESS : FAILURE; + message.sendingUid, (String) message.obj) ? SUCCESS : FAILURE; replyToMessage(message, message.what, removeResult); break; case CMD_GET_PASSPOINT_CONFIGS: @@ -4517,7 +4517,7 @@ public class ClientModeImpl extends StateMachine { break; case CMD_REMOVE_PASSPOINT_CONFIG: String fqdn = (String) message.obj; - if (mPasspointManager.removeProvider(fqdn)) { + if (mPasspointManager.removeProvider(message.sendingUid, fqdn)) { if (isProviderOwnedNetwork(mTargetNetworkId, fqdn) || isProviderOwnedNetwork(mLastNetworkId, fqdn)) { logd("Disconnect from current network since its provider is removed"); diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java index 0e07e086f..00430c80f 100644 --- a/service/java/com/android/server/wifi/WifiConfigManager.java +++ b/service/java/com/android/server/wifi/WifiConfigManager.java @@ -897,28 +897,6 @@ public class WifiConfigManager { } /** - * 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. - * <p> - * 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 || uid == mSystemUiUid) { - return true; - } else { - return WifiConfigurationUtil.doesUidBelongToAnyProfile( - uid, mUserManager.getProfiles(mCurrentUserId)); - } - } - - /** - * Copy over public elements from an external WifiConfiguration object to the internal * configuration object if element has been set in the provided external WifiConfiguration. * The only exception is the hidden |IpConfiguration| parameters, these need to be copied over * for every update. @@ -1285,7 +1263,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); } @@ -1387,7 +1365,7 @@ public class WifiConfigManager { * @return true if successful, false otherwise. */ public boolean removeNetwork(int networkId, int uid) { - if (!doesUidBelongToCurrentUser(uid)) { + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { Log.e(TAG, "UID " + uid + " not visible to the current user"); return false; } @@ -1771,7 +1749,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; } @@ -1809,7 +1787,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; } @@ -1846,7 +1824,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; } @@ -3011,8 +2989,8 @@ public class WifiConfigManager { Set<Integer> removedNetworkIds = new HashSet<>(); // Remove any private networks of the old user before switching the userId. for (WifiConfiguration config : getInternalConfiguredNetworks()) { - if (!config.shared && WifiConfigurationUtil.doesUidBelongToAnyProfile( - config.creatorUid, mUserManager.getProfiles(userId))) { + if (!config.shared && !mWifiPermissionsUtil + .doesUidBelongToCurrentUser(config.creatorUid)) { removedNetworkIds.add(config.networkId); localLog("clearInternalUserData: removed config." + " netId=" + config.networkId @@ -3222,8 +3200,8 @@ public class WifiConfigManager { // Migrate the legacy Passpoint configurations owned by the current user to // {@link PasspointManager}. - if (config.isLegacyPasspointConfig && WifiConfigurationUtil.doesUidBelongToAnyProfile( - config.creatorUid, mUserManager.getProfiles(mCurrentUserId))) { + 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)) { @@ -3240,8 +3218,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 || !WifiConfigurationUtil.doesUidBelongToAnyProfile( - config.creatorUid, mUserManager.getProfiles(mCurrentUserId))) { + 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 9ffca1326..1f89f6df8 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -282,7 +282,7 @@ public class WifiInjector { mPasspointManager = new PasspointManager(mContext, this, new Handler(mWifiCoreHandlerThread.getLooper()), mWifiNative, mWifiKeyStore, mClock, mSimAccessor, new PasspointObjectFactory(), mWifiConfigManager, mWifiConfigStore, - mWifiMetrics, makeTelephonyManager(), subscriptionManager); + mWifiMetrics, makeTelephonyManager(), subscriptionManager, mWifiPermissionsUtil); mPasspointNetworkEvaluator = new PasspointNetworkEvaluator( mPasspointManager, mWifiConfigManager, mConnectivityLocalLog, mCarrierNetworkConfig, this, subscriptionManager); diff --git a/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java b/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java index 8feb3711e..0cb844509 100644 --- a/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java +++ b/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java @@ -557,6 +557,10 @@ public class WifiNetworkSuggestionsManager { */ public @WifiManager.NetworkSuggestionsStatusCode int add( List<WifiNetworkSuggestion> 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 (mVerboseLoggingEnabled) { Log.v(TAG, "Adding " + networkSuggestions.size() + " networks from " + packageName); } @@ -667,7 +671,11 @@ public class WifiNetworkSuggestionsManager { * Remove the provided list of network suggestions from the corresponding app's active list. */ public @WifiManager.NetworkSuggestionsStatusCode int remove( - List<WifiNetworkSuggestion> networkSuggestions, String packageName) { + List<WifiNetworkSuggestion> 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 (mVerboseLoggingEnabled) { Log.v(TAG, "Removing " + networkSuggestions.size() + " networks from " + packageName); } diff --git a/service/java/com/android/server/wifi/WifiServiceImpl.java b/service/java/com/android/server/wifi/WifiServiceImpl.java index a4639924b..eedf2bd75 100644 --- a/service/java/com/android/server/wifi/WifiServiceImpl.java +++ b/service/java/com/android/server/wifi/WifiServiceImpl.java @@ -3314,11 +3314,12 @@ public class WifiServiceImpl extends BaseWifiService { if (mVerboseLoggingEnabled) { mLog.info("removeNetworkSuggestions uid=%").c(Binder.getCallingUid()).flush(); } + int callingUid = Binder.getCallingUid(); Mutable<Integer> success = new Mutable<>(); boolean runWithScissorsSuccess = mWifiInjector.getClientModeImplHandler().runWithScissors( () -> { success.value = mWifiNetworkSuggestionsManager.remove( - networkSuggestions, callingPackageName); + networkSuggestions, callingUid, callingPackageName); }, RUN_WITH_SCISSORS_TIMEOUT_MILLIS); if (!runWithScissorsSuccess) { Log.e(TAG, "Failed to post runnable to remove network suggestions"); diff --git a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java index 8f4349a1f..bdb323be8 100644 --- a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java +++ b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java @@ -59,6 +59,7 @@ import com.android.server.wifi.hotspot2.anqp.NAIRealmElement; import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.util.InformationElementUtil; import com.android.server.wifi.util.TelephonyUtil; +import com.android.server.wifi.util.WifiPermissionsUtil; import java.io.PrintWriter; import java.security.cert.X509Certificate; @@ -117,6 +118,8 @@ public class PasspointManager { private final TelephonyManager mTelephonyManager; private final AppOpsManager mAppOps; private final SubscriptionManager mSubscriptionManager; + private final WifiPermissionsUtil mWifiPermissionsUtil; + /** * Map of package name of an app to the app ops changed listener for the app. @@ -249,7 +252,7 @@ public class PasspointManager { for (Map.Entry<String, PasspointProvider> entry : getPasspointProviderWithPackage( packageName).entrySet()) { String fqdn = entry.getValue().getConfig().getHomeSp().getFqdn(); - removeProvider(fqdn); + removeProvider(Process.WIFI_UID, fqdn); disconnectIfPasspointNetwork(fqdn); } } @@ -297,7 +300,8 @@ public class PasspointManager { PasspointObjectFactory objectFactory, WifiConfigManager wifiConfigManager, WifiConfigStore wifiConfigStore, WifiMetrics wifiMetrics, - TelephonyManager telephonyManager, SubscriptionManager subscriptionManager) { + TelephonyManager telephonyManager, SubscriptionManager subscriptionManager, + WifiPermissionsUtil wifiPermissionsUtil) { mPasspointEventHandler = objectFactory.makePasspointEventHandler(wifiNative, new CallbackHandler(context)); mWifiInjector = wifiInjector; @@ -322,6 +326,7 @@ public class PasspointManager { this, wifiMetrics); mAppOps = (AppOpsManager) context.getSystemService(Context.APP_OPS_SERVICE); sPasspointManager = this; + mWifiPermissionsUtil = wifiPermissionsUtil; } /** @@ -361,6 +366,10 @@ public class PasspointManager { Log.e(TAG, "Invalid configuration"); return false; } + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(uid)) { + Log.e(TAG, "UID " + uid + " not visible to the current user"); + return false; + } // For Hotspot 2.0 Release 1, the CA Certificate must be trusted by one of the pre-loaded // public CAs in the system key store on the device. Since the provisioning method @@ -619,16 +628,21 @@ public class PasspointManager { /** * Remove a Passpoint provider identified by the given FQDN. * + * @param callingUid Calling UID. * @param fqdn The FQDN of the provider to remove * @return true if a provider is removed, false otherwise */ - public boolean removeProvider(String fqdn) { + public boolean removeProvider(int callingUid, String fqdn) { mWifiMetrics.incrementNumPasspointProviderUninstallation(); String packageName; if (!mProviders.containsKey(fqdn)) { Log.e(TAG, "Config doesn't exist"); return false; } + if (!mWifiPermissionsUtil.doesUidBelongToCurrentUser(callingUid)) { + Log.e(TAG, "UID " + callingUid + " not visible to the current user"); + return false; + } mProviders.get(fqdn).uninstallCertsAndKeys(); packageName = mProviders.get(fqdn).getPackageName(); mProviders.remove(fqdn); diff --git a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java index 2834ad765..a08d87673 100644 --- a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java +++ b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java @@ -29,6 +29,7 @@ import android.os.RemoteException; import android.os.UserHandle; import android.os.UserManager; import android.util.Slog; +import android.util.EventLog; import com.android.internal.annotations.GuardedBy; import com.android.server.wifi.WifiInjector; @@ -516,4 +517,32 @@ public class WifiPermissionsUtil { } return mode == AppOpsManager.MODE_ALLOWED; } + + /** + * 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. + * <p> + * 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; + } } |