Don't enforce control permission when preparing consented VPN.
If a VPN app requests to be prepared and has already obtained user
consent, there is no need to additionally enforce the control
permission. We only need to enforce the control permission when a VPN
is first being prepared, where such a preparation would bypass user
consent.
Also ensure that in this case, the VPN being prepared matches the
calling app. Otherwise an app could prepare another pre-consented VPN,
which is not particularly dangerous but is likely unexpected.
Finally, remove misleading comment in ConnectivityService#prepareVpn.
This method IS called from VpnService.prepare(), not only from
system-privileged apps.
Bug: 18442887
Change-Id: Ic3227c6c1c74312697f0576d3811b06692a4edff
diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index 03c05ec..c5aedd2 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -215,20 +215,11 @@
* @return true if the operation is succeeded.
*/
public synchronized boolean prepare(String oldPackage, String newPackage) {
- // Return false if the package does not match.
if (oldPackage != null && getAppUid(oldPackage, mUserHandle) != mOwnerUID) {
- // The package doesn't match. If this VPN was not previously authorized, return false
- // to force user authorization. Otherwise, revoke the VPN anyway.
+ // The package doesn't match. We return false (to obtain user consent) unless the user
+ // has already consented to that VPN package.
if (!oldPackage.equals(VpnConfig.LEGACY_VPN) && isVpnUserPreConsented(oldPackage)) {
- long token = Binder.clearCallingIdentity();
- try {
- // This looks bizarre, but it is what ConfirmDialog in VpnDialogs is doing when
- // the user clicks through to allow the VPN to consent. So we are emulating the
- // action of the dialog without actually showing it.
- prepare(null, oldPackage);
- } finally {
- Binder.restoreCallingIdentity(token);
- }
+ prepareInternal(oldPackage);
return true;
}
return false;
@@ -243,54 +234,58 @@
// Check if the caller is authorized.
enforceControlPermission();
- // Reset the interface.
- if (mInterface != null) {
- mStatusIntent = null;
- agentDisconnect();
- jniReset(mInterface);
- mInterface = null;
- mVpnUsers = null;
- }
+ prepareInternal(newPackage);
+ return true;
+ }
- // Revoke the connection or stop LegacyVpnRunner.
- if (mConnection != null) {
- try {
- mConnection.mService.transact(IBinder.LAST_CALL_TRANSACTION,
- Parcel.obtain(), null, IBinder.FLAG_ONEWAY);
- } catch (Exception e) {
- // ignore
- }
- mContext.unbindService(mConnection);
- mConnection = null;
- } else if (mLegacyVpnRunner != null) {
- mLegacyVpnRunner.exit();
- mLegacyVpnRunner = null;
- }
-
+ /** Prepare the VPN for the given package. Does not perform permission checks. */
+ private void prepareInternal(String newPackage) {
long token = Binder.clearCallingIdentity();
try {
- mNetd.denyProtect(mOwnerUID);
- } catch (Exception e) {
- Log.wtf(TAG, "Failed to disallow UID " + mOwnerUID + " to call protect() " + e);
+ // Reset the interface.
+ if (mInterface != null) {
+ mStatusIntent = null;
+ agentDisconnect();
+ jniReset(mInterface);
+ mInterface = null;
+ mVpnUsers = null;
+ }
+
+ // Revoke the connection or stop LegacyVpnRunner.
+ if (mConnection != null) {
+ try {
+ mConnection.mService.transact(IBinder.LAST_CALL_TRANSACTION,
+ Parcel.obtain(), null, IBinder.FLAG_ONEWAY);
+ } catch (Exception e) {
+ // ignore
+ }
+ mContext.unbindService(mConnection);
+ mConnection = null;
+ } else if (mLegacyVpnRunner != null) {
+ mLegacyVpnRunner.exit();
+ mLegacyVpnRunner = null;
+ }
+
+ try {
+ mNetd.denyProtect(mOwnerUID);
+ } catch (Exception e) {
+ Log.wtf(TAG, "Failed to disallow UID " + mOwnerUID + " to call protect() " + e);
+ }
+
+ Log.i(TAG, "Switched from " + mPackage + " to " + newPackage);
+ mPackage = newPackage;
+ mOwnerUID = getAppUid(newPackage, mUserHandle);
+ try {
+ mNetd.allowProtect(mOwnerUID);
+ } catch (Exception e) {
+ Log.wtf(TAG, "Failed to allow UID " + mOwnerUID + " to call protect() " + e);
+ }
+ mConfig = null;
+
+ updateState(DetailedState.IDLE, "prepare");
} finally {
Binder.restoreCallingIdentity(token);
}
-
- Log.i(TAG, "Switched from " + mPackage + " to " + newPackage);
- mPackage = newPackage;
- mOwnerUID = getAppUid(newPackage, mUserHandle);
- token = Binder.clearCallingIdentity();
- try {
- mNetd.allowProtect(mOwnerUID);
- } catch (Exception e) {
- Log.wtf(TAG, "Failed to allow UID " + mOwnerUID + " to call protect() " + e);
- } finally {
- Binder.restoreCallingIdentity(token);
- }
- mConfig = null;
-
- updateState(DetailedState.IDLE, "prepare");
- return true;
}
/**