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
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index 0ac493d..c7c75da 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -244,10 +244,25 @@
{
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 @@
}
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: {
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9d35a3f..819f3e2 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -509,6 +509,15 @@
}
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() {
diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp
index e2c038d..7f541e2 100644
--- a/services/surfaceflinger/tests/Credentials_test.cpp
+++ b/services/surfaceflinger/tests/Credentials_test.cpp
@@ -98,26 +98,6 @@
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 @@
// 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, 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);
diff --git a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp
index 8d97f27..8570032 100644
--- a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp
+++ b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp
@@ -422,7 +422,7 @@
}
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::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) {
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp b/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
index b939b9a..049264e 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
@@ -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 @@
// --------------------------------------------------------------------
// 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