From 66b5664f2d4a3ef0a1dfa60f1a9d2bf4ff61857a Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 2 Apr 2019 14:18:56 -0700 Subject: [PATCH] Careful with screenshots containing secure layers! For purposes of the screen rotation animation the system server is allowed to capture secure (not protected) layers and trusted not to persist screenshots which may contain secure layers. However when displaying the screen rotation animation, the layer the screenshot is placed on will itself not be secure, so if we record the animation the recording will contain persisted versions of the secure content. Make sure we use the new API from SurfaceFlinger to set FLAG_SECURE if our screenshot contains secure content. Bug: 69703445 Test: Transaction_test#SetFlagsSecureEuidSystem Change-Id: I1b016fb1ad56eccd712442a71cc134e5fa3b1ac6 --- .../display/DisplayManagerInternal.java | 8 ++++---- core/java/android/view/SurfaceControl.java | 16 +++++++++++++--- core/jni/android_view_SurfaceControl.cpp | 8 +++++--- .../server/display/DisplayManagerService.java | 17 +++++------------ .../server/wm/ScreenRotationAnimation.java | 15 ++++++++++++++- 5 files changed, 41 insertions(+), 23 deletions(-) diff --git a/core/java/android/hardware/display/DisplayManagerInternal.java b/core/java/android/hardware/display/DisplayManagerInternal.java index 8231985970aae..c9551378c1908 100644 --- a/core/java/android/hardware/display/DisplayManagerInternal.java +++ b/core/java/android/hardware/display/DisplayManagerInternal.java @@ -25,6 +25,7 @@ import android.util.SparseArray; import android.view.Display; import android.view.DisplayInfo; import android.view.Surface; +import android.view.SurfaceControl; import android.view.SurfaceControl.Transaction; /** @@ -64,13 +65,12 @@ public abstract class DisplayManagerInternal { public abstract boolean isProximitySensorAvailable(); /** - * Take a screenshot of the specified display into the provided {@link Surface}. + * Take a screenshot of the specified display and return a buffer. * * @param displayId The display id to take the screenshot of. - * @param outSurface The {@link Surface} to take the screenshot into. - * @return True if the screenshot is taken. + * @return The buffer or null if we have failed. */ - public abstract boolean screenshot(int displayId, Surface outSurface); + public abstract SurfaceControl.ScreenshotGraphicBuffer screenshot(int displayId); /** * Returns information about the specified logical display. diff --git a/core/java/android/view/SurfaceControl.java b/core/java/android/view/SurfaceControl.java index ec62e190d0b4f..79363edb0955c 100644 --- a/core/java/android/view/SurfaceControl.java +++ b/core/java/android/view/SurfaceControl.java @@ -439,10 +439,13 @@ public final class SurfaceControl implements Parcelable { public static class ScreenshotGraphicBuffer { private final GraphicBuffer mGraphicBuffer; private final ColorSpace mColorSpace; + private final boolean mContainsSecureLayers; - public ScreenshotGraphicBuffer(GraphicBuffer graphicBuffer, ColorSpace colorSpace) { + public ScreenshotGraphicBuffer(GraphicBuffer graphicBuffer, ColorSpace colorSpace, + boolean containsSecureLayers) { mGraphicBuffer = graphicBuffer; mColorSpace = colorSpace; + mContainsSecureLayers = containsSecureLayers; } /** @@ -453,13 +456,16 @@ public final class SurfaceControl implements Parcelable { * @param usage Hint indicating how the buffer will be used * @param unwrappedNativeObject The native object of GraphicBuffer * @param namedColorSpace Integer value of a named color space {@link ColorSpace.Named} + * @param containsSecureLayer Indicates whether this graphic buffer contains captured contents + * of secure layers, in which case the screenshot should not be persisted. */ private static ScreenshotGraphicBuffer createFromNative(int width, int height, int format, - int usage, long unwrappedNativeObject, int namedColorSpace) { + int usage, long unwrappedNativeObject, int namedColorSpace, + boolean containsSecureLayers) { GraphicBuffer graphicBuffer = GraphicBuffer.createFromExisting(width, height, format, usage, unwrappedNativeObject); ColorSpace colorSpace = ColorSpace.get(ColorSpace.Named.values()[namedColorSpace]); - return new ScreenshotGraphicBuffer(graphicBuffer, colorSpace); + return new ScreenshotGraphicBuffer(graphicBuffer, colorSpace, containsSecureLayers); } public ColorSpace getColorSpace() { @@ -469,6 +475,10 @@ public final class SurfaceControl implements Parcelable { public GraphicBuffer getGraphicBuffer() { return mGraphicBuffer; } + + public boolean containsSecureLayers() { + return mContainsSecureLayers; + } } /** diff --git a/core/jni/android_view_SurfaceControl.cpp b/core/jni/android_view_SurfaceControl.cpp index c254266961cf3..13bcbf6625e2e 100644 --- a/core/jni/android_view_SurfaceControl.cpp +++ b/core/jni/android_view_SurfaceControl.cpp @@ -250,10 +250,11 @@ static jobject nativeScreenshot(JNIEnv* env, jclass clazz, Rect sourceCrop = rectFromObj(env, sourceCropObj); sp buffer; + bool capturedSecureLayers = false; status_t res = ScreenshotClient::capture(displayToken, dataspace, ui::PixelFormat::RGBA_8888, sourceCrop, width, height, - useIdentityTransform, rotation, captureSecureLayers, &buffer); + useIdentityTransform, rotation, captureSecureLayers, &buffer, capturedSecureLayers); if (res != NO_ERROR) { return NULL; } @@ -266,7 +267,8 @@ static jobject nativeScreenshot(JNIEnv* env, jclass clazz, buffer->getPixelFormat(), (jint)buffer->getUsage(), (jlong)buffer.get(), - namedColorSpace); + namedColorSpace, + capturedSecureLayers); } static jobject nativeCaptureLayers(JNIEnv* env, jclass clazz, jobject layerHandleToken, @@ -1455,7 +1457,7 @@ int register_android_view_SurfaceControl(JNIEnv* env) MakeGlobalRefOrDie(env, screenshotGraphicsBufferClazz); gScreenshotGraphicBufferClassInfo.builder = GetStaticMethodIDOrDie(env, screenshotGraphicsBufferClazz, - "createFromNative", "(IIIIJI)Landroid/view/SurfaceControl$ScreenshotGraphicBuffer;"); + "createFromNative", "(IIIIJIZ)Landroid/view/SurfaceControl$ScreenshotGraphicBuffer;"); jclass displayedContentSampleClazz = FindClassOrDie(env, "android/hardware/display/DisplayedContentSample"); diff --git a/services/core/java/com/android/server/display/DisplayManagerService.java b/services/core/java/com/android/server/display/DisplayManagerService.java index 3010324488b85..3abd0ba29871c 100644 --- a/services/core/java/com/android/server/display/DisplayManagerService.java +++ b/services/core/java/com/android/server/display/DisplayManagerService.java @@ -1269,21 +1269,14 @@ public final class DisplayManagerService extends SystemService { return null; } - private boolean screenshotInternal(int displayId, Surface outSurface) { + private SurfaceControl.ScreenshotGraphicBuffer screenshotInternal(int displayId) { final IBinder token = getDisplayToken(displayId); if (token == null) { - return false; + return null; } - final SurfaceControl.ScreenshotGraphicBuffer gb = - SurfaceControl.screenshotToBufferWithSecureLayersUnsafe( + return SurfaceControl.screenshotToBufferWithSecureLayersUnsafe( token, new Rect(), 0 /* width */, 0 /* height */, false /* useIdentityTransform */, 0 /* rotation */); - try { - outSurface.attachAndQueueBuffer(gb.getGraphicBuffer()); - } catch (RuntimeException e) { - Slog.w(TAG, "Failed to take screenshot - " + e.getMessage()); - } - return true; } @VisibleForTesting @@ -2354,8 +2347,8 @@ public final class DisplayManagerService extends SystemService { } @Override - public boolean screenshot(int displayId, Surface outSurface) { - return screenshotInternal(displayId, outSurface); + public SurfaceControl.ScreenshotGraphicBuffer screenshot(int displayId) { + return screenshotInternal(displayId); } @Override diff --git a/services/core/java/com/android/server/wm/ScreenRotationAnimation.java b/services/core/java/com/android/server/wm/ScreenRotationAnimation.java index 2d5c97f9a3a58..b90d60227be43 100644 --- a/services/core/java/com/android/server/wm/ScreenRotationAnimation.java +++ b/services/core/java/com/android/server/wm/ScreenRotationAnimation.java @@ -275,7 +275,20 @@ class ScreenRotationAnimation { final int displayId = display.getDisplayId(); final Surface surface = mService.mSurfaceFactory.make(); surface.copyFrom(mSurfaceControl); - if (mService.mDisplayManagerInternal.screenshot(displayId, surface)) { + SurfaceControl.ScreenshotGraphicBuffer gb = + mService.mDisplayManagerInternal.screenshot(displayId); + if (gb != null) { + try { + surface.attachAndQueueBuffer(gb.getGraphicBuffer()); + } catch (RuntimeException e) { + Slog.w(TAG, "Failed to attach screenshot - " + e.getMessage()); + } + // If the screenshot contains secure layers, we have to make sure the + // screenshot surface we display it in also has FLAG_SECURE so that + // the user can not screenshot secure layers via the screenshot surface. + if (gb.containsSecureLayers()) { + t.setSecure(mSurfaceControl, true); + } t.setLayer(mSurfaceControl, SCREEN_FREEZE_LAYER_SCREENSHOT); t.setAlpha(mSurfaceControl, 0); t.show(mSurfaceControl);