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
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 5228498..0d2e941 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -224,7 +224,11 @@
@GuardedBy("mVpns")
private final SparseArray<Vpn> mVpns = new SparseArray<Vpn>();
+ // 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 @@
}
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 @@
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 @@
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 @@
}
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 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 @@
*/
@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 @@
*/
@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 @@
@Override
public VpnInfo[] getAllVpnInfo() {
enforceConnectivityInternalPermission();
- if (mLockdownEnabled) {
- return new VpnInfo[0];
- }
-
synchronized (mVpns) {
+ if (mLockdownEnabled) {
+ return new VpnInfo[0];
+ }
+
List<VpnInfo> infoList = new ArrayList<>();
for (int i = 0; i < mVpns.size(); i++) {
VpnInfo info = createVpnInfo(mVpns.valueAt(i));
@@ -3582,33 +3590,33 @@
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 @@
* 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 @@
}
}
+ @GuardedBy("mVpns")
private void throwIfLockdownEnabled() {
if (mLockdownEnabled) {
throw new IllegalStateException("Unavailable in lockdown mode");
@@ -3679,12 +3689,12 @@
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 @@
}
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 @@
}
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 @@
}
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 @@
@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 @@
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);
+ }
}
}
}