From a420a460d28b1789429e47322c8177d517874ad4 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) (cherry picked from commit 23685b8604571ec623e539f4f9c66db65c9dde81) --- .../com/android/server/wifi/WifiConfigManager.java | 44 ++++++---------------- .../java/com/android/server/wifi/WifiInjector.java | 2 +- .../server/wifi/WifiNetworkSuggestionsManager.java | 8 ++++ .../server/wifi/hotspot2/PasspointManager.java | 15 +++++++- .../server/wifi/util/WifiPermissionsUtil.java | 29 ++++++++++++++ .../android/server/wifi/WifiConfigManagerTest.java | 18 ++++++++- .../wifi/WifiNetworkSuggestionsManagerTest.java | 4 ++ .../server/wifi/hotspot2/PasspointManagerTest.java | 14 ++++--- 8 files changed, 92 insertions(+), 42 deletions(-) diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java index 8dcd1c19d..393a5c395 100644 --- a/service/java/com/android/server/wifi/WifiConfigManager.java +++ b/service/java/com/android/server/wifi/WifiConfigManager.java @@ -916,28 +916,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. - *

- * 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. @@ -1304,7 +1282,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); } @@ -1406,7 +1384,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; } @@ -1809,7 +1787,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; } @@ -1847,7 +1825,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; } @@ -1884,7 +1862,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; } @@ -3042,8 +3020,8 @@ public class WifiConfigManager { Set 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 @@ -3255,8 +3233,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)) { @@ -3273,8 +3251,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 a234d4d81..11c76aadc 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -290,7 +290,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 426dddb8e..031aec603 100644 --- a/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java +++ b/service/java/com/android/server/wifi/WifiNetworkSuggestionsManager.java @@ -563,6 +563,10 @@ public class WifiNetworkSuggestionsManager { */ public @WifiManager.NetworkSuggestionsStatusCode int add( 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 (mVerboseLoggingEnabled) { Log.v(TAG, "Adding " + networkSuggestions.size() + " networks from " + packageName); } @@ -675,6 +679,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 (mVerboseLoggingEnabled) { Log.v(TAG, "Removing " + networkSuggestions.size() + " networks from " + packageName); } diff --git a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java index 4fce55674..0666943c3 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. @@ -298,7 +301,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; @@ -323,6 +327,7 @@ public class PasspointManager { this, wifiMetrics); mAppOps = (AppOpsManager) context.getSystemService(Context.APP_OPS_SERVICE); sPasspointManager = this; + mWifiPermissionsUtil = wifiPermissionsUtil; } /** @@ -365,6 +370,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 @@ -642,6 +651,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(); packageName = provider.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 b1ceaf37a..ca93b71c0 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; @@ -517,4 +518,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. + *

+ * 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; + } } diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java index 0f48af9cc..f5ad049dd 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java @@ -227,6 +227,7 @@ public class WifiConfigManagerTest { when(mWifiInjector.getMacAddressUtil()).thenReturn(mMacAddressUtil); when(mMacAddressUtil.calculatePersistentMacForConfiguration(any(), any())) .thenReturn(TEST_RANDOMIZED_MAC); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); createWifiConfigManager(); mWifiConfigManager.setOnSavedNetworkUpdateListener(mWcmListener); @@ -3020,6 +3021,8 @@ public class WifiConfigManagerTest { setupStoreDataForUserRead(user2Networks, new HashMap<>()); // Now switch the user to user 2 and ensure that user 1's private network has been removed. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); Set removedNetworks = mWifiConfigManager.handleUserSwitch(user2); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); assertTrue((removedNetworks.size() == 1) && (removedNetworks.contains(user1NetworkId))); @@ -3099,7 +3102,7 @@ public class WifiConfigManagerTest { public void testHandleUserSwitchPushesOtherPrivateNetworksToSharedStore() throws Exception { int user1 = TEST_DEFAULT_USER; int user2 = TEST_DEFAULT_USER + 1; - setupUserProfiles(user2); + setupUserProfiles(user1); int appId = 674; @@ -3131,6 +3134,8 @@ public class WifiConfigManagerTest { } }; setupStoreDataForUserRead(userNetworks, new HashMap<>()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); // Capture the written data for the user 1 and ensure that it corresponds to what was @@ -3145,6 +3150,10 @@ public class WifiConfigManagerTest { // Now switch the user to user2 and ensure that user 2's private network has been moved to // the user store. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(true).thenReturn(false); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false).thenReturn(true); mWifiConfigManager.handleUserSwitch(user2); // Set the expected network list before comparing. user1Network should be in shared data. // Note: In the real world, user1Network will no longer be visible now because it should @@ -3209,6 +3218,8 @@ public class WifiConfigManagerTest { // Unlock the owner of the legacy Passpoint configuration, verify it is removed from // the configured networks (migrated to PasspointManager). setupStoreDataForUserRead(new ArrayList(), new HashMap<>()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(passpointConfig.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); Pair, List> writtenNetworkList = @@ -3336,7 +3347,8 @@ public class WifiConfigManagerTest { // Ensure that we have 2 networks in the database before the stop. assertEquals(2, mWifiConfigManager.getConfiguredNetworks().size()); - + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserStop(user1); // Ensure that we only have 1 shared network in the database after the stop. @@ -3537,6 +3549,8 @@ public class WifiConfigManagerTest { int creatorUid = UserHandle.getUid(user2, 674); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(creatorUid)).thenReturn(false); + // Create a network for user2 try adding it. This should be rejected. final WifiConfiguration user2Network = WifiConfigurationTestUtil.createPskNetwork(); NetworkUpdateResult result = addNetworkToWifiConfigManager(user2Network, creatorUid); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java index dabdfd569..ed5bb39ab 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java @@ -51,6 +51,7 @@ import android.net.wifi.WifiNetworkSuggestion; import android.net.wifi.WifiScanner; import android.os.Handler; import android.os.UserHandle; +import android.os.UserManager; import android.os.test.TestLooper; import android.test.suitebuilder.annotation.SmallTest; @@ -103,6 +104,7 @@ public class WifiNetworkSuggestionsManagerTest { private @Mock ClientModeImpl mClientModeImpl; private @Mock WifiMetrics mWifiMetrics; private @Mock WifiKeyStore mWifiKeyStore; + private @Mock UserManager mUserManager; private TestLooper mLooper; private ArgumentCaptor mAppOpChangedListenerCaptor = ArgumentCaptor.forClass(AppOpsManager.OnOpChangedListener.class); @@ -135,6 +137,7 @@ public class WifiNetworkSuggestionsManagerTest { when(mContext.getSystemService(Context.NOTIFICATION_SERVICE)) .thenReturn(mNotificationManger); when(mContext.getPackageManager()).thenReturn(mPackageManager); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); // setup resource strings for notification. when(mResources.getString(eq(R.string.wifi_suggestion_title), anyString())) @@ -860,6 +863,7 @@ public class WifiNetworkSuggestionsManagerTest { mWifiNetworkSuggestionsManager.add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1)); mWifiNetworkSuggestionsManager.setHasUserApprovedForApp(true, TEST_PACKAGE_1); + mInorder.verify(mWifiPermissionsUtil).doesUidBelongToCurrentUser(eq(TEST_UID_1)); // Simulate connecting to the network. mWifiNetworkSuggestionsManager.handleConnectionAttemptEnded( diff --git a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java index c7d6604eb..618f1c3d5 100644 --- a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java @@ -93,6 +93,7 @@ import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.hotspot2.anqp.eap.EAPMethod; import com.android.server.wifi.util.InformationElementUtil; import com.android.server.wifi.util.InformationElementUtil.RoamingConsortium; +import com.android.server.wifi.util.WifiPermissionsUtil; import org.junit.Before; import org.junit.Test; @@ -176,6 +177,7 @@ public class PasspointManagerTest { @Mock TelephonyManager mTelephonyManager; @Mock TelephonyManager mDataTelephonyManager; @Mock SubscriptionManager mSubscriptionManager; + @Mock WifiPermissionsUtil mWifiPermissionsUtil; Handler mHandler; TestLooper mLooper; @@ -202,11 +204,13 @@ public class PasspointManagerTest { .thenReturn(mPasspointProvisioner); when(mContext.getSystemService(Context.APP_OPS_SERVICE)).thenReturn(mAppOpsManager); when(mWifiInjector.getClientModeImpl()).thenReturn(mClientModeImpl); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); mLooper = new TestLooper(); mHandler = new Handler(mLooper.getLooper()); mManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, - mWifiConfigStore, mWifiMetrics, mTelephonyManager, mSubscriptionManager); + mWifiConfigStore, mWifiMetrics, mTelephonyManager, mSubscriptionManager, + mWifiPermissionsUtil); ArgumentCaptor callbacks = ArgumentCaptor.forClass(PasspointEventHandler.Callbacks.class); verify(mObjectFactory).makePasspointEventHandler(any(WifiNative.class), @@ -1519,7 +1523,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); assertNull(passpointManager.createEphemeralPasspointConfigForCarrier( EAPConstants.EAP_TLS)); @@ -1537,7 +1541,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); PasspointConfiguration result = passpointManager.createEphemeralPasspointConfigForCarrier( @@ -1638,7 +1642,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); assertEquals(EAPConstants.EAP_AKA, passpointManager.findEapMethodFromNAIRealmMatchedWithCarrier(scanDetails)); } finally { @@ -1667,7 +1671,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); assertEquals(-1, passpointManager.findEapMethodFromNAIRealmMatchedWithCarrier(scanDetails)); -- cgit v1.2.3 From 95673e85133c78773870fb5161bb300b7f2ee51e 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) (cherry picked from commit 23685b8604571ec623e539f4f9c66db65c9dde81) --- .../com/android/server/wifi/ClientModeImpl.java | 4 +- .../com/android/server/wifi/WifiConfigManager.java | 44 +++++------------- .../java/com/android/server/wifi/WifiInjector.java | 2 +- .../server/wifi/WifiNetworkSuggestionsManager.java | 10 +++- .../com/android/server/wifi/WifiServiceImpl.java | 3 +- .../server/wifi/hotspot2/PasspointManager.java | 20 ++++++-- .../server/wifi/util/WifiPermissionsUtil.java | 29 ++++++++++++ .../android/server/wifi/ClientModeImplTest.java | 6 +-- .../android/server/wifi/WifiConfigManagerTest.java | 19 +++++++- .../wifi/WifiNetworkSuggestionsManagerTest.java | 54 +++++++++++++++------- .../android/server/wifi/WifiServiceImplTest.java | 9 ++-- .../server/wifi/hotspot2/PasspointManagerTest.java | 20 ++++---- 12 files changed, 145 insertions(+), 75 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. - *

- * 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 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 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 networkSuggestions, String packageName) { + 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 (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 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 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. + *

+ * 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; + } } diff --git a/tests/wifitests/src/com/android/server/wifi/ClientModeImplTest.java b/tests/wifitests/src/com/android/server/wifi/ClientModeImplTest.java index b64bf3de5..aa1ab3001 100644 --- a/tests/wifitests/src/com/android/server/wifi/ClientModeImplTest.java +++ b/tests/wifitests/src/com/android/server/wifi/ClientModeImplTest.java @@ -1811,13 +1811,13 @@ public class ClientModeImplTest { @Test public void syncRemovePasspointConfig() throws Exception { String fqdn = "test.com"; - when(mPasspointManager.removeProvider(fqdn)).thenReturn(true); + when(mPasspointManager.removeProvider(anyInt(), eq(fqdn))).thenReturn(true); mLooper.startAutoDispatch(); assertTrue(mCmi.syncRemovePasspointConfig(mCmiAsyncChannel, fqdn)); mLooper.stopAutoDispatch(); reset(mPasspointManager); - when(mPasspointManager.removeProvider(fqdn)).thenReturn(false); + when(mPasspointManager.removeProvider(anyInt(), eq(fqdn))).thenReturn(false); mLooper.startAutoDispatch(); assertFalse(mCmi.syncRemovePasspointConfig(mCmiAsyncChannel, fqdn)); mLooper.stopAutoDispatch(); @@ -3484,7 +3484,7 @@ public class ClientModeImplTest { @Test public void testRemovePasspointConfig() throws Exception { String fqdn = "test.com"; - when(mPasspointManager.removeProvider(anyString())).thenReturn(true); + when(mPasspointManager.removeProvider(anyInt(), anyString())).thenReturn(true); // switch to connect mode and verify wifi is reported as enabled startSupplicantAndDispatchMessages(); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java index 7f10214be..0571ba033 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java @@ -215,6 +215,8 @@ public class WifiConfigManagerTest { when(mWifiInjector.getWifiLastResortWatchdog().shouldIgnoreSsidUpdate()) .thenReturn(false); when(mWifiInjector.getCarrierNetworkConfig()).thenReturn(mCarrierNetworkConfig); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); + createWifiConfigManager(); mWifiConfigManager.setOnSavedNetworkUpdateListener(mWcmListener); ArgumentCaptor observerCaptor = @@ -2969,6 +2971,8 @@ public class WifiConfigManagerTest { setupStoreDataForUserRead(user2Networks, new HashMap<>()); // Now switch the user to user 2 and ensure that user 1's private network has been removed. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); Set removedNetworks = mWifiConfigManager.handleUserSwitch(user2); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); assertTrue((removedNetworks.size() == 1) && (removedNetworks.contains(user1NetworkId))); @@ -3048,7 +3052,7 @@ public class WifiConfigManagerTest { public void testHandleUserSwitchPushesOtherPrivateNetworksToSharedStore() throws Exception { int user1 = TEST_DEFAULT_USER; int user2 = TEST_DEFAULT_USER + 1; - setupUserProfiles(user2); + setupUserProfiles(user1); int appId = 674; @@ -3080,6 +3084,8 @@ public class WifiConfigManagerTest { } }; setupStoreDataForUserRead(userNetworks, new HashMap<>()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); // Capture the written data for the user 1 and ensure that it corresponds to what was @@ -3094,6 +3100,10 @@ public class WifiConfigManagerTest { // Now switch the user to user2 and ensure that user 2's private network has been moved to // the user store. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(true).thenReturn(false); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false).thenReturn(true); mWifiConfigManager.handleUserSwitch(user2); // Set the expected network list before comparing. user1Network should be in shared data. // Note: In the real world, user1Network will no longer be visible now because it should @@ -3158,6 +3168,8 @@ public class WifiConfigManagerTest { // Unlock the owner of the legacy Passpoint configuration, verify it is removed from // the configured networks (migrated to PasspointManager). setupStoreDataForUserRead(new ArrayList(), new HashMap<>()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(passpointConfig.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); Pair, List> writtenNetworkList = @@ -3285,7 +3297,8 @@ public class WifiConfigManagerTest { // Ensure that we have 2 networks in the database before the stop. assertEquals(2, mWifiConfigManager.getConfiguredNetworks().size()); - + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserStop(user1); // Ensure that we only have 1 shared network in the database after the stop. @@ -3486,6 +3499,8 @@ public class WifiConfigManagerTest { int creatorUid = UserHandle.getUid(user2, 674); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(creatorUid)).thenReturn(false); + // Create a network for user2 try adding it. This should be rejected. final WifiConfiguration user2Network = WifiConfigurationTestUtil.createPskNetwork(); NetworkUpdateResult result = addNetworkToWifiConfigManager(user2Network, creatorUid); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java index ed3e6d07f..bb31dd1b7 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java @@ -50,6 +50,7 @@ import android.net.wifi.WifiManager; import android.net.wifi.WifiNetworkSuggestion; import android.os.Handler; import android.os.UserHandle; +import android.os.UserManager; import android.os.test.TestLooper; import android.test.suitebuilder.annotation.SmallTest; @@ -103,6 +104,7 @@ public class WifiNetworkSuggestionsManagerTest { private @Mock ClientModeImpl mClientModeImpl; private @Mock WifiMetrics mWifiMetrics; private @Mock WifiKeyStore mWifiKeyStore; + private @Mock UserManager mUserManager; private TestLooper mLooper; private ArgumentCaptor mAppOpChangedListenerCaptor = ArgumentCaptor.forClass(AppOpsManager.OnOpChangedListener.class); @@ -135,6 +137,7 @@ public class WifiNetworkSuggestionsManagerTest { when(mContext.getSystemService(Context.NOTIFICATION_SERVICE)) .thenReturn(mNotificationManger); when(mContext.getPackageManager()).thenReturn(mPackageManager); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); // setup resource strings for notification. when(mResources.getString(eq(R.string.wifi_suggestion_title), anyString())) @@ -252,9 +255,11 @@ public class WifiNetworkSuggestionsManagerTest { // Now remove all of them. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_UID_1, + TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_PACKAGE_2)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_UID_2, + TEST_PACKAGE_2)); assertTrue(mWifiNetworkSuggestionsManager.getAllNetworkSuggestions().isEmpty()); @@ -296,9 +301,11 @@ public class WifiNetworkSuggestionsManagerTest { // Now remove all of them by sending an empty list. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(new ArrayList<>(), TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(new ArrayList<>(), TEST_UID_1, + TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(new ArrayList<>(), TEST_PACKAGE_2)); + mWifiNetworkSuggestionsManager.remove(new ArrayList<>(), TEST_UID_2, + TEST_PACKAGE_2)); assertTrue(mWifiNetworkSuggestionsManager.getAllNetworkSuggestions().isEmpty()); } @@ -334,7 +341,7 @@ public class WifiNetworkSuggestionsManagerTest { removingSuggestion.wifiConfiguration.SSID = networkSuggestion1.wifiConfiguration.SSID; assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, mWifiNetworkSuggestionsManager.remove(Arrays.asList(removingSuggestion), - TEST_PACKAGE_1)); + TEST_UID_1, TEST_PACKAGE_1)); verify(mWifiKeyStore).removeKeys(any()); } /** @@ -355,7 +362,8 @@ public class WifiNetworkSuggestionsManagerTest { mWifiNetworkSuggestionsManager.add(networkSuggestionList1, TEST_UID_1, TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_UID_1, + TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, mWifiNetworkSuggestionsManager.add(networkSuggestionList1, TEST_UID_1, TEST_PACKAGE_1)); @@ -433,7 +441,8 @@ public class WifiNetworkSuggestionsManagerTest { } // The remove should succeed. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_UID_1, + TEST_PACKAGE_1)); // Now add 2 more. networkSuggestionList = new ArrayList<>(); @@ -473,7 +482,8 @@ public class WifiNetworkSuggestionsManagerTest { TEST_PACKAGE_1)); // Remove should fail because the network list is different. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_REMOVE_INVALID, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_UID_1, + TEST_PACKAGE_1)); } /** @@ -802,7 +812,8 @@ public class WifiNetworkSuggestionsManagerTest { // remove the suggestion & ensure lookup fails. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(Collections.EMPTY_LIST, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(Collections.EMPTY_LIST, TEST_UID_1, + TEST_PACKAGE_1)); assertNull(mWifiNetworkSuggestionsManager.getNetworkSuggestionsForScanDetail(scanDetail)); } @@ -850,6 +861,7 @@ public class WifiNetworkSuggestionsManagerTest { mWifiNetworkSuggestionsManager.add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1)); mWifiNetworkSuggestionsManager.setHasUserApprovedForApp(true, TEST_PACKAGE_1); + mInorder.verify(mWifiPermissionsUtil).doesUidBelongToCurrentUser(eq(TEST_UID_1)); // Simulate connecting to the network. mWifiNetworkSuggestionsManager.handleConnectionAttemptEnded( @@ -1241,7 +1253,8 @@ public class WifiNetworkSuggestionsManagerTest { mWifiNetworkSuggestionsManager.add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_UID_1, + TEST_PACKAGE_1)); // Verify config store interactions. verify(mWifiConfigManager, times(2)).saveToStore(true); @@ -1373,7 +1386,8 @@ public class WifiNetworkSuggestionsManagerTest { // Now remove the network suggestion and ensure we did not trigger a disconnect. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_UID_1, + TEST_PACKAGE_1)); verify(mClientModeImpl, never()).disconnectCommand(); } @@ -1520,10 +1534,12 @@ public class WifiNetworkSuggestionsManagerTest { // Now remove first add, nothing happens. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_UID_1, + TEST_PACKAGE_1)); // Stop watching app-ops changes on last remove. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_UID_1, + TEST_PACKAGE_1)); assertTrue(mWifiNetworkSuggestionsManager.getAllNetworkSuggestions().isEmpty()); mInorder.verify(mAppOpsManager).stopWatchingMode(mAppOpChangedListenerCaptor.getValue()); @@ -1702,9 +1718,11 @@ public class WifiNetworkSuggestionsManagerTest { // Remove all suggestions from TEST_PACKAGE_1 & TEST_PACKAGE_2, we should continue to track. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_UID_1, + TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_PACKAGE_2)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_UID_2, + TEST_PACKAGE_2)); assertTrue(mDataSource.hasNewDataToSerialize()); Map networkSuggestionsMapToWrite = mDataSource.toSerialize(); @@ -1766,9 +1784,11 @@ public class WifiNetworkSuggestionsManagerTest { // Remove all suggestions from TEST_PACKAGE_1 & TEST_PACKAGE_2, we should continue to track. assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_PACKAGE_1)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList1, TEST_UID_1, + TEST_PACKAGE_1)); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, - mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_PACKAGE_2)); + mWifiNetworkSuggestionsManager.remove(networkSuggestionList2, TEST_UID_2, + TEST_PACKAGE_2)); assertTrue(mDataSource.hasNewDataToSerialize()); Map networkSuggestionsMapToWrite = mDataSource.toSerialize(); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java b/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java index 378d6cf2e..d7da0caa1 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java @@ -3819,7 +3819,7 @@ public class WifiServiceImplTest { mWifiServiceImpl.addNetworkSuggestions(mock(List.class), TEST_PACKAGE_NAME)); verify(mWifiNetworkSuggestionsManager, times(2)).add( - any(), eq(Binder.getCallingUid()), eq(TEST_PACKAGE_NAME)); + any(), eq(Binder.getCallingUid()), eq(TEST_PACKAGE_NAME)); } /** @@ -3830,12 +3830,12 @@ public class WifiServiceImplTest { public void testRemoveNetworkSuggestions() { setupClientModeImplHandlerForRunWithScissors(); - when(mWifiNetworkSuggestionsManager.remove(any(), anyString())) + when(mWifiNetworkSuggestionsManager.remove(any(), anyInt(), anyString())) .thenReturn(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_REMOVE_INVALID); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_REMOVE_INVALID, mWifiServiceImpl.removeNetworkSuggestions(mock(List.class), TEST_PACKAGE_NAME)); - when(mWifiNetworkSuggestionsManager.remove(any(), anyString())) + when(mWifiNetworkSuggestionsManager.remove(any(), anyInt(), anyString())) .thenReturn(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, mWifiServiceImpl.removeNetworkSuggestions(mock(List.class), TEST_PACKAGE_NAME)); @@ -3845,7 +3845,8 @@ public class WifiServiceImplTest { assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL, mWifiServiceImpl.removeNetworkSuggestions(mock(List.class), TEST_PACKAGE_NAME)); - verify(mWifiNetworkSuggestionsManager, times(2)).remove(any(), eq(TEST_PACKAGE_NAME)); + verify(mWifiNetworkSuggestionsManager, times(2)).remove( + any(), eq(Binder.getCallingUid()), eq(TEST_PACKAGE_NAME)); } /** diff --git a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java index 013c11fe5..e4b1622df 100644 --- a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java @@ -93,6 +93,7 @@ import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.hotspot2.anqp.eap.EAPMethod; import com.android.server.wifi.util.InformationElementUtil; import com.android.server.wifi.util.InformationElementUtil.RoamingConsortium; +import com.android.server.wifi.util.WifiPermissionsUtil; import org.junit.Before; import org.junit.Test; @@ -176,6 +177,7 @@ public class PasspointManagerTest { @Mock TelephonyManager mTelephonyManager; @Mock TelephonyManager mDataTelephonyManager; @Mock SubscriptionManager mSubscriptionManager; + @Mock WifiPermissionsUtil mWifiPermissionsUtil; Handler mHandler; TestLooper mLooper; @@ -202,11 +204,13 @@ public class PasspointManagerTest { .thenReturn(mPasspointProvisioner); when(mContext.getSystemService(Context.APP_OPS_SERVICE)).thenReturn(mAppOpsManager); when(mWifiInjector.getClientModeImpl()).thenReturn(mClientModeImpl); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); mLooper = new TestLooper(); mHandler = new Handler(mLooper.getLooper()); mManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, - mWifiConfigStore, mWifiMetrics, mTelephonyManager, mSubscriptionManager); + mWifiConfigStore, mWifiMetrics, mTelephonyManager, mSubscriptionManager, + mWifiPermissionsUtil); ArgumentCaptor callbacks = ArgumentCaptor.forClass(PasspointEventHandler.Callbacks.class); verify(mObjectFactory).makePasspointEventHandler(any(WifiNative.class), @@ -538,7 +542,7 @@ public class PasspointManagerTest { assertEquals(1, mSharedDataSource.getProviderIndex()); // Remove the provider. - assertTrue(mManager.removeProvider(TEST_FQDN)); + assertTrue(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(provider).uninstallCertsAndKeys(); verify(mWifiConfigManager).saveToStore(true); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); @@ -581,7 +585,7 @@ public class PasspointManagerTest { assertEquals(1, mSharedDataSource.getProviderIndex()); // Remove the provider. - assertTrue(mManager.removeProvider(TEST_FQDN)); + assertTrue(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(provider).uninstallCertsAndKeys(); verify(mWifiConfigManager).saveToStore(true); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); @@ -710,7 +714,7 @@ public class PasspointManagerTest { */ @Test public void removeNonExistingProvider() throws Exception { - assertFalse(mManager.removeProvider(TEST_FQDN)); + assertFalse(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); verify(mWifiMetrics, never()).incrementNumPasspointProviderUninstallSuccess(); } @@ -1517,7 +1521,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); assertNull(passpointManager.createEphemeralPasspointConfigForCarrier( EAPConstants.EAP_TLS)); @@ -1535,7 +1539,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); PasspointConfiguration result = passpointManager.createEphemeralPasspointConfigForCarrier( @@ -1636,7 +1640,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); assertEquals(EAPConstants.EAP_AKA, passpointManager.findEapMethodFromNAIRealmMatchedWithCarrier(scanDetails)); } finally { @@ -1665,7 +1669,7 @@ public class PasspointManagerTest { PasspointManager passpointManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mTelephonyManager, - mSubscriptionManager); + mSubscriptionManager, mWifiPermissionsUtil); assertEquals(-1, passpointManager.findEapMethodFromNAIRealmMatchedWithCarrier(scanDetails)); -- cgit v1.2.3 From e26f3b9ffe1c862c8c8322a6ae7b03ce8f9900f9 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) (cherry picked from commit 23685b8604571ec623e539f4f9c66db65c9dde81) --- .../com/android/server/wifi/WifiConfigManager.java | 44 ++++++---------------- .../java/com/android/server/wifi/WifiInjector.java | 2 +- .../com/android/server/wifi/WifiStateMachine.java | 4 +- .../server/wifi/hotspot2/PasspointManager.java | 17 +++++++-- .../server/wifi/util/WifiPermissionsUtil.java | 29 ++++++++++++++ .../android/server/wifi/WifiConfigManagerTest.java | 18 ++++++++- .../android/server/wifi/WifiStateMachineTest.java | 4 +- .../server/wifi/hotspot2/PasspointManagerTest.java | 12 +++--- 8 files changed, 82 insertions(+), 48 deletions(-) diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java index c06e80239..28ac2056b 100644 --- a/service/java/com/android/server/wifi/WifiConfigManager.java +++ b/service/java/com/android/server/wifi/WifiConfigManager.java @@ -743,28 +743,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. - *

- * 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. @@ -1097,7 +1075,7 @@ public class WifiConfigManager { * @return NetworkUpdateResult object representing status of the update. */ public NetworkUpdateResult addOrUpdateNetwork(WifiConfiguration config, int uid) { - 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); } @@ -1173,7 +1151,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; } @@ -1510,7 +1488,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; } @@ -1545,7 +1523,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; } @@ -1580,7 +1558,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; } @@ -2663,8 +2641,8 @@ public class WifiConfigManager { Set 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 @@ -2886,8 +2864,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)) { @@ -2904,8 +2882,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 89a7f45b4..cd07187dc 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -245,7 +245,7 @@ public class WifiInjector { mSimAccessor = new SIMAccessor(mContext); mPasspointManager = new PasspointManager(mContext, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, new PasspointObjectFactory(), mWifiConfigManager, mWifiConfigStore, - mWifiMetrics); + mWifiMetrics, mWifiPermissionsUtil); mPasspointNetworkEvaluator = new PasspointNetworkEvaluator( mPasspointManager, mWifiConfigManager, mConnectivityLocalLog); mWifiMetrics.setPasspointManager(mPasspointManager); diff --git a/service/java/com/android/server/wifi/WifiStateMachine.java b/service/java/com/android/server/wifi/WifiStateMachine.java index 390a10238..dff2d6077 100644 --- a/service/java/com/android/server/wifi/WifiStateMachine.java +++ b/service/java/com/android/server/wifi/WifiStateMachine.java @@ -3503,7 +3503,7 @@ public class WifiStateMachine 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: @@ -4375,7 +4375,7 @@ public class WifiStateMachine 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/hotspot2/PasspointManager.java b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java index 988dd6642..5d3ffe3f7 100644 --- a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java +++ b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java @@ -41,6 +41,7 @@ import com.android.server.wifi.hotspot2.anqp.HSOsuProvidersElement; import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.util.InformationElementUtil; import com.android.server.wifi.util.ScanResultUtil; +import com.android.server.wifi.util.WifiPermissionsUtil; import java.io.PrintWriter; import java.util.ArrayList; @@ -88,6 +89,7 @@ public class PasspointManager { private final CertificateVerifier mCertVerifier; private final WifiMetrics mWifiMetrics; private final PasspointProvisioner mPasspointProvisioner; + private final WifiPermissionsUtil mWifiPermissionsUtil; // Counter used for assigning unique identifier to each provider. private long mProviderIndex; @@ -160,7 +162,7 @@ public class PasspointManager { public PasspointManager(Context context, WifiNative wifiNative, WifiKeyStore keyStore, Clock clock, SIMAccessor simAccessor, PasspointObjectFactory objectFactory, WifiConfigManager wifiConfigManager, WifiConfigStore wifiConfigStore, - WifiMetrics wifiMetrics) { + WifiMetrics wifiMetrics, WifiPermissionsUtil wifiPermissionsUtil) { mHandler = objectFactory.makePasspointEventHandler(wifiNative, new CallbackHandler(context)); mKeyStore = keyStore; @@ -177,6 +179,7 @@ public class PasspointManager { mKeyStore, mSimAccessor, new DataSourceHandler())); mPasspointProvisioner = objectFactory.makePasspointProvisioner(context); sPasspointManager = this; + mWifiPermissionsUtil = wifiPermissionsUtil; } /** @@ -214,6 +217,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 @@ -257,16 +264,20 @@ 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(); 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(); mProviders.remove(fqdn); mWifiConfigManager.saveToStore(true /* forceWrite */); diff --git a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java index 1a85c28ae..175b2a695 100644 --- a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java +++ b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java @@ -24,6 +24,7 @@ import android.content.pm.UserInfo; import android.os.RemoteException; import android.os.UserManager; import android.provider.Settings; +import android.util.EventLog; import com.android.server.wifi.WifiInjector; import com.android.server.wifi.WifiLog; @@ -296,4 +297,32 @@ public class WifiPermissionsUtil { android.Manifest.permission.NETWORK_SETUP_WIZARD, uid) == PackageManager.PERMISSION_GRANTED; } + + /** + * 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; + } } diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java index ca1b4a07c..dcc3303d1 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java @@ -184,6 +184,7 @@ public class WifiConfigManagerTest { when(mWifiPermissionsUtil.checkNetworkSettingsPermission(anyInt())).thenReturn(true); when(mWifiPermissionsWrapper.getDevicePolicyManagerInternal()) .thenReturn(mDevicePolicyManagerInternal); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); createWifiConfigManager(); mWifiConfigManager.setOnSavedNetworkUpdateListener(mWcmListener); } @@ -2253,6 +2254,8 @@ public class WifiConfigManagerTest { setupStoreDataForUserRead(user2Networks, new HashSet()); // Now switch the user to user 2 and ensure that user 1's private network has been removed. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); Set removedNetworks = mWifiConfigManager.handleUserSwitch(user2); verify(mWifiConfigStore).switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)); assertTrue((removedNetworks.size() == 1) && (removedNetworks.contains(user1NetworkId))); @@ -2333,7 +2336,7 @@ public class WifiConfigManagerTest { public void testHandleUserSwitchPushesOtherPrivateNetworksToSharedStore() throws Exception { int user1 = TEST_DEFAULT_USER; int user2 = TEST_DEFAULT_USER + 1; - setupUserProfiles(user2); + setupUserProfiles(user1); int appId = 674; @@ -2366,6 +2369,8 @@ public class WifiConfigManagerTest { } }; setupStoreDataForUserRead(userNetworks, new HashSet()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)); // Capture the written data for the user 1 and ensure that it corresponds to what was @@ -2380,6 +2385,10 @@ public class WifiConfigManagerTest { // Now switch the user to user2 and ensure that user 2's private network has been moved to // the user store. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(true).thenReturn(false); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false).thenReturn(true); mWifiConfigManager.handleUserSwitch(user2); // Set the expected network list before comparing. user1Network should be in shared data. // Note: In the real world, user1Network will no longer be visible now because it should @@ -2445,6 +2454,8 @@ public class WifiConfigManagerTest { // Unlock the owner of the legacy Passpoint configuration, verify it is removed from // the configured networks (migrated to PasspointManager). setupStoreDataForUserRead(new ArrayList(), new HashSet()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(passpointConfig.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)); Pair, List> writtenNetworkList = @@ -2572,7 +2583,8 @@ public class WifiConfigManagerTest { // Ensure that we have 2 networks in the database before the stop. assertEquals(2, mWifiConfigManager.getConfiguredNetworks().size()); - + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserStop(user1); // Ensure that we only have 1 shared network in the database after the stop. @@ -2701,6 +2713,8 @@ public class WifiConfigManagerTest { int creatorUid = UserHandle.getUid(user2, 674); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(creatorUid)).thenReturn(false); + // Create a network for user2 try adding it. This should be rejected. final WifiConfiguration user2Network = WifiConfigurationTestUtil.createPskNetwork(); NetworkUpdateResult result = addNetworkToWifiConfigManager(user2Network, creatorUid); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java b/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java index 8154a02b9..1a5e8c1a1 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java @@ -1541,13 +1541,13 @@ public class WifiStateMachineTest { @Test public void syncRemovePasspointConfig() throws Exception { String fqdn = "test.com"; - when(mPasspointManager.removeProvider(fqdn)).thenReturn(true); + when(mPasspointManager.removeProvider(anyInt(), eq(fqdn))).thenReturn(true); mLooper.startAutoDispatch(); assertTrue(mWsm.syncRemovePasspointConfig(mWsmAsyncChannel, fqdn)); mLooper.stopAutoDispatch(); reset(mPasspointManager); - when(mPasspointManager.removeProvider(fqdn)).thenReturn(false); + when(mPasspointManager.removeProvider(anyInt(), eq(fqdn))).thenReturn(false); mLooper.startAutoDispatch(); assertFalse(mWsm.syncRemovePasspointConfig(mWsmAsyncChannel, fqdn)); mLooper.stopAutoDispatch(); diff --git a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java index 284c7f9b9..2051bdfa2 100644 --- a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java @@ -72,6 +72,7 @@ import com.android.server.wifi.hotspot2.anqp.I18Name; import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.util.InformationElementUtil.RoamingConsortium; import com.android.server.wifi.util.ScanResultUtil; +import com.android.server.wifi.util.WifiPermissionsUtil; import org.junit.Before; import org.junit.Test; @@ -131,7 +132,7 @@ public class PasspointManagerTest { @Mock IProvisioningCallback mCallback; @Mock WfaKeyStore mWfaKeyStore; @Mock KeyStore mKeyStore; - + @Mock WifiPermissionsUtil mWifiPermissionsUtil; TestLooper mLooper; PasspointManager mManager; @@ -152,7 +153,8 @@ public class PasspointManagerTest { when(mObjectFactory.makePasspointProvisioner(any(Context.class))) .thenReturn(mPasspointProvisioner); mManager = new PasspointManager(mContext, mWifiNative, mWifiKeyStore, mClock, - mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics); + mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, + mWifiPermissionsUtil); ArgumentCaptor callbacks = ArgumentCaptor.forClass(PasspointEventHandler.Callbacks.class); verify(mObjectFactory).makePasspointEventHandler(any(WifiNative.class), @@ -383,7 +385,7 @@ public class PasspointManagerTest { assertEquals(1, mDataSource.getProviderIndex()); // Remove the provider. - assertTrue(mManager.removeProvider(TEST_FQDN)); + assertTrue(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(provider).uninstallCertsAndKeys(); verify(mWifiConfigManager).saveToStore(true); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); @@ -423,7 +425,7 @@ public class PasspointManagerTest { assertEquals(1, mDataSource.getProviderIndex()); // Remove the provider. - assertTrue(mManager.removeProvider(TEST_FQDN)); + assertTrue(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(provider).uninstallCertsAndKeys(); verify(mWifiConfigManager).saveToStore(true); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); @@ -543,7 +545,7 @@ public class PasspointManagerTest { */ @Test public void removeNonExistingProvider() throws Exception { - assertFalse(mManager.removeProvider(TEST_FQDN)); + assertFalse(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); verify(mWifiMetrics, never()).incrementNumPasspointProviderUninstallSuccess(); } -- cgit v1.2.3 From e22dc22f6c61fa374169a6c9927c9abf47fbf9ee 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) (cherry picked from commit 23685b8604571ec623e539f4f9c66db65c9dde81) --- .../com/android/server/wifi/WifiConfigManager.java | 44 ++++++---------------- .../java/com/android/server/wifi/WifiInjector.java | 2 +- .../com/android/server/wifi/WifiStateMachine.java | 4 +- .../server/wifi/hotspot2/PasspointManager.java | 17 +++++++-- .../server/wifi/util/WifiPermissionsUtil.java | 29 ++++++++++++++ .../android/server/wifi/WifiConfigManagerTest.java | 18 ++++++++- .../android/server/wifi/WifiStateMachineTest.java | 4 +- .../server/wifi/hotspot2/PasspointManagerTest.java | 11 ++++-- 8 files changed, 82 insertions(+), 47 deletions(-) diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java index bac395343..eacc8e5c4 100644 --- a/service/java/com/android/server/wifi/WifiConfigManager.java +++ b/service/java/com/android/server/wifi/WifiConfigManager.java @@ -720,28 +720,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. - *

- * 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. @@ -1072,7 +1050,7 @@ public class WifiConfigManager { * @return NetworkUpdateResult object representing status of the update. */ public NetworkUpdateResult addOrUpdateNetwork(WifiConfiguration config, int uid) { - 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); } @@ -1145,7 +1123,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; } @@ -1478,7 +1456,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; } @@ -1513,7 +1491,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; } @@ -1553,7 +1531,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; } @@ -2618,8 +2596,8 @@ public class WifiConfigManager { Set 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 @@ -2794,8 +2772,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)) { @@ -2812,8 +2790,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 4b8e6829e..c6d294377 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -213,7 +213,7 @@ public class WifiInjector { mSimAccessor = new SIMAccessor(mContext); mPasspointManager = new PasspointManager(mContext, mWifiNative, mWifiKeyStore, mClock, mSimAccessor, new PasspointObjectFactory(), mWifiConfigManager, mWifiConfigStore, - mWifiMetrics); + mWifiMetrics, mWifiPermissionsUtil); mPasspointNetworkEvaluator = new PasspointNetworkEvaluator( mPasspointManager, mWifiConfigManager, mConnectivityLocalLog); mWifiMetrics.setPasspointManager(mPasspointManager); diff --git a/service/java/com/android/server/wifi/WifiStateMachine.java b/service/java/com/android/server/wifi/WifiStateMachine.java index ac7d748bb..fda14accb 100644 --- a/service/java/com/android/server/wifi/WifiStateMachine.java +++ b/service/java/com/android/server/wifi/WifiStateMachine.java @@ -4094,7 +4094,7 @@ public class WifiStateMachine extends StateMachine implements WifiNative.WifiRss 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: @@ -5481,7 +5481,7 @@ public class WifiStateMachine extends StateMachine implements WifiNative.WifiRss 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/hotspot2/PasspointManager.java b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java index 5d79ba40a..46ec1a7ea 100644 --- a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java +++ b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java @@ -53,6 +53,7 @@ import com.android.server.wifi.hotspot2.anqp.HSOsuProvidersElement; import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.util.InformationElementUtil; import com.android.server.wifi.util.ScanResultUtil; +import com.android.server.wifi.util.WifiPermissionsUtil; import java.io.PrintWriter; import java.util.ArrayList; @@ -99,6 +100,7 @@ public class PasspointManager { private final WifiConfigManager mWifiConfigManager; private final CertificateVerifier mCertVerifier; private final WifiMetrics mWifiMetrics; + private final WifiPermissionsUtil mWifiPermissionsUtil; // Counter used for assigning unique identifier to each provider. private long mProviderIndex; @@ -197,7 +199,7 @@ public class PasspointManager { public PasspointManager(Context context, WifiNative wifiNative, WifiKeyStore keyStore, Clock clock, SIMAccessor simAccessor, PasspointObjectFactory objectFactory, WifiConfigManager wifiConfigManager, WifiConfigStore wifiConfigStore, - WifiMetrics wifiMetrics) { + WifiMetrics wifiMetrics, WifiPermissionsUtil wifiPermissionsUtil) { mHandler = objectFactory.makePasspointEventHandler(wifiNative, new CallbackHandler(context)); mKeyStore = keyStore; @@ -213,6 +215,7 @@ public class PasspointManager { wifiConfigStore.registerStoreData(objectFactory.makePasspointConfigStoreData( mKeyStore, mSimAccessor, new DataSourceHandler())); sPasspointManager = this; + mWifiPermissionsUtil = wifiPermissionsUtil; } /** @@ -235,6 +238,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 @@ -278,16 +285,20 @@ 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(); 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(); mProviders.remove(fqdn); mWifiConfigManager.saveToStore(true /* forceWrite */); diff --git a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java index 52c96183e..debb6e593 100644 --- a/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java +++ b/service/java/com/android/server/wifi/util/WifiPermissionsUtil.java @@ -32,6 +32,7 @@ import android.os.RemoteException; import android.os.UserManager; import android.provider.Settings; import android.text.TextUtils; +import android.util.EventLog; import com.android.server.wifi.FrameworkFacade; import com.android.server.wifi.WifiInjector; @@ -392,4 +393,32 @@ public class WifiPermissionsUtil { android.Manifest.permission.NETWORK_SETTINGS, uid) == PackageManager.PERMISSION_GRANTED; } + + /** + * 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; + } } diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java index c99a1496a..8990f92c1 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java @@ -177,6 +177,7 @@ public class WifiConfigManagerTest { when(mWifiPermissionsUtil.checkNetworkSettingsPermission(anyInt())).thenReturn(true); when(mWifiPermissionsWrapper.getDevicePolicyManagerInternal()) .thenReturn(mDevicePolicyManagerInternal); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); createWifiConfigManager(); mWifiConfigManager.setOnSavedNetworkUpdateListener(mWcmListener); } @@ -2148,6 +2149,8 @@ public class WifiConfigManagerTest { setupStoreDataForUserRead(user2Networks, new HashSet()); // Now switch the user to user 2 and ensure that user 1's private network has been removed. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); Set removedNetworks = mWifiConfigManager.handleUserSwitch(user2); verify(mWifiConfigStore).switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)); assertTrue((removedNetworks.size() == 1) && (removedNetworks.contains(user1NetworkId))); @@ -2228,7 +2231,7 @@ public class WifiConfigManagerTest { public void testHandleUserSwitchPushesOtherPrivateNetworksToSharedStore() throws Exception { int user1 = TEST_DEFAULT_USER; int user2 = TEST_DEFAULT_USER + 1; - setupUserProfiles(user2); + setupUserProfiles(user1); int appId = 674; @@ -2261,6 +2264,8 @@ public class WifiConfigManagerTest { } }; setupStoreDataForUserRead(userNetworks, new HashSet()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)); // Capture the written data for the user 1 and ensure that it corresponds to what was @@ -2275,6 +2280,10 @@ public class WifiConfigManagerTest { // Now switch the user to user2 and ensure that user 2's private network has been moved to // the user store. when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(true).thenReturn(false); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false).thenReturn(true); mWifiConfigManager.handleUserSwitch(user2); // Set the expected network list before comparing. user1Network should be in shared data. // Note: In the real world, user1Network will no longer be visible now because it should @@ -2340,6 +2349,8 @@ public class WifiConfigManagerTest { // Unlock the owner of the legacy Passpoint configuration, verify it is removed from // the configured networks (migrated to PasspointManager). setupStoreDataForUserRead(new ArrayList(), new HashSet()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(passpointConfig.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)); Pair, List> writtenNetworkList = @@ -2463,7 +2474,8 @@ public class WifiConfigManagerTest { // Ensure that we have 2 networks in the database before the stop. assertEquals(2, mWifiConfigManager.getConfiguredNetworks().size()); - + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserStop(user1); // Ensure that we only have 1 shared network in the database after the stop. @@ -2592,6 +2604,8 @@ public class WifiConfigManagerTest { int creatorUid = UserHandle.getUid(user2, 674); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(creatorUid)).thenReturn(false); + // Create a network for user2 try adding it. This should be rejected. final WifiConfiguration user2Network = WifiConfigurationTestUtil.createPskNetwork(); NetworkUpdateResult result = addNetworkToWifiConfigManager(user2Network, creatorUid); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java b/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java index 69e6070ed..9bdea915c 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiStateMachineTest.java @@ -1723,13 +1723,13 @@ public class WifiStateMachineTest { @Test public void syncRemovePasspointConfig() throws Exception { String fqdn = "test.com"; - when(mPasspointManager.removeProvider(fqdn)).thenReturn(true); + when(mPasspointManager.removeProvider(anyInt(), eq(fqdn))).thenReturn(true); mLooper.startAutoDispatch(); assertTrue(mWsm.syncRemovePasspointConfig(mWsmAsyncChannel, fqdn)); mLooper.stopAutoDispatch(); reset(mPasspointManager); - when(mPasspointManager.removeProvider(fqdn)).thenReturn(false); + when(mPasspointManager.removeProvider(anyInt(), eq(fqdn))).thenReturn(false); mLooper.startAutoDispatch(); assertFalse(mWsm.syncRemovePasspointConfig(mWsmAsyncChannel, fqdn)); mLooper.stopAutoDispatch(); diff --git a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java index 01566c203..abe593ce6 100644 --- a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java @@ -78,6 +78,7 @@ import com.android.server.wifi.hotspot2.anqp.HSOsuProvidersElement; import com.android.server.wifi.hotspot2.anqp.I18Name; import com.android.server.wifi.hotspot2.anqp.OsuProviderInfo; import com.android.server.wifi.util.ScanResultUtil; +import com.android.server.wifi.util.WifiPermissionsUtil; import org.junit.Before; import org.junit.Test; @@ -130,6 +131,7 @@ public class PasspointManagerTest { @Mock WifiConfigStore mWifiConfigStore; @Mock PasspointConfigStoreData.DataSource mDataSource; @Mock WifiMetrics mWifiMetrics; + @Mock WifiPermissionsUtil mWifiPermissionsUtil; PasspointManager mManager; /** Sets up test. */ @@ -141,7 +143,8 @@ public class PasspointManagerTest { .thenReturn(mAnqpRequestManager); when(mObjectFactory.makeCertificateVerifier()).thenReturn(mCertVerifier); mManager = new PasspointManager(mContext, mWifiNative, mWifiKeyStore, mClock, - mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics); + mSimAccessor, mObjectFactory, mWifiConfigManager, mWifiConfigStore, mWifiMetrics, + mWifiPermissionsUtil); ArgumentCaptor callbacks = ArgumentCaptor.forClass(PasspointEventHandler.Callbacks.class); verify(mObjectFactory).makePasspointEventHandler(any(WifiNative.class), @@ -472,7 +475,7 @@ public class PasspointManagerTest { assertEquals(1, mDataSource.getProviderIndex()); // Remove the provider. - assertTrue(mManager.removeProvider(TEST_FQDN)); + assertTrue(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(provider).uninstallCertsAndKeys(); verify(mWifiConfigManager).saveToStore(true); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); @@ -512,7 +515,7 @@ public class PasspointManagerTest { assertEquals(1, mDataSource.getProviderIndex()); // Remove the provider. - assertTrue(mManager.removeProvider(TEST_FQDN)); + assertTrue(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(provider).uninstallCertsAndKeys(); verify(mWifiConfigManager).saveToStore(true); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); @@ -632,7 +635,7 @@ public class PasspointManagerTest { */ @Test public void removeNonExistingProvider() throws Exception { - assertFalse(mManager.removeProvider(TEST_FQDN)); + assertFalse(mManager.removeProvider(TEST_CREATOR_UID, TEST_FQDN)); verify(mWifiMetrics).incrementNumPasspointProviderUninstallation(); verify(mWifiMetrics, never()).incrementNumPasspointProviderUninstallSuccess(); } -- cgit v1.2.3 From 1ffc81e5470b3e98f86ac04a7e4f9daec89768b3 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) (cherry picked from commit 23685b8604571ec623e539f4f9c66db65c9dde81) --- .../com/android/server/wifi/WifiConfigManager.java | 45 ++++---------- .../java/com/android/server/wifi/WifiInjector.java | 3 +- .../server/wifi/WifiNetworkSuggestionsManager.java | 29 ++++++++- .../com/android/server/wifi/WifiServiceImpl.java | 13 ++-- .../server/wifi/hotspot2/PasspointManager.java | 15 ++++- .../server/wifi/util/WifiPermissionsUtil.java | 29 +++++++++ .../android/server/wifi/WifiConfigManagerTest.java | 17 +++++- .../wifi/WifiNetworkSuggestionsManagerTest.java | 71 +++++++++++++++++----- .../android/server/wifi/WifiServiceImplTest.java | 10 +-- .../server/wifi/hotspot2/PasspointManagerTest.java | 53 +++++++++++++++- 10 files changed, 216 insertions(+), 69 deletions(-) diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java index 6d04b7a68..c0cb899d2 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. @@ -1334,7 +1308,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); } @@ -1440,7 +1414,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; } @@ -1849,7 +1823,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; } @@ -1887,7 +1861,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; } @@ -1955,7 +1929,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; } @@ -2927,7 +2901,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))) { removedNetworkIds.add(config.networkId); localLog("clearInternalUserData: removed config." + " netId=" + config.networkId @@ -3146,7 +3121,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)) { @@ -3166,7 +3142,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 b9aa0521f..5531c4e98 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -304,7 +304,8 @@ public class WifiInjector { mWifiCarrierInfoManager, mWifiKeyStore, mLruConnectionTracker); mPasspointManager = new PasspointManager(mContext, this, wifiHandler, mWifiNative, mWifiKeyStore, mClock, new PasspointObjectFactory(), - mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager); + mWifiConfigManager, mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager, + 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 a398eacaf..20e82d845 100644 --- a/service/java/com/android/server/wifi/WifiServiceImpl.java +++ b/service/java/com/android/server/wifi/WifiServiceImpl.java @@ -3888,13 +3888,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()); } /** @@ -4213,7 +4215,7 @@ public class WifiServiceImpl extends BaseWifiService { mWifiThreadRunner.post(() -> mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(binder, listener, - listenerIdentifier, packageName)); + listenerIdentifier, packageName, uid)); } /** @@ -4223,14 +4225,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 25553b8fe..2c291f024 100644 --- a/service/java/com/android/server/wifi/hotspot2/PasspointManager.java +++ b/service/java/com/android/server/wifi/hotspot2/PasspointManager.java @@ -54,6 +54,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; @@ -118,7 +119,7 @@ public class PasspointManager { private final PasspointProvisioner mPasspointProvisioner; private final AppOpsManager mAppOps; private final WifiCarrierInfoManager mWifiCarrierInfoManager; - + private final WifiPermissionsUtil mWifiPermissionsUtil; /** * Map of package name of an app to the app ops changed listener for the app. */ @@ -304,7 +305,8 @@ public class PasspointManager { PasspointObjectFactory objectFactory, WifiConfigManager wifiConfigManager, WifiConfigStore wifiConfigStore, WifiMetrics wifiMetrics, - WifiCarrierInfoManager wifiCarrierInfoManager) { + WifiCarrierInfoManager wifiCarrierInfoManager, + WifiPermissionsUtil wifiPermissionsUtil) { mPasspointEventHandler = objectFactory.makePasspointEventHandler(wifiNative, new CallbackHandler(context)); mWifiInjector = wifiInjector; @@ -326,6 +328,7 @@ public class PasspointManager { this, wifiMetrics); mAppOps = (AppOpsManager) context.getSystemService(Context.APP_OPS_SERVICE); sPasspointManager = this; + mWifiPermissionsUtil = wifiPermissionsUtil; } /** @@ -406,6 +409,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. @@ -499,6 +506,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; + } } diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java index d2584adbc..9d96116eb 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java @@ -222,6 +222,7 @@ public class WifiConfigManagerTest extends WifiBaseTest { .thenReturn(false); when(mWifiInjector.getMacAddressUtil()).thenReturn(mMacAddressUtil); when(mWifiInjector.getWifiMetrics()).thenReturn(mWifiMetrics); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); when(mMacAddressUtil.calculatePersistentMac(any(), any())).thenReturn(TEST_RANDOMIZED_MAC); when(mWifiScoreCard.lookupNetwork(any())).thenReturn(mPerNetwork); @@ -2912,6 +2913,8 @@ public class WifiConfigManagerTest extends WifiBaseTest { setupStoreDataForUserRead(user2Networks, new HashMap<>()); // Now switch the user to user 2 and ensure that user 1's private network has been removed. when(mUserManager.isUserUnlockingOrUnlocked(UserHandle.of(user2))).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); Set removedNetworks = mWifiConfigManager.handleUserSwitch(user2); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); assertTrue((removedNetworks.size() == 1) && (removedNetworks.contains(user1NetworkId))); @@ -2991,7 +2994,7 @@ public class WifiConfigManagerTest extends WifiBaseTest { public void testHandleUserSwitchPushesOtherPrivateNetworksToSharedStore() throws Exception { int user1 = TEST_DEFAULT_USER; int user2 = TEST_DEFAULT_USER + 1; - setupUserProfiles(user2); + setupUserProfiles(user1); int appId = 674; @@ -3029,6 +3032,8 @@ public class WifiConfigManagerTest extends WifiBaseTest { } }; setupStoreDataForUserRead(userNetworks, new HashMap<>()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); // Capture the written data for the user 1 and ensure that it corresponds to what was @@ -3043,6 +3048,10 @@ public class WifiConfigManagerTest extends WifiBaseTest { // Now switch the user to user2 and ensure that user 2's private network has been moved to // the user store. when(mUserManager.isUserUnlockingOrUnlocked(UserHandle.of(user2))).thenReturn(true); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(true).thenReturn(false); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user2Network.creatorUid)) + .thenReturn(false).thenReturn(true); mWifiConfigManager.handleUserSwitch(user2); // Set the expected network list before comparing. user1Network should be in shared data. // Note: In the real world, user1Network will no longer be visible now because it should @@ -3107,6 +3116,8 @@ public class WifiConfigManagerTest extends WifiBaseTest { // Unlock the owner of the legacy Passpoint configuration, verify it is removed from // the configured networks (migrated to PasspointManager). setupStoreDataForUserRead(new ArrayList(), new HashMap<>()); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(passpointConfig.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserUnlock(user1); verify(mWifiConfigStore).switchUserStoresAndRead(any(List.class)); Pair, List> writtenNetworkList = @@ -3234,7 +3245,8 @@ public class WifiConfigManagerTest extends WifiBaseTest { // Ensure that we have 2 networks in the database before the stop. assertEquals(2, mWifiConfigManager.getConfiguredNetworks().size()); - + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(user1Network.creatorUid)) + .thenReturn(false); mWifiConfigManager.handleUserStop(user1); // Ensure that we only have 1 shared network in the database after the stop. @@ -3436,6 +3448,7 @@ public class WifiConfigManagerTest extends WifiBaseTest { int creatorUid = UserHandle.getUid(user2, 674); when(mWifiPermissionsUtil.checkNetworkSettingsPermission(creatorUid)).thenReturn(false); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(creatorUid)).thenReturn(false); // Create a network for user2 try adding it. This should be rejected. final WifiConfiguration user2Network = WifiConfigurationTestUtil.createPskNetwork(); diff --git a/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java index 38a0026df..fd27dcc06 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiNetworkSuggestionsManagerTest.java @@ -66,6 +66,7 @@ import android.net.wifi.hotspot2.pps.HomeSp; import android.os.Handler; import android.os.IBinder; import android.os.UserHandle; +import android.os.UserManager; import android.os.test.TestLooper; import android.telephony.TelephonyManager; import android.test.suitebuilder.annotation.SmallTest; @@ -150,6 +151,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { private @Mock Notification.Builder mNotificationBuilder; private @Mock Notification mNotification; private @Mock LruConnectionTracker mLruConnectionTracker; + private @Mock UserManager mUserManager; private TestLooper mLooper; private ArgumentCaptor mAppOpChangedListenerCaptor = ArgumentCaptor.forClass(AppOpsManager.OnOpChangedListener.class); @@ -216,6 +218,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { when(mActivityManager.isLowRamDevice()).thenReturn(false); when(mActivityManager.getPackageImportance(any())).thenReturn( IMPORTANCE_FOREGROUND_SERVICE); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); // setup resource strings for notification. when(mResources.getString(eq(R.string.wifi_suggestion_title), anyString())) @@ -560,7 +563,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { TEST_PACKAGE_1, TEST_FEATURE)); assertEquals(WifiConfiguration.METERED_OVERRIDE_METERED, mWifiNetworkSuggestionsManager - .get(TEST_PACKAGE_1).get(0).wifiConfiguration.meteredOverride); + .get(TEST_PACKAGE_1, TEST_UID_1).get(0).wifiConfiguration.meteredOverride); // Verify we did not update config in WCM. verify(mWifiConfigManager, never()).addOrUpdateNetwork(any(), anyInt(), any()); } @@ -607,7 +610,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { TEST_PACKAGE_1, TEST_FEATURE)); assertEquals(WifiConfiguration.METERED_OVERRIDE_METERED, mWifiNetworkSuggestionsManager - .get(TEST_PACKAGE_1).get(0).wifiConfiguration.meteredOverride); + .get(TEST_PACKAGE_1, TEST_UID_1).get(0).wifiConfiguration.meteredOverride); verify(mWifiMetrics, never()).incrementNetworkSuggestionApiUsageNumOfAppInType(anyInt()); // Verify we did update config in WCM. ArgumentCaptor configCaptor = @@ -1066,7 +1069,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { public void testOnNetworkConnectionSuccessWithOneMatch() throws Exception { assertTrue(mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(mBinder, mListener, - NETWORK_CALLBACK_ID, TEST_PACKAGE_1)); + NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); WifiNetworkSuggestion networkSuggestion = new WifiNetworkSuggestion( WifiConfigurationTestUtil.createOpenNetwork(), null, true, false, true, true); List networkSuggestionList = @@ -1103,7 +1106,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { public void testOnNetworkConnectionSuccessWithOneMatchFromCarrierPrivilegedApp() { assertTrue(mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(mBinder, mListener, - NETWORK_CALLBACK_ID, TEST_PACKAGE_1)); + NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); when(mWifiCarrierInfoManager.getCarrierIdForPackageWithCarrierPrivileges(TEST_PACKAGE_1)) .thenReturn(TEST_CARRIER_ID); WifiNetworkSuggestion networkSuggestion = new WifiNetworkSuggestion( @@ -1138,6 +1141,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { mWifiNetworkSuggestionsManager.remove(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1)); verify(mWifiConfigManager).removeSuggestionConfiguredNetwork(anyString()); + mInorder.verify(mWifiPermissionsUtil).doesUidBelongToCurrentUser(eq(TEST_UID_1)); // Verify no more broadcast were sent out. mInorder.verifyNoMoreInteractions(); @@ -1152,7 +1156,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { public void testOnSavedOpenNetworkConnectionSuccessWithMultipleMatch() throws Exception { assertTrue(mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(mBinder, mListener, - NETWORK_CALLBACK_ID, TEST_PACKAGE_1)); + NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); when(mWifiPermissionsUtil.checkNetworkCarrierProvisioningPermission(TEST_UID_1)) .thenReturn(true); WifiConfiguration config = WifiConfigurationTestUtil.createOpenNetwork(); @@ -1199,7 +1203,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { ArgumentCaptor.forClass(IBinder.DeathRecipient.class); assertTrue(mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(mBinder, mListener, - NETWORK_CALLBACK_ID, TEST_PACKAGE_1)); + NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); verify(mBinder).linkToDeath(drCaptor.capture(), anyInt()); WifiNetworkSuggestion networkSuggestion = new WifiNetworkSuggestion( WifiConfigurationTestUtil.createOpenNetwork(), null, true, false, true, true); @@ -1249,7 +1253,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { public void testOnNetworkConnectionFailureWithOneMatch() throws Exception { assertTrue(mWifiNetworkSuggestionsManager .registerSuggestionConnectionStatusListener(mBinder, mListener, - NETWORK_CALLBACK_ID, TEST_PACKAGE_1)); + NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); WifiNetworkSuggestion networkSuggestion = new WifiNetworkSuggestion( WifiConfigurationTestUtil.createOpenNetwork(), null, true, false, true, true); List networkSuggestionList = @@ -2467,7 +2471,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { public void testGetNetworkSuggestions() { // test App never suggested. List storedNetworkSuggestionListPerApp = - mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1); + mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1); assertEquals(storedNetworkSuggestionListPerApp.size(), 0); // App add network suggestions then get stored suggestions. @@ -2489,7 +2493,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { TEST_PACKAGE_1, TEST_FEATURE)); mWifiNetworkSuggestionsManager.setHasUserApprovedForApp(true, TEST_PACKAGE_1); storedNetworkSuggestionListPerApp = - mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1); + mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1); assertEquals(new HashSet<>(networkSuggestionList), new HashSet<>(storedNetworkSuggestionListPerApp)); @@ -2498,7 +2502,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { mWifiNetworkSuggestionsManager.remove(new ArrayList<>(), TEST_UID_1, TEST_PACKAGE_1)); storedNetworkSuggestionListPerApp = - mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1); + mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1); assertEquals(storedNetworkSuggestionListPerApp.size(), 0); } @@ -3222,7 +3226,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { .add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1, TEST_FEATURE); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_ADD_NOT_ALLOWED, status); verify(mNotificationManger, never()).notify(anyInt(), any()); - assertEquals(0, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1).size()); + assertEquals(0, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1).size()); verify(mWifiMetrics, never()).incrementNetworkSuggestionApiUsageNumOfAppInType(anyInt()); } @@ -3246,7 +3250,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { .add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1, TEST_FEATURE); assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, status); verify(mNotificationManger, never()).notify(anyInt(), any()); - assertEquals(1, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1).size()); + assertEquals(1, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1).size()); verify(mWifiMetrics).incrementNetworkSuggestionApiUsageNumOfAppInType( WifiNetworkSuggestionsManager.APP_TYPE_CARRIER_PRIVILEGED); } @@ -3271,7 +3275,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { .add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1, TEST_FEATURE); assertEquals(status, WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS); verify(mNotificationManger, never()).notify(anyInt(), any()); - assertEquals(1, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1).size()); + assertEquals(1, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1).size()); } /** @@ -3333,7 +3337,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { when(mWifiCarrierInfoManager.getCarrierIdForPackageWithCarrierPrivileges(TEST_PACKAGE_1)) .thenReturn(TelephonyManager.UNKNOWN_CARRIER_ID); mWifiNetworkSuggestionsManager.resetCarrierPrivilegedApps(); - assertEquals(0, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1).size()); + assertEquals(0, mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1).size()); verify(mWifiConfigManager, times(2)).saveToStore(true); status = mWifiNetworkSuggestionsManager .add(networkSuggestionList, TEST_UID_1, TEST_PACKAGE_1, TEST_FEATURE); @@ -4033,7 +4037,7 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { public void testUnregisterSuggestionConnectionStatusListenerNeverRegistered() { int listenerIdentifier = 1234; mWifiNetworkSuggestionsManager.unregisterSuggestionConnectionStatusListener( - listenerIdentifier, TEST_PACKAGE_1); + listenerIdentifier, TEST_PACKAGE_1, TEST_UID_1); } /** @@ -4083,6 +4087,43 @@ public class WifiNetworkSuggestionsManagerTest extends WifiBaseTest { mWifiNetworkSuggestionsManager.getAllApprovedNetworkSuggestions()); } + /** + * Verify when calling API from background user will fail. + */ + @Test + public void testCallingFromBackgroundUserWillFailed() { + WifiConfiguration wifiConfiguration = WifiConfigurationTestUtil.createOpenNetwork(); + WifiNetworkSuggestion networkSuggestion = new WifiNetworkSuggestion( + wifiConfiguration, null, false, false, true, true); + + assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, + mWifiNetworkSuggestionsManager.add(Arrays.asList(networkSuggestion), TEST_UID_1, + TEST_PACKAGE_1, TEST_FEATURE)); + // When switch the user to background + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(TEST_UID_1)).thenReturn(false); + assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL, + mWifiNetworkSuggestionsManager.add(Arrays.asList(networkSuggestion), TEST_UID_1, + TEST_PACKAGE_1, TEST_FEATURE)); + assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_ERROR_INTERNAL, + mWifiNetworkSuggestionsManager.remove(Arrays.asList(networkSuggestion), TEST_UID_1, + TEST_PACKAGE_1)); + assertTrue(mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1).isEmpty()); + assertFalse(mWifiNetworkSuggestionsManager.registerSuggestionConnectionStatusListener( + mBinder, mListener, NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); + + // When switch the user back to foreground + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(TEST_UID_1)).thenReturn(true); + assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, + mWifiNetworkSuggestionsManager.add(Arrays.asList(networkSuggestion), TEST_UID_1, + TEST_PACKAGE_1, TEST_FEATURE)); + assertFalse(mWifiNetworkSuggestionsManager.get(TEST_PACKAGE_1, TEST_UID_1).isEmpty()); + assertEquals(WifiManager.STATUS_NETWORK_SUGGESTIONS_SUCCESS, + mWifiNetworkSuggestionsManager.remove(Arrays.asList(networkSuggestion), TEST_UID_1, + TEST_PACKAGE_1)); + assertTrue(mWifiNetworkSuggestionsManager.registerSuggestionConnectionStatusListener( + mBinder, mListener, NETWORK_CALLBACK_ID, TEST_PACKAGE_1, TEST_UID_1)); + } + /** * Helper function for creating a test configuration with user credential. * diff --git a/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java b/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java index 6d1ae2927..55f35e380 100644 --- a/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java +++ b/tests/wifitests/src/com/android/server/wifi/WifiServiceImplTest.java @@ -4587,12 +4587,12 @@ public class WifiServiceImplTest extends WifiBaseTest { @Test public void testGetNetworkSuggestions() { List testList = new ArrayList<>(); - when(mWifiNetworkSuggestionsManager.get(anyString())).thenReturn(testList); + when(mWifiNetworkSuggestionsManager.get(anyString(), anyInt())).thenReturn(testList); mLooper.startAutoDispatch(); assertEquals(testList, mWifiServiceImpl.getNetworkSuggestions(TEST_PACKAGE_NAME)); mLooper.stopAutoDispatchAndIgnoreExceptions(); - verify(mWifiNetworkSuggestionsManager).get(eq(TEST_PACKAGE_NAME)); + verify(mWifiNetworkSuggestionsManager).get(eq(TEST_PACKAGE_NAME), anyInt()); } /** @@ -4607,7 +4607,7 @@ public class WifiServiceImplTest extends WifiBaseTest { assertTrue(mWifiServiceImpl.getNetworkSuggestions(TEST_PACKAGE_NAME).isEmpty()); mLooper.stopAutoDispatchAndIgnoreExceptions(); - verify(mWifiNetworkSuggestionsManager, never()).get(eq(TEST_PACKAGE_NAME)); + verify(mWifiNetworkSuggestionsManager, never()).get(eq(TEST_PACKAGE_NAME), anyInt()); } /** @@ -5389,12 +5389,12 @@ public class WifiServiceImplTest extends WifiBaseTest { mLooper.dispatchAll(); verify(mWifiNetworkSuggestionsManager).registerSuggestionConnectionStatusListener( eq(mAppBinder), eq(mSuggestionConnectionStatusListener), eq(NETWORK_CALLBACK_ID), - eq(TEST_PACKAGE_NAME)); + eq(TEST_PACKAGE_NAME), anyInt()); mWifiServiceImpl.unregisterSuggestionConnectionStatusListener(NETWORK_CALLBACK_ID, TEST_PACKAGE_NAME); mLooper.dispatchAll(); verify(mWifiNetworkSuggestionsManager).unregisterSuggestionConnectionStatusListener( - eq(NETWORK_CALLBACK_ID), eq(TEST_PACKAGE_NAME)); + eq(NETWORK_CALLBACK_ID), eq(TEST_PACKAGE_NAME), anyInt()); } diff --git a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java index dbc38d493..ad207bfe2 100644 --- a/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java +++ b/tests/wifitests/src/com/android/server/wifi/hotspot2/PasspointManagerTest.java @@ -99,6 +99,7 @@ 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.InformationElementUtil.RoamingConsortium; +import com.android.server.wifi.util.WifiPermissionsUtil; import org.junit.Before; import org.junit.Test; @@ -188,6 +189,7 @@ public class PasspointManagerTest extends WifiBaseTest { @Mock TelephonyManager mTelephonyManager; @Mock SubscriptionManager mSubscriptionManager; @Mock WifiNetworkSuggestionsManager mWifiNetworkSuggestionsManager; + @Mock WifiPermissionsUtil mWifiPermissionsUtil; Handler mHandler; TestLooper mLooper; @@ -216,6 +218,7 @@ public class PasspointManagerTest extends WifiBaseTest { when(mWifiInjector.getClientModeImpl()).thenReturn(mClientModeImpl); when(mWifiInjector.getWifiNetworkSuggestionsManager()) .thenReturn(mWifiNetworkSuggestionsManager); + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(true); mWifiCarrierInfoManager = new WifiCarrierInfoManager(mTelephonyManager, mSubscriptionManager, mWifiInjector, mock(FrameworkFacade.class), mock(WifiContext.class), mWifiConfigStore, mock(Handler.class), mWifiMetrics); @@ -223,7 +226,7 @@ public class PasspointManagerTest extends WifiBaseTest { mHandler = new Handler(mLooper.getLooper()); mManager = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, mObjectFactory, mWifiConfigManager, - mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager); + mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager, mWifiPermissionsUtil); ArgumentCaptor callbacks = ArgumentCaptor.forClass(PasspointEventHandler.Callbacks.class); verify(mObjectFactory).makePasspointEventHandler(any(WifiNative.class), @@ -519,6 +522,29 @@ public class PasspointManagerTest extends WifiBaseTest { verify(mWifiMetrics, never()).incrementNumPasspointProviderInstallSuccess(); } + /** + * Verify that adding a provider from a background user will fail. + * + * @throws Exception + */ + @Test + public void addProviderWithBackgroundUser() throws Exception { + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(false); + + PasspointConfiguration config = createTestConfigWithUserCredential(TEST_FQDN, + TEST_FRIENDLY_NAME); + PasspointProvider provider = createMockProvider(config); + when(provider.getPackageName()).thenReturn(TEST_PACKAGE); + when(mObjectFactory.makePasspointProvider(eq(config), eq(mWifiKeyStore), + eq(mWifiCarrierInfoManager), anyLong(), eq(TEST_CREATOR_UID), eq(TEST_PACKAGE), + eq(false))).thenReturn(provider); + assertFalse(mManager.addOrUpdateProvider(config, TEST_CREATOR_UID, + TEST_PACKAGE, false, true)); + + verify(mWifiMetrics).incrementNumPasspointProviderInstallation(); + verify(mWifiMetrics, never()).incrementNumPasspointProviderInstallSuccess(); + } + /** * Verify that adding a user saved provider with a valid configuration and user credential will * succeed. @@ -749,7 +775,7 @@ public class PasspointManagerTest extends WifiBaseTest { .thenReturn(true); PasspointManager ut = new PasspointManager(mContext, mWifiInjector, mHandler, mWifiNative, mWifiKeyStore, mClock, spyFactory, mWifiConfigManager, - mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager); + mWifiConfigStore, mWifiMetrics, mWifiCarrierInfoManager, mWifiPermissionsUtil); assertTrue(ut.addOrUpdateProvider(config, TEST_CREATOR_UID, TEST_PACKAGE, true, true)); @@ -1894,6 +1920,29 @@ public class PasspointManagerTest extends WifiBaseTest { assertFalse(mManager.getProviderConfigs(TEST_CREATOR_UID, false).isEmpty()); } + /** + * Verify that removing a provider from a background user will fail. + * + * @throws Exception + */ + @Test + public void removeProviderWithBackgroundUser() throws Exception { + PasspointConfiguration config = createTestConfigWithUserCredential(TEST_FQDN, + TEST_FRIENDLY_NAME); + PasspointProvider provider = createMockProvider(config); + when(mObjectFactory.makePasspointProvider(eq(config), eq(mWifiKeyStore), + eq(mWifiCarrierInfoManager), anyLong(), eq(TEST_CREATOR_UID), eq(TEST_PACKAGE), + eq(false))).thenReturn(provider); + assertTrue(mManager.addOrUpdateProvider(config, TEST_CREATOR_UID, TEST_PACKAGE, + false, true)); + verifyInstalledConfig(config); + verify(mWifiMetrics).incrementNumPasspointProviderInstallation(); + verify(mWifiMetrics).incrementNumPasspointProviderInstallSuccess(); + + when(mWifiPermissionsUtil.doesUidBelongToCurrentUser(anyInt())).thenReturn(false); + assertFalse(mManager.removeProvider(TEST_CREATOR_UID, false, null, TEST_FQDN)); + } + /** * Verify that adding a suggestion provider with a valid configuration and user credential will * succeed. -- cgit v1.2.3