From 11d913944f53dd189c027068aaea5fc87316e962 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Fri, 15 Feb 2019 15:48:11 -0800 Subject: [PATCH 1/3] [RESTRICT AUTOMERGE]: Exclude secure layers from most screenshots taken by the system server. In pre-P versions of Android, it was allowed to screenshot secure layers if the buffer queue producer which was the target of the screenshot was owned by the system (in this case SurfaceFlinger). This really was a synonym for: The screen rotation animation was allowed to capture secure layers, but the other code paths weren't. In O we mistakenly changed this check to always allow the system server to capture secure layers via the captureScreen path (the captureLayers path used for TaskSnapshots was unaffected). This can result in data leakage in cases where the system server takes screenshots on behalf of other parts of the system (e.g. for the assistant). To mitigate this we provide an explicit switch for the system server to specify whether it wishes to capture Secure layers. While this is dangerous, I think it is less dangerous than the previous implicit switch of capturing secure layers based on which type of BufferQueue was passed in. The flag defaults to not capturing secure layers and we set it to true in the one place we need it (for the screen rotation animation). Non privileged clients can still not capture secure layers at all directly. Test: TransactionTest.cpp#SetFlagsSecureEUidSystem Bug: 120610669 Change-Id: I9d32c5ac2b005059be9f464859a415167d9ddbd4 (cherry picked from commit dc49e0088a05108a0616704ca5565136f89c0a1f) --- core/java/android/view/SurfaceControl.java | 26 +++++++++++++++++-- core/jni/android_view_SurfaceControl.cpp | 6 ++--- .../server/wm/ScreenRotationAnimation.java | 20 +++++++++++--- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/core/java/android/view/SurfaceControl.java b/core/java/android/view/SurfaceControl.java index ed8b0053e5bfe..eec34cdc12cc8 100644 --- a/core/java/android/view/SurfaceControl.java +++ b/core/java/android/view/SurfaceControl.java @@ -74,7 +74,8 @@ public class SurfaceControl implements Parcelable { boolean allLayers, boolean useIdentityTransform, int rotation); private static native GraphicBuffer nativeScreenshotToBuffer(IBinder displayToken, Rect sourceCrop, int width, int height, int minLayer, int maxLayer, - boolean allLayers, boolean useIdentityTransform, int rotation); + boolean allLayers, boolean useIdentityTransform, int rotation, + boolean captureSecureLayers); private static native void nativeScreenshot(IBinder displayToken, Surface consumer, Rect sourceCrop, int width, int height, int minLayer, int maxLayer, boolean allLayers, boolean useIdentityTransform); @@ -1249,7 +1250,28 @@ public class SurfaceControl implements Parcelable { IBinder displayToken = SurfaceControl.getBuiltInDisplay( SurfaceControl.BUILT_IN_DISPLAY_ID_MAIN); return nativeScreenshotToBuffer(displayToken, sourceCrop, width, height, - minLayer, maxLayer, false, useIdentityTransform, rotation); + minLayer, maxLayer, false, useIdentityTransform, rotation, + false /* captureSecureLayers */); + } + + /** + * Like screenshotToBuffer, but if the caller is AID_SYSTEM, allows + * for the capture of secure layers. This is used for the screen rotation + * animation where the system server takes screenshots but does + * not persist them or allow them to leave the server. However in other + * cases in the system server, we mostly want to omit secure layers + * like when we take a screenshot on behalf of the assistant. + * + * @hide + */ + public static GraphicBuffer screenshotToBufferWithSecureLayersUnsafe(Rect sourceCrop, + int width, int height, int minLayer, int maxLayer, boolean useIdentityTransform, + int rotation) { + IBinder displayToken = SurfaceControl.getBuiltInDisplay( + SurfaceControl.BUILT_IN_DISPLAY_ID_MAIN); + return nativeScreenshotToBuffer(displayToken, sourceCrop, width, height, + minLayer, maxLayer, false, useIdentityTransform, rotation, + true /* captureSecureLayers */); } /** diff --git a/core/jni/android_view_SurfaceControl.cpp b/core/jni/android_view_SurfaceControl.cpp index 5b4b5f2a2264d..1529a6bcf243f 100644 --- a/core/jni/android_view_SurfaceControl.cpp +++ b/core/jni/android_view_SurfaceControl.cpp @@ -160,7 +160,7 @@ static Rect rectFromObj(JNIEnv* env, jobject rectObj) { static jobject nativeScreenshotToBuffer(JNIEnv* env, jclass clazz, jobject displayTokenObj, jobject sourceCropObj, jint width, jint height, jint minLayer, jint maxLayer, bool allLayers, bool useIdentityTransform, - int rotation) { + int rotation, bool captureSecureLayers) { sp displayToken = ibinderForJavaObject(env, displayTokenObj); if (displayToken == NULL) { return NULL; @@ -173,7 +173,7 @@ static jobject nativeScreenshotToBuffer(JNIEnv* env, jclass clazz, sp buffer; status_t res = ScreenshotClient::capture(displayToken, sourceCrop, width, height, minLayer, maxLayer, useIdentityTransform, - rotation, &buffer); + rotation, captureSecureLayers, &buffer); if (res != NO_ERROR) { return NULL; } @@ -1026,7 +1026,7 @@ static const JNINativeMethod sSurfaceControlMethods[] = { {"nativeGetHandle", "(J)Landroid/os/IBinder;", (void*)nativeGetHandle }, {"nativeScreenshotToBuffer", - "(Landroid/os/IBinder;Landroid/graphics/Rect;IIIIZZI)Landroid/graphics/GraphicBuffer;", + "(Landroid/os/IBinder;Landroid/graphics/Rect;IIIIZZIZ)Landroid/graphics/GraphicBuffer;", (void*)nativeScreenshotToBuffer }, {"nativeCaptureLayers", "(Landroid/os/IBinder;Landroid/graphics/Rect;F)Landroid/graphics/GraphicBuffer;", (void*)nativeCaptureLayers }, diff --git a/services/core/java/com/android/server/wm/ScreenRotationAnimation.java b/services/core/java/com/android/server/wm/ScreenRotationAnimation.java index 755a571cf5f75..498cda11c1b4d 100644 --- a/services/core/java/com/android/server/wm/ScreenRotationAnimation.java +++ b/services/core/java/com/android/server/wm/ScreenRotationAnimation.java @@ -27,6 +27,7 @@ import static com.android.server.wm.ScreenRotationAnimationProto.ANIMATION_RUNNI import static com.android.server.wm.ScreenRotationAnimationProto.STARTED; import android.content.Context; +import android.graphics.GraphicBuffer; import android.graphics.Matrix; import android.graphics.Rect; import android.os.IBinder; @@ -285,10 +286,21 @@ class ScreenRotationAnimation { if (displayHandle != null) { Surface sur = new Surface(); sur.copyFrom(mSurfaceControl); - SurfaceControl.screenshot(displayHandle, sur); - t.setLayer(mSurfaceControl, SCREEN_FREEZE_LAYER_SCREENSHOT); - t.setAlpha(mSurfaceControl, 0); - t.show(mSurfaceControl); + GraphicBuffer gb = SurfaceControl.screenshotToBufferWithSecureLayersUnsafe( + new Rect(), 0 /* width */, 0 /* height */, 0 /* minLayer */, + 0 /* maxLayer */, false /* useIdentityTransform */, 0 /* rotation */); + if (gb != null) { + try { + sur.attachAndQueueBuffer(gb); + } catch (RuntimeException e) { + Slog.w(TAG, "Failed to attach screenshot - " + e.getMessage()); + } + t.setLayer(mSurfaceControl, SCREEN_FREEZE_LAYER_SCREENSHOT); + t.setAlpha(mSurfaceControl, 0); + t.show(mSurfaceControl); + } else { + Slog.w(TAG, "Unable to take screenshot of display " + displayId); + } sur.destroy(); } else { Slog.w(TAG, "Built-in display " + displayId + " is null."); From 3256e59a87466b1a1d20a6cfe22617e23fa25299 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Thu, 18 Apr 2019 16:32:42 -0700 Subject: [PATCH 2/3] HwBlob: s/malloc/calloc/ Since this blob is passed between processes. We could potentially only memset portions of the blob as it is written to. However, the JHwBlob API itself doesn't have to have writes in order (even though known usages of it do write in order). Because of this, keeping track of which bytes to pad would be too expensive. Bug: 131356202 Test: boot, hidl_test_java Change-Id: I48f4d7cb20c4bfe747dd323ae3744d323ad097c9 Merged-In: I48f4d7cb20c4bfe747dd323ae3744d323ad097c9 (cherry picked from commit d8157bc094569bee74976df2585d632f1793e226) --- core/jni/android_os_HwBlob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/jni/android_os_HwBlob.cpp b/core/jni/android_os_HwBlob.cpp index bb916d2431c5b..09589cdb08011 100644 --- a/core/jni/android_os_HwBlob.cpp +++ b/core/jni/android_os_HwBlob.cpp @@ -85,7 +85,7 @@ JHwBlob::JHwBlob(JNIEnv *env, jobject thiz, size_t size) mOwnsBuffer(true), mHandle(0) { if (size > 0) { - mBuffer = malloc(size); + mBuffer = calloc(size, 1); } } From da7203b66876a680fad56a5aafe3d84ae8354d4f Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 2 Apr 2019 14:18:56 -0700 Subject: [PATCH 3/3] [RESTRICT AUTOMERGE] 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: I0dd36462867da52e6b1451f65f56c2c5d37538f3 (cherry picked from commit bab740f10e0812ba47d19931fdfe2fa7e02bbd0c) --- core/jni/android_view_SurfaceControl.cpp | 8 ++-- .../java/android/graphics/GraphicBuffer.java | 37 +++++++++++++++++-- .../server/wm/ScreenRotationAnimation.java | 6 +++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/core/jni/android_view_SurfaceControl.cpp b/core/jni/android_view_SurfaceControl.cpp index 1529a6bcf243f..614a8ff124ea1 100644 --- a/core/jni/android_view_SurfaceControl.cpp +++ b/core/jni/android_view_SurfaceControl.cpp @@ -171,9 +171,10 @@ static jobject nativeScreenshotToBuffer(JNIEnv* env, jclass clazz, maxLayer = INT32_MAX; } sp buffer; + bool capturedSecureLayers = false; status_t res = ScreenshotClient::capture(displayToken, sourceCrop, width, height, minLayer, maxLayer, useIdentityTransform, - rotation, captureSecureLayers, &buffer); + rotation, captureSecureLayers, &buffer, capturedSecureLayers); if (res != NO_ERROR) { return NULL; } @@ -184,7 +185,8 @@ static jobject nativeScreenshotToBuffer(JNIEnv* env, jclass clazz, buffer->getHeight(), buffer->getPixelFormat(), (jint)buffer->getUsage(), - (jlong)buffer.get()); + (jlong)buffer.get(), + capturedSecureLayers); } static jobject nativeScreenshotBitmap(JNIEnv* env, jclass clazz, @@ -1082,7 +1084,7 @@ int register_android_view_SurfaceControl(JNIEnv* env) jclass graphicsBufferClazz = FindClassOrDie(env, "android/graphics/GraphicBuffer"); gGraphicBufferClassInfo.clazz = MakeGlobalRefOrDie(env, graphicsBufferClazz); gGraphicBufferClassInfo.builder = GetStaticMethodIDOrDie(env, graphicsBufferClazz, - "createFromExisting", "(IIIIJ)Landroid/graphics/GraphicBuffer;"); + "createFromExisting", "(IIIIJZ)Landroid/graphics/GraphicBuffer;"); return err; } diff --git a/graphics/java/android/graphics/GraphicBuffer.java b/graphics/java/android/graphics/GraphicBuffer.java index 53d21776eed7e..61dd37fb0581a 100644 --- a/graphics/java/android/graphics/GraphicBuffer.java +++ b/graphics/java/android/graphics/GraphicBuffer.java @@ -52,6 +52,7 @@ public class GraphicBuffer implements Parcelable { private final int mHeight; private final int mFormat; private final int mUsage; + private final boolean mCapturedSecureLayers; // Note: do not rename, this field is used by native code private final long mNativeObject; @@ -82,14 +83,23 @@ public class GraphicBuffer implements Parcelable { } /** - * Private use only. See {@link #create(int, int, int, int)}. + * Private use only. See {@link #create(int, int, int, int, boolean)}. */ - private GraphicBuffer(int width, int height, int format, int usage, long nativeObject) { + private GraphicBuffer(int width, int height, int format, int usage, long nativeObject, + boolean capturedSecureLayers) { mWidth = width; mHeight = height; mFormat = format; mUsage = usage; mNativeObject = nativeObject; + mCapturedSecureLayers = capturedSecureLayers; + } + + /** + * Private use only. See {@link #create(int, int, int, int)}. + */ + private GraphicBuffer(int width, int height, int format, int usage, long nativeObject) { + this(width, height, format, usage, nativeObject, false); } /** @@ -97,14 +107,33 @@ public class GraphicBuffer implements Parcelable { * @hide */ public static GraphicBuffer createFromExisting(int width, int height, - int format, int usage, long unwrappedNativeObject) { + int format, int usage, long unwrappedNativeObject, + boolean capturedSecureLayers) { long nativeObject = nWrapGraphicBuffer(unwrappedNativeObject); if (nativeObject != 0) { - return new GraphicBuffer(width, height, format, usage, nativeObject); + return new GraphicBuffer(width, height, format, usage, nativeObject, + capturedSecureLayers); } return null; } + /** + * For SurfaceControl JNI. Provides and ignored value for capturedSecureLayers for backwards + * compatibility + * @hide + */ + public static GraphicBuffer createFromExisting(int width, int height, + int format, int usage, long unwrappedNativeObject) { + return createFromExisting(width, height, format, usage, unwrappedNativeObject, false); + } + + /** + * Returns true if the buffer contains visible secure layers. + */ + public boolean doesContainSecureLayers() { + return mCapturedSecureLayers; + } + /** * Returns the width of this buffer in pixels. */ diff --git a/services/core/java/com/android/server/wm/ScreenRotationAnimation.java b/services/core/java/com/android/server/wm/ScreenRotationAnimation.java index 498cda11c1b4d..95051dea2e9a3 100644 --- a/services/core/java/com/android/server/wm/ScreenRotationAnimation.java +++ b/services/core/java/com/android/server/wm/ScreenRotationAnimation.java @@ -295,6 +295,12 @@ class ScreenRotationAnimation { } 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.doesContainSecureLayers()) { + t.setSecure(mSurfaceControl, true); + } t.setLayer(mSurfaceControl, SCREEN_FREEZE_LAYER_SCREENSHOT); t.setAlpha(mSurfaceControl, 0); t.show(mSurfaceControl);