From e0a87ee6d723063456adc6c8263de202a8b0bfac Mon Sep 17 00:00:00 2001 From: Grant Menke Date: Thu, 25 Apr 2024 10:43:43 -0700 Subject: [PATCH] DO NOT MERGE Unbind CS if connection is not created within 15 seconds. This CL adds a check to ensure that connection creation occurs within 15 seconds after binding to that ConnectionService. If the connection/conference is not created in that timespan, this CL adds logic to manually unbind the ConnectionService at that point in time. This prevents malicious apps from keeping a declared permission in forever even in the background. Bug: 293458004 Test: manually using the provided apk + atest CallsManagerTest Flag: EXEMPT Security High/Critical Severity CVE (cherry picked from commit 7aa55ffca65d6166145fd9660e0f7340c07053bf) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3706488d7f0e63feb560ef180fe6e83a52e80834) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:635e2feeb8ee6f84786e4c405c1e8fa39e6743cb) Merged-In: I30caed1481dff5af2223a8ff589846597cee8229 Change-Id: I30caed1481dff5af2223a8ff589846597cee8229 --- src/com/android/server/telecom/Call.java | 1 + .../telecom/ConnectionServiceWrapper.java | 70 ++++++++++++++++++- src/com/android/server/telecom/LogUtils.java | 2 + .../server/telecom/tests/BasicCallTests.java | 2 + .../telecom/tests/CallsManagerTest.java | 53 ++++++++++++++ .../tests/ComponentContextFixture.java | 14 ++++ 6 files changed, 140 insertions(+), 2 deletions(-) diff --git a/src/com/android/server/telecom/Call.java b/src/com/android/server/telecom/Call.java index e29a6c86c..529bd812d 100644 --- a/src/com/android/server/telecom/Call.java +++ b/src/com/android/server/telecom/Call.java @@ -1134,6 +1134,7 @@ public class Call implements CreateConnectionResponse, EventManager.Loggable, return (!mIsTransactionalCall ? mConnectionService : mTransactionalService); } + @VisibleForTesting public int getState() { return mState; } diff --git a/src/com/android/server/telecom/ConnectionServiceWrapper.java b/src/com/android/server/telecom/ConnectionServiceWrapper.java index a3264c4af..94bbdd308 100755 --- a/src/com/android/server/telecom/ConnectionServiceWrapper.java +++ b/src/com/android/server/telecom/ConnectionServiceWrapper.java @@ -45,6 +45,7 @@ import android.telecom.ConnectionService; import android.telecom.DisconnectCause; import android.telecom.GatewayInfo; import android.telecom.Log; +import android.telecom.Logging.Runnable; import android.telecom.Logging.Session; import android.telecom.ParcelableConference; import android.telecom.ParcelableConnection; @@ -73,12 +74,14 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; -import java.util.Objects; /** * Wrapper for {@link IConnectionService}s, handles binding to {@link IConnectionService} and keeps @@ -95,6 +98,11 @@ public class ConnectionServiceWrapper extends ServiceBinder implements private @Nullable CancellationSignal mOngoingQueryLocationRequest = null; private final ExecutorService mQueryLocationExecutor = Executors.newSingleThreadExecutor(); + private static final long SERVICE_BINDING_TIMEOUT = 15000L; + private ScheduledExecutorService mScheduledExecutor = + Executors.newSingleThreadScheduledExecutor(); + // Pre-allocate space for 2 calls; realistically thats all we should ever need (tm) + private final Map> mScheduledFutureMap = new ConcurrentHashMap<>(2); private final class Adapter extends IConnectionServiceAdapter.Stub { @Override @@ -107,6 +115,12 @@ public class ConnectionServiceWrapper extends ServiceBinder implements try { synchronized (mLock) { logIncoming("handleCreateConnectionComplete %s", callId); + Call call = mCallIdMapper.getCall(callId); + if (mScheduledFutureMap.containsKey(call)) { + ScheduledFuture existingTimeout = mScheduledFutureMap.get(call); + existingTimeout.cancel(false /* cancelIfRunning */); + mScheduledFutureMap.remove(call); + } // Check status hints image for cross user access if (connection.getStatusHints() != null) { Icon icon = connection.getStatusHints().getIcon(); @@ -144,6 +158,12 @@ public class ConnectionServiceWrapper extends ServiceBinder implements try { synchronized (mLock) { logIncoming("handleCreateConferenceComplete %s", callId); + Call call = mCallIdMapper.getCall(callId); + if (mScheduledFutureMap.containsKey(call)) { + ScheduledFuture existingTimeout = mScheduledFutureMap.get(call); + existingTimeout.cancel(false /* cancelIfRunning */); + mScheduledFutureMap.remove(call); + } ConnectionServiceWrapper.this .handleCreateConferenceComplete(callId, request, conference); @@ -1597,6 +1617,26 @@ public class ConnectionServiceWrapper extends ServiceBinder implements .setIsAdhocConferenceCall(call.isAdhocConferenceCall()) .build(); + Runnable r = new Runnable("CSW.cC", mLock) { + @Override + public void loggedRun() { + if (!call.isCreateConnectionComplete()) { + Log.e(this, new Exception(), + "Conference %s creation timeout", + getComponentName()); + Log.addEvent(call, LogUtils.Events.CREATE_CONFERENCE_TIMEOUT, + Log.piiHandle(call.getHandle()) + " via:" + + getComponentName().getPackageName()); + response.handleCreateConferenceFailure( + new DisconnectCause(DisconnectCause.ERROR)); + } + } + }; + // Post cleanup to the executor service and cache the future, so we can cancel it if + // needed. + ScheduledFuture future = mScheduledExecutor.schedule(r.getRunnableToCancel(), + SERVICE_BINDING_TIMEOUT, TimeUnit.MILLISECONDS); + mScheduledFutureMap.put(call, future); if (mServiceInterface != null) { try { mServiceInterface.createConference( @@ -1705,6 +1745,26 @@ public class ConnectionServiceWrapper extends ServiceBinder implements .setRttPipeToInCall(call.getCsToInCallRttPipeForCs()) .build(); + Runnable r = new Runnable("CSW.cC", mLock) { + @Override + public void loggedRun() { + if (!call.isCreateConnectionComplete()) { + Log.e(this, new Exception(), + "Connection %s creation timeout", + getComponentName()); + Log.addEvent(call, LogUtils.Events.CREATE_CONNECTION_TIMEOUT, + Log.piiHandle(call.getHandle()) + " via:" + + getComponentName().getPackageName()); + response.handleCreateConnectionFailure( + new DisconnectCause(DisconnectCause.ERROR)); + } + } + }; + // Post cleanup to the executor service and cache the future, so we can cancel it if + // needed. + ScheduledFuture future = mScheduledExecutor.schedule(r.getRunnableToCancel(), + SERVICE_BINDING_TIMEOUT, TimeUnit.MILLISECONDS); + mScheduledFutureMap.put(call, future); if (mServiceInterface != null) { try { mServiceInterface.createConnection( @@ -2181,7 +2241,8 @@ public class ConnectionServiceWrapper extends ServiceBinder implements } } - void addCall(Call call) { + @VisibleForTesting + public void addCall(Call call) { if (mCallIdMapper.getCallId(call) == null) { mCallIdMapper.addCall(call); } @@ -2658,4 +2719,9 @@ public class ConnectionServiceWrapper extends ServiceBinder implements sb.append("]"); return sb.toString(); } + + @VisibleForTesting + public void setScheduledExecutorService(ScheduledExecutorService service) { + mScheduledExecutor = service; + } } diff --git a/src/com/android/server/telecom/LogUtils.java b/src/com/android/server/telecom/LogUtils.java index 0d6acd51d..d98ebfe6b 100644 --- a/src/com/android/server/telecom/LogUtils.java +++ b/src/com/android/server/telecom/LogUtils.java @@ -139,8 +139,10 @@ public class LogUtils { public static final String STOP_CALL_WAITING_TONE = "STOP_CALL_WAITING_TONE"; public static final String START_CONNECTION = "START_CONNECTION"; public static final String CREATE_CONNECTION_FAILED = "CREATE_CONNECTION_FAILED"; + public static final String CREATE_CONNECTION_TIMEOUT = "CREATE_CONNECTION_TIMEOUT"; public static final String START_CONFERENCE = "START_CONFERENCE"; public static final String CREATE_CONFERENCE_FAILED = "CREATE_CONFERENCE_FAILED"; + public static final String CREATE_CONFERENCE_TIMEOUT = "CREATE_CONFERENCE_TIMEOUT"; public static final String BIND_CS = "BIND_CS"; public static final String CS_BOUND = "CS_BOUND"; public static final String CONFERENCE_WITH = "CONF_WITH"; diff --git a/tests/src/com/android/server/telecom/tests/BasicCallTests.java b/tests/src/com/android/server/telecom/tests/BasicCallTests.java index bd81a2ffe..51c3b33a9 100644 --- a/tests/src/com/android/server/telecom/tests/BasicCallTests.java +++ b/tests/src/com/android/server/telecom/tests/BasicCallTests.java @@ -995,6 +995,7 @@ public class BasicCallTests extends TelecomSystemTest { call.setTargetPhoneAccount(mPhoneAccountA1.getAccountHandle()); assert(call.isVideoCallingSupportedByPhoneAccount()); assertEquals(VideoProfile.STATE_BIDIRECTIONAL, call.getVideoState()); + call.setIsCreateConnectionComplete(true); } /** @@ -1018,6 +1019,7 @@ public class BasicCallTests extends TelecomSystemTest { call.setTargetPhoneAccount(mPhoneAccountA2.getAccountHandle()); assert(!call.isVideoCallingSupportedByPhoneAccount()); assertEquals(VideoProfile.STATE_AUDIO_ONLY, call.getVideoState()); + call.setIsCreateConnectionComplete(true); } /** diff --git a/tests/src/com/android/server/telecom/tests/CallsManagerTest.java b/tests/src/com/android/server/telecom/tests/CallsManagerTest.java index 56cf22fee..5134f7c23 100644 --- a/tests/src/com/android/server/telecom/tests/CallsManagerTest.java +++ b/tests/src/com/android/server/telecom/tests/CallsManagerTest.java @@ -43,6 +43,7 @@ import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static java.lang.Thread.sleep; import android.Manifest; import android.content.ComponentName; @@ -54,6 +55,7 @@ import android.media.AudioManager; import android.net.Uri; import android.os.Bundle; import android.os.Handler; +import android.os.IBinder; import android.os.Looper; import android.os.OutcomeReceiver; import android.os.Process; @@ -80,6 +82,7 @@ import android.test.suitebuilder.annotation.SmallTest; import android.util.Pair; import android.widget.Toast; +import com.android.internal.telecom.IConnectionService; import com.android.server.telecom.AnomalyReporterAdapter; import com.android.server.telecom.AsyncRingtonePlayer; import com.android.server.telecom.Call; @@ -97,6 +100,7 @@ import com.android.server.telecom.ClockProxy; import com.android.server.telecom.ConnectionServiceFocusManager; import com.android.server.telecom.ConnectionServiceFocusManager.ConnectionServiceFocusManagerFactory; import com.android.server.telecom.ConnectionServiceWrapper; +import com.android.server.telecom.CreateConnectionResponse; import com.android.server.telecom.DefaultDialerCache; import com.android.server.telecom.EmergencyCallDiagnosticLogger; import com.android.server.telecom.EmergencyCallHelper; @@ -277,6 +281,7 @@ public class CallsManagerTest extends TelecomTestCase { @Mock private BlockedNumbersAdapter mBlockedNumbersAdapter; @Mock private PhoneCapability mPhoneCapability; @Mock private CallStreamingNotification mCallStreamingNotification; + @Mock private IConnectionService mIConnectionService; private CallsManager mCallsManager; @@ -362,11 +367,19 @@ public class CallsManagerTest extends TelecomTestCase { eq(WORK_HANDLE), any())).thenReturn(WORK_ACCOUNT); when(mToastFactory.makeText(any(), anyInt(), anyInt())).thenReturn(mToast); when(mToastFactory.makeText(any(), any(), anyInt())).thenReturn(mToast); + when(mIConnectionService.asBinder()).thenReturn(mock(IBinder.class)); + + mComponentContextFixture.addConnectionService(new ComponentName(mContext.getPackageName(), + mContext.getPackageName().getClass().getName()), mIConnectionService); } @Override @After public void tearDown() throws Exception { + mComponentContextFixture.removeConnectionService( + new ComponentName(mContext.getPackageName(), + mContext.getPackageName().getClass().getName()), + mock(IConnectionService.class)); super.tearDown(); } @@ -3252,6 +3265,31 @@ public class CallsManagerTest extends TelecomTestCase { assertTrue(mCallsManager.isInSelfManagedCall(TEST_PACKAGE_NAME, TEST_USER_HANDLE)); } + @Test + public void testConnectionServiceCreateConnectionTimeout() throws Exception { + ConnectionServiceWrapper service = new ConnectionServiceWrapper(new ComponentName( + mContext.getPackageName(), mContext.getPackageName().getClass().getName()), null, + mPhoneAccountRegistrar, mCallsManager, mContext, mLock, null); + TestScheduledExecutorService scheduledExecutorService = new TestScheduledExecutorService(); + service.setScheduledExecutorService(scheduledExecutorService); + Call call = addSpyCall(); + service.addCall(call); + when(call.isCreateConnectionComplete()).thenReturn(false); + CreateConnectionResponse response = mock(CreateConnectionResponse.class); + + service.createConnection(call, response); + waitUntilConditionIsTrueOrTimeout(new Condition() { + @Override + public Object expected() { + return true; + } + + @Override + public Object actual() { + return scheduledExecutorService.isRunnableScheduledAtTime(15000L); + } + }, 5000L, "Expected job failed to schedule"); + } private Call addSpyCall() { return addSpyCall(SIM_2_HANDLE, CallState.ACTIVE); @@ -3355,4 +3393,19 @@ public class CallsManagerTest extends TelecomTestCase { when(mockTelephonyManager.getPhoneCapability()).thenReturn(mPhoneCapability); when(mPhoneCapability.getMaxActiveVoiceSubscriptions()).thenReturn(num); } + + private void waitUntilConditionIsTrueOrTimeout(Condition condition, long timeout, + String description) throws InterruptedException { + final long start = System.currentTimeMillis(); + while (!condition.expected().equals(condition.actual()) + && System.currentTimeMillis() - start < timeout) { + sleep(50); + } + assertEquals(description, condition.expected(), condition.actual()); + } + + protected interface Condition { + Object expected(); + Object actual(); + } } diff --git a/tests/src/com/android/server/telecom/tests/ComponentContextFixture.java b/tests/src/com/android/server/telecom/tests/ComponentContextFixture.java index cc22de291..df855e937 100644 --- a/tests/src/com/android/server/telecom/tests/ComponentContextFixture.java +++ b/tests/src/com/android/server/telecom/tests/ComponentContextFixture.java @@ -735,6 +735,14 @@ public class ComponentContextFixture implements TestFixture { mServiceInfoByComponentName.put(componentName, serviceInfo); } + public void removeConnectionService( + ComponentName componentName, + IConnectionService service) + throws Exception { + removeService(ConnectionService.SERVICE_INTERFACE, componentName, service); + mServiceInfoByComponentName.remove(componentName); + } + public void addInCallService( ComponentName componentName, IInCallService service, @@ -828,6 +836,12 @@ public class ComponentContextFixture implements TestFixture { mComponentNameByService.put(service, name); } + private void removeService(String action, ComponentName name, IInterface service) { + mComponentNamesByAction.remove(action, name); + mServiceByComponentName.remove(name); + mComponentNameByService.remove(service); + } + private List doQueryIntentServices(Intent intent, int flags) { List result = new ArrayList<>(); for (ComponentName componentName : mComponentNamesByAction.get(intent.getAction())) {