Revert "Revert "Revert "SF: Updating permissions checking in Surface Flinger."""
This reverts commit fbe2c00188533f459c6c675f19c1953d48429d02.
Reason for revert: Breaks Netflix/YouTube TV
Bug: 112758347
Change-Id: Ia1eba75d3ea8fecec6855f2ab83c23e0b55b63d4
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index 98ec338..e401572 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -220,12 +220,12 @@
class BnSurfaceComposer: public BnInterface<ISurfaceComposer> {
public:
- enum ISurfaceComposerTag {
+ enum {
// Note: BOOT_FINISHED must remain this value, it is called from
// Java by ActivityManagerService.
BOOT_FINISHED = IBinder::FIRST_CALL_TRANSACTION,
CREATE_CONNECTION,
- CREATE_GRAPHIC_BUFFER_ALLOC_UNUSED, // unused, fails permissions check
+ UNUSED, // formerly CREATE_GRAPHIC_BUFFER_ALLOC
CREATE_DISPLAY_EVENT_CONNECTION,
CREATE_DISPLAY,
DESTROY_DISPLAY,
@@ -236,7 +236,7 @@
GET_DISPLAY_CONFIGS,
GET_ACTIVE_CONFIG,
SET_ACTIVE_CONFIG,
- CONNECT_DISPLAY_UNUSED, // unused, fails permissions check
+ CONNECT_DISPLAY,
CAPTURE_SCREEN,
CAPTURE_LAYERS,
CLEAR_ANIMATION_FRAME_STATS,
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 76f6d4d..6b222d5 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1220,6 +1220,15 @@
status_t SurfaceFlinger::getLayerDebugInfo(std::vector<LayerDebugInfo>* outLayers) const
NO_THREAD_SAFETY_ANALYSIS {
+ IPCThreadState* ipc = IPCThreadState::self();
+ const int pid = ipc->getCallingPid();
+ const int uid = ipc->getCallingUid();
+ if ((uid != AID_SHELL) &&
+ !PermissionCache::checkPermission(sDump, pid, uid)) {
+ ALOGE("Layer debug info permission denied for pid=%d, uid=%d", pid, uid);
+ return PERMISSION_DENIED;
+ }
+
// Try to acquire a lock for 1s, fail gracefully
const status_t err = mStateLock.timedLock(s2ns(1));
const bool locked = (err == NO_ERROR);
@@ -3366,8 +3375,9 @@
IPCThreadState* ipc = IPCThreadState::self();
const int pid = ipc->getCallingPid();
const int uid = ipc->getCallingUid();
+
if ((uid != AID_GRAPHICS && uid != AID_SYSTEM) &&
- !PermissionCache::checkPermission(sAccessSurfaceFlinger, pid, uid)) {
+ !PermissionCache::checkPermission(sAccessSurfaceFlinger, pid, uid)) {
return false;
}
return true;
@@ -4553,64 +4563,51 @@
}
status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) {
-#pragma clang diagnostic push
-#pragma clang diagnostic error "-Wswitch-enum"
- switch (static_cast<ISurfaceComposerTag>(code)) {
- // These methods should at minimum make sure that the client requested
- // access to SF.
- case AUTHENTICATE_SURFACE:
- case BOOT_FINISHED:
- case CLEAR_ANIMATION_FRAME_STATS:
+ switch (code) {
case CREATE_CONNECTION:
case CREATE_DISPLAY:
- case DESTROY_DISPLAY:
- case ENABLE_VSYNC_INJECTIONS:
- case GET_ACTIVE_COLOR_MODE:
+ case BOOT_FINISHED:
+ case CLEAR_ANIMATION_FRAME_STATS:
case GET_ANIMATION_FRAME_STATS:
+ case SET_POWER_MODE:
case GET_HDR_CAPABILITIES:
- case SET_ACTIVE_CONFIG:
- case SET_ACTIVE_COLOR_MODE:
+ case ENABLE_VSYNC_INJECTIONS:
case INJECT_VSYNC:
- case SET_POWER_MODE: {
+ {
+ // codes that require permission check
if (!callingThreadHasUnscopedSurfaceFlingerAccess()) {
IPCThreadState* ipc = IPCThreadState::self();
ALOGE("Permission Denial: can't access SurfaceFlinger pid=%d, uid=%d",
ipc->getCallingPid(), ipc->getCallingUid());
return PERMISSION_DENIED;
}
+ break;
+ }
+ /*
+ * Calling setTransactionState is safe, because you need to have been
+ * granted a reference to Client* and Handle* to do anything with it.
+ *
+ * Creating a scoped connection is safe, as per discussion in ISurfaceComposer.h
+ */
+ case SET_TRANSACTION_STATE:
+ case CREATE_SCOPED_CONNECTION:
+ {
return OK;
}
- case GET_LAYER_DEBUG_INFO: {
+ case CAPTURE_SCREEN:
+ {
+ // codes that require permission check
IPCThreadState* ipc = IPCThreadState::self();
const int pid = ipc->getCallingPid();
const int uid = ipc->getCallingUid();
- if ((uid != AID_SHELL) && !PermissionCache::checkPermission(sDump, pid, uid)) {
- ALOGE("Layer debug info permission denied for pid=%d, uid=%d", pid, uid);
+ if ((uid != AID_GRAPHICS) &&
+ !PermissionCache::checkPermission(sReadFramebuffer, pid, uid)) {
+ ALOGE("Permission Denial: can't read framebuffer pid=%d, uid=%d", pid, uid);
return PERMISSION_DENIED;
}
- return OK;
+ break;
}
- // Used by apps to hook Choreographer to SurfaceFlinger.
- case CREATE_DISPLAY_EVENT_CONNECTION:
- // The following calls are currently used by clients that do not
- // request necessary permissions. However, they do not expose any secret
- // information, so it is OK to pass them.
- case GET_ACTIVE_CONFIG:
- case GET_BUILT_IN_DISPLAY:
- case GET_DISPLAY_COLOR_MODES:
- case GET_DISPLAY_CONFIGS:
- case GET_DISPLAY_STATS:
- case GET_SUPPORTED_FRAME_TIMESTAMPS:
- // Calling setTransactionState is safe, because you need to have been
- // granted a reference to Client* and Handle* to do anything with it.
- case SET_TRANSACTION_STATE:
- // Creating a scoped connection is safe, as per discussion in ISurfaceComposer.h
- case CREATE_SCOPED_CONNECTION: {
- return OK;
- }
- case CAPTURE_LAYERS:
- case CAPTURE_SCREEN: {
- // codes that require permission check
+ case CAPTURE_LAYERS: {
IPCThreadState* ipc = IPCThreadState::self();
const int pid = ipc->getCallingPid();
const int uid = ipc->getCallingUid();
@@ -4619,37 +4616,15 @@
ALOGE("Permission Denial: can't read framebuffer pid=%d, uid=%d", pid, uid);
return PERMISSION_DENIED;
}
- return OK;
- }
- // The following codes are deprecated and should never be allowed to access SF.
- case CONNECT_DISPLAY_UNUSED:
- case CREATE_GRAPHIC_BUFFER_ALLOC_UNUSED: {
- ALOGE("Attempting to access SurfaceFlinger with unused code: %u", code);
- return PERMISSION_DENIED;
+ break;
}
}
-
- // These codes are used for the IBinder protocol to either interrogate the recipient
- // side of the transaction for its canonical interface descriptor or to dump its state.
- // We let them pass by default.
- if (code == IBinder::INTERFACE_TRANSACTION || code == IBinder::DUMP_TRANSACTION ||
- code == IBinder::PING_TRANSACTION || code == IBinder::SHELL_COMMAND_TRANSACTION ||
- code == IBinder::SYSPROPS_TRANSACTION) {
- return OK;
- }
- // Numbers from 1000 to 1029 are currently use for backdoors. The code
- // in onTransact verifies that the user is root, and has access to use SF.
- if (code >= 1000 && code <= 1029) {
- ALOGV("Accessing SurfaceFlinger through backdoor code: %u", code);
- return OK;
- }
- ALOGE("Permission Denial: SurfaceFlinger did not recognize request code: %u", code);
- return PERMISSION_DENIED;
-#pragma clang diagnostic pop
+ return OK;
}
-status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* reply,
- uint32_t flags) {
+status_t SurfaceFlinger::onTransact(
+ uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags)
+{
status_t credentialCheck = CheckTransactCodeCredentials(code);
if (credentialCheck != OK) {
return credentialCheck;
diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp
index 604aa7d..c511c5e 100644
--- a/services/surfaceflinger/tests/Android.bp
+++ b/services/surfaceflinger/tests/Android.bp
@@ -17,7 +17,6 @@
defaults: ["surfaceflinger_defaults"],
test_suites: ["device-tests"],
srcs: [
- "Credentials_test.cpp",
"Stress_test.cpp",
"SurfaceInterceptor_test.cpp",
"Transaction_test.cpp",
diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp
deleted file mode 100644
index cd57411..0000000
--- a/services/surfaceflinger/tests/Credentials_test.cpp
+++ /dev/null
@@ -1,322 +0,0 @@
-#include <algorithm>
-#include <functional>
-#include <limits>
-#include <ostream>
-
-#include <gtest/gtest.h>
-
-#include <gui/ISurfaceComposer.h>
-#include <gui/LayerDebugInfo.h>
-#include <gui/Surface.h>
-#include <gui/SurfaceComposerClient.h>
-
-#include <private/android_filesystem_config.h>
-#include <private/gui/ComposerService.h>
-
-#include <ui/DisplayInfo.h>
-#include <utils/String8.h>
-
-namespace android {
-
-using Transaction = SurfaceComposerClient::Transaction;
-
-namespace {
-const String8 DISPLAY_NAME("Credentials Display Test");
-const String8 SURFACE_NAME("Test Surface Name");
-const int32_t MIN_LAYER_Z = 0;
-const int32_t MAX_LAYER_Z = std::numeric_limits<int32_t>::max();
-const uint32_t ROTATION = 0;
-const float FRAME_SCALE = 1.0f;
-} // namespace
-
-/**
- * This class tests the CheckCredentials method in SurfaceFlinger.
- * Methods like EnableVsyncInjections and InjectVsync are not tested since they do not
- * return anything meaningful.
- */
-class CredentialsTest : public ::testing::Test {
-protected:
- void SetUp() override {
- // Start the tests as root.
- seteuid(AID_ROOT);
-
- ASSERT_NO_FATAL_FAILURE(initClient());
- }
-
- void TearDown() override {
- mComposerClient->dispose();
- mBGSurfaceControl.clear();
- mComposerClient.clear();
- // Finish the tests as root.
- seteuid(AID_ROOT);
- }
-
- sp<IBinder> mDisplay;
- sp<IBinder> mVirtualDisplay;
- sp<SurfaceComposerClient> mComposerClient;
- sp<SurfaceControl> mBGSurfaceControl;
- sp<SurfaceControl> mVirtualSurfaceControl;
-
- void initClient() {
- mComposerClient = new SurfaceComposerClient;
- ASSERT_EQ(NO_ERROR, mComposerClient->initCheck());
- }
-
- void setupBackgroundSurface() {
- mDisplay = SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain);
- DisplayInfo info;
- SurfaceComposerClient::getDisplayInfo(mDisplay, &info);
- const ssize_t displayWidth = info.w;
- const ssize_t displayHeight = info.h;
-
- // Background surface
- mBGSurfaceControl =
- mComposerClient->createSurface(SURFACE_NAME, displayWidth, displayHeight,
- PIXEL_FORMAT_RGBA_8888, 0);
- ASSERT_TRUE(mBGSurfaceControl != nullptr);
- ASSERT_TRUE(mBGSurfaceControl->isValid());
-
- Transaction t;
- t.setDisplayLayerStack(mDisplay, 0);
- ASSERT_EQ(NO_ERROR,
- 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.
- */
- void setGraphicsUID() {
- seteuid(AID_ROOT);
- seteuid(AID_GRAPHICS);
- }
-
- /**
- * Sets UID to imitate System's process.
- */
- void setSystemUID() {
- seteuid(AID_ROOT);
- seteuid(AID_SYSTEM);
- }
-
- /**
- * Sets UID to imitate a process that doesn't have any special privileges in
- * our code.
- */
- void setBinUID() {
- seteuid(AID_ROOT);
- seteuid(AID_BIN);
- }
-
- /**
- * Template function the check a condition for different types of users: root
- * graphics, system, and non-supported user. Root, graphics, and system should
- * always equal privilegedValue, and non-supported user should equal unprivilegedValue.
- */
- template <typename T>
- void checkWithPrivileges(std::function<T()> condition, T privilegedValue, T unprivilegedValue) {
- // Check with root.
- seteuid(AID_ROOT);
- ASSERT_EQ(privilegedValue, condition());
-
- // Check as a Graphics user.
- setGraphicsUID();
- ASSERT_EQ(privilegedValue, condition());
-
- // Check as a system user.
- setSystemUID();
- ASSERT_EQ(privilegedValue, condition());
-
- // Check as a non-supported user.
- setBinUID();
- ASSERT_EQ(unprivilegedValue, condition());
- }
-};
-
-TEST_F(CredentialsTest, ClientInitTest) {
- // Root can init can init the client.
- ASSERT_NO_FATAL_FAILURE(initClient());
-
- // Graphics can init the client.
- setGraphicsUID();
- ASSERT_NO_FATAL_FAILURE(initClient());
-
- // System can init the client.
- setSystemUID();
- ASSERT_NO_FATAL_FAILURE(initClient());
-
- // No one else can init the client.
- setBinUID();
- mComposerClient = new SurfaceComposerClient;
- ASSERT_EQ(NO_INIT, mComposerClient->initCheck());
-}
-
-TEST_F(CredentialsTest, GetBuiltInDisplayAccessTest) {
- std::function<bool()> condition = [=]() {
- sp<IBinder> display(
- SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- return (display != nullptr);
- };
- // Anyone can access display information.
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, true));
-}
-
-TEST_F(CredentialsTest, AllowedGetterMethodsTest) {
- // The following methods are tested with a UID that is not root, graphics,
- // or system, to show that anyone can access them.
- setBinUID();
- sp<IBinder> display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- ASSERT_TRUE(display != nullptr);
-
- DisplayInfo info;
- ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayInfo(display, &info));
-
- Vector<DisplayInfo> configs;
- ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayConfigs(display, &configs));
-
- ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getActiveConfig(display));
-
- ASSERT_NE(static_cast<ui::ColorMode>(BAD_VALUE),
- SurfaceComposerClient::getActiveColorMode(display));
-}
-
-TEST_F(CredentialsTest, GetDisplayColorModesTest) {
- sp<IBinder> display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- std::function<status_t()> condition = [=]() {
- Vector<ui::ColorMode> outColorModes;
- return SurfaceComposerClient::getDisplayColorModes(display, &outColorModes);
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges<status_t>(condition, NO_ERROR, NO_ERROR));
-}
-
-TEST_F(CredentialsTest, SetActiveConfigTest) {
- sp<IBinder> display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- std::function<status_t()> condition = [=]() {
- return SurfaceComposerClient::setActiveConfig(display, 0);
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges<status_t>(condition, NO_ERROR, PERMISSION_DENIED));
-}
-
-TEST_F(CredentialsTest, SetActiveColorModeTest) {
- sp<IBinder> display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- std::function<status_t()> condition = [=]() {
- return SurfaceComposerClient::setActiveColorMode(display, ui::ColorMode::NATIVE);
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges<status_t>(condition, NO_ERROR, PERMISSION_DENIED));
-}
-
-TEST_F(CredentialsTest, CreateSurfaceTest) {
- sp<IBinder> display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- DisplayInfo info;
- SurfaceComposerClient::getDisplayInfo(display, &info);
- const ssize_t displayWidth = info.w;
- const ssize_t displayHeight = info.h;
-
- std::function<bool()> condition = [=]() {
- mBGSurfaceControl =
- mComposerClient->createSurface(SURFACE_NAME, displayWidth, displayHeight,
- PIXEL_FORMAT_RGBA_8888, 0);
- return mBGSurfaceControl != nullptr && mBGSurfaceControl->isValid();
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false));
-}
-
-TEST_F(CredentialsTest, CreateDisplayTest) {
- std::function<bool()> condition = [=]() {
- sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true);
- return testDisplay.get() != nullptr;
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false));
-
- condition = [=]() {
- sp<IBinder> testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false);
- return testDisplay.get() != nullptr;
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false));
-}
-
-TEST_F(CredentialsTest, DISABLED_DestroyDisplayTest) {
- setupVirtualDisplay();
-
- DisplayInfo info;
- ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayInfo(mVirtualDisplay, &info));
- SurfaceComposerClient::destroyDisplay(mVirtualDisplay);
- // This test currently fails. TODO(b/112002626): Find a way to properly create
- // a display in the test environment, so that destroy display can remove it.
- ASSERT_EQ(NAME_NOT_FOUND, SurfaceComposerClient::getDisplayInfo(mVirtualDisplay, &info));
-}
-
-TEST_F(CredentialsTest, CaptureTest) {
- sp<IBinder> display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain));
- std::function<status_t()> condition = [=]() {
- sp<GraphicBuffer> outBuffer;
- return ScreenshotClient::capture(display, Rect(), 0 /*reqWidth*/, 0 /*reqHeight*/,
- MIN_LAYER_Z, MAX_LAYER_Z, false, ROTATION, &outBuffer);
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges<status_t>(condition, NO_ERROR, PERMISSION_DENIED));
-}
-
-TEST_F(CredentialsTest, CaptureLayersTest) {
- setupBackgroundSurface();
- sp<GraphicBuffer> outBuffer;
- std::function<status_t()> condition = [=]() {
- sp<GraphicBuffer> outBuffer;
- return ScreenshotClient::captureLayers(mBGSurfaceControl->getHandle(), Rect(), FRAME_SCALE,
- &outBuffer);
- };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges<status_t>(condition, NO_ERROR, PERMISSION_DENIED));
-}
-
-/**
- * Test for methods accessible directly through SurfaceFlinger:
- */
-TEST_F(CredentialsTest, AuthenticateSurfaceTextureTest) {
- setupBackgroundSurface();
- sp<IGraphicBufferProducer> producer =
- mBGSurfaceControl->getSurface()->getIGraphicBufferProducer();
- sp<ISurfaceComposer> sf(ComposerService::getComposerService());
-
- std::function<bool()> condition = [=]() { return sf->authenticateSurfaceTexture(producer); };
- ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false));
-}
-
-TEST_F(CredentialsTest, GetLayerDebugInfo) {
- setupBackgroundSurface();
- sp<ISurfaceComposer> sf(ComposerService::getComposerService());
-
- // Historically, only root and shell can access the getLayerDebugInfo which
- // is called when we call dumpsys. I don't see a reason why we should change this.
- std::vector<LayerDebugInfo> outLayers;
- // Check with root.
- seteuid(AID_ROOT);
- ASSERT_EQ(NO_ERROR, sf->getLayerDebugInfo(&outLayers));
-
- // Check as a shell.
- seteuid(AID_SHELL);
- ASSERT_EQ(NO_ERROR, sf->getLayerDebugInfo(&outLayers));
-
- // Check as anyone else.
- seteuid(AID_ROOT);
- seteuid(AID_BIN);
- ASSERT_EQ(PERMISSION_DENIED, sf->getLayerDebugInfo(&outLayers));
-}
-} // namespace android
diff --git a/services/surfaceflinger/tests/SurfaceFlinger_test.filter b/services/surfaceflinger/tests/SurfaceFlinger_test.filter
index 1319e12..731e628 100644
--- a/services/surfaceflinger/tests/SurfaceFlinger_test.filter
+++ b/services/surfaceflinger/tests/SurfaceFlinger_test.filter
@@ -1,5 +1,5 @@
{
"presubmit": {
- "filter": "CredentialsTest.*:LayerTransactionTest.*:LayerUpdateTest.*:ChildLayerTest.*:SurfaceFlingerStress.*:CropLatchingTest.*:GeometryLatchingTest.*:ScreenCaptureTest.*:DereferenceSurfaceControlTest.*:SurfaceInterceptorTest.*:-CropLatchingTest.FinalCropLatchingBufferOldSize"
+ "filter": "LayerTransactionTest.*:LayerUpdateTest.*:ChildLayerTest.*:SurfaceFlingerStress.*:CropLatchingTest.*:GeometryLatchingTest.*:ScreenCaptureTest.*:DereferenceSurfaceControlTest.*:SurfaceInterceptorTest.*:-CropLatchingTest.FinalCropLatchingBufferOldSize"
}
}