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

Commit f5fb5b04 authored by Chavi Weingarten's avatar Chavi Weingarten
Browse files

Immediately fail screenshots if requesting secure without permission. 

The screenshot logic will only fail to capture when requesting secure if
there is any secure layers on screen. This is confusing since the caller
shouldn't be allowed to apss in captureSecureLayers for screenshots
without permission regardless if there are any secure layers on screen.

This makes the screenshot permission model simpler
1. If there are secure layers on screen, it will be blacked out
2. If the caller has CAPTURE_BLACKOUT_CONTENT and requests
   captureSecureLayers, they will get a screenshot that contains the
   secure layers
3. If the caller doesn't have CAPTURE_BLACKOUT_CONTENT and requests
   captureSecureLayers, they will get a permission denied callback.

Test: ScreenCaptureTest
Bug: 313697941
Change-Id: Iba64989d74ebc2076218f643a8940806e59b6b97
parent 9a296f97

services/surfaceflinger/SurfaceFlinger.cpp

+14 −24
Original line number Diff line number Diff line
@@ -7539,6 +7539,12 @@ void SurfaceFlinger::captureDisplay(const DisplayCaptureArgs& args, 
        return;

    }



    if (args.captureSecureLayers && !hasCaptureBlackoutContentPermission()) {

        ALOGE("Attempting to capture secure layers without CAPTURE_BLACKOUT_CONTENT");

        invokeScreenCaptureError(PERMISSION_DENIED, captureListener);

        return;

    }



    wp<const DisplayDevice> displayWeak;

    ui::LayerStack layerStack;

    ui::Size reqSize(args.width, args.height);
@@ -7669,8 +7675,11 @@ void SurfaceFlinger::captureLayers(const LayerCaptureArgs& args, 
    std::unordered_set<uint32_t> excludeLayerIds;

    ui::Dataspace dataspace = args.dataspace;



    // Call this before holding mStateLock to avoid any deadlocking.

    bool canCaptureBlackoutContent = hasCaptureBlackoutContentPermission();

    if (args.captureSecureLayers && !hasCaptureBlackoutContentPermission()) {

        ALOGE("Attempting to capture secure layers without CAPTURE_BLACKOUT_CONTENT");

        invokeScreenCaptureError(PERMISSION_DENIED, captureListener);

        return;

    }



    {

        Mutex::Autolock lock(mStateLock);
@@ -7682,13 +7691,6 @@ void SurfaceFlinger::captureLayers(const LayerCaptureArgs& args, 
            return;

        }



        if (!canCaptureBlackoutContent &&

            parent->getDrawingState().flags & layer_state_t::eLayerSecure) {

            ALOGW("Attempting to capture secure layer: PERMISSION_DENIED");

            invokeScreenCaptureError(PERMISSION_DENIED, captureListener);

            return;

        }



        Rect parentSourceBounds = parent->getCroppedBufferSize(parent->getDrawingState());

        if (args.sourceCrop.width() <= 0) {

            crop.left = 0;
@@ -7866,8 +7868,6 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenCommon( 
        bool grayscale, const sp<IScreenCaptureListener>& captureListener) {

    ATRACE_CALL();



    bool canCaptureBlackoutContent = hasCaptureBlackoutContentPermission();



    auto future = mScheduler->schedule(

            [=, renderAreaFuture = std::move(renderAreaFuture)]() FTL_FAKE_GUARD(

                    kMainThreadContext) mutable -> ftl::SharedFuture<FenceResult> {
@@ -7885,8 +7885,7 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenCommon( 
                ftl::SharedFuture<FenceResult> renderFuture;

                renderArea->render([&]() FTL_FAKE_GUARD(kMainThreadContext) {

                    renderFuture = renderScreenImpl(renderArea, getLayerSnapshots, buffer,

                                                    canCaptureBlackoutContent, regionSampling,

                                                    grayscale, captureResults);

                                                    regionSampling, grayscale, captureResults);

                });



                if (captureListener) {
@@ -7913,9 +7912,8 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::captureScreenCommon( 


ftl::SharedFuture<FenceResult> SurfaceFlinger::renderScreenImpl(

        std::shared_ptr<const RenderArea> renderArea, GetLayerSnapshotsFunction getLayerSnapshots,

        const std::shared_ptr<renderengine::ExternalTexture>& buffer,

        bool canCaptureBlackoutContent, bool regionSampling, bool grayscale,

        ScreenCaptureResults& captureResults) {

        const std::shared_ptr<renderengine::ExternalTexture>& buffer, bool regionSampling,

        bool grayscale, ScreenCaptureResults& captureResults) {

    ATRACE_CALL();



    auto layers = getLayerSnapshots();
@@ -7930,14 +7928,6 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::renderScreenImpl( 
                layerFE->mSnapshot->geomLayerTransform.inverse();

    }



    // We allow the system server to take screenshots of secure layers for

    // use in situations like the Screen-rotation animation and place

    // the impetus on WindowManager to not persist them.

    if (captureResults.capturedSecureLayers && !canCaptureBlackoutContent) {

        ALOGW("FB is protected: PERMISSION_DENIED");

        return ftl::yield<FenceResult>(base::unexpected(PERMISSION_DENIED)).share();

    }



    auto capturedBuffer = buffer;



    auto requestedDataspace = renderArea->getReqDataSpace();

services/surfaceflinger/SurfaceFlinger.h

+2 −2
Original line number Diff line number Diff line
@@ -858,8 +858,8 @@ private: 
            bool grayscale, const sp<IScreenCaptureListener>&);

    ftl::SharedFuture<FenceResult> renderScreenImpl(

            std::shared_ptr<const RenderArea>, GetLayerSnapshotsFunction,

            const std::shared_ptr<renderengine::ExternalTexture>&, bool canCaptureBlackoutContent,

            bool regionSampling, bool grayscale, ScreenCaptureResults&) EXCLUDES(mStateLock)

            const std::shared_ptr<renderengine::ExternalTexture>&, bool regionSampling,

            bool grayscale, ScreenCaptureResults&) EXCLUDES(mStateLock)

            REQUIRES(kMainThreadContext);



    // If the uid provided is not UNSET_UID, the traverse will skip any layers that don't have a

services/surfaceflinger/tests/ScreenCapture_test.cpp

+31 −14
Original line number Diff line number Diff line
@@ -99,6 +99,7 @@ TEST_F(ScreenCaptureTest, SetFlagsSecureEUidSystem) { 
        ASSERT_EQ(PERMISSION_DENIED, ScreenCapture::captureLayers(mCaptureArgs, mCaptureResults));

    }



    {

        UIDFaker f(AID_SYSTEM);



        // By default the system can capture screenshots with secure layers but they
@@ -111,15 +112,31 @@ TEST_F(ScreenCaptureTest, SetFlagsSecureEUidSystem) { 
            shot->expectColor(Rect(0, 0, 32, 32), Color::BLACK);

        }



    // Here we pass captureSecureLayers = true and since we are AID_SYSTEM we should be able

    // to receive them...we are expected to take care with the results.

        mCaptureArgs.captureSecureLayers = true;

        // AID_SYSTEM is allowed to capture secure content.

        ASSERT_EQ(NO_ERROR, ScreenCapture::captureLayers(mCaptureArgs, mCaptureResults));

        ASSERT_TRUE(mCaptureResults.capturedSecureLayers);

        ScreenCapture sc(mCaptureResults.buffer, mCaptureResults.capturedHdrLayers);

        sc.expectColor(Rect(0, 0, 32, 32), Color::RED);

    }



    {

        // Attempt secure screenshot from shell since it doesn't have CAPTURE_BLACKOUT_CONTENT

        // permission, but is allowed normal screenshots.

        UIDFaker faker(AID_SHELL);

        ASSERT_EQ(PERMISSION_DENIED, ScreenCapture::captureLayers(mCaptureArgs, mCaptureResults));

    }



    // Remove flag secure from the layer.

    Transaction().setFlags(layer, 0, layer_state_t::eLayerSecure).apply(true);

    {

        // Assert that screenshot fails without CAPTURE_BLACKOUT_CONTENT when requesting

        // captureSecureLayers even if there are no actual secure layers on screen.

        UIDFaker faker(AID_SHELL);

        ASSERT_EQ(PERMISSION_DENIED, ScreenCapture::captureLayers(mCaptureArgs, mCaptureResults));

    }

}



TEST_F(ScreenCaptureTest, CaptureChildSetParentFlagsSecureEUidSystem) {

    sp<SurfaceControl> parentLayer;

    ASSERT_NO_FATAL_FAILURE(

services/surfaceflinger/tests/unittests/CompositionTest.cpp

+1 −2
Original line number Diff line number Diff line
@@ -194,7 +194,6 @@ void CompositionTest::captureScreenComposition() { 
    LayerCase::setupForScreenCapture(this);



    const Rect sourceCrop(0, 0, DEFAULT_DISPLAY_WIDTH, DEFAULT_DISPLAY_HEIGHT);

    constexpr bool forSystem = true;

    constexpr bool regionSampling = false;



    auto renderArea = DisplayRenderArea::create(mDisplay, sourceCrop, sourceCrop.getSize(),
@@ -216,7 +215,7 @@ void CompositionTest::captureScreenComposition() { 
                                                                      usage);



    auto future = mFlinger.renderScreenImpl(std::move(renderArea), getLayerSnapshots,

                                            mCaptureScreenBuffer, forSystem, regionSampling);

                                            mCaptureScreenBuffer, regionSampling);

    ASSERT_TRUE(future.valid());

    const auto fenceResult = future.get();

services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h

+2 −2
Original line number Diff line number Diff line
@@ -493,11 +493,11 @@ public: 
    auto renderScreenImpl(std::shared_ptr<const RenderArea> renderArea,

                          SurfaceFlinger::GetLayerSnapshotsFunction traverseLayers,

                          const std::shared_ptr<renderengine::ExternalTexture>& buffer,

                          bool forSystem, bool regionSampling) {

                          bool regionSampling) {

        ScreenCaptureResults captureResults;

        return FTL_FAKE_GUARD(kMainThreadContext,

                              mFlinger->renderScreenImpl(std::move(renderArea), traverseLayers,

                                                         buffer, forSystem, regionSampling,

                                                         buffer, regionSampling,

                                                         false /* grayscale */, captureResults));

    }