From 7a480f3f678b511b42a174c41d6704d9bd55c57b Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Mon, 27 Nov 2017 10:57:16 +0900 Subject: [PATCH 1/3] ConnectivityService: synchronize access on mLockdownEnabled The mLockdownEnabled boolean and the mLockdownTracker objects are read and mutated in many places involving vpn logic inside of ConnectivityService. This includes codepaths run on the ConnectivityService handler and codepaths run on Binder calls from IConnectivityManager.aidl, however the access to these variables are not synchronized. This patch adds proper synchronization to mLockdownEnabled and mLockdownTracker by moving access to them into the mVpns lock used for all of vpn logic. Bug: 18331877 Test: runtest frameworks-net Change-Id: I4abde43b1036861f4486dd2b5567782d10204bd6 --- .../android/server/ConnectivityService.java | 176 ++++++++++-------- 1 file changed, 95 insertions(+), 81 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 5228498ba8..0d2e941858 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -224,7 +224,11 @@ public class ConnectivityService extends IConnectivityManager.Stub @GuardedBy("mVpns") private final SparseArray mVpns = new SparseArray(); + // TODO: investigate if mLockdownEnabled can be removed and replaced everywhere by + // a direct call to LockdownVpnTracker.isEnabled(). + @GuardedBy("mVpns") private boolean mLockdownEnabled; + @GuardedBy("mVpns") private LockdownVpnTracker mLockdownTracker; final private Context mContext; @@ -972,9 +976,9 @@ public class ConnectivityService extends IConnectivityManager.Stub } private Network[] getVpnUnderlyingNetworks(int uid) { - if (!mLockdownEnabled) { - int user = UserHandle.getUserId(uid); - synchronized (mVpns) { + synchronized (mVpns) { + if (!mLockdownEnabled) { + int user = UserHandle.getUserId(uid); Vpn vpn = mVpns.get(user); if (vpn != null && vpn.appliesToUid(uid)) { return vpn.getUnderlyingNetworks(); @@ -1062,8 +1066,10 @@ public class ConnectivityService extends IConnectivityManager.Stub if (isNetworkWithLinkPropertiesBlocked(state.linkProperties, uid, ignoreBlocked)) { state.networkInfo.setDetailedState(DetailedState.BLOCKED, null, null); } - if (mLockdownTracker != null) { - mLockdownTracker.augmentNetworkInfo(state.networkInfo); + synchronized (mVpns) { + if (mLockdownTracker != null) { + mLockdownTracker.augmentNetworkInfo(state.networkInfo); + } } } @@ -1228,8 +1234,8 @@ public class ConnectivityService extends IConnectivityManager.Stub result.put(nai.network, nc); } - if (!mLockdownEnabled) { - synchronized (mVpns) { + synchronized (mVpns) { + if (!mLockdownEnabled) { Vpn vpn = mVpns.get(userId); if (vpn != null) { Network[] networks = vpn.getUnderlyingNetworks(); @@ -1555,9 +1561,11 @@ public class ConnectivityService extends IConnectivityManager.Stub } private Intent makeGeneralIntent(NetworkInfo info, String bcastType) { - if (mLockdownTracker != null) { - info = new NetworkInfo(info); - mLockdownTracker.augmentNetworkInfo(info); + synchronized (mVpns) { + if (mLockdownTracker != null) { + info = new NetworkInfo(info); + mLockdownTracker.augmentNetworkInfo(info); + } } Intent intent = new Intent(bcastType); @@ -3422,9 +3430,9 @@ public class ConnectivityService extends IConnectivityManager.Stub public boolean prepareVpn(@Nullable String oldPackage, @Nullable String newPackage, int userId) { enforceCrossUserPermission(userId); - throwIfLockdownEnabled(); synchronized (mVpns) { + throwIfLockdownEnabled(); Vpn vpn = mVpns.get(userId); if (vpn != null) { return vpn.prepare(oldPackage, newPackage); @@ -3468,9 +3476,9 @@ public class ConnectivityService extends IConnectivityManager.Stub */ @Override public ParcelFileDescriptor establishVpn(VpnConfig config) { - throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { + throwIfLockdownEnabled(); return mVpns.get(user).establish(config); } } @@ -3481,13 +3489,13 @@ public class ConnectivityService extends IConnectivityManager.Stub */ @Override public void startLegacyVpn(VpnProfile profile) { - throwIfLockdownEnabled(); + int user = UserHandle.getUserId(Binder.getCallingUid()); final LinkProperties egress = getActiveLinkProperties(); if (egress == null) { throw new IllegalStateException("Missing active network connection"); } - int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { + throwIfLockdownEnabled(); mVpns.get(user).startLegacyVpn(profile, mKeyStore, egress); } } @@ -3513,11 +3521,11 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public VpnInfo[] getAllVpnInfo() { enforceConnectivityInternalPermission(); - if (mLockdownEnabled) { - return new VpnInfo[0]; - } - synchronized (mVpns) { + if (mLockdownEnabled) { + return new VpnInfo[0]; + } + List infoList = new ArrayList<>(); for (int i = 0; i < mVpns.size(); i++) { VpnInfo info = createVpnInfo(mVpns.valueAt(i)); @@ -3582,33 +3590,33 @@ public class ConnectivityService extends IConnectivityManager.Stub return false; } - // Tear down existing lockdown if profile was removed - mLockdownEnabled = LockdownVpnTracker.isEnabled(); - if (mLockdownEnabled) { - byte[] profileTag = mKeyStore.get(Credentials.LOCKDOWN_VPN); - if (profileTag == null) { - Slog.e(TAG, "Lockdown VPN configured but cannot be read from keystore"); - return false; - } - String profileName = new String(profileTag); - final VpnProfile profile = VpnProfile.decode( - profileName, mKeyStore.get(Credentials.VPN + profileName)); - if (profile == null) { - Slog.e(TAG, "Lockdown VPN configured invalid profile " + profileName); - setLockdownTracker(null); - return true; - } - int user = UserHandle.getUserId(Binder.getCallingUid()); - synchronized (mVpns) { + synchronized (mVpns) { + // Tear down existing lockdown if profile was removed + mLockdownEnabled = LockdownVpnTracker.isEnabled(); + if (mLockdownEnabled) { + byte[] profileTag = mKeyStore.get(Credentials.LOCKDOWN_VPN); + if (profileTag == null) { + Slog.e(TAG, "Lockdown VPN configured but cannot be read from keystore"); + return false; + } + String profileName = new String(profileTag); + final VpnProfile profile = VpnProfile.decode( + profileName, mKeyStore.get(Credentials.VPN + profileName)); + if (profile == null) { + Slog.e(TAG, "Lockdown VPN configured invalid profile " + profileName); + setLockdownTracker(null); + return true; + } + int user = UserHandle.getUserId(Binder.getCallingUid()); Vpn vpn = mVpns.get(user); if (vpn == null) { Slog.w(TAG, "VPN for user " + user + " not ready yet. Skipping lockdown"); return false; } setLockdownTracker(new LockdownVpnTracker(mContext, mNetd, this, vpn, profile)); + } else { + setLockdownTracker(null); } - } else { - setLockdownTracker(null); } return true; @@ -3618,6 +3626,7 @@ public class ConnectivityService extends IConnectivityManager.Stub * Internally set new {@link LockdownVpnTracker}, shutting down any existing * {@link LockdownVpnTracker}. Can be {@code null} to disable lockdown. */ + @GuardedBy("mVpns") private void setLockdownTracker(LockdownVpnTracker tracker) { // Shutdown any existing tracker final LockdownVpnTracker existing = mLockdownTracker; @@ -3632,6 +3641,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + @GuardedBy("mVpns") private void throwIfLockdownEnabled() { if (mLockdownEnabled) { throw new IllegalStateException("Unavailable in lockdown mode"); @@ -3679,12 +3689,12 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceConnectivityInternalPermission(); enforceCrossUserPermission(userId); - // Can't set always-on VPN if legacy VPN is already in lockdown mode. - if (LockdownVpnTracker.isEnabled()) { - return false; - } - synchronized (mVpns) { + // Can't set always-on VPN if legacy VPN is already in lockdown mode. + if (LockdownVpnTracker.isEnabled()) { + return false; + } + Vpn vpn = mVpns.get(userId); if (vpn == null) { Slog.w(TAG, "User " + userId + " has no Vpn configuration"); @@ -3860,9 +3870,9 @@ public class ConnectivityService extends IConnectivityManager.Stub } userVpn = new Vpn(mHandler.getLooper(), mContext, mNetd, userId); mVpns.put(userId, userVpn); - } - if (mUserManager.getUserInfo(userId).isPrimary() && LockdownVpnTracker.isEnabled()) { - updateLockdownVpn(); + if (mUserManager.getUserInfo(userId).isPrimary() && LockdownVpnTracker.isEnabled()) { + updateLockdownVpn(); + } } } @@ -3899,11 +3909,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void onUserUnlocked(int userId) { - // User present may be sent because of an unlock, which might mean an unlocked keystore. - if (mUserManager.getUserInfo(userId).isPrimary() && LockdownVpnTracker.isEnabled()) { - updateLockdownVpn(); - } else { - startAlwaysOnVpn(userId); + synchronized (mVpns) { + // User present may be sent because of an unlock, which might mean an unlocked keystore. + if (mUserManager.getUserInfo(userId).isPrimary() && LockdownVpnTracker.isEnabled()) { + updateLockdownVpn(); + } else { + startAlwaysOnVpn(userId); + } } } @@ -5215,11 +5227,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void notifyLockdownVpn(NetworkAgentInfo nai) { - if (mLockdownTracker != null) { - if (nai != null && nai.isVPN()) { - mLockdownTracker.onVpnStateChanged(nai.networkInfo); - } else { - mLockdownTracker.onNetworkInfoChanged(); + synchronized (mVpns) { + if (mLockdownTracker != null) { + if (nai != null && nai.isVPN()) { + mLockdownTracker.onVpnStateChanged(nai.networkInfo); + } else { + mLockdownTracker.onNetworkInfoChanged(); + } } } } @@ -5449,28 +5463,28 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public boolean addVpnAddress(String address, int prefixLength) { - throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { + throwIfLockdownEnabled(); return mVpns.get(user).addAddress(address, prefixLength); } } @Override public boolean removeVpnAddress(String address, int prefixLength) { - throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); synchronized (mVpns) { + throwIfLockdownEnabled(); return mVpns.get(user).removeAddress(address, prefixLength); } } @Override public boolean setUnderlyingNetworksForVpn(Network[] networks) { - throwIfLockdownEnabled(); int user = UserHandle.getUserId(Binder.getCallingUid()); - boolean success; + final boolean success; synchronized (mVpns) { + throwIfLockdownEnabled(); success = mVpns.get(user).setUnderlyingNetworks(networks); } if (success) { @@ -5530,31 +5544,31 @@ public class ConnectivityService extends IConnectivityManager.Stub setAlwaysOnVpnPackage(userId, null, false); setVpnPackageAuthorization(alwaysOnPackage, userId, false); } - } - // Turn Always-on VPN off - if (mLockdownEnabled && userId == UserHandle.USER_SYSTEM) { - final long ident = Binder.clearCallingIdentity(); - try { - mKeyStore.delete(Credentials.LOCKDOWN_VPN); - mLockdownEnabled = false; - setLockdownTracker(null); - } finally { - Binder.restoreCallingIdentity(ident); + // Turn Always-on VPN off + if (mLockdownEnabled && userId == UserHandle.USER_SYSTEM) { + final long ident = Binder.clearCallingIdentity(); + try { + mKeyStore.delete(Credentials.LOCKDOWN_VPN); + mLockdownEnabled = false; + setLockdownTracker(null); + } finally { + Binder.restoreCallingIdentity(ident); + } } - } - // Turn VPN off - VpnConfig vpnConfig = getVpnConfig(userId); - if (vpnConfig != null) { - if (vpnConfig.legacy) { - prepareVpn(VpnConfig.LEGACY_VPN, VpnConfig.LEGACY_VPN, userId); - } else { - // Prevent this app (packagename = vpnConfig.user) from initiating VPN connections - // in the future without user intervention. - setVpnPackageAuthorization(vpnConfig.user, userId, false); + // Turn VPN off + VpnConfig vpnConfig = getVpnConfig(userId); + if (vpnConfig != null) { + if (vpnConfig.legacy) { + prepareVpn(VpnConfig.LEGACY_VPN, VpnConfig.LEGACY_VPN, userId); + } else { + // Prevent this app (packagename = vpnConfig.user) from initiating + // VPN connections in the future without user intervention. + setVpnPackageAuthorization(vpnConfig.user, userId, false); - prepareVpn(null, VpnConfig.LEGACY_VPN, userId); + prepareVpn(null, VpnConfig.LEGACY_VPN, userId); + } } } } From 454b42eced5967808d0247c4ca97792bd115ea58 Mon Sep 17 00:00:00 2001 From: Erik Kline Date: Mon, 15 Jan 2018 17:05:07 +0900 Subject: [PATCH 2/3] change NMS removeNetwork to netd binder variant Also: require NETWORK_STACK instead of CONNECTIVITY_INTERNAL. Bug: 34953048 Bug: 64133961 Test: as follows - built - flashed - booted - runtest frameworks-net Change-Id: I26f07f51a8a2f4a8bb43a276da7c5e66c355c848 --- services/core/java/com/android/server/ConnectivityService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index f34730c084..b2acb1ca3b 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2472,6 +2472,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private void handleRemoveNetworkRequest(final NetworkRequestInfo nri) { nri.unlinkDeathRecipient(); mNetworkRequests.remove(nri.request); + synchronized (mUidToNetworkRequestCount) { int requests = mUidToNetworkRequestCount.get(nri.mUid, 0); if (requests < 1) { @@ -2484,6 +2485,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mUidToNetworkRequestCount.put(nri.mUid, requests - 1); } } + mNetworkRequestInfoLogs.log("RELEASE " + nri); if (nri.request.isRequest()) { boolean wasKept = false; From 5183e57449f1e4a5d82fb2495a6ebd75ed4b8885 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 5 Sep 2017 11:15:37 +0900 Subject: [PATCH 3/3] Change the netId to nethandle mapping. Being able to update this handle is necessary to ensure that system-only OTAs do not break vendor code that relies on nethandles. Bug: 63052780 Test: walleye builds, boots, networking works Test: MultinetworkApiTest CTS tests passes Change-Id: I049a4ad2610ca68b8f56377b63be7e5e8ce76039 --- core/java/android/net/Network.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/java/android/net/Network.java b/core/java/android/net/Network.java index 903b602b42..3683d3450b 100644 --- a/core/java/android/net/Network.java +++ b/core/java/android/net/Network.java @@ -356,13 +356,13 @@ public class Network implements Parcelable { // Multiple Provisioning Domains API recommendations, as made by the // IETF mif working group. // - // The HANDLE_MAGIC value MUST be kept in sync with the corresponding + // The handleMagic value MUST be kept in sync with the corresponding // value in the native/android/net.c NDK implementation. if (netId == 0) { return 0L; // make this zero condition obvious for debugging } - final long HANDLE_MAGIC = 0xfacade; - return (((long) netId) << 32) | HANDLE_MAGIC; + final long handleMagic = 0xcafed00dL; + return (((long) netId) << 32) | handleMagic; } // implement the Parcelable interface