Skip to content

Commit

Permalink
DO NOT MERGE
Browse files Browse the repository at this point in the history 
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:cd41ded936e3f5ce781fa9587ff602c9325d3d84)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:46ab5f5c7e9d3a61ba103f7c574b852bdffd9d92)
Merged-In: I30caed1481dff5af2223a8ff589846597cee8229
Change-Id: I30caed1481dff5af2223a8ff589846597cee8229
  • Loading branch information
@aoleary
Grant Menke authored and aoleary committed Sep 17, 2024
1 parent 957f165 commit 52282f9
Show file tree
Hide file tree
Showing 7 changed files with 453 additions and 2 deletions.
26 changes: 26 additions & 0 deletions src/com/android/server/telecom/Call.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,17 @@ public void onContactPhotoQueryComplete(Uri handle, CallerInfo callerInfo) {
/** The state of the call. */
private int mState;

/**
* Determines whether the {@link ConnectionService} has responded to the initial request to
* create the connection.
*
* {@code false} indicates the {@link Call} has been added to Telecom, but the
* {@link Connection} has not yet been returned by the associated {@link ConnectionService}.
* {@code true} indicates the {@link Call} has an associated {@link Connection} reported by the
* {@link ConnectionService}.
*/
private boolean mIsCreateConnectionComplete = false;

/** The handle with which to establish this call. */
private Uri mHandle;

Expand Down Expand Up @@ -1041,6 +1052,19 @@ public ConnectionServiceFocusManager.ConnectionServiceFocus getConnectionService
return mConnectionService;
}

/**
* @return {@code true} if the connection has been created by the underlying
* {@link ConnectionService}, {@code false} otherwise.
*/
public boolean isCreateConnectionComplete() {
return mIsCreateConnectionComplete;
}

@VisibleForTesting
public void setIsCreateConnectionComplete(boolean isCreateConnectionComplete) {
mIsCreateConnectionComplete = isCreateConnectionComplete;
}

@VisibleForTesting
public int getState() {
return mState;
Expand Down Expand Up @@ -2192,6 +2216,7 @@ public void handleCreateConferenceSuccess(
CallIdMapper idMapper,
ParcelableConference conference) {
Log.v(this, "handleCreateConferenceSuccessful %s", conference);
mIsCreateConnectionComplete = true;
setTargetPhoneAccount(conference.getPhoneAccount());
setHandle(conference.getHandle(), conference.getHandlePresentation());

Expand Down Expand Up @@ -2225,6 +2250,7 @@ public void handleCreateConnectionSuccess(
CallIdMapper idMapper,
ParcelableConnection connection) {
Log.v(this, "handleCreateConnectionSuccessful %s", connection);
mIsCreateConnectionComplete = true;
setTargetPhoneAccount(connection.getPhoneAccount());
setHandle(connection.getHandle(), connection.getHandlePresentation());
setCallerDisplayName(
Expand Down
74 changes: 72 additions & 2 deletions src/com/android/server/telecom/ConnectionServiceWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
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;
Expand All @@ -63,6 +64,11 @@
import java.util.Map;
import java.util.Set;
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;

/**
Expand All @@ -77,6 +83,11 @@ public class ConnectionServiceWrapper extends ServiceBinder implements

private static final String TELECOM_ABBREVIATION = "cast";

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<Call, ScheduledFuture<?>> mScheduledFutureMap = new ConcurrentHashMap<>(2);
private final class Adapter extends IConnectionServiceAdapter.Stub {

@Override
Expand All @@ -89,6 +100,12 @@ public void handleCreateConnectionComplete(String callId, ConnectionRequest requ
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();
Expand Down Expand Up @@ -126,6 +143,12 @@ public void handleCreateConferenceComplete(String callId, ConnectionRequest requ
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);

Expand Down Expand Up @@ -1253,7 +1276,8 @@ public void setCallDirection(String callId, int direction, Session.Info sessionI
* @param context The context.
* @param userHandle The {@link UserHandle} to use when binding.
*/
ConnectionServiceWrapper(
@VisibleForTesting
public ConnectionServiceWrapper(
ComponentName componentName,
ConnectionServiceRepository connectionServiceRepository,
PhoneAccountRegistrar phoneAccountRegistrar,
Expand Down Expand Up @@ -1350,6 +1374,26 @@ public void onSuccess() {
.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);
try {
mServiceInterface.createConference(
call.getConnectionManagerPhoneAccount(),
Expand Down Expand Up @@ -1452,6 +1496,26 @@ public void onSuccess() {
.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);
try {
mServiceInterface.createConnection(
call.getConnectionManagerPhoneAccount(),
Expand Down Expand Up @@ -1861,7 +1925,8 @@ void stopDtmfTone(Call call) {
}
}

void addCall(Call call) {
@VisibleForTesting
public void addCall(Call call) {
if (mCallIdMapper.getCallId(call) == null) {
mCallIdMapper.addCall(call);
}
Expand Down Expand Up @@ -2328,4 +2393,9 @@ public String toString() {
sb.append("]");
return sb.toString();
}

@VisibleForTesting
public void setScheduledExecutorService(ScheduledExecutorService service) {
mScheduledExecutor = service;
}
}
2 changes: 2 additions & 0 deletions src/com/android/server/telecom/LogUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,10 @@ public final static class Events {
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";
Expand Down
2 changes: 2 additions & 0 deletions tests/src/com/android/server/telecom/tests/BasicCallTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,7 @@ public void testOutgoingCallSelectPhoneAccountVideo() throws Exception {
call.setTargetPhoneAccount(mPhoneAccountA1.getAccountHandle());
assert(call.isVideoCallingSupportedByPhoneAccount());
assertEquals(VideoProfile.STATE_BIDIRECTIONAL, call.getVideoState());
call.setIsCreateConnectionComplete(true);
}

/**
Expand All @@ -1016,6 +1017,7 @@ public void testOutgoingCallSelectPhoneAccountNoVideo() throws Exception {
call.setTargetPhoneAccount(mPhoneAccountA2.getAccountHandle());
assert(!call.isVideoCallingSupportedByPhoneAccount());
assertEquals(VideoProfile.STATE_AUDIO_ONLY, call.getVideoState());
call.setIsCreateConnectionComplete(true);
}

/**
Expand Down
54 changes: 54 additions & 0 deletions tests/src/com/android/server/telecom/tests/CallsManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
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.content.ComponentName;
import android.content.ContentResolver;
Expand All @@ -49,6 +50,7 @@
import android.net.Uri;
import android.os.Bundle;
import android.os.Handler;
import android.os.IBinder;
import android.os.Looper;
import android.os.Process;
import android.os.SystemClock;
Expand All @@ -67,6 +69,7 @@
import android.util.Pair;
import android.widget.Toast;

import com.android.internal.telecom.IConnectionService;
import com.android.server.telecom.AsyncRingtonePlayer;
import com.android.server.telecom.Call;
import com.android.server.telecom.CallAudioManager;
Expand All @@ -80,6 +83,7 @@
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.EmergencyCallHelper;
import com.android.server.telecom.HeadsetMediaButton;
Expand Down Expand Up @@ -211,6 +215,7 @@ public class CallsManagerTest extends TelecomTestCase {
@Mock private RoleManagerAdapter mRoleManagerAdapter;
@Mock private ToastFactory mToastFactory;
@Mock private Toast mToast;
@Mock private IConnectionService mIConnectionService;

private CallsManager mCallsManager;

Expand Down Expand Up @@ -278,11 +283,19 @@ public void setUp() throws Exception {
eq(SIM_2_HANDLE), any())).thenReturn(SIM_2_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();
}

Expand Down Expand Up @@ -1695,6 +1708,32 @@ public void testCrossUserCallRedirectionEndEarlyForIncapablePhoneAccount() {
assertTrue(argumentCaptor.getValue().contains("Unavailable phoneAccountHandle"));
}

@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);
}
Expand Down Expand Up @@ -1787,4 +1826,19 @@ private void setupMsimAccounts() {
when(mPhoneAccountRegistrar.getSimPhoneAccountsOfCurrentUser()).thenReturn(
new ArrayList<>(Arrays.asList(SIM_1_HANDLE, SIM_2_HANDLE)));
}

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();
}
}
14 changes: 14 additions & 0 deletions tests/src/com/android/server/telecom/tests/ComponentContextFixture.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,14 @@ public void addConnectionService(
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,
Expand Down Expand Up @@ -777,6 +785,12 @@ private void addService(String action, ComponentName name, IInterface service) {
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<ResolveInfo> doQueryIntentServices(Intent intent, int flags) {
List<ResolveInfo> result = new ArrayList<>();
for (ComponentName componentName : mComponentNamesByAction.get(intent.getAction())) {
Expand Down
Loading

0 comments on commit 52282f9

Please sign in to comment.