From b21298a686b04d55ff97223dd317497845713f4b Mon Sep 17 00:00:00 2001
From: Jeff Davidson <jpd@google.com>
Date: Tue, 10 Feb 2015 10:02:11 -0800
Subject: [PATCH] Do not enforce CONTROL_VPN for calls from lockdown VPN.

Clearly document which methods in Vpn.java are designed to be used to
service a Binder call, and which must therefore check permissions and
clear the calling identity, and which methods are designed for
internal use only and which therefore need not check permission.

Add a new startLegacyVpnPrivileged method which bypasses the
permission checks, to be used by lockdown VPN which is a trusted
system service. Ensure that the existing startLegacyVpn method checks
permissions as this is used whenever we respond to a binder call.

Bug: 19311172
Change-Id: I34f13258ee7481f1356bc523124cf5db068b4972
---
 .../java/com/android/server/connectivity/Vpn.java  | 29 +++++++++++++++++++---
 .../com/android/server/net/LockdownVpnTracker.java |  8 +++---
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index f08a65222c9f..8533f69f705b 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -846,9 +846,29 @@ public class Vpn {
     /**
      * Start legacy VPN, controlling native daemons as needed. Creates a
      * secondary thread to perform connection work, returning quickly.
+     *
+     * Should only be called to respond to Binder requests as this enforces caller permission. Use
+     * {@link #startLegacyVpnPrivileged(VpnProfile, KeyStore, LinkProperties)} to skip the
+     * permission check only when the caller is trusted (or the call is initiated by the system).
      */
     public void startLegacyVpn(VpnProfile profile, KeyStore keyStore, LinkProperties egress) {
         enforceControlPermission();
+        long token = Binder.clearCallingIdentity();
+        try {
+            startLegacyVpnPrivileged(profile, keyStore, egress);
+        } finally {
+            Binder.restoreCallingIdentity(token);
+        }
+    }
+
+    /**
+     * Like {@link #startLegacyVpn(VpnProfile, KeyStore, LinkProperties)}, but does not check
+     * permissions under the assumption that the caller is the system.
+     *
+     * Callers are responsible for checking permissions if needed.
+     */
+    public void startLegacyVpnPrivileged(VpnProfile profile, KeyStore keyStore,
+            LinkProperties egress) {
         if (!keyStore.isUnlocked()) {
             throw new IllegalStateException("KeyStore isn't unlocked");
         }
@@ -959,10 +979,10 @@ public class Vpn {
     }
 
     private synchronized void startLegacyVpn(VpnConfig config, String[] racoon, String[] mtpd) {
-        stopLegacyVpn();
+        stopLegacyVpnPrivileged();
 
-        // Prepare for the new request. This also checks the caller.
-        prepare(null, VpnConfig.LEGACY_VPN);
+        // Prepare for the new request.
+        prepareInternal(VpnConfig.LEGACY_VPN);
         updateState(DetailedState.CONNECTING, "startLegacyVpn");
 
         // Start a new LegacyVpnRunner and we are done!
@@ -970,7 +990,8 @@ public class Vpn {
         mLegacyVpnRunner.start();
     }
 
-    public synchronized void stopLegacyVpn() {
+    /** Stop legacy VPN. Permissions must be checked by callers. */
+    public synchronized void stopLegacyVpnPrivileged() {
         if (mLegacyVpnRunner != null) {
             mLegacyVpnRunner.exit();
             mLegacyVpnRunner = null;
diff --git a/services/core/java/com/android/server/net/LockdownVpnTracker.java b/services/core/java/com/android/server/net/LockdownVpnTracker.java
index 3a1e4a4301ed..752614f2ec58 100644
--- a/services/core/java/com/android/server/net/LockdownVpnTracker.java
+++ b/services/core/java/com/android/server/net/LockdownVpnTracker.java
@@ -140,7 +140,7 @@ public class LockdownVpnTracker {
         if (egressDisconnected || egressChanged) {
             clearSourceRulesLocked();
             mAcceptedEgressIface = null;
-            mVpn.stopLegacyVpn();
+            mVpn.stopLegacyVpnPrivileged();
         }
         if (egressDisconnected) {
             hideNotification();
@@ -163,7 +163,9 @@ public class LockdownVpnTracker {
 
                 mAcceptedEgressIface = egressProp.getInterfaceName();
                 try {
-                    mVpn.startLegacyVpn(mProfile, KeyStore.getInstance(), egressProp);
+                    // Use the privileged method because Lockdown VPN is initiated by the system, so
+                    // no additional permission checks are necessary.
+                    mVpn.startLegacyVpnPrivileged(mProfile, KeyStore.getInstance(), egressProp);
                 } catch (IllegalStateException e) {
                     mAcceptedEgressIface = null;
                     Slog.e(TAG, "Failed to start VPN", e);
@@ -250,7 +252,7 @@ public class LockdownVpnTracker {
         mAcceptedEgressIface = null;
         mErrorCount = 0;
 
-        mVpn.stopLegacyVpn();
+        mVpn.stopLegacyVpnPrivileged();
         try {
             mNetService.setFirewallEgressDestRule(mProfile.server, 500, false);
             mNetService.setFirewallEgressDestRule(mProfile.server, 4500, false);
-- 
2.11.0