commit d4a61643bf032551c945c409b4572efd29223220
author chaviw <chaviw@google.com> Tue Sep 01 14:53:46 2020 -0700
committer chaviw <chaviw@google.com> Tue Sep 15 11:44:46 2020 -0700
tree 8a1a7b1c01e989dbf77996caf9293db6da632a7f
parent 934e82a98dd5a528bef61e035db412963f9ab5f3

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

services/surfaceflinger/tests/Credentials_test.cpp

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);