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

Commit cb464900 authored by Pranav Madapurmath's avatar Pranav Madapurmath Committed by Android Build Coastguard Worker
Browse files

Resolve StatusHints image exploit across user. 

Because of the INTERACT_ACROSS_USERS permission, an app that implements
a ConnectionService can upload an image icon belonging to another user
by setting it in the StatusHints. Validating the construction of the
StatusHints on the calling user would prevent a malicious app from
registering a connection service with the embedded image icon from a
different user.

From additional feedback, this CL also addresses potential
vulnerabilities in an app being able to directly invoke the binder for a
means to manipulate the contents of the bundle that are passed with it.
The targeted points of entry are in ConnectionServiceWrapper for the
following APIs: handleCreateConnectionComplete, setStatusHints,
addConferenceCall, and addExistingConnection.

Fixes: 280797684
Test: Manual (verified that original exploit is no longer an issue).
Test: Unit test for validating image in StatusHints constructor.
Test: Unit tests to address vulnerabilities via the binder.
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:49d19dd265bee669b230efa29bf98c83650efea6)
Merged-In: Ie1f6a8866d31d5f1099dd0630cf8e9ee782d389c
Change-Id: Ie1f6a8866d31d5f1099dd0630cf8e9ee782d389c
parent dc6a8206

src/com/android/server/telecom/ConnectionServiceWrapper.java

100755 → 100644
+31 −0
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ import android.app.AppOpsManager; 
import android.content.ComponentName;

import android.content.Context;

import android.content.pm.PackageManager;

import android.graphics.drawable.Icon;

import android.net.Uri;

import android.os.Binder;

import android.os.Bundle;
@@ -78,10 +79,17 @@ public class ConnectionServiceWrapper extends ServiceBinder implements 
                ParcelableConnection connection, Session.Info sessionInfo) {

            Log.startSession(sessionInfo, LogUtils.Sessions.CSW_HANDLE_CREATE_CONNECTION_COMPLETE,

                    mPackageAbbreviation);

            UserHandle callingUserHandle = Binder.getCallingUserHandle();

            long token = Binder.clearCallingIdentity();

            try {

                synchronized (mLock) {

                    logIncoming("handleCreateConnectionComplete %s", callId);

                    // Check status hints image for cross user access

                    if (connection.getStatusHints() != null) {

                        Icon icon = connection.getStatusHints().getIcon();

                        connection.getStatusHints().setIcon(StatusHints.

                                validateAccountIconUserBoundary(icon, callingUserHandle));

                    }

                    ConnectionServiceWrapper.this

                            .handleCreateConnectionComplete(callId, request, connection);
@@ -476,6 +484,14 @@ public class ConnectionServiceWrapper extends ServiceBinder implements 
            Log.startSession(sessionInfo, LogUtils.Sessions.CSW_ADD_CONFERENCE_CALL,

                    mPackageAbbreviation);



            UserHandle callingUserHandle = Binder.getCallingUserHandle();

            // Check status hints image for cross user access

            if (parcelableConference.getStatusHints() != null) {

                Icon icon = parcelableConference.getStatusHints().getIcon();

                parcelableConference.getStatusHints().setIcon(StatusHints.

                        validateAccountIconUserBoundary(icon, callingUserHandle));

            }



            if (parcelableConference.getConnectElapsedTimeMillis() != 0

                    && mContext.checkCallingOrSelfPermission(MODIFY_PHONE_STATE)

                            != PackageManager.PERMISSION_GRANTED) {
@@ -721,10 +737,17 @@ public class ConnectionServiceWrapper extends ServiceBinder implements 
        public void setStatusHints(String callId, StatusHints statusHints,

                Session.Info sessionInfo) {

            Log.startSession(sessionInfo, "CSW.sSH", mPackageAbbreviation);

            UserHandle callingUserHandle = Binder.getCallingUserHandle();

            long token = Binder.clearCallingIdentity();

            try {

                synchronized (mLock) {

                    logIncoming("setStatusHints %s %s", callId, statusHints);

                    // Check status hints image for cross user access

                    if (statusHints != null) {

                        Icon icon = statusHints.getIcon();

                        statusHints.setIcon(StatusHints.validateAccountIconUserBoundary(

                                icon, callingUserHandle));

                    }

                    Call call = mCallIdMapper.getCall(callId);

                    if (call != null) {

                        call.setStatusHints(statusHints);
@@ -917,6 +940,14 @@ public class ConnectionServiceWrapper extends ServiceBinder implements 
                        } else {

                            connectIdToCheck = callId;

                        }



                        // Check status hints image for cross user access

                        if (connection.getStatusHints() != null) {

                            Icon icon = connection.getStatusHints().getIcon();

                            connection.getStatusHints().setIcon(StatusHints.

                                    validateAccountIconUserBoundary(icon, userHandle));

                        }



                        // Check to see if this Connection has already been added.

                        Call alreadyAddedConnection = mCallsManager

                                .getAlreadyAddedConnection(connectIdToCheck);

tests/src/com/android/server/telecom/tests/BasicCallTests.java

+158 −8
Original line number Diff line number Diff line
@@ -16,8 +16,11 @@ 


package com.android.server.telecom.tests;



import static com.android.server.telecom.tests.ConnectionServiceFixture.STATUS_HINTS_EXTRA;



import static org.junit.Assert.assertEquals;

import static org.junit.Assert.assertFalse;

import static org.junit.Assert.assertNotNull;

import static org.junit.Assert.assertNull;

import static org.junit.Assert.assertTrue;

import static org.mockito.ArgumentMatchers.nullable;
@@ -35,9 +38,13 @@ import static org.mockito.Mockito.when; 


import android.content.Context;

import android.content.IContentProvider;

import android.content.Intent;

import android.graphics.drawable.Icon;

import android.media.AudioManager;

import android.net.Uri;

import android.os.Bundle;

import android.os.Handler;

import android.os.Looper;

import android.os.Process;

import android.provider.BlockedNumberContract;

import android.telecom.Call;
@@ -49,12 +56,14 @@ import android.telecom.Log; 
import android.telecom.ParcelableCall;

import android.telecom.PhoneAccount;

import android.telecom.PhoneAccountHandle;

import android.telecom.StatusHints;

import android.telecom.TelecomManager;

import android.telecom.VideoProfile;

import android.test.suitebuilder.annotation.LargeTest;

import android.test.suitebuilder.annotation.MediumTest;



import androidx.test.filters.FlakyTest;

import androidx.test.filters.SmallTest;



import com.android.internal.telecom.IInCallAdapter;

import android.telecom.CallerInfo;
@@ -180,7 +189,7 @@ public class BasicCallTests extends TelecomSystemTest { 
    @Test

    public void testTelecomManagerAcceptRingingVideoCall() throws Exception {

        IdPair ids = startIncomingPhoneCall("650-555-1212", mPhoneAccountA0.getAccountHandle(),

                VideoProfile.STATE_BIDIRECTIONAL, mConnectionServiceFixtureA);

                VideoProfile.STATE_BIDIRECTIONAL, mConnectionServiceFixtureA, null);



        assertEquals(Call.STATE_RINGING, mInCallServiceFixtureX.getCall(ids.mCallId).getState());

        assertEquals(Call.STATE_RINGING, mInCallServiceFixtureY.getCall(ids.mCallId).getState());
@@ -209,7 +218,7 @@ public class BasicCallTests extends TelecomSystemTest { 
    @Test

    public void testTelecomManagerAcceptRingingVideoCallAsAudio() throws Exception {

        IdPair ids = startIncomingPhoneCall("650-555-1212", mPhoneAccountA0.getAccountHandle(),

                VideoProfile.STATE_BIDIRECTIONAL, mConnectionServiceFixtureA);

                VideoProfile.STATE_BIDIRECTIONAL, mConnectionServiceFixtureA, null);



        assertEquals(Call.STATE_RINGING, mInCallServiceFixtureX.getCall(ids.mCallId).getState());

        assertEquals(Call.STATE_RINGING, mInCallServiceFixtureY.getCall(ids.mCallId).getState());
@@ -237,7 +246,7 @@ public class BasicCallTests extends TelecomSystemTest { 
    @Test

    public void testTelecomManagerAcceptRingingInvalidVideoState() throws Exception {

        IdPair ids = startIncomingPhoneCall("650-555-1212", mPhoneAccountA0.getAccountHandle(),

                VideoProfile.STATE_BIDIRECTIONAL, mConnectionServiceFixtureA);

                VideoProfile.STATE_BIDIRECTIONAL, mConnectionServiceFixtureA, null);



        assertEquals(Call.STATE_RINGING, mInCallServiceFixtureX.getCall(ids.mCallId).getState());

        assertEquals(Call.STATE_RINGING, mInCallServiceFixtureY.getCall(ids.mCallId).getState());
@@ -661,13 +670,13 @@ public class BasicCallTests extends TelecomSystemTest { 
    @MediumTest

    @Test

    public void testBasicConferenceCall() throws Exception {

        makeConferenceCall();

        makeConferenceCall(null, null);

    }



    @MediumTest

    @Test

    public void testAddCallToConference1() throws Exception {

        ParcelableCall conferenceCall = makeConferenceCall();

        ParcelableCall conferenceCall = makeConferenceCall(null, null);

        IdPair callId3 = startAndMakeActiveOutgoingCall("650-555-1214",

                mPhoneAccountA0.getAccountHandle(), mConnectionServiceFixtureA);

        // testAddCallToConference{1,2} differ in the order of arguments to InCallAdapter#conference
@@ -685,7 +694,7 @@ public class BasicCallTests extends TelecomSystemTest { 
    @MediumTest

    @Test

    public void testAddCallToConference2() throws Exception {

        ParcelableCall conferenceCall = makeConferenceCall();

        ParcelableCall conferenceCall = makeConferenceCall(null, null);

        IdPair callId3 = startAndMakeActiveOutgoingCall("650-555-1214",

                mPhoneAccountA0.getAccountHandle(), mConnectionServiceFixtureA);

        mInCallServiceFixtureX.getInCallAdapter()
@@ -944,7 +953,7 @@ public class BasicCallTests extends TelecomSystemTest { 
    public void testOutgoingCallSelectPhoneAccountVideo() throws Exception {

        startOutgoingPhoneCallPendingCreateConnection("650-555-1212",

                null, mConnectionServiceFixtureA,

                Process.myUserHandle(), VideoProfile.STATE_BIDIRECTIONAL);

                Process.myUserHandle(), VideoProfile.STATE_BIDIRECTIONAL, null);

        com.android.server.telecom.Call call = mTelecomSystem.getCallsManager().getCalls()

                .iterator().next();

        assert(call.isVideoCallingSupportedByPhoneAccount());
@@ -967,7 +976,7 @@ public class BasicCallTests extends TelecomSystemTest { 
    public void testOutgoingCallSelectPhoneAccountNoVideo() throws Exception {

        startOutgoingPhoneCallPendingCreateConnection("650-555-1212",

                null, mConnectionServiceFixtureA,

                Process.myUserHandle(), VideoProfile.STATE_BIDIRECTIONAL);

                Process.myUserHandle(), VideoProfile.STATE_BIDIRECTIONAL, null);

        com.android.server.telecom.Call call = mTelecomSystem.getCallsManager().getCalls()

                .iterator().next();

        assert(call.isVideoCallingSupportedByPhoneAccount());
@@ -1176,4 +1185,145 @@ public class BasicCallTests extends TelecomSystemTest { 
        assertTrue(muteValues.get(0));

        assertFalse(muteValues.get(1));

    }



    /**

     * Verifies that StatusHints image is validated in ConnectionServiceWrapper#addConferenceCall

     * when the image doesn't belong to the calling user. Simulates a scenario where an app

     * could manipulate the contents of the bundle and send it via the binder to upload an image

     * from another user.

     *

     * @throws Exception

     */

    @SmallTest

    @Test

    public void testValidateStatusHintsImage_addConferenceCall() throws Exception {

        Intent callIntent1 = new Intent();

        // Stub intent for call2

        Intent callIntent2 = new Intent();

        Bundle callExtras1 = new Bundle();

        Icon icon = Icon.createWithContentUri("content://10@media/external/images/media/");

        // Load StatusHints extra into TelecomManager.EXTRA_OUTGOING_CALL_EXTRAS to be processed

        // as the call extras. This will be leveraged in ConnectionServiceFixture to set the

        // StatusHints for the given connection.

        StatusHints statusHints = new StatusHints(icon);

        assertNotNull(statusHints.getIcon());

        callExtras1.putParcelable(STATUS_HINTS_EXTRA, statusHints);

        callIntent1.putExtra(TelecomManager.EXTRA_OUTGOING_CALL_EXTRAS, callExtras1);



        // Start conference call to invoke ConnectionServiceWrapper#addConferenceCall.

        // Note that the calling user would be User 0.

        ParcelableCall conferenceCall = makeConferenceCall(callIntent1, callIntent2);



        // Ensure that StatusHints was set.

        assertNotNull(mInCallServiceFixtureX.getCall(mInCallServiceFixtureX.mLatestCallId)

                .getStatusHints());

        // Ensure that the StatusHints image icon was disregarded.

        assertNull(mInCallServiceFixtureX.getCall(mInCallServiceFixtureX.mLatestCallId)

                .getStatusHints().getIcon());

    }



    /**

     * Verifies that StatusHints image is validated in

     * ConnectionServiceWrapper#handleCreateConnectionComplete when the image doesn't belong to the

     * calling user. Simulates a scenario where an app could manipulate the contents of the

     * bundle and send it via the binder to upload an image from another user.

     *

     * @throws Exception

     */

    @SmallTest

    @Test

    public void testValidateStatusHintsImage_handleCreateConnectionComplete() throws Exception {

        Bundle extras = new Bundle();

        Icon icon = Icon.createWithContentUri("content://10@media/external/images/media/");

        // Load the bundle with the test extra in order to simulate an app directly invoking the

        // binder on ConnectionServiceWrapper#handleCreateConnectionComplete.

        StatusHints statusHints = new StatusHints(icon);

        assertNotNull(statusHints.getIcon());

        extras.putParcelable(STATUS_HINTS_EXTRA, statusHints);



        // Start incoming call with StatusHints extras

        // Note that the calling user in ConnectionServiceWrapper#handleCreateConnectionComplete

        // would be User 0.

        IdPair ids = startIncomingPhoneCallWithExtras("650-555-1212",

                mPhoneAccountA0.getAccountHandle(), mConnectionServiceFixtureA, extras);



        // Ensure that StatusHints was set.

        assertNotNull(mInCallServiceFixtureX.getCall(ids.mCallId).getStatusHints());

        // Ensure that the StatusHints image icon was disregarded.

        assertNull(mInCallServiceFixtureX.getCall(ids.mCallId).getStatusHints().getIcon());

    }



    /**

     * Verifies that StatusHints image is validated in ConnectionServiceWrapper#setStatusHints

     * when the image doesn't belong to the calling user. Simulates a scenario where an app

     * could manipulate the contents of the bundle and send it via the binder to upload an image

     * from another user.

     *

     * @throws Exception

     */

    @SmallTest

    @Test

    public void testValidateStatusHintsImage_setStatusHints() throws Exception {

        IdPair outgoing = startAndMakeActiveOutgoingCall("650-555-1214",

                mPhoneAccountA0.getAccountHandle(), mConnectionServiceFixtureA);



        // Modify existing connection with StatusHints image exploit

        Icon icon = Icon.createWithContentUri("content://10@media/external/images/media/");

        StatusHints statusHints = new StatusHints(icon);

        assertNotNull(statusHints.getIcon());

        ConnectionServiceFixture.ConnectionInfo connectionInfo = mConnectionServiceFixtureA

                .mConnectionById.get(outgoing.mConnectionId);

        connectionInfo.statusHints = statusHints;



        // Invoke ConnectionServiceWrapper#setStatusHints.

        // Note that the calling user would be User 0.

        mConnectionServiceFixtureA.sendSetStatusHints(outgoing.mConnectionId);

        waitForHandlerAction(mConnectionServiceFixtureA.mConnectionServiceDelegate.getHandler(),

                TEST_TIMEOUT);



        // Ensure that StatusHints was set.

        assertNotNull(mInCallServiceFixtureX.getCall(outgoing.mCallId).getStatusHints());

        // Ensure that the StatusHints image icon was disregarded.

        assertNull(mInCallServiceFixtureX.getCall(outgoing.mCallId)

                .getStatusHints().getIcon());

    }



    /**

     * Verifies that StatusHints image is validated in

     * ConnectionServiceWrapper#addExistingConnection when the image doesn't belong to the calling

     * user. Simulates a scenario where an app could manipulate the contents of the bundle and

     * send it via the binder to upload an image from another user.

     *

     * @throws Exception

     */

    @SmallTest

    @Test

    public void testValidateStatusHintsImage_addExistingConnection() throws Exception {

        IdPair outgoing = startAndMakeActiveOutgoingCall("650-555-1214",

                mPhoneAccountA0.getAccountHandle(), mConnectionServiceFixtureA);

        Connection existingConnection = mConnectionServiceFixtureA.mLatestConnection;



        // Modify existing connection with StatusHints image exploit

        Icon icon = Icon.createWithContentUri("content://10@media/external/images/media/");

        StatusHints modifiedStatusHints = new StatusHints(icon);

        assertNotNull(modifiedStatusHints.getIcon());

        ConnectionServiceFixture.ConnectionInfo connectionInfo = mConnectionServiceFixtureA

                .mConnectionById.get(outgoing.mConnectionId);

        connectionInfo.statusHints = modifiedStatusHints;



        // Invoke ConnectionServiceWrapper#addExistingConnection.

        // Note that the calling user would be User 0.

        mConnectionServiceFixtureA.sendAddExistingConnection(outgoing.mConnectionId);

        waitForHandlerAction(mConnectionServiceFixtureA.mConnectionServiceDelegate.getHandler(),

                TEST_TIMEOUT);



        // Ensure that StatusHints was set. Due to test setup, the ParcelableConnection object that

        // is passed into sendAddExistingConnection is instantiated on invocation. The call's

        // StatusHints are not updated at the time of completion, so instead, we can verify that

        // the ParcelableConnection object was modified.

        assertNotNull(mConnectionServiceFixtureA.mLatestParcelableConnection.getStatusHints());

        // Ensure that the StatusHints image icon was disregarded.

        assertNull(mConnectionServiceFixtureA.mLatestParcelableConnection

                .getStatusHints().getIcon());

    }

}

tests/src/com/android/server/telecom/tests/CallExtrasTest.java

+3 −3
Original line number Diff line number Diff line
@@ -370,7 +370,7 @@ public class CallExtrasTest extends TelecomSystemTest { 
    @LargeTest

    @Test

    public void testConferenceSetExtras() throws Exception {

        ParcelableCall call = makeConferenceCall();

        ParcelableCall call = makeConferenceCall(null, null);

        String conferenceId = call.getId();



        Conference conference = mConnectionServiceFixtureA.mLatestConference;
@@ -414,7 +414,7 @@ public class CallExtrasTest extends TelecomSystemTest { 
    @FlakyTest(bugId = 117751305)

    @Test

    public void testConferenceExtraOperations() throws Exception {

        ParcelableCall call = makeConferenceCall();

        ParcelableCall call = makeConferenceCall(null, null);

        String conferenceId = call.getId();

        Conference conference = mConnectionServiceFixtureA.mLatestConference;

        assertNotNull(conference);
@@ -450,7 +450,7 @@ public class CallExtrasTest extends TelecomSystemTest { 
    @LargeTest

    @Test

    public void testConferenceICS() throws Exception {

        ParcelableCall call = makeConferenceCall();

        ParcelableCall call = makeConferenceCall(null, null);

        String conferenceId = call.getId();

        Conference conference = mConnectionServiceFixtureA.mLatestConference;

tests/src/com/android/server/telecom/tests/ConnectionServiceFixture.java

+20 −1
Original line number Diff line number Diff line
@@ -67,6 +67,7 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 
    static int INVALID_VIDEO_STATE = -1;

    public CountDownLatch mExtrasLock = new CountDownLatch(1);

    static int NOT_SPECIFIED = 0;

    public static final String STATUS_HINTS_EXTRA = "updateStatusHints";



    /**

     * Implementation of ConnectionService that performs no-ops for tasks normally meant for
@@ -101,6 +102,11 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 
            if (mProperties != NOT_SPECIFIED) {

                fakeConnection.setConnectionProperties(mProperties);

            }

            // Testing for StatusHints image icon cross user access

            if (request.getExtras() != null) {

                fakeConnection.setStatusHints(

                        request.getExtras().getParcelable(STATUS_HINTS_EXTRA));

            }



            return fakeConnection;

        }
@@ -117,6 +123,11 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 
            if (mProperties != NOT_SPECIFIED) {

                fakeConnection.setConnectionProperties(mProperties);

            }

            // Testing for StatusHints image icon cross user access

            if (request.getExtras() != null) {

                fakeConnection.setStatusHints(

                        request.getExtras().getParcelable(STATUS_HINTS_EXTRA));

            }

            return fakeConnection;

        }
@@ -133,6 +144,12 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 
                Conference fakeConference = new FakeConference();

                fakeConference.addConnection(cxn1);

                fakeConference.addConnection(cxn2);

                if (cxn1.getStatusHints() != null || cxn2.getStatusHints() != null) {

                    // For testing purposes, pick one of the status hints that isn't null.

                    StatusHints statusHints = cxn1.getStatusHints() != null

                            ? cxn1.getStatusHints() : cxn2.getStatusHints();

                    fakeConference.setStatusHints(statusHints);

                }

                mLatestConference = fakeConference;

                addConference(fakeConference);

            } else {
@@ -480,6 +497,7 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 


    public String mLatestConnectionId;

    public Connection mLatestConnection;

    public ParcelableConnection mLatestParcelableConnection;

    public Conference mLatestConference;

    public final Set<IConnectionServiceAdapter> mConnectionServiceAdapters = new HashSet<>();

    public final Map<String, ConnectionInfo> mConnectionById = new HashMap<>();
@@ -724,7 +742,7 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 
    }



    private ParcelableConnection parcelable(ConnectionInfo c) {

        return new ParcelableConnection(

        mLatestParcelableConnection = new ParcelableConnection(

                c.request.getAccountHandle(),

                c.state,

                c.capabilities,
@@ -745,5 +763,6 @@ public class ConnectionServiceFixture implements TestFixture<IConnectionService> 
                c.conferenceableConnectionIds,

                c.extras,

                c.callerNumberVerificationStatus);

        return mLatestParcelableConnection;

    }

}

tests/src/com/android/server/telecom/tests/TelecomSystemTest.java

+46 −20

File changed.

Preview size limit exceeded, changes collapsed.