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

Commit d4a61643 authored by chaviw's avatar chaviw
Browse files

Only allow system and graphics to create secure displays 

Previously, we allowed any process that had the permission
ACCESS_SURFACE_FLINGER to create a display, either secure or
not secure. The shell process needs this permission to create
a display for screen recording. However, we just shouldn't allow
any process to create a secure display since that would allow
them to render secure content. Instead, only allow system
and graphics to create secure displays.

Fixes: 154721930
Test: Modified screenrecord to create secure display, which fails
Test: SurfaceFlinger_test
Test: SurfaceInterceptorTest
Test: DisplayTransactionTest
Change-Id: Ib3c5b6c8abd41f3f6fc6a71273cb2a17bfdba959
parent 934e82a9

libs/gui/ISurfaceComposer.cpp

+25 −8
Original line number Diff line number Diff line
@@ -244,10 +244,25 @@ public: 
    {

        Parcel data, reply;

        data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor());

        data.writeString8(displayName);

        data.writeInt32(secure ? 1 : 0);

        remote()->transact(BnSurfaceComposer::CREATE_DISPLAY, data, &reply);

        return reply.readStrongBinder();

        status_t status = data.writeString8(displayName);

        if (status) {

            return nullptr;

        }

        status = data.writeBool(secure);

        if (status) {

            return nullptr;

        }



        status = remote()->transact(BnSurfaceComposer::CREATE_DISPLAY, data, &reply);

        if (status) {

            return nullptr;

        }

        sp<IBinder> display;

        status = reply.readNullableStrongBinder(&display);

        if (status) {

            return nullptr;

        }

        return display;

    }



    virtual void destroyDisplay(const sp<IBinder>& display)
@@ -1295,10 +1310,12 @@ status_t BnSurfaceComposer::onTransact( 
        }

        case CREATE_DISPLAY: {

            CHECK_INTERFACE(ISurfaceComposer, data, reply);

            String8 displayName = data.readString8();

            bool secure = bool(data.readInt32());

            sp<IBinder> display(createDisplay(displayName, secure));

            reply->writeStrongBinder(display);

            String8 displayName;

            SAFE_PARCEL(data.readString8, &displayName);

            bool secure = false;

            SAFE_PARCEL(data.readBool, &secure);

            sp<IBinder> display = createDisplay(displayName, secure);

            SAFE_PARCEL(reply->writeStrongBinder, display);

            return NO_ERROR;

        }

        case DESTROY_DISPLAY: {

services/surfaceflinger/SurfaceFlinger.cpp

+9 −0
Original line number Diff line number Diff line
@@ -509,6 +509,15 @@ sp<ISurfaceComposerClient> SurfaceFlinger::createConnection() { 
}



sp<IBinder> SurfaceFlinger::createDisplay(const String8& displayName, bool secure) {

    // onTransact already checks for some permissions, but adding an additional check here.

    // This is to ensure that only system and graphics can request to create a secure

    // display. Secure displays can show secure content so we add an additional restriction on it.

    const int uid = IPCThreadState::self()->getCallingUid();

    if (secure && uid != AID_GRAPHICS && uid != AID_SYSTEM) {

        ALOGE("Only privileged processes can create a secure display");

        return nullptr;

    }



    class DisplayToken : public BBinder {

        sp<SurfaceFlinger> flinger;

        virtual ~DisplayToken() {

services/surfaceflinger/tests/Credentials_test.cpp

+25 −21
Original line number Diff line number Diff line
@@ -98,26 +98,6 @@ protected: 
                  t.setLayer(mBGSurfaceControl, INT_MAX - 3).show(mBGSurfaceControl).apply());

    }



    void setupVirtualDisplay() {

        mVirtualDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);

        const ssize_t displayWidth = 100;

        const ssize_t displayHeight = 100;



        // Background surface

        mVirtualSurfaceControl =

                mComposerClient->createSurface(SURFACE_NAME, displayWidth, displayHeight,

                                               PIXEL_FORMAT_RGBA_8888, 0);

        ASSERT_TRUE(mVirtualSurfaceControl != nullptr);

        ASSERT_TRUE(mVirtualSurfaceControl->isValid());



        Transaction t;

        t.setDisplayLayerStack(mVirtualDisplay, 0);

        ASSERT_EQ(NO_ERROR,

                  t.setLayer(mVirtualSurfaceControl, INT_MAX - 3)

                          .show(mVirtualSurfaceControl)

                          .apply());

    }



    /**

     * Sets UID to imitate Graphic's process.

     */
@@ -165,6 +145,10 @@ protected: 
        // Check as a non-supported user.

        setBinUID();

        ASSERT_EQ(unprivilegedValue, condition());



        // Check as shell since shell has some additional permissions

        seteuid(AID_SHELL);

        ASSERT_EQ(unprivilegedValue, condition());

    }

};
@@ -262,11 +246,31 @@ TEST_F(CredentialsTest, SetActiveColorModeTest) { 
}



TEST_F(CredentialsTest, CreateDisplayTest) {

    // Only graphics and system processes can create a secure display.

    std::function<bool()> condition = [=]() {

        sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);

        return testDisplay.get() != nullptr;

    };

    ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false));



    // Check with root.

    seteuid(AID_ROOT);

    ASSERT_FALSE(condition());



    // Check as a Graphics user.

    setGraphicsUID();

    ASSERT_TRUE(condition());



    // Check as a system user.

    setSystemUID();

    ASSERT_TRUE(condition());



    // Check as a non-supported user.

    setBinUID();

    ASSERT_FALSE(condition());



    // Check as shell since shell has some additional permissions

    seteuid(AID_SHELL);

    ASSERT_FALSE(condition());



    condition = [=]() {

        sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);

services/surfaceflinger/tests/SurfaceInterceptor_test.cpp

+2 −2
Original line number Diff line number Diff line
@@ -422,7 +422,7 @@ void SurfaceInterceptorTest::shadowRadiusUpdate(Transaction& t) { 
}



void SurfaceInterceptorTest::displayCreation(Transaction&) {

    sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);

    sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);

    SurfaceComposerClient::destroyDisplay(testDisplay);

}
@@ -819,7 +819,7 @@ bool SurfaceInterceptorTest::surfaceDeletionFound(const Increment& increment, 


bool SurfaceInterceptorTest::displayCreationFound(const Increment& increment, bool foundDisplay) {

    bool isMatch(increment.display_creation().name() == DISPLAY_NAME.string() &&

            increment.display_creation().is_secure());

                 !increment.display_creation().is_secure());

    if (isMatch && !foundDisplay) {

        foundDisplay = true;

    } else if (isMatch && foundDisplay) {

services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp

+7 −1
Original line number Diff line number Diff line
@@ -24,6 +24,7 @@ 
#include <type_traits>



#include <android/hardware/power/Boost.h>

#include <binder/IPCThreadState.h>

#include <compositionengine/Display.h>

#include <compositionengine/DisplayColorProfile.h>

#include <compositionengine/impl/Display.h>
@@ -37,6 +38,7 @@ 
#include <gui/mock/GraphicBufferConsumer.h>

#include <gui/mock/GraphicBufferProducer.h>

#include <log/log.h>

#include <private/android_filesystem_config.h>

#include <renderengine/mock/RenderEngine.h>

#include <ui/DebugUtils.h>
@@ -1230,8 +1232,12 @@ TEST_F(DisplayTransactionTest, createDisplaySetsCurrentStateForSecureDisplay) { 


    // --------------------------------------------------------------------

    // Invocation



    int64_t oldId = IPCThreadState::self()->clearCallingIdentity();

    // Set the calling identity to graphics so captureDisplay with secure is allowed.

    IPCThreadState::self()->restoreCallingIdentity(static_cast<int64_t>(AID_GRAPHICS) << 32 |

                                                   AID_GRAPHICS);

    sp<IBinder> displayToken = mFlinger.createDisplay(name, true);

    IPCThreadState::self()->restoreCallingIdentity(oldId);



    // --------------------------------------------------------------------

    // Postconditions