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

2 files changed, 47 insertions, 52 deletions

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 17889ea..4bfc3ea 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -2813,7 +2813,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     /**
-     * Prepare for a VPN application. This method is used by system-privileged apps.
+     * Prepare for a VPN application.
      * Permissions are checked in Vpn class.
      * @hide
      */
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 @@ public class Vpn {
      * @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 @@ public class Vpn {
         // 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;
+    }
+
+    /** Prepare the VPN for the given package. Does not perform permission checks. */
+    private void prepareInternal(String newPackage) {
+        long token = Binder.clearCallingIdentity();
+        try {
+            // 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;
+            }
 
-        // Revoke the connection or stop LegacyVpnRunner.
-        if (mConnection != null) {
             try {
-                mConnection.mService.transact(IBinder.LAST_CALL_TRANSACTION,
-                        Parcel.obtain(), null, IBinder.FLAG_ONEWAY);
+                mNetd.denyProtect(mOwnerUID);
             } catch (Exception e) {
-                // ignore
+                Log.wtf(TAG, "Failed to disallow UID " + mOwnerUID + " to call protect() " + e);
             }
-            mContext.unbindService(mConnection);
-            mConnection = null;
-        } else if (mLegacyVpnRunner != null) {
-            mLegacyVpnRunner.exit();
-            mLegacyVpnRunner = null;
-        }
 
-        long token = Binder.clearCallingIdentity();
-        try {
-            mNetd.denyProtect(mOwnerUID);
-        } catch (Exception e) {
-            Log.wtf(TAG, "Failed to disallow UID " + mOwnerUID + " to call protect() " + e);
-        } finally {
-            Binder.restoreCallingIdentity(token);
-        }
+            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;
 
-        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);
+            updateState(DetailedState.IDLE, "prepare");
         } finally {
             Binder.restoreCallingIdentity(token);
         }
-        mConfig = null;
-
-        updateState(DetailedState.IDLE, "prepare");
-        return true;
     }
 
     /**