From 938ab4fa3942398e942d98aa7b16dd87dfb7ff11 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Sat, 11 Feb 2017 17:04:43 +0900 Subject: [PATCH] Move back networking policy logic into NetworkPolicyManagerService This patch removes from ConnectivityService the logic involved in deciding if a uid has access to networking based on networking policies. This logic is moved into NetworkPolicyManagerService which is the source of truth with regards to the state of networking policie, both for existing networks and uids. Instead ConnectivityService directly queries NetworkPolicyManagerService in a synchronous fashion for a specific uid or a (uid, network) pair. This eliminates the need to keep a copy of the uid policy rules inside ConnectivityService and ensures that ConnectivityService takes networking decisions based on the correct state of networking policies, and therefore eliminates certain data races in ConnectivityManager API that applications are exposed to. Test: $ runtest frameworks-net $ runtest -x frameworks/base/services/tests/../NetworkPolicyManagerServiceTest.java $ runtest -c com.android.server.net.ConnOnActivityStartTest frameworks-services Bug: 32069544, 30919851 Change-Id: Ic75d4f7a8853e6be20e51262c4b59805ec35093a --- .../android/net/INetworkPolicyManager.aidl | 3 - .../android/server/ConnectivityService.java | 173 +++--------------- .../net/NetworkPolicyManagerInternal.java | 11 ++ .../net/NetworkPolicyManagerService.java | 88 +++++++-- .../server/ConnectivityServiceTest.java | 4 + 5 files changed, 115 insertions(+), 164 deletions(-) diff --git a/core/java/android/net/INetworkPolicyManager.aidl b/core/java/android/net/INetworkPolicyManager.aidl index 495340dc72224..63bbd96bd01df 100644 --- a/core/java/android/net/INetworkPolicyManager.aidl +++ b/core/java/android/net/INetworkPolicyManager.aidl @@ -38,9 +38,6 @@ interface INetworkPolicyManager { boolean isUidForeground(int uid); - /** Higher priority listener before general event dispatch */ - void setConnectivityListener(INetworkPolicyListener listener); - void registerListener(INetworkPolicyListener listener); void unregisterListener(INetworkPolicyListener listener); diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index d02b726607093..0e06682f1547c 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -29,13 +29,6 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_METERED; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED; import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; -import static android.net.NetworkPolicyManager.RULE_ALLOW_ALL; -import static android.net.NetworkPolicyManager.RULE_ALLOW_METERED; -import static android.net.NetworkPolicyManager.RULE_NONE; -import static android.net.NetworkPolicyManager.RULE_REJECT_ALL; -import static android.net.NetworkPolicyManager.RULE_REJECT_METERED; -import static android.net.NetworkPolicyManager.RULE_TEMPORARY_ALLOW_METERED; -import static android.net.NetworkPolicyManager.uidRulesToString; import android.annotation.Nullable; import android.app.BroadcastOptions; @@ -104,7 +97,6 @@ import android.security.Credentials; import android.security.KeyStore; import android.telephony.TelephonyManager; import android.text.TextUtils; -import android.util.ArraySet; import android.util.LocalLog; import android.util.LocalLog.ReadOnlyLocalLog; import android.util.Log; @@ -129,6 +121,7 @@ import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.MessageUtils; import com.android.internal.util.WakeupMessage; import com.android.internal.util.XmlUtils; +import com.android.server.LocalServices; import com.android.server.am.BatteryStatsService; import com.android.server.connectivity.DataConnectionStats; import com.android.server.connectivity.KeepaliveTracker; @@ -146,6 +139,7 @@ import com.android.server.connectivity.Tethering; import com.android.server.connectivity.Vpn; import com.android.server.net.BaseNetworkObserver; import com.android.server.net.LockdownVpnTracker; +import com.android.server.net.NetworkPolicyManagerInternal; import com.google.android.collect.Lists; @@ -220,18 +214,6 @@ public class ConnectivityService extends IConnectivityManager.Stub private boolean mLockdownEnabled; private LockdownVpnTracker mLockdownTracker; - /** Lock around {@link #mUidRules} and {@link #mMeteredIfaces}. */ - private Object mRulesLock = new Object(); - /** Currently active network rules by UID. */ - @GuardedBy("mRulesLock") - private SparseIntArray mUidRules = new SparseIntArray(); - /** Set of ifaces that are costly. */ - @GuardedBy("mRulesLock") - private ArraySet mMeteredIfaces = new ArraySet<>(); - /** Flag indicating if background data is restricted. */ - @GuardedBy("mRulesLock") - private boolean mRestrictBackground; - final private Context mContext; private int mNetworkPreference; // 0 is full bad, 100 is full good @@ -245,6 +227,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private INetworkManagementService mNetd; private INetworkStatsService mStatsService; private INetworkPolicyManager mPolicyManager; + private NetworkPolicyManagerInternal mPolicyManagerInternal; private String mCurrentTcpBufferSizes; @@ -714,12 +697,15 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetd = checkNotNull(netManager, "missing INetworkManagementService"); mStatsService = checkNotNull(statsService, "missing INetworkStatsService"); mPolicyManager = checkNotNull(policyManager, "missing INetworkPolicyManager"); + mPolicyManagerInternal = checkNotNull( + LocalServices.getService(NetworkPolicyManagerInternal.class), + "missing NetworkPolicyManagerInternal"); + mKeyStore = KeyStore.getInstance(); mTelephonyManager = (TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE); try { - mPolicyManager.setConnectivityListener(mPolicyListener); - mRestrictBackground = mPolicyManager.getRestrictBackground(); + mPolicyManager.registerListener(mPolicyListener); } catch (RemoteException e) { // ouch, no rules updates means some processes may never get network loge("unable to register INetworkPolicyListener" + e); @@ -990,51 +976,22 @@ public class ConnectivityService extends IConnectivityManager.Stub private boolean isNetworkWithLinkPropertiesBlocked(LinkProperties lp, int uid, boolean ignoreBlocked) { // Networks aren't blocked when ignoring blocked status - if (ignoreBlocked) return false; + if (ignoreBlocked) { + return false; + } // Networks are never blocked for system services - if (isSystem(uid)) return false; - - final boolean networkMetered; - final int uidRules; - + // TODO: consider moving this check to NetworkPolicyManagerInternal.isUidNetworkingBlocked. + if (isSystem(uid)) { + return false; + } synchronized (mVpns) { final Vpn vpn = mVpns.get(UserHandle.getUserId(uid)); if (vpn != null && vpn.isBlockingUid(uid)) { return true; } } - final String iface = (lp == null ? "" : lp.getInterfaceName()); - synchronized (mRulesLock) { - networkMetered = mMeteredIfaces.contains(iface); - uidRules = mUidRules.get(uid, RULE_NONE); - } - - boolean allowed = true; - // Check Data Saver Mode first... - if (networkMetered) { - if ((uidRules & RULE_REJECT_METERED) != 0) { - if (LOGD_RULES) Log.d(TAG, "uid " + uid + " is blacklisted"); - // Explicitly blacklisted. - allowed = false; - } else { - allowed = !mRestrictBackground - || (uidRules & RULE_ALLOW_METERED) != 0 - || (uidRules & RULE_TEMPORARY_ALLOW_METERED) != 0; - if (LOGD_RULES) Log.d(TAG, "allowed status for uid " + uid + " when" - + " mRestrictBackground=" + mRestrictBackground - + ", whitelisted=" + ((uidRules & RULE_ALLOW_METERED) != 0) - + ", tempWhitelist= + ((uidRules & RULE_TEMPORARY_ALLOW_METERED) != 0)" - + ": " + allowed); - } - } - // ...then power restrictions. - if (allowed) { - allowed = (uidRules & RULE_REJECT_ALL) == 0; - if (LOGD_RULES) Log.d(TAG, "allowed status for uid " + uid + " when" - + " rule is " + uidRulesToString(uidRules) + ": " + allowed); - } - return !allowed; + return mPolicyManagerInternal.isUidNetworkingBlocked(uid, iface); } private void maybeLogBlockedNetworkInfo(NetworkInfo ni, int uid) { @@ -1480,67 +1437,24 @@ public class ConnectivityService extends IConnectivityManager.Stub return true; } - private INetworkPolicyListener mPolicyListener = new INetworkPolicyListener.Stub() { + private final INetworkPolicyListener mPolicyListener = new INetworkPolicyListener.Stub() { @Override public void onUidRulesChanged(int uid, int uidRules) { - // caller is NPMS, since we only register with them - if (LOGD_RULES) { - log("onUidRulesChanged(uid=" + uid + ", uidRules=" + uidRules + ")"); - } - - synchronized (mRulesLock) { - // skip update when we've already applied rules - final int oldRules = mUidRules.get(uid, RULE_NONE); - if (oldRules == uidRules) return; - - if (uidRules == RULE_NONE) { - mUidRules.delete(uid); - } else { - mUidRules.put(uid, uidRules); - } - } - // TODO: notify UID when it has requested targeted updates } - @Override public void onMeteredIfacesChanged(String[] meteredIfaces) { - // caller is NPMS, since we only register with them - if (LOGD_RULES) { - log("onMeteredIfacesChanged(ifaces=" + Arrays.toString(meteredIfaces) + ")"); - } - - synchronized (mRulesLock) { - mMeteredIfaces.clear(); - for (String iface : meteredIfaces) { - mMeteredIfaces.add(iface); - } - } } - @Override public void onRestrictBackgroundChanged(boolean restrictBackground) { - // caller is NPMS, since we only register with them - if (LOGD_RULES) { - log("onRestrictBackgroundChanged(restrictBackground=" + restrictBackground + ")"); - } - - synchronized (mRulesLock) { - mRestrictBackground = restrictBackground; - } - + // TODO: relocate this specific callback in Tethering. if (restrictBackground) { log("onRestrictBackgroundChanged(true): disabling tethering"); mTethering.untetherAll(); } } - @Override public void onUidPoliciesChanged(int uid, int uidPolicies) { - // caller is NPMS, since we only register with them - if (LOGD_RULES) { - log("onUidRulesChanged(uid=" + uid + ", uidPolicies=" + uidPolicies + ")"); - } } }; @@ -1982,33 +1896,6 @@ public class ConnectivityService extends IConnectivityManager.Stub pw.decreaseIndent(); pw.println(); - pw.println("Metered Interfaces:"); - pw.increaseIndent(); - for (String value : mMeteredIfaces) { - pw.println(value); - } - pw.decreaseIndent(); - pw.println(); - - pw.print("Restrict background: "); - pw.println(mRestrictBackground); - pw.println(); - - pw.println("Status for known UIDs:"); - pw.increaseIndent(); - final int size = mUidRules.size(); - for (int i = 0; i < size; i++) { - final int uid = mUidRules.keyAt(i); - pw.print("UID="); - pw.print(uid); - final int uidRules = mUidRules.get(uid, RULE_NONE); - pw.print(" rules="); - pw.print(uidRulesToString(uidRules)); - pw.println(); - } - pw.println(); - pw.decreaseIndent(); - pw.println("Network Requests:"); pw.increaseIndent(); for (NetworkRequestInfo nri : mNetworkRequests.values()) { @@ -3443,6 +3330,10 @@ public class ConnectivityService extends IConnectivityManager.Stub Slog.e(TAG, s); } + private static void loge(String s, Throwable t) { + Slog.e(TAG, s, t); + } + private static T checkNotNull(T value, String message) { if (value == null) { throw new NullPointerException(message); @@ -4203,20 +4094,16 @@ public class ConnectivityService extends IConnectivityManager.Stub private void enforceMeteredApnPolicy(NetworkCapabilities networkCapabilities) { final int uid = Binder.getCallingUid(); if (isSystem(uid)) { + // Exemption for system uid. return; } - // if UID is restricted, don't allow them to bring up metered APNs - if (networkCapabilities.hasCapability(NET_CAPABILITY_NOT_METERED) == false) { - final int uidRules; - synchronized(mRulesLock) { - uidRules = mUidRules.get(uid, RULE_ALLOW_ALL); - } - if (mRestrictBackground && (uidRules & RULE_ALLOW_METERED) == 0 - && (uidRules & RULE_TEMPORARY_ALLOW_METERED) == 0) { - // we could silently fail or we can filter the available nets to only give - // them those they have access to. Chose the more useful option. - networkCapabilities.addCapability(NET_CAPABILITY_NOT_METERED); - } + if (networkCapabilities.hasCapability(NET_CAPABILITY_NOT_METERED)) { + // Policy already enforced. + return; + } + if (mPolicyManagerInternal.isUidRestrictedOnMeteredNetworks(uid)) { + // If UID is restricted, don't allow them to bring up metered APNs. + networkCapabilities.addCapability(NET_CAPABILITY_NOT_METERED); } } diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerInternal.java b/services/core/java/com/android/server/net/NetworkPolicyManagerInternal.java index 9e4432d25c0bd..dc2ebb4451b28 100644 --- a/services/core/java/com/android/server/net/NetworkPolicyManagerInternal.java +++ b/services/core/java/com/android/server/net/NetworkPolicyManagerInternal.java @@ -27,4 +27,15 @@ public abstract class NetworkPolicyManagerInternal { * Resets all policies associated with a given user. */ public abstract void resetUserState(int userId); + + /** + * @return true if the given uid is restricted from doing networking on metered networks. + */ + public abstract boolean isUidRestrictedOnMeteredNetworks(int uid); + + /** + * @return true if networking is blocked on the given interface for the given uid according + * to current networking policies. + */ + public abstract boolean isUidNetworkingBlocked(int uid, String ifname); } diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java index f180c5089aa14..4d073aa15d997 100644 --- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java +++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java @@ -245,8 +245,8 @@ import java.util.concurrent.TimeUnit; */ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { static final String TAG = "NetworkPolicy"; - private static final boolean LOGD = false; - private static final boolean LOGV = false; + private static final boolean LOGD = true; // UNDO + private static final boolean LOGV = true; // UNDO private static final int VERSION_INIT = 1; private static final int VERSION_ADDED_SNOOZE = 2; @@ -425,9 +425,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { @GuardedBy("mUidRulesFirstLock") final SparseIntArray mUidState = new SparseIntArray(); - /** Higher priority listener before general event dispatch */ - private INetworkPolicyListener mConnectivityListener; - private final RemoteCallbackList mListeners = new RemoteCallbackList<>(); @@ -2240,15 +2237,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { return changed; } - @Override - public void setConnectivityListener(INetworkPolicyListener listener) { - mContext.enforceCallingOrSelfPermission(CONNECTIVITY_INTERNAL, TAG); - if (mConnectivityListener != null) { - throw new IllegalStateException("Connectivity listener already registered"); - } - mConnectivityListener = listener; - } - @Override public void registerListener(INetworkPolicyListener listener) { // TODO: create permission for observing network policy @@ -3560,7 +3548,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { case MSG_RULES_CHANGED: { final int uid = msg.arg1; final int uidRules = msg.arg2; - dispatchUidRulesChanged(mConnectivityListener, uid, uidRules); final int length = mListeners.beginBroadcast(); for (int i = 0; i < length; i++) { final INetworkPolicyListener listener = mListeners.getBroadcastItem(i); @@ -3571,7 +3558,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { } case MSG_METERED_IFACES_CHANGED: { final String[] meteredIfaces = (String[]) msg.obj; - dispatchMeteredIfacesChanged(mConnectivityListener, meteredIfaces); final int length = mListeners.beginBroadcast(); for (int i = 0; i < length; i++) { final INetworkPolicyListener listener = mListeners.getBroadcastItem(i); @@ -3602,7 +3588,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { } case MSG_RESTRICT_BACKGROUND_CHANGED: { final boolean restrictBackground = msg.arg1 != 0; - dispatchRestrictBackgroundChanged(mConnectivityListener, restrictBackground); final int length = mListeners.beginBroadcast(); for (int i = 0; i < length; i++) { final INetworkPolicyListener listener = mListeners.getBroadcastItem(i); @@ -3620,7 +3605,6 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { final int policy = msg.arg2; final Boolean notifyApp = (Boolean) msg.obj; // First notify internal listeners... - dispatchUidPoliciesChanged(mConnectivityListener, uid, policy); final int length = mListeners.beginBroadcast(); for (int i = 0; i < length; i++) { final INetworkPolicyListener listener = mListeners.getBroadcastItem(i); @@ -4053,6 +4037,74 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { } } } + + /** + * @return true if the given uid is restricted from doing networking on metered networks. + */ + @Override + public boolean isUidRestrictedOnMeteredNetworks(int uid) { + final int uidRules; + final boolean isBackgroundRestricted; + synchronized (mUidRulesFirstLock) { + uidRules = mUidRules.get(uid, RULE_ALLOW_ALL); + isBackgroundRestricted = mRestrictBackground; + } + return isBackgroundRestricted + && !hasRule(uidRules, RULE_ALLOW_METERED) + && !hasRule(uidRules, RULE_TEMPORARY_ALLOW_METERED); + } + + /** + * @return true if networking is blocked on the given interface for the given uid according + * to current networking policies. + */ + @Override + public boolean isUidNetworkingBlocked(int uid, String ifname) { + final int uidRules; + final boolean isBackgroundRestricted; + final boolean isNetworkMetered; + synchronized (mUidRulesFirstLock) { + uidRules = mUidRules.get(uid, RULE_NONE); + isBackgroundRestricted = mRestrictBackground; + synchronized (mNetworkPoliciesSecondLock) { + isNetworkMetered = mMeteredIfaces.contains(ifname); + } + } + if (hasRule(uidRules, RULE_REJECT_ALL)) { + if (LOGV) logUidStatus(uid, "blocked by power restrictions"); + return true; + } + if (!isNetworkMetered) { + if (LOGV) logUidStatus(uid, "allowed on unmetered network"); + return false; + } + if (hasRule(uidRules, RULE_REJECT_METERED)) { + if (LOGV) logUidStatus(uid, "blacklisted on metered network"); + return true; + } + if (hasRule(uidRules, RULE_ALLOW_METERED)) { + if (LOGV) logUidStatus(uid, "whitelisted on metered network"); + return false; + } + if (hasRule(uidRules, RULE_TEMPORARY_ALLOW_METERED)) { + if (LOGV) logUidStatus(uid, "temporary whitelisted on metered network"); + return false; + } + if (isBackgroundRestricted) { + if (LOGV) logUidStatus(uid, "blocked when background is restricted"); + return true; + } + if (LOGV) logUidStatus(uid, "allowed by default"); + return false; + } + } + + private static boolean hasRule(int uidRules, int rule) { + return (uidRules & rule) != 0; + } + + private static void logUidStatus(int uid, String descr) { + Slog.d(TAG, String.format("uid %d is %s", uid, descr)); } /** diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 04443a53527cc..c562cb95ee31d 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -80,6 +80,7 @@ import com.android.server.connectivity.NetworkAgentInfo; import com.android.server.connectivity.NetworkMonitor; import com.android.server.connectivity.NetworkMonitor.CaptivePortalProbeResult; import com.android.server.net.NetworkPinner; +import com.android.server.net.NetworkPolicyManagerInternal; import java.net.InetAddress; import java.util.ArrayList; @@ -713,6 +714,9 @@ public class ConnectivityServiceTest extends AndroidTestCase { } mServiceContext = new MockContext(getContext()); + LocalServices.removeServiceForTest(NetworkPolicyManagerInternal.class); + LocalServices.addService( + NetworkPolicyManagerInternal.class, mock(NetworkPolicyManagerInternal.class)); mService = new WrappedConnectivityService(mServiceContext, mock(INetworkManagementService.class), mock(INetworkStatsService.class),