Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit eeaf8557 authored by Chalard Jean's avatar Chalard Jean Committed by Cherrypicker Worker
Browse files

Fix: VPN is off but key is displayed in the status bar 

There seem to be a narrow race condition in which it's
possible to turn the VPN off where it won't null out
the config. It's possible to make this happen by trying
to migrate to a network that won't carry the VPN packets
(UDP 4500 or ESP, respectively) and turning off the VPN
while the IKE session is trying to establish.

Alternatively, this might be a visibility issue where
mConfig is nulled out by a thread, but the binder
thread reading it from #getVpnConfig returns the old
object, not yet nulled out. To fix this, make mConfig
guarded by "this", which it almost was already.

Bug: 289187571
Test: manual
      Turning off underlying networks, VPN stays "connected"
      in settings
      Adding a settings VPN, making sure the status is correct
      Try and fail to cause the race condition
(cherry picked from https://android-review.googlesource.com/q/commit:25181986301828f8be5101bb6b08a72dda99b0ad)
Merged-In: I1c19eb33f0c1d0f1ccca6ac17a709f8bc1ad9f94
Change-Id: I1c19eb33f0c1d0f1ccca6ac17a709f8bc1ad9f94
parent fa6cb652

services/core/java/com/android/server/connectivity/Vpn.java

+70 −55
Original line number Diff line number Diff line
@@ -380,6 +380,7 @@ public class Vpn { 
    private final INetworkManagementService mNms;

    private final INetd mNetd;

    @VisibleForTesting

    @GuardedBy("this")

    protected VpnConfig mConfig;

    private final NetworkProvider mNetworkProvider;

    @VisibleForTesting
@@ -1598,6 +1599,8 @@ public class Vpn { 
        return network;

    }



    // TODO : this is not synchronized(this) but reads from mConfig, which is dangerous

    // This file makes an effort to avoid partly initializing mConfig, but this is still not great

    private LinkProperties makeLinkProperties() {

        // The design of disabling IPv6 is only enabled for IKEv2 VPN because it needs additional

        // logic to handle IPv6 only VPN, and the IPv6 only VPN may be restarted when its MTU
@@ -1679,6 +1682,7 @@ public class Vpn { 
     * registering a new NetworkAgent. This is not always possible if the new VPN configuration

     * has certain changes, in which case this method would just return {@code false}.

     */

    // TODO : this method is not synchronized(this) but reads from mConfig

    private boolean updateLinkPropertiesInPlaceIfPossible(NetworkAgent agent, VpnConfig oldConfig) {

        // NetworkAgentConfig cannot be updated without registering a new NetworkAgent.

        // Strictly speaking, bypassability is affected by lockdown and therefore it's possible
@@ -2269,7 +2273,12 @@ public class Vpn { 
     */

    public synchronized VpnConfig getVpnConfig() {

        enforceControlPermission();

        return mConfig;

        // Constructor of VpnConfig cannot take a null parameter. Return null directly if mConfig is

        // null

        if (mConfig == null) return null;

        // mConfig is guarded by "this" and can be modified by another thread as soon as

        // this method returns, so this method must return a copy.

        return new VpnConfig(mConfig);

    }



    @Deprecated
@@ -2315,6 +2324,7 @@ public class Vpn { 
        }

    };



    @GuardedBy("this")

    private void cleanupVpnStateLocked() {

        mStatusIntent = null;

        resetNetworkCapabilities();
@@ -2837,9 +2847,7 @@ public class Vpn { 
        }



        final boolean isLegacyVpn = mVpnRunner instanceof LegacyVpnRunner;



        mVpnRunner.exit();

        mVpnRunner = null;



        // LegacyVpn uses daemons that must be shut down before new ones are brought up.

        // The same limitation does not apply to Platform VPNs.
@@ -3084,6 +3092,7 @@ public class Vpn { 
                    }

        };



        // GuardedBy("Vpn.this") (annotation can't be applied to constructor)

        IkeV2VpnRunner(

                @NonNull Ikev2VpnProfile profile, @NonNull ScheduledThreadPoolExecutor executor) {

            super(TAG);
@@ -3710,11 +3719,14 @@ public class Vpn { 
        }



        public void updateVpnTransportInfoAndNetCap(int keepaliveDelaySec) {

            final VpnTransportInfo info = new VpnTransportInfo(

            final VpnTransportInfo info;

            synchronized (Vpn.this) {

                info = new VpnTransportInfo(

                        getActiveVpnType(),

                        mConfig.session,

                        mConfig.allowBypass && !mLockdown,

                        areLongLivedTcpConnectionsExpensive(keepaliveDelaySec));

            }

            final boolean ncUpdateRequired = !info.equals(mNetworkCapabilities.getTransportInfo());

            if (ncUpdateRequired) {

                mNetworkCapabilities = new NetworkCapabilities.Builder(mNetworkCapabilities)
@@ -4220,7 +4232,7 @@ public class Vpn { 
         * consistency of the Ikev2VpnRunner fields.

         */

        private void disconnectVpnRunner() {

            mEventChanges.log("[VPNRunner] Disconnect runner, underlying network" + mActiveNetwork);

            mEventChanges.log("[VPNRunner] Disconnect runner, underlying net " + mActiveNetwork);

            mActiveNetwork = null;

            mUnderlyingNetworkCapabilities = null;

            mUnderlyingLinkProperties = null;
@@ -4293,6 +4305,7 @@ public class Vpn { 
            }

        };



        // GuardedBy("Vpn.this") (annotation can't be applied to constructor)

        LegacyVpnRunner(VpnConfig config, String[] racoon, String[] mtpd, VpnProfile profile) {

            super(TAG);

            if (racoon == null && mtpd == null) {
@@ -4500,6 +4513,7 @@ public class Vpn { 
                }



                // Set the interface and the addresses in the config.

                synchronized (Vpn.this) {

                    mConfig.interfaze = parameters[0].trim();



                    mConfig.addLegacyAddresses(parameters[1]);
@@ -4539,7 +4553,6 @@ public class Vpn { 
                    }



                    // Here is the last step and it must be done synchronously.

                synchronized (Vpn.this) {

                    // Set the start time

                    mConfig.startTime = SystemClock.elapsedRealtime();
@@ -4773,25 +4786,26 @@ public class Vpn { 


        try {

            // Build basic config

            mConfig = new VpnConfig();

            final VpnConfig config = new VpnConfig();

            if (VpnConfig.LEGACY_VPN.equals(packageName)) {

                mConfig.legacy = true;

                mConfig.session = profile.name;

                mConfig.user = profile.key;

                config.legacy = true;

                config.session = profile.name;

                config.user = profile.key;



                // TODO: Add support for configuring meteredness via Settings. Until then, use a

                // safe default.

                mConfig.isMetered = true;

                config.isMetered = true;

            } else {

                mConfig.user = packageName;

                mConfig.isMetered = profile.isMetered;

                config.user = packageName;

                config.isMetered = profile.isMetered;

            }

            mConfig.startTime = SystemClock.elapsedRealtime();

            mConfig.proxyInfo = profile.proxy;

            mConfig.requiresInternetValidation = profile.requiresInternetValidation;

            mConfig.excludeLocalRoutes = profile.excludeLocalRoutes;

            mConfig.allowBypass = profile.isBypassable;

            mConfig.disallowedApplications = getAppExclusionList(mPackage);

            config.startTime = SystemClock.elapsedRealtime();

            config.proxyInfo = profile.proxy;

            config.requiresInternetValidation = profile.requiresInternetValidation;

            config.excludeLocalRoutes = profile.excludeLocalRoutes;

            config.allowBypass = profile.isBypassable;

            config.disallowedApplications = getAppExclusionList(mPackage);

            mConfig = config;



            switch (profile.type) {

                case VpnProfile.TYPE_IKEV2_IPSEC_USER_PASS:
@@ -4805,6 +4819,7 @@ public class Vpn { 
                    mVpnRunner.start();

                    break;

                default:

                    mConfig = null;

                    updateState(DetailedState.FAILED, "Invalid platform VPN type");

                    Log.d(TAG, "Unknown VPN profile type: " + profile.type);

                    break;